Re: [PATCH v4 1/1] drm/xe: Expose number of dss per group and helpers

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

On 2024-01-30 2:28 p.m., Summers, Stuart wrote:
> On Tue, 2024-01-30 at 10:57 -0800, Zhanjun Dong wrote:
>> Expose helper for dss per group. This is a precursor patch to allow
>> 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      | 17 ++++++++++++
>>   drivers/gpu/drm/xe/xe_gt_topology.c |  1 -
>>   drivers/gpu/drm/xe/xe_gt_types.h    |  1 +
>>   4 files changed, 57 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_gt_mcr.c
>> b/drivers/gpu/drm/xe/xe_gt_mcr.c
>> index 77925b35cf8d..7cd93bcb811f 100644
>> --- a/drivers/gpu/drm/xe/xe_gt_mcr.c
>> +++ b/drivers/gpu/drm/xe/xe_gt_mcr.c
>> @@ -291,11 +291,16 @@ static void init_steering_mslice(struct xe_gt
>> *gt)
>>          gt->steering[LNCF].instance_target = 0;         /* unused */
>>   }
>>   
>> +int xe_gt_mcr_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 = xe_gt_mcr_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 +688,36 @@ void xe_gt_mcr_steering_dump(struct xe_gt *gt,
>> struct drm_printer *p)
>>                  }
>>          }
>>   }
>> +
>> +/**
>> + * xe_gt_mcr_get_dss_steering - returns the group/instance steering
>> for a SS
>> + * @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
>> + *
>> + * Returns the steering IDs (via the @group and @instance
>> parameters) that
>> + * correspond to a specific subslice/DSS ID.
>> + */
>> +void xe_gt_mcr_get_dss_steering(struct xe_gt *gt, unsigned int dss,
>> unsigned int *group,
>> +                               unsigned int *instance)
>> +{
>> +       int dss_per_grp = xe_gt_mcr_get_dss_per_group(gt);
>> +
>> +       *group = dss / dss_per_grp;
>> +       *instance = dss % dss_per_grp;
>> +}
>> +
>> +bool xe_gt_mcr_dss_has_subslice(struct xe_gt *gt, int slice, int
>> subslice)
>> +{
>> +       int dss_per_grp = xe_gt_mcr_get_dss_per_group(gt);
>> +       int index = slice * dss_per_grp + subslice;
>> +
>> +       if (index >= XE_MAX_DSS_FUSE_BITS) {
>> +               xe_gt_dbg(gt, "DSS id out of range: slice:%d
>> subslice:%d\n", slice, subslice);
>> +               return false;
>> +       }
>> +
>> +       return test_bit(index, gt->fuse_topo.g_dss_mask) ||
>> +              test_bit(index, gt->fuse_topo.c_dss_mask);
>> +}
>> diff --git a/drivers/gpu/drm/xe/xe_gt_mcr.h
>> b/drivers/gpu/drm/xe/xe_gt_mcr.h
>> index 27ca1bc880a0..7d1eb180befd 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;
>> @@ -25,5 +26,21 @@ 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);
>> +int xe_gt_mcr_get_dss_per_group(struct xe_gt *gt);
>> +void xe_gt_mcr_get_dss_steering(struct xe_gt *gt, unsigned int dss,
>> unsigned int *group,
>> +                               unsigned int *instance);
>> +bool xe_gt_mcr_dss_has_subslice(struct xe_gt *gt, int slice, int
>> subslice);
>> +
>> +#define _HAS_SS(gt_, group_, instance_)
> 
> Maybe gt__, group__, instance__
Thanks, that looks better
> 
>> xe_gt_mcr_dss_has_subslice(gt_, group_, instance_)
>> +
>> +/*
>> + * Loop over each subslice/DSS and determine the group and instance
>> IDs that
>> + * should be used to steer MCR accesses toward this DSS.
>> + */
>> +#define for_each_dss_steering(dss_, gt_, group_, instance_) \
>> +       for (dss_ = 0, xe_gt_mcr_get_dss_steering(gt_, 0, &group_,
>> &instance_); \
>> +            dss_ < XE_MAX_DSS_FUSE_BITS; \
>> +            dss_++, xe_gt_mcr_get_dss_steering(gt_, dss_, &group_,
>> &instance_)) \
> 
> I think you're still going to get those checkpatch warnings here.
> 
> Are you planning on addressing that?
The checkpatch complains 6 warnings in about 2 kind of things:
#1. "CHECK:MACRO_ARG_REUSE: Macro argument reuse 'gt_' - possible 
side-effects?"
The warning applies to all 4 aruguments, as they all being reused in 
loop init and loop iteration, this is by purpose and I will accept it.

#2. CHECK:MACRO_ARG_PRECEDENCE: Macro argument 'group_' may be better as 
'(group_)' to avoid precedence issues
Applied to  group_ and instance_

Macro body has unary operator & to get address, it could be fixed by:

#define for_each_dss_steering(dss_, gt_, group_, instance_) \
	for (dss_ = 0, xe_gt_mcr_get_dss_steering(gt_, 0, &(group_), 
&(instance_)); \
	     dss_ < XE_MAX_DSS_FUSE_BITS; \
	     dss_++, xe_gt_mcr_get_dss_steering(gt_, dss_, &(group_), 
&(instance_))) \
		for_each_if(_HAS_SS(gt_, (group_), (instance_)))

I will leaves type #1 but fix type #2, total checkpatch warnings will 
reduced from 6 to 4.

> 
>> +               for_each_if(_HAS_SS(gt_, group_, instance_))
>>   
>>   #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..c4942f2b3775 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)
> 
> Honestly I do think these should be in the same file at least. Can you
> move the EU FUSE BITS to xe_gt_types.h too?
> 
> Thanks,
> Stuart
Sure I will move it together.

Regards,
Zhanjun
> 
>>   
>>   static void
>> diff --git a/drivers/gpu/drm/xe/xe_gt_types.h
>> b/drivers/gpu/drm/xe/xe_gt_types.h
>> index 70c615dd1498..b4df7d35dec7 100644
>> --- a/drivers/gpu/drm/xe/xe_gt_types.h
>> +++ b/drivers/gpu/drm/xe/xe_gt_types.h
>> @@ -25,6 +25,7 @@ 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)];
>