Re: [PATCH 16/24] drm/xe/eudebug_test: Introduce xe_eudebug wa kunit test

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

Hi Mika,

few nits below...

nit: maybe title should be:

	drm/xe/tests: Introduce eudebug live tests


On 4/30/2026 12:51 PM, Mika Kuoppala wrote:
> From: Christoph Manszewski <christoph.manszewski@intel.com>
> 
> Introduce kunit test for eudebug. For now it checks the dynamic
> application of WAs.
> 
> v2: adapt to removal of call_for_each_device (Mika)
> v3: s/FW_RENDER/FORCEWAKE_ALL (Mika)
> 
> Signed-off-by: Christoph Manszewski <christoph.manszewski@intel.com>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
>  drivers/gpu/drm/xe/tests/xe_eudebug.c       | 183 ++++++++++++++++++++

nit: while we are still inconsistent in file naming,
maybe we should follow some rules and

- use _kunit file suffix for pure KUnit tests
- use _test file suffix for our live tests (not strictly kunit)

see https://docs.kernel.org/dev-tools/kunit/style.html#test-file-and-module-names

>  drivers/gpu/drm/xe/tests/xe_live_test_mod.c |   5 +
>  drivers/gpu/drm/xe/xe_eudebug.c             |   4 +
>  3 files changed, 192 insertions(+)
>  create mode 100644 drivers/gpu/drm/xe/tests/xe_eudebug.c
> 
> diff --git a/drivers/gpu/drm/xe/tests/xe_eudebug.c b/drivers/gpu/drm/xe/tests/xe_eudebug.c
> new file mode 100644
> index 000000000000..f839fb292b9b
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/tests/xe_eudebug.c
> @@ -0,0 +1,183 @@
> +// SPDX-License-Identifier: GPL-2.0 AND MIT
> +/*
> + * Copyright © 2024 Intel Corporation

nit: 2026

> + */
> +
> +#include <kunit/visibility.h>
> +
> +#include "regs/xe_gt_regs.h"
> +#include "regs/xe_engine_regs.h"
> +
> +#include "xe_force_wake.h"
> +#include "xe_gt_mcr.h"
> +#include "xe_mmio.h"
> +
> +#include "tests/xe_kunit_helpers.h"
> +#include "tests/xe_pci_test.h"
> +#include "tests/xe_test.h"
> +
> +#undef XE_REG_MCR
> +#define XE_REG_MCR(r_, ...)	((const struct xe_reg_mcr){					\
> +				 .__reg = XE_REG_INITIALIZER(r_,  ##__VA_ARGS__, .mcr = 1)	\
> +				 })
> +
> +static const char *reg_to_str(struct xe_reg reg)
> +{
> +	if (reg.raw == TD_CTL.__reg.raw)
> +		return "TD_CTL";
> +	else if (reg.raw == CS_DEBUG_MODE2(RENDER_RING_BASE).raw)
> +		return "CS_DEBUG_MODE2";
> +	else if (reg.raw == ROW_CHICKEN.__reg.raw)
> +		return "ROW_CHICKEN";
> +	else if (reg.raw == ROW_CHICKEN2.__reg.raw)
> +		return "ROW_CHICKEN2";
> +	else if (reg.raw == ROW_CHICKEN3.__reg.raw)
> +		return "ROW_CHICKEN3";
> +	else
> +		return "UNKNOWN REG";
> +}
> +
> +static u32 get_reg_mask(struct xe_device *xe, struct xe_reg reg)
> +{
> +	struct kunit *test = kunit_get_current_test();
> +	u32 val = 0;
> +
> +	if (reg.raw == TD_CTL.__reg.raw) {
> +		val = TD_CTL_BREAKPOINT_ENABLE |
> +		      TD_CTL_FORCE_THREAD_BREAKPOINT_ENABLE |
> +		      TD_CTL_FEH_AND_FEE_ENABLE;
> +
> +		if (GRAPHICS_VERx100(xe) >= 1250)
> +			val |= TD_CTL_GLOBAL_DEBUG_ENABLE;
> +
> +	} else if (reg.raw == CS_DEBUG_MODE2(RENDER_RING_BASE).raw) {
> +		val = GLOBAL_DEBUG_ENABLE;
> +	} else if (reg.raw == ROW_CHICKEN.__reg.raw) {
> +		val = STALL_DOP_GATING_DISABLE;
> +	} else if (reg.raw == ROW_CHICKEN2.__reg.raw) {
> +		val = XEHPC_DISABLE_BTB;
> +	} else if (reg.raw == ROW_CHICKEN3.__reg.raw) {
> +		val = XE2_EUPEND_CHK_FLUSH_DIS;
> +	} else {
> +		kunit_warn(test, "Invalid register selection: %u\n", reg.raw);

nit: use KUNIT_FAIL(kunit_get_current_test(), "...");

> +	}
> +
> +	return val;
> +}
> +
> +static u32 get_reg_expected(struct xe_device *xe, struct xe_reg reg, bool enable_eudebug)
> +{
> +	u32 reg_mask = get_reg_mask(xe, reg);
> +	u32 reg_bits = 0;
> +
> +	if (enable_eudebug || reg.raw == ROW_CHICKEN3.__reg.raw)
> +		reg_bits = reg_mask;
> +	else
> +		reg_bits = 0;
> +
> +	return reg_bits;
> +}
> +
> +static void check_reg(struct xe_gt *gt, bool enable_eudebug, struct xe_reg reg)
> +{
> +	struct kunit *test = kunit_get_current_test();

nit: as this testcase function, why not pass test as a param?

> +	struct xe_device *xe = gt_to_xe(gt);
> +	u32 reg_bits_expected = get_reg_expected(xe, reg, enable_eudebug);
> +	u32 reg_mask = get_reg_mask(xe, reg);
> +	u32 reg_bits = 0;
> +
> +	if (reg.mcr)
> +		reg_bits = xe_gt_mcr_unicast_read_any(gt, (struct xe_reg_mcr){.__reg = reg});
> +	else
> +		reg_bits = xe_mmio_read32(&gt->mmio, reg);
> +
> +	reg_bits &= reg_mask;
> +
> +	kunit_printk(KERN_DEBUG, test, "%s bits: expected == 0x%x; actual == 0x%x\n",
> +		     reg_to_str(reg), reg_bits_expected, reg_bits);

nit: is this message really useful?
if bits are different then below EXPECT macro will print them anyway

> +	KUNIT_EXPECT_EQ_MSG(test, reg_bits_expected, reg_bits,
> +			    "Invalid bits set for %s\n", reg_to_str(reg));
> +}
> +
> +static void __check_regs(struct xe_gt *gt, bool enable_eudebug)

