[libcamera-devel] [PATCH v3] v4l2: V4L2CameraProxy: Add support for PREPARE_BUF as one of the supported ioctl

[Laurent Pinchart]

On Fri, Jan 21, 2022 at 07:13:56PM +0900, paul.elder at ideasonboard.com wrote:
> On Wed, Jan 19, 2022 at 11:25:12AM +0530, Vedant Paranjape wrote:
> > Add support for PREPARE_BUF as one of the ioctl. Since this is a compat
> > layer, there doesn't seem to be an equivalent to the "transfer ownership
> > of the buffer to kernel driver" in V4L2Camera class.
> 
> + Thus, simply duplicate the checks done by vidioc_qbuf.
> 
> , perhaps? You describe a (minor) issue but no solution.
> 
> > To match the error checks done by kernel implementation, we'd have to
> > check if dmabuf fd is valid and that the buffer size is large enough.
> > Doing so will not add any particular value to the program as
> > applications most likely don't depend on these conditions being
> > handled correctly.
> 
> Would we add these once we support importing buffers, just for
> correctness?
> 
> > Signed-off-by: Vedant Paranjape <vedantparanjape160201 at gmail.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  src/v4l2/v4l2_camera_proxy.cpp | 35 +++++++++++++++++++++++++++++++++-
> >  src/v4l2/v4l2_camera_proxy.h   |  1 +
> >  2 files changed, 35 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> > index ec6daac605fb..f3470a6d312a 100644
> > --- a/src/v4l2/v4l2_camera_proxy.cpp
> > +++ b/src/v4l2/v4l2_camera_proxy.cpp
> > @@ -559,6 +559,38 @@ int V4L2CameraProxy::vidioc_querybuf(V4L2CameraFile *file, struct v4l2_buffer *a
> >  	return 0;
> >  }
> >  
> > +int V4L2CameraProxy::vidioc_prepare_buf(V4L2CameraFile *file, struct v4l2_buffer *arg)
> > +{
> > +	LOG(V4L2Compat, Debug)
> > +		<< "[" << file->description() << "] " << __func__
> > +		<< "(index=" << arg->index << ")";
> > +
> > +	if (!hasOwnership(file))
> > +		return -EBUSY;
> > +
> > +	if (arg->index >= bufferCount_)
> > +		return -EINVAL;
> > +
> > +	if (arg->flags & V4L2_BUF_FLAG_REQUEST_FD)
> > +		return -EINVAL;
> > +
> > +	if (!validateBufferType(arg->type) ||
> > +	    !validateMemoryType(arg->memory))
> > +		return -EINVAL;
> 
> I would've said that these checks should be in the same order as in
> qbuf, but I guess you've copied this order from the kernel. In which
> case qbuf should be fixed (in a separate patch; this one's fine).
> 
> > +
> > +	struct v4l2_buffer &buffer = buffers_[arg->index];
> > +
> > +	if (buffer.flags & V4L2_BUF_FLAG_QUEUED ||
> > +	    buffer.flags & V4L2_BUF_FLAG_PREPARED)
> > +		return -EINVAL;
> 
> Huh, I didn't realize that preparing a buf that's already been prepared
> would be an error. I thought you could just nop it. v4l2 docs haven't
> kept up I suppose :/
> 
> I would say that we could check the prepared flag in qbuf too, but these
> checks are few and constant time so it's probably find as-is.

You can call QBUF on a buffer that is prepared, so that shouldn't be a
problem. What I now realize is missing, however, is setting the
V4L2_BUF_FLAG_PREPARED flag in V4L2CameraProxy::vidioc_qbuf(). It can be
done in a separate patch, when fixing the order of checks in qbuf.

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

> Looks good.
> 
> With or without the suggested changes,
> 
> Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
> 
> > +
> > +	buffer.flags |= V4L2_BUF_FLAG_PREPARED;
> > +
> > +	arg->flags = buffer.flags;
> > +
> > +	return 0;
> > +}
> > +
> >  int V4L2CameraProxy::vidioc_qbuf(V4L2CameraFile *file, struct v4l2_buffer *arg)
> >  {
> >  	LOG(V4L2Compat, Debug)
> > @@ -627,7 +659,7 @@ int V4L2CameraProxy::vidioc_dqbuf(V4L2CameraFile *file, struct v4l2_buffer *arg,
> >  
> >  	struct v4l2_buffer &buf = buffers_[currentBuf_];
> >  
> > -	buf.flags &= ~(V4L2_BUF_FLAG_QUEUED | V4L2_BUF_FLAG_DONE);
> > +	buf.flags &= ~(V4L2_BUF_FLAG_QUEUED | V4L2_BUF_FLAG_DONE | V4L2_BUF_FLAG_PREPARED);
> >  	buf.length = sizeimage_;
> >  	*arg = buf;
> >  
> > @@ -729,6 +761,7 @@ const std::set<unsigned long> V4L2CameraProxy::supportedIoctls_ = {
> >  	VIDIOC_S_INPUT,
> >  	VIDIOC_REQBUFS,
> >  	VIDIOC_QUERYBUF,
> > +	VIDIOC_PREPARE_BUF,
> >  	VIDIOC_QBUF,
> >  	VIDIOC_DQBUF,
> >  	VIDIOC_EXPBUF,
> > diff --git a/src/v4l2/v4l2_camera_proxy.h b/src/v4l2/v4l2_camera_proxy.h
> > index a38b28c744d3..76ca2d8a5558 100644
> > --- a/src/v4l2/v4l2_camera_proxy.h
> > +++ b/src/v4l2/v4l2_camera_proxy.h
> > @@ -58,6 +58,7 @@ private:
> >  	int vidioc_s_input(V4L2CameraFile *file, int *arg);
> >  	int vidioc_reqbufs(V4L2CameraFile *file, struct v4l2_requestbuffers *arg);
> >  	int vidioc_querybuf(V4L2CameraFile *file, struct v4l2_buffer *arg);
> > +	int vidioc_prepare_buf(V4L2CameraFile *file, struct v4l2_buffer *arg);
> >  	int vidioc_qbuf(V4L2CameraFile *file, struct v4l2_buffer *arg);
> >  	int vidioc_dqbuf(V4L2CameraFile *file, struct v4l2_buffer *arg,
> >  			 libcamera::Mutex *lock) LIBCAMERA_TSA_REQUIRES(*lock);

-- 
Regards,

Laurent Pinchart