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

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

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,

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

Umesh