Re: [RFC PATCH] i915/gt: Reapply workarounds in case the previous attempt failed.

[Andi Shyti <andi.shyti@linux.intel.com>]

Hi Rodrigo,

On Fri, Dec 06, 2024 at 10:38:24AM -0500, Rodrigo Vivi wrote:
> On Thu, Dec 05, 2024 at 03:47:35PM +0000, Sebastian Brzezinka wrote:
> > `wa_verify`sporadically detects lost workaround on application; this
> > is unusual behavior since wa are applied at `intel_gt_init_hw` and
> > verified right away by `intel_gt_verify_workarounds`, and  `wa_verify`
> > doesn't fail on initialization as one might suspect would happen.
> > 
> > One approach that may be somewhat beneficial is to reapply workarounds
> > in the event of failure, or even get rid of verify on application,
> > since it's redundant to `intel_gt_verify_workarounds`.
> > 
> > This patch aims to resolve: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12668
> 
> It should be:
> 
> Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12668

aapart from the formatting issues this was suggested by me. We
have observed some sporadic vailures in applying the specific
workaround added by Ville (now cc'ed to the thread) in commit
0ddae025ab6c ("drm/i915: Disable compression tricks on JSL").

Because it's sporadic, we could give it one more chance and try
to re-apply it.

> > 
> > Signed-off-by: Sebastian Brzezinka <sebastian.brzezinka@intel.com>
> > ---
> >  drivers/gpu/drm/i915/gt/intel_workarounds.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > index 570c91878189..4ee623448223 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > @@ -1761,6 +1761,17 @@ static void wa_list_apply(const struct i915_wa_list *wal)
> >  				intel_gt_mcr_read_any_fw(gt, wa->mcr_reg) :
> >  				intel_uncore_read_fw(uncore, wa->reg);
> >  
> > +			if ((val ^ wa->set) & wa->read) { 
> > +				if (wa->is_mcr)
> > +					intel_gt_mcr_multicast_write_fw(gt, wa->mcr_reg, val);
> > +				else
> > +					intel_uncore_write_fw(uncore, wa->reg, val);
> > +			}
> 
> instead of duplicating the code you should extract that to an aux function
> to write it...

a for loop can decrease the amount of duplicated code.

> > +
> > +			val = wa->is_mcr ?
> > +				intel_gt_mcr_read_any_fw(gt, wa->mcr_reg) :
> > +				intel_uncore_read_fw(uncore, wa->reg);
> 
> and another one to read it...

this, indeed it's just reading, but we are trying to re-write. If
we wrote the unwanted value, we will keep reading the unwanted
value.

> > +
> >  			wa_verify(gt, wa, val, wal->name, "application");
> 
> However my biggest concern with this patch is the brute force solution
> and only on CONFIG_DRM_I915_DEBUG_GEM case...

this is a good point, indeed, I don't understand why the
confirmation should be within the DEBUG section.

Andi

> and as duplication because I see that the second write attempt is
> already happening above if (val != old || !wa->clr)
> 
> So, something is not quite right in here and this deserves another alternative..
> 
> 
> >  		}
> >  	}
> > -- 
> > 2.34.1
> >