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

[Piatkowski, Dominik Karol]

Hi Dominik,

> -----Original Message-----
> From: Grzegorzek, Dominik <dominik.grzegorzek at intel.com>
> Sent: Thursday, October 3, 2024 3:49 PM
> To: Piatkowski, Dominik Karol <dominik.karol.piatkowski at intel.com>; igt-
> dev at lists.freedesktop.org
> Cc: Kempczynski, Zbigniew <zbigniew.kempczynski at intel.com>; Sikora, Pawel
> <pawel.sikora at intel.com>; Manszewski, Christoph
> <christoph.manszewski at intel.com>; Cavitt, Jonathan
> <jonathan.cavitt at intel.com>
> Subject: Re: [PATCH v2 i-g-t 2/2] tests/xe_eudebug_online: Adjust interrupt-
> other test
> 
> 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 at 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))

Nice catch!

> > +}
> > +
> > +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.

Thanks!

> > +}
> > +
> >  #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?

Considering the case of engine_class = 0xFFFF, when we do:

if (~engine_class) ...

it evaluates to:

if (0xFFFF0000) ...

hence the cast :)

Thanks,
Dominik Karol

> 
> Other than that it is:
> Reviewed-by: Dominik Grzegorzek <dominik.grzegorzek at 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);