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

Jocelyn Falempe <jfalempe@xxxxxxxxxx> · Wed, 11 Sep 2024 10:04:26 +0200

On 11/09/2024 00:09, Dmitry Osipenko wrote:
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@xxxxxxxxxx>
Signed-off-by: Jocelyn Falempe <jfalempe@xxxxxxxxxx>
---

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.

The params.format is not used in virtio_gpu_cmd_resource_create_3d() nor 
in virtio_gpu_cmd_resource_create_blob(), it's only used for dumb buffer 
creation. So it shouldn't break the guest blob path.

  	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?

You're right that params.dumb is not used in 
virtio_gpu_cmd_create_resource() so it's useless to set it in 
virtio_gpu_deferred_create(). I can remove that line.
But it's also set in virtio_gpu_mode_dumb_create() and I didn't change 
that with this patch.

I'm testing on a s390x machine, and it allows wayland compositor to 
work. (The colors are still a mess, but at least it starts with this patch).

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.

This would also need modification in the qemu code, so it looks much 
more complex than this patch.

Best regards,

--

Jocelyn







