[PATCH 0/5] drm/shmem-helper: Follow-up on 'Switch to reservation lock'

[PATCH 0/5] drm/shmem-helper: Follow-up on 'Switch to reservation lock'

[Boris Brezillon]

Hello,

As mentioned here [1], after rebasing some of my work on
drm-misc-next this morning I noticed that the
drm_gem_shmem_get_pages() I was using to pin pages to a GEM no longer
exists, so I ended up looking at 21aa27ddc582 ("drm/shmem-helper: Switch
to reservation lock") and came up with a few changes to help clarify
the situation.

Note that we will soon need to have drm_gem_shmem_[un]pin_locked()
exposed for the PowerVR and new Mali drivers so we can pin memory
after we've acquired the GEM locks using drm_exec. Not entirely sure
if this should take the form of some generic
drm_gem_[un]pin[_unlocked]() helpers like we have for v[un]map()
operations, or if this should stay shmem-specific.

Regards,

Boris

[1]https://patchwork.freedesktop.org/patch/539994/

Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Emil Velikov <emil.l.velikov@gmail.com>
Cc: Dmitry Osipenko <dmitry.osipenko@collabora.com>

Boris Brezillon (5):
  drm/panfrost: Stop using drm_gem_shmem_put_pages()
  drm/shmem-helper: Stop exposing drm_gem_shmem_put_pages()
  drm/shmem-helper: Inline drm_gem_shmem_{get,put}_pages()
  drm/shmem-helper: Make dma_resv_assert_held() unconditional in
    drm_gem_shmem_v[un]map()
  drm/shmem-helper: Clarify drm_gem_shmem_v[un]map() usage

 drivers/gpu/drm/drm_gem_shmem_helper.c  | 125 +++++++++++-------------
 drivers/gpu/drm/panfrost/panfrost_mmu.c |  13 ++-
 include/drm/drm_gem_shmem_helper.h      |   1 -
 3 files changed, 64 insertions(+), 75 deletions(-)

-- 
2.41.0

[PATCH 1/5] drm/panfrost: Stop using drm_gem_shmem_put_pages()

[Boris Brezillon]

We want to get rid of this helper function, so let's use
drm_gem_shmem_unpin() and move this call out of the
dma_resv-locked section.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Emil Velikov <emil.l.velikov@gmail.com>
Cc: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Cc: Rob Herring <robh@kernel.org>
Cc: Steven Price <steven.price@arm.com>
---
 drivers/gpu/drm/panfrost/panfrost_mmu.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
index c0123d09f699..0b12f03ef0be 100644
--- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
@@ -447,6 +447,7 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
 	pgoff_t page_offset;
 	struct sg_table *sgt;
 	struct page **pages;
+	bool pinned = false;
 
 	bomapping = addr_to_mapping(pfdev, as, addr);
 	if (!bomapping)
@@ -488,12 +489,14 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
 		}
 		bo->base.pages = pages;
 		bo->base.pages_use_count = 1;
+		pinned = true;
 	} else {
 		pages = bo->base.pages;
 		if (pages[page_offset]) {
 			/* Pages are already mapped, bail out. */
 			goto out;
 		}
+		pinned = true;
 	}
 
 	mapping = bo->base.base.filp->f_mapping;
@@ -504,7 +507,7 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
 		if (IS_ERR(pages[i])) {
 			ret = PTR_ERR(pages[i]);
 			pages[i] = NULL;
-			goto err_pages;
+			goto err_unlock;
 		}
 	}
 
@@ -512,7 +515,7 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
 	ret = sg_alloc_table_from_pages(sgt, pages + page_offset,
 					NUM_FAULT_PAGES, 0, SZ_2M, GFP_KERNEL);
 	if (ret)
-		goto err_pages;
+		goto err_unlock;
 
 	ret = dma_map_sgtable(pfdev->dev, sgt, DMA_BIDIRECTIONAL, 0);
 	if (ret)
@@ -534,10 +537,12 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
 
 err_map:
 	sg_free_table(sgt);
-err_pages:
-	drm_gem_shmem_put_pages(&bo->base);
 err_unlock:
 	dma_resv_unlock(obj->resv);
+
+	if (ret && pinned)
+		drm_gem_shmem_unpin(&bo->base);
+
 err_bo:
 	panfrost_gem_mapping_put(bomapping);
 	return ret;
-- 
2.41.0

[PATCH 2/5] drm/shmem-helper: Stop exposing drm_gem_shmem_put_pages()

