On Wed, 31 May 2023 16:56:34 -0700, Umesh Nerlige Ramappa wrote: > Hi Umesh, > Instead of aged_tail use an iterator that starts from the hw_tail and > goes backward until the oa_buffer.tail looking for valid reports. Hmm I don't think this description is correct. All this patch is doing is the following: a. s/aged_tail/tail/ b. s/tail/iter/ So basically I don't think we need this patch. All we want to do here is change the variable name aged_tail to something else (to completely remove the concept of aging from the OA code) but other changes such as name change to iter etc. is unnecessary. So I would just keep the patch simple and change the name aged_tail to advertized_tail or exported_tail or read_tail, because basically stream->oa_buffer.tail is the tail which the writer updates (or advertizes or exports) for the reader. So we only should rename aged_tail here, the other changes are not needed. We could even squash this change into Patch 1 or Patch 2, since it is really a trivial variable rename. Thanks. -- Ashutosh > > Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@xxxxxxxxx> > --- > drivers/gpu/drm/i915/i915_perf.c | 17 ++++++++--------- > 1 file changed, 8 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c > index beb1269422ca..39f5ab1911c8 100644 > --- a/drivers/gpu/drm/i915/i915_perf.c > +++ b/drivers/gpu/drm/i915/i915_perf.c > @@ -543,7 +543,7 @@ static bool oa_buffer_check_unlocked(struct i915_perf_stream *stream) > { > u32 gtt_offset = i915_ggtt_offset(stream->oa_buffer.vma); > int report_size = stream->oa_buffer.format->size; > - u32 head, tail, aged_tail; > + u32 head, tail, iter; > unsigned long flags; > bool pollin; > u32 hw_tail; > @@ -567,15 +567,14 @@ static bool oa_buffer_check_unlocked(struct i915_perf_stream *stream) > /* Subtract partial amount off the tail */ > hw_tail = OA_TAKEN(hw_tail, partial_report_size); > > - > /* NB: The head we observe here might effectively be a little > * out of date. If a read() is in progress, the head could be > * anywhere between this head and stream->oa_buffer.tail. > */ > head = stream->oa_buffer.head - gtt_offset; > - aged_tail = stream->oa_buffer.tail - gtt_offset; > + tail = stream->oa_buffer.tail - gtt_offset; > > - tail = hw_tail; > + iter = hw_tail; > > /* Walk the stream backward until we find a report with report > * id and timestmap not at 0. Since the circular buffer pointers > @@ -588,23 +587,23 @@ static bool oa_buffer_check_unlocked(struct i915_perf_stream *stream) > * memory in the order they were written to. > * If not : (╯°□°)╯︵ ┻━┻ > */ > - while (OA_TAKEN(tail, aged_tail) >= report_size) { > - void *report = stream->oa_buffer.vaddr + tail; > + while (OA_TAKEN(iter, tail) >= report_size) { > + void *report = stream->oa_buffer.vaddr + iter; > > if (oa_report_id(stream, report) || > oa_timestamp(stream, report)) > break; > > - tail = (tail - report_size) & (OA_BUFFER_SIZE - 1); > + iter = (iter - report_size) & (OA_BUFFER_SIZE - 1); > } > > - if (OA_TAKEN(hw_tail, tail) > report_size && > + if (OA_TAKEN(hw_tail, iter) > report_size && > __ratelimit(&stream->perf->tail_pointer_race)) > drm_notice(&stream->uncore->i915->drm, > "unlanded report(s) head=0x%x tail=0x%x hw_tail=0x%x\n", > head, tail, hw_tail); > > - stream->oa_buffer.tail = gtt_offset + tail; > + stream->oa_buffer.tail = gtt_offset + iter; > > pollin = OA_TAKEN(stream->oa_buffer.tail - gtt_offset, > stream->oa_buffer.head - gtt_offset) >= report_size; > -- > 2.36.1 >