diff mbox series

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

Message ID 20190702141903.1131-17-kraxel@redhat.com (mailing list archive)
State New, archived
Headers show
Series drm/virtio: switch from ttm to gem shmem helpers. | expand

Commit Message

Gerd Hoffmann July 2, 2019, 2:19 p.m. UTC 
Switch to the virtio_gpu_array_* helper workflow.

Signed-off-by: Gerd Hoffmann <kraxel@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(-)

Comments

Gurchetan Singh July 3, 2019, 12:08 a.m. UTC | #1 
On Tue, Jul 2, 2019 at 7:19 AM Gerd Hoffmann <kraxel@redhat.com> wrote:

> Switch to the virtio_gpu_array_* helper workflow.
>
> Signed-off-by: Gerd Hoffmann <kraxel@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
>
>
Gerd Hoffmann July 4, 2019, noon UTC | #2 
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@redhat.com> wrote:
> 
> > Switch to the virtio_gpu_array_* helper workflow.
> >
> > Signed-off-by: Gerd Hoffmann <kraxel@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
diff mbox series

Patch

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);
 }