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@xxxxxxxxx>
> > cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>
> > ---
> > 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
> >
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
