[libcamera-devel] [PATCH v2 07/11] libcamera: Add a poll-based event dispatcher
Jacopo Mondi
jacopo at jmondi.org
Tue Jan 8 13:32:41 CET 2019
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 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
>
> [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
>
>
>
-------------- 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/9c217562/attachment-0001.sig>
More information about the libcamera-devel
mailing list