Re: [PATCH v10 1/1] drm/xe: Add helper macro to loop each dss

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

Please see my comments inline below.

Regards,
Zhanjun


On 2024-03-12 5:49 p.m., Michal Wajdeczko wrote:
> 
> 
> On 20.02.2024 17:19, Zhanjun Dong wrote:
>> Add helper macro to loop each dss. This is a precursor patch to allow
> 
> please be consistent in naming: s/dss/DSS
Sure
> 
>> for easier iteration through MCR registers and other per-DSS uses.
>>
>> Signed-off-by: Zhanjun Dong <zhanjun.dong@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_gt_mcr.c      | 40 ++++++++++++++++++++++++-----
>>   drivers/gpu/drm/xe/xe_gt_mcr.h      | 16 ++++++++++++
>>   drivers/gpu/drm/xe/xe_gt_topology.c | 31 +++++++++++++++++++---
>>   drivers/gpu/drm/xe/xe_gt_topology.h |  1 +
>>   drivers/gpu/drm/xe/xe_gt_types.h    |  6 +++--
>>   5 files changed, 83 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_gt_mcr.c b/drivers/gpu/drm/xe/xe_gt_mcr.c
>> index a7ab9ba645f9..2ee727ebc4a2 100644
>> --- a/drivers/gpu/drm/xe/xe_gt_mcr.c
>> +++ b/drivers/gpu/drm/xe/xe_gt_mcr.c
>> @@ -6,6 +6,7 @@
>>   #include "xe_gt_mcr.h"
>>   
>>   #include "regs/xe_gt_regs.h"
>> +#include "xe_assert.h"
>>   #include "xe_gt.h"
>>   #include "xe_gt_topology.h"
>>   #include "xe_gt_types.h"
>> @@ -294,14 +295,41 @@ static void init_steering_mslice(struct xe_gt *gt)
>>   	gt->steering[LNCF].instance_target = 0;		/* unused */
>>   }
>>   
>> -static void init_steering_dss(struct xe_gt *gt)
>> +static unsigned int get_dss_per_group(struct xe_gt *gt)
> 
> nit: as there is no "set" then maybe s/get_dss_per_group/dss_per_group ?
Will be renamed.
> 
>> +{
>> +	return gt_to_xe(gt)->info.platform == XE_PVC ? 8 : 4;
>> +}
>> +
>> +/**
>> + * xe_gt_mcr_get_dss_steering - Get the group/instance steering for a DSS
>> + * @gt: GT structure
>> + * @dss: DSS ID to obtain steering for
>> + * @group: pointer to storage for steering group ID
>> + * @instance: pointer to storage for steering instance ID
>> + *
>> + * Return: Always return true. The steering IDs (via the @group and @instance parameters) that
>> + * correspond to a specific DSS ID is saved to *group and *instance
> 
> if return is always true then maybe function should be void and just use
> comma operator if you need bool somewhere in the macro

Sure

> 
>> + */
>> +bool xe_gt_mcr_get_dss_steering(struct xe_gt *gt, unsigned int dss, u16 *group, u16 *instance)
>>   {
>> -	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;
>> +	int dss_per_grp = get_dss_per_group(gt);
>> +
>> +	xe_gt_assert(gt, dss < XE_MAX_DSS_FUSE_BITS);
>> +	xe_gt_assert(gt, group);
>> +	xe_gt_assert(gt, instance);
> 
> as general rule we shouldn't assert NULL pointers as any NPD will end
> with nice report anyway
Sure, will take it off.
> 
>>   
>> -	gt->steering[DSS].group_target = dss / dss_per_grp;
>> -	gt->steering[DSS].instance_target = dss % dss_per_grp;
>> +	*group = dss / dss_per_grp;
>> +	*instance = dss % dss_per_grp;
>> +	return true;
>> +}
>> +
>> +static void init_steering_dss(struct xe_gt *gt)
>> +{
>> +	xe_gt_mcr_get_dss_steering(gt,
>> +				   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)),
>> +				   &gt->steering[DSS].group_target,
>> +				   &gt->steering[DSS].instance_target);
>>   }
>>   
>>   static void init_steering_oaddrm(struct xe_gt *gt)
>> diff --git a/drivers/gpu/drm/xe/xe_gt_mcr.h b/drivers/gpu/drm/xe/xe_gt_mcr.h
>> index 27ca1bc880a0..0d8abd320e08 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_topology.h"
>>   
>>   struct drm_printer;
>>   struct xe_gt;
>> @@ -25,5 +26,20 @@ void xe_gt_mcr_multicast_write(struct xe_gt *gt, struct xe_reg_mcr mcr_reg,
>>   			       u32 value);
>>   
>>   void xe_gt_mcr_steering_dump(struct xe_gt *gt, struct drm_printer *p);
>> +bool xe_gt_mcr_get_dss_steering(struct xe_gt *gt, unsigned int dss, u16 *group, u16 *instance);
>> +
>> +/*
>> + * Loop over each DSS and determine the group and instance IDs that
>> + * should be used to steer MCR accesses toward this DSS.
>> + * @dss_: DSS ID to obtain steering for
>> + * @gt_: GT structure
>> + * @group_: steering group ID, data type: u16
>> + * @instance_: steering instance ID, data type: u16
>> + */
> 
> 1) no need to use underscore in param names
> 2) remember to wrap params into ()

