[igt-dev] [PATCH i-g-t 3/3] lib/amdgpu:fix the error when skip hotplug test

[igt-dev] [PATCH i-g-t 3/3] lib/amdgpu:fix the error when skip hotplug test

[Jesse Zhang]

For skipped cases, we should print "SKIP"
rather than "failure." For example

IGT-Version: 1.28-g4b3e1dc37 (x86_64) (Linux: 6.3.7+ x86_64)
Starting subtest: amdgpu_hotunplug_simple
SKIP ... more than 1 GPU is required for this test
(amd_pci_unplug:70496) amdgpu/amd_pci_unplug-CRITICAL: Test assertion failure function amdgpu_hotunplug_test, file ../lib/amdgpu/amd_pci_unplug.c:305:
(amd_pci_unplug:70496) amdgpu/amd_pci_unplug-CRITICAL: Failed assertion: r == 1
(amd_pci_unplug:70496) amdgpu/amd_pci_unplug-CRITICAL: Last errno: 22, Invalid argument
(amd_pci_unplug:70496) amdgpu/amd_pci_unplug-CRITICAL: error: 0 != 1
Stack trace:
  #0 ../lib/igt_core.c:1988 __igt_fail_assert()
  #1 ../lib/amdgpu/amd_pci_unplug.c:305 amdgpu_hotunplug_test.constprop.0.isra.0()
  #2 [amdgpu_hotunplug_simple+0x29]
  #3 ../tests/amdgpu/amd_pci_unplug.c:44 __igt_unique____real_main34()
  #4 ../tests/amdgpu/amd_pci_unplug.c:34 main()
  #5 ../sysdeps/nptl/libc_start_call_main.h:58 __libc_start_call_main()
  #6 ../csu/libc-start.c:128 __libc_start_main@@GLIBC_2.34()
  #7 [_start+0x25]
Subtest amdgpu_hotunplug_simple failed.

V2:
  - replace igt_skip_on with igt_skip_f(Kamil)

Cc: Vitaly Prosyak <vitaly.prosyak@amd.com>
Cc: Luben Tuikov <luben.tuikov@amd.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Christian Koenig <christian.koenig@amd.com>
Cc: Tim Huang <tim.huang@amd.com>
Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>

Signed-off-by: Jesse Zhang <Jesse.Zhang@amd.com>
Reviewed-by: Vitaly Prosyak <vitaly.prosyak@amd.com>
---
 lib/amdgpu/amd_pci_unplug.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/lib/amdgpu/amd_pci_unplug.c b/lib/amdgpu/amd_pci_unplug.c