nit: maybe this function should be split into two:

- first that returns register
- second that runs tests on it?

> +{
> +	struct xe_device *xe = gt_to_xe(gt);
> +
> +	if (GRAPHICS_VERx100(xe) >= 1200)
> +		check_reg(gt, enable_eudebug, TD_CTL.__reg);
> +
> +	if (GRAPHICS_VERx100(xe) >= 1250 && GRAPHICS_VERx100(xe) <= 1274)
> +		check_reg(gt, enable_eudebug, ROW_CHICKEN.__reg);
> +
> +	if (xe->info.platform == XE_PVC)
> +		check_reg(gt, enable_eudebug, ROW_CHICKEN2.__reg);
> +
> +	if (GRAPHICS_VERx100(xe) >= 2000 && GRAPHICS_VERx100(xe) <= 2004)
> +		check_reg(gt, enable_eudebug, ROW_CHICKEN3.__reg);


> +}
> +
> +static void check_regs(struct xe_device *xe, bool enable_eudebug)
> +{
> +	struct kunit *test = kunit_get_current_test();
> +	struct xe_gt *gt;
> +	unsigned int fw_ref;
> +	u8 id;
> +
> +	kunit_printk(KERN_DEBUG, test, "Check regs for eudebug %s\n",
> +		     enable_eudebug ? "enabled" : "disabled");
> +
> +	xe_pm_runtime_get(xe);

nit: no need to take extra rpm, our live test helper takes care of it

> +	for_each_gt(gt, xe, id) {
> +		if (xe_gt_is_media_type(gt))
> +			continue;
> +
> +		/* XXX: Figure out per platform proper domain */
> +		fw_ref = xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL);
> +		KUNIT_ASSERT_TRUE_MSG(test, fw_ref, "Forcewake failed.\n");
> +
> +		__check_regs(gt, enable_eudebug);
> +
> +		xe_force_wake_put(gt_to_fw(gt), fw_ref);
> +	}
> +	xe_pm_runtime_put(xe);
> +}
> +
> +static int toggle_reg_value(struct xe_device *xe)
> +{
> +	struct kunit *test = kunit_get_current_test();
> +	bool enable_eudebug = xe_eudebug_is_enabled(xe);
> +
> +	kunit_printk(KERN_DEBUG, test, "Test eudebug WAs for graphics version: %u\n",
> +		     GRAPHICS_VERx100(xe));

nit: if GFX version is so important for EUDEBUG then maybe this
message should be part of the custom test_init()?

static int xe_eudebug_test_init(test)
{
	struct xe_device *xe;

	xe_kunit_helper_xe_device_live_test_init(test);

	xe = test->priv;
	kunit_info(test, "running on graphics version: %u", GRAPHICS_VERx100(xe));
	return 0;
}


> +
> +	check_regs(xe, enable_eudebug);
> +
> +	xe_eudebug_enable(xe, !enable_eudebug);
> +	check_regs(xe, !enable_eudebug);
> +
> +	xe_eudebug_enable(xe, enable_eudebug);
> +	check_regs(xe, enable_eudebug);
> +
> +	return 0;
> +}
> +
> +static void xe_eudebug_toggle_reg_kunit(struct kunit *test)

