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

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

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

> 
> 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...

> +
> +			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...

> +
>  			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...

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
>