Re: [PATCH 3/7] drm/client: Inline drm_client_buffer_addfb() and _rmfb()

[Jocelyn Falempe <jfalempe@redhat.com>]

On 20/10/2025 17:04, Thomas Zimmermann wrote:
> Creating and deleting a client buffer always creates and deletes
> the underlying DRM framebuffer. Inline the helper functions into
> their callers.
> 
> With the _addfb code being inlined into drm_client_buffer_create(),
> clean up the function's error rollback to release the framebuffer's
> handle and GEM buffer object as needed.
> 
> Move the _rmfb code into drm_client_buffer_delete() rather than its
> current location in drm_client_framebuffer_delete(). The former is
> now the inverse of drm_client_buffer_create(). Makes no difference
> for cleaning up. Also prepares for the removal of
> drm_client_framebuffer_delete().

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 | 93 ++++++++++++++++--------------------
>   1 file changed, 40 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
> index 9bf2edfb7b64..73ae63f856a0 100644
> --- a/drivers/gpu/drm/drm_client.c
> +++ b/drivers/gpu/drm/drm_client.c
> @@ -174,6 +174,13 @@ EXPORT_SYMBOL(drm_client_release);
>   
>   static void drm_client_buffer_delete(struct drm_client_buffer *buffer)
>   {
> +	int ret;
> +
> +	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);
> @@ -186,8 +193,21 @@ static struct drm_client_buffer *
>   drm_client_buffer_create(struct drm_client_dev *client, u32 width, u32 height,
>   			 u32 format, u32 handle, u32 pitch)
>   {
> +	struct drm_mode_fb_cmd2 fb_req = {
> +		.width = width,
> +		.height = height,
> +		.pixel_format = format,
> +		.handles = {
> +			handle,
> +		},
> +		.pitches = {
> +			pitch,
> +		},
> +	};
> +	struct drm_device *dev = client->dev;
>   	struct drm_client_buffer *buffer;
>   	struct drm_gem_object *obj;
> +	struct drm_framebuffer *fb;
>   	int ret;
>   
>   	buffer = kzalloc(sizeof(*buffer), GFP_KERNEL);
> @@ -202,10 +222,30 @@ drm_client_buffer_create(struct drm_client_dev *client, u32 width, u32 height,
>   		goto err_delete;
>   	}
>   
> +	ret = drm_mode_addfb2(dev, &fb_req, client->file);
> +	if (ret)
> +		goto err_drm_gem_object_put;
> +
> +	fb = drm_framebuffer_lookup(dev, client->file, fb_req.fb_id);
> +	if (drm_WARN_ON(dev, !fb)) {
> +		ret = -ENOENT;
> +		goto err_drm_mode_rmfb;
> +	}
> +
> +	/* drop the reference we picked up in framebuffer lookup */
> +	drm_framebuffer_put(fb);
> +
> +	strscpy(fb->comm, client->name, TASK_COMM_LEN);
> +
>   	buffer->gem = obj;
> +	buffer->fb = fb;
>   
>   	return buffer;
>   
> +err_drm_mode_rmfb:
> +	drm_mode_rmfb(dev, fb_req.fb_id, client->file);
> +err_drm_gem_object_put:
> +	drm_gem_object_put(obj);
>   err_delete:
>   	kfree(buffer);
>   	return ERR_PTR(ret);
> @@ -319,51 +359,6 @@ void drm_client_buffer_vunmap(struct drm_client_buffer *buffer)
>   }
>   EXPORT_SYMBOL(drm_client_buffer_vunmap);
>   
> -static void drm_client_buffer_rmfb(struct drm_client_buffer *buffer)
> -{
> -	int ret;
> -
> -	if (!buffer->fb)
> -		return;
> -
> -	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);
> -
> -	buffer->fb = NULL;
> -}
> -
> -static int drm_client_buffer_addfb(struct drm_client_buffer *buffer,
> -				   u32 width, u32 height, u32 format,
> -				   u32 handle, u32 pitch)
> -{
> -	struct drm_client_dev *client = buffer->client;
> -	struct drm_mode_fb_cmd2 fb_req = { };
> -	int ret;
> -
> -	fb_req.width = width;
> -	fb_req.height = height;
> -	fb_req.pixel_format = format;
> -	fb_req.handles[0] = handle;
> -	fb_req.pitches[0] = pitch;
> -
> -	ret = drm_mode_addfb2(client->dev, &fb_req, client->file);
> -	if (ret)
> -		return ret;
> -
> -	buffer->fb = drm_framebuffer_lookup(client->dev, buffer->client->file, fb_req.fb_id);
> -	if (WARN_ON(!buffer->fb))
> -		return -ENOENT;
> -
> -	/* drop the reference we picked up in framebuffer lookup */
> -	drm_framebuffer_put(buffer->fb);
> -
> -	strscpy(buffer->fb->comm, client->name, TASK_COMM_LEN);
> -
> -	return 0;
> -}
> -
>   /**
>    * drm_client_framebuffer_create - Create a client framebuffer
>    * @client: DRM client
> @@ -401,11 +396,6 @@ drm_client_framebuffer_create(struct drm_client_dev *client, u32 width, u32 heig
>   		goto err_drm_mode_destroy_dumb;
>   	}
>   
> -	ret = drm_client_buffer_addfb(buffer, width, height, format,
> -				      dumb_args.handle, dumb_args.pitch);
> -	if (ret)
> -		goto err_drm_client_buffer_delete;
> -
>   	/*
>   	 * The handle is only needed for creating the framebuffer, destroy it
>   	 * again to solve a circular dependency should anybody export the GEM
> @@ -416,8 +406,6 @@ drm_client_framebuffer_create(struct drm_client_dev *client, u32 width, u32 heig
>   
>   	return buffer;
>   
> -err_drm_client_buffer_delete:
> -	drm_client_buffer_delete(buffer);
>   err_drm_mode_destroy_dumb:
>   	drm_mode_destroy_dumb(client->dev, dumb_args.handle, client->file);
>   	return ERR_PTR(ret);
> @@ -433,7 +421,6 @@ void drm_client_framebuffer_delete(struct drm_client_buffer *buffer)
>   	if (!buffer)
>   		return;
>   
> -	drm_client_buffer_rmfb(buffer);
>   	drm_client_buffer_delete(buffer);
>   }
>   EXPORT_SYMBOL(drm_client_framebuffer_delete);