[PATCH v5 1/2] drm/xe/query: Define and use a constant for the number of masks

[Matt Roper]

On Mon, Mar 25, 2024 at 04:22:03PM -0500, Lucas De Marchi wrote:
> On Mon, Mar 25, 2024 at 11:14:38AM +0000, Francois Dugast wrote:
> > Replace a magic value by a constant with an explicit name to make clear
> > what it stands for: the number of masks returned by the topology query.
> > 
> > Suggested-by: Zhanjun Dong <zhanjun.dong at intel.com>
> > Signed-off-by: Francois Dugast <francois.dugast at intel.com>
> > ---
> > drivers/gpu/drm/xe/xe_query.c | 5 ++++-
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_query.c b/drivers/gpu/drm/xe/xe_query.c
> > index df407d73e5f5..109a96212310 100644
> > --- a/drivers/gpu/drm/xe/xe_query.c
> > +++ b/drivers/gpu/drm/xe/xe_query.c
> > @@ -23,6 +23,8 @@
> > #include "xe_mmio.h"
> > #include "xe_ttm_vram_mgr.h"
> > 
> > +#define TOPO_NUMBER_OF_MASKS   (3)
> > +
> > static const u16 xe_to_user_engine_class[] = {
> > 	[XE_ENGINE_CLASS_RENDER] = DRM_XE_ENGINE_CLASS_RENDER,
> > 	[XE_ENGINE_CLASS_COPY] = DRM_XE_ENGINE_CLASS_COPY,
> > @@ -454,7 +456,8 @@ static int query_hwconfig(struct xe_device *xe,
> > static size_t calc_topo_query_size(struct xe_device *xe)
> > {
> > 	return xe->info.gt_count *
> > -		(3 * sizeof(struct drm_xe_query_topology_mask) +
> > +		(TOPO_NUMBER_OF_MASKS
> > +		 * sizeof(struct drm_xe_query_topology_mask) +
> 
> actually no.... the number here should match the entries below
> 
> > 		 sizeof_field(struct xe_gt, fuse_topo.g_dss_mask) +
> > 		 sizeof_field(struct xe_gt, fuse_topo.c_dss_mask) +
> > 		 sizeof_field(struct xe_gt, fuse_topo.eu_mask_per_dss));
> 
> what do we gain moving it elsewhere? I think it should match the number
> of types and hence maybe use same "namespace", but that also leads me to
> another odd thing:  why is drm_xe_query_topology_mask.type handled like
> a mask rather than an index?
> 
> +Matt Roper who implemented this originally afaics in commit
> eaa6ccf582f5e3252aedea81559fcbf71ed9ccde.

I think you're referring to the
        #define DRM_XE_TOPO_DSS_GEOMETRY        (1 << 0)
        #define DRM_XE_TOPO_DSS_COMPUTE         (1 << 1)
        #define DRM_XE_TOPO_EU_PER_DSS          (1 << 2)

rather than just using 1, 2, 3, etc.?  If I remember correctly, very
early plans for the query uapi was that userspace would ask for specific
things they wanted (by providing a mask of the masks they wanted to
receive).  When we scrapped that idea and decided to just shove
everything at userspace unconditionally and let them ignore the parts
they don't care about or don't recognize, we just never remembered to
come back and change the numbering here.  There isn't any reason we need
to keep this to powers of 2 anymore, so future things that get added
(like L3 bank masks) should be free to grab the currently unused ID 3.


Matt

> 
> Lucaas De Marchi
> 
> 
> > -- 
> > 2.34.1
> > 

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