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

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

On Sat, 08 Jun 2024 03:44:24 -0700, Michal Wajdeczko wrote:
>

Hi Michal,

> Hi Ashutosh,
>
> Please find few late btw comments below.

Np, thanks for the review.

You can see a consolidated (or full tree) diff of the changes in response
to your comments (in v17) here:

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

v17 itself can be seen here:

https://patchwork.freedesktop.org/series/121084/#rev17

>
> On 07.06.2024 22:43, Ashutosh Dixit wrote:
> > In Xe, the plan is to support multiple types of perf counter streams (OA is
> > only one type of these streams). Rather than introduce NxM ioctls for
> > these (N perf streams with M ioctl's per perf stream), we decide to
> > multiplex these (N different stream types and the M ops for each of these
> > stream types) through a single PERF ioctl. This multiplexing is the purpose
> > of the PERF layer.
> >
> > In addition to PERF DRM ioctl's, another set of ioctl's on the PERF fd are
> > defined. These are expected to be common to different PERF stream types and
> > therefore defined at the PERF layer itself.
> >
> > v2: Add param_size to 'struct drm_xe_perf_param' (Umesh)
> > v3: Rename 'enum drm_xe_perf_ops' to
> >     'enum drm_xe_perf_ioctls' (Guy Zadicario)
> >     Add DRM_ prefix to ioctl names to indicate uapi names
> > v4: Add 'enum drm_xe_perf_op' previously missed out (Guy Zadicario)
> > v5: Squash the ops and PERF layer patches into a single patch (Umesh)
> >     Remove param_size from struct 'drm_xe_perf_param' (Umesh)
> > v6: Add DRM_XE_PERF_IOCTL_STATUS
> > v7: Add DRM_XE_PERF_IOCTL_INFO
> >
> > Acked-by: Guy Zadicario <gzadicario@habana.ai>
> > Acked-by: José Roberto de Souza <jose.souza@intel.com>
> > Reviewed-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> > ---
> >  drivers/gpu/drm/xe/Makefile    |  1 +
> >  drivers/gpu/drm/xe/xe_device.c |  2 ++
> >  drivers/gpu/drm/xe/xe_perf.c   | 21 ++++++++++++
> >  drivers/gpu/drm/xe/xe_perf.h   | 16 +++++++++
> >  include/uapi/drm/xe_drm.h      | 63 ++++++++++++++++++++++++++++++++++
> >  5 files changed, 103 insertions(+)
> >  create mode 100644 drivers/gpu/drm/xe/xe_perf.c
> >  create mode 100644 drivers/gpu/drm/xe/xe_perf.h
> >
> > diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
> > index 0c3e3adabb27..7a91b318efa5 100644
> > --- a/drivers/gpu/drm/xe/Makefile
> > +++ b/drivers/gpu/drm/xe/Makefile
> > @@ -95,6 +95,7 @@ xe-y += xe_bb.o \
> >	xe_pat.o \
> >	xe_pci.o \
> >	xe_pcode.o \
> > +	xe_perf.o \
> >	xe_pm.o \
> >	xe_preempt_fence.o \
> >	xe_pt.o \
> > diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> > index 0ff95a0ea5ea..31ab04b4bd5c 100644
> > --- a/drivers/gpu/drm/xe/xe_device.c
> > +++ b/drivers/gpu/drm/xe/xe_device.c
> > @@ -43,6 +43,7 @@
> >  #include "xe_module.h"
> >  #include "xe_pat.h"
> >  #include "xe_pcode.h"
> > +#include "xe_perf.h"
> >  #include "xe_pm.h"
> >  #include "xe_query.h"
> >  #include "xe_sriov.h"
> > @@ -140,6 +141,7 @@ static const struct drm_ioctl_desc xe_ioctls[] = {
> >			  DRM_RENDER_ALLOW),
> >	DRM_IOCTL_DEF_DRV(XE_WAIT_USER_FENCE, xe_wait_user_fence_ioctl,
> >			  DRM_RENDER_ALLOW),
> > +	DRM_IOCTL_DEF_DRV(XE_PERF, xe_perf_ioctl, DRM_RENDER_ALLOW),
> >  };
> >
> >  static long xe_drm_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> > diff --git a/drivers/gpu/drm/xe/xe_perf.c b/drivers/gpu/drm/xe/xe_perf.c
> > new file mode 100644
> > index 000000000000..a130076b59aa
> > --- /dev/null
> > +++ b/drivers/gpu/drm/xe/xe_perf.c
> > @@ -0,0 +1,21 @@
> > +// SPDX-License-Identifier: MIT
> > +/*
> > + * Copyright © 2023 Intel Corporation
>
> maybe it's time to use 2023-2024 ?

Done.

>
> > + */
> > +
> > +#include <linux/errno.h>
> > +
> > +#include "xe_perf.h"
> > +
> > +int xe_perf_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
> > +{
> > +	struct drm_xe_perf_param *arg = data;
> > +
> > +	if (arg->extensions)
> > +		return -EINVAL;
> > +
> > +	switch (arg->perf_type) {
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > diff --git a/drivers/gpu/drm/xe/xe_perf.h b/drivers/gpu/drm/xe/xe_perf.h
> > new file mode 100644
> > index 000000000000..254cc7cf49fe
> > --- /dev/null
> > +++ b/drivers/gpu/drm/xe/xe_perf.h
> > @@ -0,0 +1,16 @@
> > +/* SPDX-License-Identifier: MIT */
> > +/*
> > + * Copyright © 2023 Intel Corporation
> > + */
> > +
> > +#ifndef _XE_PERF_H_
> > +#define _XE_PERF_H_
> > +
> > +#include <drm/xe_drm.h>
>
> this is not needed here as you already have required forward decls

Done, moved to .c

>
> > +
> > +struct drm_device;
> > +struct drm_file;
> > +
> > +int xe_perf_ioctl(struct drm_device *dev, void *data, struct drm_file *file);
> > +
> > +#endif
> > diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
> > index d7b0903c22b2..03df64600aa0 100644
> > --- a/include/uapi/drm/xe_drm.h
> > +++ b/include/uapi/drm/xe_drm.h
> > @@ -100,6 +100,8 @@ extern "C" {
> >  #define DRM_XE_EXEC_QUEUE_GET_PROPERTY	0x08
> >  #define DRM_XE_EXEC			0x09
> >  #define DRM_XE_WAIT_USER_FENCE		0x0a
> > +#define DRM_XE_PERF			0x0b
> > +
> >  /* Must be kept compact -- no holes */
> >
> >  #define DRM_IOCTL_XE_DEVICE_QUERY		DRM_IOWR(DRM_COMMAND_BASE + DRM_XE_DEVICE_QUERY, struct drm_xe_device_query)
> > @@ -113,6 +115,7 @@ extern "C" {
> >  #define DRM_IOCTL_XE_EXEC_QUEUE_GET_PROPERTY	DRM_IOWR(DRM_COMMAND_BASE + DRM_XE_EXEC_QUEUE_GET_PROPERTY, struct drm_xe_exec_queue_get_property)
> >  #define DRM_IOCTL_XE_EXEC			DRM_IOW(DRM_COMMAND_BASE + DRM_XE_EXEC, struct drm_xe_exec)
> >  #define DRM_IOCTL_XE_WAIT_USER_FENCE		DRM_IOWR(DRM_COMMAND_BASE + DRM_XE_WAIT_USER_FENCE, struct drm_xe_wait_user_fence)
> > +#define DRM_IOCTL_XE_PERF			DRM_IOW(DRM_COMMAND_BASE + DRM_XE_PERF, struct drm_xe_perf_param)
> >
> >  /**
> >   * DOC: Xe IOCTL Extensions
> > @@ -1370,6 +1373,66 @@ struct drm_xe_wait_user_fence {
> >	__u64 reserved[2];
> >  };
> >
> > +/** enum drm_xe_perf_type - Perf stream types */
>
> can we make it multi-line to look more like other top level kernel-doc?

Done.

>
> > +enum drm_xe_perf_type {
> > +	DRM_XE_PERF_TYPE_MAX,
>
> as this is likely a special case enumerator, maybe it's worth to name it
> differently then the real enumerators that should only be used
>
> and add some note about it's expected use model?

I named it __DRM_XE_PERF_TYPE_MAX and marked it as non-ABI. It will likely
not be used but I would like to introduce 'enum drm_xe_perf_type' in this
patch since it is part of the 'perf layer'. I could drop
DRM_XE_PERF_TYPE_MAX, but C does not allow empty enum's.

>
> > +};
> > +
> > +/**
> > + * enum drm_xe_perf_op - Perf stream ops
> > + */
> > +enum drm_xe_perf_op {
> > +	/** @DRM_XE_PERF_OP_STREAM_OPEN: Open a perf counter stream */
> > +	DRM_XE_PERF_OP_STREAM_OPEN,
> > +
> > +	/** @DRM_XE_PERF_OP_ADD_CONFIG: Add perf stream config */
> > +	DRM_XE_PERF_OP_ADD_CONFIG,
> > +
> > +	/** @DRM_XE_PERF_OP_REMOVE_CONFIG: Remove perf stream config */
> > +	DRM_XE_PERF_OP_REMOVE_CONFIG,
> > +};
> > +
> > +/**
> > + * struct drm_xe_perf_param - Input of &DRM_XE_PERF
> > + *
> > + * The perf layer enables multiplexing perf counter streams of multiple
> > + * types. The actual params for a particular stream operation are supplied
> > + * via the @param pointer (use __copy_from_user to get these params).
> > + */
> > +struct drm_xe_perf_param {
> > +	/** @extensions: Pointer to the first extension struct, if any */
> > +	__u64 extensions;
> > +	/** @perf_type: Perf stream type, of enum @drm_xe_perf_type */
> > +	__u64 perf_type;
> > +	/** @perf_op: Perf op, of enum @drm_xe_perf_op */
> > +	__u64 perf_op;
> > +	/** @param: Pointer to actual stream params */
> > +	__u64 param;
> > +};
> > +
> > +/**
> > + * enum drm_xe_perf_ioctls - Perf fd ioctl's
> > + *
> > + * Information exchanged between userspace and kernel for perf fd ioctl's
> > + * is stream type specific
> > + */
> > +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),
> > +
> > +	/** @DRM_XE_PERF_IOCTL_STATUS: Return stream status */
> > +	DRM_XE_PERF_IOCTL_STATUS = _IO('i', 0x3),
> > +
> > +	/** @DRM_XE_PERF_IOCTL_INFO: Return stream info */
> > +	DRM_XE_PERF_IOCTL_INFO = _IO('i', 0x4),
> > +};
> > +
> >  #if defined(__cplusplus)
> >  }
> >  #endif

Thanks.
--
Ashutosh