diff mbox series

[1/1] drm/i915/tgl: Implement Wa_1604555607

Message ID 20191122040226.15933-2-ramalingam.c@intel.com (mailing list archive)
State New, archived
Headers show
Series Wa_1604555607 implementation and verification skip | expand

Commit Message

Ramalingam C Nov. 22, 2019, 4:02 a.m. UTC 
From: Michel Thierry <michel.thierry@intel.com>

Implement Wa_1604555607 (set the DS pairing timer to 128 cycles).
FF_MODE2 is part of the register state context, that's why it is
implemented here.

At TGL A0 stepping, FF_MODE2 register read back is broken, hence
disabling the WA verification.

v2: Rebased on top of the WA refactoring (Oscar)
v3: Correctly add to ctx_workarounds_init (Michel)
v4:
  uncore read is used [Tvrtko]
  Macros as used for MASK definition [Chris]
v5:
  Skip the Wa_1604555607 verification [Ram]
  i915 ptr retrieved from engine. [Tvrtko]
v6:
  __wa_write_masked_or used with varying parameter [Tvrtko]
  Added wa_add as a wrapper for __wa_add [Chris]
v7:
  WA verification is skipped on all stepping as of now [Lucas]

BSpec: 19363
HSDES: 1604555607
Signed-off-by: Michel Thierry <michel.thierry@intel.com>
Signed-off-by: Ramalingam C <ramlingam.c@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> [v5]
---
 drivers/gpu/drm/i915/gt/intel_workarounds.c | 34 ++++++++++++++++++---
 drivers/gpu/drm/i915/i915_reg.h             |  4 +++
 2 files changed, 34 insertions(+), 4 deletions(-)

Comments

