Re: [Intel-gfx] [PATCH 1/3] drm/i915/perf: Subtract gtt_offset from hw_tail

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

On Tue, 12 Sep 2023 18:25:16 -0700, Umesh Nerlige Ramappa wrote:
>

Hi Umesh,

> On Fri, Sep 08, 2023 at 06:16:24PM -0700, Ashutosh Dixit wrote:
> > The code in oa_buffer_check_unlocked() is correct only if the OA buffer is
> > 16 MB aligned (which seems to be the case today in i915). However when the
> > 16 MB alignment is dropped, when we "Subtract partial amount off the tail",
> > the "& (OA_BUFFER_SIZE - 1)" operation in OA_TAKEN() will result in an
> > incorrect hw_tail value.
> >
> > Therefore hw_tail must be brought to the same base as head and read_tail
> > prior to OA_TAKEN by subtracting gtt_offset from hw_tail.
> >
> > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> > ---
> > drivers/gpu/drm/i915/i915_perf.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> > index 018f42fff4cc0..ec0fc2934045a 100644
> > --- a/drivers/gpu/drm/i915/i915_perf.c
> > +++ b/drivers/gpu/drm/i915/i915_perf.c
> > @@ -565,6 +565,7 @@ static bool oa_buffer_check_unlocked(struct i915_perf_stream *stream)
> >	partial_report_size %= report_size;
> >
> >	/* Subtract partial amount off the tail */
> > +	hw_tail -= gtt_offset;
> >	hw_tail = OA_TAKEN(hw_tail, partial_report_size);
>
> I see partial_report_size is a value in the 0 - report_size range and it
> may not have the gtt_offset added to it, so I guess the OA_TAKEN may result
> in a bad value, but I am not able to visualize what the specific issue
> is. Can you please provide an example with numbers?
>
> Also, slightly confused about the need for this patch. Are we dropping the
> 16 MB alignment for some reason?  If not, I suggest we can add this patch
> later with any series that drops it.

I found this issue when porting i915 OA code to XE (which doesn't have 16
MB alignment), I had to make this fix in XE in order to get things to work.

In any case, as you already pointed out, Patch 2 overrides Patch 1, so
let's just drop Patch 1.

Thanks.
--
Ashutosh