Re: [PATCH v9 1/1] drm/xe: Add helper macro to loop each dss

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

Hi Zhanjun,

I was about to push this patch, but it needs new revision, see below.
Also added some nits

On 09.02.2024 18:45, 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.

nit: please be consistent on naming dss vs DSS

> 
> Signed-off-by: Zhanjun Dong <zhanjun.dong@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_gt_mcr.c      | 32 ++++++++++++++++++++++++++++-
>  drivers/gpu/drm/xe/xe_gt_mcr.h      | 13 ++++++++++++
>  drivers/gpu/drm/xe/xe_gt_topology.c | 31 +++++++++++++++++++++++++---
>  drivers/gpu/drm/xe/xe_gt_topology.h |  1 +
>  drivers/gpu/drm/xe/xe_gt_types.h    |  2 ++
>  5 files changed, 75 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..44163fe8c7b9 100644
> --- a/drivers/gpu/drm/xe/xe_gt_mcr.c
> +++ b/drivers/gpu/drm/xe/xe_gt_mcr.c
> @@ -6,6 +6,7 @@
>  #include "xe_gt_mcr.h"
>  
>  #include "regs/xe_gt_regs.h"
> +#include "xe_assert.h"
>  #include "xe_gt.h"
>  #include "xe_gt_topology.h"
>  #include "xe_gt_types.h"
> @@ -291,11 +292,40 @@ static void init_steering_mslice(struct xe_gt *gt)
>  	gt->steering[LNCF].instance_target = 0;		/* unused */
>  }
>  
> +static int get_dss_per_group(struct xe_gt *gt)

nit: shouldn't this function be 'unsigned int' ?

> +{
> +	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.

nit: this is a 'bool' function so having a "Return:" section is desired

> + */
> +bool xe_gt_mcr_get_dss_steering(struct xe_gt *gt, unsigned int dss, unsigned int *group,
> +				unsigned int *instance)
> +{
> +	int dss_per_grp;

nit: this could be one-liner:

	unsigned int dss_per_grp = get_dss_per_group(gt);

> +
> +	xe_gt_assert(gt, dss < XE_MAX_DSS_FUSE_BITS);
> +
> +	dss_per_grp = 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 = get_dss_per_group(gt);
>  
>  	gt->steering[DSS].group_target = dss / dss_per_grp;
>  	gt->steering[DSS].instance_target = dss % dss_per_grp;

nit: this looks like a good candidate to use this new function
xe_gt_mcr_get_dss_steering() to avoid code duplication


> diff --git a/drivers/gpu/drm/xe/xe_gt_mcr.h b/drivers/gpu/drm/xe/xe_gt_mcr.h
> index 27ca1bc880a0..5b4e74da82a1 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_topology.h"
>  
>  struct drm_printer;
>  struct xe_gt;
> @@ -25,5 +26,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.
> + */

shouldn't this be regular kernel-doc ?

> +#define for_each_dss_steering(dss_, gt_, group_, instance_) \
> +	for (dss_ = xe_gt_topology_get_next_dss(gt, 0); \

BUG: using outside "gt" instead of "gt_" parameter

also it looks that upstream BKM is to _not_ use underscore for param
names as params shouldn't leak anyway)


> +	     dss_ >= 0; \
> +	     dss_ = xe_gt_topology_get_next_dss(gt, dss_ + 1)) \

ditto

> +		for_each_if(xe_gt_mcr_get_dss_steering(gt_, dss_, &(group_), &(instance_)))

btw, did you consider to split this macro into two:

xe_gt_topology.h:

	#define for_each_dss(dss, gt) \
		for (dss = xe_gt_topology_get_next_dss(gt, 0); \
		     dss >= 0; \
		     dss = xe_gt_topology_get_next_dss(gt, dss + 1))

xe_gt_mcr.h:

	#define for_each_dss_steering(dss, gt, group, instance) \
		 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..97c0a4b8fe40 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,31 @@ bool xe_gt_topology_has_dss_in_quadrant(struct xe_gt *gt, int quad)
>  
>  	return quad_first < (quad + 1) * dss_per_quad;
>  }
> +
> +/**
> + * xe_gt_topology_get_next_dss - returns the next DSS id from a start position
> + * @gt: GT structure
> + * @from: An index to start search form

typo s/form/from

> + *
> + * Search from this GT's DSS-fuses-topology and return the DSS-ID of the
> + * next valid DSS instance after DSS-ID 'from'. Depending on the platform
> + * construction, DSS instances could be for geometry (RENDER engine) or
> + * compute (COMPUTE engine). However, all current platforms that have
> + * both engines have the same instances thus we can combine both masks
> + * internally before performing this operation.
> + *
> + * Return -1 if not found since given "from" position.

you should use "Return:" tag and likely say what is returned if found

> + */
> +int xe_gt_topology_get_next_dss(struct xe_gt *gt, int from)
> +{
> +	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)];

nit: above looks like a good candidates to use new FUSE_BITS definitions

Michal