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

["Christian König" <christian.koenig@amd.com>]

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@amd.com>
>>>>> Cc: Christian König<christian.koenig@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@amd.com>
>>>>>> Cc: Harry Wentland<harry.wentland@amd.com>
>>>>>> Signed-off-by: Bhanuprakash Modem<bhanuprakash.modem@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
>>>>>>
>>