Re: [PATCH i-g-t v9 2/7] lib/drmtest: Introduced drm_open_driver_another

[Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>]

Hi Kamil,

On Monday, 19 February 2024 18:42:32 CET Kamil Konieczny wrote:
> Hi Janusz,
> On 2024-02-16 at 11:23:58 +0100, Janusz Krzysztofik wrote:
> > Hi Kamil, Dominik,
> > 
> > On Thursday, 15 February 2024 17:10:09 CET Kamil Konieczny wrote:
> > > From: Dominik Karol Piątkowski <dominik.karol.piatkowski@intel.com>
> > > 
> > > For user convenience, introduce drm_open_driver_another as
> > > a wrapper for __drm_open_driver_another with successful open
> > > assert.
> > 
> > Out of several existing variants of *drm_open_driver* functions, those which 
> > names start with "drm_", wrapped around their counterparts which names start 
> > with double underscores, used to set up an exit handler that performs 
> > necessary cleanups before exit, freeing users from taking care of that.  Don't 
> > we want to keep that convention also for drm_open_driver_another() and set up 
> > the exit handler from inside that wrapper on success?
> > 
> > Thanks,
> > Janusz
> > 
> 
> How and what should we unwind here? There is only fd, I do not see
> any other var or state to unwind? Am I missing something?

Have you had a look at drm_open()?

        /* For i915, at least, we ensure that the driver is idle before
         * starting a test and we install an exit handler to wait until
         * idle before quitting.
         */
        if (is_i915_device(fd)) {
                if (__sync_fetch_and_add(&open_count, 1) == 0) {
                        __cancel_work_at_exit(fd);
                        at_exit_drm_fd = drm_reopen_driver(fd);
                        igt_install_exit_handler(cancel_work_at_exit);
                }
        }

From that point of view, how does multi-gpu testing differ from single-gpu?

And yes, that can be not trivial to extend the existing exit handlers solution 
over multiple device file descriptor handling.

Also ...

> 
> Regards,
> Kamil
> 
> > > 
> > > v2: rebased (Kamil)
> > > v6: reword description (Janusz)
> > > 
> > > Cc: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> > > Signed-off-by: Dominik Karol Piątkowski <dominik.karol.piatkowski@intel.com>
> > > Signed-off-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>
> > > Reviewed-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
> > > ---
> > >  lib/drmtest.c | 19 +++++++++++++++++++
> > >  lib/drmtest.h |  1 +
> > >  2 files changed, 20 insertions(+)
> > > 
> > > diff --git a/lib/drmtest.c b/lib/drmtest.c
> > > index 52b5a2020..73d9159af 100644
> > > --- a/lib/drmtest.c
> > > +++ b/lib/drmtest.c
> > > @@ -563,6 +563,25 @@ int __drm_open_driver_another(int idx, int chipset)
> > >  	return fd;
> > >  }
> > >  
> > > +/*
> > > + * drm_open_driver_another
> > > + * @idx: index of the device you are opening
> > > + * @chipset: OR'd flags for each chipset to search, eg. #DRIVER_INTEL
> > > + *
> > > + * A wrapper for __drm_open_driver with successful open assert.
> > > + *
> > > + * Returns:
> > > + * An open DRM fd
> > > + */
> > > +int drm_open_driver_another(int idx, int chipset)
> > > +{
> > > +	int fd = __drm_open_driver_another(idx, chipset);
> > > +
> > > +	igt_assert_fd(fd);

drm_open_driver() skips in that case.  Shouldn't drm_open_driver_another() 
also skip for consistency?

Thanks,
Janusz

> > > +
> > > +	return fd;
> > > +}
> > > +
> > >  /**
> > >   * __drm_open_driver:
> > >   * @chipset: OR'd flags for each chipset to search, eg. #DRIVER_INTEL
> > > diff --git a/lib/drmtest.h b/lib/drmtest.h
> > > index 6bc819734..856208c48 100644
> > > --- a/lib/drmtest.h
> > > +++ b/lib/drmtest.h
> > > @@ -101,6 +101,7 @@ void __set_forced_driver(const char *name);
> > >  
> > >  int __drm_open_device(const char *name, unsigned int chipset);
> > >  void drm_load_module(unsigned int chipset);
> > > +int drm_open_driver_another(int idx, int chipset);
> > >  int drm_open_driver(int chipset);
> > >  int drm_open_driver_master(int chipset);
> > >  int drm_open_driver_render(int chipset);
> > > 
> > 
> > 
> > 
> > 
>