| Message ID | 20221020151047.369354-1-matthew.auld@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: use i915_sg_dma_sizes() for internal backend | expand |
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@intel.com> > > Signed-off-by: Matthew Auld <matthew.auld@intel.com> > Cc: Andrzej Hajda <andrzej.hajda@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)); > > return 0; >
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@intel.com> > Cc: Andrzej Hajda <andrzej.hajda@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; >
Hi Matthew, On 20.10.2022 17: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@intel.com> > Cc: Andrzej Hajda <andrzej.hajda@intel.com> The patch looks OK, but it still does not solve dmar errors in hugepages test [1]. [1]: https://gitlab.freedesktop.org/drm/intel/-/issues/5278 Anyway: Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com> Regards Andrzej > --- > 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; >
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;
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@intel.com> Cc: Andrzej Hajda <andrzej.hajda@intel.com> --- drivers/gpu/drm/i915/gem/i915_gem_internal.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)