[PATCH v2 1/1] drm/xe/guc: Expose number of dss per group and helpers
Dong, Zhanjun
zhanjun.dong at intel.com
Mon Jan 29 21:03:04 UTC 2024
Thanks for take time review it. Please see my comments below.
Regards,
Zhanjun
On 2024-01-29 3:17 p.m., Summers, Stuart wrote:
> On Fri, 2024-01-26 at 15:23 -0800, Zhanjun Dong wrote:
>> Expose helper for dss per group of mcr, features like GuC register
>> capture will need this info to prepare buffer required.
>
> I don't understand what this specifically has to do with GuC. Can we
> just list this as general functionality, not related to GuC (cover
> letter) or MCR (function names, see below)?
Yes, will move it from drm/xe/guc to drm/xe
>
>>
>> Signed-off-by: Zhanjun Dong <zhanjun.dong at intel.com>
>> ---
>> drivers/gpu/drm/xe/xe_gt_mcr.c | 38
>> ++++++++++++++++++++++++-
>> drivers/gpu/drm/xe/xe_gt_mcr.h | 17 +++++++++++
>> drivers/gpu/drm/xe/xe_gt_topology.c | 3 --
>> drivers/gpu/drm/xe/xe_hw_engine_types.h | 3 ++
>> 4 files changed, 57 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_gt_mcr.c
>> b/drivers/gpu/drm/xe/xe_gt_mcr.c
>> index 77925b35cf8d..f7002623f3b5 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)
>
> I don't understand why we're tying this name to MCR. DSS are involved
> in more than just MCR negotiation/configuration.
>
> I guess not a blocker, but I'd rather this just be something like:
> xe_gt_get_dss_per_group()
Sure, will be changed to this name
>
>> +{
>> + 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,34 @@ void xe_gt_mcr_steering_dump(struct xe_gt *gt,
>> struct drm_printer *p)
>> }
>> }
>> }
>> +
>> +/**
>> + * xe_gt_mcr_get_ss_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_ss_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)
>
> Maybe interesting to print something here from a debugging perspective,
> especially on pre-silicon where maybe the fuse registers aren't set up
> correctly. Otherwise it's a bit of a goose chase trying to track down
> why the DSS count is 0, etc.
Sure, will add debug print for this out of range case.
>
>> + return false;
>> + else
>> + 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..1365b3e77226 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_ss_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(ss_, gt_, group_, instance_)
>
> Why do we have the ss_ parameter?
Oops, not used arg
>
>> xe_gt_mcr_dss_has_subslice(gt_, group_, instance_)
>
> Same comment as above, why is this tied naming-wise to "mcr"?
>
>> +
>> +/*
>> + * 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_); \
>
> IMO it's confusing calling get_ss_steering twice here. I understand the
> macro piece, but it would be nice if we could reduce this to one call.
The 1st call is the one time execution before code block running;
the 2nd xe_gt_mcr_get_ss_steering call will be run on every iteration,
so this is by purpose and cann't be reduced.
>
>> + 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_))
>>
>> #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..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_hw_engine_types.h
>> b/drivers/gpu/drm/xe/xe_hw_engine_types.h
>> index d7f828c76cc5..3e978a190041 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)
>
> Sorry maybe obvious, but why did you make this move?
Before we expose the dss per group, the XE_MAX_DSS_FUSE_BITS only being
referenced within xe_gt_topology.c. After this patch, these 2 macros
will be used by for_each_ss_steering macro and thus being refrenced.
Move it from the .c to xe_hw_engine_types.h to provide this definition.
>
> Thanks,
> Stuart
>
>> +
>> /**
>> * struct xe_hw_engine_class_intf - per hw engine class struct
>> interface
>> *
>
More information about the Intel-xe
mailing list