[libcamera-devel] [PATCH v2 07/11] libcamera: Add a poll-based event dispatcher
Jacopo Mondi
jacopo at jmondi.org
Tue Jan 8 11:28:01 CET 2019
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 at 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 at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190108/dc47cbb4/attachment.sig>
More information about the libcamera-devel
mailing list