[Boris Brezillon]

The last user (panfrost) moved to drm_gem_shmem_unpin(), so it's now
safe to make this function private.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Emil Velikov <emil.l.velikov@gmail.com>
Cc: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 drivers/gpu/drm/drm_gem_shmem_helper.c | 5 +++--
 include/drm/drm_gem_shmem_helper.h     | 1 -
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
index a783d2245599..d6fc034164c0 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -128,6 +128,8 @@ struct drm_gem_shmem_object *drm_gem_shmem_create(struct drm_device *dev, size_t
 }
 EXPORT_SYMBOL_GPL(drm_gem_shmem_create);
 
+static void drm_gem_shmem_put_pages(struct drm_gem_shmem_object *shmem);
+
 /**
  * drm_gem_shmem_free - Free resources associated with a shmem GEM object
  * @shmem: shmem GEM object to free
@@ -204,7 +206,7 @@ static int drm_gem_shmem_get_pages(struct drm_gem_shmem_object *shmem)
  *
  * This function decreases the use count and puts the backing pages when use drops to zero.
  */
-void drm_gem_shmem_put_pages(struct drm_gem_shmem_object *shmem)
+static void drm_gem_shmem_put_pages(struct drm_gem_shmem_object *shmem)
 {
 	struct drm_gem_object *obj = &shmem->base;
 
@@ -226,7 +228,6 @@ void drm_gem_shmem_put_pages(struct drm_gem_shmem_object *shmem)
 			  shmem->pages_mark_accessed_on_put);
 	shmem->pages = NULL;
 }
-EXPORT_SYMBOL(drm_gem_shmem_put_pages);
 
 static int drm_gem_shmem_pin_locked(struct drm_gem_shmem_object *shmem)
 {
diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h
index 2867d2aba88b..f55f8739acc0 100644
--- a/include/drm/drm_gem_shmem_helper.h
+++ b/include/drm/drm_gem_shmem_helper.h
@@ -99,7 +99,6 @@ struct drm_gem_shmem_object {
 struct drm_gem_shmem_object *drm_gem_shmem_create(struct drm_device *dev, size_t size);
 void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem);
 
-void drm_gem_shmem_put_pages(struct drm_gem_shmem_object *shmem);
 int drm_gem_shmem_pin(struct drm_gem_shmem_object *shmem);
 void drm_gem_shmem_unpin(struct drm_gem_shmem_object *shmem);
 int drm_gem_shmem_vmap(struct drm_gem_shmem_object *shmem,
-- 
2.41.0

[PATCH 3/5] drm/shmem-helper: Inline drm_gem_shmem_{get,put}_pages()

[Boris Brezillon]

Move code drm_gem_shmem_{get,put}_pages() code to
drm_gem_shmem_{pin,unpin}_locked().

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Emil Velikov <emil.l.velikov@gmail.com>
Cc: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 drivers/gpu/drm/drm_gem_shmem_helper.c | 108 ++++++++++---------------
 1 file changed, 41 insertions(+), 67 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
index d6fc034164c0..f406556e42e0 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -128,46 +128,7 @@ struct drm_gem_shmem_object *drm_gem_shmem_create(struct drm_device *dev, size_t
 }
 EXPORT_SYMBOL_GPL(drm_gem_shmem_create);
 
-static void drm_gem_shmem_put_pages(struct drm_gem_shmem_object *shmem);
-
-/**
- * drm_gem_shmem_free - Free resources associated with a shmem GEM object
- * @shmem: shmem GEM object to free
- *
- * This function cleans up the GEM object state and frees the memory used to
- * store the object itself.
- */
-void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem)
-{
-	struct drm_gem_object *obj = &shmem->base;
-
-	if (obj->import_attach) {
-		drm_prime_gem_destroy(obj, shmem->sgt);
-	} else {
-		dma_resv_lock(shmem->base.resv, NULL);
-
-		drm_WARN_ON(obj->dev, shmem->vmap_use_count);
-
-		if (shmem->sgt) {
-			dma_unmap_sgtable(obj->dev->dev, shmem->sgt,
-					  DMA_BIDIRECTIONAL, 0);
-			sg_free_table(shmem->sgt);
-			kfree(shmem->sgt);
-		}
-		if (shmem->pages)
-			drm_gem_shmem_put_pages(shmem);
-
-		drm_WARN_ON(obj->dev, shmem->pages_use_count);
-
-		dma_resv_unlock(shmem->base.resv);
-	}
-
-	drm_gem_object_release(obj);
-	kfree(shmem);
-}
-EXPORT_SYMBOL_GPL(drm_gem_shmem_free);
-
-static int drm_gem_shmem_get_pages(struct drm_gem_shmem_object *shmem)
+static int drm_gem_shmem_pin_locked(struct drm_gem_shmem_object *shmem)
 {
 	struct drm_gem_object *obj = &shmem->base;
 	struct page **pages;
@@ -200,13 +161,7 @@ static int drm_gem_shmem_get_pages(struct drm_gem_shmem_object *shmem)
 	return 0;
 }
 
-/*
- * drm_gem_shmem_put_pages - Decrease use count on the backing pages for a shmem GEM object
- * @shmem: shmem GEM object
- *
- * This function decreases the use count and puts the backing pages when use drops to zero.
- */
-static void drm_gem_shmem_put_pages(struct drm_gem_shmem_object *shmem)
+static void drm_gem_shmem_unpin_locked(struct drm_gem_shmem_object *shmem)
 {
 	struct drm_gem_object *obj = &shmem->base;
 
@@ -229,23 +184,42 @@ static void drm_gem_shmem_put_pages(struct drm_gem_shmem_object *shmem)
 	shmem->pages = NULL;
 }
 
-static int drm_gem_shmem_pin_locked(struct drm_gem_shmem_object *shmem)
+/**
+ * drm_gem_shmem_free - Free resources associated with a shmem GEM object
+ * @shmem: shmem GEM object to free
+ *
+ * This function cleans up the GEM object state and frees the memory used to
+ * store the object itself.
+ */
+void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem)
 {
-	int ret;
+	struct drm_gem_object *obj = &shmem->base;
 
-	dma_resv_assert_held(shmem->base.resv);
+	if (obj->import_attach) {
+		drm_prime_gem_destroy(obj, shmem->sgt);
+	} else {
+		dma_resv_lock(shmem->base.resv, NULL);
 
-	ret = drm_gem_shmem_get_pages(shmem);
+		drm_WARN_ON(obj->dev, shmem->vmap_use_count);
 
-	return ret;
-}
-
-static void drm_gem_shmem_unpin_locked(struct drm_gem_shmem_object *shmem)
-{
-	dma_resv_assert_held(shmem->base.resv);
-
-	drm_gem_shmem_put_pages(shmem);
+		if (shmem->sgt) {
+			dma_unmap_sgtable(obj->dev->dev, shmem->sgt,
+					  DMA_BIDIRECTIONAL, 0);
+			sg_free_table(shmem->sgt);
+			kfree(shmem->sgt);
+		}
+		if (shmem->pages)
+			drm_gem_shmem_unpin_locked(shmem);
+
+		drm_WARN_ON(obj->dev, shmem->pages_use_count);
+
+		dma_resv_unlock(shmem->base.resv);
+	}
+
+	drm_gem_object_release(obj);
+	kfree(shmem);
 }
