Re: [Intel-xe] [PATCH v2 34/50] drm/xe/uapi: Move memory_region masks from GT to engine

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

On Thu, Nov 09, 2023 at 04:35:19PM +0000, Souza, Jose wrote:
> On Thu, 2023-11-09 at 08:29 -0800, José Roberto de Souza wrote:
> > On Fri, 2023-11-03 at 14:34 +0000, Francois Dugast wrote:
> > > From: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > 
> > > In the Tiled platforms, the memory is more tied to the Tile
> > > than to the GT.
> > > The distance (near vs far) makes more sense from the Engine
> > > perspective than from the GT perspective.
> > 
> > why not add a uAPI to query tile information?
> > this is duplicating a tile information onto every engine of that tile.
> > we could leave reserved fields in the tile uAPI to include additional information that might be relevant in future.

This is not necessarily a tile information. In PVC, truly the mem_region is tied to the tile,
but we don't want to fix the uapi in only one platform.
Like in the previous, the mem_region was per GT. who knows the future?!

But the engine needs the information on which mem_region could be better,
regardless of if it lives along the same gt, or the same tile, or outside.
So, near and far sounded the most generic and future proof way.

> 
> other issue here and in other patches of this huge patch series.
> 
> a previous patch in this series renamed near_mem_regions, then this one moves it to other struct... please drop the first patch and rename and move it
> into a single patch.

okay, rename and move in the same patch kind of makes sense. and patches
could be squashed together. But when doing the IGT on the side, I felt
that small changes were better, so renames goes with sed commands and
the patch was small and clear. But I don't mind if they get squashed in
the end.

> 
> a series as big as this one will cause reviews in KMD and UMD to take a while...

that's unfortunate indeed. But big patches also don't help much to speed up reviews.

> 
> > 
> > > 
> > > So, let's move this out from the GT and into the engine info.
> > > 
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > ---
> > >  drivers/gpu/drm/xe/xe_query.c | 14 +++++++-------
> > >  include/uapi/drm/xe_drm.h     | 27 ++++++++++++++-------------
> > >  2 files changed, 21 insertions(+), 20 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/xe/xe_query.c b/drivers/gpu/drm/xe/xe_query.c
> > > index aa5743e2e4d0..49a9b36f1193 100644
> > > --- a/drivers/gpu/drm/xe/xe_query.c
> > > +++ b/drivers/gpu/drm/xe/xe_query.c
> > > @@ -217,6 +217,13 @@ static int query_engines(struct xe_device *xe,
> > >  				hwe->logical_instance;
> > >  			hw_engine_info[i].instance.gt_id = gt->info.id;
> > >  			hw_engine_info[i].instance.pad = 0;
> > > +			if (!IS_DGFX(xe))
> > > +				hw_engine_info[i].near_mem_regions = 0x1;
> > > +			else
> > > +				hw_engine_info[i].near_mem_regions =
> > > +					BIT(gt_to_tile(gt)->id) << 1;
> > > +			hw_engine_info[i].far_mem_regions = xe->info.mem_region_mask ^
> > > +				hw_engine_info[i].near_mem_regions;
> > >  			memset(hw_engine_info->reserved, 0, sizeof(hw_engine_info->reserved));
> > >  
> > >  			i++;
> > > @@ -377,13 +384,6 @@ static int query_gt_list(struct xe_device *xe, struct drm_xe_device_query *query
> > >  			gt_list->gt_list[id].type = DRM_XE_QUERY_GT_TYPE_MAIN;
> > >  		gt_list->gt_list[id].gt_id = gt->info.id;
> > >  		gt_list->gt_list[id].clock_freq = gt->info.clock_freq;
> > > -		if (!IS_DGFX(xe))
> > > -			gt_list->gt_list[id].near_mem_regions = 0x1;
> > > -		else
> > > -			gt_list->gt_list[id].near_mem_regions =
> > > -				BIT(gt_to_tile(gt)->id) << 1;
> > > -		gt_list->gt_list[id].far_mem_regions = xe->info.mem_region_mask ^
> > > -			gt_list->gt_list[id].near_mem_regions;
> > >  	}
> > >  
> > >  	if (copy_to_user(query_ptr, gt_list, size)) {
> > > diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
> > > index 5164ed150a2e..8e84ef6fd46e 100644
> > > --- a/include/uapi/drm/xe_drm.h
> > > +++ b/include/uapi/drm/xe_drm.h
> > > @@ -228,6 +228,20 @@ struct drm_xe_query_engine_info {
> > >  	/** @instance: The @drm_xe_engine_class_instance */
> > >  	struct drm_xe_engine_class_instance instance;
> > >  
> > > +	/**
> > > +	 * @near_mem_regions: Bit mask of instances from
> > > +	 * drm_xe_query_mem_regions that is near this engine.
> > > +	 */
> > > +	__u64 near_mem_regions;
> > > +	/**
> > > +	 * @far_mem_regions: Bit mask of instances from
> > > +	 * drm_xe_query_mem_regions that is far from this engine.
> > > +	 * In general, it has extra indirections when compared to the
> > > +	 * @near_mem_regions. For a discrete device this could mean system
> > > +	 * memory and memory living in a different Tile.
> > > +	 */
> > > +	__u64 far_mem_regions;
> > > +
> > >  	/** @reserved: Reserved */
> > >  	__u64 reserved[3];
> > >  };
> > > @@ -401,19 +415,6 @@ struct drm_xe_query_gt {
> > >  	__u16 gt_id;
> > >  	/** @clock_freq: A clock frequency for timestamp */
> > >  	__u32 clock_freq;
> > > -	/**
> > > -	 * @near_mem_regions: Bit mask of instances from
> > > -	 * drm_xe_query_mem_regions that is near the current engines of this GT.
> > > -	 */
> > > -	__u64 near_mem_regions;
> > > -	/**
> > > -	 * @far_mem_regions: Bit mask of instances from
> > > -	 * drm_xe_query_mem_regions that is far from the engines of this GT.
> > > -	 * In general, it has extra indirections when compared to the
> > > -	 * @near_mem_regions. For a discrete device this could mean system
> > > -	 * memory and memory living in a different Tile.
> > > -	 */
> > > -	__u64 far_mem_regions;
> > >  	/** @reserved: Reserved */
> > >  	__u64 reserved[8];
> > >  };
> > 
>