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 C556CC27C78 for ; Wed, 12 Jun 2024 02:08:46 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 626A810E1DA; Wed, 12 Jun 2024 02:08:46 +0000 (UTC) Authentication-Results: gabe.freedesktop.org; dkim=pass (2048-bit key; unprotected) header.d=intel.com header.i=@intel.com header.b="RQzhPzzJ"; dkim-atps=neutral Received: from mgamail.intel.com (mgamail.intel.com [198.175.65.18]) by gabe.freedesktop.org (Postfix) with ESMTPS id A639F10E1DA for ; Wed, 12 Jun 2024 02:08:44 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1718158125; x=1749694125; h=date:message-id:from:to:cc:subject:in-reply-to: references:mime-version:content-transfer-encoding; bh=P2UdaeyrwAHyR24fBfs4zztyOFo+tUksACBejM5A4VU=; b=RQzhPzzJ2KtxIrIvbdfVcC8niMy/ZR9BOmOhb77sYfTdkLiOq13TCEjV OCKgwzpSRhxN7zM4guadm5+zc6nbFUV/yCW2hbmc8NLQE4qYLv4T1tin7 l0pKs6dvahhRy3Qk2sSiDHIevwZmWmjsbi+V3qPYyPYyC26y+fDvp10eq 8oWaNvujQ22m8WDZ4Ufn0CMAsmC/KoosW2lcY52SxQxiw6eOx5dGT4VVX krGsPfExUHfJheZ48EwTaXJECNGeEsew7enqgBRKrcRWAYkfbqwl6DkVv LieW0I07T2dPW3EIGIrsi4OsDq5oZv6fpdB4WNZkJ9SrR+NNHOF7Yfsr9 A==; X-CSE-ConnectionGUID: fVYgvGWPSpawym9WCrk8Mw== X-CSE-MsgGUID: 4NNpGk8XRbum+ZKvjN4xxQ== X-IronPort-AV: E=McAfee;i="6600,9927,11100"; a="15060171" X-IronPort-AV: E=Sophos;i="6.08,231,1712646000"; d="scan'208";a="15060171" Received: from fmviesa001.fm.intel.com ([10.60.135.141]) by orvoesa110.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 11 Jun 2024 19:08:44 -0700 X-CSE-ConnectionGUID: yIyg37nrQPSM/dVmECmu5g== X-CSE-MsgGUID: sarTz5uOQHGRWKDpkUoPYA== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.08,231,1712646000"; d="scan'208";a="70833331" Received: from jrglassb-mobl1.amr.corp.intel.com (HELO adixit-arch.intel.com) ([10.125.0.65]) by smtpauth.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 11 Jun 2024 19:08:44 -0700 Date: Tue, 11 Jun 2024 19:03:10 -0700 Message-ID: <8734pi2a01.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" To: Michal Wajdeczko Cc: Subject: Re: [PATCH 01/17] drm/xe/perf/uapi: "Perf" layer to support multiple perf counter stream types In-Reply-To: <57eea603-9fc3-4309-8349-f136ffc3a22c@intel.com> References: <20240607204322.1966831-1-ashutosh.dixit@intel.com> <20240607204322.1966831-2-ashutosh.dixit@intel.com> <57eea603-9fc3-4309-8349-f136ffc3a22c@intel.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?ISO-8859-4?Q?Goj=F2?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/29.3 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable 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" On Sat, 08 Jun 2024 03:44:24 -0700, Michal Wajdeczko wrote: > Hi Michal, > Hi Ashutosh, > > Please find few late btw comments below. Np, thanks for the review. You can see a consolidated (or full tree) diff of the changes in response to your comments (in v17) here: https://patchwork.freedesktop.org/series/134742/ v17 itself can be seen here: https://patchwork.freedesktop.org/series/121084/#rev17 > > On 07.06.2024 22:43, Ashutosh Dixit wrote: > > In Xe, the plan is to support multiple types of perf counter streams (O= A 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 the= se > > stream types) through a single PERF ioctl. This multiplexing is the pur= pose > > 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=E9 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 +=3D 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_dev= ice.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[] =3D { > > 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 =A9 2023 Intel Corporation > > maybe it's time to use 2023-2024 ? Done. > > > + */ > > + > > +#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 =3D 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 =A9 2023 Intel Corporation > > + */ > > + > > +#ifndef _XE_PERF_H_ > > +#define _XE_PERF_H_ > > + > > +#include > > this is not needed here as you already have required forward decls Done, moved to .c > > > + > > +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, st= ruct 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, st= ruct 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? Done. > > > +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? I named it __DRM_XE_PERF_TYPE_MAX and marked it as non-ABI. It will likely not be used but I would like to introduce 'enum drm_xe_perf_type' in this patch since it is part of the 'perf layer'. I could drop DRM_XE_PERF_TYPE_MAX, but C does not allow empty enum's. > > > +}; > > + > > +/** > > + * 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 supp= lied > > + * 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 ioct= l'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 =3D _IO('i', 0x0), > > + > > + /** @DRM_XE_PERF_IOCTL_DISABLE: Disable data capture for a stream */ > > + DRM_XE_PERF_IOCTL_DISABLE =3D _IO('i', 0x1), > > + > > + /** @DRM_XE_PERF_IOCTL_CONFIG: Change stream configuration */ > > + DRM_XE_PERF_IOCTL_CONFIG =3D _IO('i', 0x2), > > + > > + /** @DRM_XE_PERF_IOCTL_STATUS: Return stream status */ > > + DRM_XE_PERF_IOCTL_STATUS =3D _IO('i', 0x3), > > + > > + /** @DRM_XE_PERF_IOCTL_INFO: Return stream info */ > > + DRM_XE_PERF_IOCTL_INFO =3D _IO('i', 0x4), > > +}; > > + > > #if defined(__cplusplus) > > } > > #endif Thanks. -- Ashutosh