[PATCH v2 1/1] drm/xe/guc: Expose number of dss per group and helpers
Summers, Stuart
stuart.summers at intel.com
Mon Jan 29 20:17:07 UTC 2024
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)?
>
> 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()
> +{
> + 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.
> + 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?
> 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.
> + 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?
Thanks,
Stuart
> +
> /**
> * struct xe_hw_engine_class_intf - per hw engine class struct
> interface
> *
More information about the Intel-xe
mailing list