Re: [virtio-dev] [PATCH] [RFC RESEND] vdec: Add virtio video decode device specification

[Keiichi Watanabe <keiichiw@chromium.org>]

Hi, I just sent a patch for our virtio-vdec driver to the Linux Media
mailing list:
https://patchwork.kernel.org/patch/11220661/

This implementation uses the initial version of virtio-vdec I proposed
in this thread and doesn't contain David's recent patch for DMA
addresses of virtio-gpu buffers.
If his patch looks good to everyone, I'm happy to update my patch to
use them instead of exported virtio-gpu resource handles.

Any feedback will be appreciated. Thanks in advance.

Best regards,
Keiichi

On Tue, Oct 29, 2019 at 4:39 PM David Stevens <stevensd@chromium.org> wrote:
>
> > When running drm-misc-next you should be able to test whenever that'll
> > actually work without any virtio-gpu driver changes.
>
> I did some experimentation with the Chrome OS kernel-next branch
> (based on v5.4-rc3) plus drm-misc-next. I looked at both the chromeos
> downstream virtio-wayland driver as well as the virtio-vdec driver
> that was recently proposed to upstream.
>
> Using the dma address of buffers generally works. However, it does
> require the addition of some synchronization in the virtio-gpu driver
> to prevent races between the virtio-gpu device registering the buffer
> with the guest dma address (which happens with the ATTACH_BACKING
> command) and other virtio devices using the guest dma address as a
> buffer identifier. I've included a patch that adds this
> synchronization.
>
> Signed-off-by: David Stevens <stevensd@chromium.org>
> ---
>  drivers/gpu/drm/virtio/virtgpu_drv.h    |  2 +
>  drivers/gpu/drm/virtio/virtgpu_object.c | 15 +++++-
>  drivers/gpu/drm/virtio/virtgpu_vq.c     | 64 +++++++++++++++++++++----
>  3 files changed, 71 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h
> b/drivers/gpu/drm/virtio/virtgpu_drv.h
> index 314e02f94d9c4..00a2c0e6b6382 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_drv.h
> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
> @@ -72,6 +72,8 @@ struct virtio_gpu_object {
>         uint32_t mapped;
>         bool dumb;
>         bool created;
> +
> +       bool attach_backing_complete;
>  };
>  #define gem_to_virtio_gpu_obj(gobj) \
>         container_of((gobj), struct virtio_gpu_object, base.base)
> diff --git a/drivers/gpu/drm/virtio/virtgpu_object.c
> b/drivers/gpu/drm/virtio/virtgpu_object.c
> index 017a9e0fc3bb8..812a0a48f6385 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_object.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_object.c
> @@ -75,13 +75,26 @@ static void virtio_gpu_free_object(struct
> drm_gem_object *obj)
>         drm_gem_shmem_free_object(obj);
>  }
>
> +int virtio_gpu_gem_object_pin(struct drm_gem_object *obj)
> +{
> +       struct virtio_gpu_object *bo = gem_to_virtio_gpu_obj(obj);
> +       struct virtio_gpu_device *vgdev = obj->dev->dev_private;
> +
> +       if (!bo->attach_backing_complete)
> +               wait_event(vgdev->resp_wq, bo->attach_backing_complete);
> +       if (!bo->attach_backing_complete)
> +               return -EFAULT;
> +
> +       return drm_gem_shmem_pin(obj);
> +}
> +
>  static const struct drm_gem_object_funcs virtio_gpu_gem_funcs = {
>         .free = virtio_gpu_free_object,
>         .open = virtio_gpu_gem_object_open,
>         .close = virtio_gpu_gem_object_close,
>
>         .print_info = drm_gem_shmem_print_info,
> -       .pin = drm_gem_shmem_pin,
> +       .pin = virtio_gpu_gem_object_pin,
>         .unpin = drm_gem_shmem_unpin,
>         .get_sg_table = drm_gem_shmem_get_sg_table,
>         .vmap = drm_gem_shmem_vmap,
> diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c
> b/drivers/gpu/drm/virtio/virtgpu_vq.c
> index 80176f379ad51..8bc2359a6d625 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_vq.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
> @@ -579,26 +579,42 @@ void virtio_gpu_cmd_transfer_to_host_2d(struct
> virtio_gpu_device *vgdev,
>         virtio_gpu_queue_fenced_ctrl_buffer(vgdev, vbuf, &cmd_p->hdr, fence);
>  }
>
> +static void
> +virtio_gpu_cmd_resource_attach_backing_cb(struct virtio_gpu_device *vgdev,
> +                                         struct virtio_gpu_vbuffer *vbuf)
> +{
> +       struct virtio_gpu_object_array *objs = vbuf->objs;
> +       struct virtio_gpu_object *bo = gem_to_virtio_gpu_obj(objs->objs[0]);
> +
> +       bo->attach_backing_complete = true;
> +       wake_up_all(&vgdev->resp_wq);
> +}
> +
>  static void
>  virtio_gpu_cmd_resource_attach_backing(struct virtio_gpu_device *vgdev,
> -                                      uint32_t resource_id,
> +                                      struct virtio_gpu_object *bo,
>                                        struct virtio_gpu_mem_entry *ents,
>                                        uint32_t nents,
> +                                      struct virtio_gpu_object_array *objs,
>                                        struct virtio_gpu_fence *fence)
>  {
>         struct virtio_gpu_resource_attach_backing *cmd_p;
>         struct virtio_gpu_vbuffer *vbuf;
>
> -       cmd_p = virtio_gpu_alloc_cmd(vgdev, &vbuf, sizeof(*cmd_p));
> +       cmd_p = virtio_gpu_alloc_cmd_resp(
> +               vgdev, virtio_gpu_cmd_resource_attach_backing_cb, &vbuf,
> +               sizeof(*cmd_p), sizeof(struct virtio_gpu_ctrl_hdr), NULL);
>         memset(cmd_p, 0, sizeof(*cmd_p));
>
>         cmd_p->hdr.type = cpu_to_le32(VIRTIO_GPU_CMD_RESOURCE_ATTACH_BACKING);
> -       cmd_p->resource_id = cpu_to_le32(resource_id);
> +       cmd_p->resource_id = cpu_to_le32(bo->hw_res_handle);
>         cmd_p->nr_entries = cpu_to_le32(nents);
>
>         vbuf->data_buf = ents;
>         vbuf->data_size = sizeof(*ents) * nents;
>
> +       vbuf->objs = objs;
> +
>         virtio_gpu_queue_fenced_ctrl_buffer(vgdev, vbuf, &cmd_p->hdr, fence);
>  }
>
> @@ -1048,9 +1064,10 @@ int virtio_gpu_object_attach(struct
> virtio_gpu_device *vgdev,
>                              struct virtio_gpu_fence *fence)
>  {
>         bool use_dma_api = !virtio_has_iommu_quirk(vgdev->vdev);
> +       struct virtio_gpu_object_array *objs = NULL;
>         struct virtio_gpu_mem_entry *ents;
>         struct scatterlist *sg;
> -       int si, nents, ret;
> +       int si, nents, ret = 0;
>
>         if (WARN_ON_ONCE(!obj->created))
>                 return -EINVAL;
> @@ -1063,8 +1080,8 @@ int virtio_gpu_object_attach(struct
> virtio_gpu_device *vgdev,
>
>         obj->pages = drm_gem_shmem_get_sg_table(&obj->base.base);
>         if (obj->pages == NULL) {
> -               drm_gem_shmem_unpin(&obj->base.base);
> -               return -EINVAL;
> +               ret = -EINVAL;
> +               goto err_unpin;
>         }
>
>         if (use_dma_api) {
> @@ -1081,7 +1098,8 @@ int virtio_gpu_object_attach(struct
> virtio_gpu_device *vgdev,
>                              GFP_KERNEL);
>         if (!ents) {
>                 DRM_ERROR("failed to allocate ent list\n");
> -               return -ENOMEM;
> +               ret = -ENOMEM;
> +               goto err_unmap;
>         }
>
>         for_each_sg(obj->pages->sgl, sg, nents, si) {
> @@ -1092,10 +1110,38 @@ int virtio_gpu_object_attach(struct
> virtio_gpu_device *vgdev,
>                 ents[si].padding = 0;
>         }
>
> -       virtio_gpu_cmd_resource_attach_backing(vgdev, obj->hw_res_handle,
> +       objs = virtio_gpu_array_alloc(1);
> +       if (!objs) {
> +               ret = -ENOMEM;
> +               goto err_free_ents;
> +       }
> +       virtio_gpu_array_add_obj(objs, &obj->base.base);
> +
> +       if (fence) {
> +               ret = virtio_gpu_array_lock_resv(objs);
> +               if (ret != 0)
> +                       goto err_put_objs;
> +       }
> +
> +       virtio_gpu_cmd_resource_attach_backing(vgdev, obj,
>                                                ents, nents,
> -                                              fence);
> +                                              objs, fence);
>         return 0;
> +
> +err_put_objs:
> +       virtio_gpu_array_put_free(objs);
> +err_free_ents:
> +       kfree(ents);
> +err_unmap:
> +       if (use_dma_api) {
> +               dma_unmap_sg(vgdev->vdev->dev.parent,
> +                            obj->pages->sgl, obj->pages->nents,
> +                            DMA_TO_DEVICE);
> +               obj->mapped = 0;
> +       }
> +err_unpin:
> +       drm_gem_shmem_unpin(&obj->base.base);
> +       return ret;
>  }
>
>  void virtio_gpu_object_detach(struct virtio_gpu_device *vgdev,
> --
> 2.24.0.rc0.303.g954a862665-goog