Re: [igt-dev] [PATCH i-g-t v3 10/52] tests/gem_busy: Adopt to use allocator

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Thu, 05 Aug 2021 01:02:41 -0700, Zbigniew Kempczyński wrote:
>

Please fix the put_ahnd. With that this patch is:

Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com>

But I want to discuss the intel_allocator_multiprocess_start/stop issue a
bit more below.

> On Wed, Aug 04, 2021 at 07:07:41PM -0700, Dixit, Ashutosh wrote:
> > On Mon, 26 Jul 2021 12:59:44 -0700, Zbigniew Kempczyński wrote:
> > >
> > > For newer gens we're not able to rely on relocations. Adopt to use
> > > offsets acquired from the allocator.
> > >
> > > Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
> > > Cc: Petri Latvala <petri.latvala@intel.com>
> > > Cc: Ashutosh Dixit <ashutosh.dixit@intel.com>
> > > ---
> > >  tests/i915/gem_busy.c | 35 +++++++++++++++++++++++++++++++----
> > >  1 file changed, 31 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/tests/i915/gem_busy.c b/tests/i915/gem_busy.c
> > > index f0fca0e8a..51ec5ad04 100644
> > > --- a/tests/i915/gem_busy.c
> > > +++ b/tests/i915/gem_busy.c
> > > @@ -108,6 +108,7 @@ static void semaphore(int fd, const intel_ctx_t *ctx,
> > >	uint32_t handle[3];
> > >	uint32_t read, write;
> > >	uint32_t active;
> > > +	uint64_t ahnd = get_reloc_ahnd(fd, ctx->id);
> > >	unsigned i;
> > >
> > >	handle[TEST] = gem_create(fd, 4096);
> > > @@ -117,6 +118,7 @@ static void semaphore(int fd, const intel_ctx_t *ctx,
> > >	/* Create a long running batch which we can use to hog the GPU */
> > >	handle[BUSY] = gem_create(fd, 4096);
> > >	spin = igt_spin_new(fd,
> > > +			    .ahnd = ahnd,
> > >			    .ctx = ctx,
> > >			    .engine = e->flags,
> > >			    .dependency = handle[BUSY]);
> >
> > Missing put_ahnd.
>
> Good catch.
>
> >
> > > @@ -428,6 +442,7 @@ igt_main
> > >
> > >	igt_subtest_group {
> > >		igt_fixture {
> > > +			intel_allocator_multiprocess_start();
> > >			igt_fork_hang_detector(fd);
> > >		}
> > >
> > > @@ -445,6 +460,21 @@ igt_main
> > >			}
> > >		}
> > >
> >
> > Just above here is basic() which doesn't have a fork. Is it ok to do
> > intel_allocator_multiprocess_start/stop when we don't have a fork? If yes,
> > then can we _always_ do intel_allocator_multiprocess_start/stop rather than
> > only when we have fork? Thanks.
>
> intel_allocator_multiprocess_start() creates allocator thread which acts
> for children (igt_fork) to alloc/free offsets. If you use alloc/free within
> same process (from which thread was spawned) internal structure is mutexed
> and no IPCs are called. So only consequence of this here is additional thread
> in system/memory (which does nothing for basic() tests). It will be stopped
> with intel_allocator_multiprocess_stop().

OK, in that case let me ask the question I asked above in another way. Can
we add intel_allocator_multiprocess_start() to common_init() (the program
entry point) and similarly say intel_allocator_multiprocess_stop() to
igt_exit() (or common_exit_handler(), basically the program exit point) so
that these always run and we don't have to add them only for specific tests
which fork? What would be the disadvantage of doing this? Thanks.

> But for purity test should work without additional dependencies so I'll fix
> this - it will be sent in v4.
>
> --
> Zbigniew