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@xxxxxxxxx> > --- > 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 >
