Re: [igt-dev] [PATCH i-g-t v4 04/77] i915/gem_linear_blits: Close device before exit

[Mauro Carvalho Chehab <mauro.chehab@linux.intel.com>]

On Wed, 05 Oct 2022 17:43:33 +0300
Jani Nikula <jani.nikula@linux.intel.com> wrote:

> On Wed, 05 Oct 2022, Mauro Carvalho Chehab <mauro.chehab@linux.intel.com> wrote:
> > On Wed, 05 Oct 2022 14:09:18 +0300
> > Jani Nikula <jani.nikula@linux.intel.com> wrote:
> >  
> >> On Wed, 05 Oct 2022, Mauro Carvalho Chehab <mauro.chehab@linux.intel.com> wrote:  
> >> > From: Chris Wilson <chris.p.wilson@intel.com>
> >> >
> >> > Close the device fd before we check for leaks during the atexit
> >> > handlers.
> >> >
> >> > Signed-off-by: Chris Wilson <chris.p.wilson@intel.com>
> >> > Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>
> >> > Reviewed-by: Nirmoy Das<nirmoy.das@intel.com>
> >> > Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>
> >> > ---
> >> >  tests/i915/gem_linear_blits.c | 3 ++-
> >> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git a/tests/i915/gem_linear_blits.c b/tests/i915/gem_linear_blits.c
> >> > index 1fd5b733c3ce..d02751be9232 100644
> >> > --- a/tests/i915/gem_linear_blits.c
> >> > +++ b/tests/i915/gem_linear_blits.c
> >> > @@ -47,6 +47,7 @@
> >> >  #include "i915/gem.h"
> >> >  #include "i915/gem_create.h"
> >> >  #include "igt.h"
> >> > +#include "igt_types.h"
> >> >  
> >> >  IGT_TEST_DESCRIPTION("Test doing many blits with a working set larger than the"
> >> >  		     " aperture size.");
> >> > @@ -236,7 +237,7 @@ igt_main
> >> >  	const int ncpus = sysconf(_SC_NPROCESSORS_ONLN);
> >> >  	uint64_t count = 0;
> >> >  	bool do_relocs;
> >> > -	int fd = -1;
> >> > +	igt_fd_t(fd);    
> >> 
> >> I guess igt has long since stopped being written in C, and started being
> >> written in an "igt dialect" of its own... but the constructs still kind
> >> of resemble C.
> >> 
> >> This doesn't. It looks like a function call but isn't. Looks odd that
> >> it's among the declarations... but it is a declaration.  
> >
> > True, but this is not different than what we have already at the Linux
> > Kernel with things like DECLARE_BITMAP():
> >
> >    struct fsubdev {
> >         struct work_struct fw_work;
> >         struct work_struct pm_work;
> >         struct delayed_work ue_work;
> >         struct work_struct rescan_work;
> >         struct fdev *fdev;
> >         char __iomem *csr_base;
> >         int irq;
> >         bool fw_running;
> >         DECLARE_BITMAP(errors, NUM_SD_ERRORS);
> >         struct mbdb *mbdb;
> > ...
> >
> >
> > and other similar variable declaration macros.
> >
> > Maybe we could name it at patch 01/77 to:
> >
> >
> > 	#define DECLARE_IGT_FD(x__) \
> > 	       volatile int x__ cleanup_with(igt_cleanup_fd) = IGT_OUTER_SCOPE_INIT(-1)
> >
> > in order to be more like the Kernel.  
> 
> Yeah, maybe "declare" in the name would be helpful.
> 
> I was just making an observation, really, and I'll defer the decisions
> to Petri here.

As Petri reviewed patch 1, which added the macro, and all patches were
reviewed, I'm applying the series.

It should be easy to replace "igt_fd_t" to "DECLARE_IGT_FD" or
with something else in the future, if we think it would make it
better.

> 
> > In a matter of fact, C is just a nicer and more portable way to work
> > macro-assembler... :-p  
> 
> Macros really are a blessing and a curse here...

Agreed.

Regards,
Mauro