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 <[email protected]>
> ---
> 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
>
