diff mbox series

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

Message ID 20191121101226.10779-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. 21, 2019, 10:12 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]

BSpec: 19363
HSDES: 1604555607
Signed-off-by: Michel Thierry <michel.thierry@intel.com>
Signed-off-by: Ramalingam C <ramlingam.c@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_workarounds.c | 25 ++++++++++++++++++---
 drivers/gpu/drm/i915/i915_reg.h             |  4 ++++
 2 files changed, 26 insertions(+), 3 deletions(-)

Comments

Chris Wilson Nov. 21, 2019, 10:19 a.m. UTC | #1 
Quoting Ramalingam C (2019-11-21 10:12:26)
> 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]
> 
> BSpec: 19363
> HSDES: 1604555607
> Signed-off-by: Michel Thierry <michel.thierry@intel.com>
> Signed-off-by: Ramalingam C <ramlingam.c@intel.com>
> ---
>  drivers/gpu/drm/i915/gt/intel_workarounds.c | 25 ++++++++++++++++++---
>  drivers/gpu/drm/i915/i915_reg.h             |  4 ++++
>  2 files changed, 26 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> index 399acae2f33f..7cfd2442b736 100644
> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> @@ -147,19 +147,26 @@ 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)
> +__wa_write_masked_or(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);

You might as well call it wa_add() since it takes all the arguments to
_wa_add() and wraps them up into struct that we then copy.

>  }
>  
> +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 +575,21 @@ 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;
> +       if (IS_TGL_REVID(engine->i915, 0, TGL_REVID_A0))
> +               __wa_write_masked_or(wal, FF_MODE2,
> +                                    FF_MODE2_TDS_TIMER_MASK, val, 0);
> +       else
> +               wa_write_masked_or(wal, FF_MODE2, FF_MODE2_TDS_TIMER_MASK, val);

I still have this plan to do this as MI_MATH ops... But it's not a
blocker.
-Chris
Tvrtko Ursulin Nov. 21, 2019, 10:19 a.m. UTC | #2 
On 21/11/2019 10:12, 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]
> 
> BSpec: 19363
> HSDES: 1604555607
> Signed-off-by: Michel Thierry <michel.thierry@intel.com>
> Signed-off-by: Ramalingam C <ramlingam.c@intel.com>
> ---
>   drivers/gpu/drm/i915/gt/intel_workarounds.c | 25 ++++++++++++++++++---
>   drivers/gpu/drm/i915/i915_reg.h             |  4 ++++
>   2 files changed, 26 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> index 399acae2f33f..7cfd2442b736 100644
> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> @@ -147,19 +147,26 @@ 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)
> +__wa_write_masked_or(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)
> +{
> +	__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 +575,21 @@ 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;
> +	if (IS_TGL_REVID(engine->i915, 0, TGL_REVID_A0))
> +		__wa_write_masked_or(wal, FF_MODE2,
> +				     FF_MODE2_TDS_TIMER_MASK, val, 0);
> +	else
> +		wa_write_masked_or(wal, FF_MODE2, FF_MODE2_TDS_TIMER_MASK, val);

Hm a hybrid solution. I was thinking:

  __wa_write_masked_or(wal,
		       FF_MODE2,
		       FF_MODE2_TDS_TIMER_MASK,
		       val,
		       IS_TGL_REVID(engine->i915, 0, TGL_REVID_A0) ?
		       0 : FF_MODE2_TDS_TIMER_MASK);

But yours works as well.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

>   }
>   
>   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
>
Ramalingam C Nov. 21, 2019, 10:51 a.m. UTC | #3 
On 2019-11-21 at 10:19:13 +0000, Chris Wilson wrote:
> Quoting Ramalingam C (2019-11-21 10:12:26)
> > 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]
> > 
> > BSpec: 19363
> > HSDES: 1604555607
> > Signed-off-by: Michel Thierry <michel.thierry@intel.com>
> > Signed-off-by: Ramalingam C <ramlingam.c@intel.com>
> > ---
> >  drivers/gpu/drm/i915/gt/intel_workarounds.c | 25 ++++++++++++++++++---
> >  drivers/gpu/drm/i915/i915_reg.h             |  4 ++++
> >  2 files changed, 26 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > index 399acae2f33f..7cfd2442b736 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > @@ -147,19 +147,26 @@ 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)
> > +__wa_write_masked_or(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);
> 
> You might as well call it wa_add() since it takes all the arguments to
> _wa_add() and wraps them up into struct that we then copy.
> 
Something like this?

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 = 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);
}


-Ram
> >  }
> >  
> > +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 +575,21 @@ 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;
> > +       if (IS_TGL_REVID(engine->i915, 0, TGL_REVID_A0))
> > +               __wa_write_masked_or(wal, FF_MODE2,
> > +                                    FF_MODE2_TDS_TIMER_MASK, val, 0);
> > +       else
> > +               wa_write_masked_or(wal, FF_MODE2, FF_MODE2_TDS_TIMER_MASK, val);
> 
> I still have this plan to do this as MI_MATH ops... But it's not a
> blocker.
> -Chris
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..7cfd2442b736 100644
--- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
+++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
@@ -147,19 +147,26 @@  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)
+__wa_write_masked_or(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)
+{
+	__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 +575,21 @@  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;
+	if (IS_TGL_REVID(engine->i915, 0, TGL_REVID_A0))
+		__wa_write_masked_or(wal, FF_MODE2,
+				     FF_MODE2_TDS_TIMER_MASK, val, 0);
+	else
+		wa_write_masked_or(wal, FF_MODE2, FF_MODE2_TDS_TIMER_MASK, val);
 }
 
 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