nit: no need to add 'xe_eudebug' prefix as this test case is
already in 'xe_eudebug' suite

nit: this '_kunit' suffix seems unnecessary
see https://docs.kernel.org/dev-tools/kunit/style.html#test-cases

> +{
> +	struct xe_device *xe = test->priv;
> +
> +	toggle_reg_value(xe);
> +}
> +
> +static struct kunit_case xe_eudebug_tests[] = {
> +	KUNIT_CASE_PARAM(xe_eudebug_toggle_reg_kunit,
> +			 xe_pci_live_device_gen_param),
> +	{}
> +};
> +
> +VISIBLE_IF_KUNIT
> +struct kunit_suite xe_eudebug_test_suite = {
> +	.name = "xe_eudebug",
> +	.test_cases = xe_eudebug_tests,
> +	.init = xe_kunit_helper_xe_device_live_test_init,
> +};
> +EXPORT_SYMBOL_IF_KUNIT(xe_eudebug_test_suite);
> diff --git a/drivers/gpu/drm/xe/tests/xe_live_test_mod.c b/drivers/gpu/drm/xe/tests/xe_live_test_mod.c
> index c55e46f1ae92..dc83bb6a892d 100644
> --- a/drivers/gpu/drm/xe/tests/xe_live_test_mod.c
> +++ b/drivers/gpu/drm/xe/tests/xe_live_test_mod.c
> @@ -19,6 +19,11 @@ kunit_test_suite(xe_migrate_test_suite);
>  kunit_test_suite(xe_mocs_test_suite);
>  kunit_test_suite(xe_guc_g2g_test_suite);
>  
> +#if IS_ENABLED(CONFIG_DRM_XE_EUDEBUG)
> +extern struct kunit_suite xe_eudebug_test_suite;
> +kunit_test_suite(xe_eudebug_test_suite);
> +#endif
> +
>  MODULE_AUTHOR("Intel Corporation");
>  MODULE_LICENSE("GPL");
>  MODULE_DESCRIPTION("xe live kunit tests");
> diff --git a/drivers/gpu/drm/xe/xe_eudebug.c b/drivers/gpu/drm/xe/xe_eudebug.c
> index 2566b55f9c47..f5547c756b31 100644
> --- a/drivers/gpu/drm/xe/xe_eudebug.c
> +++ b/drivers/gpu/drm/xe/xe_eudebug.c
> @@ -2242,3 +2242,7 @@ int xe_eudebug_connect_ioctl(struct drm_device *dev,
>  
>  	return xe_eudebug_connect(xe, file, param);
>  }
> +
> +#if IS_ENABLED(CONFIG_DRM_XE_KUNIT_TEST)
> +#include "tests/xe_eudebug.c"
> +#endif