[libcamera-devel] [PATCH 3/4] libcamera: event_dispatcher: Add interrupt() function

[Kieran Bingham]

Hi Laurent,

On 23/01/2019 08:59, Laurent Pinchart wrote:
> The new interrupt() function allows interrupting in-progress blocking
> processEvents() calls. This is useful to stop running event loops.


Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  include/libcamera/event_dispatcher.h          |  2 ++
>  src/libcamera/event_dispatcher.cpp            | 10 ++++++
>  src/libcamera/event_dispatcher_poll.cpp       | 32 ++++++++++++++++++-
>  src/libcamera/include/event_dispatcher_poll.h |  3 ++
>  4 files changed, 46 insertions(+), 1 deletion(-)
> 
> diff --git a/include/libcamera/event_dispatcher.h b/include/libcamera/event_dispatcher.h
> index c20518c6866d..cb06bf208e12 100644
> --- a/include/libcamera/event_dispatcher.h
> +++ b/include/libcamera/event_dispatcher.h
> @@ -26,6 +26,8 @@ public:
>  	virtual void unregisterTimer(Timer *timer) = 0;
>  
>  	virtual void processEvents() = 0;
> +
> +	virtual void interrupt() = 0;
>  };
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/event_dispatcher.cpp b/src/libcamera/event_dispatcher.cpp
> index f7c40734095e..b82c59c3f5dc 100644
> --- a/src/libcamera/event_dispatcher.cpp
> +++ b/src/libcamera/event_dispatcher.cpp
> @@ -104,4 +104,14 @@ EventDispatcher::~EventDispatcher()
>   * it before returning.
>   */
>  
> +/**
> + * \fn EventDispatcher::interrupt()
> + * \brief Interrupt any running processEvents() call as soon as possible
> + *
> + * Calling this function interrupts any blocking processEvents() call in
> + * progress. The processEvents() function will return as soon as possible,
> + * after processing pending timers and events. If processEvents() isn't in
> + * progress, it will be interrupted immediately the next time it gets called.
> + */
> +
>  } /* namespace libcamera */
> diff --git a/src/libcamera/event_dispatcher_poll.cpp b/src/libcamera/event_dispatcher_poll.cpp
> index 6e0609c34ddc..a2674ab31135 100644
> --- a/src/libcamera/event_dispatcher_poll.cpp
> +++ b/src/libcamera/event_dispatcher_poll.cpp
> @@ -8,7 +8,10 @@
>  #include <algorithm>
>  #include <iomanip>
>  #include <poll.h>
> +#include <stdint.h>
>  #include <string.h>
> +#include <sys/eventfd.h>
> +#include <unistd.h>
>  
>  #include <libcamera/event_notifier.h>
>  #include <libcamera/timer.h>
> @@ -43,10 +46,18 @@ static const char *notifierType(EventNotifier::Type type)
>  
>  EventDispatcherPoll::EventDispatcherPoll()
>  {
> +	/*
> +	 * Create the event fd. Failures are fatal as we can't implement an
> +	 * interruptible dispatcher without the fd.
> +	 */
> +	eventfd_ = eventfd(0, EFD_CLOEXEC | EFD_NONBLOCK);

Hrm ... I didn't know about eventfd. I'd always used a pair of pipes.

Every day's a school day :)

The man page says it's linux-specific - but I think we're OK with that :-)


> +	if (eventfd_ < 0)
> +		LOG(Event, Fatal) << "Unable to create eventfd";
>  }
>  
>  EventDispatcherPoll::~EventDispatcherPoll()
>  {
> +	close(eventfd_);
>  }
>  
>  void EventDispatcherPoll::registerEventNotifier(EventNotifier *notifier)
> @@ -123,11 +134,13 @@ void EventDispatcherPoll::processEvents()
>  
>  	/* Create the pollfd array. */
>  	std::vector<struct pollfd> pollfds;
> -	pollfds.reserve(notifiers_.size());
> +	pollfds.reserve(notifiers_.size() + 1);
>  
>  	for (auto notifier : notifiers_)
>  		pollfds.push_back({ notifier.first, notifier.second.events(), 0 });
>  
> +	pollfds.push_back({ eventfd_, POLLIN, 0 });
> +
>  	/* Wait for events and process notifiers and timers. */
>  	do {
>  		ret = poll(&pollfds);
> @@ -137,12 +150,20 @@ void EventDispatcherPoll::processEvents()
>  		ret = -errno;
>  		LOG(Event, Warning) << "poll() failed with " << strerror(-ret);
>  	} else if (ret > 0) {
> +		processInterrupt(pollfds.back());
> +		pollfds.pop_back();
>  		processNotifiers(pollfds);
>  	}
>  
>  	processTimers();
>  }
>  
> +void EventDispatcherPoll::interrupt()
> +{
> +	uint64_t value = 1;
> +	write(eventfd_, &value, sizeof(value));
> +}
> +
>  short EventDispatcherPoll::EventNotifierSetPoll::events() const
>  {
>  	short events = 0;
> @@ -186,6 +207,15 @@ int EventDispatcherPoll::poll(std::vector<struct pollfd> *pollfds)
>  		     nextTimer ? &timeout : nullptr, nullptr);
>  }
>  
> +void EventDispatcherPoll::processInterrupt(const struct pollfd &pfd)
> +{
> +	if (!pfd.revents & POLLIN)
> +		return;
> +
> +	uint64_t value;
> +	read(eventfd_, &value, sizeof(value));

Should we check that the value read was expected?

I guess this could be expanded upon to use an events bitfield or enum to
actually pass meaningful messages too.

I'm not sure what those event messages would be yet - so I don't think
we need to implement without a use-case.

--
Regards

Kieran


> +}
> +
>  void EventDispatcherPoll::processNotifiers(const std::vector<struct pollfd> &pollfds)
>  {
>  	static const struct {
> diff --git a/src/libcamera/include/event_dispatcher_poll.h b/src/libcamera/include/event_dispatcher_poll.h
> index ac3efde082b4..1c0066c24dc8 100644
> --- a/src/libcamera/include/event_dispatcher_poll.h
> +++ b/src/libcamera/include/event_dispatcher_poll.h
> @@ -31,6 +31,7 @@ public:
>  	void unregisterTimer(Timer *timer);
>  
>  	void processEvents();
> +	void interrupt();
>  
>  private:
>  	struct EventNotifierSetPoll {
> @@ -40,8 +41,10 @@ private:
>  
>  	std::map<int, EventNotifierSetPoll> notifiers_;
>  	std::list<Timer *> timers_;
> +	int eventfd_;
>  
>  	int poll(std::vector<struct pollfd> *pollfds);
> +	void processInterrupt(const struct pollfd &pfd);
>  	void processNotifiers(const std::vector<struct pollfd> &pollfds);
>  	void processTimers();
>  };
> 

-- 
Regards
--
Kieran