[Intel-gfx] [PATCH] drm/i915: use i915_sg_dma_sizes() for internal backend

[Tvrtko Ursulin]

On 20/10/2022 16:10, Matthew Auld wrote:
> We rely on page_sizes.sg in setup_scratch_page() reporting the correct
> value if the underlying sgl is not contiguous, however in
> get_pages_internal() we are only looking at the layout of the created
> pages when calculating the sg_page_sizes, and not the final sgl, which
> could in theory be completely different. In such a situation we might
> incorrectly think we have a 64K scratch page, when it is actually only
> 4K or similar split over multiple non-contiguous entries, which could
> lead to broken behaviour when touching the scratch space within the
> padding of a 64K GTT page-table.  Like we already do for other backends,
> switch over to calling i915_sg_dma_sizes() after mapping the pages.
> 
> Signed-off-by: Matthew Auld <matthew.auld at intel.com>
> Cc: Andrzej Hajda <andrzej.hajda at intel.com>
> ---
>   drivers/gpu/drm/i915/gem/i915_gem_internal.c | 5 +----
>   1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_internal.c b/drivers/gpu/drm/i915/gem/i915_gem_internal.c
> index c698f95af15f..301cfb127c4c 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_internal.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_internal.c
> @@ -36,7 +36,6 @@ static int i915_gem_object_get_pages_internal(struct drm_i915_gem_object *obj)
>   	struct drm_i915_private *i915 = to_i915(obj->base.dev);
>   	struct sg_table *st;
>   	struct scatterlist *sg;
> -	unsigned int sg_page_sizes;
>   	unsigned int npages;
>   	int max_order;
>   	gfp_t gfp;
> @@ -75,7 +74,6 @@ static int i915_gem_object_get_pages_internal(struct drm_i915_gem_object *obj)
>   
>   	sg = st->sgl;
>   	st->nents = 0;
> -	sg_page_sizes = 0;
>   
>   	do {
>   		int order = min(fls(npages) - 1, max_order);
> @@ -94,7 +92,6 @@ static int i915_gem_object_get_pages_internal(struct drm_i915_gem_object *obj)
>   		} while (1);
>   
>   		sg_set_page(sg, page, PAGE_SIZE << order, 0);
> -		sg_page_sizes |= PAGE_SIZE << order;
>   		st->nents++;
>   
>   		npages -= 1 << order;
> @@ -116,7 +113,7 @@ static int i915_gem_object_get_pages_internal(struct drm_i915_gem_object *obj)
>   		goto err;
>   	}
>   
> -	__i915_gem_object_set_pages(obj, st, sg_page_sizes);
> +	__i915_gem_object_set_pages(obj, st, i915_sg_dma_sizes(st->sgl));

Should __i915_gem_object_set_pages just do that internally? I mean are 
there callers which call it before DMA mapping and legitimately pass in 
st_page_sizes different from what i915_sg_dma_sizes() would find?

Regards,

Tvrtko

>   
>   	return 0;
>