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

[Katarzyna Dec]

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.
> 
> 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?

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)) {
> -- 
> 2.21.0
>