Re: [PATCH] drm/xe/oa: Set stream->pollin in xe_oa_buffer_check_unlocked

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

On Wed, 15 Jan 2025 14:57:24 -0800, Cavitt, Jonathan wrote:
>
> -----Original Message-----
> From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of Ashutosh Dixit
> Sent: Wednesday, January 15, 2025 2:20 PM
> To: intel-xe@lists.freedesktop.org
> Cc: Nerlige Ramappa, Umesh <umesh.nerlige.ramappa@intel.com>
> Subject: [PATCH] drm/xe/oa: Set stream->pollin in xe_oa_buffer_check_unlocked
> >
> > We rely on stream->pollin to decide whether or not to block during
> > poll/read calls. However, currently there are blocking read code paths
> > which don't even set stream->pollin. The best place to consistently set
> > stream->pollin for all code paths is therefore to set it in
> > xe_oa_buffer_check_unlocked.
> >
> > Fixes: e936f885f1e9 ("drm/xe/oa/uapi: Expose OA stream fd")
> > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
>
> Minor concern below, but I don't think it's a problem, so I won't block on it.
> Reviewed-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
>
> > ---
> >  drivers/gpu/drm/xe/xe_oa.c | 9 +++------
> >  1 file changed, 3 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c
> > index eeb96b5f49e2a..e27c1752aa57c 100644
> > --- a/drivers/gpu/drm/xe/xe_oa.c
> > +++ b/drivers/gpu/drm/xe/xe_oa.c
> > @@ -237,7 +237,6 @@ static bool xe_oa_buffer_check_unlocked(struct xe_oa_stream *stream)
> >	u32 tail, hw_tail, partial_report_size, available;
> >	int report_size = stream->oa_buffer.format->size;
> >	unsigned long flags;
> > -	bool pollin;
> >
> >	spin_lock_irqsave(&stream->oa_buffer.ptr_lock, flags);
> >
> > @@ -282,11 +281,11 @@ static bool xe_oa_buffer_check_unlocked(struct xe_oa_stream *stream)
> >	stream->oa_buffer.tail = tail;
> >
> >	available = xe_oa_circ_diff(stream, stream->oa_buffer.tail, stream->oa_buffer.head);
> > -	pollin = available >= stream->wait_num_reports * report_size;
> > +	stream->pollin = available >= stream->wait_num_reports * report_size;
>
> It looks like we call xe_oa_buffer_check_unlocked in two places.
>
> The first is below, where we call it during xe_oa_poll_check_timer_cb.  I see we're
> operating on the stream->pollin value here because we're migrating the
> "stream->polling = true" code from xe_oa_poll_check_timer_cb into the
> xe_oa_buffer_check_unlocked function.
>
> However, the second location is during xe_oa_wait_unlocked.  There, we perform
> xe_oa_buffer_check_unlocked in a wait_event_interruptible loop.  Is it safe to be
> modifying the stream->pollin value in that case?
>
> I'm sure it's fine, but it's something to consider.

Yes it's fine. Actually it is one of the cases where stream->pollin is not
set (when wait_event_interruptible does *not* block). Because currently we
are setting stream->pollin only when OA data is detected from the timer
callback (when we are blocked and unblocked by the timer). But we should
really be setting stream->pollin in all cases (i.e. also in those cases
where we don't block), which is what this patch is doing.

So one of those cases where stream->pollin is not set is when
wait_event_interruptible does not block. Another potential case is what
Umesh is thinking about and might post.

Thanks,
Ashutosh

> >
> >	spin_unlock_irqrestore(&stream->oa_buffer.ptr_lock, flags);
> >
> > -	return pollin;
> > +	return stream->pollin;
> >  }
> >
> >  static enum hrtimer_restart xe_oa_poll_check_timer_cb(struct hrtimer *hrtimer)
> > @@ -294,10 +293,8 @@ static enum hrtimer_restart xe_oa_poll_check_timer_cb(struct hrtimer *hrtimer)
> >	struct xe_oa_stream *stream =
> >		container_of(hrtimer, typeof(*stream), poll_check_timer);
> >
> > -	if (xe_oa_buffer_check_unlocked(stream)) {
> > -		stream->pollin = true;
> > +	if (xe_oa_buffer_check_unlocked(stream))
> >		wake_up(&stream->poll_wq);
> > -	}
> >
> >	hrtimer_forward_now(hrtimer, ns_to_ktime(stream->poll_period_ns));
> >
> > --
> > 2.47.1
> >
> >