[libcamera-devel] [PATCH v2 17/17] v4l2: v4l2_camera_proxy: Fix buffer flags related to queueing

[Paul Elder]

On Sat, Jun 20, 2020 at 05:33:14AM +0300, Laurent Pinchart wrote:
> Hi Paul,
> 
> Thank you for the patch.
> 
> On Fri, Jun 19, 2020 at 02:41:23PM +0900, Paul Elder wrote:
> > Fix buffer flags related to queueing and dequeueing:
> > - don't allow a buffer with the same index that is already in the queue
> >   to be enqueued again
> > - don't set the done flag on dequeueing
> 
> Why ? V4L2_BUF_FLAG_DONE is never set anymore, that doesn't seem right.

It is. It's set in V4L2CameraProxy::updateBuffers(), in the first patch
that added the compatibility layer.

> > - set the mapped flag on enqueueing
> 
> Why ? Isn't it valid to queued an unmapped buffer ?

That's what I thought too. Anyway looks like it complies fine without it
so I'll get rid of it.

> > - set the flags in V4L2CameraProxy's internal buffers, and not just in
> >   the buffers returned from qbuf
> 
> It's really tempting to bundle multiple changes in the last patch, right

Well they're related and small soo... :p

> ? :-) It would be nice to split this, especially given that for the
> second and third fixes a rationale should be given in the commit message
> (assuming the fixes are correct).

I'll expand the rationale then.

> > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > 
> > ---
> > New in v2
> > - split from "Fix v4l2-compliance streaming tests"
> > ---
> >  src/v4l2/v4l2_camera_proxy.cpp | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> > index 2eb2fcc..3e5fb7c 100644
> > --- a/src/v4l2/v4l2_camera_proxy.cpp
> > +++ b/src/v4l2/v4l2_camera_proxy.cpp
> > @@ -221,7 +221,6 @@ void V4L2CameraProxy::updateBuffers()
> >  			buf.timestamp.tv_usec = fmd.timestamp % 1000000;
> >  			buf.sequence = fmd.sequence;
> >  
> > -			buf.flags |= V4L2_BUF_FLAG_DONE;
> >  			break;
> >  		case FrameMetadata::FrameError:
> >  			buf.flags |= V4L2_BUF_FLAG_ERROR;
> > @@ -569,6 +568,9 @@ int V4L2CameraProxy::vidioc_qbuf(V4L2CameraFile *cf, struct v4l2_buffer *arg)
> >  	if (arg->index >= bufferCount_)
> >  		return -EINVAL;
> >  
> > +	if (buffers_[arg->index].flags & V4L2_BUF_FLAG_QUEUED)
> > +		return -EINVAL;
> > +
> >  	int ret = lock(cf);
> >  	if (ret < 0)
> >  		return ret;
> > @@ -582,9 +584,12 @@ int V4L2CameraProxy::vidioc_qbuf(V4L2CameraFile *cf, struct v4l2_buffer *arg)
> >  	if (ret < 0)
> >  		return ret;
> >  
> > -	arg->flags |= V4L2_BUF_FLAG_QUEUED;
> > +	arg->flags |= V4L2_BUF_FLAG_QUEUED | V4L2_BUF_FLAG_MAPPED;
> >  	arg->flags &= ~V4L2_BUF_FLAG_DONE;
> >  
> > +	buffers_[arg->index].flags |= V4L2_BUF_FLAG_QUEUED | V4L2_BUF_FLAG_MAPPED;
> > +	buffers_[arg->index].flags &= ~V4L2_BUF_FLAG_DONE;
> 
> Maybe
> 
> 	buffers_[arg->index].flags |= V4L2_BUF_FLAG_QUEUED | V4L2_BUF_FLAG_MAPPED;
> 	buffers_[arg->index].flags &= ~V4L2_BUF_FLAG_DONE;
>   	arg->flags = buffers_[arg->index].flags;
> 
> to avoid duplicating the logic ?
> 
> > +
> >  	return ret;
> >  }
> >  


Thanks,

Paul