[Intel-gfx] [PATCH 1/2] drm/i915/gem: Limit lmem scatterlist elements to UINT_MAX

[Matthew Auld]

On 01/12/2020 23:58, Chris Wilson wrote:
> Adhere to the i915_sg_max_segment() limit on the lengths of individual
> scatterlist elements, and in doing so split up very large chunks of lmem
> into managable pieces for the dma-mapping backend.
> 
> Reported-by: Venkata Sandeep Dhanalakota <venkata.s.dhanalakota at intel.com>
> Suggested-by: Matthew Auld <matthew.auld at intel.com>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Venkata Sandeep Dhanalakota <venkata.s.dhanalakota at intel.com>
> Cc: Matthew Auld <matthew.auld at intel.com>
> ---
>   drivers/gpu/drm/i915/gem/i915_gem_region.c | 42 ++++++++++++----------
>   1 file changed, 24 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_region.c b/drivers/gpu/drm/i915/gem/i915_gem_region.c
> index e72d78074c9e..edb84072f979 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_region.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_region.c
> @@ -22,6 +22,7 @@ i915_gem_object_put_pages_buddy(struct drm_i915_gem_object *obj,
>   int
>   i915_gem_object_get_pages_buddy(struct drm_i915_gem_object *obj)
>   {
> +	const u64 max_segment = i915_sg_segment_size();

I think we just need to add:

@@ -150,8 +150,8 @@ static int igt_mock_contiguous(void *arg)
         if (IS_ERR(obj))
                 return PTR_ERR(obj);

-       if (obj->mm.pages->nents != 1) {
-               pr_err("%s min object spans multiple sg entries\n", 
__func__);
+       if (!list_is_singular(&obj->mm.blocks)) {
+               pr_err("%s min object spans multiple blocks\n", __func__);
                 err = -EINVAL;
                 goto err_close_objects;
         }
@@ -163,8 +163,8 @@ static int igt_mock_contiguous(void *arg)
         if (IS_ERR(obj))
                 return PTR_ERR(obj);

-       if (obj->mm.pages->nents != 1) {
-               pr_err("%s max object spans multiple sg entries\n", 
__func__);
+       if (!list_is_singular(&obj->mm.blocks)) {
+               pr_err("%s max object spans multiple blocks\n", __func__);
                 err = -EINVAL;
                 goto err_close_objects;
         }
@@ -189,8 +189,8 @@ static int igt_mock_contiguous(void *arg)
                 goto err_close_objects;
         }

-       if (obj->mm.pages->nents != 1) {
-               pr_err("%s object spans multiple sg entries\n", __func__);
+       if (!list_is_singular(&obj->mm.blocks)) {
+               pr_err("%s object spans multiple blocks\n", __func__);
                 err = -EINVAL;
                 goto err_close_objects;
         }


Thanks for fixing this,
Reviewed-by: Matthew Auld <matthew.auld at intel.com>


>   	struct intel_memory_region *mem = obj->mm.region;
>   	struct list_head *blocks = &obj->mm.blocks;
>   	resource_size_t size = obj->base.size;
> @@ -37,7 +38,7 @@ i915_gem_object_get_pages_buddy(struct drm_i915_gem_object *obj)
>   	if (!st)
>   		return -ENOMEM;
>   
> -	if (sg_alloc_table(st, size >> ilog2(mem->mm.chunk_size), GFP_KERNEL)) {
> +	if (sg_alloc_table(st, size >> PAGE_SHIFT, GFP_KERNEL)) {
>   		kfree(st);
>   		return -ENOMEM;
>   	}
> @@ -58,33 +59,38 @@ i915_gem_object_get_pages_buddy(struct drm_i915_gem_object *obj)
>   	prev_end = (resource_size_t)-1;
>   
>   	list_for_each_entry(block, blocks, link) {
> -		u64 block_size, offset;
> +		u64 block_size, offset, len;
>   
>   		block_size = min_t(u64, size,
>   				   i915_buddy_block_size(&mem->mm, block));
>   		offset = i915_buddy_block_offset(block);
>   
> -		GEM_BUG_ON(overflows_type(block_size, sg->length));
> +		while (block_size) {
> +			if (offset != prev_end || sg->length >= max_segment) {
> +				if (st->nents) {
> +					sg_page_sizes |= sg->length;
> +					sg = __sg_next(sg);
> +				}
>   
> -		if (offset != prev_end ||
> -		    add_overflows_t(typeof(sg->length), sg->length, block_size)) {
> -			if (st->nents) {
> -				sg_page_sizes |= sg->length;
> -				sg = __sg_next(sg);
> -			}
> +				len = min(block_size, max_segment);
>   
> -			sg_dma_address(sg) = mem->region.start + offset;
> -			sg_dma_len(sg) = block_size;
> +				sg_dma_address(sg) = mem->region.start + offset;
> +				sg_dma_len(sg) = len;
>   
> -			sg->length = block_size;
> +				sg->length = len;
>   
> -			st->nents++;
> -		} else {
> -			sg->length += block_size;
> -			sg_dma_len(sg) += block_size;
> -		}
> +				st->nents++;
> +			} else {
> +				len = min(block_size, max_segment - sg->length);
> +				sg->length += len;
> +				sg_dma_len(sg) += len;
> +			}
>   
> -		prev_end = offset + block_size;
> +			offset += len;
> +			block_size -= len;
> +
> +			prev_end = offset;
> +		}
>   	}
>   
>   	sg_page_sizes |= sg->length;
>