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

[Laurent Pinchart]

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.

> 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.

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

-- 
Regards,

Laurent Pinchart