Sure
> 
>> +#define for_each_dss_steering(dss_, gt_, group_, instance_) \
>> +	for (dss_ = xe_gt_topology_get_next_dss(gt_, 0); \
>> +	     dss_ >= 0; \
>> +	     dss_ = xe_gt_topology_get_next_dss(gt_, dss_ + 1)) \
>> +		for_each_if(xe_gt_mcr_get_dss_steering(gt_, dss_, &(group_), &(instance_)))
> 
> I was hoping that there could be for_each_dss defined in gt_topology.h
> something like this:
> 
> #define for_each_dss(dss, gt) 					\
> 	for ((dss) = xe_gt_topology_get_next_dss((gt), 0); 	\
> 	     (dss) >= 0;					\
> 	     (dss) = xe_gt_topology_get_next_dss((gt), (dss) + 1))
> 
> followed by:
> 
> #define for_each_dss_steering(dss, gt, group, instance) \
>    for_each_dss((dss), (gt))
>      for_each_if(get_dss_steering((gt), (dss), &(group), &(instance)))
> 

Sounds good, for the for_each_dss macro, I like the version defined below.

>>   
>>   #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 a8d7f272c30a..d9566867c7ef 100644
>> --- a/drivers/gpu/drm/xe/xe_gt_topology.c
>> +++ b/drivers/gpu/drm/xe/xe_gt_topology.c
>> @@ -11,9 +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)
> 
> nit: this definition move could be a small separate patch

This macro referenced from other 3 files, can we keep this small changes 
within?

> 
>> -
>>   static void
>>   load_dss_mask(struct xe_gt *gt, xe_dss_mask_t mask, int numregs, ...)
>>   {
>> @@ -167,3 +164,31 @@ bool xe_gt_topology_has_dss_in_quadrant(struct xe_gt *gt, int quad)
>>   
>>   	return quad_first < (quad + 1) * dss_per_quad;
>>   }
>> +
>> +/**
>> + * xe_gt_topology_get_next_dss - returns the next DSS id from a start position
>> + * @gt: GT structure
>> + * @from: An index to start search from
>> + *
>> + * Search from this GT's DSS-fuses-topology and return the DSS-ID of the
>> + * next valid DSS instance after DSS-ID 'from'. Depending on the platform
>> + * construction, DSS instances could be for geometry (RENDER engine) or
>> + * compute (COMPUTE engine). However, all current platforms that have
>> + * both engines have the same instances thus we can combine both masks
>> + * internally before performing this operation.
>> + *
>> + * Return: The next DSS id from a start position if found, otherwise -1.
>> + */
>> +int xe_gt_topology_get_next_dss(struct xe_gt *gt, int from)
>> +{
>> +	xe_dss_mask_t all_dss;
>> +	unsigned long next;
>> +
>> +	bitmap_or(all_dss, gt->fuse_topo.g_dss_mask, gt->fuse_topo.c_dss_mask,
>> +		  XE_MAX_DSS_FUSE_BITS);
>> +
>> +	next = find_next_bit(all_dss, XE_MAX_DSS_FUSE_BITS, from);
>> +	if (next == XE_MAX_DSS_FUSE_BITS)
>> +		return -1;
>> +	return next;
>> +}
> 
> maybe there is no need for this function at all as likely below macro
> could do the trick:
> 
> #define for_each_dss(dss, gt) 					\
> 	for_each_or_bit((dss),					\
> 			(gt)->fuse_topo.g_dss_mask,  		\
> 			(gt)->fuse_topo.c_dss_mask,		\
> 			XE_MAX_DSS_FUSE_BITS)
> 

I like this idea.
Yeah! this function could be optimized out.


>> diff --git a/drivers/gpu/drm/xe/xe_gt_topology.h b/drivers/gpu/drm/xe/xe_gt_topology.h
>> index d1b54fb52ea6..44bd8a58f9ce 100644
>> --- a/drivers/gpu/drm/xe/xe_gt_topology.h
>> +++ b/drivers/gpu/drm/xe/xe_gt_topology.h
>> @@ -21,5 +21,6 @@ bool xe_dss_mask_empty(const xe_dss_mask_t mask);
>>   
>>   bool
>>   xe_gt_topology_has_dss_in_quadrant(struct xe_gt *gt, int quad);
>> +int xe_gt_topology_get_next_dss(struct xe_gt *gt, int from);
>>   
>>   #endif /* _XE_GT_TOPOLOGY_H_ */
>> diff --git a/drivers/gpu/drm/xe/xe_gt_types.h b/drivers/gpu/drm/xe/xe_gt_types.h
>> index 70c615dd1498..f6da2ad9719f 100644
>> --- a/drivers/gpu/drm/xe/xe_gt_types.h
>> +++ b/drivers/gpu/drm/xe/xe_gt_types.h
>> @@ -25,10 +25,12 @@ 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
>> +#define XE_MAX_EU_FUSE_BITS	(32 * XE_MAX_EU_FUSE_REGS)
>>   
>> -typedef unsigned long xe_dss_mask_t[BITS_TO_LONGS(32 * XE_MAX_DSS_FUSE_REGS)];
>> -typedef unsigned long xe_eu_mask_t[BITS_TO_LONGS(32 * XE_MAX_EU_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(XE_MAX_EU_FUSE_BITS)];
>>   
>>   struct xe_mmio_range {
>>   	u32 start;