[PATCH v2 0/4] drm: Restrict GEM's pin/unpin to PRIME

[PATCH v2 0/4] drm: Restrict GEM's pin/unpin to PRIME

[Thomas Zimmermann]

The pin and unpin callbacks in struct drm_gem_object_funcs are
for PRIME exported dma-bufs. Remove the pin calls in the client
code, drop the unnecessary pin callbacks from gem-vram and inline
drm_gem_pin() into the only remaining caller. Do the equivalent
for drm_gem_unpin().

AFAICT the long-term plan is to switch to dynamic dma-buf imports
and remove pin/unpin from GEM object functions.

v2:
- fix coding style and comments (Dmitry)

Thomas Zimmermann (4):
  drm/client: Do not pin in drm_client_buffer_vmap()
  drm/gem-vram: Do not set pin and unpin callbacks
  drm/gem-vram: Un-export pin helpers
  drm/gem: Inline drm_gem_pin() into PRIME helpers

 drivers/gpu/drm/drm_client.c          | 35 ++----------
 drivers/gpu/drm/drm_gem.c             | 32 -----------
 drivers/gpu/drm/drm_gem_vram_helper.c | 79 +--------------------------
 drivers/gpu/drm/drm_internal.h        |  4 --
 drivers/gpu/drm/drm_prime.c           | 22 +++++++-
 include/drm/drm_gem.h                 |  3 +-
 include/drm/drm_gem_vram_helper.h     |  2 -
 7 files changed, 30 insertions(+), 147 deletions(-)


base-commit: 842c3c276c106040f9b96d72b9df35ed6aed9ae9
-- 
2.49.0

[PATCH v2 1/4] drm/client: Do not pin in drm_client_buffer_vmap()

[Thomas Zimmermann]

Pin and vmap are two distict operations. Do not mix them.

The helper drm_client_buffer_vmap() maps the pages for fbdev-dma
and fbdev-shmem. In both cases, the vmap operation ensures that
the pages are available until the vunmap happens. And as the pages
in DMA or SHMEM areas cannot be moved, there is no reason to call
pin. Hence remove the pin call.

Update drm_client_buffer_vunmap() accordingly.

v2:
- call 'locked' variants of GEM helpers (Dmitry)

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/gpu/drm/drm_client.c | 35 ++++++-----------------------------
 1 file changed, 6 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
index f1de7faf9fb4..f7197a07c045 100644
--- a/drivers/gpu/drm/drm_client.c
+++ b/drivers/gpu/drm/drm_client.c
@@ -303,34 +303,17 @@ EXPORT_SYMBOL(drm_client_buffer_vunmap_local);
  * Returns:
  *	0 on success, or a negative errno code otherwise.
  */
-int
-drm_client_buffer_vmap(struct drm_client_buffer *buffer,
-		       struct iosys_map *map_copy)
+int drm_client_buffer_vmap(struct drm_client_buffer *buffer,
+			   struct iosys_map *map_copy)
 {
-	struct drm_gem_object *gem = buffer->gem;
-	struct iosys_map *map = &buffer->map;
 	int ret;
 
-	drm_gem_lock(gem);
-
-	ret = drm_gem_pin_locked(gem);
-	if (ret)
-		goto err_drm_gem_pin_locked;
-	ret = drm_gem_vmap_locked(gem, map);
+	ret = drm_gem_vmap(buffer->gem, &buffer->map);
 	if (ret)
-		goto err_drm_gem_vmap;
-
-	drm_gem_unlock(gem);
-
-	*map_copy = *map;
+		return ret;
+	*map_copy = buffer->map;
 
 	return 0;
-
-err_drm_gem_vmap:
-	drm_gem_unpin_locked(buffer->gem);
-err_drm_gem_pin_locked:
-	drm_gem_unlock(gem);
-	return ret;
 }
 EXPORT_SYMBOL(drm_client_buffer_vmap);
 
@@ -344,13 +327,7 @@ EXPORT_SYMBOL(drm_client_buffer_vmap);
  */
 void drm_client_buffer_vunmap(struct drm_client_buffer *buffer)
 {
-	struct drm_gem_object *gem = buffer->gem;
-	struct iosys_map *map = &buffer->map;
-
-	drm_gem_lock(gem);
-	drm_gem_vunmap_locked(gem, map);
-	drm_gem_unpin_locked(gem);
-	drm_gem_unlock(gem);
+	drm_gem_vunmap(buffer->gem, &buffer->map);
 }
 EXPORT_SYMBOL(drm_client_buffer_vunmap);
 
-- 
2.49.0

[PATCH v2 2/4] drm/gem-vram: Do not set pin and unpin callbacks

[Thomas Zimmermann]

