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

[Laurent Pinchart]

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.

> > +{
> > +	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().

> > +
> > +/**
> > + * \brief Queue a request with ioctl

"with ioctl" is an implementation detail. You can write "Queue a request
to the media device".

> > + * \param[in] requestFd The request file descriptor

This function should take a MediaRequest reference.

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

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

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

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

> > +					<< strerror(-ret);
> > +
> > +	return ret;
> > +}
> > +
> >  } /* namespace libcamera */
> 

-- 
Regards,

Laurent Pinchart