[PATCH i-g-t 2/2] tests/xe_eudebug_online: Adjust interrupt-other test

[Cavitt, Jonathan]

-----Original Message-----
From: igt-dev <igt-dev-bounces at lists.freedesktop.org> On Behalf Of Dominik Karol Piatkowski
Sent: Thursday, September 26, 2024 5:19 AM
To: igt-dev at lists.freedesktop.org
Cc: Kempczynski, Zbigniew <zbigniew.kempczynski at intel.com>; Grzegorzek, Dominik <dominik.grzegorzek at intel.com>; Manszewski, Christoph <christoph.manszewski at intel.com>; Sikora, Pawel <pawel.sikora at intel.com>; Piatkowski, Dominik Karol <dominik.karol.piatkowski at intel.com>
Subject: [PATCH i-g-t 2/2] tests/xe_eudebug_online: Adjust interrupt-other test
> 
> This test fails due to preemption interfering with it.
> 
> In order to fix it, set the preempt_timeout_us for engine_class used in
> this test to maximum for the duration of the test.
> 
> Signed-off-by: Dominik Karol Piątkowski <dominik.karol.piatkowski at intel.com>
> ---
>  tests/intel/xe_eudebug_online.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/intel/xe_eudebug_online.c b/tests/intel/xe_eudebug_online.c
> index 68d1b786f..d984eaf99 100644
> --- a/tests/intel/xe_eudebug_online.c
> +++ b/tests/intel/xe_eudebug_online.c
> @@ -2212,6 +2212,8 @@ igt_main
>  	struct drm_xe_engine_class_instance *hwe;
>  	bool was_enabled;
>  	int fd;
> +	uint32_t preempt_timeout;
> +	uint16_t engine_class = 0xFFFF;
>  
>  	igt_fixture {
>  		fd = drm_open_driver(DRIVER_XE);
> @@ -2247,8 +2249,17 @@ igt_main
>  	test_gt_render_or_compute("interrupt-other-debuggable", fd, hwe)
>  		test_interrupt_other(fd, hwe, SHADER_LOOP);
>  
> -	test_gt_render_or_compute("interrupt-other", fd, hwe)
> +	test_gt_render_or_compute("interrupt-other", fd, hwe) {
> +		engine_class = hwe->engine_class;
> +		igt_skip_on(!xe_eudebug_set_preempt_timeout_to_max(fd, engine_class,
> +								   &preempt_timeout));
>  		test_interrupt_other(fd, hwe, SHADER_LOOP | DISABLE_DEBUG_MODE);
> +	}
> +
> +	igt_fixture
> +		if (engine_class != 0xFFFF &&
> +		    !xe_eudebug_restore_preempt_timeout(fd, engine_class, preempt_timeout))
> +			igt_warn("Cleanup of preempt_timeout failed!\n");

IMO I think it would be better to put this in as a part of the test_interrupt_other code:

"""
static void test_interrupt_other(int fd, struct drm_xe_engine_class_instance *hwe, int flags)
{
	struct online_debug_data *data;
	struct online_debug_data *debugee_data;
	struct xe_eudebug_session *s;
	struct xe_eudebug_client *debugee;
	int debugee_flags = SHADER_LOOP | DO_NOT_EXPECT_CANARIES;
	uint32_t preempt_timeout;
	int val;

	if (flags & DISABLE_DEBUG_MODE)
		igt_skip_on(!xe_eudebug_set_preempt_timeout_to_max(fd, hwe->engine_class,
									   &preempt_timeout));
...
	xe_eudebug_client_destroy(debugee);
	xe_eudebug_session_destroy(s);
	online_debug_data_destroy(data);
	online_debug_data_destroy(debugee_data);
	if (flags & DISABLE_DEBUG_MODE)
		igt_warn_on_f(!xe_eudebug_restore_preempt_timeout(fd, hwe->engine_class,
									preempt_timeout),
				"Cleanup of preempt_timeout failed!\n");
}
"""

Also, we should probably check to make sure that we don't need this for interrupt-other-debuggable as well.
If we do need it there additionally, the if-statements are unnecessary guards.
-Jonathan Cavitt

>  
>  	test_gt_render_or_compute("interrupt-all-set-breakpoint", fd, hwe)
>  		test_interrupt_all(fd, hwe, SHADER_LOOP | TRIGGER_RESUME_SET_BP);
> -- 
> 2.34.1
> 
>