[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(>->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(>->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