[libcamera-devel,v2,07/11] libcamera: Add a poll-based event dispatcher

170 diff mbox series

Provide a poll-based event dispatcher implementation as convenience for
applications that don't need a custom event loop. The poll-based
dispatcher is automatically instantiated if the application doesn't
provide its own dispatcher.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v1:

- Fix ASSERT() condition check
- Ignore duplicate notifiers
- Clarify warning messages for event notifier (un)registration errors
---
 include/libcamera/libcamera.h                 |   1 +
 src/libcamera/camera_manager.cpp              |  19 +-
 src/libcamera/event_dispatcher_poll.cpp       | 234 ++++++++++++++++++
 src/libcamera/include/event_dispatcher_poll.h |  50 ++++
 src/libcamera/meson.build                     |   2 +
 5 files changed, 301 insertions(+), 5 deletions(-)
 create mode 100644 src/libcamera/event_dispatcher_poll.cpp
 create mode 100644 src/libcamera/include/event_dispatcher_poll.h

Hi Laurent,
   mostly minor stuff here as well

On Tue, Jan 08, 2019 at 01:11:47AM +0200, Laurent Pinchart wrote:
> Provide a poll-based event dispatcher implementation as convenience for
> applications that don't need a custom event loop. The poll-based
> dispatcher is automatically instantiated if the application doesn't
> provide its own dispatcher.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Changes since v1:
>
> - Fix ASSERT() condition check
> - Ignore duplicate notifiers
> - Clarify warning messages for event notifier (un)registration errors
> ---
>  include/libcamera/libcamera.h                 |   1 +
>  src/libcamera/camera_manager.cpp              |  19 +-
>  src/libcamera/event_dispatcher_poll.cpp       | 234 ++++++++++++++++++
>  src/libcamera/include/event_dispatcher_poll.h |  50 ++++
>  src/libcamera/meson.build                     |   2 +
>  5 files changed, 301 insertions(+), 5 deletions(-)
>  create mode 100644 src/libcamera/event_dispatcher_poll.cpp
>  create mode 100644 src/libcamera/include/event_dispatcher_poll.h
>
> diff --git a/include/libcamera/libcamera.h b/include/libcamera/libcamera.h
> index 785babefa1c8..2dcaeda49812 100644
> --- a/include/libcamera/libcamera.h
> +++ b/include/libcamera/libcamera.h
> @@ -10,5 +10,6 @@
>  #include <libcamera/camera.h>
>  #include <libcamera/camera_manager.h>
>  #include <libcamera/event_dispatcher.h>
> +#include <libcamera/event_dispatcher_poll.h>
>
>  #endif /* __LIBCAMERA_LIBCAMERA_H__ */
> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> index cbfd977f1e3c..348ea2fedf64 100644
> --- a/src/libcamera/camera_manager.cpp
> +++ b/src/libcamera/camera_manager.cpp
> @@ -9,6 +9,7 @@
>  #include <libcamera/event_dispatcher.h>
>
>  #include "device_enumerator.h"
> +#include "event_dispatcher_poll.h"
>  #include "log.h"
>  #include "pipeline_handler.h"
>
> @@ -188,9 +189,10 @@ CameraManager *CameraManager::instance()
>   * \param dispatcher Pointer to the event dispatcher
>   *
>   * libcamera requires an event dispatcher to integrate event notification and
> - * timers with the application event loop. Applications shall call this function
> - * once and only once before the camera manager is started with start() to set
> - * the event dispatcher.
> + * timers with the application event loop. Applications that want to provide
> + * their own event dispatcher shall call this function once and only once before
> + * the camera manager is started with start(). If no event dispatcher is
> + * provided, a default poll-based implementation will be used.
>   *
>   * The CameraManager takes ownership of the event dispatcher and will delete it
>   * when the application terminates.
> @@ -208,11 +210,18 @@ void CameraManager::setEventDispatcher(EventDispatcher *dispatcher)
>  /**
>   * \fn CameraManager::eventDispatcher()
>   * \brief Retrieve the event dispatcher
> - * \return Pointer to the event dispatcher, or nullptr if no event dispatcher
> - * has been set
> + *
> + * This function retrieves the event dispatcher set with setEventDispatcher().
> + * If no dispatcher has been set, a default poll-based implementation is created
> + * and returned.

Is it worth pointing out that after this function has been called, it
is not possible to set a custom eventDispatcher anymore?

> + *
> + * \return Pointer to the event dispatcher
>   */
>  EventDispatcher *CameraManager::eventDispatcher()
>  {
> +	if (!dispatcher_)
> +		dispatcher_ = new EventDispatcherPoll();
> +
>  	return dispatcher_;
>  }
>
> diff --git a/src/libcamera/event_dispatcher_poll.cpp b/src/libcamera/event_dispatcher_poll.cpp
> new file mode 100644
> index 000000000000..0cd08b2b4cc1
> --- /dev/null
> +++ b/src/libcamera/event_dispatcher_poll.cpp
> @@ -0,0 +1,234 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * event_dispatcher_poll.cpp - Poll-based event dispatcher
> + */
> +
> +#include <algorithm>
> +#include <iomanip>
> +#include <poll.h>
> +#include <string.h>
> +
> +#include <libcamera/event_notifier.h>
> +#include <libcamera/timer.h>
> +
> +#include "event_dispatcher_poll.h"
> +#include "log.h"
> +
> +/**
> + * \file event_dispatcher_poll.h
> + */
> +
> +namespace libcamera {
> +
> +static const char *notifierType(EventNotifier::Type type)
> +{
> +	if (type == EventNotifier::Read)
> +		return "read";
> +	if (type == EventNotifier::Write)
> +		return "write";
> +	if (type == EventNotifier::Exception)
> +		return "exception";
> +
> +	return "";
> +}
> +
> +/**
> + * \class EventDispatcherPoll
> + * \brief A poll-based event dispatcher
> + */
> +

None of the functions here implemented is commented. Is this
intentional? Doxygen uses comments applied on the base class so it
does not actually complains...

> +EventDispatcherPoll::EventDispatcherPoll()
> +{
> +}
> +
> +EventDispatcherPoll::~EventDispatcherPoll()
> +{
> +}
> +
> +void EventDispatcherPoll::registerEventNotifier(EventNotifier *notifier)
> +{
> +	EventNotifierSetPoll &set = notifiers_[notifier->fd()];
> +	EventNotifier::Type type = notifier->type();
> +
> +	if (set.notifiers[type] && set.notifiers[type] != notifier) {
> +		LOG(Warning) << "Ignoring duplicate " << notifierType(type)
> +			     << " notifier for fd " << notifier->fd();
> +		return;
> +	}
> +
> +	set.notifiers[type] = notifier;
> +}
> +
> +void EventDispatcherPoll::unregisterEventNotifier(EventNotifier *notifier)
> +{
> +	auto iter = notifiers_.find(notifier->fd());
> +	if (iter == notifiers_.end())
> +		return;
> +
> +	EventNotifierSetPoll &set = iter->second;
> +	EventNotifier::Type type = notifier->type();
> +
> +	if (!set.notifiers[type])
> +		return;
> +
> +	if (set.notifiers[type] != notifier) {
> +		LOG(Warning) << notifierType(type) << " notifier for fd "
> +			     << notifier->fd() << " is not registered";
> +		return;
> +	}
> +
> +	set.notifiers[type] = nullptr;
> +	if (!set.notifiers[0] && !set.notifiers[1] && !set.notifiers[2])
> +		notifiers_.erase(iter);
> +}
> +
> +void EventDispatcherPoll::registerTimer(Timer *timer)
> +{
> +	for (auto iter = timers_.begin(); iter != timers_.end(); ++iter) {
> +		if ((*iter)->deadline() > timer->deadline()) {
> +			timers_.insert(iter, timer);
> +			return;
> +		}
> +	}
> +
> +	timers_.push_back(timer);
> +}
> +
> +void EventDispatcherPoll::unregisterTimer(Timer *timer)
> +{
> +	for (auto iter = timers_.begin(); iter != timers_.end(); ++iter) {
> +		if (*iter == timer) {
> +			timers_.erase(iter);
> +			return;
> +		}
> +
> +		/*
> +		 * As the timers list is ordered, we can stop as soon as we go
> +		 * past the deadline.
> +		 */
> +		if ((*iter)->deadline() > timer->deadline())
> +			break;
> +	}
> +}
> +

This function is probably worth being documented a bit. Just to point
out it is blocking maybe.

> +void EventDispatcherPoll::processEvents()
> +{
> +	int ret;
> +
> +	/* Create the pollfd array. */
> +	std::vector<struct pollfd> pollfds;
> +	pollfds.reserve(notifiers_.size());
> +
> +	for (auto notifier : notifiers_)
> +		pollfds.push_back({ notifier.first, notifier.second.events(), 0 });
> +
> +	/* Compute the timeout. */
> +	Timer *nextTimer = !timers_.empty() ? timers_.front() : nullptr;
> +	struct timespec timeout;
> +
> +	if (nextTimer) {
> +		clock_gettime(CLOCK_MONOTONIC, &timeout);
> +		uint64_t now = timeout.tv_sec * 1000000000ULL + timeout.tv_nsec;
> +
> +		if (nextTimer->deadline() > now) {
> +			uint64_t delta = nextTimer->deadline() - now;
> +			timeout.tv_sec = delta / 1000000000ULL;
> +			timeout.tv_nsec = delta % 1000000000ULL;
> +		} else {
> +			timeout.tv_sec = 0;
> +			timeout.tv_nsec = 0;
> +		}
> +
> +		LOG(Debug) << "timeout " << timeout.tv_sec << "."
> +			   << std::setfill('0') << std::setw(9)
> +			   << timeout.tv_nsec;
> +	}
> +
> +	/* Wait for events and process notifers and timers. */
> +	ret = ppoll(pollfds.data(), pollfds.size(),
> +		    nextTimer ? &timeout : nullptr, nullptr);
> +	if (ret < 0) {
> +		ret = -errno;
> +		LOG(Warning) << "poll() failed with " << strerror(-ret);

When poll fails, are we ok processing timers?

> +	} else if (ret > 0) {
> +		processNotifiers(pollfds);
> +	}
> +

shouldn't we process timers when poll returns 0 (timeout) ? or we go
there to check just in case we missed a deadline?

> +	processTimers();
> +}
> +
> +short EventDispatcherPoll::EventNotifierSetPoll::events() const
> +{
> +	short events = 0;
> +
> +	if (notifiers[EventNotifier::Read])
> +		events |= POLLIN;
> +	if (notifiers[EventNotifier::Write])
> +		events |= POLLOUT;
> +	if (notifiers[EventNotifier::Exception])
> +		events |= POLLPRI;
> +
> +	return events;
> +}
> +
> +void EventDispatcherPoll::processNotifiers(std::vector<struct pollfd> &pollfds)
> +{
> +	static const struct {
> +		EventNotifier::Type type;
> +		short events;
> +	} events[] = {
> +		{ EventNotifier::Read, POLLIN },
> +		{ EventNotifier::Write, POLLOUT },
> +		{ EventNotifier::Exception, POLLPRI },
> +	};
> +
> +	for (const struct pollfd &pfd : pollfds) {
> +		auto iter = notifiers_.find(pfd.fd);
> +		ASSERT(iter != notifiers_.end());

Is this fatal? I mean, this should not happen, I agree, but do we want
to abort?

> +
> +		EventNotifierSetPoll &set = iter->second;
> +
> +		for (const auto &event : events) {
> +			EventNotifier *notifier = set.notifiers[event.type];
> +
> +			if (!notifier)
> +				continue;
> +
> +			/*
> +			 * If the file descriptor is invalid, disable the
> +			 * notifier immediately.
> +			 */
> +			if (pfd.revents & POLLNVAL) {
> +				LOG(Warning) << "Disabling " << notifierType(event.type)
> +					     << " due to invalid file descriptor "
> +					     << pfd.fd;
> +				unregisterEventNotifier(notifier);

I'm not sure if POLLNVAL is exclusive (when it's set, no other bit
field is set in pfd.revents), but if it is not, and some other event
source flag is set, after unregistering, shouldn't you continue?

> +			}
> +
> +			if (pfd.revents & event.events)
> +				notifier->activated.emit(notifier);
> +		}
> +	}
> +}
> +
> +void EventDispatcherPoll::processTimers()
> +{
> +	struct timespec ts;
> +	uint64_t now;
> +	clock_gettime(CLOCK_MONOTONIC, &ts);
> +	now = ts.tv_sec * 1000000000ULL + ts.tv_nsec;
> +
> +	while (!timers_.empty()) {
> +		Timer *timer = timers_.front();
> +		if (timer->deadline() > now)
> +			break;
> +
> +		timers_.pop_front();
> +		timer->stop();
> +		timer->timeout.emit(timer);
> +	}
> +}
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/include/event_dispatcher_poll.h b/src/libcamera/include/event_dispatcher_poll.h
> new file mode 100644
> index 000000000000..600289e455e2
> --- /dev/null
> +++ b/src/libcamera/include/event_dispatcher_poll.h
> @@ -0,0 +1,50 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * event_loop.h - Camera object interface

