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

[Laurent Pinchart]

Hi Vedant,

Thank you for the patch.

On Sun, Jan 09, 2022 at 01:03:30PM +0530, Vedant Paranjape wrote:
> Add support for PREPARE_BUF as one of the ioctl. This needs adding the
> prepare_buf function to the v4l2_camera class as this operation involves

The class is called V4L2Camera, not v4l2_camera.

> passing on the ownership of buffers to the driver.
> 
> Signed-off-by: Vedant Paranjape <vedantparanjape160201 at gmail.com>
> ---
> I have not cargo copied the error checking or the function, I actually
> went to kernel source and checked what parameters are being checked to
> give out a error and checked those in my definition as well.
> 
> https://elixir.bootlin.com/linux/latest/source/drivers/media/common/videobuf2/videobuf2-v4l2.c#L729

Good idea !

> Please review the v4l2_camera_proxy part, if the function looks good
> enough hypothetically assuming vcam_->prepare_buf() has a correct
> implementation.
> 
> As for the part in v4l2_camera, prepare_buf. Seeing the pattern of code,
> any operation that needs exchanging buffers with the drivers needs to
> have a function to be implemented inside v4l2_camera just like qbuf is.
> Let me know if my assumption is wrong, and this change is absolutely not
> needed. 
> 
> I am unable to figure out what is the equivalent of transfering ownership 
> of the buffer in the libcamera compat world. Just like queueing a buffer
> has an equivalent, camera_->queueRequest(request) (not comparing apples
> to apples, but to some extent).

That's probably because there's no equivalent :-) I think you can just
drop the V4L2Camera::prepare_buf() function, and just mark the buffer
with V4L2_BUF_FLAG_PREPARED in V4L2CameraProxy::vidioc_prepare_buf() as
you're doing already. A V4L2 application will have no way to figure out
if the buffer has actually been prepared or not.

> ---
> src/v4l2/v4l2_camera.cpp       | 10 ++++++++++
>  src/v4l2/v4l2_camera.h         |  1 +
>  src/v4l2/v4l2_camera_proxy.cpp | 32 ++++++++++++++++++++++++++++++++
>  src/v4l2/v4l2_camera_proxy.h   |  1 +
>  4 files changed, 44 insertions(+)
> 
> diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp
> index e922b9e6aab2..2c155a63f836 100644
> --- a/src/v4l2/v4l2_camera.cpp
> +++ b/src/v4l2/v4l2_camera.cpp
> @@ -275,6 +275,16 @@ int V4L2Camera::qbuf(unsigned int index)
>  	return 0;
>  }
>  
> +int V4L2Camera::prepare_buf(unsigned int index, unsigned int memory)
> +{
> +	// just a stub for now
> +	// referring to
> +	// https://elixir.bootlin.com/linux/latest/source/drivers/media/common/videobuf2/videobuf2-v4l2.c#L729
> +	// https://elixir.bootlin.com/linux/latest/source/drivers/media/common/videobuf2/videobuf2-core.c#L1341
> +
> +	return 0;
> +}
> +
>  void V4L2Camera::waitForBufferAvailable()
>  {
>  	MutexLocker locker(bufferMutex_);
> diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h
> index 03e741180e8f..b4e4b65ed701 100644
> --- a/src/v4l2/v4l2_camera.h
> +++ b/src/v4l2/v4l2_camera.h
> @@ -57,6 +57,7 @@ public:
>  	int streamOff();
>  
>  	int qbuf(unsigned int index);
> +	int prepare_buf(unsigned int index, unsigned int memory);
>  
>  	void waitForBufferAvailable();
>  	bool isBufferAvailable();
> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> index 4d529bc29a4d..ae0b0691ca4e 100644
> --- a/src/v4l2/v4l2_camera_proxy.cpp
> +++ b/src/v4l2/v4l2_camera_proxy.cpp
> @@ -544,6 +544,37 @@ 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) << "Servicing vidioc_prepare_buf, index = "
> +			       << arg->index << " fd = " << file->efd();
> +
> +	if (arg->index >= bufferCount_)
> +		return -EINVAL;
> +
> +	if (arg->flags & V4L2_BUF_FLAG_REQUEST_FD ||
> +	    arg->flags & V4L2_BUF_FLAG_PREPARED)
> +		return -EINVAL;
> +
> +	if (!hasOwnership(file))
> +		return -EBUSY;

This check should go first, to mirror the order of
vb2_ioctl_prepare_buf().

> +
> +	if (!validateBufferType(arg->type) ||
> +	    !validateMemoryType(arg->memory))
> +		return -EINVAL;

You need to check the buffer state here (buffers_[arg->index].flags, not
arg->flags) to make sure it's currently dequeued, as it's invalid to
prepare a buffer that has already been prepared (see the first checks in
vb2_core_prepare_buf()).

If we wanted to fully match the error checks done by the kernel, we'd
have to check here that the buffer size is large enough, and that the
dmabuf fd is valid. I don't think it's worth it (especially the dmabuf
check), as those are error conditions that I can't imagine an
application would rely on being handled correctly. A comment to explain
this may be useful though.

> +
> +	/* \todo when DMABUF memory type is supported, add support here as well */
> +	int ret = vcam_->prepare_buf(arg->index, arg->memory);
> +	if (ret < 0)
> +		return ret;
> +
> +	buffers_[arg->index].flags |= V4L2_BUF_FLAG_PREPARED;
> +
> +	arg->flags = buffers_[arg->index].flags;
> +
> +	return ret;
> +}
> +
>  int V4L2CameraProxy::vidioc_qbuf(V4L2CameraFile *file, struct v4l2_buffer *arg)
>  {
>  	LOG(V4L2Compat, Debug) << "Servicing vidioc_qbuf, index = "
> @@ -709,6 +740,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 14e027c3e7d1..6baba94262a9 100644
> --- a/src/v4l2/v4l2_camera_proxy.h
> +++ b/src/v4l2/v4l2_camera_proxy.h
> @@ -57,6 +57,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