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

[Guy Zadicario]

types

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

Right, I know that technically, it works if I pass arguments.
Just a matter of better documentation and make it clear.
We can work with any of the options below, however the 2nd option
really make it clear what argument needs to be passed.

DRM_XE_PERF_IOCTL_ENABLE = _IO('i', 0x0)
DRM_XE_HWTRACE_IOCTL_ENABLE = _IOW('i', 0x0, struct drm_xe_hwtrace_enable_disable)

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

We can do this, and I am OK with what will be decided ...
The question is what do we gain by sharing the same ioctls.

Another option is that OA and EuStall will share drm_xe_perf_ioctls and
hwtrace will define a different set: drm_xe_hwtrace_ioctls.

Thanks,
Guy.