[libcamera-devel,v9,2/4] cam: event_loop: Add timer events to event loop

15997 diff mbox series

Extend the EventLoop class to support periodic timer events. This can be
used to run tasks periodically, such as handling the event loop of SDL.

Also delete all events in the list, before we event_base_loopbreak, in
an effort to avoid race conditions.

Signed-off-by: Eric Curtin <ecurtin@redhat.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Tested-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/cam/event_loop.cpp | 27 +++++++++++++++++++++++++++
 src/cam/event_loop.h   |  7 ++++++-
 2 files changed, 33 insertions(+), 1 deletion(-)

Hi Eric,

Thank you for the patch.

On Fri, May 20, 2022 at 08:01:04PM +0100, Eric Curtin wrote:
> Extend the EventLoop class to support periodic timer events. This can be
> used to run tasks periodically, such as handling the event loop of SDL.
> 
> Also delete all events in the list, before we event_base_loopbreak, in
> an effort to avoid race conditions.

What race condition in particular does this solve ?

> Signed-off-by: Eric Curtin <ecurtin@redhat.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Tested-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/cam/event_loop.cpp | 27 +++++++++++++++++++++++++++
>  src/cam/event_loop.h   |  7 ++++++-
>  2 files changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/src/cam/event_loop.cpp b/src/cam/event_loop.cpp
> index 2e3ce995..315da38a 100644
> --- a/src/cam/event_loop.cpp
> +++ b/src/cam/event_loop.cpp
> @@ -47,6 +47,8 @@ int EventLoop::exec()
>  void EventLoop::exit(int code)
>  {
>  	exitCode_ = code;
> +	events_.clear();
> +
>  	event_base_loopbreak(base_);
>  }
>  
> @@ -84,6 +86,31 @@ void EventLoop::addFdEvent(int fd, EventType type,
>  	events_.push_back(std::move(event));
>  }
>  
> +void EventLoop::addTimerEvent(const duration period,
> +			      const std::function<void()> &callback)
> +{
> +	std::unique_ptr<Event> event = std::make_unique<Event>(callback);
> +	event->event_ = event_new(base_, -1, EV_PERSIST, &EventLoop::Event::dispatch,
> +				  event.get());
> +	if (!event->event_) {
> +		std::cerr << "Failed to create timer event" << std::endl;
> +		return;
> +	}
> +
> +	struct timeval tv;
> +	const uint64_t usecs = std::chrono::duration_cast<std::chrono::microseconds>(period).count();
> +	tv.tv_sec = usecs / 1000000ULL;
> +	tv.tv_usec = usecs % 1000000ULL;
> +
> +	const int ret = event_add(event->event_, &tv);

No need for const here.

> +	if (ret < 0) {
> +		std::cerr << "Failed to add timer event" << std::endl;
> +		return;
> +	}
> +
> +	events_.push_back(std::move(event));
> +}
> +
>  void EventLoop::dispatchCallback([[maybe_unused]] evutil_socket_t fd,
>  				 [[maybe_unused]] short flags, void *param)
>  {
> diff --git a/src/cam/event_loop.h b/src/cam/event_loop.h
> index 79902d87..22769ea5 100644
> --- a/src/cam/event_loop.h
> +++ b/src/cam/event_loop.h
> @@ -7,9 +7,10 @@
>  
>  #pragma once
>  
> +#include <chrono>
>  #include <functional>
> -#include <memory>
>  #include <list>
> +#include <memory>
>  #include <mutex>
>  
>  #include <event2/util.h>
> @@ -37,6 +38,10 @@ public:
>  	void addFdEvent(int fd, EventType type,
>  			const std::function<void()> &handler);
>  
> +	using duration = std::chrono::steady_clock::duration;

Is there a need to use the duration of steady_clock, is there any
drawback in using microseconds or milliseconds ?

I can address these small issues when applying if the other patches in
the series are fine.

> +	void addTimerEvent(const duration period,
> +			   const std::function<void()> &handler);
> +
>  private:
>  	struct Event {
>  		Event(const std::function<void()> &callback);

Hi Eric,

On Sun, May 22, 2022 at 01:07:47PM +0300, Laurent Pinchart via libcamera-devel wrote:
> On Fri, May 20, 2022 at 08:01:04PM +0100, Eric Curtin wrote:
> > Extend the EventLoop class to support periodic timer events. This can be
> > used to run tasks periodically, such as handling the event loop of SDL.
> > 
> > Also delete all events in the list, before we event_base_loopbreak, in
> > an effort to avoid race conditions.
> 
> What race condition in particular does this solve ?

This is the only part I'm not sure about in the whole series. If there's
a race condition you've identified, could you please describe it ? I'll
then update the commit message accordingly and push. If the race is
theoritical only, I'd prefer splitting this to a separate patch, to be
discussed separately.

> > Signed-off-by: Eric Curtin <ecurtin@redhat.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > Tested-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  src/cam/event_loop.cpp | 27 +++++++++++++++++++++++++++
> >  src/cam/event_loop.h   |  7 ++++++-
> >  2 files changed, 33 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/cam/event_loop.cpp b/src/cam/event_loop.cpp
> > index 2e3ce995..315da38a 100644
> > --- a/src/cam/event_loop.cpp
> > +++ b/src/cam/event_loop.cpp
> > @@ -47,6 +47,8 @@ int EventLoop::exec()
> >  void EventLoop::exit(int code)
> >  {
> >  	exitCode_ = code;
> > +	events_.clear();
> > +
> >  	event_base_loopbreak(base_);
> >  }
> >  
> > @@ -84,6 +86,31 @@ void EventLoop::addFdEvent(int fd, EventType type,
> >  	events_.push_back(std::move(event));
> >  }
> >  
> > +void EventLoop::addTimerEvent(const duration period,
> > +			      const std::function<void()> &callback)
> > +{
> > +	std::unique_ptr<Event> event = std::make_unique<Event>(callback);
> > +	event->event_ = event_new(base_, -1, EV_PERSIST, &EventLoop::Event::dispatch,
> > +				  event.get());
> > +	if (!event->event_) {
> > +		std::cerr << "Failed to create timer event" << std::endl;
> > +		return;
> > +	}
> > +
> > +	struct timeval tv;
> > +	const uint64_t usecs = std::chrono::duration_cast<std::chrono::microseconds>(period).count();
> > +	tv.tv_sec = usecs / 1000000ULL;
> > +	tv.tv_usec = usecs % 1000000ULL;
> > +
> > +	const int ret = event_add(event->event_, &tv);
> 
> No need for const here.
> 
> > +	if (ret < 0) {
> > +		std::cerr << "Failed to add timer event" << std::endl;
> > +		return;
> > +	}
> > +
> > +	events_.push_back(std::move(event));
> > +}
> > +
> >  void EventLoop::dispatchCallback([[maybe_unused]] evutil_socket_t fd,
> >  				 [[maybe_unused]] short flags, void *param)
> >  {
> > diff --git a/src/cam/event_loop.h b/src/cam/event_loop.h
> > index 79902d87..22769ea5 100644
> > --- a/src/cam/event_loop.h
> > +++ b/src/cam/event_loop.h
> > @@ -7,9 +7,10 @@
> >  
> >  #pragma once
> >  
> > +#include <chrono>
> >  #include <functional>
> > -#include <memory>
> >  #include <list>
> > +#include <memory>
> >  #include <mutex>
> >  
> >  #include <event2/util.h>
> > @@ -37,6 +38,10 @@ public:
> >  	void addFdEvent(int fd, EventType type,
> >  			const std::function<void()> &handler);
> >  
> > +	using duration = std::chrono::steady_clock::duration;
> 
> Is there a need to use the duration of steady_clock, is there any
> drawback in using microseconds or milliseconds ?
> 
> I can address these small issues when applying if the other patches in
> the series are fine.
> 
> > +	void addTimerEvent(const duration period,
> > +			   const std::function<void()> &handler);
> > +
> >  private:
> >  	struct Event {
> >  		Event(const std::function<void()> &callback);