[PATCH v9 1/1] drm/xe: Add helper macro to loop each dss
Michal Wajdeczko
michal.wajdeczko at intel.com
Thu Feb 15 22:20:37 UTC 2024
On 15.02.2024 22:33, Dong, Zhanjun wrote:
> See my inline comments below.
>
> Regards,
> Zhanjun
>
> On 2024-02-15 2:20 p.m., Michal Wajdeczko wrote:
>> Hi Zhanjun,
>>
>> I was about to push this patch, but it needs new revision, see below.
>> Also added some nits
>>
>> On 09.02.2024 18:45, Zhanjun Dong wrote:
>>> Add helper macro to loop each dss. This is a precursor patch to allow
>>> for easier iteration through MCR registers and other per-DSS uses.
>>
>> nit: please be consistent on naming dss vs DSS
>>
>>>
>>> Signed-off-by: Zhanjun Dong <zhanjun.dong at intel.com>
>>> ---
>>> drivers/gpu/drm/xe/xe_gt_mcr.c | 32 ++++++++++++++++++++++++++++-
>>> drivers/gpu/drm/xe/xe_gt_mcr.h | 13 ++++++++++++
>>> 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 | 2 ++
>>> 5 files changed, 75 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/xe/xe_gt_mcr.c
>>> b/drivers/gpu/drm/xe/xe_gt_mcr.c
>>> index 8546cd3cc50d..44163fe8c7b9 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"
>>> @@ -291,11 +292,40 @@ static void init_steering_mslice(struct xe_gt *gt)
>>> gt->steering[LNCF].instance_target = 0; /* unused */
>>> }
>>> +static int get_dss_per_group(struct xe_gt *gt)
>>
>> nit: shouldn't this function be 'unsigned int' ?
> Sure, we can
>
>>
>>> +{
>>> + return gt_to_xe(gt)->info.platform == XE_PVC ? 8 : 4;
>>> +}
>>> +
>>> +/**
>>> + * xe_gt_mcr_get_dss_steering - returns 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
>>> + *
>>> + * Returns the steering IDs (via the @group and @instance
>>> parameters) that
>>> + * correspond to a specific DSS ID.
>>
>> nit: this is a 'bool' function so having a "Return:" section is desired
> Sure.
>>
>>> + */
>>> +bool xe_gt_mcr_get_dss_steering(struct xe_gt *gt, unsigned int dss,
>>> unsigned int *group,
>>> + unsigned int *instance)
>>> +{
>>> + int dss_per_grp;
>>
>> nit: this could be one-liner:
> Sure.
>>
>> unsigned int dss_per_grp = get_dss_per_group(gt);
>>
>>> +
>>> + xe_gt_assert(gt, dss < XE_MAX_DSS_FUSE_BITS);
>>> +
>>> + dss_per_grp = get_dss_per_group(gt);
>>> +
>>> + *group = dss / dss_per_grp;
>>> + *instance = dss % dss_per_grp;
>>> + return true;
>>> +}
>>> +
>>> 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;
>>
>> nit: this looks like a good candidate to use this new function
>> xe_gt_mcr_get_dss_steering() to avoid code duplication
> No, the pointer type is (unsigned int *) for group and instance of
> xe_gt_mcr_get_dss_steering, while here the steering[DSS].group and
> instance is u16. The pointed data type uint is to match data type being
> used in loop body, where we don't use u16 normally.
>
> I know it looks similar, but type mismatch here.
>
hmm, but if our code is already using u16 for group/instance, which
means that u16 is sufficient, then maybe helper functions should also
use that type ? then no mismatch and opportunity for code reuse
>>
>>
>>> diff --git a/drivers/gpu/drm/xe/xe_gt_mcr.h
>>> b/drivers/gpu/drm/xe/xe_gt_mcr.h
>>> index 27ca1bc880a0..5b4e74da82a1 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,17 @@ 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,
>>> unsigned int *group,
>>> + unsigned int *instance);
>>> +
>>> +/*
>>> + * Loop over each DSS and determine the group and instance IDs that
>>> + * should be used to steer MCR accesses toward this DSS.
>>> + */
>>
>> shouldn't this be regular kernel-doc >
>>> +#define for_each_dss_steering(dss_, gt_, group_, instance_) \
>>> + for (dss_ = xe_gt_topology_get_next_dss(gt, 0); \
>>
>> BUG: using outside "gt" instead of "gt_" parameter
>>
>> also it looks that upstream BKM is to _not_ use underscore for param
>> names as params shouldn't leak anyway)
> Thanks for point out the bug. Will fix it in next rev.
>
>>
>>
>>> + dss_ >= 0; \
>>> + dss_ = xe_gt_topology_get_next_dss(gt, dss_ + 1)) \
>>
>> ditto
>>
>>> + for_each_if(xe_gt_mcr_get_dss_steering(gt_, dss_, &(group_),
>>> &(instance_)))
>>
>> btw, did you consider to split this macro into two:
>>
>> xe_gt_topology.h:
>>
>> #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))
>>
>> xe_gt_mcr.h:
>>
>> #define for_each_dss_steering(dss, gt, group, instance) \
>> for_each_if(xe_gt_mcr_get_dss_steering(gt, dss, &(group),
>> &(instance)))
>>
> We can split it if Jose need it.
>
>>> #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..97c0a4b8fe40 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, ...)
>>> {
>>> @@ -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 form
>>
>> typo s/form/from
> Thanks
>>
>>> + *
>>> + * 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 -1 if not found since given "from" position.
>>
>> you should use "Return:" tag and likely say what is returned if found
> Sure, will fix it.
>
>>
>>> + */
>>> +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;
>>> +}
>>> 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..b926606edb38 100644
>>> --- a/drivers/gpu/drm/xe/xe_gt_types.h
>>> +++ b/drivers/gpu/drm/xe/xe_gt_types.h
>>> @@ -25,7 +25,9 @@ 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)];
>>
>> nit: above looks like a good candidates to use new FUSE_BITS definitions
> Sure, will do it that way.
>>
>> Michal
>>
More information about the Intel-xe
mailing list