Re: [PATCH v3 4/6] drm/xe: Add misc functions to support read of specific DSS registers

["Souza, Jose" <jose.souza@intel.com>]

On Tue, 2024-01-30 at 10:54 -0500, Dong, Zhanjun wrote:
> 
> On 2024-01-30 9:42 a.m., Souza, Jose wrote:
> > On Mon, 2024-01-29 at 13:24 -0800, Matt Roper wrote:
> > > On Mon, Jan 29, 2024 at 10:17:40AM -0800, José Roberto de Souza wrote:
> > > > Next patch will read register in specific DSS registers and this
> > > > are the functions missing to do so.
> > > > 
> > > > xe_gt_mcr_get_dss_steering() calculate and return the group and
> > > > instance that will be used by xe_gt_mcr_unicast_read().
> > > > 
> > > > xe_gt_has_geometry_dss() and xe_gt_has_compute_dss() returns true
> > > > if DSS is available for geometry of compute.
> > > > 
> > > > for_each_geometry/compute_dss() to simply the iteration over each
> > > > available DSS
> > > > 
> > > > v3:
> > > > - add for_each_geometry/compute_dss()
> > > > 
> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > Cc: Matt Roper <matthew.d.roper@intel.com>
> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > ---
> > > >   drivers/gpu/drm/xe/xe_gt.c          | 17 ++++++++++++++++
> > > >   drivers/gpu/drm/xe/xe_gt.h          |  3 +++
> > > >   drivers/gpu/drm/xe/xe_gt_mcr.c      | 17 +++++++++++++++-
> > > >   drivers/gpu/drm/xe/xe_gt_mcr.h      | 31 +++++++++++++++++++++++++++++
> > > >   drivers/gpu/drm/xe/xe_gt_topology.c |  1 -
> > > >   drivers/gpu/drm/xe/xe_gt_types.h    |  3 ++-
> > > >   6 files changed, 69 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/xe/xe_gt.c b/drivers/gpu/drm/xe/xe_gt.c
> > > > index 675a2927a19ef..9a3dce45b92ba 100644
> > > > --- a/drivers/gpu/drm/xe/xe_gt.c
> > > > +++ b/drivers/gpu/drm/xe/xe_gt.c
> > > > @@ -795,3 +795,20 @@ struct xe_hw_engine *xe_gt_any_hw_engine_by_reset_domain(struct xe_gt *gt,
> > > >   
> > > >   	return NULL;
> > > >   }
> > > > +
> > > > +static bool has_dss(xe_dss_mask_t dss_mask, unsigned int dss)
> > > > +{
> > > > +	unsigned long value = bitmap_get_value8(dss_mask, (dss / 8) * 8);
> > > > +
> > > > +	return value & BIT(dss % 8);
> > > > +}
> > > > +
> > > > +bool xe_gt_has_geometry_dss(struct xe_gt *gt, unsigned int dss)
> > > > +{
> > > > +	return has_dss(gt->fuse_topo.g_dss_mask, dss);
> > > > +}
> > > > +
> > > > +bool xe_gt_has_compute_dss(struct xe_gt *gt, unsigned int dss)
> > > > +{
> > > > +	return has_dss(gt->fuse_topo.c_dss_mask, dss);
> > > > +}
> > > 
> > > It feels like these belong better in xe_gt_topology.[ch].  Ideally we'd
> > > keep all the low-level representation of the masks abstracted away
> > > inside of those files and just provide a sane interface for the rest of
> > > the driver to query whatever it needs.
> > 
> > sure, can move it.
> > 
> > > 
> > > > diff --git a/drivers/gpu/drm/xe/xe_gt.h b/drivers/gpu/drm/xe/xe_gt.h
> > > > index c1675bd44cf6d..36815d8cbc107 100644
> > > > --- a/drivers/gpu/drm/xe/xe_gt.h
> > > > +++ b/drivers/gpu/drm/xe/xe_gt.h
> > > > @@ -70,4 +70,7 @@ static inline bool xe_gt_is_usm_hwe(struct xe_gt *gt, struct xe_hw_engine *hwe)
> > > >   		hwe->instance == gt->usm.reserved_bcs_instance;
> > > >   }
> > > >   
> > > > +bool xe_gt_has_geometry_dss(struct xe_gt *gt, unsigned int dss);
> > > > +bool xe_gt_has_compute_dss(struct xe_gt *gt, unsigned int dss);
> > > > +
> > > >   #endif
> > > > diff --git a/drivers/gpu/drm/xe/xe_gt_mcr.c b/drivers/gpu/drm/xe/xe_gt_mcr.c
> > > > index 77925b35cf8dc..e76cb0ae457aa 100644
> > > > --- a/drivers/gpu/drm/xe/xe_gt_mcr.c
> > > > +++ b/drivers/gpu/drm/xe/xe_gt_mcr.c
> > > > @@ -291,11 +291,17 @@ static void init_steering_mslice(struct xe_gt *gt)
> > > >   	gt->steering[LNCF].instance_target = 0;		/* unused */
> > > >   }
> > > >   
> > > > +static unsigned int
> > > > +get_dss_per_group(struct xe_gt *gt)
> > > > +{
> > > > +	return gt_to_xe(gt)->info.platform == XE_PVC ? 8 : 4;
> > > > +}
> > > > +
> > > >   static void init_steering_dss(struct xe_gt *gt)
> > > >   {
> > > >   	unsigned int dss = min(xe_dss_mask_group_ffs(gt->fuse_topo.g_dss_mask, 0, 0),
> > > >   			       xe_dss_mask_group_ffs(gt->fuse_topo.c_dss_mask, 0, 0));
> > > > -	unsigned int dss_per_grp = gt_to_xe(gt)->info.platform == XE_PVC ? 8 : 4;
> > > > +	unsigned int dss_per_grp = get_dss_per_group(gt);
> > > >   
> > > >   	gt->steering[DSS].group_target = dss / dss_per_grp;
> > > >   	gt->steering[DSS].instance_target = dss % dss_per_grp;
> > > > @@ -683,3 +689,12 @@ void xe_gt_mcr_steering_dump(struct xe_gt *gt, struct drm_printer *p)
> > > >   		}
> > > >   	}
> > > >   }
> > > > +
> > > > +void
> > > > +xe_gt_mcr_get_dss_steering(struct xe_gt *gt, unsigned int dss, int *group,
> > > > +			   int *instance)
> > > 
> > > I think you're primarily adding this for eventual use in the
> > > devcoredump, right?  But I think there's also been some other series
> > > trying to add some MCR registers of types other than DSS (e.g.,
> > > per-mslice registers and such).  Given that, maybe we should make this
> > > function more general where you pass the MCR class as a parameter too?
> > > E.g.,
> > 
> > Yes, need that to get instdone registers for the Mesa error dump parser tool.
> > 
> > Thanks for pointing that Zhanjun was also working on that.
> > Looking at Zhanjun patches looks like we had almost the same functions but he is using 'ss' instead of 'dss'.
> Yes, we are working on the same functions, sure I can use 'dss', no problem.

