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

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

On Thu, 19 Dec 2024 12:29:30 -0800, Harish Chegondi wrote:
>
> 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?

I do everything manually. However, looking at the patches in:

https://patchwork.freedesktop.org/series/137870/

Matt Brost seems to be doing what you are doing, so that should be ok too.

> >
> > > >
> > > > > 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

Ah ok. And poll has events too. Though we need to cover both the
non-blocking as well as blocking read cases. Blocking read does not set
POLLIN.

>
> > 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.
> > >