[Intel-gfx] [PATCH] drm/i915/gt: Skip rmw for masked registers

[Chris Wilson]

Quoting Tvrtko Ursulin (2020-01-30 14:48:47)
> 
> On 30/01/2020 14:21, Chris Wilson wrote:
> > A masked register does not need rmw to update, and it is best not to use
> > such a sequence.
> > 
> > Reported-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> > ---
> >   drivers/gpu/drm/i915/gt/intel_workarounds.c | 29 ++++++++++++++-------
> >   1 file changed, 19 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > index 5a7db279f702..89474a4fa9b9 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > @@ -167,12 +167,6 @@ wa_write_masked_or(struct i915_wa_list *wal, i915_reg_t reg, u32 mask,
> >       wa_add(wal, reg, mask, val, mask);
> >   }
> >   
> > -static void
> > -wa_masked_en(struct i915_wa_list *wal, i915_reg_t reg, u32 val)
> > -{
> > -     wa_write_masked_or(wal, reg, val, _MASKED_BIT_ENABLE(val));
> > -}
> > -
> >   static void
> >   wa_write(struct i915_wa_list *wal, i915_reg_t reg, u32 val)
> >   {
> > @@ -185,14 +179,26 @@ wa_write_or(struct i915_wa_list *wal, i915_reg_t reg, u32 val)
> >       wa_write_masked_or(wal, reg, val, val);
> >   }
> >   
> > +static void
> > +wa_masked_en(struct i915_wa_list *wal, i915_reg_t reg, u32 val)
> > +{
> > +     wa_add(wal, reg, 0, _MASKED_BIT_ENABLE(val), val);
> > +}
> > +
> > +static void
> > +wa_masked_dis(struct i915_wa_list *wal, i915_reg_t reg, u32 val)
> > +{
> > +     wa_add(wal, reg, 0, _MASKED_BIT_DISABLE(val), val);
> > +}
> > +
> >   #define WA_SET_BIT_MASKED(addr, mask) \
> > -     wa_write_masked_or(wal, (addr), (mask), _MASKED_BIT_ENABLE(mask))
> > +     wa_masked_en(wal, (addr), mask)
> >   
> >   #define WA_CLR_BIT_MASKED(addr, mask) \
> > -     wa_write_masked_or(wal, (addr), (mask), _MASKED_BIT_DISABLE(mask))
> > +     wa_masked_dis(wal, (addr), mask)
> >   
> >   #define WA_SET_FIELD_MASKED(addr, mask, value) \
> > -     wa_write_masked_or(wal, (addr), (mask), _MASKED_FIELD((mask), (value)))
> > +     wa_write_masked_or(wal, (addr), 0, _MASKED_FIELD((mask), (value)))
> >   
> >   static void gen8_ctx_workarounds_init(struct intel_engine_cs *engine,
> >                                     struct i915_wa_list *wal)
> > @@ -1020,7 +1026,10 @@ wa_list_apply(struct intel_uncore *uncore, const struct i915_wa_list *wal)
> >       intel_uncore_forcewake_get__locked(uncore, fw);
> 
> In the realm of pointless we could also consider the rmw vs wr when 
> calculating the required forcewake domains.

In all likelihood we have a blanket FORCEWAKE_ALL anyway :)

> >       for (i = 0, wa = wal->list; i < wal->count; i++, wa++) {
> > -             intel_uncore_rmw_fw(uncore, wa->reg, wa->mask, wa->val);
> > +             if (wa->mask)
> > +                     intel_uncore_rmw_fw(uncore, wa->reg, wa->mask, wa->val);
> > +             else
> > +                     intel_uncore_write(uncore, wa->reg, wa->val);
> >               if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM))
> >                       wa_verify(wa,
> >                                 intel_uncore_read_fw(uncore, wa->reg),
> > 
> 
> Looks correct.
> 
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> 
> What were the real world consequences of getting it wrong?

We're speculating the Broadwater failure to preserve MI_MODE might be
such a case.
-Chris