+EXPORT_SYMBOL_GPL(drm_gem_shmem_free);
 
 /**
  * drm_gem_shmem_pin - Pin backing pages for a shmem GEM object
@@ -332,7 +306,7 @@ int drm_gem_shmem_vmap(struct drm_gem_shmem_object *shmem,
 			return 0;
 		}
 
-		ret = drm_gem_shmem_get_pages(shmem);
+		ret = drm_gem_shmem_pin_locked(shmem);
 		if (ret)
 			goto err_zero_use;
 
@@ -355,7 +329,7 @@ int drm_gem_shmem_vmap(struct drm_gem_shmem_object *shmem,
 
 err_put_pages:
 	if (!obj->import_attach)
-		drm_gem_shmem_put_pages(shmem);
+		drm_gem_shmem_unpin_locked(shmem);
 err_zero_use:
 	shmem->vmap_use_count = 0;
 
@@ -392,7 +366,7 @@ void drm_gem_shmem_vunmap(struct drm_gem_shmem_object *shmem,
 			return;
 
 		vunmap(shmem->vaddr);
-		drm_gem_shmem_put_pages(shmem);
+		drm_gem_shmem_unpin_locked(shmem);
 	}
 
 	shmem->vaddr = NULL;
@@ -452,7 +426,7 @@ void drm_gem_shmem_purge(struct drm_gem_shmem_object *shmem)
 	kfree(shmem->sgt);
 	shmem->sgt = NULL;
 
-	drm_gem_shmem_put_pages(shmem);
+	drm_gem_shmem_unpin_locked(shmem);
 
 	shmem->madv = -1;
 
@@ -565,7 +539,7 @@ static void drm_gem_shmem_vm_close(struct vm_area_struct *vma)
 	struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
 
 	dma_resv_lock(shmem->base.resv, NULL);
-	drm_gem_shmem_put_pages(shmem);
+	drm_gem_shmem_unpin_locked(shmem);
 	dma_resv_unlock(shmem->base.resv);
 
 	drm_gem_vm_close(vma);
@@ -606,7 +580,7 @@ int drm_gem_shmem_mmap(struct drm_gem_shmem_object *shmem, struct vm_area_struct
 	}
 
 	dma_resv_lock(shmem->base.resv, NULL);
-	ret = drm_gem_shmem_get_pages(shmem);
+	ret = drm_gem_shmem_pin_locked(shmem);
 	dma_resv_unlock(shmem->base.resv);
 
 	if (ret)
@@ -674,7 +648,7 @@ static struct sg_table *drm_gem_shmem_get_pages_sgt_locked(struct drm_gem_shmem_
 
 	drm_WARN_ON(obj->dev, obj->import_attach);
 
-	ret = drm_gem_shmem_get_pages(shmem);
+	ret = drm_gem_shmem_pin_locked(shmem);
 	if (ret)
 		return ERR_PTR(ret);
 
@@ -696,7 +670,7 @@ static struct sg_table *drm_gem_shmem_get_pages_sgt_locked(struct drm_gem_shmem_
 	sg_free_table(sgt);
 	kfree(sgt);
 err_put_pages:
-	drm_gem_shmem_put_pages(shmem);
+	drm_gem_shmem_unpin_locked(shmem);
 	return ERR_PTR(ret);
 }
 
-- 
2.41.0

[PATCH 4/5] drm/shmem-helper: Make dma_resv_assert_held() unconditional in drm_gem_shmem_v[un]map()

[Boris Brezillon]

dma_resv lock should be held in both the dma_buf and native GEM case,
so let's just move the dma_resv_assert_held() check out of the !dma-buf
block.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Emil Velikov <emil.l.velikov@gmail.com>
Cc: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 drivers/gpu/drm/drm_gem_shmem_helper.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
index f406556e42e0..2b8a32f6b656 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -288,6 +288,8 @@ int drm_gem_shmem_vmap(struct drm_gem_shmem_object *shmem,
 	struct drm_gem_object *obj = &shmem->base;
 	int ret = 0;
 
+	dma_resv_assert_held(shmem->base.resv);
+
 	if (obj->import_attach) {
 		ret = dma_buf_vmap(obj->import_attach->dmabuf, map);
 		if (!ret) {
@@ -299,8 +301,6 @@ int drm_gem_shmem_vmap(struct drm_gem_shmem_object *shmem,
 	} else {
 		pgprot_t prot = PAGE_KERNEL;
 
-		dma_resv_assert_held(shmem->base.resv);
-
 		if (shmem->vmap_use_count++ > 0) {
 			iosys_map_set_vaddr(map, shmem->vaddr);
 			return 0;
@@ -354,11 +354,11 @@ void drm_gem_shmem_vunmap(struct drm_gem_shmem_object *shmem,
 {
 	struct drm_gem_object *obj = &shmem->base;
 
+	dma_resv_assert_held(shmem->base.resv);
+
 	if (obj->import_attach) {
 		dma_buf_vunmap(obj->import_attach->dmabuf, map);
 	} else {
-		dma_resv_assert_held(shmem->base.resv);
-
 		if (drm_WARN_ON_ONCE(obj->dev, !shmem->vmap_use_count))
 			return;
 
-- 
2.41.0

[PATCH 5/5] drm/shmem-helper: Clarify drm_gem_shmem_v[un]map() usage

[Boris Brezillon]

Drivers are not supposed to call these functions directly when they
want to map/unamp a GEM in kernel space. They should instead go
through drm_gem_v[un]map[_unlocked]() that will forward the request
to drm_gem_object_funcs::v[un]map() which in turn will call
drm_gem_shmem_v[un]map().

Let's clarify that in the functions doc.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Emil Velikov <emil.l.velikov@gmail.com>
Cc: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 drivers/gpu/drm/drm_gem_shmem_helper.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
index 2b8a32f6b656..daada172fe70 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -279,6 +279,11 @@ EXPORT_SYMBOL(drm_gem_shmem_unpin);
  *
  * Acquired mappings should be cleaned up by calling drm_gem_shmem_vunmap().
  *
+ * This function is not meant to be used directly, but rather used as a helper
+ * to implement driver-specific versions of drm_gem_object_funcs::vmap(). If
+ * you need to vmap() a GEM object from your driver, use
+ * drm_gem_vmap[_unlocked]() instead.
+ *
  * Returns:
  * 0 on success or a negative error code on failure.
  */
