All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/i915: Serialize GTT Updates on BXT
@ 2017-05-22 18:07 Jon Bloomfield
  2017-05-22 18:37 ` ✓ Fi.CI.BAT: success for " Patchwork
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Jon Bloomfield @ 2017-05-22 18:07 UTC (permalink / raw)
  To: intel-gfx

BXT requires accesses to the GTT (i.e. PTE updates) to be serialized
when IOMMU is enabled. This patch guarantees this by wrapping all
updates in stop_machine and using a flushing read to guarantee that
the GTT writes have reached their destination before restarting.

Signed-off-by: Jon Bloomfield <jon.bloomfield@intel.com>
Signed-off-by: John Harrison <john.C.Harrison@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 106 ++++++++++++++++++++++++++++++++++++
 1 file changed, 106 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 7c769d7..6360d92 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2191,6 +2191,100 @@ static void gen8_ggtt_clear_range(struct i915_address_space *vm,
 		gen8_set_pte(&gtt_base[i], scratch_pte);
 }
 
+#ifdef CONFIG_INTEL_IOMMU
+struct insert_page {
+	struct i915_address_space *vm;
+	dma_addr_t addr;
+	u64 offset;
+	enum i915_cache_level level;
+};
+
+static int gen8_ggtt_insert_page__cb(void *_arg)
+{
+	struct insert_page *arg = _arg;
+
+	struct drm_i915_private *dev_priv = arg->vm->i915;
+
+	gen8_ggtt_insert_page(arg->vm, arg->addr,
+				arg->offset, arg->level, 0);
+
+	POSTING_READ(GFX_FLSH_CNTL_GEN6);
+
+	return 0;
+}
+
+static void gen8_ggtt_insert_page__BKL(struct i915_address_space *vm,
+				       dma_addr_t addr,
+				       u64 offset,
+				       enum i915_cache_level level,
+				       u32 unused)
+{
+	struct insert_page arg = { vm, addr, offset, level };
+
+	stop_machine(gen8_ggtt_insert_page__cb, &arg, NULL);
+}
+
+
+struct insert_entries {
+	struct i915_address_space *vm;
+	struct sg_table *st;
+	u64 start;
+	enum i915_cache_level level;
+};
+
+static int gen8_ggtt_insert_entries__cb(void *_arg)
+{
+	struct insert_entries *arg = _arg;
+
+	struct drm_i915_private *dev_priv = arg->vm->i915;
+
+	gen8_ggtt_insert_entries(arg->vm, arg->st,
+					arg->start, arg->level, 0);
+
+	POSTING_READ(GFX_FLSH_CNTL_GEN6);
+
+	return 0;
+}
+
+static void gen8_ggtt_insert_entries__BKL(struct i915_address_space *vm,
+					  struct sg_table *st,
+					  u64 start,
+					  enum i915_cache_level level,
+					  u32 unused)
+{
+	struct insert_entries arg = { vm, st, start, level };
+
+	stop_machine(gen8_ggtt_insert_entries__cb, &arg, NULL);
+}
+
+struct clear_range {
+	struct i915_address_space *vm;
+	u64 start;
+	u64 length;
+};
+
+static int gen8_ggtt_clear_range__cb(void *_arg)
+{
+	struct clear_range *arg = _arg;
+
+	struct drm_i915_private *dev_priv = arg->vm->i915;
+
+	gen8_ggtt_clear_range(arg->vm, arg->start, arg->length);
+
+	POSTING_READ(GFX_FLSH_CNTL_GEN6);
+
+	return 0;
+}
+
+static void gen8_ggtt_clear_range__BKL(struct i915_address_space *vm,
+                                       u64 start,
+                                       u64 length)
+{
+	struct clear_range arg = { vm, start, length };
+	stop_machine(gen8_ggtt_clear_range__cb, &arg, NULL);
+}
+#endif
+
 static void gen6_ggtt_clear_range(struct i915_address_space *vm,
 				  u64 start, u64 length)
 {
@@ -2789,6 +2883,18 @@ static int gen8_gmch_probe(struct i915_ggtt *ggtt)
 
 	ggtt->base.insert_entries = gen8_ggtt_insert_entries;
 
+#ifdef CONFIG_INTEL_IOMMU
+	/* Serialize GTT updates on BXT if VT-d is on. */
+	if (IS_BROXTON(dev_priv) && intel_iommu_gfx_mapped) {
+		ggtt->base.insert_entries = gen8_ggtt_insert_entries__BKL;
+		ggtt->base.insert_page    = gen8_ggtt_insert_page__BKL;
+		if (!USES_FULL_PPGTT(dev_priv) ||
+		    intel_scanout_needs_vtd_wa(dev_priv)) {
+			ggtt->base.clear_range = gen8_ggtt_clear_range__BKL;
+		}
+	}
+#endif
+
 	ggtt->invalidate = gen6_ggtt_invalidate;
 
 	return ggtt_probe_common(ggtt, size);
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* ✓ Fi.CI.BAT: success for drm/i915: Serialize GTT Updates on BXT
  2017-05-22 18:07 [PATCH] drm/i915: Serialize GTT Updates on BXT Jon Bloomfield
@ 2017-05-22 18:37 ` Patchwork
  2017-05-22 20:05 ` [PATCH] " Chris Wilson
  2017-05-23  8:40 ` Chris Wilson
  2 siblings, 0 replies; 9+ messages in thread
