[PATCH v6 16/18] drm/virtio: rework virtio_gpu_cmd_context_{attach, detach}_resource
Gurchetan Singh
gurchetansingh at chromium.org
Wed Jul 3 00:08:46 UTC 2019
On Tue, Jul 2, 2019 at 7:19 AM Gerd Hoffmann <kraxel at redhat.com> wrote:
> Switch to the virtio_gpu_array_* helper workflow.
>
> Signed-off-by: Gerd Hoffmann <kraxel at redhat.com>
> ---
> 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20190702/04221315/attachment-0001.html>
More information about the dri-devel
mailing list