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

Tue Jun 13 08:37:25 UTC 2023



> -----Original Message-----
> From: Dixit, Ashutosh <ashutosh.dixit at intel.com>
> Sent: 08 June 2023 00:47
> To: Ghimiray, Himal Prasad <himal.prasad.ghimiray at intel.com>
> Cc: intel-xe at lists.freedesktop.org
> Subject: Re: [Intel-xe] [PATCH 1/1] drm/xe: Report tile count from device
> query api.
> 
> On Wed, 07 Jun 2023 11:31:15 -0700, Matt Roper wrote:
> >
> > 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
> 
> In i915 e.g. the number of gt's was never exposed, userland would just count
> the number of gt/gtN directories in sysfs to get number of gt's (see how this
> is done in IGT). So something like that is possible for tiles too (see email
> thread "Re: [PATCH 0/3] drm/xe: Add sysfs entry to report per gt memory
> size"), though like Matt says needs discussion with UMD teams to see which
> particular option to go with.

Thanks Ashutosh, 
For the time being I will count the tiles on the basis of sysfs entries. 
Once we conclude on the tile query uapi will make required changes in IGT 
> 
> Sometimes I think we should put whatever we can in sysfs and minimize
> things exposed via the query ioctl. Though again not sure about the
> tradeoff's of one way vs another.
> 
> Ashutosh
> 
> > > 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