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