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

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

On Tue, 2024-01-30 at 10:57 -0800, Zhanjun Dong wrote:
> Expose helper for dss per group. 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@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_gt_mcr.c      | 40
> ++++++++++++++++++++++++++++-
>  drivers/gpu/drm/xe/xe_gt_mcr.h      | 17 ++++++++++++
>  drivers/gpu/drm/xe/xe_gt_topology.c |  1 -
>  drivers/gpu/drm/xe/xe_gt_types.h    |  1 +
>  4 files changed, 57 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_gt_mcr.c
> b/drivers/gpu/drm/xe/xe_gt_mcr.c
> index 77925b35cf8d..7cd93bcb811f 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)
> +{
> +       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,36 @@ void xe_gt_mcr_steering_dump(struct xe_gt *gt,
> struct drm_printer *p)
>                 }
>         }
>  }
> +
> +/**
> + * xe_gt_mcr_get_dss_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_dss_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) {
> +               xe_gt_dbg(gt, "DSS id out of range: slice:%d
> subslice:%d\n", slice, subslice);
> +               return false;
> +       }
> +
> +       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..7d1eb180befd 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_dss_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(gt_, group_, instance_)

Maybe gt__, group__, instance__

> xe_gt_mcr_dss_has_subslice(gt_, group_, instance_)
> +
> +/*
> + * 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_dss_steering(dss_, gt_, group_, instance_) \
> +       for (dss_ = 0, xe_gt_mcr_get_dss_steering(gt_, 0, &group_,
> &instance_); \
> +            dss_ < XE_MAX_DSS_FUSE_BITS; \
> +            dss_++, xe_gt_mcr_get_dss_steering(gt_, dss_, &group_,
> &instance_)) \

I think you're still going to get those checkpatch warnings here.

Are you planning on addressing that?

> +               for_each_if(_HAS_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..c4942f2b3775 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)

Honestly I do think these should be in the same file at least. Can you
move the EU FUSE BITS to xe_gt_types.h too?

Thanks,
Stuart

>  
>  static void
> diff --git a/drivers/gpu/drm/xe/xe_gt_types.h
> b/drivers/gpu/drm/xe/xe_gt_types.h
> index 70c615dd1498..b4df7d35dec7 100644
> --- a/drivers/gpu/drm/xe/xe_gt_types.h
> +++ b/drivers/gpu/drm/xe/xe_gt_types.h
> @@ -25,6 +25,7 @@ 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)];