[igt-dev] [RFC PATCH v2 2/3] tests/gem_exec_reloc: Calculate softpin offsets from batch size

[Janusz Krzysztofik]

On Friday, October 25, 2019 11:50:49 AM CEST Katarzyna Dec wrote:
> On Wed, Oct 23, 2019 at 05:29:16PM +0200, Janusz Krzysztofik wrote:
> > From: Janusz Krzysztofik <janusz.krzysztofik at intel.com>
> > 
> > The basic-range subtest assumes 4kB minimum batch size.  On future
> > backends with possibly bigger minimum batch sizes this subtest will
> > fail as buffer objects may overlap on softpin.  To avoid object
> > overlapping, softpin offsets need to be calculated with actual minimum
> > batch size in mind.
> > 
> > Replace hardcoded constants corresponding to the assumed 4kB value with
> > variables supposed to reflect actual batch size.  For now, the
> > variables are still initialized with values specific to the 4kB minimum
> > batch size, which are suitable for backends currently supported by IGT.
> 
> This is a v2 patch, so it would be nice to know what has changed since v1.

You're right, sorry for that. I'll provide complete changelog with subsequent 
versions.

> > 
> > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik at intel.com>
> > Cc: Katarzyna Dec <katarzyna.dec at intel.com>
> > Cc: Stuart Summers <stuart.summers at intel.com>
> > ---
> >  tests/i915/gem_exec_reloc.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/tests/i915/gem_exec_reloc.c b/tests/i915/gem_exec_reloc.c
> > index f7fc0ea7..61401ea7 100644
> > --- a/tests/i915/gem_exec_reloc.c
> > +++ b/tests/i915/gem_exec_reloc.c
> > @@ -520,14 +520,16 @@ static void basic_range(int fd, unsigned flags)
> >  	uint64_t gtt_size = gem_aperture_size(fd);
> >  	const uint32_t bbe = MI_BATCH_BUFFER_END;
> >  	igt_spin_t *spin = NULL;
> > +	int batch_order = 12;
> > +	uint64_t batch_size = 1ull << batch_order; /* 4096 */
> >  	int count, n;
> >  
> >  	igt_require(gem_has_softpin(fd));
> >  
> > -	for (count = 12; gtt_size >> (count + 1); count++)
> > +	for (count = batch_order; gtt_size >> (count + 1); count++)
> >  		;
> >  
> > -	count -= 12;
> > +	count -= batch_order;
> >  
> >  	memset(obj, 0, sizeof(obj));
> >  	memset(reloc, 0, sizeof(reloc));
> > @@ -536,7 +538,7 @@ static void basic_range(int fd, unsigned flags)
> >  	n = 0;
> >  	for (int i = 0; i <= count; i++) {
> >  		obj[n].handle = gem_create(fd, 4096);
> Previous patch was changing 4096 into variable. Any reason why not to do 
this
> here? (And other places in this patch).
> If this is ok, than could you explain more your changes?

Indeed, that was the case before, however now I think those modifications 
should be dropped from the patch.  No matter if we request minimum page size 
exactly or smaller, those objects will be as large as a minimum page size 
anyway, and 4096 seems a safe default here.  With those unnecessary 
modifications dropped, the patch will be better focused on its goal, which is 
limited to fixing softpin offset calculations.

Another v2 change is the former size -> order conversion implemented as a 
for(...); loop has been reversed and implemented as size = (1ull << order) 
which is much more trivial.  As a side benefit, diff output is much better 
readable now.

Thanks,
Janusz


> Kasia
> > -		obj[n].offset = (1ull << (i + 12)) - 4096;
> > +		obj[n].offset = (1ull << (i + batch_order)) - 
batch_size;
> >  		obj[n].offset = gen8_canonical_address(obj[n].offset);
> >  		obj[n].flags = EXEC_OBJECT_PINNED | 
EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
> >  		if (!gem_uses_full_ppgtt(fd)) {
> > @@ -557,7 +559,7 @@ static void basic_range(int fd, unsigned flags)
> >  	}
> >  	for (int i = 1; i < count; i++) {
> >  		obj[n].handle = gem_create(fd, 4096);
> > -		obj[n].offset = 1ull << (i + 12);
> > +		obj[n].offset = 1ull << (i + batch_order);
> >  		obj[n].offset = gen8_canonical_address(obj[n].offset);
> >  		obj[n].flags = EXEC_OBJECT_PINNED | 
EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
> >  		if (!gem_uses_full_ppgtt(fd)) {
>