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

[Janusz Krzysztofik]

Hi Kamil,

On Tuesday, 20 February 2024 19:33:44 CET Kamil Konieczny wrote:
> Hi Janusz,
> On 2024-02-20 at 10:32:58 +0100, Janusz Krzysztofik wrote:
> > 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 at 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.
> > 
> 
> This is another problem, I try to solve one, not all at once.
> I could add a helper to stop resets from children as it can be
> helpfull not only in multi-gpu scenarios.

OK, but users may still expect extra features from the new function, compared 
to the one it wraps around, similar to those provided by drm_open_driver() vs. 
__drm_open_driver().  If that expectation can't be satisfied at the moment 
then we should warn users via a comment about those potentially expected 
features missing for now, I believe.  Or explain why drm_open_driver_another() 
is going to handle that in a way different from drm_driver_open(), if so 
decided.

Thanks,
Janusz

> 
> > Also ...
> > 
> > > 
> > > Regards,
> > > Kamil
> > > 
> > > > > 
> > > > > v2: rebased (Kamil)
> > > > > v6: reword description (Janusz)
> > > > > 
> > > > > Cc: Janusz Krzysztofik <janusz.krzysztofik at linux.intel.com>
> > > > > Signed-off-by: Dominik Karol Piątkowski <dominik.karol.piatkowski at intel.com>
> > > > > Signed-off-by: Kamil Konieczny <kamil.konieczny at linux.intel.com>
> > > > > Reviewed-by: Zbigniew Kempczyński <zbigniew.kempczynski at 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?
> 
> Good point, I will re-read this,
> 
> Regards,
> Kamil
> 
> > 
> > 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);
> > > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > 
> > 
> > 
> > 
> > 
>