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

[Dixit, Ashutosh]

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.

Thanks.
--
Ashutosh