| Message ID | 20180220044425.169493-14-acourbot@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@chromium.org> > --- > .../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. > + * > + * The return values from this function are intended to be directly returned > + * from &v4l2_ioctl_ops->vidioc_qbuf handler in driver. > + */ > +int vb2_qbuf_request(struct vb2_queue *q, struct v4l2_buffer *b, > + struct media_request_entity *entity); > + > +/** > + * vb2_request_submit() - Queue all the buffers in a v4l2 request. > + * @data: request entity data to queue buffers of > + * > + * This function should be called from the media_request_entity_ops::submit > + * hook for instances of media_request_v4l2_entity_data. It will immediately > + * queue all the request-bound buffers to their respective vb2 queues. > + * > + * Errors from vb2_core_qbuf() are returned if something happened. Also, since > + * v4l2 request entities require at least one buffer for the request to trigger, > + * this function will return -EINVAL if no buffer have been bound at all for > + * this entity. > + */ > +int vb2_request_submit(struct v4l2_request_entity_data *data); > + > +#else /* CONFIG_MEDIA_REQUEST_API */ > + > +static inline int vb2_qbuf_request(struct vb2_queue *q, struct v4l2_buffer *b, > + struct media_request_entity *entity) > +{ > + return vb2_qbuf(q, b); > +} > + > +static inline int vb2_request_submit(struct v4l2_request_entity_data *data) > +{ > + return -ENOTSUPP; > +} > + > +#endif /* CONFIG_MEDIA_REQUEST_API */ > + > /** > * vb2_expbuf() - Export a buffer as a file descriptor > * @q: pointer to &struct vb2_queue with videobuf2 queue. > Regards, Hans
On Wed, Feb 21, 2018 at 1:18 AM, Hans Verkuil <hverkuil@xs4all.nl> 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@chromium.org> >> --- >> .../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. Fixed. > >> >> 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. I was wondering about that, so glad to have your opinion on this. Will make sure buffers are prepared before queuing them to a request. > >> + * >> + * The return values from this function are intended to be directly returned >> + * from &v4l2_ioctl_ops->vidioc_qbuf handler in driver. >> + */ >> +int vb2_qbuf_request(struct vb2_queue *q, struct v4l2_buffer *b, >> + struct media_request_entity *entity); >> + >> +/** >> + * vb2_request_submit() - Queue all the buffers in a v4l2 request. >> + * @data: request entity data to queue buffers of >> + * >> + * This function should be called from the media_request_entity_ops::submit >> + * hook for instances of media_request_v4l2_entity_data. It will immediately >> + * queue all the request-bound buffers to their respective vb2 queues. >> + * >> + * Errors from vb2_core_qbuf() are returned if something happened. Also, since >> + * v4l2 request entities require at least one buffer for the request to trigger, >> + * this function will return -EINVAL if no buffer have been bound at all for >> + * this entity. >> + */ >> +int vb2_request_submit(struct v4l2_request_entity_data *data); >> + >> +#else /* CONFIG_MEDIA_REQUEST_API */ >> + >> +static inline int vb2_qbuf_request(struct vb2_queue *q, struct v4l2_buffer *b, >> + struct media_request_entity *entity) >> +{ >> + return vb2_qbuf(q, b); >> +} >> + >> +static inline int vb2_request_submit(struct v4l2_request_entity_data *data) >> +{ >> + return -ENOTSUPP; >> +} >> + >> +#endif /* CONFIG_MEDIA_REQUEST_API */ >> + >> /** >> * vb2_expbuf() - Export a buffer as a file descriptor >> * @q: pointer to &struct vb2_queue with videobuf2 queue. >> > > Regards, > > Hans
On Wed, Feb 21, 2018 at 1:18 AM, Hans Verkuil <hverkuil@xs4all.nl> 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@chromium.org> >> --- >> .../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. Best regards, Tomasz
On 02/23/2018 07:34 AM, Tomasz Figa wrote: > On Wed, Feb 21, 2018 at 1:18 AM, Hans Verkuil <hverkuil@xs4all.nl> 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@chromium.org> >>> --- >>> .../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. Regards, Hans
On Fri, Feb 23, 2018 at 4:21 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote: > On 02/23/2018 07:34 AM, Tomasz Figa wrote: >> On Wed, Feb 21, 2018 at 1:18 AM, Hans Verkuil <hverkuil@xs4all.nl> 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@chromium.org> >>>> --- >>>> .../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
On 02/23/2018 08:33 AM, Tomasz Figa wrote: > On Fri, Feb 23, 2018 at 4:21 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote: >> On 02/23/2018 07:34 AM, Tomasz Figa wrote: >>> On Wed, Feb 21, 2018 at 1:18 AM, Hans Verkuil <hverkuil@xs4all.nl> 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@chromium.org> >>>>> --- >>>>> .../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. Ah, good point. So the prepare step should be taken when the request is submitted. You definitely want to know at submit time if all the buffers in the request can be prepared so you can return an error. Userspace can also prepare the buffer with VIDIOC_BUF_PREPARE before queuing it into the request if it wants to. Regards, Hans
Hi, On Tue, 2018-02-20 at 13:44 +0900, 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. See a comment/proposal below about an issue I encountered when using multi-planar formats. > Signed-off-by: Alexandre Courbot <acourbot@chromium.org> > --- > .../media/common/videobuf2/videobuf2-v4l2.c | 129 > +++++++++++++++++- > include/media/videobuf2-v4l2.h | 59 ++++++++ > 2 files changed, 187 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c > b/drivers/media/common/videobuf2/videobuf2-v4l2.c > index 6d4d184aa68e..0627c3339572 100644 > --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c > +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c [...] > +#if IS_ENABLED(CONFIG_MEDIA_REQUEST_API) > +int vb2_qbuf_request(struct vb2_queue *q, struct v4l2_buffer *b, > + struct media_request_entity *entity) > +{ > + struct v4l2_request_entity_data *data; > + struct v4l2_vb2_request_buffer *qb; > + struct media_request *req; > + struct vb2_buffer *vb; > + int ret = 0; > + > + if (b->request_fd <= 0) > + return vb2_qbuf(q, b); > + > + if (!q->allow_requests) > + return -EINVAL; > + > + req = media_request_get_from_fd(b->request_fd); > + if (!req) > + return -EINVAL; > + > + data = to_v4l2_entity_data(media_request_get_entity_data(req, > entity)); > + if (IS_ERR(data)) { > + ret = PTR_ERR(data); > + goto out; > + } > + > + mutex_lock(&req->lock); > + > + if (req->state != MEDIA_REQUEST_STATE_IDLE) { > + ret = -EINVAL; > + goto out; > + } > + > + ret = vb2_queue_or_prepare_buf(q, b, "qbuf"); > + if (ret) > + goto out; > + > + vb = q->bufs[b->index]; > + switch (vb->state) { > + case VB2_BUF_STATE_DEQUEUED: > + break; > + case VB2_BUF_STATE_PREPARED: > + break; > + case VB2_BUF_STATE_PREPARING: > + dprintk(1, "buffer still being prepared\n"); > + ret = -EINVAL; > + goto out; > + default: > + dprintk(1, "invalid buffer state %d\n", vb->state); > + ret = -EINVAL; > + goto out; > + } > + > + /* do we already have a buffer for this request in the queue? > */ > + list_for_each_entry(qb, &data->queued_buffers, node) { > + if (qb->queue == q) { > + ret = -EBUSY; > + goto out; > + } > + } > + > + qb = kzalloc(sizeof(*qb), GFP_KERNEL); > + if (!qb) { > + ret = -ENOMEM; > + goto out; > + } > + > + /* > + * TODO should be prepare the buffer here if needed, to > report errors > + * early? > + */ > + qb->pre_req_state = vb->state; > + qb->queue = q; > + memcpy(&qb->v4l2_buf, b, sizeof(*b)); I am getting data corruption on qb->v4l2_buf.m.planes from this when using planar buffers, only after exiting the ioctl handler (i.e. when accessing this buffer later from the queue). I initially thought this was because the planes pointer was copied as-is from userspace, but Maxime Ripard suggested that this would have automatically triggered a visible fault in the kernel. Thus, my best guess is that the data is properly copied from userspace but freed when leaving the ioctl handler, which doesn't work our with the request API. A dirty fix that I came up with consists in re-allocating the planes buffer here and copying its contents from "b", so that it can live beyond the ioctl call. I am not too sure whether this should be fixed here or in the part of the v4l2 common code that frees this data. What do you think? Cheers!
On Thu, Mar 8, 2018 at 1:50 AM, Paul Kocialkowski <paul.kocialkowski@bootlin.com> wrote: > Hi, > > On Tue, 2018-02-20 at 13:44 +0900, 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. > > See a comment/proposal below about an issue I encountered when using > multi-planar formats. > >> Signed-off-by: Alexandre Courbot <acourbot@chromium.org> >> --- >> .../media/common/videobuf2/videobuf2-v4l2.c | 129 >> +++++++++++++++++- >> include/media/videobuf2-v4l2.h | 59 ++++++++ >> 2 files changed, 187 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c >> b/drivers/media/common/videobuf2/videobuf2-v4l2.c >> index 6d4d184aa68e..0627c3339572 100644 >> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c >> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c > > [...] > >> +#if IS_ENABLED(CONFIG_MEDIA_REQUEST_API) >> +int vb2_qbuf_request(struct vb2_queue *q, struct v4l2_buffer *b, >> + struct media_request_entity *entity) >> +{ >> + struct v4l2_request_entity_data *data; >> + struct v4l2_vb2_request_buffer *qb; >> + struct media_request *req; >> + struct vb2_buffer *vb; >> + int ret = 0; >> + >> + if (b->request_fd <= 0) >> + return vb2_qbuf(q, b); >> + >> + if (!q->allow_requests) >> + return -EINVAL; >> + >> + req = media_request_get_from_fd(b->request_fd); >> + if (!req) >> + return -EINVAL; >> + >> + data = to_v4l2_entity_data(media_request_get_entity_data(req, >> entity)); >> + if (IS_ERR(data)) { >> + ret = PTR_ERR(data); >> + goto out; >> + } >> + >> + mutex_lock(&req->lock); >> + >> + if (req->state != MEDIA_REQUEST_STATE_IDLE) { >> + ret = -EINVAL; >> + goto out; >> + } >> + >> + ret = vb2_queue_or_prepare_buf(q, b, "qbuf"); >> + if (ret) >> + goto out; >> + >> + vb = q->bufs[b->index]; >> + switch (vb->state) { >> + case VB2_BUF_STATE_DEQUEUED: >> + break; >> + case VB2_BUF_STATE_PREPARED: >> + break; >> + case VB2_BUF_STATE_PREPARING: >> + dprintk(1, "buffer still being prepared\n"); >> + ret = -EINVAL; >> + goto out; >> + default: >> + dprintk(1, "invalid buffer state %d\n", vb->state); >> + ret = -EINVAL; >> + goto out; >> + } >> + >> + /* do we already have a buffer for this request in the queue? >> */ >> + list_for_each_entry(qb, &data->queued_buffers, node) { >> + if (qb->queue == q) { >> + ret = -EBUSY; >> + goto out; >> + } >> + } >> + >> + qb = kzalloc(sizeof(*qb), GFP_KERNEL); >> + if (!qb) { >> + ret = -ENOMEM; >> + goto out; >> + } >> + >> + /* >> + * TODO should be prepare the buffer here if needed, to >> report errors >> + * early? >> + */ >> + qb->pre_req_state = vb->state; >> + qb->queue = q; >> + memcpy(&qb->v4l2_buf, b, sizeof(*b)); > > I am getting data corruption on qb->v4l2_buf.m.planes from this when > using planar buffers, only after exiting the ioctl handler (i.e. when > accessing this buffer later from the queue). > > I initially thought this was because the planes pointer was copied as-is > from userspace, but Maxime Ripard suggested that this would have > automatically triggered a visible fault in the kernel. > > Thus, my best guess is that the data is properly copied from userspace > but freed when leaving the ioctl handler, which doesn't work our with > the request API. > > A dirty fix that I came up with consists in re-allocating the planes > buffer here and copying its contents from "b", so that it can live > beyond the ioctl call. > > I am not too sure whether this should be fixed here or in the part of > the v4l2 common code that frees this data. What do you think? Oh, nice catch. Copying plane information indeed requires more work than a dumb memcpy(). I suppose this should be handled here, but let me come back to this after a good night of sleep. :) Thanks! I will try to fix this tomorrow and push a temporary version somewhere so you can move forward. Alex.
diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c index 6d4d184aa68e..0627c3339572 100644 --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c @@ -28,8 +28,11 @@ #include <media/v4l2-fh.h> #include <media/v4l2-event.h> #include <media/v4l2-common.h> +#include <media/v4l2-ioctl.h> +#include <media/v4l2-request.h> #include <media/videobuf2-v4l2.h> +#include <media/media-request.h> static int debug; module_param(debug, int, 0644); @@ -569,11 +572,131 @@ int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b) return -EBUSY; } + /* drivers supporting requests must call vb2_qbuf_request instead */ + if (b->request_fd > 0) { + dprintk(1, "invalid call to vb2_qbuf with request_fd set\n"); + return -EINVAL; + } + ret = vb2_queue_or_prepare_buf(q, b, "qbuf"); return ret ? ret : vb2_core_qbuf(q, b->index, b); } EXPORT_SYMBOL_GPL(vb2_qbuf); +#if IS_ENABLED(CONFIG_MEDIA_REQUEST_API) +int vb2_qbuf_request(struct vb2_queue *q, struct v4l2_buffer *b, + struct media_request_entity *entity) +{ + struct v4l2_request_entity_data *data; + struct v4l2_vb2_request_buffer *qb; + struct media_request *req; + struct vb2_buffer *vb; + int ret = 0; + + if (b->request_fd <= 0) + return vb2_qbuf(q, b); + + if (!q->allow_requests) + return -EINVAL; + + req = media_request_get_from_fd(b->request_fd); + if (!req) + return -EINVAL; + + data = to_v4l2_entity_data(media_request_get_entity_data(req, entity)); + if (IS_ERR(data)) { + ret = PTR_ERR(data); + goto out; + } + + mutex_lock(&req->lock); + + if (req->state != MEDIA_REQUEST_STATE_IDLE) { + ret = -EINVAL; + goto out; + } + + ret = vb2_queue_or_prepare_buf(q, b, "qbuf"); + if (ret) + goto out; + + vb = q->bufs[b->index]; + switch (vb->state) { + case VB2_BUF_STATE_DEQUEUED: + break; + case VB2_BUF_STATE_PREPARED: + break; + case VB2_BUF_STATE_PREPARING: + dprintk(1, "buffer still being prepared\n"); + ret = -EINVAL; + goto out; + default: + dprintk(1, "invalid buffer state %d\n", vb->state); + ret = -EINVAL; + goto out; + } + + /* do we already have a buffer for this request in the queue? */ + list_for_each_entry(qb, &data->queued_buffers, node) { + if (qb->queue == q) { + ret = -EBUSY; + goto out; + } + } + + qb = kzalloc(sizeof(*qb), GFP_KERNEL); + if (!qb) { + ret = -ENOMEM; + goto out; + } + + /* + * TODO should be prepare the buffer here if needed, to report errors + * early? + */ + qb->pre_req_state = vb->state; + qb->queue = q; + memcpy(&qb->v4l2_buf, b, sizeof(*b)); + vb->request = req; + vb->state = VB2_BUF_STATE_QUEUED; + list_add_tail(&qb->node, &data->queued_buffers); + +out: + mutex_unlock(&req->lock); + media_request_put(req); + + return ret; +} +EXPORT_SYMBOL_GPL(vb2_qbuf_request); + +int vb2_request_submit(struct v4l2_request_entity_data *data) +{ + struct v4l2_vb2_request_buffer *qb, *n; + + /* v4l2 requests require at least one buffer to reach the device */ + if (list_empty(&data->queued_buffers)) { + return -EINVAL; + } + + list_for_each_entry_safe(qb, n, &data->queued_buffers, node) { + struct vb2_queue *q = qb->queue; + struct vb2_buffer *vb = q->bufs[qb->v4l2_buf.index]; + int ret; + + list_del(&qb->node); + vb->state = qb->pre_req_state; + ret = vb2_core_qbuf(q, vb->index, &qb->v4l2_buf); + kfree(qb); + if (ret) + return ret; + } + + return 0; +} +EXPORT_SYMBOL_GPL(vb2_request_submit); + +#endif /* CONFIG_MEDIA_REQUEST_API */ + int vb2_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool nonblocking) { int ret; @@ -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; 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. + * + * The return values from this function are intended to be directly returned + * from &v4l2_ioctl_ops->vidioc_qbuf handler in driver. + */ +int vb2_qbuf_request(struct vb2_queue *q, struct v4l2_buffer *b, + struct media_request_entity *entity); + +/** + * vb2_request_submit() - Queue all the buffers in a v4l2 request. + * @data: request entity data to queue buffers of + * + * This function should be called from the media_request_entity_ops::submit + * hook for instances of media_request_v4l2_entity_data. It will immediately + * queue all the request-bound buffers to their respective vb2 queues. + * + * Errors from vb2_core_qbuf() are returned if something happened. Also, since + * v4l2 request entities require at least one buffer for the request to trigger, + * this function will return -EINVAL if no buffer have been bound at all for + * this entity. + */ +int vb2_request_submit(struct v4l2_request_entity_data *data); + +#else /* CONFIG_MEDIA_REQUEST_API */ + +static inline int vb2_qbuf_request(struct vb2_queue *q, struct v4l2_buffer *b, + struct media_request_entity *entity) +{ + return vb2_qbuf(q, b); +} + +static inline int vb2_request_submit(struct v4l2_request_entity_data *data) +{ + return -ENOTSUPP; +} + +#endif /* CONFIG_MEDIA_REQUEST_API */ + /** * vb2_expbuf() - Export a buffer as a file descriptor * @q: pointer to &struct vb2_queue with videobuf2 queue.
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@chromium.org> --- .../media/common/videobuf2/videobuf2-v4l2.c | 129 +++++++++++++++++- include/media/videobuf2-v4l2.h | 59 ++++++++ 2 files changed, 187 insertions(+), 1 deletion(-)