[{"id":11750,"web_url":"https://patchwork.libcamera.org/comment/11750/","msgid":"<20200731123052.GD6218@pendragon.ideasonboard.com>","date":"2020-07-31T12:30:52","subject":"Re: [libcamera-devel] [PATCH v2 1/3] libcamera: thread: Support\n\tselective message dispatch to thread","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nThank you for the patch.\n\nOn Fri, Jul 31, 2020 at 10:53:41AM +0000, Umang Jain wrote:\n> Extend the current dispatchMessages() to support dispatching of\n> selective messsages according to the Message::Type passed in\n> the function argument. dispatchMessages() can now be called\n> explicitly to force deliver selected type's message to the\n> thread for processing (typically when event loop is not\n> running).\n> \n> Add a helper Message::Type::CatchAll message type to deliver every\n> message posted to the thread.\n> \n> Signed-off-by: Umang Jain \n> ---\n> include/libcamera/internal/message.h | 1 +\n> include/libcamera/internal/thread.h | 3 +-\n> src/libcamera/event_dispatcher_poll.cpp | 2 +-\n> src/libcamera/message.cpp | 2 ++\n> src/libcamera/thread.cpp | 47 ++++++++++++++++++-------\n> 5 files changed, 40 insertions(+), 15 deletions(-)\n> \n> diff --git a/include/libcamera/internal/message.h b/include/libcamera/internal/message.h\n> index 92ea64a..b8d9866 100644\n> --- a/include/libcamera/internal/message.h\n> +++ b/include/libcamera/internal/message.h\n> @@ -25,6 +25,7 @@ public:\n> \t\tNone = 0,\n> \t\tInvokeMessage = 1,\n> \t\tThreadMoveMessage = 2,\n> +\t\tCatchAll = 999,\n\nI don't think we need this, using 0 should be good enough for the\npurpose at hand.\n\n> \t\tUserMessage = 1000,\n> \t};\n> \n> diff --git a/include/libcamera/internal/thread.h b/include/libcamera/internal/thread.h\n> index 7b59e58..1dfeb72 100644\n> --- a/include/libcamera/internal/thread.h\n> +++ b/include/libcamera/internal/thread.h\n> @@ -14,6 +14,7 @@\n> \n> #include \n> \n> +#include \"libcamera/internal/message.h\"\n> #include \"libcamera/internal/utils.h\"\n> \n> namespace libcamera {\n> @@ -47,7 +48,7 @@ public:\n> \tEventDispatcher *eventDispatcher();\n> \tvoid setEventDispatcher(std::unique_ptr dispatcher);\n> \n> -\tvoid dispatchMessages();\n> +\tvoid dispatchMessages(Message::Type type);\n\nI'd add a default value here\n\n\tvoid dispatchMessages(Message::Type type = Message::Type::None);\n\n> \n> protected:\n> \tint exec();\n> diff --git a/src/libcamera/event_dispatcher_poll.cpp b/src/libcamera/event_dispatcher_poll.cpp\n> index 9ab85da..b9fabf8 100644\n> --- a/src/libcamera/event_dispatcher_poll.cpp\n> +++ b/src/libcamera/event_dispatcher_poll.cpp\n> @@ -146,7 +146,7 @@ void EventDispatcherPoll::processEvents()\n> {\n> \tint ret;\n> \n> -\tThread::current()->dispatchMessages();\n> +\tThread::current()->dispatchMessages(Message::Type::CatchAll);\n\nSo this can be dropped.\n\n> \n> \t/* Create the pollfd array. */\n> \tstd::vector pollfds;\n> diff --git a/src/libcamera/message.cpp b/src/libcamera/message.cpp\n> index e9b3e73..e462f90 100644\n> --- a/src/libcamera/message.cpp\n> +++ b/src/libcamera/message.cpp\n> @@ -49,6 +49,8 @@ std::atomic_uint Message::nextUserType_{ Message::UserMessage };\n> * \\brief Asynchronous method invocation across threads\n> * \\var Message::ThreadMoveMessage\n> * \\brief Object is being moved to a different thread\n> + * \\var Message::CatchAll\n> + * \\brief Helper to match message of any type\n> * \\var Message::UserMessage\n> * \\brief First value available for user-defined messages\n> */\n> diff --git a/src/libcamera/thread.cpp b/src/libcamera/thread.cpp\n> index d1750d7..d3a5f81 100644\n> --- a/src/libcamera/thread.cpp\n> +++ b/src/libcamera/thread.cpp\n> @@ -552,26 +552,47 @@ void Thread::removeMessages(Object *receiver)\n> }\n> \n> /**\n> - * \\brief Dispatch all posted messages for this thread\n> + * \\brief Dispatch posted messages for this thread as per \\a type Message::Type.\n> + * Pass Message::Type::CatchAll to dispatch every posted message posted for\n> + * this thread.\n\nLet's keep the brief brief :-) You're also missing the parameter\ndescription.\n\n * \\brief Dispatch posted messages for this thread\n * \\param[in] type The message type\n *\n * This function immediately dispatches all the messages previously posted for\n * this thread with postMessage() that match the message \\a type. If the \\a type\n * is Message::Type::None, all messages are dispatched.\n\n> */\n> -void Thread::dispatchMessages()\n> +void Thread::dispatchMessages(Message::Type type)\n> {\n> \tMutexLocker locker(data_->messages_.mutex_);\n> \n> -\twhile (!data_->messages_.list_.empty()) {\n> -\t\tstd::unique_ptr msg = std::move(data_->messages_.list_.front());\n> -\t\tdata_->messages_.list_.pop_front();\n> -\t\tif (!msg)\n> -\t\t\tcontinue;\n> +\tif (type == Message::Type::CatchAll) {\n> +\t\twhile (!data_->messages_.list_.empty()) {\n> +\t\t\tstd::unique_ptr msg =\n> +\t\t\t\tstd::move(data_->messages_.list_.front());\n> +\t\t\tdata_->messages_.list_.pop_front();\n> +\t\t\tif (!msg)\n> +\t\t\t\tcontinue;\n> \n> -\t\tObject *receiver = msg->receiver_;\n> -\t\tASSERT(data_ == receiver->thread()->data_);\n> +\t\t\tObject *receiver = msg->receiver_;\n> +\t\t\tASSERT(data_ == receiver->thread()->data_);\n> \n> -\t\treceiver->pendingMessages_--;\n> +\t\t\treceiver->pendingMessages_--;\n> \n> -\t\tlocker.unlock();\n> -\t\treceiver->message(msg.get());\n> -\t\tlocker.lock();\n> +\t\t\tlocker.unlock();\n> +\t\t\treceiver->message(msg.get());\n> +\t\t\tlocker.lock();\n> +\t\t}\n> +\t} else {\n> +\t\tfor (std::unique_ptr &msg : data_->messages_.list_) {\n> +\t\t\tif (!msg)\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\tif (msg->type() == type) {\n> +\t\t\t\tstd::unique_ptr message = std::move(msg);\n\nYou need to erase the list entry here, moving it to a local variable is\nfine from the point of view of deleting the message itself, but the list\nentry needs to be removed too. To do so, you can use std::list::erase(),\nbut that requires an iterator, so the loop needs to be turned into a\nregular for loop.\n\n> +\t\t\t\tObject *receiver = message->receiver_;\n> +\t\t\t\tASSERT(data_ == receiver->thread()->data_);\n> +\t\t\t\treceiver->pendingMessages_--;\n> +\n> +\t\t\t\tlocker.unlock();\n> +\t\t\t\treceiver->message(message.get());\n\nAs an optimization, you can free the message here, as that's not an\noperation that needs to be protected by the lock.\n\n> +\t\t\t\tlocker.lock();\n> +\t\t\t}\n> +\t\t}\n> \t}\n\nI think you can combine both case.\n\n\tstd::list> &messages = data_->messages_.list_;\n\n\tfor (auto iter = messages.begin(); iter != messages.end(); ) {\n\t\tstd::unique_ptr &msg = *iter;\n\n\t\tif (!msg || (type != Message::Type::None && msg->type() != type)) {\n\t\t\t++iter;\n\t\t\tcontinue;\n\t\t}\n\n\t\tstd::unique_ptr message = std::move(msg);\n\t\titer = data_->messages_.list_.erase(iter);\n\n\t\tObject *receiver = message->receiver_;\n\t\tASSERT(data_ == receiver->thread()->data_);\n\t\treceiver->pendingMessages_--;\n\n\t\tlocker.unlock();\n\t\treceiver->message(message.get());\n\t\tmessage.reset();\n\t\tlocker.lock();\n\t}\n\n> }\n>","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 28C8FBD86F\n\tfor ;\n\tFri, 31 Jul 2020 12:31:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AE86461988;\n\tFri, 31 Jul 2020 14:31:04 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B44BE60396\n\tfor ;\n\tFri, 31 Jul 2020 14:31:02 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2823153C;\n\tFri, 31 Jul 2020 14:31:02 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"hZnYlpE3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1596198662;\n\tbh=2KrvbXks9L+ZBEnxLEPzC01h0DkIFCDNUt/10ezRQIg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=hZnYlpE3GgtLo2Eq7/nKDLzTsqdKxwv5Hz+WRn70i5xlaMIt8BlQjXLlt6nL3ZVQh\n\tpyCwCjomxj0AO8bBMDuWWYohycbpgsBfBOZwV0gw9hXtlsJnFLv3fRPS8VVd56Rjh0\n\tsMSVD4QVfDjXlc/fTp8V60J62auzaSU6VZvjneck=","Date":"Fri, 31 Jul 2020 15:30:52 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"<20200731123052.GD6218@pendragon.ideasonboard.com>","References":"<20200731105335.62014-1-email@uajain.com>\n\t<20200731105335.62014-2-email@uajain.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200731105335.62014-2-email@uajain.com>","Subject":"Re: [libcamera-devel] [PATCH v2 1/3] libcamera: thread: Support\n\tselective message dispatch to thread","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]