[libcamera-devel,v3,15/22] v4l2: v4l2_camera_proxy: Reset buffer flags on reqbufs 0

4189 diff mbox series

Clear the queued and done buffer flags on VIDIOC_REQBUFS with count = 0
if the stream is not on. If the stream is on and reqbufs is called with
count = 0, return -EBUSY.

Note that this is contrary to what the V4L2 docs say (reqbufs 0 when
streaming should also streamoff), but it is how the V4L2 implementation
works. v4l2-compliance doesn't seem to care either way, however, so we
cater to the implementation.

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

---
Changes in v3:
- don't streamoff in reqbufs 0; return -EBUSY instead

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

Hi Paul,

Thank you for the patch.

On Wed, Jun 24, 2020 at 04:08:29AM +0900, Paul Elder wrote:
> Clear the queued and done buffer flags on VIDIOC_REQBUFS with count = 0
> if the stream is not on. If the stream is on and reqbufs is called with
> count = 0, return -EBUSY.
> 
> Note that this is contrary to what the V4L2 docs say (reqbufs 0 when
> streaming should also streamoff), but it is how the V4L2 implementation
> works. v4l2-compliance doesn't seem to care either way, however, so we
> cater to the implementation.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> 
> ---
> Changes in v3:
> - don't streamoff in reqbufs 0; return -EBUSY instead
> 
> Changes in v2:
> - call only the necessary components, instead of
>   V4L2CameraProxy::vidioc_streamoff
> ---
>  src/v4l2/v4l2_camera_proxy.cpp | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> index 06fef21..fa58585 100644
> --- a/src/v4l2/v4l2_camera_proxy.cpp
> +++ b/src/v4l2/v4l2_camera_proxy.cpp
> @@ -444,11 +444,6 @@ int V4L2CameraProxy::freeBuffers()
>  {
>  	LOG(V4L2Compat, Debug) << "Freeing libcamera bufs";
>  
> -	int ret = vcam_->streamOff();
> -	if (ret < 0) {
> -		LOG(V4L2Compat, Error) << "Failed to stop stream";
> -		return ret;
> -	}
>  	vcam_->freeBuffers();
>  	bufferCount_ = 0;
>  
> @@ -476,6 +471,12 @@ int V4L2CameraProxy::vidioc_reqbufs(V4L2CameraFile *file, struct v4l2_requestbuf
>  	memset(arg->reserved, 0, sizeof(arg->reserved));
>  
>  	if (arg->count == 0) {
> +		if (vcam_->isRunning())
> +			return -EBUSY;
> +
> +		for (struct v4l2_buffer &buf : buffers_)
> +			buf.flags &= ~(V4L2_BUF_FLAG_QUEUED | V4L2_BUF_FLAG_DONE);
> +

Is this the correct fix ? Shouldn't you call buffers_.clear() instead ?
I'd do so in V4L2CameraProxy::freeBuffers().

With this fixed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  		release(file);
>  		return freeBuffers();

Additionally, you probably want to avoid calling release() if
freeBuffers() failed. This should probably be done in the patch that
introduces the release() call. And in the error paths below in this
function, you probably want to call release() too. To keep this simple,
maybe acquire() should be moved to the end, with just

	if (!hasOwnership() && owner_)
		return -EBUSY;

check at the top.

>  	}

Hi Laurent,

Thank you for the review.

On Wed, Jun 24, 2020 at 01:55:11AM +0300, Laurent Pinchart wrote:
> Hi Paul,
> 
> Thank you for the patch.
> 
> On Wed, Jun 24, 2020 at 04:08:29AM +0900, Paul Elder wrote:
> > Clear the queued and done buffer flags on VIDIOC_REQBUFS with count = 0
> > if the stream is not on. If the stream is on and reqbufs is called with
> > count = 0, return -EBUSY.
> > 
> > Note that this is contrary to what the V4L2 docs say (reqbufs 0 when
> > streaming should also streamoff), but it is how the V4L2 implementation
> > works. v4l2-compliance doesn't seem to care either way, however, so we
> > cater to the implementation.
> > 
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > 
> > ---
> > Changes in v3:
> > - don't streamoff in reqbufs 0; return -EBUSY instead
> > 
> > Changes in v2:
> > - call only the necessary components, instead of
> >   V4L2CameraProxy::vidioc_streamoff
> > ---
> >  src/v4l2/v4l2_camera_proxy.cpp | 11 ++++++-----
> >  1 file changed, 6 insertions(+), 5 deletions(-)
> > 
> > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> > index 06fef21..fa58585 100644
> > --- a/src/v4l2/v4l2_camera_proxy.cpp
> > +++ b/src/v4l2/v4l2_camera_proxy.cpp
> > @@ -444,11 +444,6 @@ int V4L2CameraProxy::freeBuffers()
> >  {
> >  	LOG(V4L2Compat, Debug) << "Freeing libcamera bufs";
> >  
> > -	int ret = vcam_->streamOff();
> > -	if (ret < 0) {
> > -		LOG(V4L2Compat, Error) << "Failed to stop stream";
> > -		return ret;
> > -	}
> >  	vcam_->freeBuffers();
> >  	bufferCount_ = 0;
> >  
> > @@ -476,6 +471,12 @@ int V4L2CameraProxy::vidioc_reqbufs(V4L2CameraFile *file, struct v4l2_requestbuf
> >  	memset(arg->reserved, 0, sizeof(arg->reserved));
> >  
> >  	if (arg->count == 0) {
> > +		if (vcam_->isRunning())
> > +			return -EBUSY;
> > +
> > +		for (struct v4l2_buffer &buf : buffers_)
> > +			buf.flags &= ~(V4L2_BUF_FLAG_QUEUED | V4L2_BUF_FLAG_DONE);
> > +
> 
> Is this the correct fix ? Shouldn't you call buffers_.clear() instead ?
> I'd do so in V4L2CameraProxy::freeBuffers().

Ah yes, you're right.

> With this fixed,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> >  		release(file);
> >  		return freeBuffers();
> 
> Additionally, you probably want to avoid calling release() if
> freeBuffers() failed. This should probably be done in the patch that

Well, open -> reqbufs 0 is apparently expected to succeed, whereas
FrameBufferAllocator->free() would fail, so I'm thinking freeBuffers()
could just return void because it looks like V4L2 doesn't expect reqbufs
0 to ever fail.

I think I'll call freeBuffers() before release() though, because that
seems more correct (reset the status before relinquishing ownership).

> introduces the release() call. And in the error paths below in this
> function, you probably want to call release() too. To keep this simple,
> maybe acquire() should be moved to the end, with just
> 
> 	if (!hasOwnership() && owner_)
> 		return -EBUSY;
> 
> check at the top.

Yeah.

> >  	}
> 


Thanks,

Paul