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 2A2EBC4725D for ; Sat, 20 Jan 2024 03:10:10 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id CE93210EB00; Sat, 20 Jan 2024 03:10:09 +0000 (UTC) Received: from mgamail.intel.com (mgamail.intel.com [198.175.65.11]) by gabe.freedesktop.org (Postfix) with ESMTPS id 88DA710EB00 for ; Sat, 20 Jan 2024 03:10:08 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1705720209; x=1737256209; h=date:message-id:from:to:cc:subject:in-reply-to: references:mime-version:content-transfer-encoding; bh=g4Gw57N5mBwvIGUsv+ncnPzjZGpC6CsxTjjG5/4AQ4M=; b=UIEakqSibp2skkeTYoXei5ZvkJTbHbjAs0LSHjGa4eRjfQk8bwPXA6vH 5yTkz0KocrYQSr873gHkUe2ZsoSnFcCjpt8iBWBlO24GefMYi4FE7vFSY TWpfm6+EIYHh3ztV0Gd7Q4hZjGVcAGriyo4LKV7dEejx4Slob9V4phqp0 6Ezg0AJXdhKEOkCvHHLKY6l0R18QGbu1EFNVsDqOzSHxM2NOqjE3x0nel ufn8kbtOVhPO0L8VptdwVj8zBay+nyJT4KZ1wEo80bOsmDqXgQz1ApswM G0Sxwiad0K43O4DRkCooqbnJ5UArc7zjfLRQrQ3vZR3vVnrEcE4HvGaEJ Q==; X-IronPort-AV: E=McAfee;i="6600,9927,10957"; a="7586105" X-IronPort-AV: E=Sophos;i="6.05,206,1701158400"; d="scan'208";a="7586105" Received: from orviesa004.jf.intel.com ([10.64.159.144]) by orvoesa103.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 19 Jan 2024 19:10:09 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.05,206,1701158400"; d="scan'208";a="830252" Received: from orsosgc001.jf.intel.com (HELO unerlige-ril.intel.com) ([10.165.21.138]) by orviesa004-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 19 Jan 2024 19:10:08 -0800 Date: Fri, 19 Jan 2024 19:10:08 -0800 Message-ID: <85sf2sbsz3.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" To: Umesh Nerlige Ramappa Subject: Re: [PATCH 14/17] drm/xe/oa/uapi: Query OA unit properties In-Reply-To: References: <20231208064329.2387604-1-ashutosh.dixit@intel.com> <20231208064329.2387604-15-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 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: , Cc: intel-xe@lists.freedesktop.org Errors-To: intel-xe-bounces@lists.freedesktop.org Sender: "Intel-xe" On Fri, 22 Dec 2023 16:40:47 -0800, Umesh Nerlige Ramappa wrote: > Hi Umesh, > > diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h > > index 8156301df7315..5f41c5bfe5e0e 100644 > > --- a/include/uapi/drm/xe_drm.h > > +++ b/include/uapi/drm/xe_drm.h > > @@ -517,6 +517,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; > > > > @@ -1182,6 +1183,69 @@ enum drm_xe_oa_unit_type { > > DRM_XE_OA_UNIT_TYPE_OAM, > > }; > > > > +/** > > + * 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 specifi= c to > > + * that @open_stream. Else default properties are returned. > > + */ > > +struct drm_xe_query_oa_units { > > + /** @extensions: Pointer to the first extension struct, if any */ > > + __u64 extensions; > > + > > + /** @num_oa_units: number of OA units returned in oau[] */ > > + __u32 num_oa_units; > > + > > + /** @pad: MBZ */ > > + __u32 pad; > > + > > + /** @reserved: MBZ */ > > + __u64 reserved[4]; > > For some reason I have assumed reserved fields are added only at the end = of > the uApi struct, not sure though. I have removed this in v8 and also brought 'struct drm_xe_query_oa_units' in line with other query structs (see query_engines or query_mem_regions e.g.). > > > + > > + /** @oa_units: OA units returned for this device */ > > + 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; > > + > > + /** @internal_events: True if internal events are available */ > > + __u16 internal_events; > > + > > + /** @pad: MBZ */ > > + __u16 pad; > > __u16 pad[3] for 64bit alignment internal_events and pad above are also removed. > > + > > + /** @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; > > + > > + /** @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[]; > > + } oa_units[]; > > nesting of flexible arrays; not sure about that. i think some compilers m= ay > throw an error/warning. Sending an old message from Joonas offline. =46rom what I saw that old message is inconclusive. Windows guys have not explained what they are doing and anyway why should Windows UMD talk to Linux KMD. Windows can #ifdef the struct out if needed at their end. It talks about this error: https://learn.microsoft.com/en-us/cpp/error-messages/compiler-errors-1/comp= iler-error-c2233 class B { char zeroarray[]; }; B array2[100]; // C2233 The above is obviously wrong but this is not we are doing in the above struct (we have not sized the variable length struct, only indicated that a variable length struct is inside another variable length struct, which is legitimate as we index correctly into the arrays). So I am ignoring this. Please let me know if you disagree. Or if you have any suggestions about alternative ways of doing this, we could look into it. > In general, I feel the pad and reserved fields sprinkled into the > structure. If we can avoid that in a way that they are all located at the > end of the struct, I think that would look good. Not sure about the > technical aspect though. I always assumed they were meant to be at the end > (but then structs are nested anyways, so really not sure). In v8 there is only a single reserved[4] array just before num_engines in 'struct drm_xe_oa_unit'. In case we need to add extra fields later on (after that is num_engines and the variable length eci[] array which it's better to keep together). > > +}; > > + > > /** 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