| Message ID | 20191120173155.20168-3-ramalingam.c@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Wa_1604555607 implementation and verification skip | expand |
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 >
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 > >
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
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(-)