drivers/gpu/drm/i915/gem/i915_gem_object.h | 3 +-- drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 23 +++++++++------------- drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 7 ++++--- 3 files changed, 14 insertions(+), 19 deletions(-)
When converting to folios the cleanup path of shmem_get_pages() was
missed. When a DMA remap fails and the max segment size is greater than
PAGE_SIZE it will attempt to retry the remap with a PAGE_SIZEd segment
size. The cleanup code isn't properly using the folio apis and as a
result isn't handling compound pages correctly.
v1 -> v2:
(Ville) Fixed locations where we were not clearing mapping unevictable.
Cc: stable@vger.kernel.org
Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: Vidya Srinivas <vidya.srinivas@intel.com>
Link: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13487
Link: https://lore.kernel.org/lkml/20250116135636.410164-1-bgeffon@google.com/
Fixes: 0b62af28f249 ("i915: convert shmem_sg_free_table() to use a folio_batch")
Signed-off-by: Brian Geffon <bgeffon@google.com>
Suggested-by: Tomasz Figa <tfiga@google.com>
---
drivers/gpu/drm/i915/gem/i915_gem_object.h | 3 +--
drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 23 +++++++++-------------
drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 7 ++++---
3 files changed, 14 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h
index 3dc61cbd2e11..0f122a12d4a5 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
@@ -843,8 +843,7 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915, struct sg_table *st,
size_t size, struct intel_memory_region *mr,
struct address_space *mapping,
unsigned int max_segment);
-void shmem_sg_free_table(struct sg_table *st, struct address_space *mapping,
- bool dirty, bool backup);
+void shmem_sg_free_table(struct sg_table *st, bool dirty, bool backup);
void __shmem_writeback(size_t size, struct address_space *mapping);
#ifdef CONFIG_MMU_NOTIFIER
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
index fe69f2c8527d..b320d9dfd6d3 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
@@ -29,16 +29,13 @@ static void check_release_folio_batch(struct folio_batch *fbatch)
cond_resched();
}
-void shmem_sg_free_table(struct sg_table *st, struct address_space *mapping,
- bool dirty, bool backup)
+void shmem_sg_free_table(struct sg_table *st, bool dirty, bool backup)
{
struct sgt_iter sgt_iter;
struct folio_batch fbatch;
struct folio *last = NULL;
struct page *page;
- mapping_clear_unevictable(mapping);
-
folio_batch_init(&fbatch);
for_each_sgt_page(page, sgt_iter, st) {
struct folio *folio = page_folio(page);
@@ -180,10 +177,10 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915, struct sg_table *st,
return 0;
err_sg:
sg_mark_end(sg);
+ mapping_clear_unevictable(mapping);
if (sg != st->sgl) {
- shmem_sg_free_table(st, mapping, false, false);
+ shmem_sg_free_table(st, false, false);
} else {
- mapping_clear_unevictable(mapping);
sg_free_table(st);
}
@@ -209,8 +206,6 @@ static int shmem_get_pages(struct drm_i915_gem_object *obj)
struct address_space *mapping = obj->base.filp->f_mapping;
unsigned int max_segment = i915_sg_segment_size(i915->drm.dev);
struct sg_table *st;
- struct sgt_iter sgt_iter;
- struct page *page;
int ret;
/*
@@ -239,9 +234,8 @@ static int shmem_get_pages(struct drm_i915_gem_object *obj)
* for PAGE_SIZE chunks instead may be helpful.
*/
if (max_segment > PAGE_SIZE) {
- for_each_sgt_page(page, sgt_iter, st)
- put_page(page);
- sg_free_table(st);
+ /* Leave the mapping unevictable while we retry */
+ shmem_sg_free_table(st, false, false);
kfree(st);
max_segment = PAGE_SIZE;
@@ -265,7 +259,8 @@ static int shmem_get_pages(struct drm_i915_gem_object *obj)
return 0;
err_pages:
- shmem_sg_free_table(st, mapping, false, false);
+ mapping_clear_unevictable(mapping);
+ shmem_sg_free_table(st, false, false);
/*
* shmemfs first checks if there is enough memory to allocate the page
* and reports ENOSPC should there be insufficient, along with the usual
@@ -402,8 +397,8 @@ void i915_gem_object_put_pages_shmem(struct drm_i915_gem_object *obj, struct sg_
if (i915_gem_object_needs_bit17_swizzle(obj))
i915_gem_object_save_bit_17_swizzle(obj, pages);
- shmem_sg_free_table(pages, file_inode(obj->base.filp)->i_mapping,
- obj->mm.dirty, obj->mm.madv == I915_MADV_WILLNEED);
+ mapping_clear_unevictable(file_inode(obj->base.filp)->i_mapping);
+ shmem_sg_free_table(pages, obj->mm.dirty, obj->mm.madv == I915_MADV_WILLNEED);
kfree(pages);
obj->mm.dirty = false;
}
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
index 10d8673641f7..37f51a04b838 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
@@ -232,7 +232,8 @@ static int i915_ttm_tt_shmem_populate(struct ttm_device *bdev,
return 0;
err_free_st:
- shmem_sg_free_table(st, filp->f_mapping, false, false);
+ mapping_clear_unevictable(filp->f_mapping);
+ shmem_sg_free_table(st, false, false);
return err;
}
@@ -243,8 +244,8 @@ static void i915_ttm_tt_shmem_unpopulate(struct ttm_tt *ttm)
bool backup = ttm->page_flags & TTM_TT_FLAG_SWAPPED;
struct sg_table *st = &i915_tt->cached_rsgt.table;
- shmem_sg_free_table(st, file_inode(i915_tt->filp)->i_mapping,
- backup, backup);
+ mapping_clear_unevictable(file_inode(i915_tt->filp)->i_mapping);
+ shmem_sg_free_table(st, backup, backup);
}
static void i915_ttm_tt_release(struct kref *ref)
--
2.48.0.rc2.279.g1de40edade-goog
On Thu, Jan 16, 2025 at 10:53:40AM -0500, Brian Geffon wrote: > When converting to folios the cleanup path of shmem_get_pages() was > missed. When a DMA remap fails and the max segment size is greater than > PAGE_SIZE it will attempt to retry the remap with a PAGE_SIZEd segment > size. The cleanup code isn't properly using the folio apis and as a > result isn't handling compound pages correctly. > > v1 -> v2: > (Ville) Fixed locations where we were not clearing mapping unevictable. > > Cc: stable@vger.kernel.org > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > Cc: Vidya Srinivas <vidya.srinivas@intel.com> > Link: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13487 > Link: https://lore.kernel.org/lkml/20250116135636.410164-1-bgeffon@google.com/ > Fixes: 0b62af28f249 ("i915: convert shmem_sg_free_table() to use a folio_batch") > Signed-off-by: Brian Geffon <bgeffon@google.com> > Suggested-by: Tomasz Figa <tfiga@google.com> > --- > drivers/gpu/drm/i915/gem/i915_gem_object.h | 3 +-- > drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 23 +++++++++------------- > drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 7 ++++--- > 3 files changed, 14 insertions(+), 19 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h > index 3dc61cbd2e11..0f122a12d4a5 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h > @@ -843,8 +843,7 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915, struct sg_table *st, > size_t size, struct intel_memory_region *mr, > struct address_space *mapping, > unsigned int max_segment); > -void shmem_sg_free_table(struct sg_table *st, struct address_space *mapping, > - bool dirty, bool backup); > +void shmem_sg_free_table(struct sg_table *st, bool dirty, bool backup); > void __shmem_writeback(size_t size, struct address_space *mapping); > > #ifdef CONFIG_MMU_NOTIFIER > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c > index fe69f2c8527d..b320d9dfd6d3 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c > @@ -29,16 +29,13 @@ static void check_release_folio_batch(struct folio_batch *fbatch) > cond_resched(); > } > > -void shmem_sg_free_table(struct sg_table *st, struct address_space *mapping, > - bool dirty, bool backup) > +void shmem_sg_free_table(struct sg_table *st, bool dirty, bool backup) This still makes the alloc vs. free completely asymmetric. This is not what we want because it just makes it very easy to make it mistake in the caller. I think the correct fix is to simply call the current shmem_sg_free_table() from the now broken failure path. mapping_{set,clear}_unevictable() just seems to be some bit operation so AFAICS the slight ping-pong should be inconsequential. > { > struct sgt_iter sgt_iter; > struct folio_batch fbatch; > struct folio *last = NULL; > struct page *page; > > - mapping_clear_unevictable(mapping); > - > folio_batch_init(&fbatch); > for_each_sgt_page(page, sgt_iter, st) { > struct folio *folio = page_folio(page); > @@ -180,10 +177,10 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915, struct sg_table *st, > return 0; > err_sg: > sg_mark_end(sg); > + mapping_clear_unevictable(mapping); > if (sg != st->sgl) { > - shmem_sg_free_table(st, mapping, false, false); > + shmem_sg_free_table(st, false, false); > } else { > - mapping_clear_unevictable(mapping); > sg_free_table(st); > } > > @@ -209,8 +206,6 @@ static int shmem_get_pages(struct drm_i915_gem_object *obj) > struct address_space *mapping = obj->base.filp->f_mapping; > unsigned int max_segment = i915_sg_segment_size(i915->drm.dev); > struct sg_table *st; > - struct sgt_iter sgt_iter; > - struct page *page; > int ret; > > /* > @@ -239,9 +234,8 @@ static int shmem_get_pages(struct drm_i915_gem_object *obj) > * for PAGE_SIZE chunks instead may be helpful. > */ > if (max_segment > PAGE_SIZE) { > - for_each_sgt_page(page, sgt_iter, st) > - put_page(page); > - sg_free_table(st); > + /* Leave the mapping unevictable while we retry */ > + shmem_sg_free_table(st, false, false); > kfree(st); > > max_segment = PAGE_SIZE; > @@ -265,7 +259,8 @@ static int shmem_get_pages(struct drm_i915_gem_object *obj) > return 0; > > err_pages: > - shmem_sg_free_table(st, mapping, false, false); > + mapping_clear_unevictable(mapping); > + shmem_sg_free_table(st, false, false); > /* > * shmemfs first checks if there is enough memory to allocate the page > * and reports ENOSPC should there be insufficient, along with the usual > @@ -402,8 +397,8 @@ void i915_gem_object_put_pages_shmem(struct drm_i915_gem_object *obj, struct sg_ > if (i915_gem_object_needs_bit17_swizzle(obj)) > i915_gem_object_save_bit_17_swizzle(obj, pages); > > - shmem_sg_free_table(pages, file_inode(obj->base.filp)->i_mapping, > - obj->mm.dirty, obj->mm.madv == I915_MADV_WILLNEED); > + mapping_clear_unevictable(file_inode(obj->base.filp)->i_mapping); > + shmem_sg_free_table(pages, obj->mm.dirty, obj->mm.madv == I915_MADV_WILLNEED); > kfree(pages); > obj->mm.dirty = false; > } > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c > index 10d8673641f7..37f51a04b838 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c > @@ -232,7 +232,8 @@ static int i915_ttm_tt_shmem_populate(struct ttm_device *bdev, > return 0; > > err_free_st: > - shmem_sg_free_table(st, filp->f_mapping, false, false); > + mapping_clear_unevictable(filp->f_mapping); > + shmem_sg_free_table(st, false, false); > > return err; > } > @@ -243,8 +244,8 @@ static void i915_ttm_tt_shmem_unpopulate(struct ttm_tt *ttm) > bool backup = ttm->page_flags & TTM_TT_FLAG_SWAPPED; > struct sg_table *st = &i915_tt->cached_rsgt.table; > > - shmem_sg_free_table(st, file_inode(i915_tt->filp)->i_mapping, > - backup, backup); > + mapping_clear_unevictable(file_inode(i915_tt->filp)->i_mapping); > + shmem_sg_free_table(st, backup, backup); > } > > static void i915_ttm_tt_release(struct kref *ref) > -- > 2.48.0.rc2.279.g1de40edade-goog -- Ville Syrjälä Intel
On Mon, Jan 27, 2025 at 1:47 PM Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > On Thu, Jan 16, 2025 at 10:53:40AM -0500, Brian Geffon wrote: > > When converting to folios the cleanup path of shmem_get_pages() was > > missed. When a DMA remap fails and the max segment size is greater than > > PAGE_SIZE it will attempt to retry the remap with a PAGE_SIZEd segment > > size. The cleanup code isn't properly using the folio apis and as a > > result isn't handling compound pages correctly. > > > > v1 -> v2: > > (Ville) Fixed locations where we were not clearing mapping unevictable. > > > > Cc: stable@vger.kernel.org > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > > Cc: Vidya Srinivas <vidya.srinivas@intel.com> > > Link: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13487 > > Link: https://lore.kernel.org/lkml/20250116135636.410164-1-bgeffon@google.com/ > > Fixes: 0b62af28f249 ("i915: convert shmem_sg_free_table() to use a folio_batch") > > Signed-off-by: Brian Geffon <bgeffon@google.com> > > Suggested-by: Tomasz Figa <tfiga@google.com> > > --- > > drivers/gpu/drm/i915/gem/i915_gem_object.h | 3 +-- > > drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 23 +++++++++------------- > > drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 7 ++++--- > > 3 files changed, 14 insertions(+), 19 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h > > index 3dc61cbd2e11..0f122a12d4a5 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h > > @@ -843,8 +843,7 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915, struct sg_table *st, > > size_t size, struct intel_memory_region *mr, > > struct address_space *mapping, > > unsigned int max_segment); > > -void shmem_sg_free_table(struct sg_table *st, struct address_space *mapping, > > - bool dirty, bool backup); > > +void shmem_sg_free_table(struct sg_table *st, bool dirty, bool backup); > > void __shmem_writeback(size_t size, struct address_space *mapping); > > > > #ifdef CONFIG_MMU_NOTIFIER > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c > > index fe69f2c8527d..b320d9dfd6d3 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c > > @@ -29,16 +29,13 @@ static void check_release_folio_batch(struct folio_batch *fbatch) > > cond_resched(); > > } > > > > -void shmem_sg_free_table(struct sg_table *st, struct address_space *mapping, > > - bool dirty, bool backup) > > +void shmem_sg_free_table(struct sg_table *st, bool dirty, bool backup) > > This still makes the alloc vs. free completely asymmetric. > This is not what we want because it just makes it very easy > to make it mistake in the caller. > > I think the correct fix is to simply call the current > shmem_sg_free_table() from the now broken failure path. > mapping_{set,clear}_unevictable() just seems to be some > bit operation so AFAICS the slight ping-pong should be > inconsequential. Ok, I guess that's true, it'll be set unevictable again in the call to shmem_sg_alloc_table() after it jumps back to rebuild_st. I'll send a v3 which should then become a 1 line change. > > > { > > struct sgt_iter sgt_iter; > > struct folio_batch fbatch; > > struct folio *last = NULL; > > struct page *page; > > > > - mapping_clear_unevictable(mapping); > > - > > folio_batch_init(&fbatch); > > for_each_sgt_page(page, sgt_iter, st) { > > struct folio *folio = page_folio(page); > > @@ -180,10 +177,10 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915, struct sg_table *st, > > return 0; > > err_sg: > > sg_mark_end(sg); > > + mapping_clear_unevictable(mapping); > > if (sg != st->sgl) { > > - shmem_sg_free_table(st, mapping, false, false); > > + shmem_sg_free_table(st, false, false); > > } else { > > - mapping_clear_unevictable(mapping); > > sg_free_table(st); > > } > > > > @@ -209,8 +206,6 @@ static int shmem_get_pages(struct drm_i915_gem_object *obj) > > struct address_space *mapping = obj->base.filp->f_mapping; > > unsigned int max_segment = i915_sg_segment_size(i915->drm.dev); > > struct sg_table *st; > > - struct sgt_iter sgt_iter; > > - struct page *page; > > int ret; > > > > /* > > @@ -239,9 +234,8 @@ static int shmem_get_pages(struct drm_i915_gem_object *obj) > > * for PAGE_SIZE chunks instead may be helpful. > > */ > > if (max_segment > PAGE_SIZE) { > > - for_each_sgt_page(page, sgt_iter, st) > > - put_page(page); > > - sg_free_table(st); > > + /* Leave the mapping unevictable while we retry */ > > + shmem_sg_free_table(st, false, false); > > kfree(st); > > > > max_segment = PAGE_SIZE; > > @@ -265,7 +259,8 @@ static int shmem_get_pages(struct drm_i915_gem_object *obj) > > return 0; > > > > err_pages: > > - shmem_sg_free_table(st, mapping, false, false); > > + mapping_clear_unevictable(mapping); > > + shmem_sg_free_table(st, false, false); > > /* > > * shmemfs first checks if there is enough memory to allocate the page > > * and reports ENOSPC should there be insufficient, along with the usual > > @@ -402,8 +397,8 @@ void i915_gem_object_put_pages_shmem(struct drm_i915_gem_object *obj, struct sg_ > > if (i915_gem_object_needs_bit17_swizzle(obj)) > > i915_gem_object_save_bit_17_swizzle(obj, pages); > > > > - shmem_sg_free_table(pages, file_inode(obj->base.filp)->i_mapping, > > - obj->mm.dirty, obj->mm.madv == I915_MADV_WILLNEED); > > + mapping_clear_unevictable(file_inode(obj->base.filp)->i_mapping); > > + shmem_sg_free_table(pages, obj->mm.dirty, obj->mm.madv == I915_MADV_WILLNEED); > > kfree(pages); > > obj->mm.dirty = false; > > } > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c > > index 10d8673641f7..37f51a04b838 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c > > @@ -232,7 +232,8 @@ static int i915_ttm_tt_shmem_populate(struct ttm_device *bdev, > > return 0; > > > > err_free_st: > > - shmem_sg_free_table(st, filp->f_mapping, false, false); > > + mapping_clear_unevictable(filp->f_mapping); > > + shmem_sg_free_table(st, false, false); > > > > return err; > > } > > @@ -243,8 +244,8 @@ static void i915_ttm_tt_shmem_unpopulate(struct ttm_tt *ttm) > > bool backup = ttm->page_flags & TTM_TT_FLAG_SWAPPED; > > struct sg_table *st = &i915_tt->cached_rsgt.table; > > > > - shmem_sg_free_table(st, file_inode(i915_tt->filp)->i_mapping, > > - backup, backup); > > + mapping_clear_unevictable(file_inode(i915_tt->filp)->i_mapping); > > + shmem_sg_free_table(st, backup, backup); > > } > > > > static void i915_ttm_tt_release(struct kref *ref) > > -- > > 2.48.0.rc2.279.g1de40edade-goog > > -- > Ville Syrjälä > Intel Thanks, Brian
Hello Brian, Many thanks for the fix. I am adding my tested-by. Tested-by: Vidya Srinivas <vidya.srinivas@intel.com> > -----Original Message----- > From: Brian Geffon <bgeffon@google.com> > Sent: 16 January 2025 21:24 > To: intel-gfx@lists.freedesktop.org > Cc: Wilson, Chris P <chris.p.wilson@intel.com>; Saarinen, Jani > <jani.saarinen@intel.com>; Mistat, Tomasz <tomasz.mistat@intel.com>; > Srinivas, Vidya <vidya.srinivas@intel.com>; ville.syrjala@linux.intel.com; > jani.nikula@linux.intel.com; linux-kernel@vger.kernel.org; dri- > devel@lists.freedesktop.org; Joonas Lahtinen > <joonas.lahtinen@linux.intel.com>; Brian Geffon <bgeffon@google.com>; > stable@vger.kernel.org; Tomasz Figa <tfiga@google.com> > Subject: [PATCH v2] drm/i915: Fix page cleanup on DMA remap failure > > When converting to folios the cleanup path of shmem_get_pages() was > missed. When a DMA remap fails and the max segment size is greater than > PAGE_SIZE it will attempt to retry the remap with a PAGE_SIZEd segment size. > The cleanup code isn't properly using the folio apis and as a result isn't > handling compound pages correctly. > > v1 -> v2: > (Ville) Fixed locations where we were not clearing mapping unevictable. > > Cc: stable@vger.kernel.org > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > Cc: Vidya Srinivas <vidya.srinivas@intel.com> > Link: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13487 > Link: https://lore.kernel.org/lkml/20250116135636.410164-1- > bgeffon@google.com/ > Fixes: 0b62af28f249 ("i915: convert shmem_sg_free_table() to use a > folio_batch") > Signed-off-by: Brian Geffon <bgeffon@google.com> > Suggested-by: Tomasz Figa <tfiga@google.com> > --- > drivers/gpu/drm/i915/gem/i915_gem_object.h | 3 +-- > drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 23 +++++++++------------- > drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 7 ++++--- > 3 files changed, 14 insertions(+), 19 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h > b/drivers/gpu/drm/i915/gem/i915_gem_object.h > index 3dc61cbd2e11..0f122a12d4a5 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h > @@ -843,8 +843,7 @@ int shmem_sg_alloc_table(struct drm_i915_private > *i915, struct sg_table *st, > size_t size, struct intel_memory_region *mr, > struct address_space *mapping, > unsigned int max_segment); > -void shmem_sg_free_table(struct sg_table *st, struct address_space > *mapping, > - bool dirty, bool backup); > +void shmem_sg_free_table(struct sg_table *st, bool dirty, bool backup); > void __shmem_writeback(size_t size, struct address_space *mapping); > > #ifdef CONFIG_MMU_NOTIFIER > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c > b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c > index fe69f2c8527d..b320d9dfd6d3 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c > @@ -29,16 +29,13 @@ static void check_release_folio_batch(struct > folio_batch *fbatch) > cond_resched(); > } > > -void shmem_sg_free_table(struct sg_table *st, struct address_space > *mapping, > - bool dirty, bool backup) > +void shmem_sg_free_table(struct sg_table *st, bool dirty, bool backup) > { > struct sgt_iter sgt_iter; > struct folio_batch fbatch; > struct folio *last = NULL; > struct page *page; > > - mapping_clear_unevictable(mapping); > - > folio_batch_init(&fbatch); > for_each_sgt_page(page, sgt_iter, st) { > struct folio *folio = page_folio(page); @@ -180,10 +177,10 > @@ int shmem_sg_alloc_table(struct drm_i915_private *i915, struct sg_table > *st, > return 0; > err_sg: > sg_mark_end(sg); > + mapping_clear_unevictable(mapping); > if (sg != st->sgl) { > - shmem_sg_free_table(st, mapping, false, false); > + shmem_sg_free_table(st, false, false); > } else { > - mapping_clear_unevictable(mapping); > sg_free_table(st); > } > > @@ -209,8 +206,6 @@ static int shmem_get_pages(struct > drm_i915_gem_object *obj) > struct address_space *mapping = obj->base.filp->f_mapping; > unsigned int max_segment = i915_sg_segment_size(i915->drm.dev); > struct sg_table *st; > - struct sgt_iter sgt_iter; > - struct page *page; > int ret; > > /* > @@ -239,9 +234,8 @@ static int shmem_get_pages(struct > drm_i915_gem_object *obj) > * for PAGE_SIZE chunks instead may be helpful. > */ > if (max_segment > PAGE_SIZE) { > - for_each_sgt_page(page, sgt_iter, st) > - put_page(page); > - sg_free_table(st); > + /* Leave the mapping unevictable while we retry */ > + shmem_sg_free_table(st, false, false); > kfree(st); > > max_segment = PAGE_SIZE; > @@ -265,7 +259,8 @@ static int shmem_get_pages(struct > drm_i915_gem_object *obj) > return 0; > > err_pages: > - shmem_sg_free_table(st, mapping, false, false); > + mapping_clear_unevictable(mapping); > + shmem_sg_free_table(st, false, false); > /* > * shmemfs first checks if there is enough memory to allocate the > page > * and reports ENOSPC should there be insufficient, along with the > usual @@ -402,8 +397,8 @@ void > i915_gem_object_put_pages_shmem(struct drm_i915_gem_object *obj, > struct sg_ > if (i915_gem_object_needs_bit17_swizzle(obj)) > i915_gem_object_save_bit_17_swizzle(obj, pages); > > - shmem_sg_free_table(pages, file_inode(obj->base.filp)->i_mapping, > - obj->mm.dirty, obj->mm.madv == > I915_MADV_WILLNEED); > + mapping_clear_unevictable(file_inode(obj->base.filp)->i_mapping); > + shmem_sg_free_table(pages, obj->mm.dirty, obj->mm.madv == > +I915_MADV_WILLNEED); > kfree(pages); > obj->mm.dirty = false; > } > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c > b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c > index 10d8673641f7..37f51a04b838 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c > @@ -232,7 +232,8 @@ static int i915_ttm_tt_shmem_populate(struct > ttm_device *bdev, > return 0; > > err_free_st: > - shmem_sg_free_table(st, filp->f_mapping, false, false); > + mapping_clear_unevictable(filp->f_mapping); > + shmem_sg_free_table(st, false, false); > > return err; > } > @@ -243,8 +244,8 @@ static void i915_ttm_tt_shmem_unpopulate(struct > ttm_tt *ttm) > bool backup = ttm->page_flags & TTM_TT_FLAG_SWAPPED; > struct sg_table *st = &i915_tt->cached_rsgt.table; > > - shmem_sg_free_table(st, file_inode(i915_tt->filp)->i_mapping, > - backup, backup); > + mapping_clear_unevictable(file_inode(i915_tt->filp)->i_mapping); > + shmem_sg_free_table(st, backup, backup); > } > > static void i915_ttm_tt_release(struct kref *ref) > -- > 2.48.0.rc2.279.g1de40edade-goog
On Wed, Jan 22, 2025 at 10:07 PM Srinivas, Vidya <vidya.srinivas@intel.com> wrote: > > Hello Brian, Many thanks for the fix. I am adding my tested-by. > Tested-by: Vidya Srinivas <vidya.srinivas@intel.com> Thanks for testing Vidya. Can we get a maintainer to take a look? > > > > -----Original Message----- > > From: Brian Geffon <bgeffon@google.com> > > Sent: 16 January 2025 21:24 > > To: intel-gfx@lists.freedesktop.org > > Cc: Wilson, Chris P <chris.p.wilson@intel.com>; Saarinen, Jani > > <jani.saarinen@intel.com>; Mistat, Tomasz <tomasz.mistat@intel.com>; > > Srinivas, Vidya <vidya.srinivas@intel.com>; ville.syrjala@linux.intel.com; > > jani.nikula@linux.intel.com; linux-kernel@vger.kernel.org; dri- > > devel@lists.freedesktop.org; Joonas Lahtinen > > <joonas.lahtinen@linux.intel.com>; Brian Geffon <bgeffon@google.com>; > > stable@vger.kernel.org; Tomasz Figa <tfiga@google.com> > > Subject: [PATCH v2] drm/i915: Fix page cleanup on DMA remap failure > > > > When converting to folios the cleanup path of shmem_get_pages() was > > missed. When a DMA remap fails and the max segment size is greater than > > PAGE_SIZE it will attempt to retry the remap with a PAGE_SIZEd segment size. > > The cleanup code isn't properly using the folio apis and as a result isn't > > handling compound pages correctly. > > > > v1 -> v2: > > (Ville) Fixed locations where we were not clearing mapping unevictable. > > > > Cc: stable@vger.kernel.org > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > > Cc: Vidya Srinivas <vidya.srinivas@intel.com> > > Link: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13487 > > Link: https://lore.kernel.org/lkml/20250116135636.410164-1- > > bgeffon@google.com/ > > Fixes: 0b62af28f249 ("i915: convert shmem_sg_free_table() to use a > > folio_batch") > > Signed-off-by: Brian Geffon <bgeffon@google.com> > > Suggested-by: Tomasz Figa <tfiga@google.com> > > --- > > drivers/gpu/drm/i915/gem/i915_gem_object.h | 3 +-- > > drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 23 +++++++++------------- > > drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 7 ++++--- > > 3 files changed, 14 insertions(+), 19 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h > > b/drivers/gpu/drm/i915/gem/i915_gem_object.h > > index 3dc61cbd2e11..0f122a12d4a5 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h > > @@ -843,8 +843,7 @@ int shmem_sg_alloc_table(struct drm_i915_private > > *i915, struct sg_table *st, > > size_t size, struct intel_memory_region *mr, > > struct address_space *mapping, > > unsigned int max_segment); > > -void shmem_sg_free_table(struct sg_table *st, struct address_space > > *mapping, > > - bool dirty, bool backup); > > +void shmem_sg_free_table(struct sg_table *st, bool dirty, bool backup); > > void __shmem_writeback(size_t size, struct address_space *mapping); > > > > #ifdef CONFIG_MMU_NOTIFIER > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c > > b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c > > index fe69f2c8527d..b320d9dfd6d3 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c > > @@ -29,16 +29,13 @@ static void check_release_folio_batch(struct > > folio_batch *fbatch) > > cond_resched(); > > } > > > > -void shmem_sg_free_table(struct sg_table *st, struct address_space > > *mapping, > > - bool dirty, bool backup) > > +void shmem_sg_free_table(struct sg_table *st, bool dirty, bool backup) > > { > > struct sgt_iter sgt_iter; > > struct folio_batch fbatch; > > struct folio *last = NULL; > > struct page *page; > > > > - mapping_clear_unevictable(mapping); > > - > > folio_batch_init(&fbatch); > > for_each_sgt_page(page, sgt_iter, st) { > > struct folio *folio = page_folio(page); @@ -180,10 +177,10 > > @@ int shmem_sg_alloc_table(struct drm_i915_private *i915, struct sg_table > > *st, > > return 0; > > err_sg: > > sg_mark_end(sg); > > + mapping_clear_unevictable(mapping); > > if (sg != st->sgl) { > > - shmem_sg_free_table(st, mapping, false, false); > > + shmem_sg_free_table(st, false, false); > > } else { > > - mapping_clear_unevictable(mapping); > > sg_free_table(st); > > } > > > > @@ -209,8 +206,6 @@ static int shmem_get_pages(struct > > drm_i915_gem_object *obj) > > struct address_space *mapping = obj->base.filp->f_mapping; > > unsigned int max_segment = i915_sg_segment_size(i915->drm.dev); > > struct sg_table *st; > > - struct sgt_iter sgt_iter; > > - struct page *page; > > int ret; > > > > /* > > @@ -239,9 +234,8 @@ static int shmem_get_pages(struct > > drm_i915_gem_object *obj) > > * for PAGE_SIZE chunks instead may be helpful. > > */ > > if (max_segment > PAGE_SIZE) { > > - for_each_sgt_page(page, sgt_iter, st) > > - put_page(page); > > - sg_free_table(st); > > + /* Leave the mapping unevictable while we retry */ > > + shmem_sg_free_table(st, false, false); > > kfree(st); > > > > max_segment = PAGE_SIZE; > > @@ -265,7 +259,8 @@ static int shmem_get_pages(struct > > drm_i915_gem_object *obj) > > return 0; > > > > err_pages: > > - shmem_sg_free_table(st, mapping, false, false); > > + mapping_clear_unevictable(mapping); > > + shmem_sg_free_table(st, false, false); > > /* > > * shmemfs first checks if there is enough memory to allocate the > > page > > * and reports ENOSPC should there be insufficient, along with the > > usual @@ -402,8 +397,8 @@ void > > i915_gem_object_put_pages_shmem(struct drm_i915_gem_object *obj, > > struct sg_ > > if (i915_gem_object_needs_bit17_swizzle(obj)) > > i915_gem_object_save_bit_17_swizzle(obj, pages); > > > > - shmem_sg_free_table(pages, file_inode(obj->base.filp)->i_mapping, > > - obj->mm.dirty, obj->mm.madv == > > I915_MADV_WILLNEED); > > + mapping_clear_unevictable(file_inode(obj->base.filp)->i_mapping); > > + shmem_sg_free_table(pages, obj->mm.dirty, obj->mm.madv == > > +I915_MADV_WILLNEED); > > kfree(pages); > > obj->mm.dirty = false; > > } > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c > > b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c > > index 10d8673641f7..37f51a04b838 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c > > @@ -232,7 +232,8 @@ static int i915_ttm_tt_shmem_populate(struct > > ttm_device *bdev, > > return 0; > > > > err_free_st: > > - shmem_sg_free_table(st, filp->f_mapping, false, false); > > + mapping_clear_unevictable(filp->f_mapping); > > + shmem_sg_free_table(st, false, false); > > > > return err; > > } > > @@ -243,8 +244,8 @@ static void i915_ttm_tt_shmem_unpopulate(struct > > ttm_tt *ttm) > > bool backup = ttm->page_flags & TTM_TT_FLAG_SWAPPED; > > struct sg_table *st = &i915_tt->cached_rsgt.table; > > > > - shmem_sg_free_table(st, file_inode(i915_tt->filp)->i_mapping, > > - backup, backup); > > + mapping_clear_unevictable(file_inode(i915_tt->filp)->i_mapping); > > + shmem_sg_free_table(st, backup, backup); > > } > > > > static void i915_ttm_tt_release(struct kref *ref) > > -- > > 2.48.0.rc2.279.g1de40edade-goog > Thanks Brian
> -----Original Message----- > From: Brian Geffon <bgeffon@google.com> > Sent: 24 January 2025 20:50 > To: Srinivas, Vidya <vidya.srinivas@intel.com> > Cc: intel-gfx@lists.freedesktop.org; Wilson, Chris P > <chris.p.wilson@intel.com>; Saarinen, Jani <jani.saarinen@intel.com>; > Mistat, Tomasz <tomasz.mistat@intel.com>; ville.syrjala@linux.intel.com; > jani.nikula@linux.intel.com; linux-kernel@vger.kernel.org; dri- > devel@lists.freedesktop.org; Joonas Lahtinen > <joonas.lahtinen@linux.intel.com>; stable@vger.kernel.org; Tomasz Figa > <tfiga@google.com> > Subject: Re: [PATCH v2] drm/i915: Fix page cleanup on DMA remap failure > > On Wed, Jan 22, 2025 at 10:07 PM Srinivas, Vidya <vidya.srinivas@intel.com> > wrote: > > > > Hello Brian, Many thanks for the fix. I am adding my tested-by. > > Tested-by: Vidya Srinivas <vidya.srinivas@intel.com> > > Thanks for testing Vidya. > > Can we get a maintainer to take a look? Hello Brian, Thank you so much. Tomasz has left a comment on gitlab, I think he will help on review. Regards Vidya > > > > > > > > -----Original Message----- > > > From: Brian Geffon <bgeffon@google.com> > > > Sent: 16 January 2025 21:24 > > > To: intel-gfx@lists.freedesktop.org > > > Cc: Wilson, Chris P <chris.p.wilson@intel.com>; Saarinen, Jani > > > <jani.saarinen@intel.com>; Mistat, Tomasz <tomasz.mistat@intel.com>; > > > Srinivas, Vidya <vidya.srinivas@intel.com>; > > > ville.syrjala@linux.intel.com; jani.nikula@linux.intel.com; > > > linux-kernel@vger.kernel.org; dri- devel@lists.freedesktop.org; > > > Joonas Lahtinen <joonas.lahtinen@linux.intel.com>; Brian Geffon > > > <bgeffon@google.com>; stable@vger.kernel.org; Tomasz Figa > > > <tfiga@google.com> > > > Subject: [PATCH v2] drm/i915: Fix page cleanup on DMA remap failure > > > > > > When converting to folios the cleanup path of shmem_get_pages() was > > > missed. When a DMA remap fails and the max segment size is greater > > > than PAGE_SIZE it will attempt to retry the remap with a PAGE_SIZEd > segment size. > > > The cleanup code isn't properly using the folio apis and as a result > > > isn't handling compound pages correctly. > > > > > > v1 -> v2: > > > (Ville) Fixed locations where we were not clearing mapping unevictable. > > > > > > Cc: stable@vger.kernel.org > > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > > > Cc: Vidya Srinivas <vidya.srinivas@intel.com> > > > Link: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13487 > > > Link: https://lore.kernel.org/lkml/20250116135636.410164-1- > > > bgeffon@google.com/ > > > Fixes: 0b62af28f249 ("i915: convert shmem_sg_free_table() to use a > > > folio_batch") > > > Signed-off-by: Brian Geffon <bgeffon@google.com> > > > Suggested-by: Tomasz Figa <tfiga@google.com> > > > --- > > > drivers/gpu/drm/i915/gem/i915_gem_object.h | 3 +-- > > > drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 23 +++++++++------------ > - > > > drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 7 ++++--- > > > 3 files changed, 14 insertions(+), 19 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h > > > b/drivers/gpu/drm/i915/gem/i915_gem_object.h > > > index 3dc61cbd2e11..0f122a12d4a5 100644 > > > --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h > > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h > > > @@ -843,8 +843,7 @@ int shmem_sg_alloc_table(struct > drm_i915_private > > > *i915, struct sg_table *st, > > > size_t size, struct intel_memory_region *mr, > > > struct address_space *mapping, > > > unsigned int max_segment); -void > > > shmem_sg_free_table(struct sg_table *st, struct address_space > > > *mapping, > > > - bool dirty, bool backup); > > > +void shmem_sg_free_table(struct sg_table *st, bool dirty, bool > > > +backup); > > > void __shmem_writeback(size_t size, struct address_space *mapping); > > > > > > #ifdef CONFIG_MMU_NOTIFIER > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c > > > b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c > > > index fe69f2c8527d..b320d9dfd6d3 100644 > > > --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c > > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c > > > @@ -29,16 +29,13 @@ static void check_release_folio_batch(struct > > > folio_batch *fbatch) > > > cond_resched(); > > > } > > > > > > -void shmem_sg_free_table(struct sg_table *st, struct address_space > > > *mapping, > > > - bool dirty, bool backup) > > > +void shmem_sg_free_table(struct sg_table *st, bool dirty, bool > > > +backup) > > > { > > > struct sgt_iter sgt_iter; > > > struct folio_batch fbatch; > > > struct folio *last = NULL; > > > struct page *page; > > > > > > - mapping_clear_unevictable(mapping); > > > - > > > folio_batch_init(&fbatch); > > > for_each_sgt_page(page, sgt_iter, st) { > > > struct folio *folio = page_folio(page); @@ -180,10 > > > +177,10 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915, > > > struct sg_table *st, > > > return 0; > > > err_sg: > > > sg_mark_end(sg); > > > + mapping_clear_unevictable(mapping); > > > if (sg != st->sgl) { > > > - shmem_sg_free_table(st, mapping, false, false); > > > + shmem_sg_free_table(st, false, false); > > > } else { > > > - mapping_clear_unevictable(mapping); > > > sg_free_table(st); > > > } > > > > > > @@ -209,8 +206,6 @@ static int shmem_get_pages(struct > > > drm_i915_gem_object *obj) > > > struct address_space *mapping = obj->base.filp->f_mapping; > > > unsigned int max_segment = i915_sg_segment_size(i915->drm.dev); > > > struct sg_table *st; > > > - struct sgt_iter sgt_iter; > > > - struct page *page; > > > int ret; > > > > > > /* > > > @@ -239,9 +234,8 @@ static int shmem_get_pages(struct > > > drm_i915_gem_object *obj) > > > * for PAGE_SIZE chunks instead may be helpful. > > > */ > > > if (max_segment > PAGE_SIZE) { > > > - for_each_sgt_page(page, sgt_iter, st) > > > - put_page(page); > > > - sg_free_table(st); > > > + /* Leave the mapping unevictable while we retry */ > > > + shmem_sg_free_table(st, false, false); > > > kfree(st); > > > > > > max_segment = PAGE_SIZE; @@ -265,7 +259,8 @@ > > > static int shmem_get_pages(struct drm_i915_gem_object *obj) > > > return 0; > > > > > > err_pages: > > > - shmem_sg_free_table(st, mapping, false, false); > > > + mapping_clear_unevictable(mapping); > > > + shmem_sg_free_table(st, false, false); > > > /* > > > * shmemfs first checks if there is enough memory to allocate > > > the page > > > * and reports ENOSPC should there be insufficient, along with > > > the usual @@ -402,8 +397,8 @@ void > > > i915_gem_object_put_pages_shmem(struct drm_i915_gem_object *obj, > > > struct sg_ > > > if (i915_gem_object_needs_bit17_swizzle(obj)) > > > i915_gem_object_save_bit_17_swizzle(obj, pages); > > > > > > - shmem_sg_free_table(pages, file_inode(obj->base.filp)->i_mapping, > > > - obj->mm.dirty, obj->mm.madv == > > > I915_MADV_WILLNEED); > > > + mapping_clear_unevictable(file_inode(obj->base.filp)->i_mapping); > > > + shmem_sg_free_table(pages, obj->mm.dirty, obj->mm.madv == > > > +I915_MADV_WILLNEED); > > > kfree(pages); > > > obj->mm.dirty = false; > > > } > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c > > > b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c > > > index 10d8673641f7..37f51a04b838 100644 > > > --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c > > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c > > > @@ -232,7 +232,8 @@ static int i915_ttm_tt_shmem_populate(struct > > > ttm_device *bdev, > > > return 0; > > > > > > err_free_st: > > > - shmem_sg_free_table(st, filp->f_mapping, false, false); > > > + mapping_clear_unevictable(filp->f_mapping); > > > + shmem_sg_free_table(st, false, false); > > > > > > return err; > > > } > > > @@ -243,8 +244,8 @@ static void i915_ttm_tt_shmem_unpopulate(struct > > > ttm_tt *ttm) > > > bool backup = ttm->page_flags & TTM_TT_FLAG_SWAPPED; > > > struct sg_table *st = &i915_tt->cached_rsgt.table; > > > > > > - shmem_sg_free_table(st, file_inode(i915_tt->filp)->i_mapping, > > > - backup, backup); > > > + mapping_clear_unevictable(file_inode(i915_tt->filp)->i_mapping); > > > + shmem_sg_free_table(st, backup, backup); > > > } > > > > > > static void i915_ttm_tt_release(struct kref *ref) > > > -- > > > 2.48.0.rc2.279.g1de40edade-goog > > > > Thanks > Brian
© 2016 - 2025 Red Hat, Inc.