On Mon, Dec 16, 2024 at 03:46:49PM +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`. > > v2: Remove duplicate code, move workarounds read/write > to separated functions.
I responded on v1 before I saw that a v2 had been sent here. https://lore.kernel.org/all/20241216212751.gz5...@mdroper-desk1.amr.corp.intel.com/ Matt > > Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12668 > > Signed-off-by: Sebastian Brzezinka <sebastian.brzezi...@intel.com> > --- > drivers/gpu/drm/i915/gt/intel_workarounds.c | 60 ++++++++++++--------- > 1 file changed, 36 insertions(+), 24 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c > b/drivers/gpu/drm/i915/gt/intel_workarounds.c > index 570c91878189..c0bf909afe8e 100644 > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c > @@ -1722,6 +1722,30 @@ wa_verify(struct intel_gt *gt, const struct i915_wa > *wa, u32 cur, > return true; > } > > +static u32 wa_read_fw(struct intel_gt *gt, struct i915_wa *wa) > +{ > + return wa->is_mcr ? intel_gt_mcr_read_any_fw(gt, wa->mcr_reg) : > + intel_uncore_read_fw(gt->uncore, wa->reg); > + > +} > + > +static void wa_write_fw(struct intel_gt *gt, struct i915_wa *wa) > +{ > + u32 val, old = 0; > + > + /* open-coded rmw due to steering */ > + if (wa->clr) > + old = wa_read_fw(gt, wa); > + > + val = (old & ~wa->clr) | wa->set; > + if (val != old || !wa->clr) { > + if (wa->is_mcr) > + intel_gt_mcr_multicast_write_fw(gt, wa->mcr_reg, val); > + else > + intel_uncore_write_fw(gt->uncore, wa->reg, val); > + } > +} > + > static void wa_list_apply(const struct i915_wa_list *wal) > { > struct intel_gt *gt = wal->gt; > @@ -1741,28 +1765,17 @@ static void wa_list_apply(const struct i915_wa_list > *wal) > intel_uncore_forcewake_get__locked(uncore, fw); > > for (i = 0, wa = wal->list; i < wal->count; i++, wa++) { > - u32 val, old = 0; > - > - /* open-coded rmw due to steering */ > - if (wa->clr) > - old = wa->is_mcr ? > - intel_gt_mcr_read_any_fw(gt, wa->mcr_reg) : > - intel_uncore_read_fw(uncore, wa->reg); > - val = (old & ~wa->clr) | wa->set; > - if (val != old || !wa->clr) { > - if (wa->is_mcr) > - intel_gt_mcr_multicast_write_fw(gt, > wa->mcr_reg, val); > - else > - intel_uncore_write_fw(uncore, wa->reg, val); > - } > - > - if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)) { > - u32 val = wa->is_mcr ? > - intel_gt_mcr_read_any_fw(gt, wa->mcr_reg) : > - intel_uncore_read_fw(uncore, wa->reg); > + /* > + * Writing workarounds can sporadically fail, > + * in which case try to apply it again. > + */ > + uint repeat = 1; > > - wa_verify(gt, wa, val, wal->name, "application"); > - } > + do { > + wa_write_fw(gt, wa); > + } while (!wa_verify(gt, wa, wa_read_fw(gt, wa), wal->name, > + "application") > + && repeat--); > } > > intel_uncore_forcewake_put__locked(uncore, fw); > @@ -1793,9 +1806,8 @@ static bool wa_list_verify(struct intel_gt *gt, > intel_uncore_forcewake_get__locked(uncore, fw); > > for (i = 0, wa = wal->list; i < wal->count; i++, wa++) > - ok &= wa_verify(wal->gt, wa, wa->is_mcr ? > - intel_gt_mcr_read_any_fw(gt, wa->mcr_reg) : > - intel_uncore_read_fw(uncore, wa->reg), > + ok &= wa_verify(wal->gt, wa, > + wa_read_fw(wal->gt, wa), > wal->name, from); > > intel_uncore_forcewake_put__locked(uncore, fw); > -- > 2.34.1 > -- Matt Roper Graphics Software Engineer Linux GPU Platform Enablement Intel Corporation
