Re: [Intel-xe] [PATCH] drm/xe: Extend query ioctl to expose tile count

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Thu, Oct 05, 2023 at 03:26:50PM +0000, Kumar, Janga Rahul wrote:
> 
> 
> > -----Original Message-----
> > From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> > Sent: Wednesday, October 4, 2023 9:48 PM
> > To: Kumar, Janga Rahul <janga.rahul.kumar@intel.com>
> > Cc: intel-xe@lists.freedesktop.org; Gandi, Ramadevi
> > <ramadevi.gandi@intel.com>; Roper, Matthew D
> > <matthew.d.roper@intel.com>
> > Subject: Re: [Intel-xe] [PATCH] drm/xe: Extend query ioctl to expose tile
> > count
> > 
> > On Wed, Oct 04, 2023 at 02:21:38PM +0530, Janga Rahul Kumar wrote:
> > > Tile count can be queried by UMD's using the query ioctl.
> > > VM bind ioctl has tile mask param whose valid range can be determined
> > > based on the tile count info.
> > 
> > And what exactly UMD is doing with the tile count?
> KMD is expecting UMD to pass tile_mask(bind_ops) as part of VM bind ioctl (drm_xe_vm_bind_op param)
>         /**
>          * @tile_mask: Mask for which tiles to create binds for, 0 == All tiles,
>          * only applies to creating new VMAs
>          */
>         __u64 tile_mask;
> How can UMD be sure of the valid tile_mask range if the tile count is not Known?

it is strange that it also receives a 'region' instance above...
and it is a single region and not a region_mask.

but shouldn't we replace both region instance and tile_mask simply per region_mask?

> 
> > 
> > Please let's hold on this idea for now. I'm more trending to propose killing
> > both tile and gt concepts of the uapi. For user the important parts are the
> > hw-engine and the memory-region where the BO is placed.
> Sure. For user hw-engine and region info is most important but also the relation between the engine id and its local memory region id is also needed
> in scenarios where user want to allocate a bo on same tile region on which the engine resides.

yes, if instead of query_gt we have query_engine where engine reports the
memory regions, then we don't need tile, nor gt and we can use directly
info from engines themselves.

> 
> Thanks,
> Rahul 
> > 
> > >
> > > Cc: Aravind Iddamsetty <aravind.iddamsetty@intel.com>
> > > Cc: Matt Roper <matthew.d.roper@intel.com>
> > > Signed-off-by: Janga Rahul Kumar <janga.rahul.kumar@intel.com>
> > > ---
> > >  drivers/gpu/drm/xe/xe_query.c | 2 ++
> > >  include/uapi/drm/xe_drm.h     | 3 ++-
> > >  2 files changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/xe/xe_query.c
> > > b/drivers/gpu/drm/xe/xe_query.c index a951205100fe..ada1f26dc1d9
> > > 100644
> > > --- a/drivers/gpu/drm/xe/xe_query.c
> > > +++ b/drivers/gpu/drm/xe/xe_query.c
> > > @@ -200,6 +200,8 @@ 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_exec_queue_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 d48d8e3c898c..1d207b5398ae 100644
> > > --- a/include/uapi/drm/xe_drm.h
> > > +++ b/include/uapi/drm/xe_drm.h
> > > @@ -257,7 +257,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)
> > >  	/** @info: array of elements containing the config info */
> > >  	__u64 info[];
> > >  };
> > > --
> > > 2.25.1
> > >