[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 at linux.intel.com>
Sent: Friday, November 10, 2023 12:18 AM
To: igt-dev at lists.freedesktop.org
Cc: Zhang, Jesse(Jie) <Jesse.Zhang at amd.com>; Prosyak, Vitaly <Vitaly.Prosyak at amd.com>; Deucher, Alexander <Alexander.Deucher at amd.com>; Tuikov, Luben <Luben.Tuikov at amd.com>; Koenig, Christian <Christian.Koenig at amd.com>; Huang, Tim <Tim.Huang at 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 at amd.com>
> Cc: Luben Tuikov <luben.tuikov at amd.com>
> Cc: Alex Deucher <alexander.deucher at amd.com>
> Cc: Christian Koenig <christian.koenig at amd.com>
> Cc: Tim Huang <tim.huang at amd.com>
> Cc: Kamil Konieczny <kamil.konieczny at linux.intel.com>
>
> Signed-off-by: Jesse Zhang <Jesse.Zhang at amd.com>
> Reviewed-by: Vitaly Prosyak <vitaly.prosyak at 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
>