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 C4C12CDB46E for ; Thu, 12 Oct 2023 18:13:24 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 788AD10E51C; Thu, 12 Oct 2023 18:13:24 +0000 (UTC) Received: from mgamail.intel.com (mgamail.intel.com [192.55.52.93]) by gabe.freedesktop.org (Postfix) with ESMTPS id 7E35510E51C for ; Thu, 12 Oct 2023 18:13:22 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1697134402; x=1728670402; h=date:message-id:from:to:cc:subject:in-reply-to: references:mime-version:content-transfer-encoding; bh=XAevaOLqwaXqESg9C9hGBFeKyZdGKRhLV/BZbmIzepQ=; b=FSX/AB7IROslTGeTGWTnw/r3WR/6O72My+dpkJCyiqip2+WajItrRJ/Z r1TCE6SeeHskSJRgiN086r9Wb8UpD0bb2RepbgGoMTOWz9z4KlZ3b55ag edJvT2c206roiTiy/Kwzwx4BiW2e2AAaIVaPW9gN2c+1vW+3kwseTgx+a athvY4B5p7z7KM/9if0GMi55SZn24VZ9d8H7yYw1nvq4hb0h0go96ldoc rto5qZxtGHKK/bQDSwdMZ118hHi2Zhux/EWJwf5CAkcWgOcUWO2zO/X00 mCElojCa+2BPBjsD2206+8lJAdi3gRovkv6t7tOtsf5Ll1ngqlPETUB3c g==; X-IronPort-AV: E=McAfee;i="6600,9927,10861"; a="382233011" X-IronPort-AV: E=Sophos;i="6.03,219,1694761200"; d="scan'208";a="382233011" Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga102.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 12 Oct 2023 11:13:22 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10861"; a="928096742" X-IronPort-AV: E=Sophos;i="6.03,219,1694761200"; d="scan'208";a="928096742" Received: from adixit-mobl.amr.corp.intel.com (HELO adixit-arch.intel.com) ([10.212.148.229]) by orsmga005-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 12 Oct 2023 11:13:21 -0700 Date: Thu, 12 Oct 2023 11:04:32 -0700 Message-ID: <877cnr3efj.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" To: Umesh Nerlige Ramappa In-Reply-To: References: <20231011172643.826452-1-ashutosh.dixit@intel.com> <20231011172643.826452-2-ashutosh.dixit@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.1 (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 Subject: Re: [Intel-xe] [PATCH 1/2] drm/xe/uapi: "Perf" layer to support multiple perf counter stream types 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: , Cc: robert.krzemien@intel.com, ogabbay@habana.ai, Lionel Landwerlin , gzadicario@habana.ai, Harish Chegondi , bdotan@habana.ai, talbo@habana.ai, intel-xe@lists.freedesktop.org Errors-To: intel-xe-bounces@lists.freedesktop.org Sender: "Intel-xe" On Thu, 12 Oct 2023 09:51:08 -0700, Umesh Nerlige Ramappa wrote: > Hi Umesh, > On Wed, Oct 11, 2023 at 10:26:41AM -0700, 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). This requires addition of a PERF layer= to > > multiplex these different stream types through a single set of PERF > > ioctl's. > > > > 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 are 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 Zad= icario) > > Add DRM_ prefix to ioctl names to indicate uapi names > > > > Signed-off-by: Ashutosh Dixit > > --- > > drivers/gpu/drm/xe/Makefile | 1 + > > drivers/gpu/drm/xe/xe_device.c | 4 +++ > > drivers/gpu/drm/xe/xe_perf.c | 47 ++++++++++++++++++++++++++++++++ > > drivers/gpu/drm/xe/xe_perf.h | 18 +++++++++++++ > > include/uapi/drm/xe_drm.h | 49 ++++++++++++++++++++++++++++++++++ > > 5 files changed, 119 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 175a357366d96..fb3eacbe3d6cb 100644 > > --- a/drivers/gpu/drm/xe/Makefile > > +++ b/drivers/gpu/drm/xe/Makefile > > @@ -88,6 +88,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 ef42c33e30558..3c0c28c6f9fb8 100644 > > --- a/drivers/gpu/drm/xe/xe_device.c > > +++ b/drivers/gpu/drm/xe/xe_device.c > > @@ -28,6 +28,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_tile.h" > > @@ -128,6 +129,9 @@ static const struct drm_ioctl_desc xe_ioctls[] =3D { > > DRM_IOCTL_DEF_DRV(XE_WAIT_USER_FENCE, xe_wait_user_fence_ioctl, > > 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_R= ENDER_ALLOW), > > + DRM_IOCTL_DEF_DRV(XE_PERF_REMOVE_CONFIG, xe_perf_remove_config_ioctl,= DRM_RENDER_ALLOW), > > }; > > > > static const struct file_operations xe_driver_fops =3D { > > diff --git a/drivers/gpu/drm/xe/xe_perf.c b/drivers/gpu/drm/xe/xe_perf.c > > new file mode 100644 > > index 0000000000000..de430065e0d27 > > --- /dev/null > > +++ b/drivers/gpu/drm/xe/xe_perf.c > > @@ -0,0 +1,47 @@ > > +// SPDX-License-Identifier: MIT > > +/* > > + * Copyright =A9 2023 Intel Corporation > > + */ > > + > > +#include > > + > > +#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 =3D data; > > + > > + if (arg->extensions) > > + return -EINVAL; > > + > > + switch (arg->perf_type) { > > + default: > > + return -EINVAL; > > + } > > +} > > + > > +int xe_perf_add_config_ioctl(struct drm_device *dev, void *data, struc= t drm_file *file) > > +{ > > + struct drm_xe_perf_param *arg =3D data; > > + > > + if (arg->extensions) > > + return -EINVAL; > > + > > + switch (arg->perf_type) { > > + default: > > + return -EINVAL; > > + } > > +} > > + > > +int xe_perf_remove_config_ioctl(struct drm_device *dev, void *data, st= ruct drm_file *file) > > +{ > > + struct drm_xe_perf_param *arg =3D data; > > + > > + if (arg->extensions) > > + return -EINVAL; > > + > > + switch (arg->perf_type) { > > + default: > > + return -EINVAL; > > + } > > +} > > I would just squash the 2 patches here since this is a new interface and = we > don't care much about what existed earlier. OK, I'll squash the two patches. > > diff --git a/drivers/gpu/drm/xe/xe_perf.h b/drivers/gpu/drm/xe/xe_perf.h > > new file mode 100644 > > index 0000000000000..7ee90491132a0 > > --- /dev/null > > +++ b/drivers/gpu/drm/xe/xe_perf.h > > @@ -0,0 +1,18 @@ > > +/* SPDX-License-Identifier: MIT */ > > +/* > > + * Copyright =A9 2023 Intel Corporation > > + */ > > + > > +#ifndef _XE_PERF_H_ > > +#define _XE_PERF_H_ > > + > > +#include > > + > > +struct drm_device; > > +struct drm_file; > > + > > +int xe_perf_open_ioctl(struct drm_device *dev, void *data, struct drm_= file *file); > > +int xe_perf_add_config_ioctl(struct drm_device *dev, void *data, struc= t drm_file *file); > > +int xe_perf_remove_config_ioctl(struct drm_device *dev, void *data, st= ruct drm_file *file); > > + > > +#endif > > diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h > > index d48d8e3c898ce..d734d2ee385b1 100644 > > --- a/include/uapi/drm/xe_drm.h > > +++ b/include/uapi/drm/xe_drm.h > > @@ -111,6 +111,9 @@ struct xe_user_extension { > > #define DRM_XE_WAIT_USER_FENCE 0x0b > > #define DRM_XE_VM_MADVISE 0x0c > > #define DRM_XE_EXEC_QUEUE_GET_PROPERTY 0x0d > > +#define DRM_XE_PERF_OPEN 0x0e > > +#define DRM_XE_PERF_ADD_CONFIG 0x0f > > +#define DRM_XE_PERF_REMOVE_CONFIG 0x10 > > > > /* Must be kept compact -- no holes */ > > #define DRM_IOCTL_XE_DEVICE_QUERY DRM_IOWR(DRM_COMMAND_BASE + DRM_XE_D= EVICE_QUERY, struct drm_xe_device_query) > > @@ -127,6 +130,9 @@ struct xe_user_extension { > > #define DRM_IOCTL_XE_EXEC_QUEUE_SET_PROPERTY DRM_IOW(DRM_COMMAND_BASE = + DRM_XE_EXEC_QUEUE_SET_PROPERTY, struct drm_xe_exec_queue_set_property) > > #define DRM_IOCTL_XE_WAIT_USER_FENCE DRM_IOWR(DRM_COMMAND_BASE + DRM_X= E_WAIT_USER_FENCE, struct drm_xe_wait_user_fence) > > #define DRM_IOCTL_XE_VM_MADVISE DRM_IOW(DRM_COMMAND_BASE + DRM_XE_VM= _MADVISE, struct drm_xe_vm_madvise) > > +#define DRM_IOCTL_XE_PERF_OPEN DRM_IOW(DRM_COMMAND_BASE + DRM_XE_PER= F_OPEN, struct drm_xe_perf_param) > > +#define DRM_IOCTL_XE_PERF_ADD_CONFIG DRM_IOW(DRM_COMMAND_BASE + DRM_X= E_PERF_ADD_CONFIG, struct drm_xe_perf_param) > > +#define DRM_IOCTL_XE_PERF_REMOVE_CONFIG DRM_IOW(DRM_COMMAND_BASE + DR= M_XE_PERF_REMOVE_CONFIG, struct drm_xe_perf_param) > > > > /** > > * enum drm_xe_memory_class - Supported memory classes. > > @@ -1093,6 +1099,49 @@ struct drm_xe_vm_madvise { > > #define XE_PMU_MEDIA_GROUP_BUSY(gt) ___XE_PMU_OTHER(gt, 3) > > #define XE_PMU_ANY_ENGINE_GROUP_BUSY(gt) ___XE_PMU_OTHER(gt, 4) > > > > +/** > > + * enum drm_xe_perf_type - Perf stream types > > + */ > > +enum drm_xe_perf_type { > > + DRM_XE_PERF_TYPE_MAX, > > +}; > > + > > +/** > > + * struct drm_xe_perf_param - XE perf layer param > > + * > > + * 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; > > + /** @param_size: size of data struct pointed to by @param */ > > + __u64 param_size; > > Since this structure is expandable using extensions, maybe we don't need = to > worry about the param_size. We can drop it. The size would matter if the > only way to extend a structure was to add members to the end. Hmm, note param_size is not the size of this struct, it is the "size of data struct pointed to by @param". Even the struct pointed to by @param probably will also have an extensions member. Even then I am thinking no harm in having the param_size member since it helps in cases where UMD and KMD versions are different and the struct size has changed, so each has a different notion of what the struct size is. Having param_size available enables the kernel to resolve such cases if the need arises in the future (as is done in drm_ioctl). UMD just needs to fill in param_size as sizeof(struct) it is passing in via @param, so doesn't look too horrible. So I'm thinking of leaving it in just in case. What do you think? Thanks. -- Ashutosh > > + /** @param: Pointer to actual stream params */ > > + __u64 param; > > +}; > > + > > +/** > > + * enum drm_xe_perf_ioctls - Perf fd ioctl's > > + */ > > +enum drm_xe_perf_ioctls { > > + /** > > + * @DRM_XE_PERF_IOCTL_ENABLE: Enable data capture for a stream that > > + * was e.g. either opened in a disabled state or was disabled via > > + * DRM_XE_PERF_IOCTL_DISABLE > > + */ > > + 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), > > +}; > > + > > #if defined(__cplusplus) > > } > > #endif > > -- > > 2.41.0 > >