Re: [Intel-xe] [PATCH 1/2] drm/xe/uapi: "Perf" layer to support multiple perf counter stream types

[Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>]

On Thu, Oct 12, 2023 at 11:04:32AM -0700, Dixit, Ashutosh wrote:
>On Thu, 12 Oct 2023 09:51:08 -0700, Umesh Nerlige Ramappa wrote:
>>
>
>Hi Umesh,
>
>> On Wed, Oct 11, 2023 at 10:26:41AM -0700, Ashutosh Dixit wrote:
>> > In XE, the plan is to support multiple types of perf counter streams (OA is
>> > only one type of these streams). This requires addition of a PERF layer to
>> > multiplex these different stream types through a single set of PERF
>> > ioctl's.
>> >
>> > 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 are 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
>> >
>> > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
>> > ---
>> > drivers/gpu/drm/xe/Makefile    |  1 +
>> > drivers/gpu/drm/xe/xe_device.c |  4 +++
>> > drivers/gpu/drm/xe/xe_perf.c   | 47 ++++++++++++++++++++++++++++++++
>> > drivers/gpu/drm/xe/xe_perf.h   | 18 +++++++++++++
>> > include/uapi/drm/xe_drm.h      | 49 ++++++++++++++++++++++++++++++++++
>> > 5 files changed, 119 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 175a357366d96..fb3eacbe3d6cb 100644
>> > --- a/drivers/gpu/drm/xe/Makefile
>> > +++ b/drivers/gpu/drm/xe/Makefile
>> > @@ -88,6 +88,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 ef42c33e30558..3c0c28c6f9fb8 100644
>> > --- a/drivers/gpu/drm/xe/xe_device.c
>> > +++ b/drivers/gpu/drm/xe/xe_device.c
>> > @@ -28,6 +28,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_tile.h"
>> > @@ -128,6 +129,9 @@ static const struct drm_ioctl_desc xe_ioctls[] = {
>> >	DRM_IOCTL_DEF_DRV(XE_WAIT_USER_FENCE, xe_wait_user_fence_ioctl,
>> >			  DRM_RENDER_ALLOW),
>> >	DRM_IOCTL_DEF_DRV(XE_VM_MADVISE, xe_vm_madvise_ioctl, DRM_RENDER_ALLOW),
>> > +	DRM_IOCTL_DEF_DRV(XE_PERF_OPEN, xe_perf_open_ioctl, DRM_RENDER_ALLOW),
>> > +	DRM_IOCTL_DEF_DRV(XE_PERF_ADD_CONFIG, xe_perf_add_config_ioctl, DRM_RENDER_ALLOW),
>> > +	DRM_IOCTL_DEF_DRV(XE_PERF_REMOVE_CONFIG, xe_perf_remove_config_ioctl, DRM_RENDER_ALLOW),
>> > };
>> >
>> > static const struct file_operations xe_driver_fops = {
>> > diff --git a/drivers/gpu/drm/xe/xe_perf.c b/drivers/gpu/drm/xe/xe_perf.c
>> > new file mode 100644
>> > index 0000000000000..de430065e0d27
>> > --- /dev/null
>> > +++ b/drivers/gpu/drm/xe/xe_perf.c
>> > @@ -0,0 +1,47 @@
>> > +// SPDX-License-Identifier: MIT
>> > +/*
>> > + * Copyright © 2023 Intel Corporation
>> > + */
>> > +
>> > +#include <linux/errno.h>
>> > +
>> > +#include "xe_perf.h"
>> > +
>> > +int xe_perf_open_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;
>> > +	}
>> > +}
>> > +
>> > +int xe_perf_add_config_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;
>> > +	}
>> > +}
>> > +
>> > +int xe_perf_remove_config_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;
>> > +	}
>> > +}
>>
>> I would just squash the 2 patches here since this is a new interface and we
>> don't care much about what existed earlier.
>
>OK, I'll squash the two patches.
>
>> > diff --git a/drivers/gpu/drm/xe/xe_perf.h b/drivers/gpu/drm/xe/xe_perf.h
>> > new file mode 100644
>> > index 0000000000000..7ee90491132a0
>> > --- /dev/null
>> > +++ b/drivers/gpu/drm/xe/xe_perf.h
>> > @@ -0,0 +1,18 @@
>> > +/* SPDX-License-Identifier: MIT */
>> > +/*
>> > + * Copyright © 2023 Intel Corporation
>> > + */
>> > +
>> > +#ifndef _XE_PERF_H_
>> > +#define _XE_PERF_H_
>> > +
>> > +#include <drm/xe_drm.h>
>> > +
>> > +struct drm_device;
>> > +struct drm_file;
>> > +
>> > +int xe_perf_open_ioctl(struct drm_device *dev, void *data, struct drm_file *file);
>> > +int xe_perf_add_config_ioctl(struct drm_device *dev, void *data, struct drm_file *file);
>> > +int xe_perf_remove_config_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 d48d8e3c898ce..d734d2ee385b1 100644
>> > --- a/include/uapi/drm/xe_drm.h
>> > +++ b/include/uapi/drm/xe_drm.h
>> > @@ -111,6 +111,9 @@ struct xe_user_extension {
>> > #define DRM_XE_WAIT_USER_FENCE		0x0b
>> > #define DRM_XE_VM_MADVISE		0x0c
>> > #define DRM_XE_EXEC_QUEUE_GET_PROPERTY	0x0d
>> > +#define DRM_XE_PERF_OPEN		0x0e
>> > +#define DRM_XE_PERF_ADD_CONFIG		0x0f
>> > +#define DRM_XE_PERF_REMOVE_CONFIG	0x10
>> >
>> > /* 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)
>> > @@ -127,6 +130,9 @@ struct xe_user_extension {
>> > #define DRM_IOCTL_XE_EXEC_QUEUE_SET_PROPERTY	 DRM_IOW(DRM_COMMAND_BASE + DRM_XE_EXEC_QUEUE_SET_PROPERTY, struct drm_xe_exec_queue_set_property)
>> > #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_VM_MADVISE			 DRM_IOW(DRM_COMMAND_BASE + DRM_XE_VM_MADVISE, struct drm_xe_vm_madvise)
>> > +#define DRM_IOCTL_XE_PERF_OPEN			DRM_IOW(DRM_COMMAND_BASE + DRM_XE_PERF_OPEN, struct drm_xe_perf_param)
>> > +#define DRM_IOCTL_XE_PERF_ADD_CONFIG		DRM_IOW(DRM_COMMAND_BASE + DRM_XE_PERF_ADD_CONFIG, struct drm_xe_perf_param)
>> > +#define DRM_IOCTL_XE_PERF_REMOVE_CONFIG		DRM_IOW(DRM_COMMAND_BASE + DRM_XE_PERF_REMOVE_CONFIG, struct drm_xe_perf_param)
>> >
>> > /**
>> >  * enum drm_xe_memory_class - Supported memory classes.
>> > @@ -1093,6 +1099,49 @@ struct drm_xe_vm_madvise {
>> > #define XE_PMU_MEDIA_GROUP_BUSY(gt)		___XE_PMU_OTHER(gt, 3)
>> > #define XE_PMU_ANY_ENGINE_GROUP_BUSY(gt)	___XE_PMU_OTHER(gt, 4)
>> >
>> > +/**
>> > + * enum drm_xe_perf_type - Perf stream types
>> > + */
>> > +enum drm_xe_perf_type {
>> > +	DRM_XE_PERF_TYPE_MAX,
>> > +};
>> > +
>> > +/**
>> > + * struct drm_xe_perf_param - XE perf layer param
>> > + *
>> > + * 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;
>> > +	/** @param_size: size of data struct pointed to by @param */
>> > +	__u64 param_size;
>>
>> Since this structure is expandable using extensions, maybe we don't need to
>> worry about the param_size. We can drop it. The size would matter if the
>> only way to extend a structure was to add members to the end.
>
>Hmm, note param_size is not the size of this struct, it is the "size of
>data struct pointed to by @param". Even the struct pointed to by @param
>probably will also have an extensions member.
>
>Even then I am thinking no harm in having the param_size member since it
>helps in cases where UMD and KMD versions are different and the struct size
>has changed, so each has a different notion of what the struct size
>is. Having param_size available enables the kernel to resolve such cases if
>the need arises in the future (as is done in drm_ioctl). UMD just needs to
>fill in param_size as sizeof(struct) it is passing in via @param, so
>doesn't look too horrible. So I'm thinking of leaving it in just in case.
>
>What do you think?

IMO, if extensions are members of all structs in our uApi, then we 
should just use that for extending the structs. I think the size is 
added burden on UMDs to populate. In case of drm_ioctl, the UMD does not 
see this, so it's probably okay.

Regards,
Umesh

>
>Thanks.
>--
>Ashutosh
>
>> > +	/** @param: Pointer to actual stream params */
>> > +	__u64 param;
>> > +};
>> > +
>> > +/**
>> > + * 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 that
>> > +	 * was e.g. either opened in a disabled state or was disabled via
>> > +	 * DRM_XE_PERF_IOCTL_DISABLE
>> > +	 */
>> > +	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),
>> > +};
>> > +
>> > #if defined(__cplusplus)
>> > }
>> > #endif
>> > --
>> > 2.41.0
>> >