On 23/08/2024 09:04, Gerd Hoffmann wrote:
+static void 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.width = mode_cmd->width; + params.height = mode_cmd->height; + params.size = params.height * params.width * 4; + params.size = ALIGN(params.size, PAGE_SIZE); + params.dumb = true;I'd suggest to simply store the complete virtio_gpu_object_params struct in virtio_gpu_object instead of re-creating it here.
The struct params is much bigger than the struct virtio_gpu_object, so I though it would waste too much memory. Using a pointer would add an alloc/free pair, and a potential source of memleak. And as we have the required parameters in struct drm_mode_fb_cmd2, I found it better this way.
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h index 64c236169db88..8d1e8dcfa8c15 100644 --- a/drivers/gpu/drm/virtio/virtgpu_drv.h +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h @@ -93,6 +93,9 @@ struct virtio_gpu_object { bool dumb; bool created; bool host3d_blob, guest_blob; + bool deferred; + struct virtio_gpu_mem_entry *ents; + unsigned int nents; uint32_t blob_mem, blob_flags;Put them into a new block separated by newline, add a comment saying those are needed for virtio_gpu_deferred_create?
Yes, I will do that in v2
@@ -229,9 +231,14 @@ int virtio_gpu_object_create(struct virtio_gpu_device *vgdev, 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); + /* Fence is used only with blob or virgl */ + WARN_ONCE(fence != NULL, "deferred doesn't support fence\n");Additionally check for param->dumb to take the deferred code path? That should make sure there is no fence.
I think I can also duplicate virtio_gpu_object_create(), and have one version only for deferred dumb buffer. I will see if that doesn't make too much code duplication.
take care, Gerd
Thanks for the review, -- Jocelyn
