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

[vitaly prosyak <vprosyak@amd.com>]

Hi Kamil,


On 2023-11-07 04:12, Kamil Konieczny wrote:
> 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@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>
>> ---
>>  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?
You are correct, we may have APU and dGPU in this case the skip-in fixture would be a mistake solution.
We implemented in IGT where we handle more than one GPU case where exported fence to another GPU(amdgpu_hotunplug_with_exported_fence)
The function amdgpu_hotunplug_setup_test would be a good place to check whether it is dGPU or APU.

We have to manually igt_info {.. about skipping the test} vs when having built-in feature in fixture function.

Thanks Vitaly

> 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
>>