[igt-dev] [PATCH V2 i-g-t] test/amdgpu: add apu check for pciplug test

[Kamil Konieczny]

Hi Jesse,
On 2023-11-07 at 11:37:33 +0800, Jesse Zhang wrote:
> For apu, it is integrated with cpu.
> So hotplug test should be unnecessary for it.
> 
> V2:
>  -IGT built-in approach to check this condition
>    in the fixture function. (Vitlay)
> 
> 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>
> ---
>  tests/amdgpu/amd_pci_unplug.c | 32 +++++++++++++++++++++++++++++++-
>  1 file changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/amdgpu/amd_pci_unplug.c b/tests/amdgpu/amd_pci_unplug.c
> index 4c055b99f..22e053795 100644
> --- a/tests/amdgpu/amd_pci_unplug.c
> +++ b/tests/amdgpu/amd_pci_unplug.c
> @@ -30,15 +30,42 @@
>  #include "lib/amdgpu/amd_pci_unplug.h"
>  #include "lib/amdgpu/amd_ip_blocks.h"
>  
> +static bool
> +is_hot_plug_test_enable(amdgpu_device_handle device_handle, const struct amdgpu_gpu_info *gpu_info)
---^^^^^^^^^^^^^^^^^^^
Better: is_hotplug_supported

> +{
> +	bool enable = true;
> +
> +		/* skip hotplug test on APUs */
---------- ^
You are not skipping anything in this function.

> +	if (gpu_info->ids_flags & AMDGPU_IDS_FLAGS_FUSION) {
> +		igt_info("Don't support hot plug on APUs, hot plug test is disabled\n");
----------------- ^^^^^^^^^^^^^
Better: Hotplug not possible for APU.

Btw what is APU? I ask because that is another shortcut.

> +		enable = false;
> +	}
> +	return enable;
> +}
>  
>  igt_main
>  {
>  
>  	struct amd_pci_unplug_setup setup = {0};
>  	struct amd_pci_unplug unplug = {0};
> +	amdgpu_device_handle device;
> +	struct amdgpu_gpu_info gpu_info = {};
> +	int fd = -1;
>  
>  	igt_fixture {
> +		uint32_t major, minor;
> +		int err;
> +
> +		fd = drm_open_driver(DRIVER_AMDGPU);
> +		err = amdgpu_device_initialize(fd, &major, &minor, &device);
> +		igt_require(err == 0);
> +		igt_info("Initialized amdgpu, driver version %d.%d\n",
> +			 major, minor);
> +		err = amdgpu_query_gpu_info(device, &gpu_info);
> +		igt_assert_eq(err, 0);
> +
>  		setup.minor_version_req = 46;
> +		igt_skip_on(!is_hot_plug_test_enable(device, &gpu_info));

Problem with such skip in fixture is that it is global,
it will aplly to all cases even when you will not have APU
or have both APU and dGPU connected by PCI.
Imho it is better to place this in particular subtests.

Btw when you have APU you still would want to test hotunplug
for dGPU connected by PCI? Or am I missing something?
Could you have APU on separate PCI card?

Regards,
Kamil

>  	}
>  
>  	igt_subtest("amdgpu_hotunplug_simple")
> @@ -54,5 +81,8 @@ igt_main
>  	igt_subtest("amdgpu_hotunplug_with_exported_fence")
>  		amdgpu_hotunplug_with_exported_fence(&setup, &unplug);
>  
> -	igt_fixture { }
> +	igt_fixture {
> +		amdgpu_device_deinitialize(device);
> +		drm_close_driver(fd);
> +	}
>  }
> -- 
> 2.25.1
>