Gem-vram helpers do not support PRIME dma-buf sharing. So nothing
will ever call pin/unpin on its buffer objects. Do not set these
callbacks in struct drm_gem_object_funcs.

v2:
- fix typo in commit description

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 drivers/gpu/drm/drm_gem_vram_helper.c | 37 ---------------------------
 1 file changed, 37 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
index 22b1fe9c03b8..c73480d31836 100644
--- a/drivers/gpu/drm/drm_gem_vram_helper.c
+++ b/drivers/gpu/drm/drm_gem_vram_helper.c
@@ -689,41 +689,6 @@ EXPORT_SYMBOL(drm_gem_vram_plane_helper_cleanup_fb);
  * PRIME helpers
  */
 
-/**
- * drm_gem_vram_object_pin() - Implements &struct drm_gem_object_funcs.pin
- * @gem:	The GEM object to pin
- *
- * Returns:
- * 0 on success, or
- * a negative errno code otherwise.
- */
-static int drm_gem_vram_object_pin(struct drm_gem_object *gem)
-{
-	struct drm_gem_vram_object *gbo = drm_gem_vram_of_gem(gem);
-
-	/*
-	 * Fbdev console emulation is the use case of these PRIME
-	 * helpers. This may involve updating a hardware buffer from
-	 * a shadow FB. We pin the buffer to it's current location
-	 * (either video RAM or system memory) to prevent it from
-	 * being relocated during the update operation. If you require
-	 * the buffer to be pinned to VRAM, implement a callback that
-	 * sets the flags accordingly.
-	 */
-	return drm_gem_vram_pin_locked(gbo, 0);
-}
-
-/**
- * drm_gem_vram_object_unpin() - Implements &struct drm_gem_object_funcs.unpin
- * @gem:	The GEM object to unpin
- */
-static void drm_gem_vram_object_unpin(struct drm_gem_object *gem)
-{
-	struct drm_gem_vram_object *gbo = drm_gem_vram_of_gem(gem);
-
-	drm_gem_vram_unpin_locked(gbo);
-}
-
 /**
  * drm_gem_vram_object_vmap() -
  *	Implements &struct drm_gem_object_funcs.vmap
@@ -762,8 +727,6 @@ static void drm_gem_vram_object_vunmap(struct drm_gem_object *gem,
 
 static const struct drm_gem_object_funcs drm_gem_vram_object_funcs = {
 	.free	= drm_gem_vram_object_free,
-	.pin	= drm_gem_vram_object_pin,
-	.unpin	= drm_gem_vram_object_unpin,
 	.vmap	= drm_gem_vram_object_vmap,
 	.vunmap	= drm_gem_vram_object_vunmap,
 	.mmap   = drm_gem_ttm_mmap,
-- 
2.49.0

[PATCH v2 3/4] drm/gem-vram: Un-export pin helpers

[Thomas Zimmermann]

There are no external callers of the gem-vram pin helpers. Hence
unexport them.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 drivers/gpu/drm/drm_gem_vram_helper.c | 42 ++-------------------------
 include/drm/drm_gem_vram_helper.h     |  2 --
 2 files changed, 2 insertions(+), 42 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
index c73480d31836..ead50fef5e7d 100644
--- a/drivers/gpu/drm/drm_gem_vram_helper.c
+++ b/drivers/gpu/drm/drm_gem_vram_helper.c
@@ -88,11 +88,6 @@ static const struct drm_gem_object_funcs drm_gem_vram_object_funcs;
  * drmm_vram_helper_init() is a managed interface that installs a
  * clean-up handler to run during the DRM device's release.
  *
- * For drawing or scanout operations, rsp. buffer objects have to be pinned
- * in video RAM. Call drm_gem_vram_pin() with &DRM_GEM_VRAM_PL_FLAG_VRAM or
- * &DRM_GEM_VRAM_PL_FLAG_SYSTEM to pin a buffer object in video RAM or system
- * memory. Call drm_gem_vram_unpin() to release the pinned object afterwards.
- *
  * A buffer object that is pinned in video RAM has a fixed address within that
  * memory region. Call drm_gem_vram_offset() to retrieve this value. Typically
  * it's used to program the hardware's scanout engine for framebuffers, set
@@ -299,30 +294,7 @@ static int drm_gem_vram_pin_locked(struct drm_gem_vram_object *gbo,
 	return 0;
 }
 
-/**
- * drm_gem_vram_pin() - Pins a GEM VRAM object in a region.
- * @gbo:	the GEM VRAM object
- * @pl_flag:	a bitmask of possible memory regions
- *
- * Pinning a buffer object ensures that it is not evicted from
- * a memory region. A pinned buffer object has to be unpinned before
- * it can be pinned to another region. If the pl_flag argument is 0,
- * the buffer is pinned at its current location (video RAM or system
- * memory).
- *
- * Small buffer objects, such as cursor images, can lead to memory
- * fragmentation if they are pinned in the middle of video RAM. This
- * is especially a problem on devices with only a small amount of
- * video RAM. Fragmentation can prevent the primary framebuffer from
- * fitting in, even though there's enough memory overall. The modifier
- * DRM_GEM_VRAM_PL_FLAG_TOPDOWN marks the buffer object to be pinned
- * at the high end of the memory region to avoid fragmentation.
- *
- * Returns:
- * 0 on success, or
- * a negative error code otherwise.
- */
-int drm_gem_vram_pin(struct drm_gem_vram_object *gbo, unsigned long pl_flag)
+static int drm_gem_vram_pin(struct drm_gem_vram_object *gbo, unsigned long pl_flag)
 {
 	int ret;
 
@@ -334,7 +306,6 @@ int drm_gem_vram_pin(struct drm_gem_vram_object *gbo, unsigned long pl_flag)
 
 	return ret;
 }
