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

["Grzegorzek, Dominik" <dominik.grzegorzek@intel.com>]

Hi Dominik!

On Wed, 2024-10-02 at 13:29 +0200, Dominik Karol Piątkowski wrote:
> 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.
> 
> It is set to maximum inside test_gt_render_or_compute. If set before,
> we would lack engine_class info. If set inside the test function, we
> would need to pass original preempt_timeout_us value from there,
> unnecessarily complicating the declaration of test function.
> 
> It is restored in subsequent igt_fixture. This way, a proper cleanup
> can be assured. If restored inside the test function or even
> test_gt_render_or_compute section, any failed assert preceding cleanup
> inside test function would prevent cleanup from happening. We cannot
> allow that.
> 
> In order to underline that the subsequent igt_fixture is related to
> this test, both test_gt_render_or_compute and igt_fixture sections
> were put inside igt_subtest_group, hopefully increasing readability.
> 
> Signed-off-by: Dominik Karol Piątkowski <dominik.karol.piatkowski@intel.com>
> ---
>  tests/intel/xe_eudebug_online.c | 45 +++++++++++++++++++++++++++++++--
>  1 file changed, 43 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/intel/xe_eudebug_online.c b/tests/intel/xe_eudebug_online.c
> index 68d1b786f..84736192f 100644
> --- a/tests/intel/xe_eudebug_online.c
> +++ b/tests/intel/xe_eudebug_online.c
> @@ -16,6 +16,7 @@
>  #include "xe/xe_ioctl.h"
>  #include "xe/xe_query.h"
>  #include "igt.h"
> +#include "igt_sysfs.h"
>  #include "intel_pat.h"
>  #include "intel_mocs.h"
>  #include "gpgpu_shader.h"
> @@ -2201,6 +2202,30 @@ static struct drm_xe_engine_class_instance *pick_compute(int fd, int gt)
>  	return NULL;
>  }
>  
> +static bool set_preempt_timeout_to_max(int fd, uint16_t engine_class, uint32_t *preempt_timeout)
> +{
> +	uint32_t preempt_timeout_max;
> +
> +	if (!xe_sysfs_engine_class_get_property(fd, 0, engine_class, "preempt_timeout_max",
> +						&preempt_timeout_max))
> +		return false;
> +
> +	if (!xe_sysfs_engine_class_set_property(fd, 0, engine_class, "preempt_timeout_us",
> +						preempt_timeout_max, preempt_timeout))
> +		return false;
> +
> +	return true;
Minor nit, you could ommit second if:
	return xe_sysfs_engine_class_set_property(fd, 0, engine_class, "preempt_timeout_us",
+						preempt_timeout_max, preempt_timeout))
> +}
> +
> +static bool restore_preempt_timeout(int fd, uint16_t engine_class, uint32_t preempt_timeout)
> +{
> +	if (!xe_sysfs_engine_class_set_property(fd, 0, engine_class, "preempt_timeout_us",
> +						preempt_timeout, NULL))
> +		return false;
> +
> +	return true;
same as above.
> +}
> +
>  #define test_gt_render_or_compute(t, fd, __hwe) \
>  	igt_subtest_with_dynamic(t) \
>  		for (int gt = 0; (__hwe = pick_compute(fd, gt)); gt++) \
> @@ -2212,6 +2237,8 @@ igt_main
>  	struct drm_xe_engine_class_instance *hwe;
>  	bool was_enabled;
>  	int fd;
> +	uint16_t engine_class = 0xFFFF;
> +	uint32_t preempt_timeout = 0xFFFFFFFF;
>  
>  	igt_fixture {
>  		fd = drm_open_driver(DRIVER_XE);
> @@ -2247,8 +2274,22 @@ 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_interrupt_other(fd, hwe, SHADER_LOOP | DISABLE_DEBUG_MODE);
> +	igt_subtest_group {
> +		test_gt_render_or_compute("interrupt-other", fd, hwe) {
> +			engine_class = hwe->engine_class;
> +
> +			igt_skip_on(!set_preempt_timeout_to_max(fd, engine_class,
> +								&preempt_timeout));
> +
> +			test_interrupt_other(fd, hwe, SHADER_LOOP | DISABLE_DEBUG_MODE);
> +		}
> +
> +		igt_fixture {
> +			if ((uint16_t)~engine_class && ~preempt_timeout)
Maybe I'm not seeing sth but why do we need to cast it to uint16, isn't it already that type?

Other than that it is:
Reviewed-by: Dominik Grzegorzek <dominik.grzegorzek@intel.com>
> +				if (!restore_preempt_timeout(fd, engine_class, preempt_timeout))
> +					igt_warn("Cleanup of preempt_timeout failed!\n");
> +		}
> +	}
>  
>  	test_gt_render_or_compute("interrupt-all-set-breakpoint", fd, hwe)
>  		test_interrupt_all(fd, hwe, SHADER_LOOP | TRIGGER_RESUME_SET_BP);