[PATCH 1/1] libcamera: Extend V4L2 videodevice to accept media request API

[Laurent Pinchart]

Hi Harvey, Han-Lin,

Thank you for the patch.

On Fri, Nov 08, 2024 at 07:38:27AM +0000, Harvey Yang wrote:
> From: Han-Lin Chen <hanlinchen at chromium.org>
> 
> Add request fd as an extra argument to V4L2 device queueBuffer.
> Default to -1 for backward compatibility.
> 
> It's used to bind multiple buffers into the same request.
> 
> Signed-off-by: Han-Lin Chen <hanlinchen at chromium.org>
> Co-developed-by: Harvey Yang <chenghaoyang at chromium.org>
> Signed-off-by: Harvey Yang <chenghaoyang at chromium.org>
> ---
>  include/libcamera/internal/v4l2_videodevice.h | 2 +-
>  src/libcamera/v4l2_videodevice.cpp            | 7 ++++++-
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
> index f021c2a01..6e2b1e1f1 100644
> --- a/include/libcamera/internal/v4l2_videodevice.h
> +++ b/include/libcamera/internal/v4l2_videodevice.h
> @@ -218,7 +218,7 @@ public:
>  	int importBuffers(unsigned int count);
>  	int releaseBuffers();
>  
> -	int queueBuffer(FrameBuffer *buffer);
> +	int queueBuffer(FrameBuffer *buffer, int requestFd = -1);
>  	Signal<FrameBuffer *> bufferReady;
>  
>  	int streamOn();
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index 14eba0561..0c6ea086d 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -1604,6 +1604,7 @@ int V4L2VideoDevice::releaseBuffers()
>  /**
>   * \brief Queue a buffer to the video device
>   * \param[in] buffer The buffer to be queued
> + * \param[in] requestFd The fd of the request to queue the buffer to

Why do you pass the request FD and not the request object pointer ?

>   *
>   * For capture video devices the \a buffer will be filled with data by the
>   * device. For output video devices the \a buffer shall contain valid data and
> @@ -1618,7 +1619,7 @@ int V4L2VideoDevice::releaseBuffers()
>   *
>   * \return 0 on success or a negative error code otherwise
>   */
> -int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)
> +int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer, int requestFd)
>  {
>  	struct v4l2_plane v4l2Planes[VIDEO_MAX_PLANES] = {};
>  	struct v4l2_buffer buf = {};
> @@ -1647,6 +1648,10 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)
>  	buf.type = bufferType_;
>  	buf.memory = memoryType_;
>  	buf.field = V4L2_FIELD_NONE;
> +	if (requestFd >= 0) {
> +		buf.flags |= V4L2_BUF_FLAG_REQUEST_FD;
> +		buf.request_fd = requestFd;
> +	}

What will happen if MEDIA_REQUEST_IOC_REINIT is called on the request ?
As far as I understand, on the kernel side, the buffer is queued to vb2
only when the request is queued, while here, the buffer will be
considered queued as soon as queueBuffer() is called. If the request is
then reinitialized instead of being queued, the kernel and userspace
state will lose sync. How should this be addressed ?

>  
>  	bool multiPlanar = V4L2_TYPE_IS_MULTIPLANAR(buf.type);
>  	const std::vector<FrameBuffer::Plane> &planes = buffer->planes();

-- 
Regards,

Laurent Pinchart