[igt-dev] [i-g-t 05/20] tests/amdgpu: Close the fd before exit

[Modem, Bhanuprakash]

On Thu-15-06-2023 12:43 pm, Christian König wrote:
> Hi guys,
> 
> Am 15.06.23 um 06:46 schrieb Modem, Bhanuprakash:
>> Hi Vitaliy,
>>
>> On Thu-15-06-2023 05:44 am, vitaly prosyak wrote:
>>> Hi Kamil,
>>>
>>> Thanks for letting us know about new(?) function
>>> /drm_close_driver/.
>>>
>>> Is there a pending change, I could not find this function in the DRM 
>>> or IGT.
>>
>> Yes, changes are not yet landed into the IGT (WIP).
>>
>> Please refer Patch 1/20 in this series:
>> https://patchwork.freedesktop.org/patch/538070/?series=117263&rev=5
> 
> well neither this patch here nor the referenced series is correct.
> 
> The libdrm functions take ownership of the file descriptor given to them!
> 
> So the application or in this case IGT is *not* supposed to close them 
> at all.

Unfortunately, that is not correct.

The application (IGT in this case), opens the driver and uses the libdrm 
functions to perform operations (like ioctl), but file descriptor is 
still with the IGT only.

Once it is done with the operations performed, it's IGT responsibility 
to close the driver (I think there are few gcc options available to 
close the device on IGT exit)

A typical IGT sudo-code:

	fd = open(/dev/device);

	libdrm_call(fd);
	libdrm_call(fd);

	close(fd);

- Bhanu

