From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id D7A4DC25B76 for ; Sat, 8 Jun 2024 10:44:31 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 66DE410E063; Sat, 8 Jun 2024 10:44:31 +0000 (UTC) Authentication-Results: gabe.freedesktop.org; dkim=pass (2048-bit key; unprotected) header.d=intel.com header.i=@intel.com header.b="RYVkh22B"; dkim-atps=neutral Received: from mgamail.intel.com (mgamail.intel.com [198.175.65.21]) by gabe.freedesktop.org (Postfix) with ESMTPS id D18E010E063 for ; Sat, 8 Jun 2024 10:44:29 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1717843470; x=1749379470; h=message-id:date:mime-version:subject:to:references:from: in-reply-to:content-transfer-encoding; bh=hmxGxlIe9qEu2wwZhNtQCj616r/45i1XiELBSsr6G1U=; b=RYVkh22BhGMKt+MtOoIB4a8LwT3ty2BVMx2QykAWjAvf6Vtb/XSCHXo/ xBYfyZ+xNih1xQitoVxE5NmRDhq4c6RCl7ZDCxzxaXBGS02lW8c6U2mlF BHBMCMIK++Tj9SJLaqo9I90vO5qSxbQH/iuqnCcU2ajyU7jLrTI6yMdk9 kD4GYhq40O8eTrrqf1jdXD6jmuKOvFR+YCGW7HjqVEv4lmdWrRSlmrcYx Ae3hEipp1q9//lsDX2675WDJUQ55O02svXpnD+4eNdKWDDx/3jPZIWztb X6376xjn6xldZT8V+crTTwU3Dv1DQC54MixqPJ10wyQwJ3jfHiIFYrghe g==; X-CSE-ConnectionGUID: O1/TFLVtRyapDnajYvEmsA== X-CSE-MsgGUID: F9w3+2gGS4uYHqgqhRebnA== X-IronPort-AV: E=McAfee;i="6600,9927,11096"; a="14519634" X-IronPort-AV: E=Sophos;i="6.08,223,1712646000"; d="scan'208";a="14519634" Received: from orviesa001.jf.intel.com ([10.64.159.141]) by orvoesa113.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 08 Jun 2024 03:44:29 -0700 X-CSE-ConnectionGUID: d1KUGqZuSdqQQftc2gsemw== X-CSE-MsgGUID: TSAhr1pxRjmCEMzov9jj8g== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.08,223,1712646000"; d="scan'208";a="76060772" Received: from irvmail002.ir.intel.com ([10.43.11.120]) by orviesa001.jf.intel.com with ESMTP; 08 Jun 2024 03:44:27 -0700 Received: from [10.245.82.128] (mwajdecz-MOBL.ger.corp.intel.com [10.245.82.128]) by irvmail002.ir.intel.com (Postfix) with ESMTP id 59489125BC; Sat, 8 Jun 2024 11:44:25 +0100 (IST) Message-ID: <57eea603-9fc3-4309-8349-f136ffc3a22c@intel.com> Date: Sat, 8 Jun 2024 12:44:24 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH 01/17] drm/xe/perf/uapi: "Perf" layer to support multiple perf counter stream types To: Ashutosh Dixit , intel-xe@lists.freedesktop.org References: <20240607204322.1966831-1-ashutosh.dixit@intel.com> <20240607204322.1966831-2-ashutosh.dixit@intel.com> Content-Language: en-US From: Michal Wajdeczko In-Reply-To: <20240607204322.1966831-2-ashutosh.dixit@intel.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-BeenThere: intel-xe@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel Xe graphics driver List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-xe-bounces@lists.freedesktop.org Sender: "Intel-xe" Hi Ashutosh, Please find few late btw comments below. 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 > Acked-by: José Roberto de Souza > Reviewed-by: Umesh Nerlige Ramappa > Signed-off-by: Ashutosh Dixit > --- > 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 ? > + */ > + > +#include > + > +#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 this is not needed here as you already have required forward decls > + > +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? > +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? > +}; > + > +/** > + * 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