Re: [Intel-xe] [PATCH 13/21] drm/xe/uapi: Multiplex PERF ops through a single PERF ioctl

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

On Tue, Sep 19, 2023 at 09:10:41AM -0700, Ashutosh Dixit wrote:
>Since we are already mulitplexing multiple perf counter stream types
>through the PERF layer, it seems odd to retain separate ioctls for perf
>op's (add/remove config). In fact it seems logical to also multiplex these
>ops through a single PERF ioctl. This also affords greater flexibility to
>add stream specific ops if needed for different perf stream types.
>
>Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
>---
> drivers/gpu/drm/xe/xe_device.c |  5 +----
> drivers/gpu/drm/xe/xe_perf.c   | 32 ++++++++------------------------
> drivers/gpu/drm/xe/xe_perf.h   |  4 +---
> include/uapi/drm/xe_drm.h      | 16 ++++++++++------
> 4 files changed, 20 insertions(+), 37 deletions(-)
>
>diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
>index 770b9fe6e65df..24018a0801788 100644
>--- a/drivers/gpu/drm/xe/xe_device.c
>+++ b/drivers/gpu/drm/xe/xe_device.c
>@@ -115,10 +115,7 @@ 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_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),
>-
>+	DRM_IOCTL_DEF_DRV(XE_PERF, xe_perf_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
>index 0f747af59f245..f8d7eae8fffe0 100644
>--- a/drivers/gpu/drm/xe/xe_perf.c
>+++ b/drivers/gpu/drm/xe/xe_perf.c
>@@ -6,37 +6,21 @@
> #include "xe_oa.h"
> #include "xe_perf.h"
>
>-int xe_perf_open_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
>+int xe_oa_ioctl(struct drm_device *dev, struct drm_xe_perf_param *arg, 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:
>+	switch (arg->perf_op) {
>+	case XE_PERF_STREAM_OPEN:
> 		return xe_oa_stream_open_ioctl(dev, (void *)arg->param, file);

It's a nice idea to reduce the ioctls, but if your struct 
drm_xe_perf_param *arg is overloaded based on the PERF_OP passed, then I 
would recommend validating that the right arg is passed for the 
corresponding OP. Ideally I wouldn't go that route since that would 
require some sort of signature in the arg which would identify it as the 
correct param. Instead I would be okay with retaining separate ioctls 
for the 3 operations.

Umesh