[PATCH v2] drm/i915/selftest: allow larger memory allocation

[Krzysztof Niemiec]

Hi Mikolaj,

On 2025-03-21 at 15:02:49 GMT, Mikolaj Wasiak wrote:
> Due to changes in allocator, the size of the allocation for
> contiguous region is not rounded up to a power-of-two and
> instead allocated as is. Thus, change the part of test that
> expected the allocation to fail.
> 
> Signed-off-by: Mikolaj Wasiak <mikolaj.wasiak at intel.com>
> ---
> v1 -> v2:
> - Added negative test for too large allocation
> 
>  .../drm/i915/selftests/intel_memory_region.c  | 22 ++++++++++---------
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/selftests/intel_memory_region.c b/drivers/gpu/drm/i915/selftests/intel_memory_region.c
> index f08f6674911e..7e72e12b26ca 100644
> --- a/drivers/gpu/drm/i915/selftests/intel_memory_region.c
> +++ b/drivers/gpu/drm/i915/selftests/intel_memory_region.c
> @@ -413,15 +413,8 @@ static int igt_mock_splintered_region(void *arg)
>  
>  	close_objects(mem, &objects);
>  
> -	/*
> -	 * While we should be able allocate everything without any flag
> -	 * restrictions, if we consider I915_BO_ALLOC_CONTIGUOUS then we are
> -	 * actually limited to the largest power-of-two for the region size i.e
> -	 * max_order, due to the inner workings of the buddy allocator. So make
> -	 * sure that does indeed hold true.
> -	 */
> -
> -	obj = igt_object_create(mem, &objects, size, I915_BO_ALLOC_CONTIGUOUS);
> +	obj = igt_object_create(mem, &objects, roundup_pow_of_two(size),
> +				I915_BO_ALLOC_CONTIGUOUS);
>  	if (!IS_ERR(obj)) {
>  		pr_err("%s too large contiguous allocation was not rejected\n",
>  		       __func__);
> @@ -429,10 +422,19 @@ static int igt_mock_splintered_region(void *arg)
>  		goto out_close;
>  	}
>  
> +	obj = igt_object_create(mem, &objects, size, I915_BO_ALLOC_CONTIGUOUS);
> +	if (IS_ERR(obj)) {
> +		pr_err("%s largest possible contiguous allocation failed\n",
> +		       __func__);
> +		err = PTR_ERR(obj);
> +		goto out_close;
> +	}
> +	close_objects(mem, &objects);
> +
>  	obj = igt_object_create(mem, &objects, rounddown_pow_of_two(size),
>  				I915_BO_ALLOC_CONTIGUOUS);
>  	if (IS_ERR(obj)) {
> -		pr_err("%s largest possible contiguous allocation failed\n",
> +		pr_err("%s largest rounded possible contiguous allocation failed\n",
>  		       __func__);
>  		err = PTR_ERR(obj);
>  		goto out_close;

The very last check is redundant, and this is why:

Firstly, we already try to allocate a contiguous block of size. Since
size >= rounddown_pow_of_two(size), if size can get allocated
contiguously, so can the rounddown.

Secondly (and more importantly):

To quote an email from Krzysztof Karas from comments on v1:

	" We need the following:
	  1) Fail check on "too large",
	  2) Pass check on "max possible size",
	  3) Pass check on contiguous object. "

1) is checked with the first object, 2) is checked with the second
object. 3) is checked with both the second and the third object, because
they both use the I915_BO_ALLOC_CONTIGUOUS flag.

It is worth noting here that we also check 2) with a previous obj, at
the very beginning of the test just after creating the region:

	size = (SZ_4G - 1) & PAGE_MASK;
	mem = mock_region_create(i915, 0, size, PAGE_SIZE, 0, 0);
	if (IS_ERR(mem))
		return PTR_ERR(mem);

	obj = igt_object_create(mem, &objects, size, 0);
	if (IS_ERR(obj)) {
		err = PTR_ERR(obj);
		goto out_close;
	}

(this is from selftests/intel_memory_region.c:386, just before the
checks changed in this patch); Notice how this doesn't have the
contiguous flag.

So given the agreed upon behavior of this test, the optimal way to do
the checks would be:

- object from the snippet above (that's not touched by the patch) (size
  unchanged, no flag) - this checks 2);
- the first object from the patch (size := roundup_pow_of_two(size), flag
  or no flag should fail either way) - this checks 1);
- the second object from the patch (size unchanged, with flag) - this
  checks 3)

So the last object is unneeded.

TLDR there's no need to do the check on rounddown_pow_of_two(size). With
that changed, it'll be LGTM.

Thanks
Krzysztof

> -- 
> 2.49.0
>