diff mbox series

[10/13] drm/fbdev-generic: Fix locking with drm_client_buffer_vmap_local()

Message ID 20240227113853.8464-11-tzimmermann@suse.de
State Accepted
Commit b4b0193e83cb987143583e2b4011b35331f429bd
Headers show
Series drm: Fix reservation locking for pin/unpin and console | expand

Commit Message

Thomas Zimmermann Feb. 27, 2024, 10:14 a.m. UTC
Temporarily lock the fbdev buffer object during updates to prevent
memory managers from evicting/moving the buffer. Moving a buffer
object while update its content results in undefined behaviour.

Fbdev-generic updates its buffer object from a shadow buffer. Gem-shmem
and gem-dma helpers do not move buffer objects, so they are safe to be
used with fbdev-generic. Gem-vram and qxl are based on TTM, but pin
buffer objects are part of the vmap operation. So both are also safe
to be used with fbdev-generic.

Amdgpu and nouveau do not pin or lock the buffer object during an
update. Their TTM-based memory management could move the buffer object
while the update is ongoing.

The new vmap_local and vunmap_local helpers hold the buffer object's
reservation lock during the buffer update. This prevents moving the
buffer object on all memory managers.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_client.c        | 68 +++++++++++++++++++++++++----
 drivers/gpu/drm/drm_fbdev_generic.c |  4 +-
 drivers/gpu/drm/drm_gem.c           | 12 +++++
 include/drm/drm_client.h            | 10 +++++
 include/drm/drm_gem.h               |  3 ++
 5 files changed, 87 insertions(+), 10 deletions(-)

Comments

Sui Jingfeng March 11, 2024, 10:36 p.m. UTC | #1
Hi,


