Re: [PATCH] drm/xe/reg_sr: Add debugfs to verify status of reg_sr programming

Matt Roper
2026-02-11T00:48:20Z

On Mon, Feb 09, 2026 at 03:56:11PM -0800, Dixit, Ashutosh wrote:
> On Fri, 06 Feb 2026 16:23:05 -0800, Matt Roper wrote:
> >
> > When applying save-restore register programming for workarounds, tuning
> > settings, and general device configuration we assume the programming was
> > successful. However there are a number of cases where the desired
> > reg_sr programming can become lost:
> >
> > - workarounds implemented on the wrong RTP table might not get
> > saved/restored at the right time leading to, for example, failure to
> > re-apply the programming after engine resets
>
> I guess this can be checked in the code, post reset, and we could print
> that reg s/r is wrong, rather than rely on debugfs. But anyway this is
> future patch, if at all.

That's sort of what we did with i915 but it caused a lot of headaches
there. If any workaround/tuning didn't apply (or didn't read back
properly), the CI system would see the warning in dmesg and consider the
entire driver probe to have failed, blocking all other testing. I'd
like to avoid that and just make it something that we can manually test
when we want to (and it should be easy to wrap an IGT around the debugfs
to let CI exercise this in a way that won't break all other platform
testing if something does go wrong).

