Gerd Hoffmann
2019-Jul-02 14:19 UTC
[PATCH v6 16/18] drm/virtio: rework virtio_gpu_cmd_context_{attach, detach}_resource
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);
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
Gurchetan Singh
2019-Jul-03 00:08 UTC
[PATCH v6 16/18] drm/virtio: rework virtio_gpu_cmd_context_{attach, detach}_resource
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: <http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20190702/04221315/attachment.html>
Gerd Hoffmann
2019-Jul-04 12:00 UTC
[PATCH v6 16/18] drm/virtio: rework virtio_gpu_cmd_context_{attach,detach}_resource
On Tue, Jul 02, 2019 at 05:08:46PM -0700, Gurchetan Singh wrote:> 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?Yes. Fixed.> Some miscellaneous comments: > 1) Maybe virtio_gpu_array can have it's own header and file? virtgpu_drv.h > is getting rather big..Longer-term it might move out anyway due to becoming a generic drm helper.> 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)?Protect the (guest) gem object, specifically make sure the bo->hw_res_handle stays valid. cheers, Gerd
Possibly Parallel Threads
- [PATCH v6 16/18] drm/virtio: rework virtio_gpu_cmd_context_{attach, detach}_resource
- [PATCH v6 16/18] drm/virtio: rework virtio_gpu_cmd_context_{attach, detach}_resource
- [PATCH v7 12/18] drm/virtio: rework virtio_gpu_cmd_context_{attach, detach}_resource
- [PATCH v2 4/6] virtio-gpu: add 3d/virgl support
- [PATCH v2 4/6] virtio-gpu: add 3d/virgl support