[PATCH v2] drm/xe/xe_guc_ads: Add nonpriv registers to write list

Cavitt, Jonathan jonathan.cavitt at intel.com
Fri Nov 15 17:28:25 UTC 2024


-----Original Message-----
From: De Marchi, Lucas <lucas.demarchi at intel.com> 
Sent: Thursday, November 14, 2024 4:31 PM
To: Cavitt, Jonathan <jonathan.cavitt at intel.com>
Cc: intel-xe at lists.freedesktop.org; Gupta, saurabhg <saurabhg.gupta at intel.com>; Zuo, Alex <alex.zuo at intel.com>; Vishwanathapura, Niranjana <niranjana.vishwanathapura at intel.com>; brian.welty at intel.com; Roper, Matthew D <matthew.d.roper at intel.com>; Brost, Matthew <matthew.brost at intel.com>; Harrison, John C <john.c.harrison at intel.com>
Subject: Re: [PATCH v2] drm/xe/xe_guc_ads: Add nonpriv registers to write list
> 
> On Fri, Nov 01, 2024 at 10:23:50PM +0000, Jonathan Cavitt wrote:
> >When performing a guc_mmio_regset_write, we add all the registers in the
> >reg_sr list to the save/restore list, but do not do the same for the
> >nonpriv registers.  Add them in.
> >
> >v2:
> >- Add all NONPRIV registers to avoid undefined behavior (Harrison)
> >- s/whitelist/nonpriv
> >
> >Closes: https://gitlab.freedesktop.org/drm/xe/kernel/issues/2249
> >Signed-off-by: Jonathan Cavitt <jonathan.cavitt at intel.com>
> >CC: Lucas de Marchi <lucas.demarchi at intel.com>
> >CC: Matt Roper <matthew.d.roper at intel.com>
> >CC: John Harrison <john.c.harrison at intel.com>
> >CC: Umesh Nerlige Ramappa <umesh.nerlige.ramappa at intel.com>
> >CC: Ashutosh Dixit <ashutosh.dixit at intel.com>
> >---
> > drivers/gpu/drm/xe/xe_guc_ads.c | 7 +++++++
> > 1 file changed, 7 insertions(+)
> >
> >diff --git a/drivers/gpu/drm/xe/xe_guc_ads.c b/drivers/gpu/drm/xe/xe_guc_ads.c
> >index 943146e5b460..b0afb89d9d90 100644
> >--- a/drivers/gpu/drm/xe/xe_guc_ads.c
> >+++ b/drivers/gpu/drm/xe/xe_guc_ads.c
> >@@ -243,6 +243,8 @@ static size_t calculate_regset_size(struct xe_gt *gt)
> > 		xa_for_each(&hwe->reg_sr.xa, sr_idx, sr_entry)
> > 			count++;
> >
> >+	count += RING_MAX_NONPRIV_SLOTS * XE_NUM_HW_ENGINES;
> >+
> > 	count += ADS_REGSET_EXTRA_MAX * XE_NUM_HW_ENGINES;
> >
> > 	if (XE_WA(gt, 1607983814))
> >@@ -727,6 +729,11 @@ static unsigned int guc_mmio_regset_write(struct xe_guc_ads *ads,
> > 	xa_for_each(&hwe->reg_sr.xa, idx, entry)
> > 		guc_mmio_regset_write_one(ads, regset_map, entry->reg, count++);
> >
> >+	for (i = 0; i < RING_MAX_NONPRIV_SLOTS; i++)
> >+		guc_mmio_regset_write_one(ads, regset_map,
> >+					  RING_FORCE_TO_NONPRIV(hwe->mmio_base, i),
> >+					  count++);
> 
> I guess this fixes the issue. I'm still wondering why we are changing it
> in the ads though.
> 
> Ideally we'd do:
> 
> a) switch the order in hw_engine_init() so it does:
> 
> 	xe_reg_sr_apply_whitelist(hwe);
> 	xe_reg_sr_apply_mmio(&hwe->reg_sr, gt);
> 
> b) change xe_reg_sr_apply_whitelist() so instead of writting the
>     mmio slots by itself, it writes them to hwe->reg_sr
> 
> 
> I **think** it would also work and stop handling the nonpriv slots as
> snow flakes. They would also show up in debugfs/.../register-save-restore
> file.
> 
> Can you give it a try?  If it doesn't work, feel free to
> add my
> 
> 	Reviewed-by: Lucas De Marchi <lucas.demarchi at intel.com>
> 
> and we can try to polish it later.

