[PATCH 1/1] libcamera: Add request API support for media controller device

[Cheng-Hao Yang]

Hi Nicolas and Laurent,

On Wed, Sep 25, 2024 at 3:43 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hello,
>
> On Mon, Sep 23, 2024 at 02:09:51PM -0400, Nicolas Dufresne wrote:
> > Le lundi 23 septembre 2024 à 06:02 +0000, Harvey Yang a écrit :
> > > From: Han-Lin Chen <hanlinchen at chromium.org>
> > >
> > > This patch adds `allocateRequests` with MEDIA_IOC_REQUEST_ALLOC
> > > , `queueRequest` with MEDIA_REQUEST_IOC_QUEUE, and `reInitRequest`
> > > with MEDIA_REQUEST_IOC_REINIT.
> > >
> > > Signed-off-by: Han-Lin Chen <hanlinchen at chromium.org>
> > > Co-developed-by: Harvey Yang <chenghaoyang at chromium.org>
> > > ---
> > >  include/libcamera/internal/media_device.h |  4 ++
> > >  src/libcamera/media_device.cpp            | 87 +++++++++++++++++++++++
> > >  2 files changed, 91 insertions(+)
> > >
> > > diff --git a/include/libcamera/internal/media_device.h b/include/libcamera/internal/media_device.h
> > > index e412d3a0..8f54dd12 100644
> > > --- a/include/libcamera/internal/media_device.h
> > > +++ b/include/libcamera/internal/media_device.h
> > > @@ -53,6 +53,10 @@ public:
> > >     MediaLink *link(const MediaPad *source, const MediaPad *sink);
> > >     int disableLinks();
> > >
> > > +   int allocateRequests(unsigned int count, std::vector<UniqueFD> &requests);
> > > +   int queueRequest(int requestFd);
> > > +   int reInitRequest(int requestFd);
> > > +
> > >     Signal<> disconnected;
> > >
> > >  protected:
> > > diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp
> > > index bd054552..47b75809 100644
> > > --- a/src/libcamera/media_device.cpp
> > > +++ b/src/libcamera/media_device.cpp
> > > @@ -9,6 +9,7 @@
> > >
> > >  #include <errno.h>
> > >  #include <fcntl.h>
> > > +#include <poll.h>
> > >  #include <stdint.h>
> > >  #include <string>
> > >  #include <string.h>
> > > @@ -836,4 +837,90 @@ int MediaDevice::setupLink(const MediaLink *link, unsigned int flags)
> > >     return 0;
> > >  }
> > >
> > > +/**
> > > + * \brief Allocate \a count requests with ioctl
> > > + * \param[in] count The number of requests to be allocated
> > > + * \param[out] requests The request file descriptors to be returned
> > > + *
> > > + * This function returns request file descriptors if the MediaDevice supports
> > > + * requests. The file descriptors can then be queued and re-inited afterwards.
> > > + *
> > > + * \sa queueRequest(int requestFd)
> > > + * \sa reInitRequest(int requestFd)
> > > + *
> > > + * \return 0 on success or a negative error code otherwise
> > > + */
> > > +int MediaDevice::allocateRequests(unsigned int count, std::vector<UniqueFD> &requests)
>
> I think I would limit this function to allocating a single request, and
> handle the loop in the caller. This will give us a nicer function
> signature, and will allow the caller to decide what to do if only a
> subset of the requests can be allocated.

Done

>
> > > +{
> > > +   for (unsigned int i = 0; i < count; i++) {
> > > +           int fd;
> > > +           int ret = ioctl(fd_.get(), MEDIA_IOC_REQUEST_ALLOC, &fd);
> > > +           if (ret) {
> > > +                   LOG(MediaDevice, Error) << "Allocate request failed "
> > > +                                           << strerror(-ret);
> > > +                   return -EBUSY;
> > > +           }
> > > +           requests.emplace_back(fd);
> > > +   }
> > > +
> > > +   return 0;
> > > +}
> >
> > Wouldn't if be nicer if the request was an object ?
>
> I agree, I think that would be nicer. The reInitRequest() function would
> then become a member function of the MediaRequest class, and would be
> called reinit().

