Re: [PATCH 1/3] drm/xe: Cleanup CCS mode related code

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

On Sat, 2023-12-16 at 13:41 -0800, Niranjana Vishwanathapura wrote:
> Refactor some of the CCS mode related code to make it compliant
> with the coding guidelines.
> 
> v2: Fix macro usage
> 
> Signed-off-by: Niranjana Vishwanathapura
> <niranjana.vishwanathapura@intel.com>
> ---
>  drivers/gpu/drm/xe/regs/xe_gt_regs.h | 12 +-------
>  drivers/gpu/drm/xe/xe_gt.c           |  4 +--
>  drivers/gpu/drm/xe/xe_gt.h           |  2 --
>  drivers/gpu/drm/xe/xe_gt_ccs_mode.c  | 45 ++++++++++++++++++++------
> --
>  drivers/gpu/drm/xe/xe_gt_ccs_mode.h  | 12 ++------
>  5 files changed, 37 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/regs/xe_gt_regs.h
> b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
> index 1dd361046b5dc..2d9c004838437 100644
> --- a/drivers/gpu/drm/xe/regs/xe_gt_regs.h
> +++ b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
> @@ -386,18 +386,8 @@
>  #define   RCU_MODE_FIXED_SLICE_CCS_MODE                REG_BIT(1)
>  #define   RCU_MODE_CCS_ENABLE                  REG_BIT(0)
>  
> -/*
> - * Total of 4 cslices, where each cslice is in the form:
> - *   [0-3]     CCS ID
> - *   [4-6]     RSVD
> - *   [7]       Disabled
> - */
>  #define CCS_MODE                               XE_REG(0x14804)
> -#define   CCS_MODE_CSLICE_0_3_MASK             REG_GENMASK(11, 0) /*
> 3 bits per cslice */
> -#define   CCS_MODE_CSLICE_MASK                 0x7 /* CCS0-3 + rsvd
> */
> -#define  
> CCS_MODE_CSLICE_WIDTH                        ilog2(CCS_MODE_CSLICE_MA
> SK + 1)
> -#define   CCS_MODE_CSLICE(cslice, ccs) \
> -       ((ccs) << ((cslice) * CCS_MODE_CSLICE_WIDTH))
> +#define   CCS_MODE_CSLICE_ASSIGNMENT           REG_GENMASK(2, 0)
>  
>  #define FORCEWAKE_ACK_GT                       XE_REG(0x130044)
>  #define   FORCEWAKE_KERNEL                     BIT(0)
> diff --git a/drivers/gpu/drm/xe/xe_gt.c b/drivers/gpu/drm/xe/xe_gt.c
> index 3af2adec12956..342f82f245f39 100644
> --- a/drivers/gpu/drm/xe/xe_gt.c
> +++ b/drivers/gpu/drm/xe/xe_gt.c
> @@ -462,7 +462,7 @@ static int all_fw_domain_init(struct xe_gt *gt)
>         /* Configure default CCS mode of 1 engine with all resources
> */
>         if (xe_gt_ccs_mode_enabled(gt)) {
>                 gt->ccs_mode = 1;
> -               xe_gt_apply_ccs_mode(gt);
> +               xe_gt_ccs_mode_apply(gt);
>         }
>  
>         if (IS_SRIOV_PF(gt_to_xe(gt)) && !xe_gt_is_media_type(gt))
> @@ -584,7 +584,7 @@ static int do_gt_restart(struct xe_gt *gt)
>         }
>  
>         /* Get CCS mode in sync between sw/hw */
> -       xe_gt_apply_ccs_mode(gt);
> +       xe_gt_ccs_mode_apply(gt);
>  
>         return 0;
>  }
> diff --git a/drivers/gpu/drm/xe/xe_gt.h b/drivers/gpu/drm/xe/xe_gt.h
> index f3c780bd266dd..11265e851a811 100644
> --- a/drivers/gpu/drm/xe/xe_gt.h
> +++ b/drivers/gpu/drm/xe/xe_gt.h
> @@ -17,8 +17,6 @@
>                 for_each_if(((hwe__) = (gt__)->hw_engines + (id__))
> && \
>                           xe_hw_engine_is_valid((hwe__)))
>  
> -#define CCS_MASK(gt) (((gt)->info.engine_mask &
> XE_HW_ENGINE_CCS_MASK) >> XE_HW_ENGINE_CCS0)
> -
>  #ifdef CONFIG_FAULT_INJECTION
>  extern struct fault_attr gt_reset_failure;
>  static inline bool xe_fault_inject_gt_reset(void)
> diff --git a/drivers/gpu/drm/xe/xe_gt_ccs_mode.c
> b/drivers/gpu/drm/xe/xe_gt_ccs_mode.c
> index 529fc286cd06c..ee1065c44da7f 100644
> --- a/drivers/gpu/drm/xe/xe_gt_ccs_mode.c
> +++ b/drivers/gpu/drm/xe/xe_gt_ccs_mode.c
> @@ -12,17 +12,38 @@
>  #include "xe_gt_sysfs.h"
>  #include "xe_mmio.h"
>  
> -static void __xe_gt_apply_ccs_mode(struct xe_gt *gt, u32
> num_engines)
> +#define
> CCS_MODE_CSLICE_WIDTH          ilog2(CCS_MODE_CSLICE_ASSIGNMENT + 1)
> +#define CCS_MODE_CSLICE(cslice, ccs)   \
> +       ((ccs) << ((cslice) * CCS_MODE_CSLICE_WIDTH))
> +
> +#define CCS_MASK(gt) (((gt)->info.engine_mask &
> XE_HW_ENGINE_CCS_MASK) >> XE_HW_ENGINE_CCS0)
> +
> +/* Get CCS mode mask to disable all compute slices */
> +static u32 xe_gt_ccs_mode_disabled(struct xe_gt *gt)
>  {
> -       u32 mode = CCS_MODE_CSLICE_0_3_MASK; /* disable all by
> default */
> +       int i, num_slices = hweight32(gt->info.__engine_mask &
> XE_HW_ENGINE_CCS_MASK);
> +       u32 mode = 0;
> +
> +       for (i = 0; i < num_slices; i++)
> +               mode |= CCS_MODE_CSLICE(i,
> CCS_MODE_CSLICE_ASSIGNMENT);
> +
> +       return mode;
> +}
> +
> +void xe_gt_ccs_mode_apply(struct xe_gt *gt)
> +{
> +       u32 mode = xe_gt_ccs_mode_disabled(gt); /* disable all by
> default */

This comment doesn't apply any more if there's a new calculation which
can reduce from all right?

>         int num_slices = hweight32(CCS_MASK(gt));
>         struct xe_device *xe = gt_to_xe(gt);
> +       u32 num_engines = gt->ccs_mode;
>         int width, cslice = 0;
>         u32 config = 0;
>  
> -       xe_assert(xe, xe_gt_ccs_mode_enabled(gt));
> +       if (!num_engines)
> +               return;
>  
> -       xe_assert(xe, num_engines && num_engines <= num_slices);
> +       xe_assert(xe, xe_gt_ccs_mode_enabled(gt));
> +       xe_assert(xe, num_engines <= num_slices);
>         xe_assert(xe, !(num_slices % num_engines));
>  
>         /*
> @@ -60,7 +81,7 @@ static void __xe_gt_apply_ccs_mode(struct xe_gt
> *gt, u32 num_engines)
>                         while ((CCS_MASK(gt) & BIT(cslice)) == 0)
>                                 cslice++;
>  
> -                       mode &= ~CCS_MODE_CSLICE(cslice,
> CCS_MODE_CSLICE_MASK);
> +                       mode &= ~CCS_MODE_CSLICE(cslice,
> CCS_MODE_CSLICE_ASSIGNMENT);
>                         mode |= CCS_MODE_CSLICE(cslice, hwe-
> >instance);
>                         cslice++;
>                 }
> @@ -72,14 +93,6 @@ static void __xe_gt_apply_ccs_mode(struct xe_gt
> *gt, u32 num_engines)
>                    mode, config, num_engines, num_slices);
>  }
>  
> -void xe_gt_apply_ccs_mode(struct xe_gt *gt)
> -{
> -       if (!gt->ccs_mode)
> -               return;
> -
> -       __xe_gt_apply_ccs_mode(gt, gt->ccs_mode);
> -}
> -
>  static ssize_t
>  num_cslices_show(struct device *kdev,
>                  struct device_attribute *attr, char *buf)
> @@ -189,3 +202,9 @@ void xe_gt_ccs_mode_sysfs_init(struct xe_gt *gt)
>                          __func__, err);
>         }
>  }
> +
> +bool xe_gt_ccs_mode_enabled(const struct xe_gt *gt)
> +{
> +       /* Check if there are more than one compute engines available

/* Check if there is more than one compute engine available */

Thanks,
Stuart

> */
> +       return hweight32(CCS_MASK(gt)) > 1;
> +}
> diff --git a/drivers/gpu/drm/xe/xe_gt_ccs_mode.h
> b/drivers/gpu/drm/xe/xe_gt_ccs_mode.h
> index f39975aaaab0d..0dc191a59525c 100644
> --- a/drivers/gpu/drm/xe/xe_gt_ccs_mode.h
> +++ b/drivers/gpu/drm/xe/xe_gt_ccs_mode.h
> @@ -6,19 +6,11 @@
>  #ifndef _XE_GT_CCS_MODE_H_
>  #define _XE_GT_CCS_MODE_H_
>  
> -#include "xe_device_types.h"
> -#include "xe_gt.h"
>  #include "xe_gt_types.h"
> -#include "xe_platform_types.h"
>  
> -void xe_gt_apply_ccs_mode(struct xe_gt *gt);
> +void xe_gt_ccs_mode_apply(struct xe_gt *gt);
>  void xe_gt_ccs_mode_sysfs_init(struct xe_gt *gt);
> -
> -static inline bool xe_gt_ccs_mode_enabled(const struct xe_gt *gt)
> -{
> -       /* Check if there are more than one compute engines available
> */
> -       return hweight32(CCS_MASK(gt)) > 1;
> -}
> +bool xe_gt_ccs_mode_enabled(const struct xe_gt *gt);
>  
>  #endif
>