[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 12:03:34 UTC 2023
Am 15.06.23 um 13:48 schrieb Kamil Konieczny:
> Hi Christian,
>
> I dropped David Zhang from Cc because I got bounce:
>
> David Zhang (dingchen.zhang at amd.com)<mailto:dingchen.zhang at amd.com>
> The email address you entered couldn't be found
>
> Maybe address should be david.zhang at amd.com ?
David unfortunately doesn't work for AMD any more. Just completely drop him.
>
> On 2023-06-15 at 10:43:05 +0200, Christian König wrote:
>> 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);
>> }
>>
> I see, so we should drop amd patches with drm_close_driver()
> from this patch series.
>
> Btw I do see in tests/amdgpu/amd_info.c at last fixture:
>
> igt_fixture {
> amdgpu_device_deinitialize(dev);
> close(fd);
> }
>
> That "close(fd);" should be removed from amd_info.c test.
After digging through the full source code I've found the matching dup()
to the close in amdgpu_device_free_internal().
It's part of amdgpu_device_initialize() but uses the fnctl() version of
dup(), so my "grep dup*" I couldn't find it of hand:
dev->fd = fcntl(fd, F_DUPFD_CLOEXEC, 0);
Sorry for the noise, your original patch was correct to begin with.
Regards,
Christian.
>
> Regards,
> Kamil
>
>>> 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