Added `MediaDevice::Request`. Please take a look.

>
> > > +
> > > +/**
> > > + * \brief Queue a request with ioctl
>
> "with ioctl" is an implementation detail. You can write "Queue a request
> to the media device".

Done

>
> > > + * \param[in] requestFd The request file descriptor
>
> This function should take a MediaRequest reference.

I make it a MediaDevice::Request's member function, so that
it doesn't take an input.

>
> > > + *
> > > + * This function queues a request that was allocated before.
>
> If you pass a request reference, the "that was allocated before" becomes
> implicit, you can drop it.

Done

>
> > > + *
> > > + * \sa allocateRequests(unsigned int count, std::vector<UniqueFD> &requests)
> > > + * \sa reInitRequest(int requestFd)
> > > + *
> > > + * \return 0 on success or a negative error code otherwise
> > > + */
> > > +int MediaDevice::queueRequest(int requestFd)
> > > +{
> > > +   int ret = ioctl(requestFd, MEDIA_REQUEST_IOC_QUEUE, NULL);
> > > +   if (ret)
> > > +           LOG(MediaDevice, Error) << "QueueRequest fd " << requestFd
> > > +                                   << "failed: " << strerror(-ret);
> > > +   return ret;
> > > +}
> > > +
> > > +/**
> > > + * \brief Re-init a request with ioctl
> > > + * \param[in] requestFd The request file descriptor
> > > + *
> > > + * This function recycles a request that was allocated and queued before. It
> > > + * polls on \a requestFd to ensure the request is completed, and reinits the
> > > + * request.
> > > + *
> > > + * \sa allocateRequests(unsigned int count, std::vector<UniqueFD> &requests)
> > > + * \sa queueRequest(int requestFd)
> > > + *
> > > + * \return 0 on success or a negative error code otherwise
> > > + */
> > > +int MediaDevice::reInitRequest(int requestFd)
> > > +{
> > > +   struct pollfd pfd;
> > > +
> > > +   pfd.fd = requestFd;
> > > +   pfd.events = POLLPRI;
> > > +
> > > +   int ret = TEMP_FAILURE_RETRY(poll(&pfd, 1, 300));
>
> I didn't know about TEMP_FAILURE_RETRY. Shouldn't we wrap fds in a class
> that provides posix function wrappers with EINTR handling, instead of
> springkling it manually through the code ?

Removed

>
> >
> > Shouldn't we split polling and re-init ? Polling is a very important feature of
> > Requests, since it can be used as a synchronization point, avoiding numerous
> > independent queue polling. Its also nicer if callers can control (or avoid) the
> > timeout.
>
> Agreed, and polling with a timeout is a bad idea. You'll block the
> camera thread for up to 300ms, that's way too long. Don't block,
> implement the behaviour you want in an event-driven way.

Used EventNotifier instead.

>
> > > +   if (ret < 0)
> > > +           LOG(MediaDevice, Error) << "The request " << requestFd
> > > +                                   << " polled failed: " << strerror(-ret);
> > > +   else if (ret == 0)
> > > +           LOG(MediaDevice, Error) << "The request " << requestFd
> > > +                                   << " polled timeout: " << strerror(-ret);
> > > +
> > > +   ret = ::ioctl(requestFd, MEDIA_REQUEST_IOC_REINIT, NULL);
> > > +   if (ret)
> > > +           LOG(MediaDevice, Error) << "The request " << requestFd
> > > +                                   << " is queued but not yet completed: "
>
> Is this the right message ?

I think so:
https://www.kernel.org/doc/html/v5.8/userspace-api/media/mediactl/media-request-ioc-reinit.html
It states the error EBUSY to be the case.
WDYT?


>
> > > +                                   << strerror(-ret);
> > > +
> > > +   return ret;
> > > +}
> > > +
> > >  } /* namespace libcamera */
> >
>
> --
> Regards,
>
> Laurent Pinchart