diff mbox series

[2/2] drm/i915/gt: Wait for aux invalidation on Tigerlake

Message ID 20200716203201.11977-2-chris@chris-wilson.co.uk (mailing list archive)
State New, archived
Headers show
Series [1/2] drm/i915/selftests: Add compiler paranoia for checking HWSP values | expand

Commit Message

Chris Wilson July 16, 2020, 8:32 p.m. UTC 
Add a SRM read back of the aux invalidation register after poking
hsdes: 1809175790, as failing to do so leads to writes going astray.

Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/2169
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/gt/intel_lrc.c | 31 ++++++++++++++++++++++-------
 1 file changed, 24 insertions(+), 7 deletions(-)

Comments

Chris Wilson July 16, 2020, 9:32 p.m. UTC | #1 
Quoting Chris Wilson (2020-07-16 21:32:01)
> Add a SRM read back of the aux invalidation register after poking
> hsdes: 1809175790, as failing to do so leads to writes going astray.
> 
> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/2169
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/gt/intel_lrc.c | 31 ++++++++++++++++++++++-------
>  1 file changed, 24 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index e0280a672f1d..c9e46792b976 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -4757,14 +4757,21 @@ static int gen12_emit_flush(struct i915_request *request, u32 mode)
>         intel_engine_mask_t aux_inv = 0;
>         u32 cmd, *cs;
>  
> +       cmd = 4;
> +       if (mode & EMIT_INVALIDATE)
> +               cmd += 2;
>         if (mode & EMIT_INVALIDATE)
>                 aux_inv = request->engine->mask & ~BIT(BCS0);
> +       if (aux_inv)
> +               cmd += 2 * hweight8(aux_inv) + 6;
>  
> -       cs = intel_ring_begin(request,
> -                             4 + (aux_inv ? 2 * hweight8(aux_inv) + 2 : 0));
> +       cs = intel_ring_begin(request, cmd);
>         if (IS_ERR(cs))
>                 return PTR_ERR(cs);
>  
> +       if (mode & EMIT_INVALIDATE)
> +               *cs++ = preparser_disable(true);
> +
>         cmd = MI_FLUSH_DW + 1;
>  
>         /* We always require a command barrier so that subsequent
> @@ -4780,11 +4787,6 @@ static int gen12_emit_flush(struct i915_request *request, u32 mode)
>                         cmd |= MI_INVALIDATE_BSD;
>         }
>  
> -       *cs++ = cmd;
> -       *cs++ = LRC_PPHWSP_SCRATCH_ADDR;
> -       *cs++ = 0; /* upper addr */
> -       *cs++ = 0; /* value */
> -
>         if (aux_inv) { /* hsdes: 1809175790 */
>                 struct intel_engine_cs *engine;
>                 unsigned int tmp;
> @@ -4796,7 +4798,22 @@ static int gen12_emit_flush(struct i915_request *request, u32 mode)
>                         *cs++ = AUX_INV;
>                 }
>                 *cs++ = MI_NOOP;
> +
> +               *cs++ = MI_STORE_REGISTER_MEM | MI_USE_GGTT;

Sigh. So I fixed this to MI_SRM_GEN8 and tgl failed the test again.

> +               *cs++ = i915_mmio_reg_offset(aux_inv_reg(request->engine));
> +               *cs++ = i915_ggtt_offset(engine->status_page.vma) +
> +                       I915_GEM_HWS_SCRATCH * sizeof(u32);
> +               *cs++ = 0;

-Chris
Mika Kuoppala July 17, 2020, 8:30 a.m. UTC | #2 
Chris Wilson <chris@chris-wilson.co.uk> writes:

> Add a SRM read back of the aux invalidation register after poking
> hsdes: 1809175790, as failing to do so leads to writes going astray.
>
> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/2169
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/gt/intel_lrc.c | 31 ++++++++++++++++++++++-------
>  1 file changed, 24 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index e0280a672f1d..c9e46792b976 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -4757,14 +4757,21 @@ static int gen12_emit_flush(struct i915_request *request, u32 mode)
>  	intel_engine_mask_t aux_inv = 0;
>  	u32 cmd, *cs;
>  
> +	cmd = 4;
> +	if (mode & EMIT_INVALIDATE)
> +		cmd += 2;
>  	if (mode & EMIT_INVALIDATE)
>  		aux_inv = request->engine->mask & ~BIT(BCS0);
> +	if (aux_inv)
> +		cmd += 2 * hweight8(aux_inv) + 6;
>  
> -	cs = intel_ring_begin(request,
> -			      4 + (aux_inv ? 2 * hweight8(aux_inv) + 2 : 0));
> +	cs = intel_ring_begin(request, cmd);
>  	if (IS_ERR(cs))
>  		return PTR_ERR(cs);
>  
> +	if (mode & EMIT_INVALIDATE)
> +		*cs++ = preparser_disable(true);

This makes sense. Could be even separate patch.

On invalidate, care to try if the actual invalidate LRI
with POSTED set (after disabling preparser) could also fix this?

It feels like the posted was missing from the start.

-Mika

