[{"id":23123,"web_url":"https://patchwork.libcamera.org/comment/23123/","msgid":"","date":"2022-05-22T10:07:47","subject":"Re: [libcamera-devel] [PATCH v9 2/4] cam: event_loop: Add timer\n\tevents to event loop","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Eric,\n\nThank you for the patch.\n\nOn Fri, May 20, 2022 at 08:01:04PM +0100, Eric Curtin wrote:\n> Extend the EventLoop class to support periodic timer events. This can be\n> used to run tasks periodically, such as handling the event loop of SDL.\n> \n> Also delete all events in the list, before we event_base_loopbreak, in\n> an effort to avoid race conditions.\n\nWhat race condition in particular does this solve ?\n\n> Signed-off-by: Eric Curtin \n> Reviewed-by: Laurent Pinchart \n> Reviewed-by: Kieran Bingham \n> Tested-by: Jacopo Mondi \n> ---\n> src/cam/event_loop.cpp | 27 +++++++++++++++++++++++++++\n> src/cam/event_loop.h | 7 ++++++-\n> 2 files changed, 33 insertions(+), 1 deletion(-)\n> \n> diff --git a/src/cam/event_loop.cpp b/src/cam/event_loop.cpp\n> index 2e3ce995..315da38a 100644\n> --- a/src/cam/event_loop.cpp\n> +++ b/src/cam/event_loop.cpp\n> @@ -47,6 +47,8 @@ int EventLoop::exec()\n> void EventLoop::exit(int code)\n> {\n> \texitCode_ = code;\n> +\tevents_.clear();\n> +\n> \tevent_base_loopbreak(base_);\n> }\n> \n> @@ -84,6 +86,31 @@ void EventLoop::addFdEvent(int fd, EventType type,\n> \tevents_.push_back(std::move(event));\n> }\n> \n> +void EventLoop::addTimerEvent(const duration period,\n> +\t\t\t const std::function &callback)\n> +{\n> +\tstd::unique_ptr event = std::make_unique(callback);\n> +\tevent->event_ = event_new(base_, -1, EV_PERSIST, &EventLoop::Event::dispatch,\n> +\t\t\t\t event.get());\n> +\tif (!event->event_) {\n> +\t\tstd::cerr << \"Failed to create timer event\" << std::endl;\n> +\t\treturn;\n> +\t}\n> +\n> +\tstruct timeval tv;\n> +\tconst uint64_t usecs = std::chrono::duration_cast(period).count();\n> +\ttv.tv_sec = usecs / 1000000ULL;\n> +\ttv.tv_usec = usecs % 1000000ULL;\n> +\n> +\tconst int ret = event_add(event->event_, &tv);\n\nNo need for const here.\n\n> +\tif (ret < 0) {\n> +\t\tstd::cerr << \"Failed to add timer event\" << std::endl;\n> +\t\treturn;\n> +\t}\n> +\n> +\tevents_.push_back(std::move(event));\n> +}\n> +\n> void EventLoop::dispatchCallback([[maybe_unused]] evutil_socket_t fd,\n> \t\t\t\t [[maybe_unused]] short flags, void *param)\n> {\n> diff --git a/src/cam/event_loop.h b/src/cam/event_loop.h\n> index 79902d87..22769ea5 100644\n> --- a/src/cam/event_loop.h\n> +++ b/src/cam/event_loop.h\n> @@ -7,9 +7,10 @@\n> \n> #pragma once\n> \n> +#include \n> #include \n> -#include \n> #include \n> +#include \n> #include \n> \n> #include \n> @@ -37,6 +38,10 @@ public:\n> \tvoid addFdEvent(int fd, EventType type,\n> \t\t\tconst std::function &handler);\n> \n> +\tusing duration = std::chrono::steady_clock::duration;\n\nIs there a need to use the duration of steady_clock, is there any\ndrawback in using microseconds or milliseconds ?\n\nI can address these small issues when applying if the other patches in\nthe series are fine.\n\n> +\tvoid addTimerEvent(const duration period,\n> +\t\t\t const std::function &handler);\n> +\n> private:\n> \tstruct Event {\n> \t\tEvent(const std::function &callback);","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 5816DBD161\n\tfor ;\n\tSun, 22 May 2022 10:07:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A3A6065663;\n\tSun, 22 May 2022 12:07:53 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1246760419\n\tfor ;\n\tSun, 22 May 2022 12:07:53 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(static-11-127-145-212.ipcom.comunitel.net [212.145.127.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 62D74563;\n\tSun, 22 May 2022 12:07:52 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1653214073;\n\tbh=CZnTHI6NOdnFqDEOrdoB/+RwYFPrxOs6heVS4tHiUiI=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=qI89FcDPOmZ1fE5HRjFNFWkI4sL7ElyrDajUmdcvm+unytatWIwM0EmC7lUffFitO\n\tRhjMCNhf9hq0D33GDtH046xrXyRxPKT7/qNc6VUHyQBGK3+eWOyJX76ehBytT/CJ4u\n\tutK+Fn6Ny81B5b1VyE4mUhRHLWxZnY2JG+6J3dEpfzNF/2UjF1fYA6HsCo02m7Cp56\n\ttKOu0oM4WLIKM44odyRRlh5tT5PQVvjmxodUg5olMNZAJNUUp+zMDYvDM4nmDKZq97\n\txiWMgqg17GY4lfh6f8kGoR7+lLjbbjBw5lxvTI8BqyKRyMFV75BeC+dqqF/toCUpWL\n\t6pSG3CthwljpQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1653214072;\n\tbh=CZnTHI6NOdnFqDEOrdoB/+RwYFPrxOs6heVS4tHiUiI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Y3YIez/f8HctAIwkKZftiZMYvBKSylPnPHX+1YcaLgkybMLJxPLiQlmXs+HVjPbEV\n\tEihYtj6JBwPqZItr1YQ+JDLfU6/ra2Og3fTmkYC/Ho0AUq7yYtsOfTx/nwNAdJYyCl\n\tD7jJvnNsLTdxH9A4oBeco63GSXPwiOR6wdvXz9ng="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Y3YIez/f\"; dkim-atps=neutral","Date":"Sun, 22 May 2022 13:07:47 +0300","To":"Eric Curtin ","Message-ID":"","References":"<20220520190106.425386-1-ecurtin@redhat.com>\n\t<20220520190106.425386-3-ecurtin@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220520190106.425386-3-ecurtin@redhat.com>","Subject":"Re: [libcamera-devel] [PATCH v9 2/4] cam: event_loop: Add timer\n\tevents to event loop","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23128,"web_url":"https://patchwork.libcamera.org/comment/23128/","msgid":"","date":"2022-05-22T17:32:09","subject":"Re: [libcamera-devel] [PATCH v9 2/4] cam: event_loop: Add timer\n\tevents to event loop","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Eric,\n\nOn Sun, May 22, 2022 at 01:07:47PM +0300, Laurent Pinchart via libcamera-devel wrote:\n> On Fri, May 20, 2022 at 08:01:04PM +0100, Eric Curtin wrote:\n> > Extend the EventLoop class to support periodic timer events. This can be\n> > used to run tasks periodically, such as handling the event loop of SDL.\n> > \n> > Also delete all events in the list, before we event_base_loopbreak, in\n> > an effort to avoid race conditions.\n> \n> What race condition in particular does this solve ?\n\nThis is the only part I'm not sure about in the whole series. If there's\na race condition you've identified, could you please describe it ? I'll\nthen update the commit message accordingly and push. If the race is\ntheoritical only, I'd prefer splitting this to a separate patch, to be\ndiscussed separately.\n\n> > Signed-off-by: Eric Curtin \n> > Reviewed-by: Laurent Pinchart \n> > Reviewed-by: Kieran Bingham \n> > Tested-by: Jacopo Mondi \n> > ---\n> > src/cam/event_loop.cpp | 27 +++++++++++++++++++++++++++\n> > src/cam/event_loop.h | 7 ++++++-\n> > 2 files changed, 33 insertions(+), 1 deletion(-)\n> > \n> > diff --git a/src/cam/event_loop.cpp b/src/cam/event_loop.cpp\n> > index 2e3ce995..315da38a 100644\n> > --- a/src/cam/event_loop.cpp\n> > +++ b/src/cam/event_loop.cpp\n> > @@ -47,6 +47,8 @@ int EventLoop::exec()\n> > void EventLoop::exit(int code)\n> > {\n> > \texitCode_ = code;\n> > +\tevents_.clear();\n> > +\n> > \tevent_base_loopbreak(base_);\n> > }\n> > \n> > @@ -84,6 +86,31 @@ void EventLoop::addFdEvent(int fd, EventType type,\n> > \tevents_.push_back(std::move(event));\n> > }\n> > \n> > +void EventLoop::addTimerEvent(const duration period,\n> > +\t\t\t const std::function &callback)\n> > +{\n> > +\tstd::unique_ptr event = std::make_unique(callback);\n> > +\tevent->event_ = event_new(base_, -1, EV_PERSIST, &EventLoop::Event::dispatch,\n> > +\t\t\t\t event.get());\n> > +\tif (!event->event_) {\n> > +\t\tstd::cerr << \"Failed to create timer event\" << std::endl;\n> > +\t\treturn;\n> > +\t}\n> > +\n> > +\tstruct timeval tv;\n> > +\tconst uint64_t usecs = std::chrono::duration_cast(period).count();\n> > +\ttv.tv_sec = usecs / 1000000ULL;\n> > +\ttv.tv_usec = usecs % 1000000ULL;\n> > +\n> > +\tconst int ret = event_add(event->event_, &tv);\n> \n> No need for const here.\n> \n> > +\tif (ret < 0) {\n> > +\t\tstd::cerr << \"Failed to add timer event\" << std::endl;\n> > +\t\treturn;\n> > +\t}\n> > +\n> > +\tevents_.push_back(std::move(event));\n> > +}\n> > +\n> > void EventLoop::dispatchCallback([[maybe_unused]] evutil_socket_t fd,\n> > \t\t\t\t [[maybe_unused]] short flags, void *param)\n> > {\n> > diff --git a/src/cam/event_loop.h b/src/cam/event_loop.h\n> > index 79902d87..22769ea5 100644\n> > --- a/src/cam/event_loop.h\n> > +++ b/src/cam/event_loop.h\n> > @@ -7,9 +7,10 @@\n> > \n> > #pragma once\n> > \n> > +#include \n> > #include \n> > -#include \n> > #include \n> > +#include \n> > #include \n> > \n> > #include \n> > @@ -37,6 +38,10 @@ public:\n> > \tvoid addFdEvent(int fd, EventType type,\n> > \t\t\tconst std::function &handler);\n> > \n> > +\tusing duration = std::chrono::steady_clock::duration;\n> \n> Is there a need to use the duration of steady_clock, is there any\n> drawback in using microseconds or milliseconds ?\n> \n> I can address these small issues when applying if the other patches in\n> the series are fine.\n> \n> > +\tvoid addTimerEvent(const duration period,\n> > +\t\t\t const std::function &handler);\n> > +\n> > private:\n> > \tstruct Event {\n> > \t\tEvent(const std::function &callback);","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 70471BD160\n\tfor ;\n\tSun, 22 May 2022 17:32:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 228F560419;\n\tSun, 22 May 2022 19:32:18 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C5B4360419\n\tfor ;\n\tSun, 22 May 2022 19:32:16 +0200 (CEST)","from pendragon.ideasonboard.com (unknown [86.8.200.222])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id F10F545F;\n\tSun, 22 May 2022 19:32:15 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1653240738;\n\tbh=FTdF1OAFNoN8O1OBTwBRkYGx3tPx+V7nU7iO1HUFyL0=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=fR16XmO85krOicM07QbuOXDTE/r6ttKoYlQ5v6O6+LDzWL58liiDTUgaKEmy340j7\n\tflNt48LoawkwND3mq0fZsCm7zwWgvYJDy6NrQu0rSetpuButVvnYhSjhgd+jgNPjsS\n\tSbgbla89YwmzwEHRuVvvekXFqHVZRcCXllztpNFbfMbwn5soPt9IqAY2LGW/2NFXV8\n\tIQgctaT2ujw6GOEg8dVIbH5xm2uImul49FD2WkpxQ96tsetWAp4KAO0Ut1OWXz4luS\n\tg9QJ1CX7s/rBTb6BhKm0TpVq58zvF2avlc65o5syHEGKeJdGma4bYEvWoEV68NteOo\n\tl7vJ4I4IC2XZA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1653240736;\n\tbh=FTdF1OAFNoN8O1OBTwBRkYGx3tPx+V7nU7iO1HUFyL0=;\n\th=Date:From:To:Subject:References:In-Reply-To:From;\n\tb=LzXDLTu83r0vX/ISWDvo7KVI5lWZjz0ff/XCkbcmtGF0rxPAPjBkD0Ru3vT57N0bB\n\tkibqmgYUZA9XJW1DU/u5W2zU51PDmORHfP/VyFbkXTdVzbBN0Ej8zutVQYdKT02dmx\n\t1a1m9DMKEg1zCoJXc1GpYBUAgGWInjuAFC0O853Q="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"LzXDLTu8\"; dkim-atps=neutral","Date":"Sun, 22 May 2022 20:32:09 +0300","To":"Eric Curtin , libcamera-devel@lists.libcamera.org","Message-ID":"","References":"<20220520190106.425386-1-ecurtin@redhat.com>\n\t<20220520190106.425386-3-ecurtin@redhat.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v9 2/4] cam: event_loop: Add timer\n\tevents to event loop","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]