> 
> Regards,
> Christian.
> 
>>
>> - Bhanu
>>
>>>
>>> We open the driver with /drm_open_driver/, but then we use close vs 
>>> /drm_close_driver/. I makes sense to have a symmetrical call:
>>> /open/close/ and /drm_open_driver/drm_close_driver/
>>>
>>>
>>> Thanks, Vitaly
>>>
>>> On 2023-06-14 08:39, Kamil Konieczny wrote:
>>>> Hi Bhanuprakash,
>>>>
>>>> +cc Vitaliy and Christian
>>>>
>>>> Cc: Vitaly Prosyak<vitaly.prosyak at amd.com>
>>>> Cc: Christian König<christian.koenig at amd.com>
>>>>
>>>> Regards,
>>>> Kamil
>>>>
>>>> On 2023-05-16 at 22:20:43 +0530, Bhanuprakash Modem wrote:
>>>>> Close the file descriptor before exiting the test.
>>>>>
>>>>> Cc: David Zhang<dingchen.zhang at amd.com>
>>>>> Cc: Harry Wentland<harry.wentland at amd.com>
>>>>> Signed-off-by: Bhanuprakash Modem<bhanuprakash.modem at intel.com>
>>>>> ---
>>>>>   tests/amdgpu/amd_abm.c                 | 1 +
>>>>>   tests/amdgpu/amd_assr.c                | 1 +
>>>>>   tests/amdgpu/amd_freesync_video_mode.c | 1 +
>>>>>   tests/amdgpu/amd_mem_leak.c            | 1 +
>>>>>   tests/amdgpu/amd_psr.c                 | 3 ++-
>>>>>   tests/amdgpu/amd_vrr_range.c           | 1 +
>>>>>   6 files changed, 7 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/tests/amdgpu/amd_abm.c b/tests/amdgpu/amd_abm.c
>>>>> index 9400ed806..082da7ed6 100644
>>>>> --- a/tests/amdgpu/amd_abm.c
>>>>> +++ b/tests/amdgpu/amd_abm.c
>>>>> @@ -378,5 +378,6 @@ igt_main
>>>>>         igt_fixture {
>>>>>           igt_display_fini(&data.display);
>>>>> +        drm_close_driver(data.drm_fd);
>>>>>       }
>>>>>   }
>>>>> diff --git a/tests/amdgpu/amd_assr.c b/tests/amdgpu/amd_assr.c
>>>>> index 80cbbe8e9..fc2367f99 100644
>>>>> --- a/tests/amdgpu/amd_assr.c
>>>>> +++ b/tests/amdgpu/amd_assr.c
>>>>> @@ -299,5 +299,6 @@ igt_main
>>>>>       igt_fixture
>>>>>       {
>>>>>           igt_display_fini(&data.display);
>>>>> +        drm_close_driver(data.fd);
>>>>>       }
>>>>>   }
>>>>> diff --git a/tests/amdgpu/amd_freesync_video_mode.c 
>>>>> b/tests/amdgpu/amd_freesync_video_mode.c
>>>>> index 579d24436..62d42a06c 100644
>>>>> --- a/tests/amdgpu/amd_freesync_video_mode.c
>>>>> +++ b/tests/amdgpu/amd_freesync_video_mode.c
>>>>> @@ -868,5 +868,6 @@ igt_main
>>>>>         igt_fixture {
>>>>>           igt_display_fini(&data.display);
>>>>> +        drm_close_driver(data.drm_fd);
>>>>>       }
>>>>>   }
>>>>> diff --git a/tests/amdgpu/amd_mem_leak.c b/tests/amdgpu/amd_mem_leak.c
>>>>> index dee563cbe..e4a4b5c47 100644
>>>>> --- a/tests/amdgpu/amd_mem_leak.c
>>>>> +++ b/tests/amdgpu/amd_mem_leak.c
>>>>> @@ -232,5 +232,6 @@ igt_main
>>>>>       igt_fixture
>>>>>       {
>>>>>           igt_display_fini(&data.display);
>>>>> +        drm_close_driver(data.fd);
>>>>>       }
>>>>>   }
>>>>> diff --git a/tests/amdgpu/amd_psr.c b/tests/amdgpu/amd_psr.c
>>>>> index 966a0dccf..3932e143a 100644
>>>>> --- a/tests/amdgpu/amd_psr.c
>>>>> +++ b/tests/amdgpu/amd_psr.c
>>>>> @@ -640,7 +640,7 @@ static void run_check_psr_su_cursor(data_t 
>>>>> *data, bool test_mpo)
>>>>>        * panning the primary plane at the top-left of screen
>>>>>        * set alpha region in overlay plane and set alpha to 0.0 to 
>>>>> show primary plane
>>>>>        * set cursor plane and starting from position of (0, 0)
>>>>> -     */
>>>>> +     */
>>>>>       draw_color_alpha(&data->ov_fb[0], 0, 0, data->pfb_w, 
>>>>> data->pfb_h, 1.0, 1.0, 1.0, .0);
>>>>>       igt_plane_set_fb(data->primary, &data->pm_fb[0]);
>>>>>       igt_plane_set_fb(data->overlay, &data->ov_fb[0]);
>>>>> @@ -763,5 +763,6 @@ igt_main_args("", long_options, help_str, 
>>>>> opt_handler, NULL)
>>>>>           }
>>>>>           close(data.debugfs_fd);
>>>>>           igt_display_fini(&data.display);
>>>>> +        drm_close_driver(data.fd);
>>>>>       }
>>>>>   }
>>>>> diff --git a/tests/amdgpu/amd_vrr_range.c 
>>>>> b/tests/amdgpu/amd_vrr_range.c
>>>>> index 2f27296dd..cacd668cd 100644
>>>>> --- a/tests/amdgpu/amd_vrr_range.c
>>>>> +++ b/tests/amdgpu/amd_vrr_range.c
>>>>> @@ -338,5 +338,6 @@ igt_main
>>>>>       igt_fixture
>>>>>       {
>>>>>           igt_display_fini(&data.display);
>>>>> +        drm_close_driver(data.fd);
>>>>>       }
>>>>>   }
>>>>> -- 
>>>>> 2.40.0
>>>>>
>