Re: [RFCv4 13/21] media: videobuf2-v4l2: support for requests

[Tomasz Figa]

On Fri, Feb 23, 2018 at 4:21 PM, Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
> On 02/23/2018 07:34 AM, Tomasz Figa wrote:
>> On Wed, Feb 21, 2018 at 1:18 AM, Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
>>> On 02/20/2018 05:44 AM, Alexandre Courbot wrote:
>>>> Add a new vb2_qbuf_request() (a request-aware version of vb2_qbuf())
>>>> that request-aware drivers can call to queue a buffer into a request
>>>> instead of directly into the vb2 queue if relevent.
>>>>
>>>> This function expects that drivers invoking it are using instances of
>>>> v4l2_request_entity and v4l2_request_entity_data to describe their
>>>> entity and entity data respectively.
>>>>
>>>> Also add the vb2_request_submit() helper function which drivers can
>>>> invoke in order to queue all the buffers previously queued into a
>>>> request into the regular vb2 queue.
>>>>
>>>> Signed-off-by: Alexandre Courbot <acourbot@xxxxxxxxxxxx>
>>>> ---
>>>>  .../media/common/videobuf2/videobuf2-v4l2.c   | 129 +++++++++++++++++-
>>>>  include/media/videobuf2-v4l2.h                |  59 ++++++++
>>>>  2 files changed, 187 insertions(+), 1 deletion(-)
>>>>
>>>
>>> <snip>
>>>
>>>> @@ -776,10 +899,14 @@ 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 v4l2_fh *fh = NULL;
>>>> +
>>>> +     if (test_bit(V4L2_FL_USES_V4L2_FH, &vdev->flags))
>>>> +             fh = file->private_data;
>>>
>>> No need for this. All drivers using vb2 will also use v4l2_fh.
>>>
>>>>
>>>>       if (vb2_queue_is_busy(vdev, file))
>>>>               return -EBUSY;
>>>> -     return vb2_qbuf(vdev->queue, p);
>>>> +     return vb2_qbuf_request(vdev->queue, p, fh ? fh->entity : NULL);
>>>>  }
>>>>  EXPORT_SYMBOL_GPL(vb2_ioctl_qbuf);
>>>>
>>>> diff --git a/include/media/videobuf2-v4l2.h b/include/media/videobuf2-v4l2.h
>>>> index 3d5e2d739f05..d4dfa266a0da 100644
>>>> --- a/include/media/videobuf2-v4l2.h
>>>> +++ b/include/media/videobuf2-v4l2.h
>>>> @@ -23,6 +23,12 @@
>>>>  #error VB2_MAX_PLANES != VIDEO_MAX_PLANES
>>>>  #endif
>>>>
>>>> +struct media_entity;
>>>> +struct v4l2_fh;
>>>> +struct media_request;
>>>> +struct media_request_entity;
>>>> +struct v4l2_request_entity_data;
>>>> +
>>>>  /**
>>>>   * struct vb2_v4l2_buffer - video buffer information for v4l2.
>>>>   *
>>>> @@ -116,6 +122,59 @@ int vb2_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b);
>>>>   */
>>>>  int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b);
>>>>
>>>> +#if IS_ENABLED(CONFIG_MEDIA_REQUEST_API)
>>>> +
>>>> +/**
>>>> + * vb2_qbuf_request() - Queue a buffer, with request support
>>>> + * @q:               pointer to &struct vb2_queue with videobuf2 queue.
>>>> + * @b:               buffer structure passed from userspace to
>>>> + *           &v4l2_ioctl_ops->vidioc_qbuf handler in driver
>>>> + * @entity:  request entity to queue for if requests are used.
>>>> + *
>>>> + * Should be called from &v4l2_ioctl_ops->vidioc_qbuf handler of a driver.
>>>> + *
>>>> + * If requests are not in use, calling this is equivalent to calling vb2_qbuf().
>>>> + *
>>>> + * If the request_fd member of b is set, then the buffer represented by b is
>>>> + * queued in the request instead of the vb2 queue. The buffer will be passed
>>>> + * to the vb2 queue when the request is submitted.
>>>
>>> I would definitely also prepare the buffer at this time. That way you'll see any
>>> errors relating to the prepare early on.
>>
>> Would the prepare operation be completely independent of other state?
>> I can see a case when how the buffer is to be prepared may depend on
>> values of some controls. If so, it would be only possible at request
>> submission time. Alternatively, the application would have to be
>> mandated to include any controls that may affect buffer preparing in
>> the request and before the QBUF is called.
>
> The buffer is just memory. Controls play no role here. So the prepare
> operation is indeed independent of other state. Anything else would make
> this horrible complicated. And besides, with buffers allocated by other
> subsystems (dmabuf) how could controls from our subsystems ever affect
> those? The videobuf(2) frameworks have always just operated on memory
> buffers without any knowledge of what it contains.

What you said applies to the videobuf(2) frameworks, but driver
callback is explicitly defined as having access to the buffer
contents:

 * @buf_prepare: called every time the buffer is queued from userspace
 * and from the VIDIOC_PREPARE_BUF() ioctl; drivers may
 * perform any initialization required before each
 * hardware operation in this callback; drivers can
 * access/modify the buffer here as it is still synced for
 * the CPU; drivers that support VIDIOC_CREATE_BUFS() must
 * also validate the buffer size; if an error is returned,
 * the buffer will not be queued in driver; optional.

https://elixir.bootlin.com/linux/latest/source/include/media/videobuf2-core.h#L330

Best regards,
Tomasz