Tvrtko Ursulin Nov. 22, 2019, 9:21 a.m. UTC | #1 
On 22/11/2019 04:02, Ramalingam C wrote:
> From: Michel Thierry <michel.thierry@intel.com>
> 
> Implement Wa_1604555607 (set the DS pairing timer to 128 cycles).
> FF_MODE2 is part of the register state context, that's why it is
> implemented here.
> 
> At TGL A0 stepping, FF_MODE2 register read back is broken, hence
> disabling the WA verification.
> 
> v2: Rebased on top of the WA refactoring (Oscar)
> v3: Correctly add to ctx_workarounds_init (Michel)
> v4:
>    uncore read is used [Tvrtko]
>    Macros as used for MASK definition [Chris]
> v5:
>    Skip the Wa_1604555607 verification [Ram]
>    i915 ptr retrieved from engine. [Tvrtko]
> v6:
>    __wa_write_masked_or used with varying parameter [Tvrtko]
>    Added wa_add as a wrapper for __wa_add [Chris]
> v7:
>    WA verification is skipped on all stepping as of now [Lucas]
> 
> BSpec: 19363
> HSDES: 1604555607
> Signed-off-by: Michel Thierry <michel.thierry@intel.com>
> Signed-off-by: Ramalingam C <ramlingam.c@intel.com>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> [v5]
> ---
>   drivers/gpu/drm/i915/gt/intel_workarounds.c | 34 ++++++++++++++++++---
>   drivers/gpu/drm/i915/i915_reg.h             |  4 +++
>   2 files changed, 34 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> index 399acae2f33f..b11540caa92d 100644
> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> @@ -146,20 +146,33 @@ static void _wa_add(struct i915_wa_list *wal, const struct i915_wa *wa)
>   	}
>   }
>   
> -static void
> -wa_write_masked_or(struct i915_wa_list *wal, i915_reg_t reg, u32 mask,
> -		   u32 val)
> +static void wa_add(struct i915_wa_list *wal, i915_reg_t reg, u32 mask,
> +		   u32 val, u32 read_mask)
>   {
>   	struct i915_wa wa = {
>   		.reg  = reg,
>   		.mask = mask,
>   		.val  = val,
> -		.read = mask,
> +		.read = read_mask,
>   	};
>   
>   	_wa_add(wal, &wa);
>   }
>   
> +static void
> +__wa_write_masked_or(struct i915_wa_list *wal, i915_reg_t reg, u32 mask,
> +		     u32 val, u32 read_mask)
> +{
> +	wa_add(wal, reg, mask, val, read_mask);
> +}
> +
> +static void
> +wa_write_masked_or(struct i915_wa_list *wal, i915_reg_t reg, u32 mask,
> +		   u32 val)
> +{
> +	__wa_write_masked_or(wal, reg, mask, val, mask);

Minor point - not sure about the need for this helper now, you could 
just call wa_add from here.

> +}
> +
>   static void
>   wa_masked_en(struct i915_wa_list *wal, i915_reg_t reg, u32 val)
>   {
> @@ -568,9 +581,22 @@ static void icl_ctx_workarounds_init(struct intel_engine_cs *engine,
>   static void tgl_ctx_workarounds_init(struct intel_engine_cs *engine,
>   				     struct i915_wa_list *wal)
>   {
> +	u32 val;
> +
>   	/* Wa_1409142259:tgl */
>   	WA_SET_BIT_MASKED(GEN11_COMMON_SLICE_CHICKEN3,
>   			  GEN12_DISABLE_CPS_AWARE_COLOR_PIPE);
> +
> +	/* Wa_1604555607:tgl */
> +	val = intel_uncore_read(engine->uncore, FF_MODE2);
> +	val &= ~FF_MODE2_TDS_TIMER_MASK;
> +	val |= FF_MODE2_TDS_TIMER_128;
> +	/*
> +	 * FIXME: FF_MODE2 register is not readable till TGL B0. We can
> +	 * enable verification of WA from the later steppings, which enables
> +	 * the read of FF_MODE2.
> +	 */
> +	__wa_write_masked_or(wal, FF_MODE2, FF_MODE2_TDS_TIMER_MASK, val, 0);

If I was a betting man I'd bet no one will ever remember to add the 
verification back. So I have to say I disagree with Lucas on this point. 
Someone do a casting vote please. :)

We know it will remain broken until at least after B0?

>   }
>   
>   static void
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 94d0f593eeb7..a99fdf8ea53b 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7922,6 +7922,10 @@ enum {
>   #define   PIXEL_ROUNDING_TRUNC_FB_PASSTHRU 	(1 << 15)
>   #define   PER_PIXEL_ALPHA_BYPASS_EN		(1 << 7)
>   
> +#define FF_MODE2			_MMIO(0x6604)
> +#define   FF_MODE2_TDS_TIMER_MASK	REG_GENMASK(23, 16)
> +#define   FF_MODE2_TDS_TIMER_128	REG_FIELD_PREP(FF_MODE2_TDS_TIMER_MASK, 4)
> +
>   /* PCH */
>   
>   #define PCH_DISPLAY_BASE	0xc0000u
> 

Regards,

Tvrtko
Chris Wilson Nov. 22, 2019, 9:29 a.m. UTC | #2 
Quoting Tvrtko Ursulin (2019-11-22 09:21:45)
> 
> On 22/11/2019 04:02, Ramalingam C wrote:
> > @@ -568,9 +581,22 @@ static void icl_ctx_workarounds_init(struct intel_engine_cs *engine,
> >   static void tgl_ctx_workarounds_init(struct intel_engine_cs *engine,
> >                                    struct i915_wa_list *wal)
> >   {
> > +     u32 val;
> > +
> >       /* Wa_1409142259:tgl */
> >       WA_SET_BIT_MASKED(GEN11_COMMON_SLICE_CHICKEN3,
> >                         GEN12_DISABLE_CPS_AWARE_COLOR_PIPE);
> > +
> > +     /* Wa_1604555607:tgl */
> > +     val = intel_uncore_read(engine->uncore, FF_MODE2);
> > +     val &= ~FF_MODE2_TDS_TIMER_MASK;
> > +     val |= FF_MODE2_TDS_TIMER_128;
> > +     /*
> > +      * FIXME: FF_MODE2 register is not readable till TGL B0. We can
> > +      * enable verification of WA from the later steppings, which enables
> > +      * the read of FF_MODE2.
> > +      */
> > +     __wa_write_masked_or(wal, FF_MODE2, FF_MODE2_TDS_TIMER_MASK, val, 0);
> 
> If I was a betting man I'd bet no one will ever remember to add the 
> verification back. So I have to say I disagree with Lucas on this point. 
> Someone do a casting vote please. :)

I would go with IS_TGL_REVID(A0, A0) as we expect it to be picked up by
the selftests if we have a new stepping that is unfixed -- and a blip in
CI is a much clearer reminder to come back and revisit this code. We
should be able to go "oops, live_workarounds is red, failing on ctx:0xf00"
and from there find this fixme. And so update for a new stepping in the
course of a day (because that's how long it takes for CI to approve a
patch).
-Chris
Ramalingam C Nov. 26, 2019, 8 a.m. UTC | #3 
On 2019-11-22 at 09:29:43 +0000, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-11-22 09:21:45)
> > 
> > On 22/11/2019 04:02, Ramalingam C wrote:
> > > @@ -568,9 +581,22 @@ static void icl_ctx_workarounds_init(struct intel_engine_cs *engine,
> > >   static void tgl_ctx_workarounds_init(struct intel_engine_cs *engine,
> > >                                    struct i915_wa_list *wal)
> > >   {
> > > +     u32 val;
> > > +
> > >       /* Wa_1409142259:tgl */
> > >       WA_SET_BIT_MASKED(GEN11_COMMON_SLICE_CHICKEN3,
> > >                         GEN12_DISABLE_CPS_AWARE_COLOR_PIPE);
> > > +
> > > +     /* Wa_1604555607:tgl */
> > > +     val = intel_uncore_read(engine->uncore, FF_MODE2);
> > > +     val &= ~FF_MODE2_TDS_TIMER_MASK;
> > > +     val |= FF_MODE2_TDS_TIMER_128;
> > > +     /*
> > > +      * FIXME: FF_MODE2 register is not readable till TGL B0. We can
> > > +      * enable verification of WA from the later steppings, which enables
> > > +      * the read of FF_MODE2.
> > > +      */
> > > +     __wa_write_masked_or(wal, FF_MODE2, FF_MODE2_TDS_TIMER_MASK, val, 0);
> > 
> > If I was a betting man I'd bet no one will ever remember to add the 
> > verification back. So I have to say I disagree with Lucas on this point. 
> > Someone do a casting vote please. :)
> 
> I would go with IS_TGL_REVID(A0, A0) as we expect it to be picked up by
> the selftests if we have a new stepping that is unfixed -- and a blip in
> CI is a much clearer reminder to come back and revisit this code. We
> should be able to go "oops, live_workarounds is red, failing on ctx:0xf00"
> and from there find this fixme. And so update for a new stepping in the
> course of a day (because that's how long it takes for CI to approve a
> patch).
Tvrtko and Chris,

So I take it as, we want to exclude the WA verification for the current
steppings alone that is A0 alone, when new stepping comes with
readability broken then we will excude the verification at that time.

Shall I revert to the previous version of patch, just to exclude the WA
verification for A0 alone.?

-Ram
> -Chris
Lucas De Marchi Nov. 26, 2019, 9:38 a.m. UTC | #4 
On Fri, Nov 22, 2019 at 1:30 AM Chris Wilson <chris@chris-wilson.co.uk> wrote:
>
> Quoting Tvrtko Ursulin (2019-11-22 09:21:45)
> >
> > On 22/11/2019 04:02, Ramalingam C wrote:
> > > @@ -568,9 +581,22 @@ static void icl_ctx_workarounds_init(struct intel_engine_cs *engine,
> > >   static void tgl_ctx_workarounds_init(struct intel_engine_cs *engine,
> > >                                    struct i915_wa_list *wal)
> > >   {
> > > +     u32 val;
> > > +
> > >       /* Wa_1409142259:tgl */
> > >       WA_SET_BIT_MASKED(GEN11_COMMON_SLICE_CHICKEN3,
> > >                         GEN12_DISABLE_CPS_AWARE_COLOR_PIPE);
> > > +
> > > +     /* Wa_1604555607:tgl */
> > > +     val = intel_uncore_read(engine->uncore, FF_MODE2);
> > > +     val &= ~FF_MODE2_TDS_TIMER_MASK;
> > > +     val |= FF_MODE2_TDS_TIMER_128;
> > > +     /*
> > > +      * FIXME: FF_MODE2 register is not readable till TGL B0. We can
> > > +      * enable verification of WA from the later steppings, which enables
> > > +      * the read of FF_MODE2.
> > > +      */
> > > +     __wa_write_masked_or(wal, FF_MODE2, FF_MODE2_TDS_TIMER_MASK, val, 0);
> >
> > If I was a betting man I'd bet no one will ever remember to add the
> > verification back. So I have to say I disagree with Lucas on this point.
> > Someone do a casting vote please. :)
>
> I would go with IS_TGL_REVID(A0, A0) as we expect it to be picked up by

then it is broken from start? In A2 it's not fixed yet.... not sure if
CI has A2.
But we should add at least A2 and A3 and make it pass on these.


Lucas De Marchi

> the selftests if we have a new stepping that is unfixed -- and a blip in
> CI is a much clearer reminder to come back and revisit this code. We
> should be able to go "oops, live_workarounds is red, failing on ctx:0xf00"
> and from there find this fixme. And so update for a new stepping in the
> course of a day (because that's how long it takes for CI to approve a
> patch).
> -Chris
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Ramalingam C Nov. 26, 2019, 10:10 a.m. UTC | #5 
On 2019-11-26 at 01:38:20 -0800, Lucas De Marchi wrote:
> On Fri, Nov 22, 2019 at 1:30 AM Chris Wilson <chris@chris-wilson.co.uk> wrote:
> >
> > Quoting Tvrtko Ursulin (2019-11-22 09:21:45)
> > >
> > > On 22/11/2019 04:02, Ramalingam C wrote:
> > > > @@ -568,9 +581,22 @@ static void icl_ctx_workarounds_init(struct intel_engine_cs *engine,
> > > >   static void tgl_ctx_workarounds_init(struct intel_engine_cs *engine,
> > > >                                    struct i915_wa_list *wal)
> > > >   {
> > > > +     u32 val;
> > > > +
> > > >       /* Wa_1409142259:tgl */
> > > >       WA_SET_BIT_MASKED(GEN11_COMMON_SLICE_CHICKEN3,
> > > >                         GEN12_DISABLE_CPS_AWARE_COLOR_PIPE);
> > > > +
> > > > +     /* Wa_1604555607:tgl */
> > > > +     val = intel_uncore_read(engine->uncore, FF_MODE2);
> > > > +     val &= ~FF_MODE2_TDS_TIMER_MASK;
> > > > +     val |= FF_MODE2_TDS_TIMER_128;
> > > > +     /*
> > > > +      * FIXME: FF_MODE2 register is not readable till TGL B0. We can
> > > > +      * enable verification of WA from the later steppings, which enables
> > > > +      * the read of FF_MODE2.
> > > > +      */
> > > > +     __wa_write_masked_or(wal, FF_MODE2, FF_MODE2_TDS_TIMER_MASK, val, 0);
> > >
> > > If I was a betting man I'd bet no one will ever remember to add the
> > > verification back. So I have to say I disagree with Lucas on this point.
> > > Someone do a casting vote please. :)
> >
> > I would go with IS_TGL_REVID(A0, A0) as we expect it to be picked up by
> 
> then it is broken from start? In A2 it's not fixed yet.... not sure if
> CI has A2.
> But we should add at least A2 and A3 and make it pass on these.
Lucas,

How to get the revision details for this A2 and A3 stepping? pointers
plz...

-Ram
> 
> 
> Lucas De Marchi
> 
> > the selftests if we have a new stepping that is unfixed -- and a blip in
> > CI is a much clearer reminder to come back and revisit this code. We
> > should be able to go "oops, live_workarounds is red, failing on ctx:0xf00"
> > and from there find this fixme. And so update for a new stepping in the
> > course of a day (because that's how long it takes for CI to approve a
> > patch).
> > -Chris
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Lucas De Marchi
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
index 399acae2f33f..b11540caa92d 100644
--- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
+++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
@@ -146,20 +146,33 @@  static void _wa_add(struct i915_wa_list *wal, const struct i915_wa *wa)
 	}
 }
 
-static void
-wa_write_masked_or(struct i915_wa_list *wal, i915_reg_t reg, u32 mask,
-		   u32 val)
+static void wa_add(struct i915_wa_list *wal, i915_reg_t reg, u32 mask,
+		   u32 val, u32 read_mask)
 {
 	struct i915_wa wa = {
 		.reg  = reg,
 		.mask = mask,
 		.val  = val,
-		.read = mask,
+		.read = read_mask,
 	};
 
 	_wa_add(wal, &wa);
 }
 
+static void
+__wa_write_masked_or(struct i915_wa_list *wal, i915_reg_t reg, u32 mask,
+		     u32 val, u32 read_mask)
+{
+	wa_add(wal, reg, mask, val, read_mask);
+}
+
+static void
+wa_write_masked_or(struct i915_wa_list *wal, i915_reg_t reg, u32 mask,
+		   u32 val)
+{
+	__wa_write_masked_or(wal, reg, mask, val, mask);
+}
+
 static void
 wa_masked_en(struct i915_wa_list *wal, i915_reg_t reg, u32 val)
 {
@@ -568,9 +581,22 @@  static void icl_ctx_workarounds_init(struct intel_engine_cs *engine,
 static void tgl_ctx_workarounds_init(struct intel_engine_cs *engine,
 				     struct i915_wa_list *wal)
 {
+	u32 val;
+
 	/* Wa_1409142259:tgl */
 	WA_SET_BIT_MASKED(GEN11_COMMON_SLICE_CHICKEN3,
 			  GEN12_DISABLE_CPS_AWARE_COLOR_PIPE);
+
+	/* Wa_1604555607:tgl */
+	val = intel_uncore_read(engine->uncore, FF_MODE2);
+	val &= ~FF_MODE2_TDS_TIMER_MASK;
+	val |= FF_MODE2_TDS_TIMER_128;
+	/*
+	 * FIXME: FF_MODE2 register is not readable till TGL B0. We can
+	 * enable verification of WA from the later steppings, which enables
+	 * the read of FF_MODE2.
+	 */
+	__wa_write_masked_or(wal, FF_MODE2, FF_MODE2_TDS_TIMER_MASK, val, 0);
 }
 
 static void
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 94d0f593eeb7..a99fdf8ea53b 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7922,6 +7922,10 @@  enum {
 #define   PIXEL_ROUNDING_TRUNC_FB_PASSTHRU 	(1 << 15)
 #define   PER_PIXEL_ALPHA_BYPASS_EN		(1 << 7)
 
+#define FF_MODE2			_MMIO(0x6604)
+#define   FF_MODE2_TDS_TIMER_MASK	REG_GENMASK(23, 16)
+#define   FF_MODE2_TDS_TIMER_128	REG_FIELD_PREP(FF_MODE2_TDS_TIMER_MASK, 4)
+
 /* PCH */
 
 #define PCH_DISPLAY_BASE	0xc0000u