[PATCH v2] drm/xe/xe_guc_ads: Add nonpriv registers to write list
Lucas De Marchi
lucas.demarchi at intel.com
Fri Nov 15 18:56:57 UTC 2024
On Fri, Nov 15, 2024 at 11:28:25AM -0600, Cavitt, Jonathan wrote:
>-----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);
you need to add it to the hwe->reg_sr, not to the whitelist. There are
more things to be simplified as this doesn't touch the hw anymore (no
need for forcewake), it's probably in the wrong place now as it's not
really related to the reg_sr. Let's land the other previous patch and
maybe cleanup it later.
Lucas De Marchi
> }
>
> 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