Re: FW: [01/17] drm/xe/perf/uapi: "Perf" layer to support multiple perf counter stream types

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

On Mon, 18 Dec 2023 19:04:32 -0800, Dixit, Ashutosh wrote:
>
> On Sun, 17 Dec 2023 00:45:52 -0800, Guy Zadicario wrote:
> >
> > >diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
> > >index eb03a49c17a13..3539e0781d700 100644
> > >--- a/include/uapi/drm/xe_drm.h
> > >+++ b/include/uapi/drm/xe_drm.h
> >
> > ..
> >
> > >+
> > >+/**
> > >+ * enum drm_xe_perf_ioctls - Perf fd ioctl's
> > >+ */
> > >+enum drm_xe_perf_ioctls {
> > >+	/** @DRM_XE_PERF_IOCTL_ENABLE: Enable data capture for a stream */
> > >+	DRM_XE_PERF_IOCTL_ENABLE = _IO('i', 0x0),
> > >+
> > >+	/** @DRM_XE_PERF_IOCTL_DISABLE: Disable data capture for a stream */
> > >+	DRM_XE_PERF_IOCTL_DISABLE = _IO('i', 0x1),
> > >+
> > >+	/** @DRM_XE_PERF_IOCTL_CONFIG: Change stream configuration */
> > >+	DRM_XE_PERF_IOCTL_CONFIG = _IO('i', 0x2),
> > >+};
> > >+
> >
>
> Hi Guy,
>
> Let's discuss this a bit and then decide what to do.
>
> > It apears that in the hwtrace PERF stream I need to pass some arguments
> > in the ENABLE/DISABLE ioctls. This is required since firmware will
> > implement the enable/disable functionality.  I would suggest to use
> > different enums for the stream ioctls, you can rename drm_xe_perf_ioctls
> > to drm_xe_oa_ioctls and I will define drm_xe_hwtrace_ioctls.
>
> I meant the above ioctls to be ioctl's which could be common between the
> different stream types. A couple of points:
>
> * The above ioctl's do not prevent you from passing in any data. E.g. with:
>
>   static long xe_hwtrace_ioctl(struct file *file, unsigned int cmd, unsigned long arg);
>
>   If 'arg' is a pointer, any data you want can be passed into any of the
>   ioctls (enable/disable/config). So there are no restrictions. Each stream
>   type is free to do what it needs.
>
> * Each stream type is free to add whatever ioctl's it needs, or ignore
>   these common ioctls. E.g. this is possible for HWTRACE:
>
>	#define DRM_XE_PERF_IOCTL_ENABLE   _IO('i', 0x0)
>	#define DRM_XE_PERF_IOCTL_DISABLE  _IO('i', 0x1)
>	#define DRM_XE_PERF_IOCTL_CONFIG   _IO('i', 0x2)
>	#define DRM_XE_HWTRACE_IOCTL_ABC   _IO('i', 0x3)
>	#define DRM_XE_HWTRACE_IOCTL_XYZ   _IO('j', 0x4)
>	#define DRM_XE_HWTRACE_IOCTL_XYZ1  _IO('q', 0x9)
>
> * Currently we expect to have 3 stream types: OA, EuStall and HwTrace. The
>   above ioctl's are used by both OA and Eustall (and also it appears
>   HwTrace). EuStall just uses enable/disable, it doesn't have 'config'.
>
> > I'd also like to add another stream ioctl to retrieve the stream status
> > and avaialable bytes.
>
> So I believe I have already answered this above.
>
> Maybe having each stream type (OA, EuStall, HwTrace) have its own separate
> ioctl's is cleaner, and we can switch to that if people want that. But I
> think the current scheme doesn't prevent the sort of operation you want.

Hi Guy,

Actually, thinking a bit more about this, you could just add a new
DRM_XE_PERF_IOCTL_STATUS (rather than say DRM_XE_HWTRACE_IOCTL_STATUS) if
neeeded. We can keep the ioctl's common at the PERF layer, but individual
stream types can implement only what they need.

Thanks.
--
Ashutosh