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

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

On Mon, 28 Oct 2024 13:38:29 -0700, Umesh Nerlige Ramappa wrote:
>
> On Mon, Oct 28, 2024 at 09:36:32AM -0700, Dixit, Ashutosh wrote:
> > On Wed, 23 Oct 2024 13:07:15 -0700, Jonathan Cavitt wrote:
> >>
> >
> > Hi Umesh,
> >
> >> @@ -748,6 +754,14 @@ static unsigned int guc_mmio_regset_write(struct xe_guc_ads *ads,
> >>		}
> >>	}
> >>
> >> +	guc_mmio_regset_write_one(ads, regset_map, EU_PERF_CNTL0, count++);
> >> +	guc_mmio_regset_write_one(ads, regset_map, EU_PERF_CNTL1, count++);
> >> +	guc_mmio_regset_write_one(ads, regset_map, EU_PERF_CNTL2, count++);
> >> +	guc_mmio_regset_write_one(ads, regset_map, EU_PERF_CNTL3, count++);
> >> +	guc_mmio_regset_write_one(ads, regset_map, EU_PERF_CNTL4, count++);
> >> +	guc_mmio_regset_write_one(ads, regset_map, EU_PERF_CNTL5, count++);
> >> +	guc_mmio_regset_write_one(ads, regset_map, EU_PERF_CNTL6, count++);
> >
> > I am trying to understand how this works. So these registers are
> > saved/restored by GuC because they are not part of HW context image
>
> correct.
>
> > and that is why GuC needs to do the save/restore?
>
> yes, only if GuC performs an engine reset
>
> > Bspec 46458/56839 do seem to
> > be saying that these registers are context saved/restored? If that is
> > indeed true (though not sure), do they need to be here?
>
> For pre-gen12 they were part of the engine context image, but not from
> gen12 onwards. From gen12, they are in the power context image.
>
> These were added because users were seeing the EuStall and EuActive
> counters zeroed out during OA use case. GuC was doing an engine reset for
> some reason and that was resetting these registers. Once we added it here
> (so GuC would save restore these), the counters had correct values.

Hi Umesh, thanks for the explanation, yes let's just leave these here.

Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com>