| Message ID | 20230909011626.1643734-2-ashutosh.dixit@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/perf: A few fixes and enhancements | expand |
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. Thanks, Umesh > > /* NB: The head we observe here might effectively be a little >-- >2.41.0 >
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
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); /* NB: The head we observe here might effectively be a little
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(+)