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

[Paul Elder]

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 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().

Ah yes, you're right.

> 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

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