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

[Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>]

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@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.
>
>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