Please then lets go with your version.

Will rebase this series when you land your patches.

> > 
> > > 
> > >    void
> > >    xe_gt_mcr_get_steering(struct xe_gt *gt,
> > >                           enum xe_steering_type type,
> > >                           unsigned int id,
> > >                           unsigned int *group,
> > >                           unsigned int *instance)
> > > 
> > > 
> > > BTW, I think Zhanjun is working along similar lines to what you have
> > > here, so you guys might want to sync up?
> > 
> > I don't think the function above is needed, what do you think Zhanjun?
> 
> This function is to get group/instance steering from dss id, the only 
> place used is by for_each_xxx, the macro call this function, loop with 
> dss id, check against mask and call a code block, that's the typical 
> usage. For Xe, as we are using the g_dss_mask and c_dss_mask to test 
> bit, the converted group and instance is not used, so looks like 
> xe_gt_mcr_get_steering could be optimized out and the for_each macro 
> could be optimized to:
> 
> #define for_each_geometry_dss(gt, dss) \
> 	for (dss = 0; dss < XE_MAX_DSS_FUSE_BITS; dss++) \
> 		if (_HAS_DSS(gt, dss))
> 
> *_HAS_DSS means xe_gt_has_geometry_dss/xe_gt_has_compute_dss
> 
> The above is based on only this macro calls xe_gt_mcr_get_steering, not 
> sure if Matthew has some thought about the steering_type?
> 
> Regards,
> Zhanjun
> > 
> > > 
> > > 
> > > Matt
> > > 
> > > > +{
> > > > +	unsigned int dss_per_group = get_dss_per_group(gt);
> > > > +	*group = dss / dss_per_group;
> > > > +	*instance = dss % dss_per_group;
> > > > +}
> > > > diff --git a/drivers/gpu/drm/xe/xe_gt_mcr.h b/drivers/gpu/drm/xe/xe_gt_mcr.h
> > > > index 27ca1bc880a00..9f5f7dbb6fca8 100644
> > > > --- a/drivers/gpu/drm/xe/xe_gt_mcr.h
> > > > +++ b/drivers/gpu/drm/xe/xe_gt_mcr.h
> > > > @@ -7,6 +7,7 @@
> > > >   #define _XE_GT_MCR_H_
> > > >   
> > > >   #include "regs/xe_reg_defs.h"
> > > > +#include "xe_gt_types.h"
> > > >   
> > > >   struct drm_printer;
> > > >   struct xe_gt;
> > > > @@ -24,6 +25,36 @@ void xe_gt_mcr_unicast_write(struct xe_gt *gt, struct xe_reg_mcr mcr_reg,
> > > >   void xe_gt_mcr_multicast_write(struct xe_gt *gt, struct xe_reg_mcr mcr_reg,
> > > >   			       u32 value);
> > > >   
> > > > +void
> > > > +xe_gt_mcr_get_dss_steering(struct xe_gt *gt, unsigned int dss, int *group,
> > > > +			   int *instance);
> > > > +
> > > >   void xe_gt_mcr_steering_dump(struct xe_gt *gt, struct drm_printer *p);
> > > >   
> > > > +/**
> > > > + * for_each_geometry_dss - Iterate over each DSS available for geometry
> > > > + * @gt: GT structure
> > > > + * @dss: DSS id
> > > > + * @grp: group id to be in xe_gt_mcr_unicast_read()
> > > > + * @inst: instance id to be in xe_gt_mcr_unicast_read()
> > > > + */
> > > > +#define for_each_geometry_dss(gt, dss, grp, inst) \
> > > > +	for (dss = 0, xe_gt_mcr_get_dss_steering(gt, dss, &grp, &inst); \
> > > > +	     dss < XE_MAX_DSS_FUSE_BITS; \
> > > > +	     dss++, xe_gt_mcr_get_dss_steering(gt, dss, &grp, &inst)) \
> > > > +		if (xe_gt_has_geometry_dss(gt, dss))
> > > > +
> > > > +/**
> > > > + * for_each_compute_dss - Iterate over each DSS available for compute
> > > > + * @gt: GT structure
> > > > + * @dss: DSS id
> > > > + * @grp: group id to be in xe_gt_mcr_unicast_read()
> > > > + * @inst: instance id to be in xe_gt_mcr_unicast_read()
> > > > + */
> > > > +#define for_each_compute_dss(gt, dss, grp, inst) \
> > > > +	for (dss = 0, xe_gt_mcr_get_dss_steering(gt, dss, &grp, &inst); \
> > > > +	     dss < XE_MAX_DSS_FUSE_BITS; \
> > > > +	     dss++, xe_gt_mcr_get_dss_steering(gt, dss, &grp, &inst)) \
> > > > +		if (xe_gt_has_compute_dss(gt, dss))
> > > > +
> > > >   #endif /* _XE_GT_MCR_H_ */
> > > > diff --git a/drivers/gpu/drm/xe/xe_gt_topology.c b/drivers/gpu/drm/xe/xe_gt_topology.c
> > > > index a8d7f272c30a0..c4942f2b37751 100644
> > > > --- a/drivers/gpu/drm/xe/xe_gt_topology.c
> > > > +++ b/drivers/gpu/drm/xe/xe_gt_topology.c
> > > > @@ -11,7 +11,6 @@
> > > >   #include "xe_gt.h"
> > > >   #include "xe_mmio.h"
> > > >   
> > > > -#define XE_MAX_DSS_FUSE_BITS (32 * XE_MAX_DSS_FUSE_REGS)
> > > >   #define XE_MAX_EU_FUSE_BITS (32 * XE_MAX_EU_FUSE_REGS)
> > > >   
> > > >   static void
> > > > diff --git a/drivers/gpu/drm/xe/xe_gt_types.h b/drivers/gpu/drm/xe/xe_gt_types.h
> > > > index 70c615dd14986..bb6dc1fcaa7dd 100644
> > > > --- a/drivers/gpu/drm/xe/xe_gt_types.h
> > > > +++ b/drivers/gpu/drm/xe/xe_gt_types.h
> > > > @@ -25,9 +25,10 @@ enum xe_gt_type {
> > > >   };
> > > >   
> > > >   #define XE_MAX_DSS_FUSE_REGS	3
> > > > +#define XE_MAX_DSS_FUSE_BITS   (32 * XE_MAX_DSS_FUSE_REGS)
> > > >   #define XE_MAX_EU_FUSE_REGS	1
> > > >   
> > > > -typedef unsigned long xe_dss_mask_t[BITS_TO_LONGS(32 * XE_MAX_DSS_FUSE_REGS)];
> > > > +typedef unsigned long xe_dss_mask_t[BITS_TO_LONGS(XE_MAX_DSS_FUSE_BITS)];
> > > >   typedef unsigned long xe_eu_mask_t[BITS_TO_LONGS(32 * XE_MAX_EU_FUSE_REGS)];
> > > >   
> > > >   struct xe_mmio_range {
> > > > -- 
> > > > 2.43.0
> > > > 
> > > 
> >