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
>
