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

21325 diff mbox series

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

Hi,

Le lundi 23 septembre 2024 à 06:02 +0000, Harvey Yang a écrit :
> From: Han-Lin Chen <hanlinchen@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@chromium.org>
> Co-developed-by: Harvey Yang <chenghaoyang@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)
> +{
> +	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 ?

> +
> +/**
> + * \brief Queue a request with ioctl
> + * \param[in] requestFd The request file descriptor
> + *
> + * This function queues a request that was allocated before.
> + *
> + * \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));

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.

Nicolas

> +	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: "
> +					<< strerror(-ret);
> +
> +	return ret;
> +}
> +
>  } /* namespace libcamera */

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@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@chromium.org>
> > Co-developed-by: Harvey Yang <chenghaoyang@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 */
>

Hi Nicolas and Laurent,

On Wed, Sep 25, 2024 at 3:43 AM Laurent Pinchart
<laurent.pinchart@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@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@chromium.org>
> > > Co-developed-by: Harvey Yang <chenghaoyang@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