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

["Dong, Zhanjun" <zhanjun.dong@intel.com>]

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.
> 
>>
>>    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
>>>
>>
>