> +
>  	cmd = MI_FLUSH_DW + 1;
>  
>  	/* We always require a command barrier so that subsequent
> @@ -4780,11 +4787,6 @@ static int gen12_emit_flush(struct i915_request *request, u32 mode)
>  			cmd |= MI_INVALIDATE_BSD;
>  	}
>  
> -	*cs++ = cmd;
> -	*cs++ = LRC_PPHWSP_SCRATCH_ADDR;
> -	*cs++ = 0; /* upper addr */
> -	*cs++ = 0; /* value */
> -
>  	if (aux_inv) { /* hsdes: 1809175790 */
>  		struct intel_engine_cs *engine;
>  		unsigned int tmp;
> @@ -4796,7 +4798,22 @@ static int gen12_emit_flush(struct i915_request *request, u32 mode)
>  			*cs++ = AUX_INV;
>  		}
>  		*cs++ = MI_NOOP;
> +
> +		*cs++ = MI_STORE_REGISTER_MEM | MI_USE_GGTT;
> +		*cs++ = i915_mmio_reg_offset(aux_inv_reg(request->engine));
> +		*cs++ = i915_ggtt_offset(engine->status_page.vma) +
> +			I915_GEM_HWS_SCRATCH * sizeof(u32);
> +		*cs++ = 0;
>  	}
> +
> +	*cs++ = cmd;
> +	*cs++ = LRC_PPHWSP_SCRATCH_ADDR;
> +	*cs++ = 0; /* upper addr */
> +	*cs++ = 0; /* value */
> +
> +	if (mode & EMIT_INVALIDATE)
> +		*cs++ = preparser_disable(false);
> +
>  	intel_ring_advance(request, cs);
>  
>  	return 0;
> -- 
> 2.20.1
Chris Wilson July 17, 2020, 10:24 a.m. UTC | #3 
Quoting Mika Kuoppala (2020-07-17 09:30:07)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > Add a SRM read back of the aux invalidation register after poking
> > hsdes: 1809175790, as failing to do so leads to writes going astray.
> >
> > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/2169
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/gt/intel_lrc.c | 31 ++++++++++++++++++++++-------
> >  1 file changed, 24 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > index e0280a672f1d..c9e46792b976 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > @@ -4757,14 +4757,21 @@ static int gen12_emit_flush(struct i915_request *request, u32 mode)
> >       intel_engine_mask_t aux_inv = 0;
> >       u32 cmd, *cs;
> >  
> > +     cmd = 4;
> > +     if (mode & EMIT_INVALIDATE)
> > +             cmd += 2;
> >       if (mode & EMIT_INVALIDATE)
> >               aux_inv = request->engine->mask & ~BIT(BCS0);
> > +     if (aux_inv)
> > +             cmd += 2 * hweight8(aux_inv) + 6;
> >  
> > -     cs = intel_ring_begin(request,
> > -                           4 + (aux_inv ? 2 * hweight8(aux_inv) + 2 : 0));
> > +     cs = intel_ring_begin(request, cmd);
> >       if (IS_ERR(cs))
> >               return PTR_ERR(cs);
> >  
> > +     if (mode & EMIT_INVALIDATE)
> > +             *cs++ = preparser_disable(true);
> 
> This makes sense. Could be even separate patch.
> 
> On invalidate, care to try if the actual invalidate LRI
> with POSTED set (after disabling preparser) could also fix this?

I may have accidentally broke tgl1-gem and it needs some tlc ;)
-Chris
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index e0280a672f1d..c9e46792b976 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -4757,14 +4757,21 @@  static int gen12_emit_flush(struct i915_request *request, u32 mode)
 	intel_engine_mask_t aux_inv = 0;
 	u32 cmd, *cs;
 
+	cmd = 4;
+	if (mode & EMIT_INVALIDATE)
+		cmd += 2;
 	if (mode & EMIT_INVALIDATE)
 		aux_inv = request->engine->mask & ~BIT(BCS0);
+	if (aux_inv)
+		cmd += 2 * hweight8(aux_inv) + 6;
 
-	cs = intel_ring_begin(request,
-			      4 + (aux_inv ? 2 * hweight8(aux_inv) + 2 : 0));
+	cs = intel_ring_begin(request, cmd);
 	if (IS_ERR(cs))
 		return PTR_ERR(cs);
 
+	if (mode & EMIT_INVALIDATE)
+		*cs++ = preparser_disable(true);
+
 	cmd = MI_FLUSH_DW + 1;
 
 	/* We always require a command barrier so that subsequent
@@ -4780,11 +4787,6 @@  static int gen12_emit_flush(struct i915_request *request, u32 mode)
 			cmd |= MI_INVALIDATE_BSD;
 	}
 
-	*cs++ = cmd;
-	*cs++ = LRC_PPHWSP_SCRATCH_ADDR;
-	*cs++ = 0; /* upper addr */
-	*cs++ = 0; /* value */
-
 	if (aux_inv) { /* hsdes: 1809175790 */
 		struct intel_engine_cs *engine;
 		unsigned int tmp;
@@ -4796,7 +4798,22 @@  static int gen12_emit_flush(struct i915_request *request, u32 mode)
 			*cs++ = AUX_INV;
 		}
 		*cs++ = MI_NOOP;
+
+		*cs++ = MI_STORE_REGISTER_MEM | MI_USE_GGTT;
+		*cs++ = i915_mmio_reg_offset(aux_inv_reg(request->engine));
+		*cs++ = i915_ggtt_offset(engine->status_page.vma) +
+			I915_GEM_HWS_SCRATCH * sizeof(u32);
+		*cs++ = 0;
 	}
+
+	*cs++ = cmd;
+	*cs++ = LRC_PPHWSP_SCRATCH_ADDR;
+	*cs++ = 0; /* upper addr */
+	*cs++ = 0; /* value */
+
+	if (mode & EMIT_INVALIDATE)
+		*cs++ = preparser_disable(false);
+
 	intel_ring_advance(request, cs);
 
 	return 0;