Re: [PATCH v2 1/1] drm/xe/guc: Expose number of dss per group and helpers

["Summers, Stuart" <stuart.summers@intel.com>]

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@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
>   *