From: Patchwork @ 2017-05-22 18:37 UTC (permalink / raw)
  To: Bloomfield, Jon; +Cc: intel-gfx

== Series Details ==

Series: drm/i915: Serialize GTT Updates on BXT
URL   : https://patchwork.freedesktop.org/series/24793/
State : success

== Summary ==

Series 24793v1 drm/i915: Serialize GTT Updates on BXT
https://patchwork.freedesktop.org/api/1.0/series/24793/revisions/1/mbox/

Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-uc:
                pass       -> FAIL       (fi-snb-2600) fdo#100007
Test kms_flip:
        Subgroup basic-flip-vs-dpms:
                pass       -> DMESG-WARN (fi-skl-6700hq) fdo#101144
        Subgroup basic-flip-vs-wf_vblank:
                fail       -> PASS       (fi-skl-6770hq) fdo#99739

fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007
fdo#101144 https://bugs.freedesktop.org/show_bug.cgi?id=101144
fdo#99739 https://bugs.freedesktop.org/show_bug.cgi?id=99739

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time:447s
fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time:440s
fi-bsw-n3050     total:278  pass:242  dwarn:0   dfail:0   fail:0   skip:36  time:585s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time:512s
fi-byt-j1900     total:278  pass:254  dwarn:0   dfail:0   fail:0   skip:24  time:495s
fi-byt-n2820     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:488s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:419s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:415s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time:419s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:498s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:462s
fi-kbl-7500u     total:278  pass:255  dwarn:5   dfail:0   fail:0   skip:18  time:465s
fi-kbl-7560u     total:278  pass:263  dwarn:5   dfail:0   fail:0   skip:10  time:570s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:458s
fi-skl-6700hq    total:278  pass:260  dwarn:1   dfail:0   fail:0   skip:17  time:574s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time:465s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:505s
fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time:436s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:538s
fi-snb-2600      total:278  pass:248  dwarn:0   dfail:0   fail:1   skip:29  time:414s

5c5ce2ab8183e4bd85a603169e260cc938837e2c drm-tip: 2017y-05m-22d-13h-42m-03s UTC integration manifest
1d67986 drm/i915: Serialize GTT Updates on BXT

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4775/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] drm/i915: Serialize GTT Updates on BXT
  2017-05-22 18:07 [PATCH] drm/i915: Serialize GTT Updates on BXT Jon Bloomfield
  2017-05-22 18:37 ` ✓ Fi.CI.BAT: success for " Patchwork
@ 2017-05-22 20:05 ` Chris Wilson
  2017-05-22 22:18   ` Bloomfield, Jon
  2017-05-23  8:40 ` Chris Wilson
  2 siblings, 1 reply; 9+ messages in thread