-EXPORT_SYMBOL(drm_gem_vram_pin);
 
 static void drm_gem_vram_unpin_locked(struct drm_gem_vram_object *gbo)
 {
@@ -343,15 +314,7 @@ static void drm_gem_vram_unpin_locked(struct drm_gem_vram_object *gbo)
 	ttm_bo_unpin(&gbo->bo);
 }
 
-/**
- * drm_gem_vram_unpin() - Unpins a GEM VRAM object
- * @gbo:	the GEM VRAM object
- *
- * Returns:
- * 0 on success, or
- * a negative error code otherwise.
- */
-int drm_gem_vram_unpin(struct drm_gem_vram_object *gbo)
+static int drm_gem_vram_unpin(struct drm_gem_vram_object *gbo)
 {
 	int ret;
 
@@ -364,7 +327,6 @@ int drm_gem_vram_unpin(struct drm_gem_vram_object *gbo)
 
 	return 0;
 }
-EXPORT_SYMBOL(drm_gem_vram_unpin);
 
 /**
  * drm_gem_vram_vmap() - Pins and maps a GEM VRAM object into kernel address
diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h
index 00830b49a3ff..2dd42bed679d 100644
--- a/include/drm/drm_gem_vram_helper.h
+++ b/include/drm/drm_gem_vram_helper.h
@@ -94,8 +94,6 @@ struct drm_gem_vram_object *drm_gem_vram_create(struct drm_device *dev,
 						unsigned long pg_align);
 void drm_gem_vram_put(struct drm_gem_vram_object *gbo);
 s64 drm_gem_vram_offset(struct drm_gem_vram_object *gbo);
-int drm_gem_vram_pin(struct drm_gem_vram_object *gbo, unsigned long pl_flag);
-int drm_gem_vram_unpin(struct drm_gem_vram_object *gbo);
 int drm_gem_vram_vmap(struct drm_gem_vram_object *gbo, struct iosys_map *map);
 void drm_gem_vram_vunmap(struct drm_gem_vram_object *gbo,
 			 struct iosys_map *map);
-- 
2.49.0

[PATCH v2 4/4] drm/gem: Inline drm_gem_pin() into PRIME helpers

[Thomas Zimmermann]

Inline drm_gem_pin() into its only caller drm_gem_map_attach()
and update the documentation in the callback's purpose. Do the
equivalent for drm_gem_unpin(). Also add stricter error checking
on the involved locking.

The pin operation in the GEM object functions is a helper for
PRIME-exported buffer objects. Having drm_gem_pin() gives the
impression of a general-purpose interface, which is not the case.
Removing it makes the pin callback a bit harder to misuse.

v2:
- clarify comment on pin callback (Dmitry)

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 drivers/gpu/drm/drm_gem.c      | 32 --------------------------------
 drivers/gpu/drm/drm_internal.h |  4 ----
 drivers/gpu/drm/drm_prime.c    | 22 ++++++++++++++++++++--
 include/drm/drm_gem.h          |  3 ++-
 4 files changed, 22 insertions(+), 39 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 1e659d2660f7..a0a3b6baa569 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -1184,38 +1184,6 @@ void drm_gem_print_info(struct drm_printer *p, unsigned int indent,
 		obj->funcs->print_info(p, indent, obj);
 }
 
-int drm_gem_pin_locked(struct drm_gem_object *obj)
-{
-	if (obj->funcs->pin)
-		return obj->funcs->pin(obj);
-
-	return 0;
-}
-
-void drm_gem_unpin_locked(struct drm_gem_object *obj)
-{
-	if (obj->funcs->unpin)
-		obj->funcs->unpin(obj);
-}
-
-int drm_gem_pin(struct drm_gem_object *obj)
-{
-	int ret;
-
-	dma_resv_lock(obj->resv, NULL);
-	ret = drm_gem_pin_locked(obj);
-	dma_resv_unlock(obj->resv);
-
-	return ret;
-}
-
-void drm_gem_unpin(struct drm_gem_object *obj)
-{
-	dma_resv_lock(obj->resv, NULL);
-	drm_gem_unpin_locked(obj);
-	dma_resv_unlock(obj->resv);
-}
-
 int drm_gem_vmap_locked(struct drm_gem_object *obj, struct iosys_map *map)
 {
 	int ret;
diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
index e44f28fd81d3..442eb31351dd 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -175,10 +175,6 @@ void drm_gem_release(struct drm_device *dev, struct drm_file *file_private);
 void drm_gem_print_info(struct drm_printer *p, unsigned int indent,
 			const struct drm_gem_object *obj);
 
-int drm_gem_pin_locked(struct drm_gem_object *obj);
-void drm_gem_unpin_locked(struct drm_gem_object *obj);
-int drm_gem_pin(struct drm_gem_object *obj);
-void drm_gem_unpin(struct drm_gem_object *obj);
 int drm_gem_vmap_locked(struct drm_gem_object *obj, struct iosys_map *map);
 void drm_gem_vunmap_locked(struct drm_gem_object *obj, struct iosys_map *map);
 
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index d828502268b8..a1852c02f512 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -599,6 +599,7 @@ int drm_gem_map_attach(struct dma_buf *dma_buf,
 		       struct dma_buf_attachment *attach)
 {
 	struct drm_gem_object *obj = dma_buf->priv;
+	int ret;
 
 	/*
 	 * drm_gem_map_dma_buf() requires obj->get_sg_table(), but drivers
@@ -608,7 +609,16 @@ int drm_gem_map_attach(struct dma_buf *dma_buf,
 	    !obj->funcs->get_sg_table)
 		return -ENOSYS;
 
-	return drm_gem_pin(obj);
+	if (!obj->funcs->pin)
+		return 0;
+
+	ret = dma_resv_lock(obj->resv, NULL);
+	if (ret)
+		return ret;
+	ret = obj->funcs->pin(obj);
+	dma_resv_unlock(obj->resv);
+
+	return ret;
 }
 EXPORT_SYMBOL(drm_gem_map_attach);
 
@@ -625,8 +635,16 @@ void drm_gem_map_detach(struct dma_buf *dma_buf,
 			struct dma_buf_attachment *attach)
 {
 	struct drm_gem_object *obj = dma_buf->priv;
+	int ret;
 
-	drm_gem_unpin(obj);
+	if (!obj->funcs->unpin)
+		return;
+
+	ret = dma_resv_lock(obj->resv, NULL);
+	if (drm_WARN_ON(obj->dev, ret))
+		return;
+	obj->funcs->unpin(obj);
+	dma_resv_unlock(obj->resv);
 }
 EXPORT_SYMBOL(drm_gem_map_detach);
 
diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
index a3133a08267c..1a79ec3fe45c 100644
--- a/include/drm/drm_gem.h
+++ b/include/drm/drm_gem.h
@@ -126,7 +126,8 @@ struct drm_gem_object_funcs {
 	/**
 	 * @pin:
 	 *
-	 * Pin backing buffer in memory. Used by the drm_gem_map_attach() helper.
+	 * Pin backing buffer in memory, such that dma-buf importers can
+	 * access it. Used by the drm_gem_map_attach() helper.
 	 *
 	 * This callback is optional.
 	 */
-- 
2.49.0

Re: [PATCH v2 1/4] drm/client: Do not pin in drm_client_buffer_vmap()

[Dmitry Osipenko]

On 5/26/25 16:25, Thomas Zimmermann wrote:
> Pin and vmap are two distict operations. Do not mix them.
> 
> The helper drm_client_buffer_vmap() maps the pages for fbdev-dma
> and fbdev-shmem. In both cases, the vmap operation ensures that
> the pages are available until the vunmap happens. And as the pages
> in DMA or SHMEM areas cannot be moved, there is no reason to call
> pin. Hence remove the pin call.
> 
> Update drm_client_buffer_vunmap() accordingly.
> 
> v2:
> - call 'locked' variants of GEM helpers (Dmitry)
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
>  drivers/gpu/drm/drm_client.c | 35 ++++++-----------------------------
>  1 file changed, 6 insertions(+), 29 deletions(-)

Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>

-- 
Best regards,
Dmitry