On Tue, Oct 29, 2024 at 12:33:13PM -0700, Matt Roper wrote:
On Tue, Oct 29, 2024 at 12:32:54PM -0500, Lucas De Marchi wrote:
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@xxxxxxxxx>
> > > Suggested-by: John Harrison <john.c.harrison@xxxxxxxxx>
> > > Signed-off-by: Jonathan Cavitt <jonathan.cavitt@xxxxxxxxx>
> > > CC: stable@xxxxxxxxxxxxxxx # v6.11+
> > > Acked-by: Ashutosh Dixit <ashutosh.dixit@xxxxxxxxx>
> > > Reviewed-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@xxxxxxxxx>
> > > ---
> > > 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.
Also, this patch failed pre-merge BAT since it added new regset entries
that we never actually allocated storage space for. Now that it's been
applied, we're seeing CI failures on lots of tests from this:
https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/3295
Sorry, didn't fully understand how this works in Xe KMD.
Does this mean that we should just add stuff into the
register_whitelist[] array in xe_reg_whitelist.c OR should this be added
to hwe->reg_sr using the xe_rtp_process_to_sr() interface? What's the
difference between the 2 ways or when to use which one?
Thanks,
Umesh
Matt
Lucas De Marchi
>
> Ashutosh
--
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation
