[PATCH v3 2/9] drm/xe/guc: Expose dss per group for GuC error capture

[Matt Roper]

On Thu, Jan 18, 2024 at 04:41:56PM -0800, Zhanjun Dong wrote:
> Expose helper for dss per group of mcr, GuC error capture feature
> need this info to prepare buffer required.
> 
> Signed-off-by: Zhanjun Dong <zhanjun.dong at intel.com>
> ---
>  drivers/gpu/drm/xe/xe_gt_mcr.c          |  2 +-
>  drivers/gpu/drm/xe/xe_gt_mcr.h          |  3 +++
>  drivers/gpu/drm/xe/xe_gt_topology.c     |  3 ---
>  drivers/gpu/drm/xe/xe_guc_capture.c     | 29 +++++++++++++++++++++++++
>  drivers/gpu/drm/xe/xe_hw_engine_types.h |  3 +++
>  5 files changed, 36 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..5874eb71cbc1 100644
> --- a/drivers/gpu/drm/xe/xe_gt_mcr.c
> +++ b/drivers/gpu/drm/xe/xe_gt_mcr.c
> @@ -295,7 +295,7 @@ 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_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..91e6f14b934f 100644
> --- a/drivers/gpu/drm/xe/xe_gt_mcr.h
> +++ b/drivers/gpu/drm/xe/xe_gt_mcr.h
> @@ -7,10 +7,13 @@
>  #define _XE_GT_MCR_H_
>  
>  #include "regs/xe_reg_defs.h"
> +#include "xe_gt_types.h"
>  
>  struct drm_printer;
>  struct xe_gt;
>  
> +#define XE_GT_MCR_DSS_PER_GROUP(gt) (gt_to_xe(gt)->info.platform == XE_PVC ? 8 : 4)

I'd just go ahead and move this to a real function rather than using a
macro.  We expect the hardware to expose the proper value here via a
fuse register at some point in the future (meaning we'll be able to stop
hardcoding magic numbers in the driver), and once that happens we'll
want to have a function anyway.  That will also allow us to avoid the
extra header include.


> +
>  void xe_gt_mcr_init(struct xe_gt *gt);
>  
>  void xe_gt_mcr_set_implicit_defaults(struct xe_gt *gt);
> 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_guc_capture.c b/drivers/gpu/drm/xe/xe_guc_capture.c
> index cacd50f4718a..2e1ed5bbdd56 100644
> --- a/drivers/gpu/drm/xe/xe_guc_capture.c
> +++ b/drivers/gpu/drm/xe/xe_guc_capture.c
> @@ -93,6 +93,35 @@
>  	{ SFC_DONE(2),		    0,      0, "SFC_DONE[2]" }, \
>  	{ SFC_DONE(3),		    0,      0, "SFC_DONE[3]" }
>  
> +static 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_DSS_PER_GROUP(gt);
> +
> +	*group = dss / dss_per_grp;
> +	*instance = dss % dss_per_grp;
> +}
> +
> +static bool xe_sseu_has_subslice(struct xe_gt *gt, int slice, int subslice)
> +{
> +	int dss_per_grp = XE_GT_MCR_DSS_PER_GROUP(gt);
> +	int index = slice * dss_per_grp + subslice;
> +
> +	return index >= XE_MAX_DSS_FUSE_BITS ? false : test_bit(index, gt->fuse_topo.g_dss_mask);
> +}

It would be better if we could avoid leaking the internal implementation
of the topology outside of xe_gt_topology.[ch]; if we ever need to
change it in the future it's easier when it's self-contained inside a
single file with well-defined interfaces to the rest of the driver.  So
it might be better to have

    bool xe_gt_topology_has_dss(struct xe_gt *gt, int dss_num);

to wrap the test_bit part, and

    int xe_gt_mcr_get_steering(struct xe_gt* gt, enum xe_steering_type type,
                               int instance, int *grp, int *inst);

for looking up the steering targets of a specific instance of an MCR
register.  Then you can use those two helper functions in the GuC
capture code without exposing the internal interface details (and they
can be re-used in other parts of the code as well).


BTW, why does your function here only check the g_dss_mask?  On a
platform like PVC that won't match any DSS (since the geometry mask is
empty and only the compute mask is set).


Matt

> +
> +#define _HAS_SS(ss_, gt_, group_, instance_) xe_sseu_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_ss_steering(ss_, gt_, group_, instance_) \
> +	for (ss_ = 0, xe_gt_mcr_get_ss_steering(gt_, 0, &group_, &instance_); \
> +	     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_))
> +
>  int xe_guc_capture_init(struct xe_guc *guc)
>  {
>  	return 0;
> diff --git a/drivers/gpu/drm/xe/xe_hw_engine_types.h b/drivers/gpu/drm/xe/xe_hw_engine_types.h
> index dfeaaac08b7f..c258228b244f 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)
> +
>  /**
>   * struct xe_hw_engine_class_intf - per hw engine class struct interface
>   *
> -- 
> 2.34.1
> 

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation