Re: [PATCH v3 2/9] drm/xe/guc: Expose dss per group for GuC error capture

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

Please see my comments below.

Regards,
Zhanjun

On 2024-01-22 4:57 p.m., Matt Roper wrote:
>> @@ -7,10 +7,13 @@
>>   #define _XE_GT_MCR_H_
>>   
>>   #include "regs/xe_reg_defs.h"
>> +#include "xe_gt_types.h"
>>   
>>   struct drm_printer;
>>   struct xe_gt;
>>   
>> +#define XE_GT_MCR_DSS_PER_GROUP(gt) (gt_to_xe(gt)->info.platform == XE_PVC ? 8 : 4)
> 
> I'd just go ahead and move this to a real function rather than using a
> macro.  We expect the hardware to expose the proper value here via a
> fuse register at some point in the future (meaning we'll be able to stop
> hardcoding magic numbers in the driver), and once that happens we'll
> want to have a function anyway.  That will also allow us to avoid the
> extra header include.
> 
Yes, will do that in next rev.
> 
>> +
>>   void xe_gt_mcr_init(struct xe_gt *gt);
>>   
>>   void xe_gt_mcr_set_implicit_defaults(struct xe_gt *gt);
>> diff --git a/drivers/gpu/drm/xe/xe_gt_topology.c b/drivers/gpu/drm/xe/xe_gt_topology.c
>> index a8d7f272c30a..e973eeaac7f1 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)
>> -
>>   static void
>>   load_dss_mask(struct xe_gt *gt, xe_dss_mask_t mask, int numregs, ...)
>>   {
>> diff --git a/drivers/gpu/drm/xe/xe_guc_capture.c b/drivers/gpu/drm/xe/xe_guc_capture.c
>> index cacd50f4718a..2e1ed5bbdd56 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_capture.c
>> +++ b/drivers/gpu/drm/xe/xe_guc_capture.c
>> @@ -93,6 +93,35 @@
>>   	{ SFC_DONE(2),		    0,      0, "SFC_DONE[2]" }, \
>>   	{ SFC_DONE(3),		    0,      0, "SFC_DONE[3]" }
>>   
>> +static void xe_gt_mcr_get_ss_steering(struct xe_gt *gt, unsigned int dss,
>> +				      unsigned int *group, unsigned int *instance)
>> +{
>> +	int dss_per_grp = XE_GT_MCR_DSS_PER_GROUP(gt);
>> +
>> +	*group = dss / dss_per_grp;
>> +	*instance = dss % dss_per_grp;
>> +}
>> +
>> +static bool xe_sseu_has_subslice(struct xe_gt *gt, int slice, int subslice)
>> +{
>> +	int dss_per_grp = XE_GT_MCR_DSS_PER_GROUP(gt);
>> +	int index = slice * dss_per_grp + subslice;
>> +
>> +	return index >= XE_MAX_DSS_FUSE_BITS ? false : test_bit(index, gt->fuse_topo.g_dss_mask);
>> +}
> 
> It would be better if we could avoid leaking the internal implementation
> of the topology outside of xe_gt_topology.[ch]; if we ever need to
> change it in the future it's easier when it's self-contained inside a
> single file with well-defined interfaces to the rest of the driver.  So
> it might be better to have
> 
>      bool xe_gt_topology_has_dss(struct xe_gt *gt, int dss_num);
> 
> to wrap the test_bit part, and
> 
>      int xe_gt_mcr_get_steering(struct xe_gt* gt, enum xe_steering_type type,
>                                 int instance, int *grp, int *inst);
> 
> for looking up the steering targets of a specific instance of an MCR
> register.  Then you can use those two helper functions in the GuC
> capture code without exposing the internal interface details (and they
> can be re-used in other parts of the code as well).
> 
> 
> BTW, why does your function here only check the g_dss_mask?  On a
> platform like PVC that won't match any DSS (since the geometry mask is
> empty and only the compute mask is set).
> 
> 
> Matt

Good catch!
I will rewrite this part, do some test and get back to you later.

> 
>> +
>> +#define _HAS_SS(ss_, gt_, group_, instance_) xe_sseu_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_ss_steering(ss_, gt_, group_, instance_) \
>> +	for (ss_ = 0, xe_gt_mcr_get_ss_steering(gt_, 0, &group_, &instance_); \
>> +	     ss_ < XE_MAX_DSS_FUSE_BITS; \
>> +	     ss_++, xe_gt_mcr_get_ss_steering(gt_, ss_, &group_, &instance_)) \
>> +		for_each_if(_HAS_SS(ss_, gt_, group_, instance_))
>> +
>>   int xe_guc_capture_init(struct xe_guc *guc)
>>   {
>>   	return 0;
>> diff --git a/drivers/gpu/drm/xe/xe_hw_engine_types.h b/drivers/gpu/drm/xe/xe_hw_engine_types.h
>> index dfeaaac08b7f..c258228b244f 100644
>> --- a/drivers/gpu/drm/xe/xe_hw_engine_types.h
>> +++ b/drivers/gpu/drm/xe/xe_hw_engine_types.h
>> @@ -65,6 +65,9 @@ struct xe_bo;
>>   struct xe_execlist_port;
>>   struct xe_gt;
>>   
>> +#define XE_MAX_DSS_FUSE_BITS (32 * XE_MAX_DSS_FUSE_REGS)
>> +#define XE_MAX_EU_FUSE_BITS (32 * XE_MAX_EU_FUSE_REGS)
>> +
>>   /**
>>    * struct xe_hw_engine_class_intf - per hw engine class struct interface
>>    *
>> -- 
>> 2.34.1
>>
>