Re: [PATCH v2 11/27] media: videobuf2-v4l2: Support vb2_queue embedded in a context

[John Cox]

> Support device drivers that implement multi-context operations in the
> videobuf2 core by providing an helper to retrieve the vb2_queue from
> the context associated with an open file handle. If no context is
> associated with a file handle, retrieve it from the video device
> default context, created by the core for multi-context aware drivers.
>
> Fall-back to use the vb2_queue from the video_device to support existing
> drivers which are not context aware.
>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx>
> ---
>  drivers/media/common/videobuf2/videobuf2-v4l2.c | 139 +++++++++++++++---------
>  1 file changed, 88 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> index 1cd26faee50338aefeb670c6865da7c2d43f44d3..ad7892551f54c5abc7db7641bb8fb89d77b1337f 100644
> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> @@ -998,23 +998,46 @@ EXPORT_SYMBOL_GPL(vb2_poll);
>   * and so they simplify the driver code.
>   */
>
> +/*
> + * Helper to get the vb2 queue either from:
> + * 1) The video context bound to the open file handle
> + * 2) The default context for context-aware drivers if userspace has not bound
> + *    a context to the file handle
> + * 3) From the video device for non-context aware drivers
> + */
> +static struct vb2_queue *get_vb2_queue(struct file *file,
> +                                      struct video_device *vdev)
> +{
> +#if defined(CONFIG_MEDIA_CONTROLLER)
> +       struct video_device_context *ctx =
> +                               video_device_context_from_file(file, vdev);
> +
> +       return ctx ? &ctx->queue
> +                  : vdev->default_context ? &vdev->default_context->queue
> +                  : vdev->queue;
> +#else
> +       return vdev->queue;
> +#endif /* CONFIG_MEDIA_CONTROLLER */
> +}
> +
>  /* vb2 ioctl helpers */
>
>  int vb2_ioctl_remove_bufs(struct file *file, void *priv,
>                           struct v4l2_remove_buffers *d)
>  {
>         struct video_device *vdev = video_devdata(file);
> +       struct vb2_queue *q = get_vb2_queue(file, vdev);
>
> -       if (vdev->queue->type != d->type)
> +       if (q->type != d->type)
>                 return -EINVAL;
>
>         if (d->count == 0)
>                 return 0;
>
> -       if (vb2_queue_is_busy(vdev->queue, file))
> +       if (vb2_queue_is_busy(q, file))
>                 return -EBUSY;
>
> -       return vb2_core_remove_bufs(vdev->queue, d->index, d->count);
> +       return vb2_core_remove_bufs(q, d->index, d->count);
>  }
>  EXPORT_SYMBOL_GPL(vb2_ioctl_remove_bufs);
>
> @@ -1022,21 +1045,21 @@ int vb2_ioctl_reqbufs(struct file *file, void *priv,
>                           struct v4l2_requestbuffers *p)
>  {
>         struct video_device *vdev = video_devdata(file);
> -       int res = vb2_verify_memory_type(vdev->queue, p->memory, p->type);
> +       struct vb2_queue *q = get_vb2_queue(file, vdev);
> +       int res = vb2_verify_memory_type(q, p->memory, p->type);
>         u32 flags = p->flags;
>
> -       vb2_set_flags_and_caps(vdev->queue, p->memory, &flags,
> -                              &p->capabilities, NULL);
> +       vb2_set_flags_and_caps(q, p->memory, &flags, &p->capabilities, NULL);
>         p->flags = flags;
>         if (res)
>                 return res;
> -       if (vb2_queue_is_busy(vdev->queue, file))
> +       if (vb2_queue_is_busy(q, file))
>                 return -EBUSY;
> -       res = vb2_core_reqbufs(vdev->queue, p->memory, p->flags, &p->count);
> +       res = vb2_core_reqbufs(q, p->memory, p->flags, &p->count);
>         /* If count == 0, then the owner has released all buffers and he
>            is no longer owner of the queue. Otherwise we have a new owner. */
>         if (res == 0)
> -               vdev->queue->owner = p->count ? file->private_data : NULL;
> +               q->owner = p->count ? file->private_data : NULL;
>         return res;
>  }
>  EXPORT_SYMBOL_GPL(vb2_ioctl_reqbufs);
> @@ -1045,11 +1068,12 @@ int vb2_ioctl_create_bufs(struct file *file, void *priv,
>                           struct v4l2_create_buffers *p)
>  {
>         struct video_device *vdev = video_devdata(file);
> -       int res = vb2_verify_memory_type(vdev->queue, p->memory, p->format.type);
> +       struct vb2_queue *q = get_vb2_queue(file, vdev);
> +       int res = vb2_verify_memory_type(q, p->memory, p->format.type);
>
> -       p->index = vb2_get_num_buffers(vdev->queue);
> -       vb2_set_flags_and_caps(vdev->queue, p->memory, &p->flags,
> -                              &p->capabilities, &p->max_num_buffers);
> +       p->index = vb2_get_num_buffers(q);
> +       vb2_set_flags_and_caps(q, p->memory, &p->flags, &p->capabilities,
> +                              &p->max_num_buffers);
>         /*
>          * If count == 0, then just check if memory and type are valid.
>          * Any -EBUSY result from vb2_verify_memory_type can be mapped to 0.
> @@ -1058,12 +1082,12 @@ int vb2_ioctl_create_bufs(struct file *file, void *priv,
>                 return res != -EBUSY ? res : 0;
>         if (res)
>                 return res;
> -       if (vb2_queue_is_busy(vdev->queue, file))
> +       if (vb2_queue_is_busy(q, file))
>                 return -EBUSY;
>
> -       res = vb2_create_bufs(vdev->queue, p);
> +       res = vb2_create_bufs(q, p);
>         if (res == 0)
> -               vdev->queue->owner = file->private_data;
> +               q->owner = file->private_data;
>         return res;
>  }
>  EXPORT_SYMBOL_GPL(vb2_ioctl_create_bufs);
> @@ -1072,69 +1096,76 @@ int vb2_ioctl_prepare_buf(struct file *file, void *priv,
>                           struct v4l2_buffer *p)
>  {
>         struct video_device *vdev = video_devdata(file);
> +       struct vb2_queue *q = get_vb2_queue(file, vdev);
>
> -       if (vb2_queue_is_busy(vdev->queue, file))
> +       if (vb2_queue_is_busy(q, file))
>                 return -EBUSY;
> -       return vb2_prepare_buf(vdev->queue, vdev->v4l2_dev->mdev, p);
> +       return vb2_prepare_buf(q, vdev->v4l2_dev->mdev, p);
>  }
>  EXPORT_SYMBOL_GPL(vb2_ioctl_prepare_buf);
>
>  int vb2_ioctl_querybuf(struct file *file, void *priv, struct v4l2_buffer *p)
>  {
>         struct video_device *vdev = video_devdata(file);
> +       struct vb2_queue *q = get_vb2_queue(file, vdev);
>
>         /* No need to call vb2_queue_is_busy(), anyone can query buffers. */
> -       return vb2_querybuf(vdev->queue, p);
> +       return vb2_querybuf(q, p);
>  }
>  EXPORT_SYMBOL_GPL(vb2_ioctl_querybuf);
>
>  int vb2_ioctl_qbuf(struct file *file, void *priv, struct v4l2_buffer *p)
>  {
>         struct video_device *vdev = video_devdata(file);
> +       struct vb2_queue *q = get_vb2_queue(file, vdev);
>
> -       if (vb2_queue_is_busy(vdev->queue, file))
> +       if (vb2_queue_is_busy(q, file))
>                 return -EBUSY;
> -       return vb2_qbuf(vdev->queue, vdev->v4l2_dev->mdev, p);
> +       return vb2_qbuf(q, vdev->v4l2_dev->mdev, p);
>  }
>  EXPORT_SYMBOL_GPL(vb2_ioctl_qbuf);
>
>  int vb2_ioctl_dqbuf(struct file *file, void *priv, struct v4l2_buffer *p)
>  {
>         struct video_device *vdev = video_devdata(file);
> +       struct vb2_queue *q = get_vb2_queue(file, vdev);
>
> -       if (vb2_queue_is_busy(vdev->queue, file))
> +       if (vb2_queue_is_busy(q, file))
>                 return -EBUSY;
> -       return vb2_dqbuf(vdev->queue, p, file->f_flags & O_NONBLOCK);
> +       return vb2_dqbuf(q, p, file->f_flags & O_NONBLOCK);
>  }
>  EXPORT_SYMBOL_GPL(vb2_ioctl_dqbuf);
>
>  int vb2_ioctl_streamon(struct file *file, void *priv, enum v4l2_buf_type i)
>  {
>         struct video_device *vdev = video_devdata(file);
> +       struct vb2_queue *q = get_vb2_queue(file, vdev);
>
> -       if (vb2_queue_is_busy(vdev->queue, file))
> +       if (vb2_queue_is_busy(q, file))
>                 return -EBUSY;
> -       return vb2_streamon(vdev->queue, i);
> +       return vb2_streamon(q, i);
>  }
>  EXPORT_SYMBOL_GPL(vb2_ioctl_streamon);
>
>  int vb2_ioctl_streamoff(struct file *file, void *priv, enum v4l2_buf_type i)
>  {
>         struct video_device *vdev = video_devdata(file);
> +       struct vb2_queue *q = get_vb2_queue(file, vdev);
>
> -       if (vb2_queue_is_busy(vdev->queue, file))
> +       if (vb2_queue_is_busy(q, file))
>                 return -EBUSY;
> -       return vb2_streamoff(vdev->queue, i);
> +       return vb2_streamoff(q, i);
>  }
>  EXPORT_SYMBOL_GPL(vb2_ioctl_streamoff);
>
>  int vb2_ioctl_expbuf(struct file *file, void *priv, struct v4l2_exportbuffer *p)
>  {
>         struct video_device *vdev = video_devdata(file);
> +       struct vb2_queue *q = get_vb2_queue(file, vdev);
>
> -       if (vb2_queue_is_busy(vdev->queue, file))
> +       if (vb2_queue_is_busy(q, file))
>                 return -EBUSY;
> -       return vb2_expbuf(vdev->queue, p);
> +       return vb2_expbuf(q, p);
>  }
>  EXPORT_SYMBOL_GPL(vb2_ioctl_expbuf);
>
> @@ -1143,20 +1174,22 @@ EXPORT_SYMBOL_GPL(vb2_ioctl_expbuf);
>  int vb2_fop_mmap(struct file *file, struct vm_area_struct *vma)
>  {
>         struct video_device *vdev = video_devdata(file);
> +       struct vb2_queue *q = get_vb2_queue(file, vdev);
>
> -       return vb2_mmap(vdev->queue, vma);
> +       return vb2_mmap(q, vma);
>  }
>  EXPORT_SYMBOL_GPL(vb2_fop_mmap);
>
>  int _vb2_fop_release(struct file *file, struct mutex *lock)
>  {
>         struct video_device *vdev = video_devdata(file);
> +       struct vb2_queue *q = get_vb2_queue(file, vdev);
>
>         if (lock)
>                 mutex_lock(lock);
> -       if (!vdev->queue->owner || file->private_data == vdev->queue->owner) {
> -               vb2_queue_release(vdev->queue);
> -               vdev->queue->owner = NULL;
> +       if (!q->owner || file->private_data == q->owner) {
> +               vb2_queue_release(q);
> +               q->owner = NULL;
>         }
>         if (lock)
>                 mutex_unlock(lock);
> @@ -1167,7 +1200,8 @@ EXPORT_SYMBOL_GPL(_vb2_fop_release);
>  int vb2_fop_release(struct file *file)
>  {
>         struct video_device *vdev = video_devdata(file);
> -       struct mutex *lock = vdev->queue->lock ? vdev->queue->lock : vdev->lock;
> +       struct vb2_queue *q = get_vb2_queue(file, vdev);
> +       struct mutex *lock = q->lock ? q->lock : vdev->lock;
>
>         return _vb2_fop_release(file, lock);
>  }
> @@ -1177,19 +1211,20 @@ ssize_t vb2_fop_write(struct file *file, const char __user *buf,
>                 size_t count, loff_t *ppos)
>  {
>         struct video_device *vdev = video_devdata(file);
> -       struct mutex *lock = vdev->queue->lock ? vdev->queue->lock : vdev->lock;
> +       struct vb2_queue *q = get_vb2_queue(file, vdev);
> +       struct mutex *lock = q->lock ? q->lock : vdev->lock;
>         int err = -EBUSY;
>
> -       if (!(vdev->queue->io_modes & VB2_WRITE))
> +       if (!(q->io_modes & VB2_WRITE))
>                 return -EINVAL;
>         if (lock && mutex_lock_interruptible(lock))
>                 return -ERESTARTSYS;
> -       if (vb2_queue_is_busy(vdev->queue, file))
> +       if (vb2_queue_is_busy(q, file))
>                 goto exit;
> -       err = vb2_write(vdev->queue, buf, count, ppos,
> -                      file->f_flags & O_NONBLOCK);
> -       if (vdev->queue->fileio)
> -               vdev->queue->owner = file->private_data;
> +       err = vb2_write(q, buf, count, ppos,
> +                       file->f_flags & O_NONBLOCK);
> +       if (q->fileio)
> +               q->owner = file->private_data;
>  exit:
>         if (lock)
>                 mutex_unlock(lock);
> @@ -1201,20 +1236,21 @@ ssize_t vb2_fop_read(struct file *file, char __user *buf,
>                 size_t count, loff_t *ppos)
>  {
>         struct video_device *vdev = video_devdata(file);
> -       struct mutex *lock = vdev->queue->lock ? vdev->queue->lock : vdev->lock;
> +       struct vb2_queue *q = get_vb2_queue(file, vdev);
> +       struct mutex *lock = q->lock ? q->lock : vdev->lock;
>         int err = -EBUSY;
>
> -       if (!(vdev->queue->io_modes & VB2_READ))
> +       if (!(q->io_modes & VB2_READ))
>                 return -EINVAL;
>         if (lock && mutex_lock_interruptible(lock))
>                 return -ERESTARTSYS;
> -       if (vb2_queue_is_busy(vdev->queue, file))
> +       if (vb2_queue_is_busy(q, file))
>                 goto exit;
> -       vdev->queue->owner = file->private_data;
> -       err = vb2_read(vdev->queue, buf, count, ppos,
> +       q->owner = file->private_data;
> +       err = vb2_read(q, buf, count, ppos,
>                        file->f_flags & O_NONBLOCK);
> -       if (!vdev->queue->fileio)
> -               vdev->queue->owner = NULL;
> +       if (!q->fileio)
> +               q->owner = NULL;
>  exit:
>         if (lock)
>                 mutex_unlock(lock);
> @@ -1225,7 +1261,7 @@ EXPORT_SYMBOL_GPL(vb2_fop_read);
>  __poll_t vb2_fop_poll(struct file *file, poll_table *wait)
>  {
>         struct video_device *vdev = video_devdata(file);
> -       struct vb2_queue *q = vdev->queue;
> +       struct vb2_queue *q = get_vb2_queue(file, vdev);
>         struct mutex *lock = q->lock ? q->lock : vdev->lock;
>         __poll_t res;
>         void *fileio;
> @@ -1241,7 +1277,7 @@ __poll_t vb2_fop_poll(struct file *file, poll_table *wait)
>
>         fileio = q->fileio;
>
> -       res = vb2_poll(vdev->queue, file, wait);
> +       res = vb2_poll(q, file, wait);
>
>         /* If fileio was started, then we have a new queue owner. */
>         if (!fileio && q->fileio)
> @@ -1257,8 +1293,9 @@ unsigned long vb2_fop_get_unmapped_area(struct file *file, unsigned long addr,
>                 unsigned long len, unsigned long pgoff, unsigned long flags)
>  {
>         struct video_device *vdev = video_devdata(file);
> +       struct vb2_queue *q = get_vb2_queue(file, vdev);
>
> -       return vb2_get_unmapped_area(vdev->queue, addr, len, pgoff, flags);
> +       return vb2_get_unmapped_area(q, addr, len, pgoff, flags);
>  }
>  EXPORT_SYMBOL_GPL(vb2_fop_get_unmapped_area);
>  #endif

You need similar logic in v4l2-ioctl.c to retrieve the correct queue from
a video_device and v4l2_fh so v4l2_ioctl_get_lock retrieves the correct
lock.

Regards

John Cox