Re: [PATCH 4/7] drm/client: Deprecate struct drm_client_buffer.gem

[Jocelyn Falempe <jfalempe@redhat.com>]

On 20/10/2025 17:04, Thomas Zimmermann wrote:
> The client buffer's framebuffer holds a reference and pointer on
> each of its GEM buffer objects. Thus the field gem in the client-
> buffer struct is not necessary. Deprecated the field and convert
> the client-buffer helpers to use the framebuffer's objects.
> 
> In drm_client_buffer_delete(), do a possible vunmap before releasing
> the framebuffer. Otherwise we'd eventually release the framebuffer
> before unmaping its buffer objects.

Thanks, it looks good to me.

Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com>


> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>   drivers/gpu/drm/drm_client.c | 20 ++++++++++++--------
>   include/drm/drm_client.h     |  9 +++------
>   2 files changed, 15 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
> index 73ae63f856a0..29ed5f55b5f9 100644
> --- a/drivers/gpu/drm/drm_client.c
> +++ b/drivers/gpu/drm/drm_client.c
> @@ -17,6 +17,7 @@
>   #include <drm/drm_fourcc.h>
>   #include <drm/drm_framebuffer.h>
>   #include <drm/drm_gem.h>
> +#include <drm/drm_gem_framebuffer_helper.h>
>   #include <drm/drm_mode.h>
>   #include <drm/drm_print.h>
>   
> @@ -174,17 +175,17 @@ EXPORT_SYMBOL(drm_client_release);
>   
>   static void drm_client_buffer_delete(struct drm_client_buffer *buffer)
>   {
> +	struct drm_gem_object *gem = drm_gem_fb_get_obj(buffer->fb, 0);
>   	int ret;
>   
> +	drm_gem_vunmap(gem, &buffer->map);
> +
>   	ret = drm_mode_rmfb(buffer->client->dev, buffer->fb->base.id, buffer->client->file);
>   	if (ret)
>   		drm_err(buffer->client->dev,
>   			"Error removing FB:%u (%d)\n", buffer->fb->base.id, ret);
>   
> -	if (buffer->gem) {
> -		drm_gem_vunmap(buffer->gem, &buffer->map);
> -		drm_gem_object_put(buffer->gem);
> -	}
> +	drm_gem_object_put(buffer->gem);
>   
>   	kfree(buffer);
>   }
> @@ -274,7 +275,7 @@ drm_client_buffer_create(struct drm_client_dev *client, u32 width, u32 height,
>   int drm_client_buffer_vmap_local(struct drm_client_buffer *buffer,
>   				 struct iosys_map *map_copy)
>   {
> -	struct drm_gem_object *gem = buffer->gem;
> +	struct drm_gem_object *gem = drm_gem_fb_get_obj(buffer->fb, 0);
>   	struct iosys_map *map = &buffer->map;
>   	int ret;
>   
> @@ -303,7 +304,7 @@ EXPORT_SYMBOL(drm_client_buffer_vmap_local);
>    */
>   void drm_client_buffer_vunmap_local(struct drm_client_buffer *buffer)
>   {
> -	struct drm_gem_object *gem = buffer->gem;
> +	struct drm_gem_object *gem = drm_gem_fb_get_obj(buffer->fb, 0);
>   	struct iosys_map *map = &buffer->map;
>   
>   	drm_gem_vunmap_locked(gem, map);
> @@ -334,9 +335,10 @@ EXPORT_SYMBOL(drm_client_buffer_vunmap_local);
>   int drm_client_buffer_vmap(struct drm_client_buffer *buffer,
>   			   struct iosys_map *map_copy)
>   {
> +	struct drm_gem_object *gem = drm_gem_fb_get_obj(buffer->fb, 0);
>   	int ret;
>   
> -	ret = drm_gem_vmap(buffer->gem, &buffer->map);
> +	ret = drm_gem_vmap(gem, &buffer->map);
>   	if (ret)
>   		return ret;
>   	*map_copy = buffer->map;
> @@ -355,7 +357,9 @@ EXPORT_SYMBOL(drm_client_buffer_vmap);
>    */
>   void drm_client_buffer_vunmap(struct drm_client_buffer *buffer)
>   {
> -	drm_gem_vunmap(buffer->gem, &buffer->map);
> +	struct drm_gem_object *gem = drm_gem_fb_get_obj(buffer->fb, 0);
> +
> +	drm_gem_vunmap(gem, &buffer->map);
>   }
>   EXPORT_SYMBOL(drm_client_buffer_vunmap);
>   
> diff --git a/include/drm/drm_client.h b/include/drm/drm_client.h
> index db0665263a10..5909664d4d43 100644
> --- a/include/drm/drm_client.h
> +++ b/include/drm/drm_client.h
> @@ -166,12 +166,9 @@ 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.
> +	 * FIXME: The DRM framebuffer holds a reference on its GEM
> +	 * buffer objects. Do not use this field in new code and
> +	 * update existing users.
>   	 */
>   	struct drm_gem_object *gem;
>