@@ -348,6 +353,11 @@ EXPORT_SYMBOL(drm_gem_shmem_vmap);
  *
  * This function hides the differences between dma-buf imported and natively
  * allocated objects.
+ *
+ * This function is not meant to be used directly, but rather used as a helper
+ * to implement driver-specific versions of drm_gem_object_funcs::vunmap(). If
+ * you need to vunmap() a GEM object from your driver, use
+ * drm_gem_vunmap[_unlocked]() instead.
  */
 void drm_gem_shmem_vunmap(struct drm_gem_shmem_object *shmem,
 			  struct iosys_map *map)
-- 
2.41.0

Re: [PATCH 1/5] drm/panfrost: Stop using drm_gem_shmem_put_pages()

[Dmitry Osipenko]

On 6/26/23 15:02, Boris Brezillon wrote:
> -err_pages:
> -	drm_gem_shmem_put_pages(&bo->base);
>  err_unlock:
>  	dma_resv_unlock(obj->resv);
> +
> +	if (ret && pinned)
> +		drm_gem_shmem_unpin(&bo->base);

The drm_gem_shmem_unpin() was supposed to be used only in conjunction
with drm_gem_shmem_pin(). I've a pending patch to enable the pin/unpin
refcounting needed by drm-shmem shrinker, it will prohibit invocation of
unpin without a previous pin.

