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

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

On Tue, 03 Oct 2023 19:13:31 -0700, Umesh Nerlige Ramappa wrote:
>
> On Tue, Sep 19, 2023 at 09:10:40AM -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.
> >
> > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> > ---
> > drivers/gpu/drm/xe/Makefile    |  1 +
> > drivers/gpu/drm/xe/xe_device.c |  8 +++---
> > drivers/gpu/drm/xe/xe_oa.c     | 43 +++++++++++++++++-----------
> > drivers/gpu/drm/xe/xe_perf.c   | 52 ++++++++++++++++++++++++++++++++++
> > drivers/gpu/drm/xe/xe_perf.h   | 18 ++++++++++++
> > include/uapi/drm/xe_drm.h      | 44 +++++++++++++++++++---------
> > 6 files changed, 133 insertions(+), 33 deletions(-)
> > 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 a40c4827b9c85..294874681cc6c 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 7a179c4515633..770b9fe6e65df 100644
> > --- a/drivers/gpu/drm/xe/xe_device.c
> > +++ b/drivers/gpu/drm/xe/xe_device.c
> > @@ -25,8 +25,8 @@
> > #include "xe_irq.h"
> > #include "xe_mmio.h"
> > #include "xe_module.h"
> > -#include "xe_oa.h"
> > #include "xe_pcode.h"
> > +#include "xe_perf.h"
> > #include "xe_pm.h"
> > #include "xe_query.h"
> > #include "xe_tile.h"
> > @@ -115,9 +115,9 @@ static const struct drm_ioctl_desc xe_ioctls[] = {
> >			  DRM_RENDER_ALLOW),
> >	DRM_IOCTL_DEF_DRV(XE_VM_MADVISE, xe_vm_madvise_ioctl, DRM_RENDER_ALLOW),
> >
> > -	DRM_IOCTL_DEF_DRV(XE_OA_OPEN, xe_oa_stream_open_ioctl, DRM_RENDER_ALLOW),
> > -	DRM_IOCTL_DEF_DRV(XE_OA_ADD_CONFIG, xe_oa_add_config_ioctl, DRM_RENDER_ALLOW),
> > -	DRM_IOCTL_DEF_DRV(XE_OA_REMOVE_CONFIG, xe_oa_remove_config_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),
> >
> > };
> >
> > diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c
> > index 506dd056805b2..63db0969a86b2 100644
> > --- a/drivers/gpu/drm/xe/xe_oa.c
> > +++ b/drivers/gpu/drm/xe/xe_oa.c
> > @@ -1173,13 +1173,13 @@ static long xe_oa_ioctl_locked(struct xe_oa_stream *stream,
> >			       unsigned long arg)
> > {
> >	switch (cmd) {
> > -	case XE_OA_IOCTL_ENABLE:
> > +	case XE_PERF_IOCTL_ENABLE:
> >		xe_oa_enable_locked(stream);
> >		return 0;
> > -	case XE_OA_IOCTL_DISABLE:
> > +	case XE_PERF_IOCTL_DISABLE:
> >		xe_oa_disable_locked(stream);
> >		return 0;
> > -	case XE_OA_IOCTL_CONFIG:
> > +	case XE_PERF_IOCTL_CONFIG:
> >		return xe_oa_config_locked(stream, arg);
> >	}
> >
> > @@ -1692,12 +1692,11 @@ static int xe_oa_read_properties_unlocked(struct xe_oa *oa, u64 __user *uprops,
> >	return 0;
> > }
> >
> > -int xe_oa_stream_open_ioctl(struct drm_device *dev, void *data,
> > -			    struct drm_file *file)
> > +int xe_oa_stream_open_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
> > {
> >	struct xe_oa *oa = &to_xe_device(dev)->oa;
> > -	struct drm_xe_oa_open_param *param = data;
> >	struct xe_oa_open_properties props = {};
> > +	struct drm_xe_oa_open_param param;
> >	u32 known_open_flags;
> >	struct xe_gt *gt;
> >	int ret;
> > @@ -1707,14 +1706,18 @@ int xe_oa_stream_open_ioctl(struct drm_device *dev, void *data,
> >		return -ENODEV;
> >	}
> >
> > +	ret = __copy_from_user(&param, data, sizeof(param));
> > +	if (XE_IOCTL_DBG(oa->xe, ret))
> > +		return -EFAULT;
> > +
> >	known_open_flags = XE_OA_FLAG_FD_CLOEXEC | XE_OA_FLAG_FD_NONBLOCK | XE_OA_FLAG_DISABLED;
> > -	if (param->flags & ~known_open_flags) {
> > +	if (param.flags & ~known_open_flags) {
> >		drm_dbg(&oa->xe->drm, "Unknown drm_xe_oa_open_param flag\n");
> >		return -EINVAL;
> >	}
> >
> > -	ret = xe_oa_read_properties_unlocked(oa, u64_to_user_ptr(param->properties_ptr),
> > -					     param->num_properties,
> > +	ret = xe_oa_read_properties_unlocked(oa, u64_to_user_ptr(param.properties_ptr),
> > +					     param.num_properties,
> >					     &props);
> >	if (ret)
> >		return ret;
> > @@ -1722,7 +1725,7 @@ int xe_oa_stream_open_ioctl(struct drm_device *dev, void *data,
> >	gt = props.hwe->gt;
> >
> >	mutex_lock(&gt->oa.lock);
> > -	ret = xe_oa_stream_open_ioctl_locked(oa, param, &props, file);
> > +	ret = xe_oa_stream_open_ioctl_locked(oa, &param, &props, file);
> >	mutex_unlock(&gt->oa.lock);
> >
> >	return ret;
> > @@ -1918,7 +1921,8 @@ int xe_oa_add_config_ioctl(struct drm_device *dev, void *data,
> >			   struct drm_file *file)
> > {
> >	struct xe_oa *oa = &to_xe_device(dev)->oa;
> > -	struct drm_xe_oa_config *arg = data;
> > +	struct drm_xe_oa_config param;
> > +	struct drm_xe_oa_config *arg = &param;
> >	struct xe_oa_config *oa_config, *tmp;
> >	struct xe_oa_reg *regs;
> >	int err, id;
> > @@ -1933,6 +1937,10 @@ int xe_oa_add_config_ioctl(struct drm_device *dev, void *data,
> >		return -EACCES;
> >	}
> >
> > +	err = __copy_from_user(&param, data, sizeof(param));
> > +	if (XE_IOCTL_DBG(oa->xe, err))
> > +		return -EFAULT;
> > +
> >	if ((!arg->mux_regs_ptr || !arg->n_mux_regs) &&
> >	    (!arg->boolean_regs_ptr || !arg->n_boolean_regs) &&
> >	    (!arg->flex_regs_ptr || !arg->n_flex_regs)) {
> > @@ -2035,7 +2043,7 @@ int xe_oa_remove_config_ioctl(struct drm_device *dev, void *data,
> > {
> >	struct xe_oa *oa = &to_xe_device(dev)->oa;
> >	struct xe_oa_config *oa_config;
> > -	u64 *arg = data;
> > +	u64 arg, *ptr = data;
> >	int ret;
> >
> >	if (!oa->xe) {
> > @@ -2048,22 +2056,25 @@ int xe_oa_remove_config_ioctl(struct drm_device *dev, void *data,
> >		return -EACCES;
> >	}
> >
> > +	ret = get_user(arg, ptr);
> > +	if (XE_IOCTL_DBG(oa->xe, ret))
> > +		return ret;
> > +
> >	ret = mutex_lock_interruptible(&oa->metrics_lock);
> >	if (ret)
> >		return ret;
> >
> > -	oa_config = idr_find(&oa->metrics_idr, *arg);
> > +	oa_config = idr_find(&oa->metrics_idr, arg);
> >	if (!oa_config) {
> >		drm_dbg(&oa->xe->drm, "Failed to remove unknown OA config\n");
> >		ret = -ENOENT;
> >		goto err_unlock;
> >	}
> >
> > -	WARN_ON(*arg != oa_config->id);
> > +	WARN_ON(arg != oa_config->id);
> >
> >	sysfs_remove_group(oa->metrics_kobj, &oa_config->sysfs_metric);
> > -
> > -	idr_remove(&oa->metrics_idr, *arg);
> > +	idr_remove(&oa->metrics_idr, arg);
> >
> >	mutex_unlock(&oa->metrics_lock);
> >
> > diff --git a/drivers/gpu/drm/xe/xe_perf.c b/drivers/gpu/drm/xe/xe_perf.c
> > new file mode 100644
> > index 0000000000000..0f747af59f245
> > --- /dev/null
> > +++ b/drivers/gpu/drm/xe/xe_perf.c
> > @@ -0,0 +1,52 @@
> > +// SPDX-License-Identifier: MIT
> > +/*
> > + * Copyright © 2023 Intel Corporation
> > + */
> > +
> > +#include "xe_oa.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) {
> > +	case XE_PERF_TYPE_OA:
> > +		return xe_oa_stream_open_ioctl(dev, (void *)arg->param, file);
> > +	default:
> > +		return -EINVAL;
>
> Wondering if a different unique error must be returned to indicate that a
> particular perf module is not supported. Other than that, this lgtm,

Somthing weird like ECONNABORTED? If ops are included here probably
EOPNOSUPP would also work. Probably doesn't matter much. Maybe I will
change to EOPNOSUPP for now.

> Reviewed-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>

Thanks.
--
Ashutosh