On Thu, 2022-10-20 at 16:10 +0100, 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.
Makes sense to me. Is there a bug tracking this though? If so please
add here.
Otherwise:
Reviewed-by: Stuart Summers <stuart.summers@xxxxxxxxx>
>
> Signed-off-by: Matthew Auld <matthew.auld@xxxxxxxxx>
> Cc: Andrzej Hajda <andrzej.hajda@xxxxxxxxx>
> ---
> 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));
>
> return 0;
>
