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 56EECC4828F for ; Thu, 8 Feb 2024 06:05:33 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 5E91810E111; Thu, 8 Feb 2024 06:05:27 +0000 (UTC) Authentication-Results: gabe.freedesktop.org; dkim=pass (2048-bit key; unprotected) header.d=intel.com header.i=@intel.com header.b="aqQ0vEgk"; dkim-atps=neutral Received: from mgamail.intel.com (mgamail.intel.com [192.198.163.19]) by gabe.freedesktop.org (Postfix) with ESMTPS id 922F010E111 for ; Thu, 8 Feb 2024 06:05:26 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1707372327; x=1738908327; h=date:message-id:from:to:cc:subject:in-reply-to: references:mime-version; bh=y/DEUZEzavmWydxYzND3kQwNuokFX6qmo4i3KZ4LqNg=; b=aqQ0vEgkfOh2XTQ6mDuSCqp40QH+saP2XM8Ura3WSAAOzQH59HW0b+87 MhW7j01vSECcsNYMAKm/E1qH6Py9fiALUBZ073AeV4X+25KQlZA+hLfNN 3j0ddK6BKbnTBvtJWHlW1S1BrCqmg0kXPnM9gnRym0M8k/rUVW9djpLNa iqbSx5GAf9PLcZUvXa2pm7k8FyBSKmJR4wplC8NJ2TkeSpxi+nUJxWglh 9ovzXIGhCgcjKHmxczWeRSIgHGojrfphnihJMWmK1HWWBllIVph5D/Hia JP/US2Sxehu69gvHSNAyaSF0oUuk+qLMtFLx764OTLllNhzhJof8gfpnq g==; X-IronPort-AV: E=McAfee;i="6600,9927,10977"; a="1051154" X-IronPort-AV: E=Sophos;i="6.05,252,1701158400"; d="scan'208";a="1051154" Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmvoesa113.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 07 Feb 2024 22:05:26 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10977"; a="824732770" X-IronPort-AV: E=Sophos;i="6.05,252,1701158400"; d="scan'208";a="824732770" Received: from orsosgc001.jf.intel.com (HELO unerlige-ril.intel.com) ([10.165.21.138]) by orsmga001-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 07 Feb 2024 22:05:25 -0800 Date: Wed, 07 Feb 2024 22:05:25 -0800 Message-ID: <85bk8rwkwq.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" To: Umesh Nerlige Ramappa Cc: intel-xe@lists.freedesktop.org Subject: Re: [PATCH 13/16] drm/xe/oa/uapi: Query OA unit properties In-Reply-To: References: <20240120020026.1261201-1-ashutosh.dixit@intel.com> <20240120020026.1261201-14-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/28.2 (x86_64-redhat-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII 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 Tue, 06 Feb 2024 15:00:29 -0800, Umesh Nerlige Ramappa wrote: > Hi Umesh, > On Fri, Jan 19, 2024 at 06:00:23PM -0800, Ashutosh Dixit wrote: > > Implement query for properties of OA units present on a device. > > > > v2: Clean up reserved/pad fields (Umesh) > > Follow the same scheme as other query structs > > > > Signed-off-by: Ashutosh Dixit > > --- > > drivers/gpu/drm/xe/xe_query.c | 81 +++++++++++++++++++++++++++++++++++ > > include/uapi/drm/xe_drm.h | 55 ++++++++++++++++++++++++ > > 2 files changed, 136 insertions(+) > > > > diff --git a/drivers/gpu/drm/xe/xe_query.c b/drivers/gpu/drm/xe/xe_query.c > > index 9b35673b286c8..eac266a3d66d5 100644 > > --- a/drivers/gpu/drm/xe/xe_query.c > > +++ b/drivers/gpu/drm/xe/xe_query.c > > @@ -520,6 +520,86 @@ static int query_gt_topology(struct xe_device *xe, > > return 0; > > } > > > > +static size_t calc_oa_unit_query_size(struct xe_device *xe) > > +{ > > + size_t size = sizeof(struct drm_xe_query_oa_units); > > + struct xe_gt *gt; > > + int i, id; > > + > > + for_each_gt(gt, xe, id) { > > + for (i = 0; i < gt->oa.num_oa_units; i++) { > > + size += sizeof(struct drm_xe_oa_unit); > > + size += gt->oa.oa_unit[i].num_engines * > > + sizeof(struct drm_xe_engine_class_instance); > > + } > > + } > > + > > + return size; > > +} > > + > > +static int query_oa_units(struct xe_device *xe, > > + struct drm_xe_device_query *query) > > +{ > > + void __user *query_ptr = u64_to_user_ptr(query->data); > > + size_t size = calc_oa_unit_query_size(xe); > > + struct drm_xe_query_oa_units *qoa; > > + enum xe_hw_engine_id hwe_id; > > + struct drm_xe_oa_unit *du; > > + struct xe_hw_engine *hwe; > > + struct xe_oa_unit *u; > > + int gt_id, i, j, ret; > > + struct xe_gt *gt; > > + u8 *pdu; > > + > > + if (query->size == 0) { > > + query->size = size; > > + return 0; > > + } else if (XE_IOCTL_DBG(xe, query->size != size)) { > > + return -EINVAL; > > + } > > + > > + qoa = kzalloc(size, GFP_KERNEL); > > + if (!qoa) > > + return -ENOMEM; > > + > > + pdu = (u8 *)&qoa->oa_units[0]; > > + for_each_gt(gt, xe, gt_id) { > > + for (i = 0; i < gt->oa.num_oa_units; i++) { > > + u = >->oa.oa_unit[i]; > > + du = (struct drm_xe_oa_unit *)pdu; > > + > > + du->oa_unit_id = u->oa_unit_id; > > + du->oa_unit_type = u->type; > > + du->gt_id = gt->info.id; > > + du->open_stream = !!u->exclusive_stream; > > + du->oa_timestamp_freq = xe_oa_timestamp_frequency(gt); > > + du->oa_buf_size = XE_OA_BUFFER_SIZE; > > + du->num_engines = u->num_engines; > > + > > + for (j = 1; j < DRM_XE_OA_PROPERTY_MAX; j++) > > + du->capabilities |= BIT(j); Could you take a look at this too? So capabilities are just a bit mask of the properties. This would mean that in the future any changes we make will need to exposed via new properties, so that they get reflected in capabilities. > > + > > + j = 0; > > + for_each_hw_engine(hwe, gt, hwe_id) { > > + if (xe_oa_unit_id(hwe) == u->oa_unit_id) { > > + du->eci[j].engine_class = > > + xe_to_user_engine_class[hwe->class]; > > + du->eci[j].engine_instance = hwe->logical_instance; > > + du->eci[j].gt_id = gt->info.id; > > + j++; > > + } > > + } > > + pdu += sizeof(*du) + j * sizeof(du->eci[0]); > > + qoa->num_oa_units++; > > + } > > + } > > + > > + ret = copy_to_user(query_ptr, qoa, size); > > + kfree(qoa); > > + > > + return ret ? -EFAULT : 0; > > +} > > + > > static int (* const xe_query_funcs[])(struct xe_device *xe, > > struct drm_xe_device_query *query) = { > > query_engines, > > @@ -529,6 +609,7 @@ static int (* const xe_query_funcs[])(struct xe_device *xe, > > query_hwconfig, > > query_gt_topology, > > query_engine_cycles, > > + query_oa_units, > > }; > > > > int xe_query_ioctl(struct drm_device *dev, void *data, struct drm_file *file) > > diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h > > index 42474629244d4..2c0949b248d9a 100644 > > --- a/include/uapi/drm/xe_drm.h > > +++ b/include/uapi/drm/xe_drm.h > > @@ -646,6 +646,7 @@ struct drm_xe_device_query { > > #define DRM_XE_DEVICE_QUERY_HWCONFIG 4 > > #define DRM_XE_DEVICE_QUERY_GT_TOPOLOGY 5 > > #define DRM_XE_DEVICE_QUERY_ENGINE_CYCLES 6 > > +#define DRM_XE_DEVICE_QUERY_OA_UNITS 7 > > /** @query: The type of data to query */ > > __u32 query; > > > > @@ -1405,6 +1406,60 @@ enum drm_xe_oa_unit_type { > > DRM_XE_OA_UNIT_TYPE_OAM, > > }; > > > > +/** > > + * struct drm_xe_oa_unit - describe OA unit > > + */ > > +struct drm_xe_oa_unit { > > + /** @oa_unit_id: OA unit ID */ > > + __u16 oa_unit_id; > > + > > + /** @oa_unit_type: OA unit type of @drm_xe_oa_unit_type */ > > + __u16 oa_unit_type; > > + > > + /** @gt_id: GT ID for this OA unit */ > > + __u16 gt_id; > > + > > + /** @open_stream: True if a stream is open on the OA unit */ > > + __u16 open_stream; > > How would the user end up using this? Ideally, I would think that user > should only need to query all this info once and that would be before the > user opens any streams. If the user did open a stream, then that info is > already known to the user that a specific oa_unit_id is opened. > > Someone else or the same user trying to open the stream again would get an > error anyways. > > > + > > + /** @capabilities: OA capabilities bit-mask */ > > + __u64 capabilities; > > + > > + /** @oa_timestamp_freq: OA timestamp freq */ > > + __u64 oa_timestamp_freq; > > + > > + /** @oa_buf_size: OA buffer size */ > > + __u64 oa_buf_size; > > I think oa_buf_size value also changes based on whether the stream is > opened or not. We could have a oa_max_buf_size and oa_buf_size separately. > > Also, I am reluctant having this query return different values based on > whether the stream is opened of not. Determining the state/info of an open > stream should be a different query/interface. I think the ioctl on the open > fd was a good choice for that since that would return info on the open fd > and would anyways be serialized with close(fd). > > If the user can somehow know that the stream is opened and also know the > oa_buffer_size already, then we can do away with this (open_stream and > oa_buf_size) altogether. Agreed, this was kludgy. I have eliminated open_stream and oa_buf_size from this struct in v9 and exposed it via a new DRM_XE_PERF_IOCTL_INFO ioctl on the perf stream fd. Please take a look at "drm/xe/oa/uapi: Query OA unit properties" in v9. I have also eliminated gt_id from the above struct, since gt_id is available in 'struct drm_xe_engine_class_instance' via the eci[] array below. > > + > > + /** @reserved: MBZ */ > > + __u64 reserved[4]; > > + > > + /** @num_engines: number of engines in @eci array */ > > + __u64 num_engines; > > + > > + /** @eci: engines attached to this OA unit */ > > + struct drm_xe_engine_class_instance eci[]; > > +}; > > + > > +/** > > + * struct drm_xe_query_oa_units - describe OA units > > + * > > + * If a query is made with a struct drm_xe_device_query where .query > > + * is equal to DRM_XE_DEVICE_QUERY_OA_UNITS, then the reply uses struct > > + * drm_xe_query_oa_units in .data. > > + * > > + * When there is an @open_stream, the query returns properties specific to > > + * that @open_stream. Else default properties are returned. Removed this comment and added a code-block showing how to parse through this variable length struct in v9. > > + */ > > +struct drm_xe_query_oa_units { > > + /** @num_oa_units: number of OA units returned in oau[] */ > > + __u32 num_oa_units; > > + /** @pad: MBZ */ > > + __u32 pad; > > + /** @oa_units: OA units returned for this device */ > > + struct drm_xe_oa_unit oa_units[]; > > +}; > > + > > /** enum drm_xe_oa_format_type - OA format types */ > > enum drm_xe_oa_format_type { > > DRM_XE_OA_FMT_TYPE_OAG, > > -- > > 2.41.0 > > Thanks. -- Ashutosh