Re: [PATCH 2/3] drm/xe/xe_oa: Avoid checking and setting fields in the OA report

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

On Wed, 26 Aug 2026 16:30:31 -0700, Umesh Nerlige Ramappa wrote:
>
> On Mon, Aug 24, 2026 at 08:39:12AM -0700, Dixit, Ashutosh wrote:
> > On Fri, 21 Aug 2026 15:23:41 -0700, Umesh Nerlige Ramappa wrote:
> >>
> >> @@ -260,7 +224,7 @@ static bool mert_wa_14026633728(struct xe_oa_stream *s)
> >>  static bool xe_oa_buffer_check_unlocked(struct xe_oa_stream *stream)
> >>  {
> >>	u32 gtt_offset = xe_bo_ggtt_addr(stream->oa_buffer.bo);
> >> -	u32 tail, hw_tail, partial_report_size, available;
> >> +	u32 hw_tail, partial_report_size, available;
> >>	int report_size = stream->oa_buffer.format->size;
> >>	unsigned long flags;
> >>
> >> @@ -280,29 +244,7 @@ static bool xe_oa_buffer_check_unlocked(struct xe_oa_stream *stream)
> >>	/* Subtract partial amount off the tail */
> >>	hw_tail = xe_oa_circ_diff(stream, hw_tail, partial_report_size);
> >>
> >> -	tail = hw_tail;
> >> -
> >> -	/*
> >> -	 * Walk the stream backward until we find a report with report id and timestamp
> >> -	 * not 0. We can't tell whether a report has fully landed in memory before the
> >> -	 * report id and timestamp of the following report have landed.
> >> -	 *
> >> -	 * This is assuming that the writes of the OA unit land in memory in the order
> >> -	 * they were written.  If not : (╯°□°）╯︵ ┻━┻
> >> -	 */
> >> -	while (xe_oa_circ_diff(stream, tail, stream->oa_buffer.tail) >= report_size) {
> >> -		if (oa_report_id(stream, tail) || oa_timestamp(stream, tail))
> >> -			break;
> >> -
> >> -		tail = xe_oa_circ_diff(stream, tail, report_size);
> >> -	}
> >> -
> >> -	if (xe_oa_circ_diff(stream, hw_tail, tail) > report_size)
> >> -		drm_dbg(&stream->oa->xe->drm,
> >> -			"unlanded report(s) head=0x%x tail=0x%x hw_tail=0x%x\n",
> >> -			stream->oa_buffer.head, tail, hw_tail);
> >> -
> >> -	stream->oa_buffer.tail = tail;
> >> +	stream->oa_buffer.tail = hw_tail;
> >
> > This line is added here and then is modified in Patch 3. So maybe we should
> > not add this line here, just add the line in Patch 3? And just delete line
> > in this patch? So this patch will only contain code deletion.
>
> I left it in here intentionally. This represents a state where we still
> have functional OA but tail is just tracking hw_tail and we may run into
> the unlanded OA report bug. The subsequent patch fixes that bug.
>
> If that's messy, then I would just squash this and the next patch into
> one. Thoughts?

No, in that case leave as is, since it's easier to review.


>
> >
> > Also take care of the issue reported by sashiko:
> >
> > - [Low] The macro `oa_report_header_64bit` is left defined but is completely unused after its only callers were removed.
> >
> > Apart from these nits, this is:
> >
> > Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com>