On 2024/2/27 18:14, Thomas Zimmermann wrote:
> Temporarily lock the fbdev buffer object during updates to prevent
> memory managers from evicting/moving the buffer. Moving a buffer
> object while update its content results in undefined behaviour.
>
> Fbdev-generic updates its buffer object from a shadow buffer. Gem-shmem
> and gem-dma helpers do not move buffer objects, so they are safe to be
> used with fbdev-generic. Gem-vram and qxl are based on TTM, but pin
> buffer objects are part of the vmap operation. So both are also safe
> to be used with fbdev-generic.
>
> Amdgpu and nouveau do not pin or lock the buffer object during an
> update. Their TTM-based memory management could move the buffer object
> while the update is ongoing.
>
> The new vmap_local and vunmap_local helpers hold the buffer object's
> reservation lock during the buffer update. This prevents moving the
> buffer object on all memory managers.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>   drivers/gpu/drm/drm_client.c        | 68 +++++++++++++++++++++++++----
>   drivers/gpu/drm/drm_fbdev_generic.c |  4 +-
>   drivers/gpu/drm/drm_gem.c           | 12 +++++
>   include/drm/drm_client.h            | 10 +++++
>   include/drm/drm_gem.h               |  3 ++
>   5 files changed, 87 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
> index 9403b3f576f7b..2cc81831236b5 100644
> --- a/drivers/gpu/drm/drm_client.c
> +++ b/drivers/gpu/drm/drm_client.c
> @@ -304,6 +304,66 @@ drm_client_buffer_create(struct drm_client_dev *client, u32 width, u32 height,
>   	return ERR_PTR(ret);
>   }
>   
> +/**
> + * drm_client_buffer_vmap_local - Map DRM client buffer into address space
> + * @buffer: DRM client buffer
> + * @map_copy: Returns the mapped memory's address
> + *
> + * This function maps a client buffer into kernel address space. If the
> + * buffer is already mapped, it returns the existing mapping's address.
> + *
> + * Client buffer mappings are not ref'counted. Each call to
> + * drm_client_buffer_vmap_local() should be closely followed by a call to
> + * drm_client_buffer_vunmap_local(). See drm_client_buffer_vmap() for
> + * long-term mappings.
> + *
> + * The returned address is a copy of the internal value. In contrast to
> + * other vmap interfaces, you don't need it for the client's vunmap
> + * function. So you can modify it at will during blit and draw operations.
> + *
> + * Returns:
> + *	0 on success, or a negative errno code otherwise.
> + */
> +int drm_client_buffer_vmap_local(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_vmap(gem, map);
> +	if (ret)
> +		goto err_drm_gem_vmap_unlocked;
> +	*map_copy = *map;
> +
> +	return 0;
> +
> +err_drm_gem_vmap_unlocked:
> +	drm_gem_unlock(gem);
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_client_buffer_vmap_local);
> +
> +/**
> + * drm_client_buffer_vunmap_local - Unmap DRM client buffer
> + * @buffer: DRM client buffer
> + *
> + * This function removes a client buffer's memory mapping established
> + * with drm_client_buffer_vunmap_local(). Calling this function is only
> + * required by clients that manage their buffer mappings by themselves.
> + */
> +void drm_client_buffer_vunmap_local(struct drm_client_buffer *buffer)
> +{
> +	struct drm_gem_object *gem = buffer->gem;
> +	struct iosys_map *map = &buffer->map;
> +
> +	drm_gem_vunmap(gem, map);
> +	drm_gem_unlock(gem);
> +}
> +EXPORT_SYMBOL(drm_client_buffer_vunmap_local);
> +
>   /**
>    * drm_client_buffer_vmap - Map DRM client buffer into address space
>    * @buffer: DRM client buffer
> @@ -331,14 +391,6 @@ drm_client_buffer_vmap(struct drm_client_buffer *buffer,
>   	struct iosys_map *map = &buffer->map;
>   	int ret;
>   
> -	/*
> -	 * FIXME: The dependency on GEM here isn't required, we could
> -	 * convert the driver handle to a dma-buf instead and use the
> -	 * backend-agnostic dma-buf vmap support instead. This would
> -	 * require that the handle2fd prime ioctl is reworked to pull the
> -	 * fd_install step out of the driver backend hooks, to make that
> -	 * final step optional for internal users.
> -	 */
>   	ret = drm_gem_vmap_unlocked(buffer->gem, map);
>   	if (ret)
>   		return ret;
> diff --git a/drivers/gpu/drm/drm_fbdev_generic.c b/drivers/gpu/drm/drm_fbdev_generic.c
> index d647d89764cb9..be357f926faec 100644
> --- a/drivers/gpu/drm/drm_fbdev_generic.c
> +++ b/drivers/gpu/drm/drm_fbdev_generic.c
> @@ -197,14 +197,14 @@ static int drm_fbdev_generic_damage_blit(struct drm_fb_helper *fb_helper,
>   	 */
>   	mutex_lock(&fb_helper->lock);
>   
> -	ret = drm_client_buffer_vmap(buffer, &map);
> +	ret = drm_client_buffer_vmap_local(buffer, &map);
>   	if (ret)
>   		goto out;
>   
>   	dst = map;

Then, please remove the local variable 'dst' (struct iosys_map) at here.
As you said, the returned iosys_map is another copy of the original backup,
we can play with this local variable at will, there no need to duplicate
another time again.

I have modified and tested with fbdev generic, no problem. With this trivial
issue resolved. For fbdev-generic:


Acked-by: Sui Jingfeng <sui.jingfeng@linux.dev>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
index 9403b3f576f7b..2cc81831236b5 100644
--- a/drivers/gpu/drm/drm_client.c
+++ b/drivers/gpu/drm/drm_client.c
@@ -304,6 +304,66 @@  drm_client_buffer_create(struct drm_client_dev *client, u32 width, u32 height,
 	return ERR_PTR(ret);
 }
 
+/**
+ * drm_client_buffer_vmap_local - Map DRM client buffer into address space
+ * @buffer: DRM client buffer
+ * @map_copy: Returns the mapped memory's address
+ *
+ * This function maps a client buffer into kernel address space. If the
+ * buffer is already mapped, it returns the existing mapping's address.
+ *
+ * Client buffer mappings are not ref'counted. Each call to
+ * drm_client_buffer_vmap_local() should be closely followed by a call to
+ * drm_client_buffer_vunmap_local(). See drm_client_buffer_vmap() for
+ * long-term mappings.
+ *
+ * The returned address is a copy of the internal value. In contrast to
+ * other vmap interfaces, you don't need it for the client's vunmap
+ * function. So you can modify it at will during blit and draw operations.
+ *
+ * Returns:
+ *	0 on success, or a negative errno code otherwise.
+ */
+int drm_client_buffer_vmap_local(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_vmap(gem, map);
+	if (ret)
+		goto err_drm_gem_vmap_unlocked;
+	*map_copy = *map;
+
+	return 0;
+
+err_drm_gem_vmap_unlocked:
+	drm_gem_unlock(gem);
+	return 0;
+}
+EXPORT_SYMBOL(drm_client_buffer_vmap_local);
+
+/**
+ * drm_client_buffer_vunmap_local - Unmap DRM client buffer
+ * @buffer: DRM client buffer
+ *
+ * This function removes a client buffer's memory mapping established
+ * with drm_client_buffer_vunmap_local(). Calling this function is only
+ * required by clients that manage their buffer mappings by themselves.
+ */
+void drm_client_buffer_vunmap_local(struct drm_client_buffer *buffer)
+{
+	struct drm_gem_object *gem = buffer->gem;
+	struct iosys_map *map = &buffer->map;
+
+	drm_gem_vunmap(gem, map);
+	drm_gem_unlock(gem);
+}
+EXPORT_SYMBOL(drm_client_buffer_vunmap_local);
+
 /**
  * drm_client_buffer_vmap - Map DRM client buffer into address space
  * @buffer: DRM client buffer
@@ -331,14 +391,6 @@  drm_client_buffer_vmap(struct drm_client_buffer *buffer,
 	struct iosys_map *map = &buffer->map;
 	int ret;
 
-	/*
-	 * FIXME: The dependency on GEM here isn't required, we could
-	 * convert the driver handle to a dma-buf instead and use the
-	 * backend-agnostic dma-buf vmap support instead. This would
-	 * require that the handle2fd prime ioctl is reworked to pull the
-	 * fd_install step out of the driver backend hooks, to make that
-	 * final step optional for internal users.
-	 */
 	ret = drm_gem_vmap_unlocked(buffer->gem, map);
 	if (ret)
 		return ret;
diff --git a/drivers/gpu/drm/drm_fbdev_generic.c b/drivers/gpu/drm/drm_fbdev_generic.c
index d647d89764cb9..be357f926faec 100644
--- a/drivers/gpu/drm/drm_fbdev_generic.c
+++ b/drivers/gpu/drm/drm_fbdev_generic.c
@@ -197,14 +197,14 @@  static int drm_fbdev_generic_damage_blit(struct drm_fb_helper *fb_helper,
 	 */
 	mutex_lock(&fb_helper->lock);
 
-	ret = drm_client_buffer_vmap(buffer, &map);
+	ret = drm_client_buffer_vmap_local(buffer, &map);
 	if (ret)
 		goto out;
 
 	dst = map;
 	drm_fbdev_generic_damage_blit_real(fb_helper, clip, &dst);
 
-	drm_client_buffer_vunmap(buffer);
+	drm_client_buffer_vunmap_local(buffer);
 
 out:
 	mutex_unlock(&fb_helper->lock);
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index e0f80c6a7096f..d4bbc5d109c8b 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -1227,6 +1227,18 @@  void drm_gem_vunmap(struct drm_gem_object *obj, struct iosys_map *map)
 }
 EXPORT_SYMBOL(drm_gem_vunmap);
 
+void drm_gem_lock(struct drm_gem_object *obj)
+{
+	dma_resv_lock(obj->resv, NULL);
+}
+EXPORT_SYMBOL(drm_gem_lock);
+
+void drm_gem_unlock(struct drm_gem_object *obj)
+{
+	dma_resv_unlock(obj->resv);
+}
+EXPORT_SYMBOL(drm_gem_unlock);
+
 int drm_gem_vmap_unlocked(struct drm_gem_object *obj, struct iosys_map *map)
 {
 	int ret;
diff --git a/include/drm/drm_client.h b/include/drm/drm_client.h
index d47458ecdac46..bc0e66f9c4251 100644
--- a/include/drm/drm_client.h
+++ b/include/drm/drm_client.h
@@ -141,6 +141,13 @@  struct drm_client_buffer {
 
 	/**
 	 * @gem: GEM object backing this buffer
+	 *
+	 * FIXME: The dependency on GEM here isn't required, we could
+	 * convert the driver handle to a dma-buf instead and use the
+	 * backend-agnostic dma-buf vmap support instead. This would
+	 * require that the handle2fd prime ioctl is reworked to pull the
+	 * fd_install step out of the driver backend hooks, to make that
+	 * final step optional for internal users.
 	 */
 	struct drm_gem_object *gem;
 
@@ -159,6 +166,9 @@  struct drm_client_buffer *
 drm_client_framebuffer_create(struct drm_client_dev *client, u32 width, u32 height, u32 format);
 void drm_client_framebuffer_delete(struct drm_client_buffer *buffer);
 int drm_client_framebuffer_flush(struct drm_client_buffer *buffer, struct drm_rect *rect);
+int drm_client_buffer_vmap_local(struct drm_client_buffer *buffer,
+				 struct iosys_map *map_copy);
+void drm_client_buffer_vunmap_local(struct drm_client_buffer *buffer);
 int drm_client_buffer_vmap(struct drm_client_buffer *buffer,
 			   struct iosys_map *map);
 void drm_client_buffer_vunmap(struct drm_client_buffer *buffer);
diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
index 2ebec3984cd44..bae4865b2101a 100644
--- a/include/drm/drm_gem.h
+++ b/include/drm/drm_gem.h
@@ -527,6 +527,9 @@  struct page **drm_gem_get_pages(struct drm_gem_object *obj);
 void drm_gem_put_pages(struct drm_gem_object *obj, struct page **pages,
 		bool dirty, bool accessed);
 
+void drm_gem_lock(struct drm_gem_object *obj);
+void drm_gem_unlock(struct drm_gem_object *obj);
+
 int drm_gem_vmap_unlocked(struct drm_gem_object *obj, struct iosys_map *map);
 void drm_gem_vunmap_unlocked(struct drm_gem_object *obj, struct iosys_map *map);