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

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

[Guy Zadicario]

..

>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 Ashutosh,
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'd also like to add another stream ioctl to retrieve the stream status and avaialable bytes.

Thanks,
Guy.

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

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

[Dixit, Ashutosh]

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