From: Chris Wilson @ 2017-05-22 20:05 UTC (permalink / raw)
  To: Jon Bloomfield; +Cc: intel-gfx

On Mon, May 22, 2017 at 11:07:25AM -0700, Jon Bloomfield wrote:
> BXT requires accesses to the GTT (i.e. PTE updates) to be serialized
> when IOMMU is enabled.

Serialised with what, since all writes are serialized already?

The reason is that you need to explain the hw model you are protecting,
for example do clears need to be protected?

> This patch guarantees this by wrapping all
> updates in stop_machine and using a flushing read to guarantee that
> the GTT writes have reached their destination before restarting.

If you mention which patch you are reinstating (for a new problem) and
cc the author, he might point out what has changed in the meantime.

Note, the flush here is not about ensuring the GTT writes reach their
destination.
 
> Signed-off-by: Jon Bloomfield <jon.bloomfield@intel.com>

If you are the author and sender, what is John's s-o-b doing afterwards?

> Signed-off-by: John Harrison <john.C.Harrison@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 106 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 106 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 7c769d7..6360d92 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2191,6 +2191,100 @@ static void gen8_ggtt_clear_range(struct i915_address_space *vm,
>  		gen8_set_pte(&gtt_base[i], scratch_pte);
>  }
>  
> +#ifdef CONFIG_INTEL_IOMMU
> +struct insert_page {
> +	struct i915_address_space *vm;
> +	dma_addr_t addr;
> +	u64 offset;
> +	enum i915_cache_level level;
> +};
> +
> +static int gen8_ggtt_insert_page__cb(void *_arg)
> +{
> +	struct insert_page *arg = _arg;
> +
> +	struct drm_i915_private *dev_priv = arg->vm->i915;
> +
> +	gen8_ggtt_insert_page(arg->vm, arg->addr,
> +				arg->offset, arg->level, 0);
> +
> +	POSTING_READ(GFX_FLSH_CNTL_GEN6);

This is now just a call to i915_ggtt_invalidate() because we are now
also responsible for invalidating the guc tlbs as well as the chipset.
And more importantly it is already done by gen8_ggtt_insert_page.

All the POSTING_READ(GFX_FLSH_CNTL_GEN6) are spurious.

>  static void gen6_ggtt_clear_range(struct i915_address_space *vm,
>  				  u64 start, u64 length)
>  {
> @@ -2789,6 +2883,18 @@ static int gen8_gmch_probe(struct i915_ggtt *ggtt)
>  
>  	ggtt->base.insert_entries = gen8_ggtt_insert_entries;
>  
> +#ifdef CONFIG_INTEL_IOMMU
> +	/* Serialize GTT updates on BXT if VT-d is on. */
> +	if (IS_BROXTON(dev_priv) && intel_iommu_gfx_mapped) {

Move to a header and don't ifdef out the users. A small cost in object
side for the benefit of keeping these ifdef out of code.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] drm/i915: Serialize GTT Updates on BXT
  2017-05-22 20:05 ` [PATCH] " Chris Wilson
@ 2017-05-22 22:18   ` Bloomfield, Jon
  2017-05-23  7:33     ` Chris Wilson
  0 siblings, 1 reply; 9+ messages in thread
From: Bloomfield, Jon @ 2017-05-22 22:18 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

> -----Original Message-----
> From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> Sent: Monday, May 22, 2017 1:05 PM
> To: Bloomfield, Jon <jon.bloomfield@intel.com>
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Serialize GTT Updates on BXT
> 
> On Mon, May 22, 2017 at 11:07:25AM -0700, Jon Bloomfield wrote:
> > BXT requires accesses to the GTT (i.e. PTE updates) to be serialized
> > when IOMMU is enabled.
> 
> Serialised with what, since all writes are serialized already?

Fair cop guv. I'll reword.

> 
> The reason is that you need to explain the hw model you are protecting, for
> example do clears need to be protected?
> 
> > This patch guarantees this by wrapping all updates in stop_machine and
> > using a flushing read to guarantee that the GTT writes have reached
> > their destination before restarting.
> 
> If you mention which patch you are reinstating (for a new problem) and cc
> the author, he might point out what has changed in the meantime.

I don't understand. I'm not re-instating any patches to my knowledge, so it's a bit hard to cc the author.

> 
> Note, the flush here is not about ensuring the GTT writes reach their
> destination.
> 
> > Signed-off-by: Jon Bloomfield <jon.bloomfield@intel.com>
> 
> If you are the author and sender, what is John's s-o-b doing afterwards?
This patch was previously signed off by John.

> 
> > Signed-off-by: John Harrison <john.C.Harrison@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_gem_gtt.c | 106
> > ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 106 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > index 7c769d7..6360d92 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > @@ -2191,6 +2191,100 @@ static void gen8_ggtt_clear_range(struct
> i915_address_space *vm,
> >  		gen8_set_pte(&gtt_base[i], scratch_pte);  }
> >
> > +#ifdef CONFIG_INTEL_IOMMU
> > +struct insert_page {
> > +	struct i915_address_space *vm;
> > +	dma_addr_t addr;
> > +	u64 offset;
> > +	enum i915_cache_level level;
> > +};
> > +
> > +static int gen8_ggtt_insert_page__cb(void *_arg) {
> > +	struct insert_page *arg = _arg;
> > +
> > +	struct drm_i915_private *dev_priv = arg->vm->i915;
> > +
> > +	gen8_ggtt_insert_page(arg->vm, arg->addr,
> > +				arg->offset, arg->level, 0);
> > +
> > +	POSTING_READ(GFX_FLSH_CNTL_GEN6);
> 
> This is now just a call to i915_ggtt_invalidate() because we are now also
> responsible for invalidating the guc tlbs as well as the chipset.
> And more importantly it is already done by gen8_ggtt_insert_page.
> 
> All the POSTING_READ(GFX_FLSH_CNTL_GEN6) are spurious.

Are you sure - The purpose of the register read is to ensure that all the PTE writes are flushed from the h/w queue
before we restart the machine. It is critical that all the PTE writes have left this queue before any other accesses are
allowed to begin.
Isn't the invalidate a posted write ? If so, it won't drain the queue.
Even if the invalidate is guaranteed to effect this pipeline flish, the clear_page path doesn't call invalidate, so it's
certainly required there.

> 
> >  static void gen6_ggtt_clear_range(struct i915_address_space *vm,
> >  				  u64 start, u64 length)
> >  {
> > @@ -2789,6 +2883,18 @@ static int gen8_gmch_probe(struct i915_ggtt
> > *ggtt)
> >
> >  	ggtt->base.insert_entries = gen8_ggtt_insert_entries;
> >
> > +#ifdef CONFIG_INTEL_IOMMU
> > +	/* Serialize GTT updates on BXT if VT-d is on. */
> > +	if (IS_BROXTON(dev_priv) && intel_iommu_gfx_mapped) {
> 
> Move to a header and don't ifdef out the users. A small cost in object side for
> the benefit of keeping these ifdef out of code.
Move what to a header ? You mean create a macro for the test, the whole block, or something else ?

I was following the pattern used elsewhere in the code in the vain hope that by following established
convention we might avoid bike-shedding. Every single use of intel_iommu_gfx_mapped in this file is protected by
the #ifdef.

> -Chris
> 
> --
> Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] drm/i915: Serialize GTT Updates on BXT
  2017-05-22 22:18   ` Bloomfield, Jon
@ 2017-05-23  7:33     ` Chris Wilson
  2017-05-23 14:28       ` Bloomfield, Jon
  2017-05-23 23:44       ` Bloomfield, Jon
  0 siblings, 2 replies; 9+ messages in thread
From: Chris Wilson @ 2017-05-23  7:33 UTC (permalink / raw)
  To: Bloomfield, Jon; +Cc: intel-gfx

On Mon, May 22, 2017 at 10:18:31PM +0000, Bloomfield, Jon wrote:
> > -----Original Message-----
> > From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> > Sent: Monday, May 22, 2017 1:05 PM
> > To: Bloomfield, Jon <jon.bloomfield@intel.com>
> > Cc: intel-gfx@lists.freedesktop.org
> > Subject: Re: [Intel-gfx] [PATCH] drm/i915: Serialize GTT Updates on BXT
> > 
> > On Mon, May 22, 2017 at 11:07:25AM -0700, Jon Bloomfield wrote:
> > > BXT requires accesses to the GTT (i.e. PTE updates) to be serialized
> > > when IOMMU is enabled.
> > 
> > Serialised with what, since all writes are serialized already?
> 
> Fair cop guv. I'll reword.
> 
> > 
> > The reason is that you need to explain the hw model you are protecting, for
> > example do clears need to be protected?
> > 
> > > This patch guarantees this by wrapping all updates in stop_machine and
> > > using a flushing read to guarantee that the GTT writes have reached
> > > their destination before restarting.
> > 
> > If you mention which patch you are reinstating (for a new problem) and cc
> > the author, he might point out what has changed in the meantime.
> 
> I don't understand. I'm not re-instating any patches to my knowledge, so it's a bit hard to cc the author.

Please then review history before submitting *copied* code.

> > Note, the flush here is not about ensuring the GTT writes reach their
> > destination.
> > 
> > > Signed-off-by: Jon Bloomfield <jon.bloomfield@intel.com>
> > 
> > If you are the author and sender, what is John's s-o-b doing afterwards?
> This patch was previously signed off by John.
> 
> > 
> > > Signed-off-by: John Harrison <john.C.Harrison@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_gem_gtt.c | 106
> > > ++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 106 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > index 7c769d7..6360d92 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > @@ -2191,6 +2191,100 @@ static void gen8_ggtt_clear_range(struct
> > i915_address_space *vm,
> > >  		gen8_set_pte(&gtt_base[i], scratch_pte);  }
> > >
> > > +#ifdef CONFIG_INTEL_IOMMU
> > > +struct insert_page {
> > > +	struct i915_address_space *vm;
> > > +	dma_addr_t addr;
> > > +	u64 offset;
> > > +	enum i915_cache_level level;
> > > +};
> > > +
> > > +static int gen8_ggtt_insert_page__cb(void *_arg) {
> > > +	struct insert_page *arg = _arg;
> > > +
> > > +	struct drm_i915_private *dev_priv = arg->vm->i915;
> > > +
> > > +	gen8_ggtt_insert_page(arg->vm, arg->addr,
> > > +				arg->offset, arg->level, 0);
> > > +
> > > +	POSTING_READ(GFX_FLSH_CNTL_GEN6);
> > 
> > This is now just a call to i915_ggtt_invalidate() because we are now also
> > responsible for invalidating the guc tlbs as well as the chipset.
> > And more importantly it is already done by gen8_ggtt_insert_page.
> > 
> > All the POSTING_READ(GFX_FLSH_CNTL_GEN6) are spurious.
> 
> Are you sure - The purpose of the register read is to ensure that all the PTE writes are flushed from the h/w queue
> before we restart the machine. It is critical that all the PTE writes have left this queue before any other accesses are
> allowed to begin.
> Isn't the invalidate a posted write ? If so, it won't drain the queue.
> Even if the invalidate is guaranteed to effect this pipeline flish, the clear_page path doesn't call invalidate, so it's
> certainly required there.

It's an uncached mmio write. It is strongly ordered and serial. Besides
if you feel it is wrong, fix the bug at source.

> > >  static void gen6_ggtt_clear_range(struct i915_address_space *vm,
> > >  				  u64 start, u64 length)
> > >  {
> > > @@ -2789,6 +2883,18 @@ static int gen8_gmch_probe(struct i915_ggtt
> > > *ggtt)
> > >
> > >  	ggtt->base.insert_entries = gen8_ggtt_insert_entries;
> > >
> > > +#ifdef CONFIG_INTEL_IOMMU
> > > +	/* Serialize GTT updates on BXT if VT-d is on. */
> > > +	if (IS_BROXTON(dev_priv) && intel_iommu_gfx_mapped) {
> > 
> > Move to a header and don't ifdef out the users. A small cost in object side for
> > the benefit of keeping these ifdef out of code.
> Move what to a header ? You mean create a macro for the test, the whole block, or something else ?
> 
> I was following the pattern used elsewhere in the code in the vain hope that by following established
> convention we might avoid bike-shedding. Every single use of intel_iommu_gfx_mapped in this file is protected by
> the #ifdef.

Otoh, the recent patches adding more intel_iommu_gfx_mapped have avoided
adding ifdefs to the code.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] drm/i915: Serialize GTT Updates on BXT
  2017-05-22 18:07 [PATCH] drm/i915: Serialize GTT Updates on BXT Jon Bloomfield
  2017-05-22 18:37 ` ✓ Fi.CI.BAT: success for " Patchwork
  2017-05-22 20:05 ` [PATCH] " Chris Wilson
@ 2017-05-23  8:40 ` Chris Wilson
  2017-05-23 14:35   ` Bloomfield, Jon
  2 siblings, 1 reply; 9+ messages in thread
From: Chris Wilson @ 2017-05-23  8:40 UTC (permalink / raw)
  To: Jon Bloomfield; +Cc: intel-gfx

On Mon, May 22, 2017 at 11:07:25AM -0700, Jon Bloomfield wrote:
> BXT requires accesses to the GTT (i.e. PTE updates) to be serialized
> when IOMMU is enabled. This patch guarantees this by wrapping all
> updates in stop_machine and using a flushing read to guarantee that
> the GTT writes have reached their destination before restarting.
> 

Testcase? igt/gem_concurrent_blit shows the failure.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] drm/i915: Serialize GTT Updates on BXT
  2017-05-23  7:33     ` Chris Wilson
@ 2017-05-23 14:28       ` Bloomfield, Jon
  2017-05-23 23:44       ` Bloomfield, Jon
  1 sibling, 0 replies; 9+ messages in thread
From: Bloomfield, Jon @ 2017-05-23 14:28 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

> -----Original Message-----
> From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> Sent: Tuesday, May 23, 2017 12:33 AM
> To: Bloomfield, Jon <jon.bloomfield@intel.com>
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Serialize GTT Updates on BXT
> 
> On Mon, May 22, 2017 at 10:18:31PM +0000, Bloomfield, Jon wrote:
> > > -----Original Message-----
> > > From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> > > Sent: Monday, May 22, 2017 1:05 PM
> > > To: Bloomfield, Jon <jon.bloomfield@intel.com>
> > > Cc: intel-gfx@lists.freedesktop.org
> > > Subject: Re: [Intel-gfx] [PATCH] drm/i915: Serialize GTT Updates on BXT
> > >
> > > On Mon, May 22, 2017 at 11:07:25AM -0700, Jon Bloomfield wrote:
> > > > BXT requires accesses to the GTT (i.e. PTE updates) to be serialized
> > > > when IOMMU is enabled.
> > >
> > > Serialised with what, since all writes are serialized already?
> >
> > Fair cop guv. I'll reword.
> >
> > >
> > > The reason is that you need to explain the hw model you are protecting,
> for
> > > example do clears need to be protected?
> > >
> > > > This patch guarantees this by wrapping all updates in stop_machine and
> > > > using a flushing read to guarantee that the GTT writes have reached
> > > > their destination before restarting.
> > >
> > > If you mention which patch you are reinstating (for a new problem) and
> cc
> > > the author, he might point out what has changed in the meantime.
> >
> > I don't understand. I'm not re-instating any patches to my knowledge, so
> it's a bit hard to cc the author.
> 
> Please then review history before submitting *copied* code.
> 
> > > Note, the flush here is not about ensuring the GTT writes reach their
> > > destination.
> > >
> > > > Signed-off-by: Jon Bloomfield <jon.bloomfield@intel.com>
> > >
> > > If you are the author and sender, what is John's s-o-b doing afterwards?
> > This patch was previously signed off by John.
> >
> > >
> > > > Signed-off-by: John Harrison <john.C.Harrison@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_gem_gtt.c | 106
> > > > ++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 106 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > > b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > > index 7c769d7..6360d92 100644
> > > > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > > @@ -2191,6 +2191,100 @@ static void gen8_ggtt_clear_range(struct
> > > i915_address_space *vm,
> > > >  		gen8_set_pte(&gtt_base[i], scratch_pte);  }
> > > >
> > > > +#ifdef CONFIG_INTEL_IOMMU
> > > > +struct insert_page {
> > > > +	struct i915_address_space *vm;
> > > > +	dma_addr_t addr;
> > > > +	u64 offset;
> > > > +	enum i915_cache_level level;
> > > > +};
> > > > +
> > > > +static int gen8_ggtt_insert_page__cb(void *_arg) {
> > > > +	struct insert_page *arg = _arg;
> > > > +
> > > > +	struct drm_i915_private *dev_priv = arg->vm->i915;
> > > > +
> > > > +	gen8_ggtt_insert_page(arg->vm, arg->addr,
> > > > +				arg->offset, arg->level, 0);
> > > > +
> > > > +	POSTING_READ(GFX_FLSH_CNTL_GEN6);
> > >
> > > This is now just a call to i915_ggtt_invalidate() because we are now also
> > > responsible for invalidating the guc tlbs as well as the chipset.
> > > And more importantly it is already done by gen8_ggtt_insert_page.
> > >
> > > All the POSTING_READ(GFX_FLSH_CNTL_GEN6) are spurious.
> >
> > Are you sure - The purpose of the register read is to ensure that all the PTE
> writes are flushed from the h/w queue
> > before we restart the machine. It is critical that all the PTE writes have left
> this queue before any other accesses are
> > allowed to begin.
> > Isn't the invalidate a posted write ? If so, it won't drain the queue.
> > Even if the invalidate is guaranteed to effect this pipeline flish, the
> clear_page path doesn't call invalidate, so it's
> > certainly required there.
> 
> It's an uncached mmio write. It is strongly ordered and serial. Besides
> if you feel it is wrong, fix the bug at source.

Strongly ordered is not enough, nor is being uncached - that just ensures the PTE
writes have left the CPU. We need to ensure they have left the GAM before we
allow any following Aperture accesses to reach the GAM. On the other hand it
may be that the write to the flushctl reg will explicitly cause the h/w to clear the
fifo. I'll check with hw.

Re fixing at source: Assuming you mean just put the read into the gtt functions 
next to the invalidate, then I can do that, but it would mean either another 
bxt/iommu test or executing the read unconditionally for all gens. Are you happy
with that, and if so which ?

> 
> > > >  static void gen6_ggtt_clear_range(struct i915_address_space *vm,
> > > >  				  u64 start, u64 length)
> > > >  {
> > > > @@ -2789,6 +2883,18 @@ static int gen8_gmch_probe(struct i915_ggtt
> > > > *ggtt)
> > > >
> > > >  	ggtt->base.insert_entries = gen8_ggtt_insert_entries;
> > > >
> > > > +#ifdef CONFIG_INTEL_IOMMU
> > > > +	/* Serialize GTT updates on BXT if VT-d is on. */
> > > > +	if (IS_BROXTON(dev_priv) && intel_iommu_gfx_mapped) {
> > >
> > > Move to a header and don't ifdef out the users. A small cost in object side
> for
> > > the benefit of keeping these ifdef out of code.
> > Move what to a header ? You mean create a macro for the test, the whole
> block, or something else ?
> >
> > I was following the pattern used elsewhere in the code in the vain hope
> that by following established
> > convention we might avoid bike-shedding. Every single use of
> intel_iommu_gfx_mapped in this file is protected by
> > the #ifdef.
> 
> Otoh, the recent patches adding more intel_iommu_gfx_mapped have
> avoided
> adding ifdefs to the code.

I've searched the i915 directory on drm-intel-nightly and not found one instance
of intel_iommu_gfx_mapped that is outside a #ifdef. But I'm happy to make my
patch the only one.

> -Chris
> 
> --
> Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] drm/i915: Serialize GTT Updates on BXT
  2017-05-23  8:40 ` Chris Wilson
@ 2017-05-23 14:35   ` Bloomfield, Jon
  0 siblings, 0 replies; 9+ messages in thread
From: Bloomfield, Jon @ 2017-05-23 14:35 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

> -----Original Message-----
> From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> Sent: Tuesday, May 23, 2017 1:41 AM
> To: Bloomfield, Jon <jon.bloomfield@intel.com>
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Serialize GTT Updates on BXT
> 
> On Mon, May 22, 2017 at 11:07:25AM -0700, Jon Bloomfield wrote:
> > BXT requires accesses to the GTT (i.e. PTE updates) to be serialized
> > when IOMMU is enabled. This patch guarantees this by wrapping all
> > updates in stop_machine and using a flushing read to guarantee that
> > the GTT writes have reached their destination before restarting.
> >
> 
> Testcase? igt/gem_concurrent_blit shows the failure.

I was using a combination of tests, run in parallel to hit this bug. I need to get hold of
a system again to re-run. Are you saying you have also repro'd the bug just with
gem_concurrent_blit or just suggesting that it might be a good candidate ?

I'll also re-try without the flushing read, but I'm wary of removing this unless I can
understand why the mmio write has the same effect. It might be luck.

> -Chris
> 
> --
> Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] drm/i915: Serialize GTT Updates on BXT
  2017-05-23  7:33     ` Chris Wilson
  2017-05-23 14:28       ` Bloomfield, Jon
@ 2017-05-23 23:44       ` Bloomfield, Jon
  1 sibling, 0 replies; 9+ messages in thread
From: Bloomfield, Jon @ 2017-05-23 23:44 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

> -----Original Message-----
> From: Bloomfield, Jon
> Sent: Tuesday, May 23, 2017 7:28 AM
> To: 'Chris Wilson' <chris@chris-wilson.co.uk>
> Cc: intel-gfx@lists.freedesktop.org
> Subject: RE: [Intel-gfx] [PATCH] drm/i915: Serialize GTT Updates on BXT
> 

> > -----Original Message-----
> > From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> > Sent: Tuesday, May 23, 2017 12:33 AM
> > To: Bloomfield, Jon <jon.bloomfield@intel.com>
> > Cc: intel-gfx@lists.freedesktop.org
> > Subject: Re: [Intel-gfx] [PATCH] drm/i915: Serialize GTT Updates on BXT
> > It's an uncached mmio write. It is strongly ordered and serial. Besides
> > if you feel it is wrong, fix the bug at source.
> 
> Strongly ordered is not enough, nor is being uncached - that just ensures the
> PTE
> writes have left the CPU. We need to ensure they have left the GAM before
> we
> allow any following Aperture accesses to reach the GAM. On the other hand
> it
> may be that the write to the flushctl reg will explicitly cause the h/w to clear
> the
> fifo. I'll check with hw.

H/W have confirmed that the flushing write is not sufficient to avoid the bug. 
In their words [almost]:

	"You need the read.  The [FLSH_CTRL write] will invalidate the GTLB.  
	 However, it won't ensure a [Aperture] read isn't in the pipe."

So the mmio read needs to stay, and we're left with where to issue it. I placed 
it in the _cb function because it is only needed for BXT and doing it there 
avoids any (admittedly small) extra overhead for other devices. But I have 
no strong opinion on this, so if you really want it to go into the main function
I'll move it. I do think it should be conditional though.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2017-05-23 23:44 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-22 18:07 [PATCH] drm/i915: Serialize GTT Updates on BXT Jon Bloomfield
2017-05-22 18:37 ` ✓ Fi.CI.BAT: success for " Patchwork
2017-05-22 20:05 ` [PATCH] " Chris Wilson
2017-05-22 22:18   ` Bloomfield, Jon
2017-05-23  7:33     ` Chris Wilson
2017-05-23 14:28       ` Bloomfield, Jon
2017-05-23 23:44       ` Bloomfield, Jon
2017-05-23  8:40 ` Chris Wilson
2017-05-23 14:35   ` Bloomfield, Jon

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.