>
> > - some hardware registers become "locked" and can no longer be updated
> > after firmware or the driver finishes initializing them
> > - sometimes the hardware teams just made a mistake when documenting the
> > register and/or bits that needed to be programmed
> >
> > Add a debugfs entry that will read back the registers referenced on a
> > GT's save-restore lists and print any cases where the desired
> > programming is no longer in effect. Such cases might indicate the
> > presence of a driver/firmware bug, might indicate that the documentation
> > we were following has a mistake, or might be benign (occasionally
> > registers have broken read-back capability preventing verification, but
> > previous writes were still successful and effective).
> >
> > For now we only verify the GT and engine reg_sr lists. Verifying the
> > LRC list will require checking the expected programming against the
> > default_lrc contents, not the live registers (which may not reflect the
> > reg_sr programming if no context is actively running).
> >
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > ---
> > drivers/gpu/drm/xe/xe_gt_debugfs.c | 26 +++++++++++++++++++++++
> > drivers/gpu/drm/xe/xe_reg_sr.c | 34 ++++++++++++++++++++++++++++++
> > drivers/gpu/drm/xe/xe_reg_sr.h | 3 +++
> > 3 files changed, 63 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/xe/xe_gt_debugfs.c b/drivers/gpu/drm/xe/xe_gt_debugfs.c
> > index 4363bc9c3606..aa43427a9f4b 100644
> > --- a/drivers/gpu/drm/xe/xe_gt_debugfs.c
> > +++ b/drivers/gpu/drm/xe/xe_gt_debugfs.c
> > @@ -155,6 +155,30 @@ static int register_save_restore(struct xe_gt *gt, struct drm_printer *p)
> > return 0;
> > }
> >
> > +/*
> > + * Check the registers referenced on a save-restore list and report any
> > + * save-restore entries that did not get applied.
> > + */
> > +static int register_save_restore_check(struct xe_gt *gt, struct drm_printer *p)
> > +{
> > + struct xe_hw_engine *hwe;
> > + enum xe_hw_engine_id id;
> > +
> > + CLASS(xe_force_wake, fw_ref)(gt_to_fw(gt), XE_FORCEWAKE_ALL);
> > + if (!xe_force_wake_ref_has_domain(fw_ref.domains, XE_FORCEWAKE_ALL)) {
> > + drm_printf(p, "ERROR: Could not acquire forcewake\n");
> > + return -ETIMEDOUT;
> > + }
> > +
> > + xe_reg_sr_readback_check(&gt->reg_sr, gt, p);
> > + for_each_hw_engine(hwe, gt, id)
> > + xe_reg_sr_readback_check(&hwe->reg_sr, gt, p);
> > +
> > + /* TODO: Check hwe->reg_lrc against contents of default_lrc. */
> > +
> > + return 0;
> > +}
> > +
> > static int rcs_default_lrc(struct xe_gt *gt, struct drm_printer *p)
> > {
> > xe_lrc_dump_default(p, gt, XE_ENGINE_CLASS_RENDER);
> > @@ -209,6 +233,8 @@ static const struct drm_info_list vf_safe_debugfs_list[] = {
> > { "default_lrc_vecs", .show = xe_gt_debugfs_show_with_rpm, .data = vecs_default_lrc },
> > { "hwconfig", .show = xe_gt_debugfs_show_with_rpm, .data = hwconfig },
> > { "pat_sw_config", .show = xe_gt_debugfs_simple_show, .data = xe_pat_dump_sw_config },
> > + { "register-save-restore-check",
> > + .show = xe_gt_debugfs_show_with_rpm, .data = register_save_restore_check },
>
> Not sure about this, so asking: can "register-save-restore-check" be added
> to vf_safe_debugfs_list[] or must it be added to pf_only_debugfs_list[]?

We definitely want it on the VF list for when we add the LRC
verification, which I'll post soon.

I have a local patch that ensures we don't parse rtp -> reg_sr for the
gt/hwe lists when in a VF (which also prevents confusion in other
debugfs entries since even in the existing "register-save-restore"
debugfs we shouldn't be printing a bunch of things that never actually
got applied). I guess I should upstream that patch before this one, or
include it as part of v2 of this series.

>
> > };
> >
> > /* everything else should be added here */
> > diff --git a/drivers/gpu/drm/xe/xe_reg_sr.c b/drivers/gpu/drm/xe/xe_reg_sr.c
> > index d3e13ea33123..37555d5211d8 100644
> > --- a/drivers/gpu/drm/xe/xe_reg_sr.c
> > +++ b/drivers/gpu/drm/xe/xe_reg_sr.c
> > @@ -204,3 +204,37 @@ void xe_reg_sr_dump(struct xe_reg_sr *sr, struct drm_printer *p)
> > str_yes_no(entry->reg.masked),
> > str_yes_no(entry->reg.mcr));
> > }
> > +
> > +static u32 readback_reg(struct xe_gt *gt, struct xe_reg reg)
> > +{
> > + struct xe_reg_mcr mcr_reg = to_xe_reg_mcr(reg);
> > +
> > + if (reg.mcr)
> > + return xe_gt_mcr_unicast_read_any(gt, mcr_reg);
> > + else
> > + return xe_mmio_read32(&gt->mmio, reg);
> > +}
> > +
> > +/**
> > + * xe_reg_sr_readback_check() - Readback registers referenced in save/restore
> > + * entries and check whether the programming is in place.
> > + * @sr: Save/restore entries
> > + * @gt: GT to read register from
> > + * @p: DRM printer to report discrepancies on
> > + */
> > +void xe_reg_sr_readback_check(struct xe_reg_sr *sr,
> > + struct xe_gt *gt,
> > + struct drm_printer *p)
> > +{
> > + struct xe_reg_sr_entry *entry;
> > + unsigned long offset;
> > +
>
> Not sure if it is desirable/possible to add a "xe_force_wake_assert_held"
> here...

We don't have a way to determine which forcewake domain(s) (if any) are
actually necessary for a specific register. Today this readback check
function only gets called from a single callsite, and that callsite just
grabs 'ALL' so that it doesn't have to worry about it. So we could add
an assert on FORCEWAKE_ALL here for the time being. But adding an
assertion is presumably meant to protect us if we add another callsite
in the future that forgets the forcewake; if the new callsite uses more
precise forcewake acquisition (for some special reg_sr that has
registers within a specific domain), then asserting on FORCEWAKE_ALL
would actually cause problems.

Matt

>
> Assuming the comments above are ok, LGTM:
>
> Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
>
> > + xa_for_each(&sr->xa, offset, entry) {
> > + u32 val = readback_reg(gt, entry->reg);
> > + u32 mask = entry->clr_bits | entry->set_bits;
> > +
> > + if ((val & mask) != entry->set_bits)
> > + drm_printf(p, "%#8lx & %#10x :: expected %#10x got %#10x\n",
> > + offset, mask, entry->set_bits, val & mask);
> > + }
> > +}
> > diff --git a/drivers/gpu/drm/xe/xe_reg_sr.h b/drivers/gpu/drm/xe/xe_reg_sr.h
> > index 51fbba423e27..cd133a09aa9b 100644
> > --- a/drivers/gpu/drm/xe/xe_reg_sr.h
> > +++ b/drivers/gpu/drm/xe/xe_reg_sr.h
> > @@ -19,6 +19,9 @@ struct drm_printer;
> >
> > int xe_reg_sr_init(struct xe_reg_sr *sr, const char *name, struct xe_device *xe);
> > void xe_reg_sr_dump(struct xe_reg_sr *sr, struct drm_printer *p);
> > +void xe_reg_sr_readback_check(struct xe_reg_sr *sr,
> > + struct xe_gt *gt,
> > + struct drm_printer *p);
> >
> > int xe_reg_sr_add(struct xe_reg_sr *sr, const struct xe_reg_sr_entry *e,
> > struct xe_gt *gt);
> > --
> > 2.52.0
> >

--
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation