diff mbox series

[1/5] drm/i915/gt: One more flush for Baytrail clear residuals

Message ID 20210119094053.6919-1-chris@chris-wilson.co.uk (mailing list archive)
State New, archived
Headers show
Series [1/5] drm/i915/gt: One more flush for Baytrail clear residuals | expand

Commit Message

Chris Wilson Jan. 19, 2021, 9:40 a.m. UTC 
CI reports that Baytail requires one more invalidate after CACHE_MODE
for it to be happy.

Fixes: ace44e13e577 ("drm/i915/gt: Clear CACHE_MODE prior to clearing residuals")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Cc: Akeem G Abodunrin <akeem.g.abodunrin@intel.com>
---
 drivers/gpu/drm/i915/gt/gen7_renderclear.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Comments

Abodunrin, Akeem G Jan. 19, 2021, 10:21 a.m. UTC | #1 
> -----Original Message-----
> From: Chris Wilson <chris@chris-wilson.co.uk>
> Sent: Tuesday, January 19, 2021 1:41 AM
> To: intel-gfx@lists.freedesktop.org
> Cc: mika.kuoppala@linux.intel.com; Chris Wilson <chris@chris-wilson.co.uk>;
> Abodunrin, Akeem G <akeem.g.abodunrin@intel.com>
> Subject: [PATCH 1/5] drm/i915/gt: One more flush for Baytrail clear residuals
> 
> CI reports that Baytail requires one more invalidate after CACHE_MODE for it
> to be happy.
> 
> Fixes: ace44e13e577 ("drm/i915/gt: Clear CACHE_MODE prior to clearing
> residuals")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Cc: Akeem G Abodunrin <akeem.g.abodunrin@intel.com>
> ---
>  drivers/gpu/drm/i915/gt/gen7_renderclear.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/gen7_renderclear.c
> b/drivers/gpu/drm/i915/gt/gen7_renderclear.c
> index 39478712769f..8551e6de50e8 100644
> --- a/drivers/gpu/drm/i915/gt/gen7_renderclear.c
> +++ b/drivers/gpu/drm/i915/gt/gen7_renderclear.c
> @@ -353,19 +353,21 @@ static void gen7_emit_pipeline_flush(struct
> batch_chunk *batch)
> 
>  static void gen7_emit_pipeline_invalidate(struct batch_chunk *batch)  {
> -	u32 *cs = batch_alloc_items(batch, 0, 8);
> +	u32 *cs = batch_alloc_items(batch, 0, 10);
> 
>  	/* ivb: Stall before STATE_CACHE_INVALIDATE */
> -	*cs++ = GFX_OP_PIPE_CONTROL(4);
> +	*cs++ = GFX_OP_PIPE_CONTROL(5);
>  	*cs++ = PIPE_CONTROL_STALL_AT_SCOREBOARD |
>  		PIPE_CONTROL_CS_STALL;
>  	*cs++ = 0;
>  	*cs++ = 0;
> +	*cs++ = 0;
> 
> -	*cs++ = GFX_OP_PIPE_CONTROL(4);
> +	*cs++ = GFX_OP_PIPE_CONTROL(5);
>  	*cs++ = PIPE_CONTROL_STATE_CACHE_INVALIDATE;
>  	*cs++ = 0;
>  	*cs++ = 0;
> +	*cs++ = 0;
> 
>  	batch_advance(batch, cs);
>  }
> @@ -397,6 +399,7 @@ static void emit_batch(struct i915_vma * const vma,
>  	batch_add(&cmds, 0xffff0000);
>  	batch_add(&cmds, i915_mmio_reg_offset(CACHE_MODE_1));
>  	batch_add(&cmds, 0xffff0000 |
> PIXEL_SUBSPAN_COLLECT_OPT_DISABLE);
> +	gen7_emit_pipeline_invalidate(&cmds);
>  	gen7_emit_pipeline_flush(&cmds);
> 
>  	/* Switch to the media pipeline and our base address */
> --
> 2.20.1

Reviewed-by: Akeem G Abodunrin <akeem.g.abodunrin@intel.com>

Thank you!
~Akeem
Mika Kuoppala Jan. 19, 2021, 10:25 a.m. UTC | #2 
Chris Wilson <chris@chris-wilson.co.uk> writes:

> CI reports that Baytail requires one more invalidate after CACHE_MODE
> for it to be happy.
>
> Fixes: ace44e13e577 ("drm/i915/gt: Clear CACHE_MODE prior to clearing residuals")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Cc: Akeem G Abodunrin <akeem.g.abodunrin@intel.com>
> ---
>  drivers/gpu/drm/i915/gt/gen7_renderclear.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/gen7_renderclear.c b/drivers/gpu/drm/i915/gt/gen7_renderclear.c
> index 39478712769f..8551e6de50e8 100644
> --- a/drivers/gpu/drm/i915/gt/gen7_renderclear.c
> +++ b/drivers/gpu/drm/i915/gt/gen7_renderclear.c
> @@ -353,19 +353,21 @@ static void gen7_emit_pipeline_flush(struct batch_chunk *batch)
>  
>  static void gen7_emit_pipeline_invalidate(struct batch_chunk *batch)
>  {
> -	u32 *cs = batch_alloc_items(batch, 0, 8);
> +	u32 *cs = batch_alloc_items(batch, 0, 10);
>  
>  	/* ivb: Stall before STATE_CACHE_INVALIDATE */
> -	*cs++ = GFX_OP_PIPE_CONTROL(4);
> +	*cs++ = GFX_OP_PIPE_CONTROL(5);
>  	*cs++ = PIPE_CONTROL_STALL_AT_SCOREBOARD |
>  		PIPE_CONTROL_CS_STALL;
>  	*cs++ = 0;
>  	*cs++ = 0;
> +	*cs++ = 0;

dw[5] seems to be only for gen8+. Does it make a difference?

-Mika

>  
> -	*cs++ = GFX_OP_PIPE_CONTROL(4);
> +	*cs++ = GFX_OP_PIPE_CONTROL(5);
>  	*cs++ = PIPE_CONTROL_STATE_CACHE_INVALIDATE;
>  	*cs++ = 0;
>  	*cs++ = 0;
> +	*cs++ = 0;
>  
>  	batch_advance(batch, cs);
>  }
> @@ -397,6 +399,7 @@ static void emit_batch(struct i915_vma * const vma,
>  	batch_add(&cmds, 0xffff0000);
>  	batch_add(&cmds, i915_mmio_reg_offset(CACHE_MODE_1));
>  	batch_add(&cmds, 0xffff0000 | PIXEL_SUBSPAN_COLLECT_OPT_DISABLE);
> +	gen7_emit_pipeline_invalidate(&cmds);
>  	gen7_emit_pipeline_flush(&cmds);
>  
>  	/* Switch to the media pipeline and our base address */
> -- 
> 2.20.1
Chris Wilson Jan. 19, 2021, 10:34 a.m. UTC | #3 
Quoting Mika Kuoppala (2021-01-19 10:25:14)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > CI reports that Baytail requires one more invalidate after CACHE_MODE
> > for it to be happy.
> >
> > Fixes: ace44e13e577 ("drm/i915/gt: Clear CACHE_MODE prior to clearing residuals")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> > Cc: Akeem G Abodunrin <akeem.g.abodunrin@intel.com>
> > ---
> >  drivers/gpu/drm/i915/gt/gen7_renderclear.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/gen7_renderclear.c b/drivers/gpu/drm/i915/gt/gen7_renderclear.c
> > index 39478712769f..8551e6de50e8 100644
> > --- a/drivers/gpu/drm/i915/gt/gen7_renderclear.c
> > +++ b/drivers/gpu/drm/i915/gt/gen7_renderclear.c
> > @@ -353,19 +353,21 @@ static void gen7_emit_pipeline_flush(struct batch_chunk *batch)
> >  
> >  static void gen7_emit_pipeline_invalidate(struct batch_chunk *batch)
> >  {
> > -     u32 *cs = batch_alloc_items(batch, 0, 8);
> > +     u32 *cs = batch_alloc_items(batch, 0, 10);
> >  
> >       /* ivb: Stall before STATE_CACHE_INVALIDATE */
> > -     *cs++ = GFX_OP_PIPE_CONTROL(4);
> > +     *cs++ = GFX_OP_PIPE_CONTROL(5);
> >       *cs++ = PIPE_CONTROL_STALL_AT_SCOREBOARD |
> >               PIPE_CONTROL_CS_STALL;
> >       *cs++ = 0;
> >       *cs++ = 0;
> > +     *cs++ = 0;
> 
> dw[5] seems to be only for gen8+. Does it make a difference?

Pipe-control has always supported a qword-write, so a packet-length of 4
or 5 on gen4-7. As I recall the debate for gen8+ was whether
pipe-control still supported only a dword-write, and so we went with
always using the qword-length.
-Chris
Mika Kuoppala Jan. 19, 2021, 10:59 a.m. UTC | #4 
Chris Wilson <chris@chris-wilson.co.uk> writes:

> Quoting Mika Kuoppala (2021-01-19 10:25:14)
>> Chris Wilson <chris@chris-wilson.co.uk> writes:
>> 
>> > CI reports that Baytail requires one more invalidate after CACHE_MODE
>> > for it to be happy.
>> >
>> > Fixes: ace44e13e577 ("drm/i915/gt: Clear CACHE_MODE prior to clearing residuals")
>> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>> > Cc: Akeem G Abodunrin <akeem.g.abodunrin@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/gt/gen7_renderclear.c | 9 ++++++---
>> >  1 file changed, 6 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/gt/gen7_renderclear.c b/drivers/gpu/drm/i915/gt/gen7_renderclear.c
>> > index 39478712769f..8551e6de50e8 100644
>> > --- a/drivers/gpu/drm/i915/gt/gen7_renderclear.c
>> > +++ b/drivers/gpu/drm/i915/gt/gen7_renderclear.c
>> > @@ -353,19 +353,21 @@ static void gen7_emit_pipeline_flush(struct batch_chunk *batch)
>> >  
>> >  static void gen7_emit_pipeline_invalidate(struct batch_chunk *batch)
>> >  {
>> > -     u32 *cs = batch_alloc_items(batch, 0, 8);
>> > +     u32 *cs = batch_alloc_items(batch, 0, 10);
>> >  
>> >       /* ivb: Stall before STATE_CACHE_INVALIDATE */
>> > -     *cs++ = GFX_OP_PIPE_CONTROL(4);
>> > +     *cs++ = GFX_OP_PIPE_CONTROL(5);
>> >       *cs++ = PIPE_CONTROL_STALL_AT_SCOREBOARD |
>> >               PIPE_CONTROL_CS_STALL;
>> >       *cs++ = 0;
>> >       *cs++ = 0;
>> > +     *cs++ = 0;
>> 
>> dw[5] seems to be only for gen8+. Does it make a difference?
>
> Pipe-control has always supported a qword-write, so a packet-length of 4
> or 5 on gen4-7. As I recall the debate for gen8+ was whether
> pipe-control still supported only a dword-write, and so we went with
> always using the qword-length.

Yes, there seem to ample evidence that 5 is fine.

Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>

> -Chris
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/gt/gen7_renderclear.c b/drivers/gpu/drm/i915/gt/gen7_renderclear.c
index 39478712769f..8551e6de50e8 100644
--- a/drivers/gpu/drm/i915/gt/gen7_renderclear.c
+++ b/drivers/gpu/drm/i915/gt/gen7_renderclear.c
@@ -353,19 +353,21 @@  static void gen7_emit_pipeline_flush(struct batch_chunk *batch)
 
 static void gen7_emit_pipeline_invalidate(struct batch_chunk *batch)
 {
-	u32 *cs = batch_alloc_items(batch, 0, 8);
+	u32 *cs = batch_alloc_items(batch, 0, 10);
 
 	/* ivb: Stall before STATE_CACHE_INVALIDATE */
-	*cs++ = GFX_OP_PIPE_CONTROL(4);
+	*cs++ = GFX_OP_PIPE_CONTROL(5);
 	*cs++ = PIPE_CONTROL_STALL_AT_SCOREBOARD |
 		PIPE_CONTROL_CS_STALL;
 	*cs++ = 0;
 	*cs++ = 0;
+	*cs++ = 0;
 
-	*cs++ = GFX_OP_PIPE_CONTROL(4);
+	*cs++ = GFX_OP_PIPE_CONTROL(5);
 	*cs++ = PIPE_CONTROL_STATE_CACHE_INVALIDATE;
 	*cs++ = 0;
 	*cs++ = 0;
+	*cs++ = 0;
 
 	batch_advance(batch, cs);
 }
@@ -397,6 +399,7 @@  static void emit_batch(struct i915_vma * const vma,
 	batch_add(&cmds, 0xffff0000);
 	batch_add(&cmds, i915_mmio_reg_offset(CACHE_MODE_1));
 	batch_add(&cmds, 0xffff0000 | PIXEL_SUBSPAN_COLLECT_OPT_DISABLE);
+	gen7_emit_pipeline_invalidate(&cmds);
 	gen7_emit_pipeline_flush(&cmds);
 
 	/* Switch to the media pipeline and our base address */