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

[Cheng-Hao Yang]

Hi Laurent,

On Mon, Nov 11, 2024 at 4:08 PM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> 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 ?

Yeah that was a question I asked in the cover letter. I'm not sure if passing
the request object pointer is necessary.

The question below shows the necessity of passing the pointer though.

>
> >   *
> >   * 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 ?

That's a good question. We assumed that the user of the request and buffers
should make sure the request is queued after being bound with buffer(s).

Maybe we can set the state transition within the request object:
{Idle, BoundToBuffers, Queued}
In "V4L2VideoDevice::queueBuffer()", we can pass the request object pointer
to transit from Idle/BoundToBuffers to BoundToBuffers.
In "Request::queueRequest()", let the state transit from BoundToBuffers to
Queued.
In "Request::reinit()", let the state transit from Queued to Idle.

WDYT?

BR,
Harvey

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