Re: [PATCH 2/2] drm/xe/oa: Allow subsequent OA streams to be opened on an exec_queue

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

On Fri, 08 Nov 2024 15:42:40 -0800, Cavitt, Jonathan wrote:
>

Hi Jonathan,

> -----Original Message-----
> From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of Ashutosh Dixit
> Sent: Friday, November 8, 2024 3:06 PM
> To: intel-xe@lists.freedesktop.org
> Cc: Nerlige Ramappa, Umesh <umesh.nerlige.ramappa@intel.com>
> Subject: [PATCH 2/2] drm/xe/oa: Allow subsequent OA streams to be opened on an exec_queue
> >
> > The restriction that OA not be enabled on an active exec queue only applies
> > to the first OA stream opened on the exec queue. Subsequent OA streams can
> > be opened on the exec queue since this will not toggle the
> > OAC_CONTEXT_ENABLE bit.
> >
> > Fixes: 2f4a730fcd2d ("drm/xe/oa: Add OAR support")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_exec_queue_types.h | 2 ++
> >  drivers/gpu/drm/xe/xe_oa.c               | 8 ++++++--
> >  2 files changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/xe/xe_exec_queue_types.h b/drivers/gpu/drm/xe/xe_exec_queue_types.h
> > index 1158b6062a6cd..6e3311c22404e 100644
> > --- a/drivers/gpu/drm/xe/xe_exec_queue_types.h
> > +++ b/drivers/gpu/drm/xe/xe_exec_queue_types.h
> > @@ -142,6 +142,8 @@ struct xe_exec_queue {
> >	u64 tlb_flush_seqno;
> >	/** @hw_engine_group_link: link into exec queues in the same hw engine group */
> >	struct list_head hw_engine_group_link;
> > +	/** @oa: this exec_queue is used for OA (OAR/OAC) */
> > +	bool oa;
>
> Opening the first OA stream sets the OAC_CONTEXT_ENABLE bit.  Would it be possible
> to read that bit instead of adding an additional bool to the exec queue?
>
> I won't block on this, just asking.

It was indeed possible, thanks for the suggestion. I have implemented it in
v2 of Patch 1 and dropped this Patch 2, since it doesn't seem to be needed
if we read the bit directly. Note though the register is read from the
context image, it is not an mmio register.

I have also dropped your R-b from Patch 1, since the patch is significantly
different now (after addressing both yours and Matt Brost's reviews). So if
you could please take a quick look again that would be great.

Thanks.
Ashutosh

> -Jonathan Cavitt
>
> >	/** @lrc: logical ring context for this exec queue */
> >	struct xe_lrc *lrc[] __counted_by(width);
> >  };
> > diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c
> > index b0692b8ca0a3d..53d2946ea3052 100644
> > --- a/drivers/gpu/drm/xe/xe_oa.c
> > +++ b/drivers/gpu/drm/xe/xe_oa.c
> > @@ -1915,6 +1915,9 @@ static int xe_oa_stream_open_ioctl_locked(struct xe_oa *oa,
> >		goto err_disable;
> >	}
> >
> > +	if (stream->exec_q && !stream->exec_q->oa)
> > +		stream->exec_q->oa = true;
> > +
> >	/* Hold a reference on the drm device till stream_fd is released */
> >	drm_dev_get(&stream->oa->xe->drm);
> >
> > @@ -2068,9 +2071,10 @@ int xe_oa_stream_open_ioctl(struct drm_device *dev, u64 data, struct drm_file *f
> >		/*
> >		 * Disallow OA from being enabled on active exec_queue's. Enabling OA toggles
> >		 * the OAC_CONTEXT_ENABLE bit in CTXT_SR_CTL register, which changes the size
> > -		 * and layout of the underlying HW context image and can cause hangs.
> > +		 * and layout of the underlying HW context image and can cause hangs. This
> > +		 * restriction holds only for the first OA stream opened on the exec queue.
> >		 */
> > -		if (XE_IOCTL_DBG(oa->xe, exec_queue_enabled(param.exec_q))) {
> > +		if (XE_IOCTL_DBG(oa->xe, !param.exec_q->oa && exec_queue_enabled(param.exec_q))) {
> >			ret = -EADDRINUSE;
> >			goto err_exec_q;
> >		}
> > --
> > 2.41.0
> >
> >