Re: [PATCH v6 16/18] drm/virtio: rework virtio_gpu_cmd_context_{attach, detach}_resource

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On Tue, Jul 2, 2019 at 7:19 AM Gerd Hoffmann <kraxel@xxxxxxxxxx> wrote:
Switch to the virtio_gpu_array_* helper workflow.

Signed-off-by: Gerd Hoffmann <kraxel@xxxxxxxxxx>
---
 drivers/gpu/drm/virtio/virtgpu_drv.h |  4 ++--
 drivers/gpu/drm/virtio/virtgpu_gem.c | 24 +++++++++++-------------
 drivers/gpu/drm/virtio/virtgpu_vq.c  | 10 ++++++----
 3 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
index b1f63a21abb6..d99c54abd090 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -292,10 +292,10 @@ void virtio_gpu_cmd_context_destroy(struct virtio_gpu_device *vgdev,
                                    uint32_t id);
 void virtio_gpu_cmd_context_attach_resource(struct virtio_gpu_device *vgdev,
                                            uint32_t ctx_id,
-                                           uint32_t resource_id);
+                                           struct virtio_gpu_object_array *objs);
 void virtio_gpu_cmd_context_detach_resource(struct virtio_gpu_device *vgdev,
                                            uint32_t ctx_id,
-                                           uint32_t resource_id);
+                                           struct virtio_gpu_object_array *objs);
 void virtio_gpu_cmd_submit(struct virtio_gpu_device *vgdev,
                           void *data, uint32_t data_size,
                           uint32_t ctx_id,
diff --git a/drivers/gpu/drm/virtio/virtgpu_gem.c b/drivers/gpu/drm/virtio/virtgpu_gem.c
index 6baf64141645..e75819dbba80 100644
--- a/drivers/gpu/drm/virtio/virtgpu_gem.c
+++ b/drivers/gpu/drm/virtio/virtgpu_gem.c
@@ -111,19 +111,18 @@ int virtio_gpu_gem_object_open(struct drm_gem_object *obj,
 {
        struct virtio_gpu_device *vgdev = obj->dev->dev_private;
        struct virtio_gpu_fpriv *vfpriv = file->driver_priv;
-       struct virtio_gpu_object *qobj = gem_to_virtio_gpu_obj(obj);
-       int r;
+       struct virtio_gpu_object_array *objs;

        if (!vgdev->has_virgl_3d)
                return 0;

-       r = virtio_gpu_object_reserve(qobj);
-       if (r)
-               return r;
+       objs = virtio_gpu_array_alloc(1);
+       if (!objs)
+               return -ENOMEM;
+       virtio_gpu_array_add_obj(objs, obj);

        virtio_gpu_cmd_context_attach_resource(vgdev, vfpriv->ctx_id,
-                                              qobj->hw_res_handle);
-       virtio_gpu_object_unreserve(qobj);
+                                              objs);
        return 0;
 }

@@ -132,19 +131,18 @@ void virtio_gpu_gem_object_close(struct drm_gem_object *obj,
 {
        struct virtio_gpu_device *vgdev = obj->dev->dev_private;
        struct virtio_gpu_fpriv *vfpriv = file->driver_priv;
-       struct virtio_gpu_object *qobj = gem_to_virtio_gpu_obj(obj);
-       int r;
+       struct virtio_gpu_object_array *objs;

        if (!vgdev->has_virgl_3d)
                return;

-       r = virtio_gpu_object_reserve(qobj);
-       if (r)
+       objs = virtio_gpu_array_alloc(1);
+       if (!objs)
                return;
+       virtio_gpu_array_add_obj(objs, obj);

This seems to call drm_gem_object_get.  Without adding the objects to the vbuf, aren't we missing the corresponding drm_gem_object_put_unlocked?

Some miscellaneous comments:
1) Maybe virtio_gpu_array can have it's own header and file?  virtgpu_drv.h is getting rather big..
2) What data are you trying to protect with the additional references?  Is it host side resources (i.e, the host GL texture or buffer object) or is it guest side resources (fences)?  Guest side resources seem properly counted to me.  GL is supposed to reference count pending resources as well, but that's not always the case in practice.  A little blurb somewhere like "hold on to 3D GEM buffers until the host response as a safety measure" or "we could potential cause a kernel oops if [...]" would be useful.

The guest side looks sufficiently referenced to me, while the GL spec indicates   
 

        virtio_gpu_cmd_context_detach_resource(vgdev, vfpriv->ctx_id,
-                                               qobj->hw_res_handle);
-       virtio_gpu_object_unreserve(qobj);
+                                              objs);
 }

 struct virtio_gpu_object_array *virtio_gpu_array_alloc(u32 nents)
diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c
index 1c0097de419a..799d1339ee0f 100644
--- a/drivers/gpu/drm/virtio/virtgpu_vq.c
+++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
@@ -835,8 +835,9 @@ void virtio_gpu_cmd_context_destroy(struct virtio_gpu_device *vgdev,

 void virtio_gpu_cmd_context_attach_resource(struct virtio_gpu_device *vgdev,
                                            uint32_t ctx_id,
-                                           uint32_t resource_id)
+                                           struct virtio_gpu_object_array *objs)
 {
+       struct virtio_gpu_object *bo = gem_to_virtio_gpu_obj(objs->objs[0]);
        struct virtio_gpu_ctx_resource *cmd_p;
        struct virtio_gpu_vbuffer *vbuf;

@@ -845,15 +846,16 @@ void virtio_gpu_cmd_context_attach_resource(struct virtio_gpu_device *vgdev,

        cmd_p->hdr.type = cpu_to_le32(VIRTIO_GPU_CMD_CTX_ATTACH_RESOURCE);
        cmd_p->hdr.ctx_id = cpu_to_le32(ctx_id);
-       cmd_p->resource_id = cpu_to_le32(resource_id);
+       cmd_p->resource_id = cpu_to_le32(bo->hw_res_handle);
        virtio_gpu_queue_ctrl_buffer(vgdev, vbuf);

 }

 void virtio_gpu_cmd_context_detach_resource(struct virtio_gpu_device *vgdev,
                                            uint32_t ctx_id,
-                                           uint32_t resource_id)
+                                           struct virtio_gpu_object_array *objs)
 {
+       struct virtio_gpu_object *bo = gem_to_virtio_gpu_obj(objs->objs[0]);
        struct virtio_gpu_ctx_resource *cmd_p;
        struct virtio_gpu_vbuffer *vbuf;

@@ -862,7 +864,7 @@ void virtio_gpu_cmd_context_detach_resource(struct virtio_gpu_device *vgdev,

        cmd_p->hdr.type = cpu_to_le32(VIRTIO_GPU_CMD_CTX_DETACH_RESOURCE);
        cmd_p->hdr.ctx_id = cpu_to_le32(ctx_id);
-       cmd_p->resource_id = cpu_to_le32(resource_id);
+       cmd_p->resource_id = cpu_to_le32(bo->hw_res_handle);
        virtio_gpu_queue_ctrl_buffer(vgdev, vbuf);
 }

--
2.18.1

_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[Index of Archives]     [KVM Development]     [Libvirt Development]     [Libvirt Users]     [CentOS Virtualization]     [Netdev]     [Ethernet Bridging]     [Linux Wireless]     [Kernel Newbies]     [Security]     [Linux for Hams]     [Netfilter]     [Bugtraq]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux RAID]     [Linux Admin]     [Samba]

  Powered by Linux