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

[Laurent Pinchart]

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 at 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 at 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.

>  	}

-- 
Regards,

Laurent Pinchart