Re: [PATCH v2 3/4] drm/xe/oa: Provide OA status through status ioctl for mmap interface

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

On Thu, 25 Jun 2026 16:16:33 -0700, Umesh Nerlige Ramappa wrote:
>

Hi Umesh,

> On Tue, Jun 16, 2026 at 06:54:21PM -0700, Ashutosh Dixit wrote:
> > When only the OA mmap interface is in use, OA status can be provided
> > through the status ioctl. This enables us to stop whitelisting the OA
> > status register, thereby saving one nonpriv slot per OA unit.
> >
> > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> > ---
> > drivers/gpu/drm/xe/xe_oa.c       | 7 +++++++
> > drivers/gpu/drm/xe/xe_oa_types.h | 3 +++
> > 2 files changed, 10 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c
> > index 9fbd21b0ef974..985be10855c33 100644
> > --- a/drivers/gpu/drm/xe/xe_oa.c
> > +++ b/drivers/gpu/drm/xe/xe_oa.c
> > @@ -544,6 +544,7 @@ static int __xe_oa_read(struct xe_oa_stream *stream, char __user *buf,
> >	/* Only clear our bits to avoid side-effects */
> >	stream->oa_status = xe_mmio_rmw32(&stream->gt->mmio, __oa_regs(stream)->oa_status,
> >					  OASTATUS_RELEVANT_BITS, 0);
> > +	stream->oa_status_read = true;
> >	/*
> >	 * Signal to userspace that there is non-zero OA status to read via
> >	 * @DRM_XE_OBSERVATION_IOCTL_STATUS observation stream fd ioctl
> > @@ -1603,6 +1604,12 @@ static long xe_oa_status_locked(struct xe_oa_stream *stream, unsigned long arg)
> >	struct drm_xe_oa_stream_status status = {};
> >	void __user *uaddr = (void __user *)arg;
> >
> > +	/* Provide oa_status through the status ioctl, when only mmap() interface is used */
> > +	if (!stream->oa_status_read)
> > +		stream->oa_status = xe_mmio_rmw32(&stream->gt->mmio, __oa_regs(stream)->oa_status,
> > +						  OASTATUS_RELEVANT_BITS, 0);
> > +	stream->oa_status_read = false;
> > +
>
> This is asymmetric especially since we cannot control a user calling mmap
> and read from the same application. I would suggest that we avoid clearing
> the register status bits in _xe_oa_read.  read() will continue to return
> EIO until the status ioctl is called. In the status ioctl, we always read
> the latest value from the HW REG, return that and then clear the register
> bits.

This is the text in xe_drm.h:
"
 * %DRM_XE_OBSERVATION_IOCTL_STATUS observation stream fd ioctl. Userspace can
 * call the ioctl to query stream status in response to EIO errno from
 * observation fd read().
"

So, at least in my mind, calling the status ioctl was always optional
(indicated by "Userspace *can* call") allowing userspace to ignore the EIO
return and still be able to continue. So if we do what you suggest above,
even our own IGT's will need to be fixed, because they can hang, if EIO is
returned continuously.

There is no perfect solution to this problem and that is why I came up with
the scheme in this patch, which to me seems to be good enough and works for
all common use cases.

Thanks.
--
Ashutosh

> >	/* Map from register to uapi bits */
> >	if (stream->oa_status & OASTATUS_REPORT_LOST)
> >		status.oa_status |= DRM_XE_OASTATUS_REPORT_LOST;
> > diff --git a/drivers/gpu/drm/xe/xe_oa_types.h b/drivers/gpu/drm/xe/xe_oa_types.h
> > index 3d9ec8490899c..a84456540a13c 100644
> > --- a/drivers/gpu/drm/xe/xe_oa_types.h
> > +++ b/drivers/gpu/drm/xe/xe_oa_types.h
> > @@ -244,6 +244,9 @@ struct xe_oa_stream {
> >	/** @oa_status: temporary storage for oa_status register value */
> >	u32 oa_status;
> >
> > +	/** @oa_status_read: true when read() interface is used to read oa_status */
> > +	bool oa_status_read;
> > +
> >	/** @no_preempt: Whether preemption and timeslicing is disabled for stream exec_q */
> >	u32 no_preempt;
> >
> > --
> > 2.54.0
> >