[10/13] drm/fbdev-generic: Fix locking with drm_client_buffer_vmap_local()
Sui Jingfeng
sui.jingfeng at linux.dev
Mon Mar 11 22:36:31 UTC 2024
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 at 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 at linux.dev>
More information about the amd-gfx
mailing list