[PATCH v6 2/7] drm/xe/uapi: Introduce API for EU stall sampling

[Harish Chegondi]

On Thu, Dec 19, 2024 at 08:27:56AM -0800, Dixit, Ashutosh wrote:
> On Wed, 18 Dec 2024 14:51:34 -0800, Harish Chegondi wrote:
> >
> > On Tue, Dec 17, 2024 at 12:35:15PM -0800, Dixit, Ashutosh wrote:
> > > On Tue, 17 Dec 2024 01:46:52 -0800, Harish Chegondi wrote:
> > > >
> > >
> > > Hi Harish,
> > >
> > > Only reviewing the uapi once again.
> > >
> > > > A user space consumer for this feature is Mesa.
> > > >
> > > > Mesa PR: https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/30142
> > >
> > > Mesa PR should be in the cover letter, not in the patch itself. And we'll
> > > need to eventually show that the Mesa PR is consuming all aspects of the
> > > uapi being introduced.
> > Okay, will fix in the next patch series. Mesa PR still need some uAPI
> > changes I made in this patch series.
> > >
> > > >
> > > > v6: Change the input sampling rate to GPU cycles instead of
> > > >     GPU cycles multiplier.
> > >
> > > Note that if your series is v6 each patch in the series is not necessarily
> > > v6. A patch can be v2 e.g. So you should capture the version and changelog
> > > of each patch separately.
> > Makes sense. But how would the reviewers know if a patch v2 in a series
> > v6 has been updated?
> 
> They can check, say in v7 if the patch has gone from v2 to v3. And anyway
> reviewers need to be aware of what is going on. There should be no
> significant changes to the patch after a R-b, otherwise typically the patch
> will change and it versions increment.
> 
> With what you are doing, the patch will go from v6 to v7 even if there are
> no changes to the patch.
When I do a git format-patch, I specify the --subject-prefix="PATCH version".
Since this is a patch series, all the patches in the series will be
assigned the new version even though I don't change some of the patches
in the series. Is there a way I can specify the version for individual patches?
> 
> > >
> > > > diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
> > > > index f62689ca861a..4ee3b04a1bb5 100644
> > > > --- a/include/uapi/drm/xe_drm.h
> > > > +++ b/include/uapi/drm/xe_drm.h
> > > > @@ -1397,6 +1397,8 @@ struct drm_xe_wait_user_fence {
> > > >  enum drm_xe_observation_type {
> > > >	/** @DRM_XE_OBSERVATION_TYPE_OA: OA observation stream type */
> > > >	DRM_XE_OBSERVATION_TYPE_OA,
> > > > +	/** @DRM_XE_OBSERVATION_TYPE_EU_STALL: EU stall sampling observation stream type */
> > > > +	DRM_XE_OBSERVATION_TYPE_EU_STALL,
> > > >  };
> > > >
> > > >  /**
> > > > @@ -1729,6 +1731,45 @@ struct drm_xe_oa_stream_info {
> > > >	__u64 reserved[3];
> > > >  };
> > > >
> > > > +/**
> > > > + * enum drm_xe_eu_stall_property_id - EU stall sampling input property ids.
> > > > + *
> > > > + * These properties are passed to the driver at open as a chain of
> > > > + * @drm_xe_ext_set_property structures with @property set to these
> > > > + * properties' enums and @value set to the corresponding values of these
> > > > + * properties. @drm_xe_user_extension base.name should be set to
> > > > + * @DRM_XE_EU_STALL_EXTENSION_SET_PROPERTY.
> > > > + *
> > > > + * With the file descriptor obtained from open, user space must enable
> > > > + * the EU stall stream fd with @DRM_XE_OBSERVATION_IOCTL_ENABLE before
> > > > + * calling read(). read() returns number of bytes of EU stall data read
> > > > + * from the EU stall data buffer or an error. One of the errors returned
> > >
> > > No need to explain what read() returns, read() is a system call, user can
> > > read the read man page.
> > >
> > > > + * from read is -EIO which indicates HW dropped data due to full buffer.
> > >
> > > Just say "EIO errno from read() indicates data loss due to buffer
> > > overflow".
> > >
> > > Also, -EIO is not returned to userspace, errno is set for userspace.
> > >
> > > > + *
> > > > + */
> > > > +enum drm_xe_eu_stall_property_id {
> > > > +#define DRM_XE_EU_STALL_EXTENSION_SET_PROPERTY		0
> > > > +	/**
> > > > +	 * @DRM_XE_EU_STALL_PROP_GT_ID: GT ID of the GT on which
> > >
> > > @gt_id
> > >
> > > > +	 * EU stall data will be captured.
> > > > +	 */
> > > > +	DRM_XE_EU_STALL_PROP_GT_ID = 1,
> > > > +
> > > > +	/**
> > > > +	 * @DRM_XE_EU_STALL_PROP_SAMPLE_RATE: Sampling rate
> > > > +	 * in GPU cycles. Valid values are:
> > > > +	 * 251, 251x2, 251x3, 251x4, 251x5, 251x6 and 251x7.
> > >
> > > This 251 stuff needs to go, as was already mentioned the last
> > > time. Something like:
> > >
> > > "@DRM_XE_EU_STALL_PROP_SAMPLE_RATE: Sampling rate in GPU cycles, from
> > > @sampling_rates in struct @drm_xe_query_eu_stall".
> > Will change.
> > >
> > > > +	 */
> > > > +	DRM_XE_EU_STALL_PROP_SAMPLE_RATE,
> > > > +
> > > > +	/**
> > > > +	 * @DRM_XE_EU_STALL_PROP_EVENT_REPORT_COUNT: Minimum number of
> > > > +	 * EU stall data rows to be present in the kernel buffer for
> > > > +	 * poll() to set POLLIN (data present).
> > > > +	 */
> > > > +	DRM_XE_EU_STALL_PROP_EVENT_REPORT_COUNT,
> > >
> > > We called this DRM_XE_OA_PROPERTY_WAIT_NUM_REPORTS for OA. So maybe
> > > DRM_XE_EU_STALL_PROP_WAIT_NUM_REPORTS? Or WAIT_REPORT_COUNT? Not sure what
> > > EVENT is referring to?
> > Here is EVENT is referring to POLLIN (new EU stall data in the buffer)
> > from poll(). This property would specify the minimum EU stall data
> > records to be present in the buffer for poll() to set POLLIN.
> 
> Note that above I said use EIO errno, not -EIO return code? The reason for
> that is that this is userspace facing file, to be consumed by
> userspace. Userspace doesn't know what POLLIN/EVENT mean, those things are

Userspace knows POLLIN - https://man7.org/linux/man-pages/man2/poll.2.html

> internal to the kernel implemenation and kernel API's. So these need to be
> changed too. Here is the example from OA for this property:
> 
> 	/**
> 	 * @DRM_XE_OA_PROPERTY_WAIT_NUM_REPORTS: Number of reports to wait
> 	 * for before unblocking poll or read
> 	 */
> 	DRM_XE_OA_PROPERTY_WAIT_NUM_REPORTS,
> 
> So here there is no mention of kernel implementation/API's, only about user
> threads getting unblocked. And anyway there are no events to userspace
> kernel is sending.
If I remember correctly, the name event report count was suggested by
the user space folks. I can change it so it is consistent with the term
used in OA to DRM_XE_EU_STALL_PROP_WAIT_NUM_REPORTS.
> 
> > >
> > > > +};systemctl start gdm3
> >
> > > > +
> > > >  #if defined(__cplusplus)
> > > >  }
> > > >  #endif
> > > > --
> > > > 2.47.0
> > > >
> > >
> > > Ashutosh
> > Thank you
> > Harish.
> >