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
