[libcamera-devel,v2,11/17] v4l2: v4l2_camera_proxy: Reset buffer flags on reqbufs 0

4091 diff mbox series

VIDIOC_REQBUFS with count = 0 should also exhibit the same effects as
VIDIOC_STREAMOFF if the stream is on. Mainly, the queued and done flags
need to be cleared. Do these in the handler for VIDIOC_REQBUFS.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>

---
Changes in v2:
- call only the necessary components, instead of
  V4L2CameraProxy::vidioc_streamoff
---
 src/v4l2/v4l2_camera_proxy.cpp | 5 +++++
 1 file changed, 5 insertions(+)

Hi Paul,

Thank you for the patch.

On Fri, Jun 19, 2020 at 02:41:17PM +0900, Paul Elder wrote:
> VIDIOC_REQBUFS with count = 0 should also exhibit the same effects as
> VIDIOC_STREAMOFF if the stream is on.

That's what the V4L2 spec says, but it doesn't seem to correspond to the
vb2 implementation. Does v4l2-compliance check for this ? Otherwise I
think you should instead return -EBUSY is vcam_->isRunning().

> Mainly, the queued and done flags
> need to be cleared. Do these in the handler for VIDIOC_REQBUFS.

This however is needed, if buffer have been queued without calling
streamon, then they should be returned to the dequeued state.

> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> 
> ---
> Changes in v2:
> - call only the necessary components, instead of
>   V4L2CameraProxy::vidioc_streamoff

Unless I'm mistaken regarding the discrepancy between the spec and the
implementation, freeBuffers() shouldn't call vcam_->streamOff(). The
function would then only do

	vcam_->freeBuffers();
	bufferCount_ = 0;

and could be inlined in its caller.

> ---
>  src/v4l2/v4l2_camera_proxy.cpp | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> index 4d37662..ea9222e 100644
> --- a/src/v4l2/v4l2_camera_proxy.cpp
> +++ b/src/v4l2/v4l2_camera_proxy.cpp
> @@ -467,6 +467,11 @@ int V4L2CameraProxy::vidioc_reqbufs(V4L2CameraFile *cf, struct v4l2_requestbuffe
>  	memset(arg->reserved, 0, sizeof(arg->reserved));
>  
>  	if (arg->count == 0) {
> +		if (vcam_->isRunning()) {
> +			for (struct v4l2_buffer &buf : buffers_)
> +				buf.flags &= ~(V4L2_BUF_FLAG_QUEUED | V4L2_BUF_FLAG_DONE);
> +		}
> +
>  		unlock(cf);
>  		return freeBuffers();
>  	}

Hi Laurent,

Thank you for the review.

On Sat, Jun 20, 2020 at 05:04:03AM +0300, Laurent Pinchart wrote:
> Hi Paul,
> 
> Thank you for the patch.
> 
> On Fri, Jun 19, 2020 at 02:41:17PM +0900, Paul Elder wrote:
> > VIDIOC_REQBUFS with count = 0 should also exhibit the same effects as
> > VIDIOC_STREAMOFF if the stream is on.
> 
> That's what the V4L2 spec says, but it doesn't seem to correspond to the
> vb2 implementation. Does v4l2-compliance check for this ? Otherwise I
> think you should instead return -EBUSY is vcam_->isRunning().

I see. More things that need to be fixed in the V4L2 spec :/
Looks it seems that v4l2-compliance doesn't care. I returned EBUSY for
reqbufs 0 when streaming (without clearing the buffer flags) and
v4l2-compliance didn't complain. It also didn't complain with what I had
here, though.

> > Mainly, the queued and done flags
> > need to be cleared. Do these in the handler for VIDIOC_REQBUFS.
> 
> This however is needed, if buffer have been queued without calling
> streamon, then they should be returned to the dequeued state.

ack

> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > 
> > ---
> > Changes in v2:
> > - call only the necessary components, instead of
> >   V4L2CameraProxy::vidioc_streamoff
> 
> Unless I'm mistaken regarding the discrepancy between the spec and the
> implementation, freeBuffers() shouldn't call vcam_->streamOff(). The
> function would then only do
> 
> 	vcam_->freeBuffers();
> 	bufferCount_ = 0;
> 
> and could be inlined in its caller.

Well it's called in a couple of places (both in reqbufs, for count = 0
and > 0) so I think I'll keep it.

> > ---
> >  src/v4l2/v4l2_camera_proxy.cpp | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> > index 4d37662..ea9222e 100644
> > --- a/src/v4l2/v4l2_camera_proxy.cpp
> > +++ b/src/v4l2/v4l2_camera_proxy.cpp
> > @@ -467,6 +467,11 @@ int V4L2CameraProxy::vidioc_reqbufs(V4L2CameraFile *cf, struct v4l2_requestbuffe
> >  	memset(arg->reserved, 0, sizeof(arg->reserved));
> >  
> >  	if (arg->count == 0) {
> > +		if (vcam_->isRunning()) {
> > +			for (struct v4l2_buffer &buf : buffers_)
> > +				buf.flags &= ~(V4L2_BUF_FLAG_QUEUED | V4L2_BUF_FLAG_DONE);
> > +		}
> > +
> >  		unlock(cf);
> >  		return freeBuffers();
> >  	}


Thanks,

Paul