[igt-dev] [PATCH i-g-t 1/1] tests/gem_sync: Use softpin path for no-reloc gens

[Zbigniew Kempczyński]

On Tue, Nov 02, 2021 at 12:40:14PM -0700, Dixit, Ashutosh wrote:
> On Tue, 02 Nov 2021 01:39:41 -0700, Zbigniew Kempczyński wrote:
> >
> > Migrate to softpin with pre-warming batch or an allocator. Some tests
> > which don't fork can use pre-warming batch which establishes offsets in
> > the kernel before real work is performed. For such adding pinned flag
> > + zeroing relocation is enough. For multiprocess scenarios we need to
> > arbitrate and an allocator need to be used instead.
> >
> > v2: fix also tests which are not running on CI (Ashutosh)
> >
> > v3: use appropriate allocator handle (ahnd) within active_wakeup_ring
> >     spinners (Ashutosh)
> 
> Reviewed-by: Ashutosh Dixit <ashutosh.dixit at intel.com>
> 
> One question below though.
> 
> > Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>
> > Cc: Ashutosh Dixit <ashutosh.dixit at intel.com>
> > ---
> >  tests/i915/gem_sync.c | 59 ++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 55 insertions(+), 4 deletions(-)
> >
> > diff --git a/tests/i915/gem_sync.c b/tests/i915/gem_sync.c
> > index 0093ca25f..8c435845e 100644
> > --- a/tests/i915/gem_sync.c
> > +++ b/tests/i915/gem_sync.c
> > @@ -237,6 +237,7 @@ wakeup_ring(int fd, const intel_ctx_t *ctx, unsigned ring,
> >	    int timeout, int wlen)
> >  {
> >	struct intel_engine_data ied;
> > +	uint64_t ahnd = get_reloc_ahnd(fd, ctx->id);
> 
> Is this really needed in the parent even when they are only used in the
> child processes? This pattern is used everywhere in this patch. Is this a
> requirement of the allocator API? Thanks.

It removes race which may generate errors when children requests for offsets
from the allocator. Example of error behavior (order in time):

child 1:
	ahnd = get_reloc_ahnd();  // creates new allocator, ahnd refcount 0->1
        offset = get_offset(ahnd, ...); //allocator returns first offset, lets say 0x10000
        put_ahnd(ahnd);           // ahnd refcount 1->0, thus frees allocator

child 2:
        ahnd = get_reloc_ahnd();  // creates new allocator, ahnd refcount 0->1
        offset = get_offset(ahnd, ...); //allocator returns first offset, again same 0x10000
        put_ahnd(ahnd);           // ahnd refcount 1->0, thus frees allocator
  
Additional empty handler around children keeps refcount > 0 so allocator
won't be free and recreated so proposed offsets in reloc pseudoallocator
will be contigues.

--
Zbigniew