[Intel-gfx] [PATCH] drm/i915/lmem: Limit block size to 4G

[Chris Wilson]

Quoting Matthew Auld (2020-11-30 13:08:43)
> From: Venkata Sandeep Dhanalakota <venkata.s.dhanalakota at intel.com>
> 
> Block sizes are only limited by the largest power-of-two that will fit
> in the region size, but to construct an object we also require feeding
> it into an sg list, where the upper limit of the sg entry is at most
> UINT_MAX. Therefore to prevent issues with allocating blocks that are
> too large, add the flag I915_ALLOC_MAX_SEGMENT_SIZE which should limit
> block sizes to the i915_sg_segment_size().
> 
> v2: (matt)
>   - query the max segment.
>   - prefer flag to limit block size to 4G, since it's best not to assume
>     the user will feed the blocks into an sg list.
>   - simple selftest so we don't have to guess.
> 
> Cc: Niranjana Vishwanathapura <niranjana.vishwanathapura at intel.com>
> Cc: Matthew Auld <matthew.auld at intel.com>
> Cc: CQ Tang <cq.tang at intel.com>
> Signed-off-by: Venkata Sandeep Dhanalakota <venkata.s.dhanalakota at intel.com>
> Signed-off-by: Matthew Auld <matthew.auld at intel.com>
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_region.c    |  2 +-
>  drivers/gpu/drm/i915/intel_memory_region.c    | 16 +++++-
>  drivers/gpu/drm/i915/intel_memory_region.h    |  5 +-
>  .../drm/i915/selftests/intel_memory_region.c  | 50 +++++++++++++++++++
>  4 files changed, 69 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_region.c b/drivers/gpu/drm/i915/gem/i915_gem_region.c
> index 1515384d7e0e..e72d78074c9e 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_region.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_region.c
> @@ -42,7 +42,7 @@ i915_gem_object_get_pages_buddy(struct drm_i915_gem_object *obj)
>                 return -ENOMEM;
>         }
>  
> -       flags = I915_ALLOC_MIN_PAGE_SIZE;
> +       flags = I915_ALLOC_MIN_PAGE_SIZE | I915_ALLOC_MAX_SEGMENT_SIZE;
>         if (obj->flags & I915_BO_ALLOC_CONTIGUOUS)
>                 flags |= I915_ALLOC_CONTIGUOUS;
>  
> diff --git a/drivers/gpu/drm/i915/intel_memory_region.c b/drivers/gpu/drm/i915/intel_memory_region.c
> index b326993a1026..8a376f1b5b3b 100644
> --- a/drivers/gpu/drm/i915/intel_memory_region.c
> +++ b/drivers/gpu/drm/i915/intel_memory_region.c
> @@ -72,6 +72,7 @@ __intel_memory_region_get_pages_buddy(struct intel_memory_region *mem,
>                                       struct list_head *blocks)
>  {
>         unsigned int min_order = 0;
> +       unsigned int max_order;
>         unsigned long n_pages;
>  
>         GEM_BUG_ON(!IS_ALIGNED(size, mem->mm.chunk_size));
> @@ -92,13 +93,26 @@ __intel_memory_region_get_pages_buddy(struct intel_memory_region *mem,
>  
>         n_pages = size >> ilog2(mem->mm.chunk_size);
>  
> +       /*
> +        * If we going to feed this into an sg list we should limit the block
> +        * sizes such that we don't exceed the i915_sg_segment_size().
> +        */
> +       if (flags & I915_ALLOC_MAX_SEGMENT_SIZE) {
> +               unsigned int max_segment = i915_sg_segment_size();
> +
> +               GEM_BUG_ON(max_segment < mem->mm.chunk_size);

iirc, the swiotlb segment size can be adjusted by user parameter.
[Don't ask if swiotlb is compatible with lmem, I suspect not ;]

I think err on the side of safety, just in case the user does find a way
to adjust the parameter,

if (GEM_WARN_ON(max_segment < mem->mm.chunk_size))
	max_order = 0;
else
	max_order = ilog2(max_segment) - ilog2(mem->mm.chunk_size);

> +               max_order = ilog2(max_segment) - ilog2(mem->mm.chunk_size);
> +       } else {
> +               max_order = mem->mm.max_order;
> +       }
> +
>         mutex_lock(&mem->mm_lock);
>  
>         do {
>                 struct i915_buddy_block *block;
>                 unsigned int order;
>  
> -               order = fls(n_pages) - 1;
> +               order = min_t(u32, (fls(n_pages) - 1), max_order);

Spare () around fls.

>                 GEM_BUG_ON(order > mem->mm.max_order);
>                 GEM_BUG_ON(order < min_order);
>  
> diff --git a/drivers/gpu/drm/i915/intel_memory_region.h b/drivers/gpu/drm/i915/intel_memory_region.h
> index 232490d89a83..5fb9bcf86b97 100644
> --- a/drivers/gpu/drm/i915/intel_memory_region.h
> +++ b/drivers/gpu/drm/i915/intel_memory_region.h
> @@ -44,8 +44,9 @@ enum intel_region_id {
>  #define MEMORY_TYPE_FROM_REGION(r) (ilog2((r) >> INTEL_MEMORY_TYPE_SHIFT))
>  #define MEMORY_INSTANCE_FROM_REGION(r) (ilog2((r) & 0xffff))
>  
> -#define I915_ALLOC_MIN_PAGE_SIZE  BIT(0)
> -#define I915_ALLOC_CONTIGUOUS     BIT(1)
> +#define I915_ALLOC_MIN_PAGE_SIZE       BIT(0)
> +#define I915_ALLOC_CONTIGUOUS          BIT(1)
> +#define I915_ALLOC_MAX_SEGMENT_SIZE    BIT(2)
>  
>  #define for_each_memory_region(mr, i915, id) \
>         for (id = 0; id < ARRAY_SIZE((i915)->mm.regions); id++) \
> diff --git a/drivers/gpu/drm/i915/selftests/intel_memory_region.c b/drivers/gpu/drm/i915/selftests/intel_memory_region.c
> index 0aeba8e3af28..cd706c0d9213 100644
> --- a/drivers/gpu/drm/i915/selftests/intel_memory_region.c
> +++ b/drivers/gpu/drm/i915/selftests/intel_memory_region.c
> @@ -337,6 +337,55 @@ static int igt_mock_splintered_region(void *arg)
>         return err;
>  }
>  
> +#define SZ_8G BIT(33)

BIT_ULL

Otherwise makes sense.

Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
-Chris