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@xxxxxxxxxxxxxxx>
> > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxx>
>
> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
