Re: [PATCH i-g-t 2/3] tests/intel/xe_eudebug: Add basic-vm-bind-drop-debugger-before-ufence-ack test

["Manszewski, Christoph" <christoph.manszewski@intel.com>]

Hi Dominik,

On 9.12.2024 15:13, Dominik Karol Piątkowski wrote:
> Add a test that gives user fence in application, holds it, drops the
> debugger connection and checks if anything breaks. It is expected that
> held acks are released when connection is dropped.
> 
> Signed-off-by: Dominik Karol Piątkowski <dominik.karol.piatkowski@intel.com>
> ---
>   tests/intel/xe_eudebug.c | 50 ++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 50 insertions(+)
> 
> diff --git a/tests/intel/xe_eudebug.c b/tests/intel/xe_eudebug.c
> index 1853dee40..0a4bebf1e 100644
> --- a/tests/intel/xe_eudebug.c
> +++ b/tests/intel/xe_eudebug.c
> @@ -2171,6 +2171,53 @@ static void test_basic_ufence(int fd, unsigned int flags)
>   	ufence_priv_destroy(priv);
>   }
>   
> +/**
> + * SUBTEST: basic-vm-bind-drop-debugger-before-ufence-ack
> + * Description:
> + *	Give user fence in application, hold it, drop the debugger connection and check if anything
> + *	breaks. Expect that held acks are released when connection is dropped.
> + */
> +static void test_ufence_drop_debugger_before_ack(int fd)


I can't help but imagine merging this code with the existing 
'test_basic_ufence'.

> +{
> +	struct xe_eudebug_debugger *d;
> +	struct xe_eudebug_session *s;
> +	struct xe_eudebug_client *c;
> +	struct ufence_priv *priv;
> +
> +	priv = ufence_priv_create();
> +	s = xe_eudebug_session_create(fd, basic_ufence_client, 0, priv);
> +	c = s->client;
> +	d = s->debugger;
> +
> +	xe_eudebug_debugger_add_trigger(d,
> +					DRM_XE_EUDEBUG_EVENT_VM_BIND_UFENCE,
> +					basic_ufence_trigger);
> +
> +	igt_assert_eq(xe_eudebug_debugger_attach(d, c), 0);
> +	xe_eudebug_debugger_start_worker(d);
> +	xe_eudebug_client_start(c);
> +
> +	xe_eudebug_debugger_wait_stage(s, STAGE_CLIENT_WAIT_ON_UFENCE_DONE);
> +	xe_eudebug_assert_f(d, wait_for_ufence_events(priv, XE_EUDEBUG_DEFAULT_TIMEOUT_SEC * MSEC_PER_SEC) == 0,
> +			    "missing ufence events\n");
> +
> +	xe_eudebug_debugger_detach(d);
> +	sleep(1);
> +	igt_assert_eq(xe_eudebug_debugger_attach(d, c), 0);

Basically it's this block vs ack.

> +
> +	xe_eudebug_client_wait_done(c);
> +	xe_eudebug_debugger_stop_worker(d, 1);
> +
> +	xe_eudebug_event_log_print(d->log, true);
> +	xe_eudebug_event_log_print(c->log, true);
> +
> +	xe_eudebug_session_check(s, true, XE_EUDEBUG_FILTER_EVENT_VM_BIND |
> +				 XE_EUDEBUG_FILTER_EVENT_VM | XE_EUDEBUG_FILTER_EVENT_OPEN);

And this. Given the fact that the 'UFENCE' filter in 'test_basic_ufence' 
is redundant, we could just introduce a filter variable, that would be 
set accordingly together with the reconnect block.

> +
> +	xe_eudebug_session_destroy(s);
> +	ufence_priv_destroy(priv);
> +} > +
>   struct vm_bind_clear_thread_priv {
>   	struct drm_xe_engine_class_instance *hwe;
>   	struct xe_eudebug_client *c;
> @@ -2825,6 +2872,9 @@ igt_main
>   	igt_subtest("basic-vm-bind-ufence-delay-ack")
>   		test_basic_ufence(fd, VM_BIND_DELAY_UFENCE_ACK);
>   
> +	igt_subtest("basic-vm-bind-drop-debugger-before-ufence-ack")

No strong opinion but 'basic-vm-bind-ufence-reconnect' would be somewhat 
inline what we have in our online test, nicely allows to select ufence 
tests with a glob and is a little bit shorter. It is less descriptive 
but then it also seems quite logical to expect the reconnect to happen 
at the most interesting moment - which for this test is the ufence blocking.

Other than some code organization it looks good so:

Reviewed-by: Christoph Manszewski <christoph.manszewski@intel.com>

Thanks,
Christoph

> +		test_ufence_drop_debugger_before_ack(fd);
> +
>   	igt_subtest("vma-ufence")
>   		test_vma_ufence(fd, 0);
>