Thank you for the conditional RB.  Here's what I attempted WRT this request:

"""
diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c b/drivers/gpu/drm/xe/xe_hw_engine.c
index c4b0dc3be39c..095df24c61b8 100644
--- a/drivers/gpu/drm/xe/xe_hw_engine.c
+++ b/drivers/gpu/drm/xe/xe_hw_engine.c
@@ -573,8 +573,8 @@ static int hw_engine_init(struct xe_gt *gt, struct xe_hw_engine *hwe,
        xe_gt_assert(gt, id < ARRAY_SIZE(engine_infos) && engine_infos[id].name);
        xe_gt_assert(gt, gt->info.engine_mask & BIT(id));

-       xe_reg_sr_apply_mmio(&hwe->reg_sr, gt);
        xe_reg_sr_apply_whitelist(hwe);
+       xe_reg_sr_apply_mmio(&hwe->reg_sr, gt);

        hwe->hwsp = xe_managed_bo_create_pin_map(xe, tile, SZ_4K,
                                                 XE_BO_FLAG_VRAM_IF_DGFX(tile) |
diff --git a/drivers/gpu/drm/xe/xe_reg_sr.c b/drivers/gpu/drm/xe/xe_reg_sr.c
index e1a0e27cda14..809a72767bde 100644
--- a/drivers/gpu/drm/xe/xe_reg_sr.c
+++ b/drivers/gpu/drm/xe/xe_reg_sr.c
@@ -234,6 +236,13 @@ void xe_reg_sr_apply_whitelist(struct xe_hw_engine *hwe)

        p = drm_dbg_printer(&xe->drm, DRM_UT_DRIVER, NULL);
        xa_for_each(&sr->xa, reg, entry) {
+               struct xe_reg_sr_entry sr_entry = {
+                       .reg = RING_FORCE_TO_NONPRIV(mmio_base, slot),
+                       .clr_bits = entry->clr_bits,
+                       .set_bits = reg | entry->set_bits,
+                       .read_mask = entry->read_mask,
+               };
+
                if (slot == RING_MAX_NONPRIV_SLOTS) {
                        xe_gt_err(gt,
                                  "hwe %s: maximum register whitelist slots (%d) reached, refusing to add more\n",
@@ -242,16 +251,19 @@ void xe_reg_sr_apply_whitelist(struct xe_hw_engine *hwe)
                }

                xe_reg_whitelist_print_entry(&p, 0, reg, entry);
-               xe_mmio_write32(&gt->mmio, RING_FORCE_TO_NONPRIV(mmio_base, slot),
-                               reg | entry->set_bits);
+               xe_reg_sr_add(sr, &sr_entry, gt);
                slot++;
        }

        /* And clear the rest just in case of garbage */
        for (; slot < RING_MAX_NONPRIV_SLOTS; slot++) {
-               u32 addr = RING_NOPID(mmio_base).addr;
-
-               xe_mmio_write32(&gt->mmio, RING_FORCE_TO_NONPRIV(mmio_base, slot), addr);
+               struct xe_reg_sr_entry sr_entry = {
+                       .reg = RING_FORCE_TO_NONPRIV(mmio_base, slot),
+                       .clr_bits = 0,
+                       .set_bits = RING_NOPID(mmio_base).addr,
+                       .read_mask = 0,
+               };
+               xe_reg_sr_add(sr, &sr_entry, gt);
        }

        xe_force_wake_put(gt_to_fw(gt), fw_ref);
"""
This didn't seem to have the intended effect, and xe_oa at mmio-triggered-reports test still failed
after an engine reset.  Also, we seemed to be running up against the "if (xa_empty(&sr->xa))" case
in xe_reg_sr_apply_whitelist, though I can't tell if that's normally what happens, or if it's a result
of the hw_engine_init reordering.
-Jonathan Cavitt

> 
> thanks
> Lucas De Marchi
> 
> >+
> > 	for (e = extra_regs; e < extra_regs + ARRAY_SIZE(extra_regs); e++) {
> > 		if (e->skip)
> > 			continue;
> >-- 
> >2.43.0
> >
> 


More information about the Intel-xe mailing list