[igt-dev] [PATCH i-g-t] tests/gem_*: Fix release offsets

[Zbigniew Kempczyński]

On Mon, Dec 19, 2022 at 12:26:54PM +0100, Kamil Konieczny wrote:
> 
> Hi Zbigniew,
> 
> On 2022-12-19 at 08:01:20 +0100, Zbigniew Kempczyński wrote:
> > After changing reloc allocator to track offsets allocations all
> > shortcommings started to be visible. Release offsets requires
> > handles, not offsets so fix this in the tests.
> 
> In gem_whisper you also corrected allocation, so imho you should
> write about it or put that in separate patch, see below.
> 
> > 
> > Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>
> > Cc: Kamil Konieczny <kamil.konieczny at linux.intel.com>
> > ---
> >  tests/i915/gem_blits.c         | 12 ++++++------
> >  tests/i915/gem_ctx_isolation.c |  6 +++---
> >  tests/i915/gem_exec_whisper.c  |  7 ++++---
> >  3 files changed, 13 insertions(+), 12 deletions(-)
> > 
> > diff --git a/tests/i915/gem_blits.c b/tests/i915/gem_blits.c
> > index 24e83b9f51..d9296cf2d1 100644
> > --- a/tests/i915/gem_blits.c
> > +++ b/tests/i915/gem_blits.c
> > @@ -264,11 +264,11 @@ static void buffer_set_tiling(const struct device *device,
> >  	gem_execbuf(device->fd, &execbuf);
> >  
> >  	gem_close(device->fd, obj[2].handle);
> > -	put_offset(device->ahnd, obj[2].offset);
> > +	put_offset(device->ahnd, obj[2].handle);
> >  
> >  	gem_close(device->fd, obj[1].handle);
> > +	put_offset(device->ahnd, obj[1].handle);
> >  
> > -	put_offset(device->ahnd, buffer->gtt_offset);
> >  	buffer->gtt_offset = obj[0].offset;
> >  	buffer->handle = obj[0].handle;
> >  
> > @@ -403,11 +403,11 @@ static bool blit_to_linear(const struct device *device,
> >  
> >  	gem_execbuf(device->fd, &execbuf);
> >  	gem_close(device->fd, obj[2].handle);
> > -	put_offset(device->ahnd, obj[2].offset);
> > +	put_offset(device->ahnd, obj[2].handle);
> >  
> >  	gem_sync(device->fd, obj[0].handle);
> >  	gem_close(device->fd, obj[0].handle);
> > -	put_offset(device->ahnd, obj[0].offset);
> > +	put_offset(device->ahnd, obj[0].handle);
> >  
> >  	return true;
> >  }
> > @@ -531,7 +531,7 @@ static void buffer_free(const struct device *device, struct buffer *buffer)
> >  {
> >  	igt_assert(buffer_check(device, buffer, GTT));
> >  	gem_close(device->fd, buffer->handle);
> > -	put_offset(device->ahnd, buffer->gtt_offset);
> > +	put_offset(device->ahnd, buffer->handle);
> >  	free(buffer);
> >  }
> >  
> > @@ -744,7 +744,7 @@ blit(const struct device *device,
> >  
> >  	gem_execbuf(device->fd, &execbuf);
> >  	gem_close(device->fd, obj[2].handle);
> > -	put_offset(device->ahnd, obj[2].offset);
> > +	put_offset(device->ahnd, obj[2].handle);
> >  
> >  	dst->gtt_offset = obj[0].offset;
> >  	src->gtt_offset = obj[1].offset;
> > diff --git a/tests/i915/gem_ctx_isolation.c b/tests/i915/gem_ctx_isolation.c
> > index ec69357140..2def529ac3 100644
> > --- a/tests/i915/gem_ctx_isolation.c
> > +++ b/tests/i915/gem_ctx_isolation.c
> > @@ -437,7 +437,7 @@ static void write_regs(int fd, uint64_t ahnd,
> >  	}
> >  	gem_execbuf(fd, &execbuf);
> >  	gem_close(fd, obj.handle);
> > -	put_offset(ahnd, obj.offset);
> > +	put_offset(ahnd, obj.handle);
> >  }
> >  
> >  static void restore_regs(int fd,
> > @@ -524,8 +524,8 @@ static void restore_regs(int fd,
> >  	execbuf.rsvd1 = ctx->id;
> >  	gem_execbuf(fd, &execbuf);
> >  	gem_close(fd, obj[1].handle);
> > -	put_offset(ahnd, obj[0].offset);
> > -	put_offset(ahnd, obj[1].offset);
> > +	put_offset(ahnd, obj[0].handle);
> > +	put_offset(ahnd, obj[1].handle);
> >  }
> >  
> >  __attribute__((unused))
> > diff --git a/tests/i915/gem_exec_whisper.c b/tests/i915/gem_exec_whisper.c
> > index c3fc5ba804..616231aa96 100644
> > --- a/tests/i915/gem_exec_whisper.c
> > +++ b/tests/i915/gem_exec_whisper.c
> > @@ -106,10 +106,10 @@ static void init_hang(struct hang *h, int fd, const intel_ctx_cfg_t *cfg)
> >  	if (gem_has_contexts(fd)) {
> ---------------------------- ^
> Should this be h->fd ?
> 
> >  		h->ctx = intel_ctx_create(h->fd, cfg);
> >  		h->execbuf.rsvd1 = h->ctx->id;
> > -		h->ahnd = get_reloc_ahnd(fd, h->ctx->id);
> > +		h->ahnd = get_reloc_ahnd(h->fd, h->ctx->id);
> 
> This is allocation.
> 
> >  	} else {
> >  		h->ctx = NULL;
> > -		h->ahnd = get_reloc_ahnd(fd, 0);
> > +		h->ahnd = get_reloc_ahnd(h->fd, 0);
> 
> Same here, so maybe put this also in description ?
> Or make it separate patch ?

Ok, I change logic a bit here so it is worth to add separate
patch.

> 
> >  	}
> >  
> >  	memset(&h->execbuf, 0, sizeof(h->execbuf));
> > @@ -174,7 +174,8 @@ static void submit_hang(struct hang *h, unsigned *engines, int nengine, unsigned
> >  
> >  static void fini_hang(struct hang *h)
> >  {
> > -	put_offset(h->ahnd, h->bb_offset);
> > +	gem_close(h->fd, h->obj.handle);
> 
> Here you fix another bug, missed gem_close.

Generally it will be closed with h->fd closing (close(h->fd)) so this
only is adding explicit handling (for purity, not really necessary as
we're on reopened drm fd).

--
Zbigniew
> 
> Regards,
> Kamil
> 
> > +	put_offset(h->ahnd, h->obj.handle);
> >  	put_ahnd(h->ahnd);
> >  	intel_ctx_destroy(h->fd, h->ctx);
> >  	close(h->fd);
> > -- 
> > 2.34.1
> >