[PATCH 1/3] drm/xe: Cleanup CCS mode related code
Niranjana Vishwanathapura
niranjana.vishwanathapura at intel.com
Wed Jan 17 07:42:49 UTC 2024
On Tue, Jan 16, 2024 at 04:25:12PM -0600, Lucas De Marchi wrote:
>On Sat, Dec 16, 2023 at 01:41:16PM -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 at 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_MASK + 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)
>
>if you are using this as argument to hweight32(), you actually don't
>need it
>
>num_ccs = hweight32(gt->info.engine_mask & XE_HW_ENGINE_CCS_MASK)
>
There is a usage below where we use it without hweight32().
>>+
>>+/* 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);
>
>what's up with the use of __engine_mask here?
>
The __engine_mask gives us all engines including the ones that are fused off.
I am using this so as to use the disable mask for fused off engines (not really
sure if that makes any difference, but it is cleaner when we write to ccs_mode
register).
>>+ 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 */
>> int num_slices = hweight32(CCS_MASK(gt));
>> struct xe_device *xe = gt_to_xe(gt);
>>+ u32 num_engines = gt->ccs_mode;
>
>
>this is a source of confusion I mentioned. Just above you do:
>
> mode = xe_gt_ccs_mode_disabled(gt)
>
>in which "mode" refers to the register format. Then here you do:
>
> num_engines = gt->ccs_mode
>
>so "ccs_mode" means something else. Am I misunderstanding anything or
>gt->ccs_mode should be called something else?
gt->ccs_mode is just the number of compute engines enabled by the user
(as documented in the kernel-doc comment for this parameter). As 'ccs_mode'
is the name used for this in all discussions, I kept the same name in
gt->ccs_mode.
>
>> int width, cslice = 0;
>
>cslice = 0, but still using num_slices rather than num_cslices. Similar
>thing here:
>
> /*
> * Loop over all available slices and assign each a user engine.
> ^^^^^^
>
num_slices are changed to num_cslices in the next patch in this series.
Yah, in some comments, "slice/s" is still being used. I will change it to
"compute slice/s"
Thanks,
Niranjana
>Lucas De Marchi
>
>> 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 */
>>+ 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
>>
>>--
>>2.43.0
>>
More information about the Intel-xe
mailing list