[PATCH v2 1/2] drm/virtio: Defer the host dumb buffer creation

[Dmitry Osipenko]

On 9/3/24 10:48, Jocelyn Falempe wrote:
> The host dumb buffer command needs a format, but the
> DRM_IOCTL_MODE_CREATE_DUMB only provides a buffer size.
> So wait for the DRM_IOCTL_MODE_ADDFB(2), to get the format, and create
> the host resources at this time.
> 
> This will allow virtio-gpu to support multiple pixel formats.
> 
> This problem was noticed in commit 42fd9e6c29b39 ("drm/virtio: fix
> DRM_FORMAT_* handling")
> 
> Suggested-by: Gerd Hoffmann <kraxel at redhat.com>
> Signed-off-by: Jocelyn Falempe <jfalempe at redhat.com>
> ---
> 
> v2:
>  * Move virtio_gpu_object deferred field to its own block (Geerd Hoffmann)
>  * Check that the size of the dumb buffer can hold the framebuffer (Geerd Hoffmann)
> 
>  drivers/gpu/drm/virtio/virtgpu_display.c | 33 ++++++++++++++++++++++++
>  drivers/gpu/drm/virtio/virtgpu_drv.h     |  5 ++++
>  drivers/gpu/drm/virtio/virtgpu_gem.c     |  1 -
>  drivers/gpu/drm/virtio/virtgpu_object.c  | 13 +++++++---
>  4 files changed, 47 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/virtio/virtgpu_display.c b/drivers/gpu/drm/virtio/virtgpu_display.c
> index 64baf2f22d9f0..5e8ca742c6d00 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_display.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_display.c
> @@ -290,6 +290,30 @@ static int vgdev_output_init(struct virtio_gpu_device *vgdev, int index)
>  	return 0;
>  }
>  
> +static int virtio_gpu_deferred_create(struct virtio_gpu_object *bo,
> +				      struct virtio_gpu_device *vgdev,
> +				      const struct drm_mode_fb_cmd2 *mode_cmd)
> +{
> +	struct virtio_gpu_object_params params = { 0 };
> +
> +	params.format = virtio_gpu_translate_format(mode_cmd->pixel_format);
> +	params.dumb = true;
> +	params.width = mode_cmd->width;
> +	params.height = mode_cmd->height;
> +	params.size = params.height * params.width * 4;
> +	params.size = ALIGN(params.size, PAGE_SIZE);
> +
> +	if (params.size > bo->base.base.size)
> +		return -EINVAL;
> +
> +	virtio_gpu_cmd_create_resource(vgdev, bo, &params, NULL, NULL);
> +	virtio_gpu_object_attach(vgdev, bo, bo->ents, bo->nents);
> +
> +	bo->deferred = false;
> +	bo->ents = NULL;
> +	return 0;
> +}
> +
>  static struct drm_framebuffer *
>  virtio_gpu_user_framebuffer_create(struct drm_device *dev,
>  				   struct drm_file *file_priv,
> @@ -297,6 +321,8 @@ virtio_gpu_user_framebuffer_create(struct drm_device *dev,
>  {
>  	struct drm_gem_object *obj = NULL;
>  	struct virtio_gpu_framebuffer *virtio_gpu_fb;
> +	struct virtio_gpu_device *vgdev = dev->dev_private;
> +	struct virtio_gpu_object *bo;
>  	int ret;
>  
>  	if (mode_cmd->pixel_format != DRM_FORMAT_HOST_XRGB8888 &&
> @@ -308,6 +334,13 @@ virtio_gpu_user_framebuffer_create(struct drm_device *dev,
>  	if (!obj)
>  		return ERR_PTR(-EINVAL);
>  
> +	bo = gem_to_virtio_gpu_obj(obj);
> +	if (bo->deferred) {
> +		ret = virtio_gpu_deferred_create(bo, vgdev, mode_cmd);
> +		if (ret)
> +			return ERR_PTR(ret);
> +	}
> +
>  	virtio_gpu_fb = kzalloc(sizeof(*virtio_gpu_fb), GFP_KERNEL);
>  	if (virtio_gpu_fb == NULL) {
>  		drm_gem_object_put(obj);
> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
> index 64c236169db88..4302933e5067c 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_drv.h
> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
> @@ -95,6 +95,11 @@ struct virtio_gpu_object {
>  	bool host3d_blob, guest_blob;
>  	uint32_t blob_mem, blob_flags;
>  
> +	/* For deferred dumb buffer creation */
> +	bool deferred;
> +	struct virtio_gpu_mem_entry *ents;
> +	unsigned int nents;
> +
>  	int uuid_state;
>  	uuid_t uuid;
>  };
> diff --git a/drivers/gpu/drm/virtio/virtgpu_gem.c b/drivers/gpu/drm/virtio/virtgpu_gem.c
> index 7db48d17ee3a8..33ad15fed30f6 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_gem.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_gem.c
> @@ -75,7 +75,6 @@ int virtio_gpu_mode_dumb_create(struct drm_file *file_priv,
>  	args->size = pitch * args->height;
>  	args->size = ALIGN(args->size, PAGE_SIZE);
>  
> -	params.format = virtio_gpu_translate_format(DRM_FORMAT_HOST_XRGB8888);

This will break the guest blob code path in virtio_gpu_object_create(),
AFAICT.

>  	params.width = args->width;
>  	params.height = args->height;
>  	params.size = args->size;
> diff --git a/drivers/gpu/drm/virtio/virtgpu_object.c b/drivers/gpu/drm/virtio/virtgpu_object.c
> index c7e74cf130221..b5a537a761294 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_object.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_object.c
> @@ -67,6 +67,8 @@ void virtio_gpu_cleanup_object(struct virtio_gpu_object *bo)
>  
>  	virtio_gpu_resource_id_put(vgdev, bo->hw_res_handle);
>  	if (virtio_gpu_is_shmem(bo)) {
> +		if (bo->deferred)
> +			kvfree(bo->ents);
>  		drm_gem_shmem_free(&bo->base);
>  	} else if (virtio_gpu_is_vram(bo)) {
>  		struct virtio_gpu_object_vram *vram = to_virtio_gpu_vram(bo);
> @@ -228,10 +230,13 @@ int virtio_gpu_object_create(struct virtio_gpu_device *vgdev,
>  		virtio_gpu_cmd_resource_create_3d(vgdev, bo, params,
>  						  objs, fence);
>  		virtio_gpu_object_attach(vgdev, bo, ents, nents);
> -	} else {
> -		virtio_gpu_cmd_create_resource(vgdev, bo, params,
> -					       objs, fence);
> -		virtio_gpu_object_attach(vgdev, bo, ents, nents);
> +	} else if (params->dumb) {
> +		/* Create the host resource in virtio_gpu_user_framebuffer_create()
> +		 * because the pixel format is not specified yet
> +		 */
> +		bo->ents = ents;
> +		bo->nents = nents;
> +		bo->deferred = true;
>  	}

AFAICS, the "params.dumb = true" should be set in
virtio_gpu_mode_dumb_create() and not in virtio_gpu_deferred_create().
Was this patch tested?

Overall, this deferred resource creation doesn't look robust. Could be
better to either add SET_SCANOUT2 with the format info or add cmd that
overrides the res fmt.