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

[Cheng-Hao Yang]

Hi Laurent,

On Tue, Nov 12, 2024 at 2:50 PM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Harvey,
>
> On Mon, Nov 11, 2024 at 05:02:43PM +0800, Cheng-Hao Yang wrote:
> > On Mon, Nov 11, 2024 at 4:08 PM Laurent Pinchart wrote:
> > > 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).
>
> Can that always be guaranteed, even in case of errors ? Graceful
> recovery of errors is important.

Ah, now I understand your question: You're worrying that
"Request::queueRequest()" is not called or returns an error after a
buffer is bound to a request. I was thinking that in my patch
https://patchwork.libcamera.org/patch/21817/
, `MEDIA_REQUEST_IOC_REINIT` is only called after
"Request::queueRequest()" and notified by the kernel/hardware that
the request is done.

>
> > 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?
>
> We can certainly keep track of request states, which would help catching
> errors such as trying to queue an already queued request, although I
> expect the kernel to return an error anyway.
>
> I'm not sure how that will help with the issue at hand though, which is
> that the internal state of the V4L2VideoDevice class can get out of sync
> with the device state.

I think we can register `V4L2VideoDevice`s into `MediaDevice::Request`,
and trigger their `dequeueBuffer()` function when
`MediaDevice::Request::queueRequest` fails.

Alternatively, we can let `MediaDevice::Request` handle queuing buffers
by registering the pairs of <V4L2VideoDevice*, FrameBuffer*>, and
calling `V4L2VideoDevice::queueBuffer()` right before
`ioctl(fd, MEDIA_REQUEST_IOC_QUEUE)`.

As for the state "buffer queued" out of sync, I don't have a good answer
now. If there's a component that needs to check if the request that a buffer
is bound to is queued or not, we can keep the request's pointer in
V4L2VideoDevice and add an API to get access to the request's state.

WDYT?

BR,
Harvey

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