From: Lucas De Marchi <lucas.demarchi@intel.com>
To: "Dixit, Ashutosh" <ashutosh.dixit@intel.com>
Cc: <intel-xe@lists.freedesktop.org>,
Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
Subject: Re: [PATCH 12/17] drm/xe/oa/uapi: Query OA unit properties
Date: Thu, 25 Apr 2024 08:10:26 -0500 [thread overview]
Message-ID: <yg5xs4zqutz2qmaeingpennxj6n7da2jqjl3f5d2i6o43n5xuk@yo3cwwfkv7ec> (raw)
In-Reply-To: <851q6umjez.wl-ashutosh.dixit@intel.com>
On Wed, Apr 24, 2024 at 04:26:44PM GMT, Ashutosh Dixit wrote:
>On Thu, 14 Mar 2024 18:35:13 -0700, Ashutosh Dixit wrote:
>>
>> diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
>> index 4bfa06ebf6da..54d0912f2ba8 100644
>>
>> +/**
>> + * struct drm_xe_oa_unit - describe OA unit
>> + */
>> +struct drm_xe_oa_unit {
>> + /** @extensions: Pointer to the first extension struct, if any */
>> + __u64 extensions;
>> +
>> + /** @oa_unit_id: OA unit ID */
>> + __u32 oa_unit_id;
>> +
>> + /** @oa_unit_type: OA unit type of @drm_xe_oa_unit_type */
>> + __u32 oa_unit_type;
>> +
>> + /** @capabilities: OA capabilities bit-mask */
>> + __u64 capabilities;
>> +#define DRM_XE_OA_CAPS_BASE (1 << 0)
>> +
>> + /** @oa_timestamp_freq: OA timestamp freq */
>> + __u64 oa_timestamp_freq;
>> +
>> + /** @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.
>> + *
>> + * OA unit properties for all OA units can be accessed using a code block
>> + * such as the one below:
>> + *
>> + * .. code-block:: C
>> + *
>> + * struct drm_xe_query_oa_units *qoa;
>> + * struct drm_xe_oa_unit *oau;
>> + * u8 *poau;
>> + *
>> + * // malloc qoa and issue DRM_XE_DEVICE_QUERY_OA_UNITS. Then:
>> + * poau = (u8 *)&qoa->oa_units[0];
>> + * for (int i = 0; i < qoa->num_oa_units; i++) {
>> + * oau = (struct drm_xe_oa_unit *)poau;
>> + * // Access 'struct drm_xe_oa_unit' fields here
>> + * poau += sizeof(*oau) + oau->num_engines * sizeof(oau->eci[0]);
>> + * }
>> + */
>> +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;
>> + /** @oa_units: OA units returned for this device */
>> + struct drm_xe_oa_unit oa_units[];
>> +};
>
>It has been pointed out that the doubly nested flexible arrays used here
>break compilation on Windows due to this MSVC issue:
>
>https://learn.microsoft.com/en-us/cpp/error-messages/compiler-errors-1/compiler-error-c2233?view=msvc-170
>
>Umesh had previously already pointed this out here:
>
>https://patchwork.freedesktop.org/patch/571290/?series=121084&rev=7#comment_1048210
>
>In any case, this is considered unacceptabe and needs to change.
>
>Current options:
>
>a. Include a "next" pointer in 'struct drm_xe_oa_unit' (suggested by
> Lucas).
>
> This works except people not familiar with the MSVC issue might wonder
> why it is not simply an array, or why we are using a linked list
> construct for something which is actually an array.
it's actually quite a common approach when you are parsing binary files
or protocol buffers: you have a header that points to the first item
and each item points to the next one. Extending it to this query just
seemed natural to me, but I don't have a strong opinion right now on
which of them is better.
>
>b. I am thinking another option may be to just do this:
>
> diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
> index 03c559af2027..6c864dc1b65f 100644
> --- a/include/uapi/drm/xe_drm.h
> +++ b/include/uapi/drm/xe_drm.h
> @@ -1506,8 +1506,8 @@ struct drm_xe_query_oa_units {
> __u32 num_oa_units;
> /** @pad: MBZ */
> __u32 pad;
> - /** @oa_units: OA units returned for this device */
> - struct drm_xe_oa_unit oa_units[];
> + /** @oa_units: struct drm_xe_oa_unit array returned for this device */
> + __u8 oa_units[];
given the expected use is to use just as an address that is then cast to
the actual type, I think that should work fine even if it looks ugly.
One caveat is about alignment: we'd in theory need to make this u64 so
it matches the alignment. Not a real issue here since all the UAPI
structs have pads to cover holes.
Lucas De Marchi
> };
>
> Though the next pointer has the advantage of it being simpler to reach
> the nth element of the array.
>
>So I am still trying to figure out the least hacky way to change this. If
>you have any suggestions/opinion about this please respond.
>
>Thanks.
>--
>Ashutosh
next prev parent reply other threads:[~2024-04-25 13:10 UTC|newest]
Thread overview: 53+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-15 1:35 [PATCH 00/17] Add OA functionality to Xe Ashutosh Dixit
2024-03-15 1:35 ` [PATCH 01/17] drm/xe/perf/uapi: "Perf" layer to support multiple perf counter stream types Ashutosh Dixit
2024-03-15 1:35 ` [PATCH 02/17] drm/xe/perf/uapi: Add perf_stream_paranoid sysctl Ashutosh Dixit
2024-03-15 1:35 ` [PATCH 03/17] drm/xe/oa/uapi: Add OA data formats Ashutosh Dixit
2024-03-15 1:35 ` [PATCH 04/17] drm/xe/oa/uapi: Initialize OA units Ashutosh Dixit
2024-03-15 1:35 ` [PATCH 05/17] drm/xe/oa/uapi: Add/remove OA config perf ops Ashutosh Dixit
2024-03-15 1:35 ` [PATCH 06/17] drm/xe/oa/uapi: Define and parse OA stream properties Ashutosh Dixit
2024-03-15 1:35 ` [PATCH 07/17] drm/xe/oa: OA stream initialization (OAG) Ashutosh Dixit
2024-03-15 1:35 ` [PATCH 08/17] drm/xe/oa/uapi: Expose OA stream fd Ashutosh Dixit
2024-03-15 1:35 ` [PATCH 09/17] drm/xe/oa/uapi: Read file_operation Ashutosh Dixit
2024-03-15 1:35 ` [PATCH 10/17] drm/xe/oa: Add OAR support Ashutosh Dixit
2024-03-15 1:35 ` [PATCH 11/17] drm/xe/oa: Add OAC support Ashutosh Dixit
2024-03-15 1:35 ` [PATCH 12/17] drm/xe/oa/uapi: Query OA unit properties Ashutosh Dixit
2024-04-24 23:26 ` Dixit, Ashutosh
2024-04-25 13:10 ` Lucas De Marchi [this message]
2024-03-15 1:35 ` [PATCH 13/17] drm/xe/oa/uapi: OA buffer mmap Ashutosh Dixit
2024-03-15 1:35 ` [PATCH 14/17] drm/xe/oa: Add MMIO trigger support Ashutosh Dixit
2024-03-15 1:35 ` [PATCH 15/17] drm/xe/oa: Override GuC RC with OA on PVC Ashutosh Dixit
2024-03-15 1:35 ` [PATCH 16/17] drm/xe/oa: Changes to OA_TAKEN Ashutosh Dixit
2024-03-15 1:35 ` [PATCH 17/17] drm/xe/oa: Enable Xe2+ overrun mode Ashutosh Dixit
2024-03-15 1:53 ` ✓ CI.Patch_applied: success for Add OA functionality to Xe (rev13) Patchwork
2024-03-15 1:53 ` ✗ CI.checkpatch: warning " Patchwork
2024-03-15 1:54 ` ✓ CI.KUnit: success " Patchwork
2024-03-15 2:05 ` ✓ CI.Build: " Patchwork
2024-03-15 2:07 ` ✗ CI.Hooks: failure " Patchwork
2024-03-15 2:08 ` ✓ CI.checksparse: success " Patchwork
2024-03-15 2:34 ` ✓ CI.BAT: " Patchwork
2024-05-17 18:42 ` [PATCH 00/17] Add OA functionality to Xe Souza, Jose
2024-05-18 1:42 ` Dixit, Ashutosh
2024-05-21 14:43 ` Souza, Jose
2024-05-21 14:47 ` Souza, Jose
2024-05-21 16:10 ` Dixit, Ashutosh
2024-05-21 16:29 ` Souza, Jose
2024-05-21 16:43 ` Dixit, Ashutosh
2024-05-21 17:39 ` Souza, Jose
2024-05-21 18:02 ` Dixit, Ashutosh
2024-05-21 18:11 ` Dixit, Ashutosh
2024-05-21 19:01 ` Souza, Jose
2024-05-21 18:48 ` Souza, Jose
2024-05-21 20:24 ` Dixit, Ashutosh
2024-05-21 21:00 ` Souza, Jose
2024-05-22 2:28 ` Dixit, Ashutosh
2024-05-22 16:08 ` Souza, Jose
2024-05-22 4:42 ` Dixit, Ashutosh
2024-05-22 16:13 ` Souza, Jose
2024-05-22 18:50 ` Dixit, Ashutosh
2024-05-22 19:30 ` Souza, Jose
2024-05-25 1:16 ` Dixit, Ashutosh
2024-05-27 17:02 ` Souza, Jose
2024-05-21 19:58 ` Souza, Jose
-- strict thread matches above, loose matches on Subject: below --
2024-05-27 1:43 [PATCH v15 " Ashutosh Dixit
2024-05-27 1:43 ` [PATCH 12/17] drm/xe/oa/uapi: Query OA unit properties Ashutosh Dixit
2024-05-24 19:01 [PATCH v14 00/17] Add OA functionality to Xe Ashutosh Dixit
2024-05-24 19:01 ` [PATCH 12/17] drm/xe/oa/uapi: Query OA unit properties Ashutosh Dixit
2024-03-12 3:38 [PATCH v12 00/17] Add OA functionality to Xe Ashutosh Dixit
2024-03-12 3:39 ` [PATCH 12/17] drm/xe/oa/uapi: Query OA unit properties Ashutosh Dixit
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=yg5xs4zqutz2qmaeingpennxj6n7da2jqjl3f5d2i6o43n5xuk@yo3cwwfkv7ec \
--to=lucas.demarchi@intel.com \
--cc=ashutosh.dixit@intel.com \
--cc=intel-xe@lists.freedesktop.org \
--cc=umesh.nerlige.ramappa@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).