index b7af95dec..8346aeb98 100644
--- a/lib/amdgpu/amd_pci_unplug.c
+++ b/lib/amdgpu/amd_pci_unplug.c
@@ -101,8 +101,7 @@ amdgpu_hotunplug_setup_test(bool render_mode, const struct amd_pci_unplug_setup
 
 	if (setup->open_device && setup->open_device2 && unplug->num_devices < 2) {
 		/*Not enough board for the test*/
-		printf("SKIP ... more than 1 GPU is required for this test\n");
-		goto release;
+		igt_skip_on_f(ret, "SKIP ... more than 1 GPU is required for this test\n");
 	}
 
 	tmp_str = amdgpu_get_device_from_fd(unplug->drm_amdgpu_fds[0]);
@@ -117,7 +116,9 @@ amdgpu_hotunplug_setup_test(bool render_mode, const struct amd_pci_unplug_setup
 		goto release;
 
 	if (minor_version < setup->minor_version_req)
-		goto release;
+		igt_skip_on_f(minor_version < setup->minor_version_req,
+			"It has minor_version: %d but it is required: %d for this test\n",
+			minor_version, setup->minor_version_req);
 
 	/* skip hotplug test on APU (Accelerated Processing Unit CPU + iGPU) */
 	r = amdgpu_query_gpu_info(unplug->device_handle, &gpu_info);
@@ -138,7 +139,9 @@ amdgpu_hotunplug_setup_test(bool render_mode, const struct amd_pci_unplug_setup
 		if (r != 0)
 			goto release;
 		if (minor_version < setup->minor_version_req)
-			goto release;
+			igt_skip_on_f(minor_version < setup->minor_version_req,
+				"It has minor_version: %d but it is required: %d for this test\n",
+				minor_version, setup->minor_version_req);
 
 		r = amdgpu_query_gpu_info(unplug->device_handle2, &gpu_info);
 		igt_assert_eq(r, 0);
-- 
2.25.1

Re: [igt-dev] [PATCH i-g-t 3/3] lib/amdgpu:fix the error when skip hotplug test

[Kamil Konieczny]

Hi Jesse,
On 2023-11-09 at 10:42:29 +0800, Jesse Zhang wrote:
> For skipped cases, we should print "SKIP"
> rather than "failure." For example
> 
> IGT-Version: 1.28-g4b3e1dc37 (x86_64) (Linux: 6.3.7+ x86_64)
> Starting subtest: amdgpu_hotunplug_simple
> SKIP ... more than 1 GPU is required for this test
> (amd_pci_unplug:70496) amdgpu/amd_pci_unplug-CRITICAL: Test assertion failure function amdgpu_hotunplug_test, file ../lib/amdgpu/amd_pci_unplug.c:305:
> (amd_pci_unplug:70496) amdgpu/amd_pci_unplug-CRITICAL: Failed assertion: r == 1
> (amd_pci_unplug:70496) amdgpu/amd_pci_unplug-CRITICAL: Last errno: 22, Invalid argument
> (amd_pci_unplug:70496) amdgpu/amd_pci_unplug-CRITICAL: error: 0 != 1
> Stack trace:
>   #0 ../lib/igt_core.c:1988 __igt_fail_assert()
>   #1 ../lib/amdgpu/amd_pci_unplug.c:305 amdgpu_hotunplug_test.constprop.0.isra.0()
>   #2 [amdgpu_hotunplug_simple+0x29]
>   #3 ../tests/amdgpu/amd_pci_unplug.c:44 __igt_unique____real_main34()
>   #4 ../tests/amdgpu/amd_pci_unplug.c:34 main()
>   #5 ../sysdeps/nptl/libc_start_call_main.h:58 __libc_start_call_main()
>   #6 ../csu/libc-start.c:128 __libc_start_main@@GLIBC_2.34()
>   #7 [_start+0x25]
> Subtest amdgpu_hotunplug_simple failed.
> 
> V2:
>   - replace igt_skip_on with igt_skip_f(Kamil)
> 
> Cc: Vitaly Prosyak <vitaly.prosyak@amd.com>
> Cc: Luben Tuikov <luben.tuikov@amd.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Christian Koenig <christian.koenig@amd.com>
> Cc: Tim Huang <tim.huang@amd.com>
> Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
> 
> Signed-off-by: Jesse Zhang <Jesse.Zhang@amd.com>
> Reviewed-by: Vitaly Prosyak <vitaly.prosyak@amd.com>
> ---
>  lib/amdgpu/amd_pci_unplug.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/amdgpu/amd_pci_unplug.c b/lib/amdgpu/amd_pci_unplug.c
> index b7af95dec..8346aeb98 100644
> --- a/lib/amdgpu/amd_pci_unplug.c
> +++ b/lib/amdgpu/amd_pci_unplug.c
> @@ -101,8 +101,7 @@ amdgpu_hotunplug_setup_test(bool render_mode, const struct amd_pci_unplug_setup
>  
>  	if (setup->open_device && setup->open_device2 && unplug->num_devices < 2) {
>  		/*Not enough board for the test*/
> -		printf("SKIP ... more than 1 GPU is required for this test\n");
> -		goto release;
> +		igt_skip_on_f(ret, "SKIP ... more than 1 GPU is required for this test\n");
--------------------------- ^^^^^^^^^
No need for repeating SKIP here.

With this fixed:

Reviewed-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>

>  	}
>  
>  	tmp_str = amdgpu_get_device_from_fd(unplug->drm_amdgpu_fds[0]);
> @@ -117,7 +116,9 @@ amdgpu_hotunplug_setup_test(bool render_mode, const struct amd_pci_unplug_setup
>  		goto release;
>  
>  	if (minor_version < setup->minor_version_req)
> -		goto release;
> +		igt_skip_on_f(minor_version < setup->minor_version_req,
> +			"It has minor_version: %d but it is required: %d for this test\n",
> +			minor_version, setup->minor_version_req);
>  
>  	/* skip hotplug test on APU (Accelerated Processing Unit CPU + iGPU) */
>  	r = amdgpu_query_gpu_info(unplug->device_handle, &gpu_info);
> @@ -138,7 +139,9 @@ amdgpu_hotunplug_setup_test(bool render_mode, const struct amd_pci_unplug_setup
>  		if (r != 0)
>  			goto release;
>  		if (minor_version < setup->minor_version_req)
> -			goto release;
> +			igt_skip_on_f(minor_version < setup->minor_version_req,
> +				"It has minor_version: %d but it is required: %d for this test\n",
> +				minor_version, setup->minor_version_req);
>  
>  		r = amdgpu_query_gpu_info(unplug->device_handle2, &gpu_info);
>  		igt_assert_eq(r, 0);
> -- 
> 2.25.1
> 

Re: [igt-dev] [PATCH i-g-t 3/3] lib/amdgpu:fix the error when skip hotplug test

[Kamil Konieczny]

Hi Jesse,

one small nit about subject, put space after ':',

[PATCH i-g-t 3/3] lib/amdgpu:fix the error when skip hotplug test
----------------------------^^
s/amdgpu:fix/amdgpu: fix/

should be:
[PATCH i-g-t 3/3] lib/amdgpu: fix the error when skip hotplug test

On 2023-11-09 at 10:42:29 +0800, Jesse Zhang wrote:
> For skipped cases, we should print "SKIP"
> rather than "failure." For example
> 
> IGT-Version: 1.28-g4b3e1dc37 (x86_64) (Linux: 6.3.7+ x86_64)
> Starting subtest: amdgpu_hotunplug_simple
> SKIP ... more than 1 GPU is required for this test
> (amd_pci_unplug:70496) amdgpu/amd_pci_unplug-CRITICAL: Test assertion failure function amdgpu_hotunplug_test, file ../lib/amdgpu/amd_pci_unplug.c:305:
> (amd_pci_unplug:70496) amdgpu/amd_pci_unplug-CRITICAL: Failed assertion: r == 1
> (amd_pci_unplug:70496) amdgpu/amd_pci_unplug-CRITICAL: Last errno: 22, Invalid argument
> (amd_pci_unplug:70496) amdgpu/amd_pci_unplug-CRITICAL: error: 0 != 1
> Stack trace:
>   #0 ../lib/igt_core.c:1988 __igt_fail_assert()
>   #1 ../lib/amdgpu/amd_pci_unplug.c:305 amdgpu_hotunplug_test.constprop.0.isra.0()
--------- ^^^ -------------------------------------------- ^^^^

Did you write test inside library? Is it called from other tests?

Regards,
Kamil

>   #2 [amdgpu_hotunplug_simple+0x29]
>   #3 ../tests/amdgpu/amd_pci_unplug.c:44 __igt_unique____real_main34()
>   #4 ../tests/amdgpu/amd_pci_unplug.c:34 main()
>   #5 ../sysdeps/nptl/libc_start_call_main.h:58 __libc_start_call_main()
>   #6 ../csu/libc-start.c:128 __libc_start_main@@GLIBC_2.34()
>   #7 [_start+0x25]
> Subtest amdgpu_hotunplug_simple failed.
> 
> V2:
>   - replace igt_skip_on with igt_skip_f(Kamil)
> 
> Cc: Vitaly Prosyak <vitaly.prosyak@amd.com>
> Cc: Luben Tuikov <luben.tuikov@amd.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Christian Koenig <christian.koenig@amd.com>
> Cc: Tim Huang <tim.huang@amd.com>
> Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
> 
> Signed-off-by: Jesse Zhang <Jesse.Zhang@amd.com>
> Reviewed-by: Vitaly Prosyak <vitaly.prosyak@amd.com>
> ---
>  lib/amdgpu/amd_pci_unplug.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/amdgpu/amd_pci_unplug.c b/lib/amdgpu/amd_pci_unplug.c
> index b7af95dec..8346aeb98 100644
> --- a/lib/amdgpu/amd_pci_unplug.c
> +++ b/lib/amdgpu/amd_pci_unplug.c
> @@ -101,8 +101,7 @@ amdgpu_hotunplug_setup_test(bool render_mode, const struct amd_pci_unplug_setup
>  
>  	if (setup->open_device && setup->open_device2 && unplug->num_devices < 2) {
>  		/*Not enough board for the test*/
> -		printf("SKIP ... more than 1 GPU is required for this test\n");
> -		goto release;
> +		igt_skip_on_f(ret, "SKIP ... more than 1 GPU is required for this test\n");
>  	}
>  
>  	tmp_str = amdgpu_get_device_from_fd(unplug->drm_amdgpu_fds[0]);
> @@ -117,7 +116,9 @@ amdgpu_hotunplug_setup_test(bool render_mode, const struct amd_pci_unplug_setup
>  		goto release;
>  
>  	if (minor_version < setup->minor_version_req)
> -		goto release;
> +		igt_skip_on_f(minor_version < setup->minor_version_req,
> +			"It has minor_version: %d but it is required: %d for this test\n",
> +			minor_version, setup->minor_version_req);
>  
>  	/* skip hotplug test on APU (Accelerated Processing Unit CPU + iGPU) */
>  	r = amdgpu_query_gpu_info(unplug->device_handle, &gpu_info);
> @@ -138,7 +139,9 @@ amdgpu_hotunplug_setup_test(bool render_mode, const struct amd_pci_unplug_setup
>  		if (r != 0)
>  			goto release;
>  		if (minor_version < setup->minor_version_req)
> -			goto release;
> +			igt_skip_on_f(minor_version < setup->minor_version_req,
> +				"It has minor_version: %d but it is required: %d for this test\n",
> +				minor_version, setup->minor_version_req);
>  
>  		r = amdgpu_query_gpu_info(unplug->device_handle2, &gpu_info);
>  		igt_assert_eq(r, 0);
> -- 
> 2.25.1
> 

Re: [igt-dev] [PATCH i-g-t 3/3] lib/amdgpu:fix the error when skip hotplug test

[Zhang, Jesse(Jie)]

[AMD Official Use Only - General]

HI Kamil,
  Sorry for missing this email.
-----Original Message-----
From: Kamil Konieczny <kamil.konieczny@linux.intel.com>
Sent: Friday, November 10, 2023 12:18 AM
To: igt-dev@lists.freedesktop.org
Cc: Zhang, Jesse(Jie) <Jesse.Zhang@amd.com>; Prosyak, Vitaly <Vitaly.Prosyak@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>; Tuikov, Luben <Luben.Tuikov@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>; Huang, Tim <Tim.Huang@amd.com>
Subject: Re: [PATCH i-g-t 3/3] lib/amdgpu:fix the error when skip hotplug test

Hi Jesse,

one small nit about subject, put space after ':',

[PATCH i-g-t 3/3] lib/amdgpu:fix the error when skip hotplug test ----------------------------^^
s/amdgpu:fix/amdgpu: fix/

should be:
[PATCH i-g-t 3/3] lib/amdgpu: fix the error when skip hotplug test
        [Zhang, Jesse(Jie)]   yes, thanks for this comment.
On 2023-11-09 at 10:42:29 +0800, Jesse Zhang wrote:
> For skipped cases, we should print "SKIP"
> rather than "failure." For example
>
> IGT-Version: 1.28-g4b3e1dc37 (x86_64) (Linux: 6.3.7+ x86_64) Starting
> subtest: amdgpu_hotunplug_simple SKIP ... more than 1 GPU is required
> for this test
> (amd_pci_unplug:70496) amdgpu/amd_pci_unplug-CRITICAL: Test assertion failure function amdgpu_hotunplug_test, file ../lib/amdgpu/amd_pci_unplug.c:305:
> (amd_pci_unplug:70496) amdgpu/amd_pci_unplug-CRITICAL: Failed
> assertion: r == 1
> (amd_pci_unplug:70496) amdgpu/amd_pci_unplug-CRITICAL: Last errno: 22,
> Invalid argument
> (amd_pci_unplug:70496) amdgpu/amd_pci_unplug-CRITICAL: error: 0 != 1
> Stack trace:
>   #0 ../lib/igt_core.c:1988 __igt_fail_assert()
>   #1 ../lib/amdgpu/amd_pci_unplug.c:305
> amdgpu_hotunplug_test.constprop.0.isra.0()
--------- ^^^ -------------------------------------------- ^^^^

Did you write test inside library? Is it called from other tests?
[Zhang, Jesse(Jie)] hi Kamil, I just wrote some negative tests inside library.
For example, version mismatch. It should print "SKIP" for this test, and it should not fail.
Regards
Jesse

Regards,
Kamil

>   #2 [amdgpu_hotunplug_simple+0x29]
>   #3 ../tests/amdgpu/amd_pci_unplug.c:44 __igt_unique____real_main34()
>   #4 ../tests/amdgpu/amd_pci_unplug.c:34 main()
>   #5 ../sysdeps/nptl/libc_start_call_main.h:58 __libc_start_call_main()
>   #6 ../csu/libc-start.c:128 __libc_start_main@@GLIBC_2.34()
>   #7 [_start+0x25]
> Subtest amdgpu_hotunplug_simple failed.
>
> V2:
>   - replace igt_skip_on with igt_skip_f(Kamil)
>
> Cc: Vitaly Prosyak <vitaly.prosyak@amd.com>
> Cc: Luben Tuikov <luben.tuikov@amd.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Christian Koenig <christian.koenig@amd.com>
> Cc: Tim Huang <tim.huang@amd.com>
> Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
>
> Signed-off-by: Jesse Zhang <Jesse.Zhang@amd.com>
> Reviewed-by: Vitaly Prosyak <vitaly.prosyak@amd.com>
> ---
>  lib/amdgpu/amd_pci_unplug.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/lib/amdgpu/amd_pci_unplug.c b/lib/amdgpu/amd_pci_unplug.c
> index b7af95dec..8346aeb98 100644
> --- a/lib/amdgpu/amd_pci_unplug.c
> +++ b/lib/amdgpu/amd_pci_unplug.c
> @@ -101,8 +101,7 @@ amdgpu_hotunplug_setup_test(bool render_mode,
> const struct amd_pci_unplug_setup
>
>       if (setup->open_device && setup->open_device2 && unplug->num_devices < 2) {
>               /*Not enough board for the test*/
> -             printf("SKIP ... more than 1 GPU is required for this test\n");
> -             goto release;
> +             igt_skip_on_f(ret, "SKIP ... more than 1 GPU is required for this
> +test\n");
>       }
>
>       tmp_str = amdgpu_get_device_from_fd(unplug->drm_amdgpu_fds[0]);
> @@ -117,7 +116,9 @@ amdgpu_hotunplug_setup_test(bool render_mode, const struct amd_pci_unplug_setup
>               goto release;
>
>       if (minor_version < setup->minor_version_req)
> -             goto release;
> +             igt_skip_on_f(minor_version < setup->minor_version_req,
> +                     "It has minor_version: %d but it is required: %d for this test\n",
> +                     minor_version, setup->minor_version_req);
>
>       /* skip hotplug test on APU (Accelerated Processing Unit CPU + iGPU) */
>       r = amdgpu_query_gpu_info(unplug->device_handle, &gpu_info); @@
> -138,7 +139,9 @@ amdgpu_hotunplug_setup_test(bool render_mode, const struct amd_pci_unplug_setup
>               if (r != 0)
>                       goto release;
>               if (minor_version < setup->minor_version_req)
> -                     goto release;
> +                     igt_skip_on_f(minor_version < setup->minor_version_req,
> +                             "It has minor_version: %d but it is required: %d for this test\n",
> +                             minor_version, setup->minor_version_req);
>
>               r = amdgpu_query_gpu_info(unplug->device_handle2, &gpu_info);
>               igt_assert_eq(r, 0);
> --
> 2.25.1
>