Re: [PATCH v3] drm/xe/xe_guc_ads: save/restore OA registers

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Tue, 29 Oct 2024 10:32:54 -0700, Lucas De Marchi wrote:
>

Hi Lucas,

> On Tue, Oct 29, 2024 at 10:15:54AM -0700, Ashutosh Dixit wrote:
> > On Tue, 29 Oct 2024 09:23:49 -0700, Lucas De Marchi wrote:
> >>
> >> On Wed, Oct 23, 2024 at 08:07:15PM +0000, Jonathan Cavitt wrote:
> >> > Several OA registers and allowlist registers were missing from the
> >> > save/restore list for GuC and could be lost during an engine reset.  Add
> >> > them to the list.
> >> >
> >> > v2:
> >> > - Fix commit message (Umesh)
> >> > - Add missing closes (Ashutosh)
> >> >
> >> > v3:
> >> > - Add missing fixes (Ashutosh)
> >> >
> >> > Closes: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/2249
> >> > Fixes: dd08ebf6c352 ("drm/xe: Introduce a new DRM driver for Intel GPUs")
> >> > Suggested-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> >> > Suggested-by: John Harrison <john.c.harrison@intel.com>
> >> > Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
> >> > CC: stable@vger.kernel.org # v6.11+
> >> > Acked-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> >> > Reviewed-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> >> > ---
> >> > drivers/gpu/drm/xe/xe_guc_ads.c | 14 ++++++++++++++
> >> > 1 file changed, 14 insertions(+)
> >> >
> >> > diff --git a/drivers/gpu/drm/xe/xe_guc_ads.c b/drivers/gpu/drm/xe/xe_guc_ads.c
> >> > index 4e746ae98888..a196c4fb90fc 100644
> >> > --- a/drivers/gpu/drm/xe/xe_guc_ads.c
> >> > +++ b/drivers/gpu/drm/xe/xe_guc_ads.c
> >> > @@ -15,6 +15,7 @@
> >> > #include "regs/xe_engine_regs.h"
> >> > #include "regs/xe_gt_regs.h"
> >> > #include "regs/xe_guc_regs.h"
> >> > +#include "regs/xe_oa_regs.h"
> >> > #include "xe_bo.h"
> >> > #include "xe_gt.h"
> >> > #include "xe_gt_ccs_mode.h"
> >> > @@ -740,6 +741,11 @@ static unsigned int guc_mmio_regset_write(struct xe_guc_ads *ads,
> >> >		guc_mmio_regset_write_one(ads, regset_map, e->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++);
> >>
> >> this is not the proper place. See drivers/gpu/drm/xe/xe_reg_whitelist.c.
> >
> > Yikes, this got merged yesterday.
> >
> >>
> >> The loop just before these added lines should be sufficient to go over
> >> all engine save/restore register and give them to guc.
> >
> > You probably mean this one?
> >
> >	xa_for_each(&hwe->reg_sr.xa, idx, entry)
> >		guc_mmio_regset_write_one(ads, regset_map, entry->reg, count++);
> >
> > But then how come this patch fixed GL #2249?
>
> it fixes, it just doesn't put it in the right place according to the
> driver arch. Whitelists should be in that other file so it shows up in
> debugfs, (/sys/kernel/debug/dri/*/*/register-save-restore), detect
> clashes when we try to add the same register, etc.

Sorry, still not following. OA registers are in xe_reg_whitelist.c (see
entries for "oa_reg_render" and "oa_reg_compute" in that file). To
whiteliest registers, the registers need to be added to NONPRIV
registers. This loop mentioned above:

	xa_for_each(&hwe->reg_sr.xa, idx, entry)
		guc_mmio_regset_write_one(ads, regset_map, entry->reg, count++);

seems to add the original OA registers to GuC save/restore list. But this
new code:

	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++);

Now adds the NONPRIV registers to GuC save/restore list (which fixes GL
#2249). So not sure what is not in the right place, adding to GuC
save/restore list is right here where the code is added.

Also we don't want to whitelist NONPRIV registers, we only want to add them
to GuC save/restore list.

Thanks.
--
Ashutosh