I'm wondering whether it will be okay to simply remove
drm_gem_shmem_put_pages() from the Panfrost code, letting pages to be
kept allocated in a error case. They will be freed once BO is destroyed.

-- 
Best regards,
Dmitry

Re: [PATCH 1/5] drm/panfrost: Stop using drm_gem_shmem_put_pages()

[Boris Brezillon]

On Mon, 26 Jun 2023 16:20:53 +0300
Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote:

> On 6/26/23 15:02, Boris Brezillon wrote:
> > -err_pages:
> > -	drm_gem_shmem_put_pages(&bo->base);
> >  err_unlock:
> >  	dma_resv_unlock(obj->resv);
> > +
> > +	if (ret && pinned)
> > +		drm_gem_shmem_unpin(&bo->base);  
> 
> The drm_gem_shmem_unpin() was supposed to be used only in conjunction
> with drm_gem_shmem_pin(). I've a pending patch to enable the pin/unpin
> refcounting needed by drm-shmem shrinker, it will prohibit invocation of
> unpin without a previous pin.

That driver is a bit special in that, in the growable BO case
(AKA pin-on-demand), the driver replaces the drm_gem_shmem_pin()
implementation by a custom one (the logic in
panfrost_mmu_map_fault_addr()), but still relies on the
default implementation to release things. We do increment the
pages_use_count manually to make sure the drm_gem_shmem_unpin() is
balanced.

> 
> I'm wondering whether it will be okay to simply remove
> drm_gem_shmem_put_pages() from the Panfrost code, letting pages to be
> kept allocated in a error case. They will be freed once BO is destroyed.
> 

I'm pretty sure the implementation will then complain about unbalanced
pin/unamp (or get_pages/put_pages) if we do that. I guess one option
would be to completely bypass drm_gem_shmem_[un]pin() for growable BOs
and manage the pages separately at the panfrost_gem_object level, but
the original idea was probably to re-use some of the fields/logic we
had in drm_gem_shmem_object and make partial pinning as close as
possible to regular pinning. Another option would be to teach the shmem
about partial pinning, but I'm not sure we want to expose such a
feature.

Re: [PATCH 3/5] drm/shmem-helper: Inline drm_gem_shmem_{get,put}_pages()

[Boris Brezillon]

On Mon, 26 Jun 2023 14:02:45 +0200
Boris Brezillon <boris.brezillon@collabora.com> wrote:

> Move code drm_gem_shmem_{get,put}_pages() code to
> drm_gem_shmem_{pin,unpin}_locked().

After having a closer look at 'Add generic memory shrinker to VirtIO-GPU
and  Panfrost DRM drivers', I realize that's not what we want. We must
differentiate hard-pinning (as in, can't be evicted until all users
give up the ref they have) and soft-pinning (users can survive a
swapout, basically userspace mappings created through
drm_gem_shmem_mmap()).

> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Emil Velikov <emil.l.velikov@gmail.com>
> Cc: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> ---
>  drivers/gpu/drm/drm_gem_shmem_helper.c | 108 ++++++++++---------------
>  1 file changed, 41 insertions(+), 67 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> index d6fc034164c0..f406556e42e0 100644
> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> @@ -128,46 +128,7 @@ struct drm_gem_shmem_object *drm_gem_shmem_create(struct drm_device *dev, size_t
>  }
>  EXPORT_SYMBOL_GPL(drm_gem_shmem_create);
>  
> -static void drm_gem_shmem_put_pages(struct drm_gem_shmem_object *shmem);
> -
> -/**
> - * drm_gem_shmem_free - Free resources associated with a shmem GEM object
> - * @shmem: shmem GEM object to free
> - *
> - * This function cleans up the GEM object state and frees the memory used to
> - * store the object itself.
> - */
> -void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem)
> -{
> -	struct drm_gem_object *obj = &shmem->base;
> -
> -	if (obj->import_attach) {
> -		drm_prime_gem_destroy(obj, shmem->sgt);
> -	} else {
> -		dma_resv_lock(shmem->base.resv, NULL);
> -
> -		drm_WARN_ON(obj->dev, shmem->vmap_use_count);
> -
> -		if (shmem->sgt) {
> -			dma_unmap_sgtable(obj->dev->dev, shmem->sgt,
> -					  DMA_BIDIRECTIONAL, 0);
> -			sg_free_table(shmem->sgt);
> -			kfree(shmem->sgt);
> -		}
> -		if (shmem->pages)
> -			drm_gem_shmem_put_pages(shmem);
> -
> -		drm_WARN_ON(obj->dev, shmem->pages_use_count);
> -
> -		dma_resv_unlock(shmem->base.resv);
> -	}
> -
> -	drm_gem_object_release(obj);
> -	kfree(shmem);
> -}
> -EXPORT_SYMBOL_GPL(drm_gem_shmem_free);
> -
> -static int drm_gem_shmem_get_pages(struct drm_gem_shmem_object *shmem)
> +static int drm_gem_shmem_pin_locked(struct drm_gem_shmem_object *shmem)
>  {
>  	struct drm_gem_object *obj = &shmem->base;
>  	struct page **pages;
> @@ -200,13 +161,7 @@ static int drm_gem_shmem_get_pages(struct drm_gem_shmem_object *shmem)
>  	return 0;
>  }
>  
> -/*
> - * drm_gem_shmem_put_pages - Decrease use count on the backing pages for a shmem GEM object
> - * @shmem: shmem GEM object
> - *
> - * This function decreases the use count and puts the backing pages when use drops to zero.
> - */
> -static void drm_gem_shmem_put_pages(struct drm_gem_shmem_object *shmem)
> +static void drm_gem_shmem_unpin_locked(struct drm_gem_shmem_object *shmem)
>  {
>  	struct drm_gem_object *obj = &shmem->base;
>  
> @@ -229,23 +184,42 @@ static void drm_gem_shmem_put_pages(struct drm_gem_shmem_object *shmem)
>  	shmem->pages = NULL;
>  }
>  
> -static int drm_gem_shmem_pin_locked(struct drm_gem_shmem_object *shmem)
> +/**
> + * drm_gem_shmem_free - Free resources associated with a shmem GEM object
> + * @shmem: shmem GEM object to free
> + *
> + * This function cleans up the GEM object state and frees the memory used to
> + * store the object itself.
> + */
> +void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem)
>  {
> -	int ret;
> +	struct drm_gem_object *obj = &shmem->base;
>  
> -	dma_resv_assert_held(shmem->base.resv);
> +	if (obj->import_attach) {
> +		drm_prime_gem_destroy(obj, shmem->sgt);
> +	} else {
> +		dma_resv_lock(shmem->base.resv, NULL);
>  
> -	ret = drm_gem_shmem_get_pages(shmem);
> +		drm_WARN_ON(obj->dev, shmem->vmap_use_count);
>  
> -	return ret;
> -}
> -
> -static void drm_gem_shmem_unpin_locked(struct drm_gem_shmem_object *shmem)
> -{
> -	dma_resv_assert_held(shmem->base.resv);
> -
> -	drm_gem_shmem_put_pages(shmem);
> +		if (shmem->sgt) {
> +			dma_unmap_sgtable(obj->dev->dev, shmem->sgt,
> +					  DMA_BIDIRECTIONAL, 0);
> +			sg_free_table(shmem->sgt);
> +			kfree(shmem->sgt);
> +		}
> +		if (shmem->pages)
> +			drm_gem_shmem_unpin_locked(shmem);
> +
> +		drm_WARN_ON(obj->dev, shmem->pages_use_count);
> +
> +		dma_resv_unlock(shmem->base.resv);
> +	}
> +
> +	drm_gem_object_release(obj);
> +	kfree(shmem);
>  }
> +EXPORT_SYMBOL_GPL(drm_gem_shmem_free);
>  
>  /**
>   * drm_gem_shmem_pin - Pin backing pages for a shmem GEM object
> @@ -332,7 +306,7 @@ int drm_gem_shmem_vmap(struct drm_gem_shmem_object *shmem,
>  			return 0;
>  		}
>  
> -		ret = drm_gem_shmem_get_pages(shmem);
> +		ret = drm_gem_shmem_pin_locked(shmem);
>  		if (ret)
>  			goto err_zero_use;
>  
> @@ -355,7 +329,7 @@ int drm_gem_shmem_vmap(struct drm_gem_shmem_object *shmem,
>  
>  err_put_pages:
>  	if (!obj->import_attach)
> -		drm_gem_shmem_put_pages(shmem);
> +		drm_gem_shmem_unpin_locked(shmem);
>  err_zero_use:
>  	shmem->vmap_use_count = 0;
>  
> @@ -392,7 +366,7 @@ void drm_gem_shmem_vunmap(struct drm_gem_shmem_object *shmem,
>  			return;
>  
>  		vunmap(shmem->vaddr);
> -		drm_gem_shmem_put_pages(shmem);
> +		drm_gem_shmem_unpin_locked(shmem);
>  	}
>  
>  	shmem->vaddr = NULL;
> @@ -452,7 +426,7 @@ void drm_gem_shmem_purge(struct drm_gem_shmem_object *shmem)
>  	kfree(shmem->sgt);
>  	shmem->sgt = NULL;
>  
> -	drm_gem_shmem_put_pages(shmem);
> +	drm_gem_shmem_unpin_locked(shmem);
>  
>  	shmem->madv = -1;
>  
> @@ -565,7 +539,7 @@ static void drm_gem_shmem_vm_close(struct vm_area_struct *vma)
>  	struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
>  
>  	dma_resv_lock(shmem->base.resv, NULL);
> -	drm_gem_shmem_put_pages(shmem);
> +	drm_gem_shmem_unpin_locked(shmem);
>  	dma_resv_unlock(shmem->base.resv);
>  
>  	drm_gem_vm_close(vma);
> @@ -606,7 +580,7 @@ int drm_gem_shmem_mmap(struct drm_gem_shmem_object *shmem, struct vm_area_struct
>  	}
>  
>  	dma_resv_lock(shmem->base.resv, NULL);
> -	ret = drm_gem_shmem_get_pages(shmem);
> +	ret = drm_gem_shmem_pin_locked(shmem);
>  	dma_resv_unlock(shmem->base.resv);
>  
>  	if (ret)
> @@ -674,7 +648,7 @@ static struct sg_table *drm_gem_shmem_get_pages_sgt_locked(struct drm_gem_shmem_
>  
>  	drm_WARN_ON(obj->dev, obj->import_attach);
>  
> -	ret = drm_gem_shmem_get_pages(shmem);
> +	ret = drm_gem_shmem_pin_locked(shmem);
>  	if (ret)
>  		return ERR_PTR(ret);
>  
> @@ -696,7 +670,7 @@ static struct sg_table *drm_gem_shmem_get_pages_sgt_locked(struct drm_gem_shmem_
>  	sg_free_table(sgt);
>  	kfree(sgt);
>  err_put_pages:
> -	drm_gem_shmem_put_pages(shmem);
> +	drm_gem_shmem_unpin_locked(shmem);
>  	return ERR_PTR(ret);
>  }
>  

Re: [PATCH 1/5] drm/panfrost: Stop using drm_gem_shmem_put_pages()

[Boris Brezillon]

On Mon, 26 Jun 2023 16:20:53 +0300
Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote:

> On 6/26/23 15:02, Boris Brezillon wrote:
> > -err_pages:
> > -	drm_gem_shmem_put_pages(&bo->base);
> >  err_unlock:
> >  	dma_resv_unlock(obj->resv);
> > +
> > +	if (ret && pinned)
> > +		drm_gem_shmem_unpin(&bo->base);  
> 
> The drm_gem_shmem_unpin() was supposed to be used only in conjunction
> with drm_gem_shmem_pin(). I've a pending patch to enable the pin/unpin
> refcounting needed by drm-shmem shrinker, it will prohibit invocation of
> unpin without a previous pin.
> 
> I'm wondering whether it will be okay to simply remove
> drm_gem_shmem_put_pages() from the Panfrost code, letting pages to be
> kept allocated in a error case. They will be freed once BO is destroyed.
> 

Okay, so after looking at your shmem-shrinker series, I confirm we need
to take a pin ref here (hard-pin), otherwise the buffer might be
evicted before the GPU is done, especially after you drop gpu_usecount
and use only pin_count to check whether a GEM object can be evicted or
not.

Re: [PATCH 1/5] drm/panfrost: Stop using drm_gem_shmem_put_pages()

[Dmitry Osipenko]

On 6/26/23 18:43, Boris Brezillon wrote:
> On Mon, 26 Jun 2023 16:20:53 +0300
> Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote:
> 
>> On 6/26/23 15:02, Boris Brezillon wrote:
>>> -err_pages:
>>> -	drm_gem_shmem_put_pages(&bo->base);
>>>  err_unlock:
>>>  	dma_resv_unlock(obj->resv);
>>> +
>>> +	if (ret && pinned)
>>> +		drm_gem_shmem_unpin(&bo->base);  
>>
>> The drm_gem_shmem_unpin() was supposed to be used only in conjunction
>> with drm_gem_shmem_pin(). I've a pending patch to enable the pin/unpin
>> refcounting needed by drm-shmem shrinker, it will prohibit invocation of
>> unpin without a previous pin.
>>
>> I'm wondering whether it will be okay to simply remove
>> drm_gem_shmem_put_pages() from the Panfrost code, letting pages to be
>> kept allocated in a error case. They will be freed once BO is destroyed.
>>
> 
> Okay, so after looking at your shmem-shrinker series, I confirm we need
> to take a pin ref here (hard-pin), otherwise the buffer might be
> evicted before the GPU is done, especially after you drop gpu_usecount
> and use only pin_count to check whether a GEM object can be evicted or
> not.

See the drm_gem_evict() [1], it checks whether GEM is busy, preventing
BO eviction while it is in-use by GPU. Note that in case of Panfrost,
shrinker isn't enabled for growable BOs.

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/gpu/drm/drm_gem.c?h=next-20230626#n1495

-- 
Best regards,
Dmitry

Re: [PATCH 1/5] drm/panfrost: Stop using drm_gem_shmem_put_pages()

[Boris Brezillon]

On Mon, 26 Jun 2023 19:06:55 +0300
Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote:

> On 6/26/23 18:43, Boris Brezillon wrote:
> > On Mon, 26 Jun 2023 16:20:53 +0300
> > Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote:
> >   
> >> On 6/26/23 15:02, Boris Brezillon wrote:  
> >>> -err_pages:
> >>> -	drm_gem_shmem_put_pages(&bo->base);
> >>>  err_unlock:
> >>>  	dma_resv_unlock(obj->resv);
> >>> +
> >>> +	if (ret && pinned)
> >>> +		drm_gem_shmem_unpin(&bo->base);    
> >>
> >> The drm_gem_shmem_unpin() was supposed to be used only in conjunction
> >> with drm_gem_shmem_pin(). I've a pending patch to enable the pin/unpin
> >> refcounting needed by drm-shmem shrinker, it will prohibit invocation of
> >> unpin without a previous pin.
> >>
> >> I'm wondering whether it will be okay to simply remove
> >> drm_gem_shmem_put_pages() from the Panfrost code, letting pages to be
> >> kept allocated in a error case. They will be freed once BO is destroyed.
> >>  
> > 
> > Okay, so after looking at your shmem-shrinker series, I confirm we need
> > to take a pin ref here (hard-pin), otherwise the buffer might be
> > evicted before the GPU is done, especially after you drop gpu_usecount
> > and use only pin_count to check whether a GEM object can be evicted or
> > not.  
> 
> See the drm_gem_evict() [1], it checks whether GEM is busy, preventing
> BO eviction while it is in-use by GPU. Note that in case of Panfrost,
> shrinker isn't enabled for growable BOs.

Okay, we should be good then, sorry for the confusion.