From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mgamail.intel.com (mgamail.intel.com [192.198.163.7]) by gabe.freedesktop.org (Postfix) with ESMTPS id 1877010E5E8 for ; Thu, 28 Sep 2023 08:19:57 +0000 (UTC) Message-ID: <7c6c7da3-f19e-f718-053e-0f2ba8951f50@linux.intel.com> Date: Thu, 28 Sep 2023 09:19:52 +0100 MIME-Version: 1.0 Content-Language: en-US To: Rodrigo Vivi References: <20230926130054.6-1-francois.dugast@intel.com> <20230926130054.6-12-francois.dugast@intel.com> From: Tvrtko Ursulin In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [igt-dev] [PATCH v3 11/24] drm-uapi/xe: Replace useless 'instance' per unique gt_id List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: igt-dev@lists.freedesktop.org Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" List-ID: On 27/09/2023 17:53, Rodrigo Vivi wrote: > On Tue, Sep 26, 2023 at 05:47:22PM +0100, Tvrtko Ursulin wrote: >> >> On 26/09/2023 14:00, Francois Dugast wrote: >>> From: Rodrigo Vivi >>> >>> Align with commit ("drm/xe/uapi: Replace useless 'instance' per unique gt_id") >>> >>> Signed-off-by: Rodrigo Vivi >>> Signed-off-by: Francois Dugast >>> --- >>> include/drm-uapi/xe_drm.h | 65 ++++++++++++++++++++++++++------------- >>> tests/intel/xe_query.c | 2 +- >>> 2 files changed, 44 insertions(+), 23 deletions(-) >>> >>> diff --git a/include/drm-uapi/xe_drm.h b/include/drm-uapi/xe_drm.h >>> index 13c693393..68cc5e051 100644 >>> --- a/include/drm-uapi/xe_drm.h >>> +++ b/include/drm-uapi/xe_drm.h >>> @@ -337,6 +337,47 @@ struct drm_xe_query_config { >>> __u64 info[]; >>> }; >>> +/** >>> + * struct drm_xe_query_gt - describe an individual GT. >>> + * >>> + * To be used with drm_xe_query_gts, which will return a list with all the >>> + * existing GT individual descriptions. >>> + * Graphics Technology (GT) is a subset of a GPU/tile that is responsible for >>> + * implementing graphics and/or media operations. >>> + */ >>> +struct drm_xe_query_gt { >>> +#define XE_QUERY_GT_TYPE_MAIN 0 >>> +#define XE_QUERY_GT_TYPE_REMOTE 1 >>> +#define XE_QUERY_GT_TYPE_MEDIA 2 >>> + /** @type: GT type: Main, Remote, or Media */ >>> + __u16 type; >>> + /** @gt_id: Unique ID of this GT within the PCI Device */ >>> + __u16 gt_id; >>> + /** @clock_freq: A clock frequency for timestamp */ >>> + __u32 clock_freq; >>> + /** @features: Reserved for future information about GT features */ >>> + __u64 features; >>> + /** >>> + * @native_mem_regions: Bit mask of instances from >>> + * drm_xe_query_mem_usage that lives on the same GPU/Tile and have >>> + * direct access. >>> + */ >>> + __u64 native_mem_regions; >> >> s/native/local/ ? >> >> Although what was wrong with distance query? It was more future proof (can't >> ever end up with fast, slow, slower, .. ) and avoids a bit of a vague >> non-technical names like "slow". >> >>> + /** >>> + * @slow_mem_regions: Bit mask of instances from >>> + * drm_xe_query_mem_usage that this GT can indirectly access, although >>> + * they live on a different GPU/Tile. >>> + */ >>> + __u64 slow_mem_regions; >>> + /** >>> + * @inaccessible_mem_regions: Bit mask of instances from >>> + * drm_xe_query_mem_usage that is not accessible by this GT at all. >>> + */ >>> + __u64 inaccessible_mem_regions; >> >> Equal to ~(native | slow) so redundant? >> >> Btw drm_xe_query_mem_usage is just a list of regions, nothing about usage >> like memory usage? > > I agree with all your comments here. The way xe is currently handling > memory regions and mixing gt in the middle is strange and I'm going to > scrutinize and change that on a follow up. > > This patch here now is just to change the name to the gt_id and this > IGT change ended up picking together the movement of the gt struct > out of the list struct definition. > > So, basically the only relevant portion of this patch is s/instance/gt_id. > Everything else should be a follow-up work. Ah okay. I was asked to review the gem_wsim adaptation for xe so went looking for xe uapi. My bad looking at the wrong place. And from what you say I guess I can freely hold of looking into details until the uapi settles down. Regards, Tvrtko > >> >> Regards, >> >> Tvrtko >> >>> + /** @reserved: Reserved */ >>> + __u64 reserved[8]; >>> +}; >>> + >>> /** >>> * struct drm_xe_query_gts - describe GTs >>> * >>> @@ -347,30 +388,10 @@ struct drm_xe_query_config { >>> struct drm_xe_query_gts { >>> /** @num_gt: number of GTs returned in gts */ >>> __u32 num_gt; >>> - >>> /** @pad: MBZ */ >>> __u32 pad; >>> - >>> - /** >>> - * @gts: The GTs returned for this device >>> - * >>> - * TODO: convert drm_xe_query_gt to proper kernel-doc. >>> - * TODO: Perhaps info about every mem region relative to this GT? e.g. >>> - * bandwidth between this GT and remote region? >>> - */ >>> - struct drm_xe_query_gt { >>> -#define XE_QUERY_GT_TYPE_MAIN 0 >>> -#define XE_QUERY_GT_TYPE_REMOTE 1 >>> -#define XE_QUERY_GT_TYPE_MEDIA 2 >>> - __u16 type; >>> - __u16 instance; >>> - __u32 clock_freq; >>> - __u64 features; >>> - __u64 native_mem_regions; /* bit mask of instances from drm_xe_query_mem_usage */ >>> - __u64 slow_mem_regions; /* bit mask of instances from drm_xe_query_mem_usage */ >>> - __u64 inaccessible_mem_regions; /* bit mask of instances from drm_xe_query_mem_usage */ >>> - __u64 reserved[8]; >>> - } gts[]; >>> + /** @gts: The GT list returned for this device */ >>> + struct drm_xe_query_gt gts[]; >>> }; >>> /** >>> diff --git a/tests/intel/xe_query.c b/tests/intel/xe_query.c >>> index acf069f46..eb8d52897 100644 >>> --- a/tests/intel/xe_query.c >>> +++ b/tests/intel/xe_query.c >>> @@ -279,7 +279,7 @@ test_query_gts(int fd) >>> for (i = 0; i < gts->num_gt; i++) { >>> igt_info("type: %d\n", gts->gts[i].type); >>> - igt_info("instance: %d\n", gts->gts[i].instance); >>> + igt_info("gt_id: %d\n", gts->gts[i].gt_id); >>> igt_info("clock_freq: %u\n", gts->gts[i].clock_freq); >>> igt_info("features: 0x%016llx\n", gts->gts[i].features); >>> igt_info("native_mem_regions: 0x%016llx\n",