[PATCH v3 4/6] drm/xe: Add misc functions to support read of specific DSS registers
Dong, Zhanjun
zhanjun.dong at intel.com
Tue Jan 30 16:52:23 UTC 2024
On 2024-01-30 11:14 a.m., Souza, Jose wrote:
> On Tue, 2024-01-30 at 10:54 -0500, Dong, Zhanjun wrote:
>>
>> On 2024-01-30 9:42 a.m., Souza, Jose wrote:
>>> On Mon, 2024-01-29 at 13:24 -0800, Matt Roper wrote:
>>>> On Mon, Jan 29, 2024 at 10:17:40AM -0800, José Roberto de Souza wrote:
>>>>> Next patch will read register in specific DSS registers and this
>>>>> are the functions missing to do so.
>>>>>
>>>>> xe_gt_mcr_get_dss_steering() calculate and return the group and
>>>>> instance that will be used by xe_gt_mcr_unicast_read().
>>>>>
>>>>> xe_gt_has_geometry_dss() and xe_gt_has_compute_dss() returns true
>>>>> if DSS is available for geometry of compute.
>>>>>
>>>>> for_each_geometry/compute_dss() to simply the iteration over each
>>>>> available DSS
>>>>>
>>>>> v3:
>>>>> - add for_each_geometry/compute_dss()
>>>>>
>>>>> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
>>>>> Cc: Matt Roper <matthew.d.roper at intel.com>
>>>>> Signed-off-by: José Roberto de Souza <jose.souza at intel.com>
>>>>> ---
>>>>> drivers/gpu/drm/xe/xe_gt.c | 17 ++++++++++++++++
>>>>> drivers/gpu/drm/xe/xe_gt.h | 3 +++
>>>>> drivers/gpu/drm/xe/xe_gt_mcr.c | 17 +++++++++++++++-
>>>>> drivers/gpu/drm/xe/xe_gt_mcr.h | 31 +++++++++++++++++++++++++++++
>>>>> drivers/gpu/drm/xe/xe_gt_topology.c | 1 -
>>>>> drivers/gpu/drm/xe/xe_gt_types.h | 3 ++-
>>>>> 6 files changed, 69 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/xe/xe_gt.c b/drivers/gpu/drm/xe/xe_gt.c
>>>>> index 675a2927a19ef..9a3dce45b92ba 100644
>>>>> --- a/drivers/gpu/drm/xe/xe_gt.c
>>>>> +++ b/drivers/gpu/drm/xe/xe_gt.c
>>>>> @@ -795,3 +795,20 @@ struct xe_hw_engine *xe_gt_any_hw_engine_by_reset_domain(struct xe_gt *gt,
>>>>>
>>>>> return NULL;
>>>>> }
>>>>> +
>>>>> +static bool has_dss(xe_dss_mask_t dss_mask, unsigned int dss)
>>>>> +{
>>>>> + unsigned long value = bitmap_get_value8(dss_mask, (dss / 8) * 8);
>>>>> +
>>>>> + return value & BIT(dss % 8);
>>>>> +}
>>>>> +
>>>>> +bool xe_gt_has_geometry_dss(struct xe_gt *gt, unsigned int dss)
>>>>> +{
>>>>> + return has_dss(gt->fuse_topo.g_dss_mask, dss);
>>>>> +}
>>>>> +
>>>>> +bool xe_gt_has_compute_dss(struct xe_gt *gt, unsigned int dss)
>>>>> +{
>>>>> + return has_dss(gt->fuse_topo.c_dss_mask, dss);
>>>>> +}
>>>>
>>>> It feels like these belong better in xe_gt_topology.[ch]. Ideally we'd
>>>> keep all the low-level representation of the masks abstracted away
>>>> inside of those files and just provide a sane interface for the rest of
>>>> the driver to query whatever it needs.
>>>
>>> sure, can move it.
>>>
>>>>
>>>>> diff --git a/drivers/gpu/drm/xe/xe_gt.h b/drivers/gpu/drm/xe/xe_gt.h
>>>>> index c1675bd44cf6d..36815d8cbc107 100644
>>>>> --- a/drivers/gpu/drm/xe/xe_gt.h
>>>>> +++ b/drivers/gpu/drm/xe/xe_gt.h
>>>>> @@ -70,4 +70,7 @@ static inline bool xe_gt_is_usm_hwe(struct xe_gt *gt, struct xe_hw_engine *hwe)
>>>>> hwe->instance == gt->usm.reserved_bcs_instance;
>>>>> }
>>>>>
>>>>> +bool xe_gt_has_geometry_dss(struct xe_gt *gt, unsigned int dss);
>>>>> +bool xe_gt_has_compute_dss(struct xe_gt *gt, unsigned int dss);
>>>>> +
>>>>> #endif
>>>>> diff --git a/drivers/gpu/drm/xe/xe_gt_mcr.c b/drivers/gpu/drm/xe/xe_gt_mcr.c
>>>>> index 77925b35cf8dc..e76cb0ae457aa 100644
>>>>> --- a/drivers/gpu/drm/xe/xe_gt_mcr.c
>>>>> +++ b/drivers/gpu/drm/xe/xe_gt_mcr.c
>>>>> @@ -291,11 +291,17 @@ static void init_steering_mslice(struct xe_gt *gt)
>>>>> gt->steering[LNCF].instance_target = 0; /* unused */
>>>>> }
>>>>>
>>>>> +static unsigned int
>>>>> +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 = 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 +689,12 @@ void xe_gt_mcr_steering_dump(struct xe_gt *gt, struct drm_printer *p)
>>>>> }
>>>>> }
>>>>> }
>>>>> +
>>>>> +void
>>>>> +xe_gt_mcr_get_dss_steering(struct xe_gt *gt, unsigned int dss, int *group,
>>>>> + int *instance)
>>>>
>>>> I think you're primarily adding this for eventual use in the
>>>> devcoredump, right? But I think there's also been some other series
>>>> trying to add some MCR registers of types other than DSS (e.g.,
>>>> per-mslice registers and such). Given that, maybe we should make this
>>>> function more general where you pass the MCR class as a parameter too?
>>>> E.g.,
>>>
>>> Yes, need that to get instdone registers for the Mesa error dump parser tool.
>>>
>>> Thanks for pointing that Zhanjun was also working on that.
>>> Looking at Zhanjun patches looks like we had almost the same functions but he is using 'ss' instead of 'dss'.
>> Yes, we are working on the same functions, sure I can use 'dss', no problem.
>
> Please then lets go with your version.
>
> Will rebase this series when you land your patches.
>
>>>
>>>>
>>>> void
>>>> xe_gt_mcr_get_steering(struct xe_gt *gt,
>>>> enum xe_steering_type type,
>>>> unsigned int id,
>>>> unsigned int *group,
>>>> unsigned int *instance)
>>>>
>>>>
>>>> BTW, I think Zhanjun is working along similar lines to what you have
>>>> here, so you guys might want to sync up?
>>>
>>> I don't think the function above is needed, what do you think Zhanjun?
>>
>> This function is to get group/instance steering from dss id, the only
>> place used is by for_each_xxx, the macro call this function, loop with
>> dss id, check against mask and call a code block, that's the typical
>> usage. For Xe, as we are using the g_dss_mask and c_dss_mask to test
>> bit, the converted group and instance is not used, so looks like
>> xe_gt_mcr_get_steering could be optimized out and the for_each macro
>> could be optimized to:
>>
>> #define for_each_geometry_dss(gt, dss) \
>> for (dss = 0; dss < XE_MAX_DSS_FUSE_BITS; dss++) \
>> if (_HAS_DSS(gt, dss))
>>
>> *_HAS_DSS means xe_gt_has_geometry_dss/xe_gt_has_compute_dss
Sorry, the above is for this macro body itself, when we iterate slices,
when the code block runs on each iteration, the group and instance id
will be referenced. So we can't go through the above optimization.
Zhanjun
>>
>> The above is based on only this macro calls xe_gt_mcr_get_steering, not
>> sure if Matthew has some thought about the steering_type?
>>
>> Regards,
>> Zhanjun
>>>
>>>>
>>>>
>>>> Matt
>>>>
>>>>> +{
>>>>> + unsigned int dss_per_group = get_dss_per_group(gt);
>>>>> + *group = dss / dss_per_group;
>>>>> + *instance = dss % dss_per_group;
>>>>> +}
>>>>> diff --git a/drivers/gpu/drm/xe/xe_gt_mcr.h b/drivers/gpu/drm/xe/xe_gt_mcr.h
>>>>> index 27ca1bc880a00..9f5f7dbb6fca8 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;
>>>>> @@ -24,6 +25,36 @@ void xe_gt_mcr_unicast_write(struct xe_gt *gt, struct xe_reg_mcr mcr_reg,
>>>>> void xe_gt_mcr_multicast_write(struct xe_gt *gt, struct xe_reg_mcr mcr_reg,
>>>>> u32 value);
>>>>>
>>>>> +void
>>>>> +xe_gt_mcr_get_dss_steering(struct xe_gt *gt, unsigned int dss, int *group,
>>>>> + int *instance);
>>>>> +
>>>>> void xe_gt_mcr_steering_dump(struct xe_gt *gt, struct drm_printer *p);
>>>>>
>>>>> +/**
>>>>> + * for_each_geometry_dss - Iterate over each DSS available for geometry
>>>>> + * @gt: GT structure
>>>>> + * @dss: DSS id
>>>>> + * @grp: group id to be in xe_gt_mcr_unicast_read()
>>>>> + * @inst: instance id to be in xe_gt_mcr_unicast_read()
>>>>> + */
>>>>> +#define for_each_geometry_dss(gt, dss, grp, inst) \
>>>>> + for (dss = 0, xe_gt_mcr_get_dss_steering(gt, dss, &grp, &inst); \
>>>>> + dss < XE_MAX_DSS_FUSE_BITS; \
>>>>> + dss++, xe_gt_mcr_get_dss_steering(gt, dss, &grp, &inst)) \
>>>>> + if (xe_gt_has_geometry_dss(gt, dss))
>>>>> +
>>>>> +/**
>>>>> + * for_each_compute_dss - Iterate over each DSS available for compute
>>>>> + * @gt: GT structure
>>>>> + * @dss: DSS id
>>>>> + * @grp: group id to be in xe_gt_mcr_unicast_read()
>>>>> + * @inst: instance id to be in xe_gt_mcr_unicast_read()
>>>>> + */
>>>>> +#define for_each_compute_dss(gt, dss, grp, inst) \
>>>>> + for (dss = 0, xe_gt_mcr_get_dss_steering(gt, dss, &grp, &inst); \
>>>>> + dss < XE_MAX_DSS_FUSE_BITS; \
>>>>> + dss++, xe_gt_mcr_get_dss_steering(gt, dss, &grp, &inst)) \
>>>>> + if (xe_gt_has_compute_dss(gt, dss))
>>>>> +
>>>>> #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 a8d7f272c30a0..c4942f2b37751 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)
>>>>>
>>>>> static void
>>>>> diff --git a/drivers/gpu/drm/xe/xe_gt_types.h b/drivers/gpu/drm/xe/xe_gt_types.h
>>>>> index 70c615dd14986..bb6dc1fcaa7dd 100644
>>>>> --- a/drivers/gpu/drm/xe/xe_gt_types.h
>>>>> +++ b/drivers/gpu/drm/xe/xe_gt_types.h
>>>>> @@ -25,9 +25,10 @@ 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)];
>>>>> +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(32 * XE_MAX_EU_FUSE_REGS)];
>>>>>
>>>>> struct xe_mmio_range {
>>>>> --
>>>>> 2.43.0
>>>>>
>>>>
>>>
>
More information about the Intel-xe
mailing list