[PATCH v6 1/1] drm/xe: Add helper macro to loop each dss
Matt Roper
matthew.d.roper at intel.com
Wed Jan 31 22:49:11 UTC 2024
On Wed, Jan 31, 2024 at 01:48:11PM -0800, 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.
>
> Signed-off-by: Zhanjun Dong <zhanjun.dong at intel.com>
> ---
> drivers/gpu/drm/xe/xe_gt_mcr.c | 34 ++++++++++++++++++++++++++++-
> drivers/gpu/drm/xe/xe_gt_mcr.h | 14 ++++++++++++
> drivers/gpu/drm/xe/xe_gt_topology.c | 17 ++++++++++++---
> drivers/gpu/drm/xe/xe_gt_topology.h | 1 +
> drivers/gpu/drm/xe/xe_gt_types.h | 2 ++
> 5 files changed, 64 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..711cbfd8a488 100644
> --- a/drivers/gpu/drm/xe/xe_gt_mcr.c
> +++ b/drivers/gpu/drm/xe/xe_gt_mcr.c
> @@ -291,11 +291,43 @@ static void init_steering_mslice(struct xe_gt *gt)
> gt->steering[LNCF].instance_target = 0; /* unused */
> }
>
> +static int xe_gt_mcr_get_dss_per_group(struct xe_gt *gt)
Minor nitpick: We usually don't put naming prefixes on static
functions. Just calling it something like get_dss_per_group() helps
reinforce that it's just a local function that isn't exposed to the
greater driver.
> +{
> + 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.
> + */
> +bool xe_gt_mcr_get_dss_steering(struct xe_gt *gt, unsigned int dss, unsigned int *group,
> + unsigned int *instance)
> +{
> + int dss_per_grp;
> +
> + if (dss >= XE_MAX_DSS_FUSE_BITS) {
We don't expect to ever get called with an out-of-range DSS ID, right?
If we hit this case it indicates a driver bug at the caller? If so, we
might want to just change this to an assert:
xe_gt_assert(dss < XE_MAX_DSS_FUSE_BITS);
so that this gets flagged loudly in CI debug builds and we notice the
mistake quickly.
The assert will be compiled out of non-debug builds, and even if we trip
this we'll just continue on and return 'true' at the end of the function
anyway, but even if we screw up enough that this leaks into a production
driver we're just going to return bogus group / instance IDs; it won't
cause any kind of greater problem like a kernel crash or anything. The
for_each_dss_steering() should still finish looping based on the
"dss_ >= 0" condition (since xe_gt_topology_get_next_dss() returns -1 if
there are no DSS left to return), so that loop won't go infinite either.
> + xe_gt_dbg(gt, "DSS id out of range: %d\n", dss);
> + return false;
> + }
> +
> + dss_per_grp = xe_gt_mcr_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 = 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;
> diff --git a/drivers/gpu/drm/xe/xe_gt_mcr.h b/drivers/gpu/drm/xe/xe_gt_mcr.h
> index 27ca1bc880a0..91cf02b97bc1 100644
> --- a/drivers/gpu/drm/xe/xe_gt_mcr.h
> +++ b/drivers/gpu/drm/xe/xe_gt_mcr.h
> @@ -7,6 +7,8 @@
> #define _XE_GT_MCR_H_
>
> #include "regs/xe_reg_defs.h"
> +#include "xe_gt_types.h"
I don't think this include is necessary anymore is it?
If I'm missing something and we actually do need it, then it should come
below the topology header to keep these in alphabetic order.
> +#include "xe_gt_topology.h"
>
> struct drm_printer;
> struct xe_gt;
> @@ -25,5 +27,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.
> + */
> +#define for_each_dss_steering(dss_, gt_, group_, instance_) \
> + for (dss_ = xe_gt_topology_get_next_dss(gt, 0); \
> + dss_ >= 0; \
> + dss_ = xe_gt_topology_get_next_dss(gt, dss_ + 1)) \
> + for_each_if(xe_gt_mcr_get_dss_steering(gt_, dss_, &(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..37596d5f9574 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,17 @@ bool xe_gt_topology_has_dss_in_quadrant(struct xe_gt *gt, int quad)
>
> return quad_first < (quad + 1) * dss_per_quad;
> }
> +
> +int xe_gt_topology_get_next_dss(struct xe_gt *gt, int from)
We should probably add a kerneldoc block for this function explaining
the interface. Otherwise it may not be immediately obvious that this
function returns -1 after all of the bits have been returned, thus
allowing our 'for each' loop to terminate properly.
Matt
> +{
> + 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)];
> --
> 2.34.1
>
--
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation
More information about the Intel-xe
mailing list