[igt-dev] [i-g-t 05/20] tests/amdgpu: Close the fd before exit
Christian König
christian.koenig at amd.com
Thu Jun 15 08:43:05 UTC 2023
Am 15.06.23 um 10:28 schrieb 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.
Ok, so we dup() the file descriptor somewhere?
Because at least amdgpu_device_free_internal() is clearly closing the fd
before freeing the device structure:
static void amdgpu_device_free_internal(amdgpu_device_handle dev)
{
...
close(dev->fd);
...
free(dev);
}
>
> 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)
Well no, that's not related to gcc in any way possible. File descriptors
are closed by the kernel on process exit.
Regards,
Christian.
>
> 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
>>>>>>
>>
More information about the igt-dev
mailing list