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

21860 diff mbox series

From: Han-Lin Chen <hanlinchen@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@chromium.org>
Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
---
 include/libcamera/internal/v4l2_videodevice.h | 2 +-
 src/libcamera/v4l2_videodevice.cpp            | 7 ++++++-
 2 files changed, 7 insertions(+), 2 deletions(-)

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@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@chromium.org>
> Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
> Signed-off-by: Harvey Yang <chenghaoyang@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();

Hi Laurent,

On Mon, Nov 11, 2024 at 4:08 PM Laurent Pinchart
<laurent.pinchart@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@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@chromium.org>
> > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
> > Signed-off-by: Harvey Yang <chenghaoyang@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

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@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@chromium.org>
> > > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
> > > Signed-off-by: Harvey Yang <chenghaoyang@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();

Hi Laurent,

On Tue, Nov 12, 2024 at 2:50 PM Laurent Pinchart
<laurent.pinchart@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@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@chromium.org>
> > > > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
> > > > Signed-off-by: Harvey Yang <chenghaoyang@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