[Intel-xe] [PATCH v2 05/14] drm/xe/uapi: Align on a common way to return arrays (memory regions)

[Francois Dugast]

On Tue, Nov 28, 2023 at 03:51:13PM -0500, Rodrigo Vivi wrote:
> On Fri, Nov 24, 2023 at 06:19:47PM +0000, Souza, Jose wrote:
> > On Wed, 2023-11-22 at 14:38 +0000, Francois Dugast wrote:
> > > The uAPI provides queries which return arrays of elements. As of now
> > > the format used in the struct is different depending on which element
> > > is queried. Fix this for memory regions by applying the pattern below:
> > > 
> > >     struct drm_xe_query_Xs {
> > >        __u32 num_Xs;
> > >        struct drm_xe_X Xs[];
> > >        ...
> > >     }
> > > 
> > > This removes "query" in the name of struct drm_xe_query_mem_region
> > > as it is not returned from the query IOCTL. There is no functional
> > > change.
> > > 
> > > v2: Only rename drm_xe_query_mem_region to drm_xe_mem_region
> > >     (José Roberto de Souza)
> > > 
> > > Signed-off-by: Francois Dugast <francois.dugast at intel.com>
> > > ---
> > >  drivers/gpu/drm/xe/xe_query.c | 36 ++++++++++++++++++-----------------
> > >  include/uapi/drm/xe_drm.h     | 12 ++++++------
> > >  2 files changed, 25 insertions(+), 23 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/xe/xe_query.c b/drivers/gpu/drm/xe/xe_query.c
> > > index 0cbfeaeb1330..f321ed4d3b0b 100644
> > > --- a/drivers/gpu/drm/xe/xe_query.c
> > > +++ b/drivers/gpu/drm/xe/xe_query.c
> > > @@ -240,11 +240,11 @@ static size_t calc_mem_regions_size(struct xe_device *xe)
> > >  		if (ttm_manager_type(&xe->ttm, i))
> > >  			num_managers++;
> > >  
> > > -	return offsetof(struct drm_xe_query_mem_regions, regions[num_managers]);
> > > +	return offsetof(struct drm_xe_query_mem_regions, mem_regions[num_managers]);
> > >  }
> > >  
> > >  static int query_mem_regions(struct xe_device *xe,
> > > -			     struct drm_xe_device_query *query)
> > > +			    struct drm_xe_device_query *query)
> > >  {
> > >  	size_t size = calc_mem_regions_size(xe);
> > >  	struct drm_xe_query_mem_regions *usage;
> > > @@ -265,36 +265,38 @@ static int query_mem_regions(struct xe_device *xe,
> > >  		return -ENOMEM;
> > >  
> > >  	man = ttm_manager_type(&xe->ttm, XE_PL_TT);
> > > -	usage->regions[0].mem_class = DRM_XE_MEM_REGION_CLASS_SYSMEM;
> > > -	usage->regions[0].instance = 0;
> > > -	usage->regions[0].min_page_size = PAGE_SIZE;
> > > -	usage->regions[0].total_size = man->size << PAGE_SHIFT;
> > > +	usage->mem_regions[0].mem_class = DRM_XE_MEM_REGION_CLASS_SYSMEM;
> > 
> > nit: can you also rename 'usage' to something else, it was named 'usage' because of the old query name.
> 
> my bad for having forgotten this 'usage' in here. Let's fix that in a follow-up after this series is merged.
> 
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi at intel.com>

Thanks, it is already fixed in the next revision.

Francois

> 
> > 
> > 
> > > +	usage->mem_regions[0].instance = 0;
> > > +	usage->mem_regions[0].min_page_size = PAGE_SIZE;
> > > +	usage->mem_regions[0].total_size = man->size << PAGE_SHIFT;
> > >  	if (perfmon_capable())
> > > -		usage->regions[0].used = ttm_resource_manager_usage(man);
> > > -	usage->num_regions = 1;
> > > +		usage->mem_regions[0].used = ttm_resource_manager_usage(man);
> > > +	usage->num_mem_regions = 1;
> > >  
> > >  	for (i = XE_PL_VRAM0; i <= XE_PL_VRAM1; ++i) {
> > >  		man = ttm_manager_type(&xe->ttm, i);
> > >  		if (man) {
> > > -			usage->regions[usage->num_regions].mem_class =
> > > +			usage->mem_regions[usage->num_mem_regions].mem_class =
> > >  				DRM_XE_MEM_REGION_CLASS_VRAM;
> > > -			usage->regions[usage->num_regions].instance =
> > > -				usage->num_regions;
> > > -			usage->regions[usage->num_regions].min_page_size =
> > > +			usage->mem_regions[usage->num_mem_regions].instance =
> > > +				usage->num_mem_regions;
> > > +			usage->mem_regions[usage->num_mem_regions].min_page_size =
> > >  				xe->info.vram_flags & XE_VRAM_FLAGS_NEED64K ?
> > >  				SZ_64K : PAGE_SIZE;
> > > -			usage->regions[usage->num_regions].total_size =
> > > +			usage->mem_regions[usage->num_mem_regions].total_size =
> > >  				man->size;
> > >  
> > >  			if (perfmon_capable()) {
> > >  				xe_ttm_vram_get_used(man,
> > > -						     &usage->regions[usage->num_regions].used,
> > > -						     &usage->regions[usage->num_regions].cpu_visible_used);
> > > +						     &usage->mem_regions
> > > +						     [usage->num_mem_regions].used,
> > > +						     &usage->mem_regions
> > > +						     [usage->num_mem_regions].cpu_visible_used);
> > >  			}
> > >  
> > > -			usage->regions[usage->num_regions].cpu_visible_size =
> > > +			usage->mem_regions[usage->num_mem_regions].cpu_visible_size =
> > >  				xe_ttm_vram_get_cpu_visible_size(man);
> > > -			usage->num_regions++;
> > > +			usage->num_mem_regions++;
> > >  		}
> > >  	}
> > >  
> > > diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
> > > index da10d946930b..a9bbdf141fe2 100644
> > > --- a/include/uapi/drm/xe_drm.h
> > > +++ b/include/uapi/drm/xe_drm.h
> > > @@ -182,10 +182,10 @@ enum drm_xe_memory_class {
> > >  };
> > >  
> > >  /**
> > > - * struct drm_xe_query_mem_region - Describes some region as known to
> > > + * struct drm_xe_mem_region - Describes some region as known to
> > >   * the driver.
> > >   */
> > > -struct drm_xe_query_mem_region {
> > > +struct drm_xe_mem_region {
> > >  	/**
> > >  	 * @mem_class: The memory class describing this region.
> > >  	 *
> > > @@ -322,12 +322,12 @@ struct drm_xe_query_engine_cycles {
> > >   * struct drm_xe_query_mem_regions in .data.
> > >   */
> > >  struct drm_xe_query_mem_regions {
> > > -	/** @num_regions: number of memory regions returned in @regions */
> > > -	__u32 num_regions;
> > > +	/** @num_mem_regions: number of memory regions returned in @mem_regions */
> > > +	__u32 num_mem_regions;
> > >  	/** @pad: MBZ */
> > >  	__u32 pad;
> > > -	/** @regions: The returned regions for this device */
> > > -	struct drm_xe_query_mem_region regions[];
> > > +	/** @mem_regions: The returned memory regions for this device */
> > > +	struct drm_xe_mem_region mem_regions[];
> > >  };
> > >  
> > >  /**
> >