Camera object?

> + */
> +#ifndef __LIBCAMERA_EVENT_DISPATCHER_POLL_H__
> +#define __LIBCAMERA_EVENT_DISPATCHER_POLL_H__
> +
> +#include <libcamera/event_dispatcher.h>
> +
> +#include <list>
> +#include <map>
> +#include <vector>
> +
> +namespace libcamera {
> +
> +class EventNotifier;
> +class Timer;

Still a bit skeptical about forward declarations when an include would
suffice, but that's up to you actually.

> +
> +class EventDispatcherPoll final : public EventDispatcher
> +{
> +public:
> +	EventDispatcherPoll();
> +	~EventDispatcherPoll();
> +
> +	void registerEventNotifier(EventNotifier *notifier);
> +	void unregisterEventNotifier(EventNotifier *notifier);
> +
> +	void registerTimer(Timer *timer);
> +	void unregisterTimer(Timer *timer);
> +
> +	void processEvents();
> +
> +private:
> +	struct EventNotifierSetPoll {
> +		short events() const;
> +		EventNotifier *notifiers[3];

Shouldn't a value from the EventNotifier::Type be used? Like an
additional MaxType ? I agree this is mostly fixed though, so feel free
to ignore the comment.

> +	};
> +
> +	std::map<int, EventNotifierSetPoll> notifiers_;
> +	std::list<Timer *> timers_;
> +
> +	void processNotifiers(std::vector<struct pollfd> &pollfds);
> +	void processTimers();
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_EVENT_DISPATCHER_POLL_H__ */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 61fb93205b34..abf9a71d4172 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -3,6 +3,7 @@ libcamera_sources = files([
>      'camera_manager.cpp',
>      'device_enumerator.cpp',
>      'event_dispatcher.cpp',
> +    'event_dispatcher_poll.cpp',
>      'event_notifier.cpp',
>      'log.cpp',
>      'media_device.cpp',
> @@ -14,6 +15,7 @@ libcamera_sources = files([
>
>  libcamera_headers = files([
>      'include/device_enumerator.h',
> +    'include/event_dispatcher_poll.h',
>      'include/log.h',
>      'include/media_device.h',
>      'include/media_object.h',
> --
> Regards,
>
> Laurent Pinchart

Thanks
  j

>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Jacopo,

On Tuesday, 8 January 2019 12:28:01 EET Jacopo Mondi wrote:
> Hi Laurent,
>    mostly minor stuff here as well
> 
> On Tue, Jan 08, 2019 at 01:11:47AM +0200, Laurent Pinchart wrote:
> > Provide a poll-based event dispatcher implementation as convenience for
> > applications that don't need a custom event loop. The poll-based
> > dispatcher is automatically instantiated if the application doesn't
> > provide its own dispatcher.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > Changes since v1:
> > 
> > - Fix ASSERT() condition check
> > - Ignore duplicate notifiers
> > - Clarify warning messages for event notifier (un)registration errors
> > ---
> > 
> >  include/libcamera/libcamera.h                 |   1 +
> >  src/libcamera/camera_manager.cpp              |  19 +-
> >  src/libcamera/event_dispatcher_poll.cpp       | 234 ++++++++++++++++++
> >  src/libcamera/include/event_dispatcher_poll.h |  50 ++++
> >  src/libcamera/meson.build                     |   2 +
> >  5 files changed, 301 insertions(+), 5 deletions(-)
> >  create mode 100644 src/libcamera/event_dispatcher_poll.cpp
> >  create mode 100644 src/libcamera/include/event_dispatcher_poll.h

[snip]

> > diff --git a/src/libcamera/camera_manager.cpp
> > b/src/libcamera/camera_manager.cpp index cbfd977f1e3c..348ea2fedf64
> > 100644
> > --- a/src/libcamera/camera_manager.cpp
> > +++ b/src/libcamera/camera_manager.cpp

[snip]

> > @@ -208,11 +210,18 @@ void
> > CameraManager::setEventDispatcher(EventDispatcher *dispatcher)> 
> >  /**
> >   * \fn CameraManager::eventDispatcher()
> >   * \brief Retrieve the event dispatcher
> > - * \return Pointer to the event dispatcher, or nullptr if no event
> > dispatcher - * has been set
> > + *
> > + * This function retrieves the event dispatcher set with
> > setEventDispatcher(). + * If no dispatcher has been set, a default
> > poll-based implementation is created + * and returned.
> 
> Is it worth pointing out that after this function has been called, it
> is not possible to set a custom eventDispatcher anymore?

Good point. I'll add ", and no custom event dispatcher may be installed 
anymore."

> > + *
> > + * \return Pointer to the event dispatcher
> >   */

[snip]

> > diff --git a/src/libcamera/event_dispatcher_poll.cpp
> > b/src/libcamera/event_dispatcher_poll.cpp new file mode 100644
> > index 000000000000..0cd08b2b4cc1
> > --- /dev/null
> > +++ b/src/libcamera/event_dispatcher_poll.cpp

[snip]

> > +/**
> > + * \class EventDispatcherPoll
> > + * \brief A poll-based event dispatcher
> > + */
> > +
> 
> None of the functions here implemented is commented. Is this
> intentional? Doxygen uses comments applied on the base class so it
> does not actually complains...

It's intentional, as EventDispatcherPoll is an internal implementation that 
doesn't create any API, neither externally not internally. There is thus not 
much to document from an API point of view.

> > +EventDispatcherPoll::EventDispatcherPoll()
> > +{
> > +}
> > +
> > +EventDispatcherPoll::~EventDispatcherPoll()
> > +{
> > +}
> > +
> > +void EventDispatcherPoll::registerEventNotifier(EventNotifier *notifier)
> > +{
> > +	EventNotifierSetPoll &set = notifiers_[notifier->fd()];
> > +	EventNotifier::Type type = notifier->type();
> > +
> > +	if (set.notifiers[type] && set.notifiers[type] != notifier) {
> > +		LOG(Warning) << "Ignoring duplicate " << notifierType(type)
> > +			     << " notifier for fd " << notifier->fd();
> > +		return;
> > +	}
> > +
> > +	set.notifiers[type] = notifier;
> > +}
> > +
> > +void EventDispatcherPoll::unregisterEventNotifier(EventNotifier
> > *notifier)
> > +{
> > +	auto iter = notifiers_.find(notifier->fd());
> > +	if (iter == notifiers_.end())
> > +		return;
> > +
> > +	EventNotifierSetPoll &set = iter->second;
> > +	EventNotifier::Type type = notifier->type();
> > +
> > +	if (!set.notifiers[type])
> > +		return;
> > +
> > +	if (set.notifiers[type] != notifier) {
> > +		LOG(Warning) << notifierType(type) << " notifier for fd "
> > +			     << notifier->fd() << " is not registered";
> > +		return;
> > +	}
> > +
> > +	set.notifiers[type] = nullptr;
> > +	if (!set.notifiers[0] && !set.notifiers[1] && !set.notifiers[2])
> > +		notifiers_.erase(iter);
> > +}
> > +
> > +void EventDispatcherPoll::registerTimer(Timer *timer)
> > +{
> > +	for (auto iter = timers_.begin(); iter != timers_.end(); ++iter) {
> > +		if ((*iter)->deadline() > timer->deadline()) {
> > +			timers_.insert(iter, timer);
> > +			return;
> > +		}
> > +	}
> > +
> > +	timers_.push_back(timer);
> > +}
> > +
> > +void EventDispatcherPoll::unregisterTimer(Timer *timer)
> > +{
> > +	for (auto iter = timers_.begin(); iter != timers_.end(); ++iter) {
> > +		if (*iter == timer) {
> > +			timers_.erase(iter);
> > +			return;
> > +		}
> > +
> > +		/*
> > +		 * As the timers list is ordered, we can stop as soon as we go
> > +		 * past the deadline.
> > +		 */
> > +		if ((*iter)->deadline() > timer->deadline())
> > +			break;
> > +	}
> > +}
> > +
> 
> This function is probably worth being documented a bit. Just to point
> out it is blocking maybe.

It's not supposed to be blocking :-)

> > +void EventDispatcherPoll::processEvents()
> > +{
> > +	int ret;
> > +
> > +	/* Create the pollfd array. */
> > +	std::vector<struct pollfd> pollfds;
> > +	pollfds.reserve(notifiers_.size());
> > +
> > +	for (auto notifier : notifiers_)
> > +		pollfds.push_back({ notifier.first, notifier.second.events(), 0 });
> > +
> > +	/* Compute the timeout. */
> > +	Timer *nextTimer = !timers_.empty() ? timers_.front() : nullptr;
> > +	struct timespec timeout;
> > +
> > +	if (nextTimer) {
> > +		clock_gettime(CLOCK_MONOTONIC, &timeout);
> > +		uint64_t now = timeout.tv_sec * 1000000000ULL + timeout.tv_nsec;
> > +
> > +		if (nextTimer->deadline() > now) {
> > +			uint64_t delta = nextTimer->deadline() - now;
> > +			timeout.tv_sec = delta / 1000000000ULL;
> > +			timeout.tv_nsec = delta % 1000000000ULL;
> > +		} else {
> > +			timeout.tv_sec = 0;
> > +			timeout.tv_nsec = 0;
> > +		}
> > +
> > +		LOG(Debug) << "timeout " << timeout.tv_sec << "."
> > +			   << std::setfill('0') << std::setw(9)
> > +			   << timeout.tv_nsec;
> > +	}
> > +
> > +	/* Wait for events and process notifers and timers. */
> > +	ret = ppoll(pollfds.data(), pollfds.size(),
> > +		    nextTimer ? &timeout : nullptr, nullptr);
> > +	if (ret < 0) {
> > +		ret = -errno;
> > +		LOG(Warning) << "poll() failed with " << strerror(-ret);
> 
> When poll fails, are we ok processing timers?
> 
> > +	} else if (ret > 0) {
> > +		processNotifiers(pollfds);
> > +	}
> > +
> 
> shouldn't we process timers when poll returns 0 (timeout) ? or we go
> there to check just in case we missed a deadline?

I think processing timers is fine in that case, just in case. A poll() failure 
will likely be fatal anyway, so it won't matter much.

> > +	processTimers();
> > +}
> > +
> > +short EventDispatcherPoll::EventNotifierSetPoll::events() const
> > +{
> > +	short events = 0;
> > +
> > +	if (notifiers[EventNotifier::Read])
> > +		events |= POLLIN;
> > +	if (notifiers[EventNotifier::Write])
> > +		events |= POLLOUT;
> > +	if (notifiers[EventNotifier::Exception])
> > +		events |= POLLPRI;
> > +
> > +	return events;
> > +}
> > +
> > +void EventDispatcherPoll::processNotifiers(std::vector<struct pollfd>
> > &pollfds)
> > +{
> > +	static const struct {
> > +		EventNotifier::Type type;
> > +		short events;
> > +	} events[] = {
> > +		{ EventNotifier::Read, POLLIN },
> > +		{ EventNotifier::Write, POLLOUT },
> > +		{ EventNotifier::Exception, POLLPRI },
> > +	};
> > +
> > +	for (const struct pollfd &pfd : pollfds) {
> > +		auto iter = notifiers_.find(pfd.fd);
> > +		ASSERT(iter != notifiers_.end());
> 
> Is this fatal? I mean, this should not happen, I agree, but do we want
> to abort?

It's a serious bug in the library that would likely indicate a memory 
corruption of some sort, or really incorrect library code. I expect the 
program to crash later in that case, aborting early will make it easier to 
debug the problem.

> > +
> > +		EventNotifierSetPoll &set = iter->second;
> > +
> > +		for (const auto &event : events) {
> > +			EventNotifier *notifier = set.notifiers[event.type];
> > +
> > +			if (!notifier)
> > +				continue;
> > +
> > +			/*
> > +			 * If the file descriptor is invalid, disable the
> > +			 * notifier immediately.
> > +			 */
> > +			if (pfd.revents & POLLNVAL) {
> > +				LOG(Warning) << "Disabling " << notifierType(event.type)
> > +					     << " due to invalid file descriptor "
> > +					     << pfd.fd;
> > +				unregisterEventNotifier(notifier);
> 
> I'm not sure if POLLNVAL is exclusive (when it's set, no other bit
> field is set in pfd.revents), but if it is not, and some other event
> source flag is set, after unregistering, shouldn't you continue?

I would assume it's exclusive indeed, a continue shouldn't hurt here, I'll add 
one (but it shouldn't make a big difference either, given that POLLNVAL 
shouldn't occur at all in the normal case).

> > +			}
> > +
> > +			if (pfd.revents & event.events)
> > +				notifier->activated.emit(notifier);
> > +		}
> > +	}
> > +}
> > +
> > +void EventDispatcherPoll::processTimers()
> > +{
> > +	struct timespec ts;
> > +	uint64_t now;
> > +	clock_gettime(CLOCK_MONOTONIC, &ts);
> > +	now = ts.tv_sec * 1000000000ULL + ts.tv_nsec;
> > +
> > +	while (!timers_.empty()) {
> > +		Timer *timer = timers_.front();
> > +		if (timer->deadline() > now)
> > +			break;
> > +
> > +		timers_.pop_front();
> > +		timer->stop();
> > +		timer->timeout.emit(timer);
> > +	}
> > +}
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/libcamera/include/event_dispatcher_poll.h
> > b/src/libcamera/include/event_dispatcher_poll.h new file mode 100644
> > index 000000000000..600289e455e2
> > --- /dev/null
> > +++ b/src/libcamera/include/event_dispatcher_poll.h
> > @@ -0,0 +1,50 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * event_loop.h - Camera object interface
> 
> Camera object?

Can you guess where this was copied from ? :-)

I've now fixed it (as well as the file name).

> > + */
> > +#ifndef __LIBCAMERA_EVENT_DISPATCHER_POLL_H__
> > +#define __LIBCAMERA_EVENT_DISPATCHER_POLL_H__
> > +
> > +#include <libcamera/event_dispatcher.h>
> > +
> > +#include <list>
> > +#include <map>
> > +#include <vector>
> > +
> > +namespace libcamera {
> > +
> > +class EventNotifier;
> > +class Timer;
> 
> Still a bit skeptical about forward declarations when an include would
> suffice, but that's up to you actually.

On the plus side, they speed up compilation and avoid potential future 
circular inclusions. Do they cause issues ?

> > +
> > +class EventDispatcherPoll final : public EventDispatcher
> > +{
> > +public:
> > +	EventDispatcherPoll();
> > +	~EventDispatcherPoll();
> > +
> > +	void registerEventNotifier(EventNotifier *notifier);
> > +	void unregisterEventNotifier(EventNotifier *notifier);
> > +
> > +	void registerTimer(Timer *timer);
> > +	void unregisterTimer(Timer *timer);
> > +
> > +	void processEvents();
> > +
> > +private:
> > +	struct EventNotifierSetPoll {
> > +		short events() const;
> > +		EventNotifier *notifiers[3];
> 
> Shouldn't a value from the EventNotifier::Type be used? Like an
> additional MaxType ? I agree this is mostly fixed though, so feel free
> to ignore the comment.

As it is indeed fixed by the poll() API, which I don't expect to change any 
time soon, I think a fixed-size array is fine.

> > +	};
> > +
> > +	std::map<int, EventNotifierSetPoll> notifiers_;
> > +	std::list<Timer *> timers_;
> > +
> > +	void processNotifiers(std::vector<struct pollfd> &pollfds);
> > +	void processTimers();
> > +};
> > +
> > +} /* namespace libcamera */
> > +
> > +#endif /* __LIBCAMERA_EVENT_DISPATCHER_POLL_H__ */

[snip]

HI Laurent,

On Tue, Jan 08, 2019 at 12:54:57PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Tuesday, 8 January 2019 12:28:01 EET Jacopo Mondi wrote:
> > Hi Laurent,
> >    mostly minor stuff here as well
> >
> > On Tue, Jan 08, 2019 at 01:11:47AM +0200, Laurent Pinchart wrote:
> > > Provide a poll-based event dispatcher implementation as convenience for
> > > applications that don't need a custom event loop. The poll-based
> > > dispatcher is automatically instantiated if the application doesn't
> > > provide its own dispatcher.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > > Changes since v1:
> > >
> > > - Fix ASSERT() condition check
> > > - Ignore duplicate notifiers
> > > - Clarify warning messages for event notifier (un)registration errors
> > > ---
> > >
> > >  include/libcamera/libcamera.h                 |   1 +
> > >  src/libcamera/camera_manager.cpp              |  19 +-
> > >  src/libcamera/event_dispatcher_poll.cpp       | 234 ++++++++++++++++++
> > >  src/libcamera/include/event_dispatcher_poll.h |  50 ++++
> > >  src/libcamera/meson.build                     |   2 +
> > >  5 files changed, 301 insertions(+), 5 deletions(-)
> > >  create mode 100644 src/libcamera/event_dispatcher_poll.cpp
> > >  create mode 100644 src/libcamera/include/event_dispatcher_poll.h
>
> [snip]
>
> > > diff --git a/src/libcamera/camera_manager.cpp
> > > b/src/libcamera/camera_manager.cpp index cbfd977f1e3c..348ea2fedf64
> > > 100644
> > > --- a/src/libcamera/camera_manager.cpp
> > > +++ b/src/libcamera/camera_manager.cpp
>
> [snip]
>
> > > @@ -208,11 +210,18 @@ void
> > > CameraManager::setEventDispatcher(EventDispatcher *dispatcher)>
> > >  /**
> > >   * \fn CameraManager::eventDispatcher()
> > >   * \brief Retrieve the event dispatcher
> > > - * \return Pointer to the event dispatcher, or nullptr if no event
> > > dispatcher - * has been set
> > > + *
> > > + * This function retrieves the event dispatcher set with
> > > setEventDispatcher(). + * If no dispatcher has been set, a default
> > > poll-based implementation is created + * and returned.
> >
> > Is it worth pointing out that after this function has been called, it
> > is not possible to set a custom eventDispatcher anymore?
>
> Good point. I'll add ", and no custom event dispatcher may be installed
> anymore."
>
> > > + *
> > > + * \return Pointer to the event dispatcher
> > >   */
>
> [snip]
>
> > > diff --git a/src/libcamera/event_dispatcher_poll.cpp
> > > b/src/libcamera/event_dispatcher_poll.cpp new file mode 100644
> > > index 000000000000..0cd08b2b4cc1
> > > --- /dev/null
> > > +++ b/src/libcamera/event_dispatcher_poll.cpp
>
> [snip]
>
> > > +/**
> > > + * \class EventDispatcherPoll
> > > + * \brief A poll-based event dispatcher
> > > + */
> > > +
> >
> > None of the functions here implemented is commented. Is this
> > intentional? Doxygen uses comments applied on the base class so it
> > does not actually complains...
>
> It's intentional, as EventDispatcherPoll is an internal implementation that
> doesn't create any API, neither externally not internally. There is thus not
> much to document from an API point of view.
>

I see, fine with me

> > > +EventDispatcherPoll::EventDispatcherPoll()
> > > +{
> > > +}
> > > +
> > > +EventDispatcherPoll::~EventDispatcherPoll()
> > > +{
> > > +}
> > > +
> > > +void EventDispatcherPoll::registerEventNotifier(EventNotifier *notifier)
> > > +{
> > > +	EventNotifierSetPoll &set = notifiers_[notifier->fd()];
> > > +	EventNotifier::Type type = notifier->type();
> > > +
> > > +	if (set.notifiers[type] && set.notifiers[type] != notifier) {
> > > +		LOG(Warning) << "Ignoring duplicate " << notifierType(type)
> > > +			     << " notifier for fd " << notifier->fd();
> > > +		return;
> > > +	}
> > > +
> > > +	set.notifiers[type] = notifier;
> > > +}
> > > +
> > > +void EventDispatcherPoll::unregisterEventNotifier(EventNotifier
> > > *notifier)
> > > +{
> > > +	auto iter = notifiers_.find(notifier->fd());
> > > +	if (iter == notifiers_.end())
> > > +		return;
> > > +
> > > +	EventNotifierSetPoll &set = iter->second;
> > > +	EventNotifier::Type type = notifier->type();
> > > +
> > > +	if (!set.notifiers[type])
> > > +		return;
> > > +
> > > +	if (set.notifiers[type] != notifier) {
> > > +		LOG(Warning) << notifierType(type) << " notifier for fd "
> > > +			     << notifier->fd() << " is not registered";
> > > +		return;
> > > +	}
> > > +
> > > +	set.notifiers[type] = nullptr;
> > > +	if (!set.notifiers[0] && !set.notifiers[1] && !set.notifiers[2])
> > > +		notifiers_.erase(iter);
> > > +}
> > > +
> > > +void EventDispatcherPoll::registerTimer(Timer *timer)
> > > +{
> > > +	for (auto iter = timers_.begin(); iter != timers_.end(); ++iter) {
> > > +		if ((*iter)->deadline() > timer->deadline()) {
> > > +			timers_.insert(iter, timer);
> > > +			return;
> > > +		}
> > > +	}
> > > +
> > > +	timers_.push_back(timer);
> > > +}
> > > +
> > > +void EventDispatcherPoll::unregisterTimer(Timer *timer)
> > > +{
> > > +	for (auto iter = timers_.begin(); iter != timers_.end(); ++iter) {
> > > +		if (*iter == timer) {
> > > +			timers_.erase(iter);
> > > +			return;
> > > +		}
> > > +
> > > +		/*
> > > +		 * As the timers list is ordered, we can stop as soon as we go
> > > +		 * past the deadline.
> > > +		 */
> > > +		if ((*iter)->deadline() > timer->deadline())
> > > +			break;
> > > +	}
> > > +}
> > > +
> >
> > This function is probably worth being documented a bit. Just to point
> > out it is blocking maybe.
>
> It's not supposed to be blocking :-)
>

I mean processEvents() not unregisterTimer() :)

> > > +void EventDispatcherPoll::processEvents()
> > > +{
> > > +	int ret;
> > > +
> > > +	/* Create the pollfd array. */
> > > +	std::vector<struct pollfd> pollfds;
> > > +	pollfds.reserve(notifiers_.size());
> > > +
> > > +	for (auto notifier : notifiers_)
> > > +		pollfds.push_back({ notifier.first, notifier.second.events(), 0 });
> > > +
> > > +	/* Compute the timeout. */
> > > +	Timer *nextTimer = !timers_.empty() ? timers_.front() : nullptr;
> > > +	struct timespec timeout;
> > > +
> > > +	if (nextTimer) {
> > > +		clock_gettime(CLOCK_MONOTONIC, &timeout);
> > > +		uint64_t now = timeout.tv_sec * 1000000000ULL + timeout.tv_nsec;
> > > +
> > > +		if (nextTimer->deadline() > now) {
> > > +			uint64_t delta = nextTimer->deadline() - now;
> > > +			timeout.tv_sec = delta / 1000000000ULL;
> > > +			timeout.tv_nsec = delta % 1000000000ULL;
> > > +		} else {
> > > +			timeout.tv_sec = 0;
> > > +			timeout.tv_nsec = 0;
> > > +		}
> > > +
> > > +		LOG(Debug) << "timeout " << timeout.tv_sec << "."
> > > +			   << std::setfill('0') << std::setw(9)
> > > +			   << timeout.tv_nsec;
> > > +	}
> > > +
> > > +	/* Wait for events and process notifers and timers. */
> > > +	ret = ppoll(pollfds.data(), pollfds.size(),
> > > +		    nextTimer ? &timeout : nullptr, nullptr);
> > > +	if (ret < 0) {
> > > +		ret = -errno;
> > > +		LOG(Warning) << "poll() failed with " << strerror(-ret);
> >
> > When poll fails, are we ok processing timers?
> >
> > > +	} else if (ret > 0) {
> > > +		processNotifiers(pollfds);
> > > +	}
> > > +
> >
> > shouldn't we process timers when poll returns 0 (timeout) ? or we go
> > there to check just in case we missed a deadline?
>
> I think processing timers is fine in that case, just in case. A poll() failure
> will likely be fatal anyway, so it won't matter much.

So it is not worth aborting/returning the poll errno?

>
> > > +	processTimers();
> > > +}
> > > +
> > > +short EventDispatcherPoll::EventNotifierSetPoll::events() const
> > > +{
> > > +	short events = 0;
> > > +
> > > +	if (notifiers[EventNotifier::Read])
> > > +		events |= POLLIN;
> > > +	if (notifiers[EventNotifier::Write])
> > > +		events |= POLLOUT;
> > > +	if (notifiers[EventNotifier::Exception])
> > > +		events |= POLLPRI;
> > > +
> > > +	return events;
> > > +}
> > > +
> > > +void EventDispatcherPoll::processNotifiers(std::vector<struct pollfd>
> > > &pollfds)
> > > +{
> > > +	static const struct {
> > > +		EventNotifier::Type type;
> > > +		short events;
> > > +	} events[] = {
> > > +		{ EventNotifier::Read, POLLIN },
> > > +		{ EventNotifier::Write, POLLOUT },
> > > +		{ EventNotifier::Exception, POLLPRI },
> > > +	};
> > > +
> > > +	for (const struct pollfd &pfd : pollfds) {
> > > +		auto iter = notifiers_.find(pfd.fd);
> > > +		ASSERT(iter != notifiers_.end());
> >
> > Is this fatal? I mean, this should not happen, I agree, but do we want
> > to abort?
>
> It's a serious bug in the library that would likely indicate a memory
> corruption of some sort, or really incorrect library code. I expect the
> program to crash later in that case, aborting early will make it easier to
> debug the problem.

A notifier can be removed between the time poll is called and poll is
returned. I agree it's a corner case though.
>
> > > +
> > > +		EventNotifierSetPoll &set = iter->second;
> > > +
> > > +		for (const auto &event : events) {
> > > +			EventNotifier *notifier = set.notifiers[event.type];
> > > +
> > > +			if (!notifier)
> > > +				continue;
> > > +
> > > +			/*
> > > +			 * If the file descriptor is invalid, disable the
> > > +			 * notifier immediately.
> > > +			 */
> > > +			if (pfd.revents & POLLNVAL) {
> > > +				LOG(Warning) << "Disabling " << notifierType(event.type)
> > > +					     << " due to invalid file descriptor "
> > > +					     << pfd.fd;
> > > +				unregisterEventNotifier(notifier);
> >
> > I'm not sure if POLLNVAL is exclusive (when it's set, no other bit
> > field is set in pfd.revents), but if it is not, and some other event
> > source flag is set, after unregistering, shouldn't you continue?
>
> I would assume it's exclusive indeed, a continue shouldn't hurt here, I'll add
> one (but it shouldn't make a big difference either, given that POLLNVAL
> shouldn't occur at all in the normal case).
>
> > > +			}
> > > +
> > > +			if (pfd.revents & event.events)
> > > +				notifier->activated.emit(notifier);
> > > +		}
> > > +	}
> > > +}
> > > +
> > > +void EventDispatcherPoll::processTimers()
> > > +{
> > > +	struct timespec ts;
> > > +	uint64_t now;
> > > +	clock_gettime(CLOCK_MONOTONIC, &ts);
> > > +	now = ts.tv_sec * 1000000000ULL + ts.tv_nsec;
> > > +
> > > +	while (!timers_.empty()) {
> > > +		Timer *timer = timers_.front();
> > > +		if (timer->deadline() > now)
> > > +			break;
> > > +
> > > +		timers_.pop_front();
> > > +		timer->stop();
> > > +		timer->timeout.emit(timer);
> > > +	}
> > > +}
> > > +
> > > +} /* namespace libcamera */
> > > diff --git a/src/libcamera/include/event_dispatcher_poll.h
> > > b/src/libcamera/include/event_dispatcher_poll.h new file mode 100644
> > > index 000000000000..600289e455e2
> > > --- /dev/null
> > > +++ b/src/libcamera/include/event_dispatcher_poll.h
> > > @@ -0,0 +1,50 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2019, Google Inc.
> > > + *
> > > + * event_loop.h - Camera object interface
> >
> > Camera object?
>
> Can you guess where this was copied from ? :-)
>
> I've now fixed it (as well as the file name).
>
> > > + */
> > > +#ifndef __LIBCAMERA_EVENT_DISPATCHER_POLL_H__
> > > +#define __LIBCAMERA_EVENT_DISPATCHER_POLL_H__
> > > +
> > > +#include <libcamera/event_dispatcher.h>
> > > +
> > > +#include <list>
> > > +#include <map>
> > > +#include <vector>
> > > +
> > > +namespace libcamera {
> > > +
> > > +class EventNotifier;
> > > +class Timer;
> >
> > Still a bit skeptical about forward declarations when an include would
> > suffice, but that's up to you actually.
>
> On the plus side, they speed up compilation and avoid potential future
> circular inclusions. Do they cause issues ?
>

Not particularly, as it is unlikely we miss recompiling this file if
the header change as the library is built together.

Thanks for the multiple clarifications.
It is my opinion you should push the next iteration of the series.

Thanks
  j


> > > +
> > > +class EventDispatcherPoll final : public EventDispatcher
> > > +{
> > > +public:
> > > +	EventDispatcherPoll();
> > > +	~EventDispatcherPoll();
> > > +
> > > +	void registerEventNotifier(EventNotifier *notifier);
> > > +	void unregisterEventNotifier(EventNotifier *notifier);
> > > +
> > > +	void registerTimer(Timer *timer);
> > > +	void unregisterTimer(Timer *timer);
> > > +
> > > +	void processEvents();
> > > +
> > > +private:
> > > +	struct EventNotifierSetPoll {
> > > +		short events() const;
> > > +		EventNotifier *notifiers[3];
> >
> > Shouldn't a value from the EventNotifier::Type be used? Like an
> > additional MaxType ? I agree this is mostly fixed though, so feel free
> > to ignore the comment.
>
> As it is indeed fixed by the poll() API, which I don't expect to change any
> time soon, I think a fixed-size array is fine.
>
> > > +	};
> > > +
> > > +	std::map<int, EventNotifierSetPoll> notifiers_;
> > > +	std::list<Timer *> timers_;
> > > +
> > > +	void processNotifiers(std::vector<struct pollfd> &pollfds);
> > > +	void processTimers();
> > > +};
> > > +
> > > +} /* namespace libcamera */
> > > +
> > > +#endif /* __LIBCAMERA_EVENT_DISPATCHER_POLL_H__ */
>
> [snip]
>
> --
> Regards,
>
> Laurent Pinchart
>
>
>

Hi Jacopo,

On Tuesday, 8 January 2019 14:32:41 EET Jacopo Mondi wrote:
> On Tue, Jan 08, 2019 at 12:54:57PM +0200, Laurent Pinchart wrote:
> > On Tuesday, 8 January 2019 12:28:01 EET Jacopo Mondi wrote:
> >> On Tue, Jan 08, 2019 at 01:11:47AM +0200, Laurent Pinchart wrote:
> >>> Provide a poll-based event dispatcher implementation as convenience
> >>> for applications that don't need a custom event loop. The poll-based
> >>> dispatcher is automatically instantiated if the application doesn't
> >>> provide its own dispatcher.
> >>> 
> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>> ---
> >>> Changes since v1:
> >>> 
> >>> - Fix ASSERT() condition check
> >>> - Ignore duplicate notifiers
> >>> - Clarify warning messages for event notifier (un)registration errors
> >>> ---
> >>> 
> >>>  include/libcamera/libcamera.h                 |   1 +
> >>>  src/libcamera/camera_manager.cpp              |  19 +-
> >>>  src/libcamera/event_dispatcher_poll.cpp       | 234 ++++++++++++++++++
> >>>  src/libcamera/include/event_dispatcher_poll.h |  50 ++++
> >>>  src/libcamera/meson.build                     |   2 +
> >>>  5 files changed, 301 insertions(+), 5 deletions(-)
> >>>  create mode 100644 src/libcamera/event_dispatcher_poll.cpp
> >>>  create mode 100644 src/libcamera/include/event_dispatcher_poll.h

[snip]

> >>> diff --git a/src/libcamera/event_dispatcher_poll.cpp
> >>> b/src/libcamera/event_dispatcher_poll.cpp new file mode 100644
> >>> index 000000000000..0cd08b2b4cc1
> >>> --- /dev/null
> >>> +++ b/src/libcamera/event_dispatcher_poll.cpp

[snip]

> >>> +void EventDispatcherPoll::unregisterTimer(Timer *timer)
> >>> +{
> >>> +	for (auto iter = timers_.begin(); iter != timers_.end(); ++iter) {
> >>> +		if (*iter == timer) {
> >>> +			timers_.erase(iter);
> >>> +			return;
> >>> +		}
> >>> +
> >>> +		/*
> >>> +		 * As the timers list is ordered, we can stop as soon as we go
> >>> +		 * past the deadline.
> >>> +		 */
> >>> +		if ((*iter)->deadline() > timer->deadline())
> >>> +			break;
> >>> +	}
> >>> +}
> >>> +
> >> 
> >> This function is probably worth being documented a bit. Just to point
> >> out it is blocking maybe.
> > 
> > It's not supposed to be blocking :-)
> 
> I mean processEvents() not unregisterTimer() :)

That makes more sense indeed :-)

The EventDispatcher::processEvents() function is already documented as 
blocking, so I don't think we need to repeat this here.

> >>> +void EventDispatcherPoll::processEvents()
> >>> +{
> >>> +	int ret;
> >>> +
> >>> +	/* Create the pollfd array. */
> >>> +	std::vector<struct pollfd> pollfds;
> >>> +	pollfds.reserve(notifiers_.size());
> >>> +
> >>> +	for (auto notifier : notifiers_)
> >>> +		pollfds.push_back({ notifier.first, notifier.second.events(), 0 
});
> >>> +
> >>> +	/* Compute the timeout. */
> >>> +	Timer *nextTimer = !timers_.empty() ? timers_.front() : nullptr;
> >>> +	struct timespec timeout;
> >>> +
> >>> +	if (nextTimer) {
> >>> +		clock_gettime(CLOCK_MONOTONIC, &timeout);
> >>> +		uint64_t now = timeout.tv_sec * 1000000000ULL + timeout.tv_nsec;
> >>> +
> >>> +		if (nextTimer->deadline() > now) {
> >>> +			uint64_t delta = nextTimer->deadline() - now;
> >>> +			timeout.tv_sec = delta / 1000000000ULL;
> >>> +			timeout.tv_nsec = delta % 1000000000ULL;
> >>> +		} else {
> >>> +			timeout.tv_sec = 0;
> >>> +			timeout.tv_nsec = 0;
> >>> +		}
> >>> +
> >>> +		LOG(Debug) << "timeout " << timeout.tv_sec << "."
> >>> +			   << std::setfill('0') << std::setw(9)
> >>> +			   << timeout.tv_nsec;
> >>> +	}
> >>> +
> >>> +	/* Wait for events and process notifers and timers. */
> >>> +	ret = ppoll(pollfds.data(), pollfds.size(),
> >>> +		    nextTimer ? &timeout : nullptr, nullptr);
> >>> +	if (ret < 0) {
> >>> +		ret = -errno;
> >>> +		LOG(Warning) << "poll() failed with " << strerror(-ret);
> >> 
> >> When poll fails, are we ok processing timers?
> >> 
> >>> +	} else if (ret > 0) {
> >>> +		processNotifiers(pollfds);
> >>> +	}
> >>> +
> >> 
> >> shouldn't we process timers when poll returns 0 (timeout) ? or we go
> >> there to check just in case we missed a deadline?
> > 
> > I think processing timers is fine in that case, just in case. A poll()
> > failure will likely be fatal anyway, so it won't matter much.
> 
> So it is not worth aborting/returning the poll errno?

According to its man page, poll() and ppoll() can return the following errors:

- EFAULT The array given as argument was not contained in the calling 
program's address space.
- EINTR  A signal occurred before any requested event; see signal(7).
- EINVAL The nfds value exceeds the RLIMIT_NOFILE value.
- ENOMEM There was no space to allocate file descriptor tables.

EFAULT means we did something really wrong and I expect a crash to follow soon 
afterwards. ENOMEM is also a symptom of a really bad problem that will at best 
invoke the OOM killer in the near future. EINVAL is more debatable, but I'm 
not sure what we could do there. EINTR is likely the only error we really need 
to handle, and that should be handled internally. I'll post a patch on top of 
this series (with a related test case).

> >>> +	processTimers();
> >>> +}
> >>> +
> >>> +short EventDispatcherPoll::EventNotifierSetPoll::events() const
> >>> +{
> >>> +	short events = 0;
> >>> +
> >>> +	if (notifiers[EventNotifier::Read])
> >>> +		events |= POLLIN;
> >>> +	if (notifiers[EventNotifier::Write])
> >>> +		events |= POLLOUT;
> >>> +	if (notifiers[EventNotifier::Exception])
> >>> +		events |= POLLPRI;
> >>> +
> >>> +	return events;
> >>> +}
> >>> +
> >>> +void EventDispatcherPoll::processNotifiers(std::vector<struct pollfd>
> >>> &pollfds)
> >>> +{
> >>> +	static const struct {
> >>> +		EventNotifier::Type type;
> >>> +		short events;
> >>> +	} events[] = {
> >>> +		{ EventNotifier::Read, POLLIN },
> >>> +		{ EventNotifier::Write, POLLOUT },
> >>> +		{ EventNotifier::Exception, POLLPRI },
> >>> +	};
> >>> +
> >>> +	for (const struct pollfd &pfd : pollfds) {
> >>> +		auto iter = notifiers_.find(pfd.fd);
> >>> +		ASSERT(iter != notifiers_.end());
> >> 
> >> Is this fatal? I mean, this should not happen, I agree, but do we want
> >> to abort?
> > 
> > It's a serious bug in the library that would likely indicate a memory
> > corruption of some sort, or really incorrect library code. I expect the
> > program to crash later in that case, aborting early will make it easier to
> > debug the problem.
> 
> A notifier can be removed between the time poll is called and poll is
> returned. I agree it's a corner case though.

Not in a single-threaded application though. For the multi-threaded, if other 
threads start corrupting our state, we're screwed anyway. They must not do 
that. We'll have to document the thread-related requirements of the API when 
we'll implement multi-thread support.

> >>> +
> >>> +		EventNotifierSetPoll &set = iter->second;
> >>> +
> >>> +		for (const auto &event : events) {
> >>> +			EventNotifier *notifier = set.notifiers[event.type];
> >>> +
> >>> +			if (!notifier)
> >>> +				continue;
> >>> +
> >>> +			/*
> >>> +			 * If the file descriptor is invalid, disable the
> >>> +			 * notifier immediately.
> >>> +			 */
> >>> +			if (pfd.revents & POLLNVAL) {
> >>> +				LOG(Warning) << "Disabling " << notifierType(event.type)
> >>> +					     << " due to invalid file descriptor "
> >>> +					     << pfd.fd;
> >>> +				unregisterEventNotifier(notifier);
> >> 
> >> I'm not sure if POLLNVAL is exclusive (when it's set, no other bit
> >> field is set in pfd.revents), but if it is not, and some other event
> >> source flag is set, after unregistering, shouldn't you continue?
> > 
> > I would assume it's exclusive indeed, a continue shouldn't hurt here, I'll
> > add one (but it shouldn't make a big difference either, given that
> > POLLNVAL shouldn't occur at all in the normal case).
> > 
> >>> +			}
> >>> +
> >>> +			if (pfd.revents & event.events)
> >>> +				notifier->activated.emit(notifier);
> >>> +		}
> >>> +	}
> >>> +}

[snip]

> >>> diff --git a/src/libcamera/include/event_dispatcher_poll.h
> >>> b/src/libcamera/include/event_dispatcher_poll.h new file mode 100644
> >>> index 000000000000..600289e455e2
> >>> --- /dev/null
> >>> +++ b/src/libcamera/include/event_dispatcher_poll.h
> >>> @@ -0,0 +1,50 @@
> >>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >>> +/*
> >>> + * Copyright (C) 2019, Google Inc.
> >>> + *
> >>> + * event_loop.h - Camera object interface
> >> 
> >> Camera object?
> > 
> > Can you guess where this was copied from ? :-)
> > 
> > I've now fixed it (as well as the file name).
> > 
> >>> + */
> >>> +#ifndef __LIBCAMERA_EVENT_DISPATCHER_POLL_H__
> >>> +#define __LIBCAMERA_EVENT_DISPATCHER_POLL_H__
> >>> +
> >>> +#include <libcamera/event_dispatcher.h>
> >>> +
> >>> +#include <list>
> >>> +#include <map>
> >>> +#include <vector>
> >>> +
> >>> +namespace libcamera {
> >>> +
> >>> +class EventNotifier;
> >>> +class Timer;
> >> 
> >> Still a bit skeptical about forward declarations when an include would
> >> suffice, but that's up to you actually.
> > 
> > On the plus side, they speed up compilation and avoid potential future
> > circular inclusions. Do they cause issues ?
> 
> Not particularly, as it is unlikely we miss recompiling this file if
> the header change as the library is built together.
> 
> Thanks for the multiple clarifications.
> It is my opinion you should push the next iteration of the series.

Thank you.

[snip]