[Intel-xe] [PATCH 1/1] drm/xe: Report tile count from device query api.

[Matt Roper]

On Wed, Jun 07, 2023 at 04:49:09AM +0000, Ghimiray, Himal Prasad wrote:
> 
> 
> > -----Original Message-----
> > From: Roper, Matthew D <matthew.d.roper at intel.com>
> > Sent: 07 June 2023 01:26
> > To: Ghimiray, Himal Prasad <himal.prasad.ghimiray at intel.com>
> > Cc: intel-xe at lists.freedesktop.org; Dugast, Francois
> > <francois.dugast at intel.com>
> > Subject: Re: [PATCH 1/1] drm/xe: Report tile count from device query api.
> > 
> > On Tue, Jun 06, 2023 at 03:19:29PM +0530, Himal Prasad Ghimiray wrote:
> > > Cc: Matt Roper <matthew.d.roper at intel.com>
> > > Cc: Francois Dugast <francois.dugast at intel.com>
> > >
> > > Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray at intel.com>
> > 
> > Does userspace actually need this reported in the device query?  It seems more
> > likely that they're going to need a tile-based equivalent of 'drm_xe_query_gt'
> > that returns not only the tile count, but also further details about each tile.  If
> > that's the case, then also adding this count as an extra field to the device-level
> > query seems redundant (and the existing GT count seems like it's probably
> > redundant as well).
> Hi Matt,
> 
> From userspace  perspective the requirement is to know number of tiles supported by device 
> and agreed 'drm_xe_query_tile' should provide this info along with other details.
> Can you please provide your inputs on what other details can be part of tile query ? 

It's really something that would need to be discussed and agreed upon
with the userspace teams, but I imagine details about VRAM might be
something they'd want to receive via such a query.

> > 
> > Are there patches for any of the userspace components yet that show
> > how/when they'd use this?
> https://patchwork.freedesktop.org/series/118927/ exposes per tile vram addr_range via sysfs. 
> While writing IGT to test this sysfs entry, I came across the requirement. IGT patches are WIP.
> 
> Just a thought, will it makes sense to expose number of tiles device supports by sysfs ? 
> Something like: <device-properties>/number_tiles
> IMO this info should be available to userspace even without making any code changes .
> There may be other such potential candidates which can fall under device-properties.

That's really something that we need guidance on from the userspace
teams (mesa, media, compute, etc.).  It's hard for us kernel developers
to really know exactly what the best uapi would be for userspace.
That's part of why the DRM subsystem requires that any uapi change
proposed for i915 or Xe can't be added to the driver until the
corresponding userspace patches have also been developed and fully
reviewed.  We don't want to write UAPI that winds up not really meeting
userspace's needs and then be stuck supporting that broken uapi forever.


Matt

> 
> Himal 
> > 
> > 
> > Matt
> > 
> > > ---
> > >  drivers/gpu/drm/xe/xe_query.c | 1 +
> > >  include/uapi/drm/xe_drm.h     | 3 ++-
> > >  2 files changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/xe/xe_query.c
> > > b/drivers/gpu/drm/xe/xe_query.c index c4165fa3428e..54efbefd663f
> > > 100644
> > > --- a/drivers/gpu/drm/xe/xe_query.c
> > > +++ b/drivers/gpu/drm/xe/xe_query.c
> > > @@ -197,6 +197,7 @@ static int query_config(struct xe_device *xe, struct
> > drm_xe_device_query *query)
> > >  		hweight_long(xe->info.mem_region_mask);
> > >  	config->info[XE_QUERY_CONFIG_MAX_ENGINE_PRIORITY] =
> > >  		xe_engine_device_get_max_priority(xe);
> > > +	config->info[XE_QUERY_CONFIG_TILE_COUNT] = xe->info.tile_count;
> > >
> > >  	if (copy_to_user(query_ptr, config, size)) {
> > >  		kfree(config);
> > > diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
> > > index 0ebc50beb5e5..8e552aa55037 100644
> > > --- a/include/uapi/drm/xe_drm.h
> > > +++ b/include/uapi/drm/xe_drm.h
> > > @@ -185,7 +185,8 @@ struct drm_xe_query_config {
> > >  #define XE_QUERY_CONFIG_GT_COUNT		4
> > >  #define XE_QUERY_CONFIG_MEM_REGION_COUNT	5
> > >  #define XE_QUERY_CONFIG_MAX_ENGINE_PRIORITY	6
> > > -#define XE_QUERY_CONFIG_NUM_PARAM
> > 	XE_QUERY_CONFIG_MAX_ENGINE_PRIORITY + 1
> > > +#define XE_QUERY_CONFIG_TILE_COUNT		7
> > > +#define XE_QUERY_CONFIG_NUM_PARAM
> > 	XE_QUERY_CONFIG_TILE_COUNT + 1
> > >  	__u64 info[];
> > >  };
> > >
> > > --
> > > 2.25.1
> > >
> > 
> > --
> > Matt Roper
> > Graphics Software Engineer
> > Linux GPU Platform Enablement
> > Intel Corporation

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation