Re: [PATCH v2 i-g-t 2/3] tests/intel/xe_eudebug: Add basic-vm-bind-ufence-reconnect test

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

Hi Dominik,

On 11.12.2024 14:50, 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>
> Reviewed-by: Christoph Manszewski <christoph.manszewski@intel.com>
> ---
> v2:
> - Rename basic-vm-bind-drop-debugger-before-ufence-ack to
>    basic-vm-bind-ufence-reconnect and merge it into test_basic_ufence
> ---
>   tests/intel/xe_eudebug.c | 21 +++++++++++++++++++--
>   1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/intel/xe_eudebug.c b/tests/intel/xe_eudebug.c
> index 1853dee40..77ce905f0 100644
> --- a/tests/intel/xe_eudebug.c
> +++ b/tests/intel/xe_eudebug.c
> @@ -64,6 +64,7 @@ static void test_sysfs_toggle(int fd)
>   #define VM_BIND_OP_MAP_USERPTR		(1 << 7)
>   #define EXEC_QUEUES_PLACEMENTS		(1 << 8)
>   #define VM_BIND_DELAY_UFENCE_ACK	(1 << 9)
> +#define VM_BIND_UFENCE_RECONNECT	(1 << 10)
>   #define TEST_DISCOVERY			(1 << 31)
>   
>   #define PAGE_SIZE SZ_4K
> @@ -2129,6 +2130,11 @@ static int wait_for_ufence_events(struct ufence_priv *priv, int timeout_ms)
>    * SUBTEST: basic-vm-bind-ufence-delay-ack
>    * Description:
>    *	Give user fence in application and check if delayed ufence ack works
> + *
> + * SUBTEST: basic-vm-bind-ufence-reconnect
> + * 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_basic_ufence(int fd, unsigned int flags)
>   {
> @@ -2136,6 +2142,7 @@ static void test_basic_ufence(int fd, unsigned int flags)
>   	struct xe_eudebug_session *s;
>   	struct xe_eudebug_client *c;
>   	struct ufence_priv *priv;
> +	uint32_t filter = XE_EUDEBUG_FILTER_EVENT_VM_BIND_UFENCE;
>   
>   	priv = ufence_priv_create();
>   	s = xe_eudebug_session_create(fd, basic_ufence_client, flags, priv);
> @@ -2157,7 +2164,14 @@ static void test_basic_ufence(int fd, unsigned int flags)
>   	if (flags & VM_BIND_DELAY_UFENCE_ACK)
>   		sleep(XE_EUDEBUG_DEFAULT_TIMEOUT_SEC * 4 / 5);
>   
> -	ack_fences(d);
> +	if (flags & VM_BIND_UFENCE_RECONNECT) {
> +		filter = XE_EUDEBUG_FILTER_EVENT_VM_BIND | XE_EUDEBUG_FILTER_EVENT_VM |
> +				XE_EUDEBUG_FILTER_EVENT_OPEN;
> +		xe_eudebug_debugger_detach(d);
> +		sleep(1);

Sorry for missing that in the first round but what do we wait for here 
exactly? To me it looks a little like we try to wait for the client to 
finish, so we don't get a replay in the event log and don't call our 
trigger again. That is properly handled in interrupt-reconnect I think. 
It requires the client wait for a stage, cleaning the debugger log and 
removing the trigger.

But I don't think it is worth the hassle and your approach works fine. 
Just please use 'xe_eudebug_client_wait_done' instead of a timeout if 
this what you are actually trying to achieve here (unless you don't, 
then please just add some short comment/explanation in the code).

> +		igt_assert_eq(xe_eudebug_debugger_attach(d, c), 0);
> +	} else
> +		ack_fences(d);

Nit: missing braces.

Thanks,
Christoph
>   
>   	xe_eudebug_client_wait_done(c);
>   	xe_eudebug_debugger_stop_worker(d, 1);
> @@ -2165,7 +2179,7 @@ static void test_basic_ufence(int fd, unsigned int flags)
>   	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_UFENCE);
> +	xe_eudebug_session_check(s, true, filter);
>   
>   	xe_eudebug_session_destroy(s);
>   	ufence_priv_destroy(priv);
> @@ -2825,6 +2839,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-ufence-reconnect")
> +		test_basic_ufence(fd, VM_BIND_UFENCE_RECONNECT);
> +
>   	igt_subtest("vma-ufence")
>   		test_vma_ufence(fd, 0);
>