[2/2] drm/i915: Skip the Wa_1604555607 verification
diff mbox series

Message ID 20191120173155.20168-3-ramalingam.c@intel.com
State New
Headers show
Series
  • Wa_1604555607 implementation and verification skip
Related show

Commit Message

Ramalingam C Nov. 20, 2019, 5:31 p.m. UTC
At TGL A0 stepping, FF_MODE2 register read back is broken, hence
disabling the WA verification.

Helper function called wa_write_masked_or_no_verify is defined for the
same purpose.

Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
---
 drivers/gpu/drm/i915/gt/intel_workarounds.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

Comments

Tvrtko Ursulin Nov. 20, 2019, 5:50 p.m. UTC | #1
On 20/11/2019 17:31, Ramalingam C wrote:
> At TGL A0 stepping, FF_MODE2 register read back is broken, hence
> disabling the WA verification.
> 
> Helper function called wa_write_masked_or_no_verify is defined for the
> same purpose.
> 
> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/gt/intel_workarounds.c | 20 +++++++++++++++++++-
>   1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> index 93efefa205d6..1698330c6f23 100644
> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> @@ -160,6 +160,20 @@ wa_write_masked_or(struct i915_wa_list *wal, i915_reg_t reg, u32 mask,
>   	_wa_add(wal, &wa);
>   }
>   
> +static void
> +wa_write_masked_or_no_verify(struct i915_wa_list *wal, i915_reg_t reg, u32 mask,
> +		   u32 val)
> +{
> +	struct i915_wa wa = {
> +		.reg  = reg,
> +		.mask = mask,
> +		.val  = val,
> +		.read = 0,
> +	};
> +
> +	_wa_add(wal, &wa);
> +}
> +
>   static void
>   wa_masked_en(struct i915_wa_list *wal, i915_reg_t reg, u32 val)
>   {
> @@ -578,7 +592,11 @@ static void tgl_ctx_workarounds_init(struct intel_engine_cs *engine,
>   	val = intel_uncore_read(engine->uncore, FF_MODE2);
>   	val &= ~FF_MODE2_TDS_TIMER_MASK;
>   	val |= FF_MODE2_TDS_TIMER_128;
> -	wa_write_masked_or(wal, FF_MODE2, FF_MODE2_TDS_TIMER_MASK, val);
> +	if (IS_TGL_REVID(engine->uncore->i915, 0, TGL_REVID_A0))

There is engine->i915.

> +		wa_write_masked_or_no_verify(wal, FF_MODE2,
> +					     FF_MODE2_TDS_TIMER_MASK, val);
> +	else
> +		wa_write_masked_or(wal, FF_MODE2, FF_MODE2_TDS_TIMER_MASK, val);

You did not think suggested alternative where read mask is directly 
passed in would work better? It would read a bit more compact:

   __wa_write_masked_or(..., IS_TGL_REVID(..) ? 0 : val)

Up to you what you prefer, I guess the explicit _no_verify brings some 
self-documenting benefits.

Also, do you think there is value in having two patches instead of just 
squashing into one? I would be tempted to squash.

Regards,

Tvrtko

>   }
>   
>   static void
>
Ramalingam C Nov. 20, 2019, 6:05 p.m. UTC | #2
On 2019-11-20 at 17:50:51 +0000, Tvrtko Ursulin wrote:
> 
> On 20/11/2019 17:31, Ramalingam C wrote:
> > At TGL A0 stepping, FF_MODE2 register read back is broken, hence
> > disabling the WA verification.
> > 
> > Helper function called wa_write_masked_or_no_verify is defined for the
> > same purpose.
> > 
> > Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> > cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> > ---
> >   drivers/gpu/drm/i915/gt/intel_workarounds.c | 20 +++++++++++++++++++-
> >   1 file changed, 19 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > index 93efefa205d6..1698330c6f23 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > @@ -160,6 +160,20 @@ wa_write_masked_or(struct i915_wa_list *wal, i915_reg_t reg, u32 mask,
> >   	_wa_add(wal, &wa);
> >   }
> > +static void
> > +wa_write_masked_or_no_verify(struct i915_wa_list *wal, i915_reg_t reg, u32 mask,
> > +		   u32 val)
> > +{
> > +	struct i915_wa wa = {
> > +		.reg  = reg,
> > +		.mask = mask,
> > +		.val  = val,
> > +		.read = 0,
> > +	};
> > +
> > +	_wa_add(wal, &wa);
> > +}
> > +
> >   static void
> >   wa_masked_en(struct i915_wa_list *wal, i915_reg_t reg, u32 val)
> >   {
> > @@ -578,7 +592,11 @@ static void tgl_ctx_workarounds_init(struct intel_engine_cs *engine,
> >   	val = intel_uncore_read(engine->uncore, FF_MODE2);
> >   	val &= ~FF_MODE2_TDS_TIMER_MASK;
> >   	val |= FF_MODE2_TDS_TIMER_128;
> > -	wa_write_masked_or(wal, FF_MODE2, FF_MODE2_TDS_TIMER_MASK, val);
> > +	if (IS_TGL_REVID(engine->uncore->i915, 0, TGL_REVID_A0))
> 
> There is engine->i915.
sure will use it.
> 
> > +		wa_write_masked_or_no_verify(wal, FF_MODE2,
> > +					     FF_MODE2_TDS_TIMER_MASK, val);
> > +	else
> > +		wa_write_masked_or(wal, FF_MODE2, FF_MODE2_TDS_TIMER_MASK, val);
> 
> You did not think suggested alternative where read mask is directly passed
> in would work better? It would read a bit more compact:
> 
>   __wa_write_masked_or(..., IS_TGL_REVID(..) ? 0 : val)
True bit this will change all callers of the fucntion. more over as you
mentioned this this _no_verify is explicit for reader. Hence I prefer
thsi wway
> 
> Up to you what you prefer, I guess the explicit _no_verify brings some
> self-documenting benefits.
> 
> Also, do you think there is value in having two patches instead of just
> squashing into one? I would be tempted to squash.
I would prefer to have it splitted as this non readability this register
is temparary. keeping it separate will indicate we need to fix it in
later stepping. Suggest me if you feel other way around.

-Ram
> 
> Regards,
> 
> Tvrtko
> 
> >   }
> >   static void
> >

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
index 93efefa205d6..1698330c6f23 100644
--- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
+++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
@@ -160,6 +160,20 @@  wa_write_masked_or(struct i915_wa_list *wal, i915_reg_t reg, u32 mask,
 	_wa_add(wal, &wa);
 }
 
+static void
+wa_write_masked_or_no_verify(struct i915_wa_list *wal, i915_reg_t reg, u32 mask,
+		   u32 val)
+{
+	struct i915_wa wa = {
+		.reg  = reg,
+		.mask = mask,
+		.val  = val,
+		.read = 0,
+	};
+
+	_wa_add(wal, &wa);
+}
+
 static void
 wa_masked_en(struct i915_wa_list *wal, i915_reg_t reg, u32 val)
 {
@@ -578,7 +592,11 @@  static void tgl_ctx_workarounds_init(struct intel_engine_cs *engine,
 	val = intel_uncore_read(engine->uncore, FF_MODE2);
 	val &= ~FF_MODE2_TDS_TIMER_MASK;
 	val |= FF_MODE2_TDS_TIMER_128;
-	wa_write_masked_or(wal, FF_MODE2, FF_MODE2_TDS_TIMER_MASK, val);
+	if (IS_TGL_REVID(engine->uncore->i915, 0, TGL_REVID_A0))
+		wa_write_masked_or_no_verify(wal, FF_MODE2,
+					     FF_MODE2_TDS_TIMER_MASK, val);
+	else
+		wa_write_masked_or(wal, FF_MODE2, FF_MODE2_TDS_TIMER_MASK, val);
 }
 
 static void