[igt-dev] [i-g-t 05/20] tests/amdgpu: Close the fd before exit
Kamil Konieczny
kamil.konieczny at linux.intel.com
Thu Jun 15 11:48:47 UTC 2023
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 ?
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.
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