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@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));
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;
