[{"id":28459,"web_url":"https://patchwork.libcamera.org/comment/28459/","msgid":"<170518591472.3418233.17233321911691749297@ping.linuxembedded.co.uk>","date":"2024-01-13T22:45:14","subject":"Re: [libcamera-devel] [PATCH v2 05/18] libcamera: internal:\n\tDocument the SharedMemObject class","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Hans de Goede via libcamera-devel (2024-01-13 14:22:05)\n> From: Dennis Bonke \n> \n> Document the SharedMemObject class.\n> \n> Signed-off-by: Dennis Bonke \n> Signed-off-by: Hans de Goede \n> Tested-by: Bryan O'Donoghue # sc8280xp Lenovo x13s\n> Tested-by: Pavel Machek \n> ---\n> .../libcamera/internal/shared_mem_object.h | 53 +++++++++++++++++++\n> 1 file changed, 53 insertions(+)\n> \n> diff --git a/include/libcamera/internal/shared_mem_object.h b/include/libcamera/internal/shared_mem_object.h\n> index bfb639ee..e862ce48 100644\n> --- a/include/libcamera/internal/shared_mem_object.h\n> +++ b/include/libcamera/internal/shared_mem_object.h\n> @@ -19,10 +19,20 @@\n> \n> namespace libcamera {\n> \n> +/**\n> + * \\class SharedMemObject\n> + * \\brief Helper class for shared memory allocations.\n> + *\n> + * Takes a template T which is used to indicate the\n> + * data type of the object stored.\n\nI'd wrap this to the usual 80 chars. Not critical though.\n\nIt might be nice to explain here what the class is doing, as the code\nmay not be visible to the reader.\n\nThe only part I'd add is perhaps something like:\n\n\"\"\"\nMemory is allocated and exposed as a SharedFD for use across IPC\nboundaries.\n\"\"\"\n\nBut either way,\n\nReviewed-by: Kieran Bingham \n\n> + */\n> template\n> class SharedMemObject\n> {\n> public:\n> + /**\n> + * \\brief The size of the object that is going to be stored here.\n> + */\n> static constexpr std::size_t SIZE = sizeof(T);\n> \n> SharedMemObject()\n> @@ -30,6 +40,11 @@ public:\n> {\n> }\n> \n> + /**\n> + * \\brief Contstructor for the SharedMemObject.\n> + * \\param[in] name The requested name.\n> + * \\param[in] args Any additional args.\n> + */\n> template\n> SharedMemObject(const std::string &name, Args &&...args)\n> : name_(name), obj_(nullptr)\n> @@ -57,6 +72,10 @@ public:\n> obj_ = new (mem) T(std::forward(args)...);\n> }\n> \n> + /**\n> + * \\brief Move constructor for SharedMemObject.\n> + * \\param[in] rhs The object to move.\n> + */\n> SharedMemObject(SharedMemObject &&rhs)\n> {\n> this->name_ = std::move(rhs.name_);\n> @@ -76,6 +95,10 @@ public:\n> /* Make SharedMemObject non-copyable for now. */\n> LIBCAMERA_DISABLE_COPY(SharedMemObject)\n> \n> + /**\n> + * \\brief Operator= for SharedMemObject.\n> + * \\param[in] rhs The SharedMemObject object to take the data from.\n> + */\n> SharedMemObject &operator=(SharedMemObject &&rhs)\n> {\n> this->name_ = std::move(rhs.name_);\n> @@ -85,31 +108,61 @@ public:\n> return *this;\n> }\n> \n> + /**\n> + * \\brief Operator-> for SharedMemObject.\n> + *\n> + * \\return the object.\n> + */\n> T *operator->()\n> {\n> return obj_;\n> }\n> \n> + /**\n> + * \\brief Operator-> for SharedMemObject.\n> + *\n> + * \\return the object.\n> + */\n> const T *operator->() const\n> {\n> return obj_;\n> }\n> \n> + /**\n> + * \\brief Operator* for SharedMemObject.\n> + *\n> + * \\return the object.\n> + */\n> T &operator*()\n> {\n> return *obj_;\n> }\n> \n> + /**\n> + * \\brief Operator* for SharedMemObject.\n> + *\n> + * \\return the object.\n> + */\n> const T &operator*() const\n> {\n> return *obj_;\n> }\n> \n> + /**\n> + * \\brief Gets the file descriptor for the underlaying storage file.\n> + *\n> + * \\return the file descriptor.\n> + */\n> const SharedFD &fd() const\n> {\n> return fd_;\n> }\n> \n> + /**\n> + * \\brief Operator bool() for SharedMemObject.\n> + *\n> + * \\return true if the object is not null, false otherwise.\n> + */\n> explicit operator bool() const\n> {\n> return !!obj_;\n> -- \n> 2.43.0\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 736DEC323E\n\tfor ;\n\tSat, 13 Jan 2024 22:45:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8FC54628B7;\n\tSat, 13 Jan 2024 23:45:19 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F1F0661D57\n\tfor ;\n\tSat, 13 Jan 2024 23:45:17 +0100 (CET)","from pendragon.ideasonboard.com\n\t(aztw-30-b2-v4wan-166917-cust845.vm26.cable.virginm.net\n\t[82.37.23.78])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B499D975;\n\tSat, 13 Jan 2024 23:44:10 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1705185919;\n\tbh=AMCHY5pISpUvJad5+icOgStkJiPvGHQC0TylIRWaaZk=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=rKwUnbfJlPhVkLt3IxMfzAA+Qq2Yxpd6y+byCbbhVmpfflZeXtg8lc0qyOwaId8TC\n\tjfIr/g1/20B0wHkuxmjE70Qb5fIUHDaqqYKzD4lXR9qDjMvhOtR0KCZ5HPl1cjIxO4\n\tyXOoqVStA+gAMjv+Tet92zXyU7Hxm+OjUWhtVIVDGm3I4ygODxtt3AQFeFUdUMOBmL\n\tN5z80l6XioMHUd8gbXMMh5IH8+b8ajbuBQO521ImLXztSp1/Qb7qCuVLBEYsKGjrr+\n\ti3wrIcQG+O4qgi7RE700w/HP0mWlsCUuWzZUdM6FAbPaJQOP5bwDo6YWy8wBlH/0TP\n\t8+Cku+jYwbNdA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1705185850;\n\tbh=AMCHY5pISpUvJad5+icOgStkJiPvGHQC0TylIRWaaZk=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=Chkmqbnw8cZDEzBM8eIhzx1BK7hf1j8xHpoBq0RLox7ykxXwLr45mcPXh0qg3rrq5\n\tUYVwQYaQfP7cwWccXwloXoUqRk1WiSkf9wOSkbfYL5TXktJLVivra5GdO68c9oKpqW\n\tVP7ysXKvTgL8fXJpCsEGC5Mlx0eo8PeeeUnBrRA8="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Chkmqbnw\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240113142218.28063-6-hdegoede@redhat.com>","References":"<20240113142218.28063-1-hdegoede@redhat.com>\n\t<20240113142218.28063-6-hdegoede@redhat.com>","To":"Andrey Konovalov ,\n\tHans de Goede , libcamera-devel@lists.libcamera.org","Date":"Sat, 13 Jan 2024 22:45:14 +0000","Message-ID":"<170518591472.3418233.17233321911691749297@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v2 05/18] libcamera: internal:\n\tDocument the SharedMemObject class","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":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"Maxime Ripard , g.martti@gmail.com,\n\tt.langendam@gmail.com, srinivas.kandagatla@linaro.org,\n\tPavel Machek ,\n\tBryan O'Donoghue , admin@dennisbonke.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28590,"web_url":"https://patchwork.libcamera.org/comment/28590/","msgid":"<87jzo09oov.fsf@redhat.com>","date":"2024-01-23T13:14:40","subject":"Re: [PATCH v2 05/18] libcamera: internal: Document the\n\tSharedMemObject class","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hans de Goede writes:\n\n> From: Dennis Bonke \n>\n> Document the SharedMemObject class.\n>\n> Signed-off-by: Dennis Bonke \n> Signed-off-by: Hans de Goede \n> Tested-by: Bryan O'Donoghue # sc8280xp Lenovo x13s\n> Tested-by: Pavel Machek \n> ---\n\nBesides what Kieran has already said:\n\n> .../libcamera/internal/shared_mem_object.h | 53 +++++++++++++++++++\n> 1 file changed, 53 insertions(+)\n>\n> diff --git a/include/libcamera/internal/shared_mem_object.h b/include/libcamera/internal/shared_mem_object.h\n> index bfb639ee..e862ce48 100644\n> --- a/include/libcamera/internal/shared_mem_object.h\n> +++ b/include/libcamera/internal/shared_mem_object.h\n> @@ -19,10 +19,20 @@\n> \n> namespace libcamera {\n> \n> +/**\n> + * \\class SharedMemObject\n> + * \\brief Helper class for shared memory allocations.\n> + *\n> + * Takes a template T which is used to indicate the\n> + * data type of the object stored.\n> + */\n> template\n> class SharedMemObject\n> {\n> public:\n> +\t/**\n> +\t * \\brief The size of the object that is going to be stored here.\n> +\t */\n> \tstatic constexpr std::size_t SIZE = sizeof(T);\n> \n> \tSharedMemObject()\n> @@ -30,6 +40,11 @@ public:\n> \t{\n> \t}\n> \n> +\t/**\n> +\t * \\brief Contstructor for the SharedMemObject.\n ^^^^^^^^^^^^\n\nConstructor\n\n> +\t * \\param[in] name The requested name.\n\nOf what? What's its purpose? Is it unique in any sense?\n\n> +\t * \\param[in] args Any additional args.\n\nTo what?\n\n> +\t */\n> \ttemplate\n> \tSharedMemObject(const std::string &name, Args &&...args)\n> \t\t: name_(name), obj_(nullptr)\n> @@ -57,6 +72,10 @@ public:\n> \t\tobj_ = new (mem) T(std::forward(args)...);\n> \t}\n> \n> +\t/**\n> +\t * \\brief Move constructor for SharedMemObject.\n> +\t * \\param[in] rhs The object to move.\n> +\t */\n> \tSharedMemObject(SharedMemObject &&rhs)\n> \t{\n> \t\tthis->name_ = std::move(rhs.name_);\n> @@ -76,6 +95,10 @@ public:\n> \t/* Make SharedMemObject non-copyable for now. */\n> \tLIBCAMERA_DISABLE_COPY(SharedMemObject)\n> \n> +\t/**\n> +\t * \\brief Operator= for SharedMemObject.\n> +\t * \\param[in] rhs The SharedMemObject object to take the data from.\n> +\t */\n> \tSharedMemObject &operator=(SharedMemObject &&rhs)\n> \t{\n> \t\tthis->name_ = std::move(rhs.name_);\n> @@ -85,31 +108,61 @@ public:\n> \t\treturn *this;\n> \t}\n> \n> +\t/**\n> +\t * \\brief Operator-> for SharedMemObject.\n> +\t *\n> +\t * \\return the object.\n> +\t */\n> \tT *operator->()\n> \t{\n> \t\treturn obj_;\n> \t}\n> \n> +\t/**\n> +\t * \\brief Operator-> for SharedMemObject.\n> +\t *\n> +\t * \\return the object.\n> +\t */\n> \tconst T *operator->() const\n> \t{\n> \t\treturn obj_;\n> \t}\n> \n> +\t/**\n> +\t * \\brief Operator* for SharedMemObject.\n> +\t *\n> +\t * \\return the object.\n> +\t */\n> \tT &operator*()\n> \t{\n> \t\treturn *obj_;\n> \t}\n> \n> +\t/**\n> +\t * \\brief Operator* for SharedMemObject.\n> +\t *\n> +\t * \\return the object.\n> +\t */\n> \tconst T &operator*() const\n> \t{\n> \t\treturn *obj_;\n> \t}\n> \n> +\t/**\n> +\t * \\brief Gets the file descriptor for the underlaying storage file.\n> +\t *\n> +\t * \\return the file descriptor.\n> +\t */\n> \tconst SharedFD &fd() const\n> \t{\n> \t\treturn fd_;\n> \t}\n> \n> +\t/**\n> +\t * \\brief Operator bool() for SharedMemObject.\n> +\t *\n> +\t * \\return true if the object is not null, false otherwise.\n> +\t */\n> \texplicit operator bool() const\n> \t{\n> \t\treturn !!obj_;","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 8E852BDC71\n\tfor ;\n\tTue, 23 Jan 2024 13:14:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D6CF062944;\n\tTue, 23 Jan 2024 14:14:46 +0100 (CET)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4704E628E9\n\tfor ;\n\tTue, 23 Jan 2024 14:14:45 +0100 (CET)","from mail-wm1-f69.google.com (mail-wm1-f69.google.com\n\t[209.85.128.69]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-653-UGe-wglyOd26wv7h9_XSYw-1; Tue, 23 Jan 2024 08:14:42 -0500","by mail-wm1-f69.google.com with SMTP id\n\t5b1f17b1804b1-40e9d6a364eso35575675e9.3\n\tfor ;\n\tTue, 23 Jan 2024 05:14:42 -0800 (PST)","from nuthatch (nat-pool-brq-t.redhat.com. [213.175.37.10])\n\tby smtp.gmail.com with ESMTPSA id\n\to31-20020a05600c511f00b0040e703ad630sm33838092wms.22.2024.01.23.05.14.40\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 23 Jan 2024 05:14:40 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"BFStznjZ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1706015684;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=ECAnwToAAmF/dTwr+4OO1EkKUQ+1g2977vqUWgmI4cY=;\n\tb=BFStznjZyjeIF4GTgqX0gDAHSvHkQRfC7bu9PbMCb9JzXgLutR7R6LeepbcedxS5g/MIcN\n\tIedJBPe47/FNIbHBVxqB6jeACCh/shi9SYl9+LHbUHnmDK8oAFpt3ecEeQPfhmcVSyLD+q\n\t3nrlbOAklNWH2sTJ7W6MMODPH2umHTQ=","X-MC-Unique":"UGe-wglyOd26wv7h9_XSYw-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1706015681; x=1706620481;\n\th=mime-version:user-agent:message-id:date:references:in-reply-to\n\t:subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=ECAnwToAAmF/dTwr+4OO1EkKUQ+1g2977vqUWgmI4cY=;\n\tb=RkwLo0dfUGJDafwnaxsRQRjoa/0SZxLY1rjfTeyhQWApGh60gKzUjnxGaXqgx6qoqp\n\tXXMx0ckqJ2NItIp+sJcnacBYWeOEZT8gzTC/bsI4s0quroHhOjYogoiXoyqYXq8vp2x2\n\tbCK578kuv+lkggycpmx460b6CNPDljyciyhtwrImTfHbv6b0m8jITv9hxX+OWf1OilYx\n\t5HqERbpaylwCGF828u6kVAKDJSRcZdRRUHnwFvdMOkF6McWK7xc9TIKZiGuD6DuJ3w7m\n\tuqpLocIKkFNoDe3TQs7Y4Z28QIXAmC+Og8ugqqXAsCx/dzUzqjYpNFpK0+0iASdBA5Vi\n\tXCzg==","X-Gm-Message-State":"AOJu0Yx7MgcM+4nzhQxFHQO5O7faw/XZ/zFLas4Ds6Fskc1mbfKt4quC\n\tK31UNYR0GtzOYB7kp/f6wXBCOLHWBe3jcedL6xgBTpBWQdoZnnqoSbBG2Fw/fg5xwP6g8aiGCqW\n\tns5tHhkJbyeCVZ3v6HMsS2wNdUCw+OUVvH5OsXuleC+SJDRMZEW7TWydI0W+idVLANZeNwNk=","X-Received":["by 2002:a05:600c:3396:b0:40d:9006:51c5 with SMTP id\n\to22-20020a05600c339600b0040d900651c5mr118506wmp.143.1706015681703; \n\tTue, 23 Jan 2024 05:14:41 -0800 (PST)","by 2002:a05:600c:3396:b0:40d:9006:51c5 with SMTP id\n\to22-20020a05600c339600b0040d900651c5mr118499wmp.143.1706015681376; \n\tTue, 23 Jan 2024 05:14:41 -0800 (PST)"],"X-Google-Smtp-Source":"AGHT+IHg29dHPhBSjTc27DdwUGv+qvXLfnT6ns4qH3PLAfEfzdQxp/5N28RMIMvKvD02mxu1DKL+gg==","From":"Milan Zamazal ","To":"Hans de Goede ","Subject":"Re: [PATCH v2 05/18] libcamera: internal: Document the\n\tSharedMemObject class","In-Reply-To":"<20240113142218.28063-6-hdegoede@redhat.com> (Hans de Goede's\n\tmessage of \"Sat, 13 Jan 2024 15:22:05 +0100\")","References":"<20240113142218.28063-1-hdegoede@redhat.com>\n\t<20240113142218.28063-6-hdegoede@redhat.com>","Date":"Tue, 23 Jan 2024 14:14:40 +0100","Message-ID":"<87jzo09oov.fsf@redhat.com>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","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":"Maxime Ripard , g.martti@gmail.com,\n\tt.langendam@gmail.com, libcamera-devel@lists.libcamera.org,\n\tsrinivas.kandagatla@linaro.org, Pavel Machek ,\n\tBryan O'Donoghue , admin@dennisbonke.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28591,"web_url":"https://patchwork.libcamera.org/comment/28591/","msgid":"<20240123133453.GD32612@pendragon.ideasonboard.com>","date":"2024-01-23T13:34:53","subject":"Re: [libcamera-devel] [PATCH v2 05/18] libcamera: internal: Document\n\tthe SharedMemObject class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Sat, Jan 13, 2024 at 10:45:14PM +0000, 📷-dev wrote:\n> Quoting Hans de Goede via libcamera-devel (2024-01-13 14:22:05)\n> > From: Dennis Bonke \n> > \n> > Document the SharedMemObject class.\n> > \n> > Signed-off-by: Dennis Bonke \n> > Signed-off-by: Hans de Goede \n> > Tested-by: Bryan O'Donoghue # sc8280xp Lenovo x13s\n> > Tested-by: Pavel Machek \n> > ---\n> > .../libcamera/internal/shared_mem_object.h | 53 +++++++++++++++++++\n> > 1 file changed, 53 insertions(+)\n> > \n> > diff --git a/include/libcamera/internal/shared_mem_object.h b/include/libcamera/internal/shared_mem_object.h\n> > index bfb639ee..e862ce48 100644\n> > --- a/include/libcamera/internal/shared_mem_object.h\n> > +++ b/include/libcamera/internal/shared_mem_object.h\n> > @@ -19,10 +19,20 @@\n> > \n> > namespace libcamera {\n> > \n> > +/**\n> > + * \\class SharedMemObject\n> > + * \\brief Helper class for shared memory allocations.\n> > + *\n> > + * Takes a template T which is used to indicate the\n> > + * data type of the object stored.\n> \n> I'd wrap this to the usual 80 chars. Not critical though.\n\nNot critical, but no reason not to do so :-)\n\n> It might be nice to explain here what the class is doing, as the code\n> may not be visible to the reader.\n> \n> The only part I'd add is perhaps something like:\n> \n> \"\"\"\n> Memory is allocated and exposed as a SharedFD for use across IPC\n> boundaries.\n> \"\"\"\n\nI would like more documentation too, with an explanation of how this is\nsupposed to be used.\n\n> But either way,\n> \n> Reviewed-by: Kieran Bingham \n\nDocumentation goes to .cpp files. The documentation should also come in\nthe same patch as the one that adds the class.\n\n> > + */\n> > template\n> > class SharedMemObject\n> > {\n> > public:\n> > + /**\n> > + * \\brief The size of the object that is going to be stored here.\n> > + */\n> > static constexpr std::size_t SIZE = sizeof(T);\n> > \n> > SharedMemObject()\n> > @@ -30,6 +40,11 @@ public:\n> > {\n> > }\n> > \n> > + /**\n> > + * \\brief Contstructor for the SharedMemObject.\n> > + * \\param[in] name The requested name.\n> > + * \\param[in] args Any additional args.\n> > + */\n> > template\n> > SharedMemObject(const std::string &name, Args &&...args)\n> > : name_(name), obj_(nullptr)\n> > @@ -57,6 +72,10 @@ public:\n> > obj_ = new (mem) T(std::forward(args)...);\n> > }\n> > \n> > + /**\n> > + * \\brief Move constructor for SharedMemObject.\n> > + * \\param[in] rhs The object to move.\n> > + */\n> > SharedMemObject(SharedMemObject &&rhs)\n> > {\n> > this->name_ = std::move(rhs.name_);\n> > @@ -76,6 +95,10 @@ public:\n> > /* Make SharedMemObject non-copyable for now. */\n> > LIBCAMERA_DISABLE_COPY(SharedMemObject)\n> > \n> > + /**\n> > + * \\brief Operator= for SharedMemObject.\n> > + * \\param[in] rhs The SharedMemObject object to take the data from.\n> > + */\n> > SharedMemObject &operator=(SharedMemObject &&rhs)\n> > {\n> > this->name_ = std::move(rhs.name_);\n> > @@ -85,31 +108,61 @@ public:\n> > return *this;\n> > }\n> > \n> > + /**\n> > + * \\brief Operator-> for SharedMemObject.\n> > + *\n> > + * \\return the object.\n> > + */\n> > T *operator->()\n> > {\n> > return obj_;\n> > }\n> > \n> > + /**\n> > + * \\brief Operator-> for SharedMemObject.\n> > + *\n> > + * \\return the object.\n> > + */\n> > const T *operator->() const\n> > {\n> > return obj_;\n> > }\n> > \n> > + /**\n> > + * \\brief Operator* for SharedMemObject.\n> > + *\n> > + * \\return the object.\n> > + */\n> > T &operator*()\n> > {\n> > return *obj_;\n> > }\n> > \n> > + /**\n> > + * \\brief Operator* for SharedMemObject.\n> > + *\n> > + * \\return the object.\n> > + */\n> > const T &operator*() const\n> > {\n> > return *obj_;\n> > }\n> > \n> > + /**\n> > + * \\brief Gets the file descriptor for the underlaying storage file.\n> > + *\n> > + * \\return the file descriptor.\n> > + */\n> > const SharedFD &fd() const\n> > {\n> > return fd_;\n> > }\n> > \n> > + /**\n> > + * \\brief Operator bool() for SharedMemObject.\n> > + *\n> > + * \\return true if the object is not null, false otherwise.\n> > + */\n> > explicit operator bool() const\n> > {\n> > return !!obj_;","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 D27A6C323E\n\tfor ;\n\tTue, 23 Jan 2024 13:34:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 861F762945;\n\tTue, 23 Jan 2024 14:34:56 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C8EB062916\n\tfor ;\n\tTue, 23 Jan 2024 14:34:54 +0100 (CET)","from pendragon.ideasonboard.com (89-27-53-110.bb.dnainternet.fi\n\t[89.27.53.110])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DA37B1571;\n\tTue, 23 Jan 2024 14:33:40 +0100 (CET)"],"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=\"e8MLNCVy\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1706016821;\n\tbh=og1Rl2WXB7KSPGWqZN12FcDIJPUonT8wZTBXrNIMliM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=e8MLNCVyaLmEZlaXOSSXClT2f0ZkIONutHAsjA5gxQzPJ9XnReevlO+mCKLnd4aDP\n\tXawoU4OBZfDZ0+oaLe7JMkMFHuxcU0odbrcQfaKEzb+nM/12KWu2mvdxUVrxR8i50g\n\tMlS6Y0FS+Ig/VYjGVnSopW25Q56TYCTxvPMNecTo=","Date":"Tue, 23 Jan 2024 15:34:53 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Subject":"Re: [libcamera-devel] [PATCH v2 05/18] libcamera: internal: Document\n\tthe SharedMemObject class","Message-ID":"<20240123133453.GD32612@pendragon.ideasonboard.com>","References":"<20240113142218.28063-1-hdegoede@redhat.com>\n\t<20240113142218.28063-6-hdegoede@redhat.com>\n\t<170518591472.3418233.17233321911691749297@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<170518591472.3418233.17233321911691749297@ping.linuxembedded.co.uk>","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":"Maxime Ripard , g.martti@gmail.com,\n\tt.langendam@gmail.com, libcamera-devel@lists.libcamera.org,\n\tsrinivas.kandagatla@linaro.org, Pavel Machek ,\n\tBryan O'Donoghue , admin@dennisbonke.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28600,"web_url":"https://patchwork.libcamera.org/comment/28600/","msgid":"<20240123144327.GD10679@pendragon.ideasonboard.com>","date":"2024-01-23T14:43:27","subject":"Re: [libcamera-devel] [PATCH v2 05/18] libcamera: internal: Document\n\tthe SharedMemObject class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Tue, Jan 23, 2024 at 03:34:53PM +0200, Laurent Pinchart wrote:\n> On Sat, Jan 13, 2024 at 10:45:14PM +0000, 📷-dev wrote:\n> > Quoting Hans de Goede via libcamera-devel (2024-01-13 14:22:05)\n> > > From: Dennis Bonke \n> > > \n> > > Document the SharedMemObject class.\n> > > \n> > > Signed-off-by: Dennis Bonke \n> > > Signed-off-by: Hans de Goede \n> > > Tested-by: Bryan O'Donoghue # sc8280xp Lenovo x13s\n> > > Tested-by: Pavel Machek \n> > > ---\n> > > .../libcamera/internal/shared_mem_object.h | 53 +++++++++++++++++++\n> > > 1 file changed, 53 insertions(+)\n> > > \n> > > diff --git a/include/libcamera/internal/shared_mem_object.h b/include/libcamera/internal/shared_mem_object.h\n> > > index bfb639ee..e862ce48 100644\n> > > --- a/include/libcamera/internal/shared_mem_object.h\n> > > +++ b/include/libcamera/internal/shared_mem_object.h\n> > > @@ -19,10 +19,20 @@\n> > > \n> > > namespace libcamera {\n> > > \n> > > +/**\n> > > + * \\class SharedMemObject\n> > > + * \\brief Helper class for shared memory allocations.\n> > > + *\n> > > + * Takes a template T which is used to indicate the\n> > > + * data type of the object stored.\n> > \n> > I'd wrap this to the usual 80 chars. Not critical though.\n> \n> Not critical, but no reason not to do so :-)\n> \n> > It might be nice to explain here what the class is doing, as the code\n> > may not be visible to the reader.\n> > \n> > The only part I'd add is perhaps something like:\n> > \n> > \"\"\"\n> > Memory is allocated and exposed as a SharedFD for use across IPC\n> > boundaries.\n> > \"\"\"\n> \n> I would like more documentation too, with an explanation of how this is\n> supposed to be used.\n> \n> > But either way,\n> > \n> > Reviewed-by: Kieran Bingham \n> \n> Documentation goes to .cpp files. The documentation should also come in\n> the same patch as the one that adds the class.\n\nJust to be clear, as there's no .cpp file yet, I think most of the\nSharedMemObject constructor should go to a separate init(size_t size)\nfunction moved to a .cpp file, as it doesn't otherwise depend on the\ntype T or arguments Args.\n\nThe way we create a single memfd for every SharedMemObject isn't very\nnice, as it could cause lots of fd allocations if callers are not aware\nthat they should create a single large object instead. This should be at\nthe very least clearly explained in the documentation. Ideally the\nclass should be refactored to make it possible to allocate multiple\nobjects within a single shared memory segment, but that's out of scope\nfor this series.\n\nAs for squashing documentation with the patch that introduces the class,\nI hadn't realized that the class had simply be moved from the rpi code.\nI'm OK keeping the two patches separate.\n\n> > > + */\n> > > template\n> > > class SharedMemObject\n> > > {\n> > > public:\n> > > + /**\n> > > + * \\brief The size of the object that is going to be stored here.\n> > > + */\n> > > static constexpr std::size_t SIZE = sizeof(T);\n\nWe don't use all uppercase constants. You can name this kSize or size.\n\n> > > \n> > > SharedMemObject()\n> > > @@ -30,6 +40,11 @@ public:\n> > > {\n> > > }\n> > > \n> > > + /**\n> > > + * \\brief Contstructor for the SharedMemObject.\n> > > + * \\param[in] name The requested name.\n> > > + * \\param[in] args Any additional args.\n> > > + */\n\nYou need to expand the documentation. Consider the point of view of a\nreader who hasn't seen the code. They can't tell from the documentation\nwhat the name and args are for.\n\nAlso, I'd like unit tests.\n\n> > > template\n> > > SharedMemObject(const std::string &name, Args &&...args)\n> > > : name_(name), obj_(nullptr)\n> > > @@ -57,6 +72,10 @@ public:\n> > > obj_ = new (mem) T(std::forward(args)...);\n> > > }\n> > > \n> > > + /**\n> > > + * \\brief Move constructor for SharedMemObject.\n> > > + * \\param[in] rhs The object to move.\n> > > + */\n> > > SharedMemObject(SharedMemObject &&rhs)\n> > > {\n> > > this->name_ = std::move(rhs.name_);\n> > > @@ -76,6 +95,10 @@ public:\n> > > /* Make SharedMemObject non-copyable for now. */\n> > > LIBCAMERA_DISABLE_COPY(SharedMemObject)\n> > > \n> > > + /**\n> > > + * \\brief Operator= for SharedMemObject.\n> > > + * \\param[in] rhs The SharedMemObject object to take the data from.\n> > > + */\n> > > SharedMemObject &operator=(SharedMemObject &&rhs)\n> > > {\n> > > this->name_ = std::move(rhs.name_);\n> > > @@ -85,31 +108,61 @@ public:\n> > > return *this;\n> > > }\n> > > \n> > > + /**\n> > > + * \\brief Operator-> for SharedMemObject.\n> > > + *\n> > > + * \\return the object.\n> > > + */\n> > > T *operator->()\n> > > {\n> > > return obj_;\n> > > }\n> > > \n> > > + /**\n> > > + * \\brief Operator-> for SharedMemObject.\n> > > + *\n> > > + * \\return the object.\n> > > + */\n> > > const T *operator->() const\n> > > {\n> > > return obj_;\n> > > }\n> > > \n> > > + /**\n> > > + * \\brief Operator* for SharedMemObject.\n> > > + *\n> > > + * \\return the object.\n> > > + */\n> > > T &operator*()\n> > > {\n> > > return *obj_;\n> > > }\n> > > \n> > > + /**\n> > > + * \\brief Operator* for SharedMemObject.\n> > > + *\n> > > + * \\return the object.\n> > > + */\n> > > const T &operator*() const\n> > > {\n> > > return *obj_;\n> > > }\n> > > \n> > > + /**\n> > > + * \\brief Gets the file descriptor for the underlaying storage file.\n> > > + *\n> > > + * \\return the file descriptor.\n> > > + */\n> > > const SharedFD &fd() const\n> > > {\n> > > return fd_;\n> > > }\n> > > \n> > > + /**\n> > > + * \\brief Operator bool() for SharedMemObject.\n> > > + *\n> > > + * \\return true if the object is not null, false otherwise.\n> > > + */\n> > > explicit operator bool() const\n> > > {\n> > > return !!obj_;","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 D594DBDC71\n\tfor ;\n\tTue, 23 Jan 2024 14:43:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2674762944;\n\tTue, 23 Jan 2024 15:43:31 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 71BCD628E9\n\tfor ;\n\tTue, 23 Jan 2024 15:43:29 +0100 (CET)","from pendragon.ideasonboard.com (89-27-53-110.bb.dnainternet.fi\n\t[89.27.53.110])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4C0741890;\n\tTue, 23 Jan 2024 15:42:15 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"rFKBzHva\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1706020935;\n\tbh=n4QTWQDPvshfghaG2T+KERNFMyefB9RiqX9M6cQB5p4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=rFKBzHvamyi3loduDAfLp0dxhXQ8+cPURcBtN4SsU+n7qQCn5DMy1EKYd4Nje33d1\n\th5boT6o2NkFH/JqNK27XOfdfMbxbkUz2QXoN5E0SDtoJVzZpG18zLBb5vXvud9Bp/n\n\twmdpeC/MA425XrukhfNK5ZPw9LIH9O/HhHrZK9/g=","Date":"Tue, 23 Jan 2024 16:43:27 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Subject":"Re: [libcamera-devel] [PATCH v2 05/18] libcamera: internal: Document\n\tthe SharedMemObject class","Message-ID":"<20240123144327.GD10679@pendragon.ideasonboard.com>","References":"<20240113142218.28063-1-hdegoede@redhat.com>\n\t<20240113142218.28063-6-hdegoede@redhat.com>\n\t<170518591472.3418233.17233321911691749297@ping.linuxembedded.co.uk>\n\t<20240123133453.GD32612@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20240123133453.GD32612@pendragon.ideasonboard.com>","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":"Maxime Ripard , g.martti@gmail.com,\n\tt.langendam@gmail.com, libcamera-devel@lists.libcamera.org,\n\tsrinivas.kandagatla@linaro.org, Pavel Machek ,\n\tBryan O'Donoghue , admin@dennisbonke.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28621,"web_url":"https://patchwork.libcamera.org/comment/28621/","msgid":"","date":"2024-01-24T11:07:35","subject":"Re: [libcamera-devel] [PATCH v2 05/18] libcamera: internal: Document\n\tthe SharedMemObject class","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi all,\n\nOn Tue, 23 Jan 2024 at 14:43, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n>\n> On Tue, Jan 23, 2024 at 03:34:53PM +0200, Laurent Pinchart wrote:\n> > On Sat, Jan 13, 2024 at 10:45:14PM +0000, -dev wrote:\n> > > Quoting Hans de Goede via libcamera-devel (2024-01-13 14:22:05)\n> > > > From: Dennis Bonke \n> > > >\n> > > > Document the SharedMemObject class.\n> > > >\n> > > > Signed-off-by: Dennis Bonke \n> > > > Signed-off-by: Hans de Goede \n> > > > Tested-by: Bryan O'Donoghue # sc8280xp\nLenovo x13s\n> > > > Tested-by: Pavel Machek \n> > > > ---\n> > > > .../libcamera/internal/shared_mem_object.h | 53\n+++++++++++++++++++\n> > > > 1 file changed, 53 insertions(+)\n> > > >\n> > > > diff --git a/include/libcamera/internal/shared_mem_object.h\nb/include/libcamera/internal/shared_mem_object.h\n> > > > index bfb639ee..e862ce48 100644\n> > > > --- a/include/libcamera/internal/shared_mem_object.h\n> > > > +++ b/include/libcamera/internal/shared_mem_object.h\n> > > > @@ -19,10 +19,20 @@\n> > > >\n> > > > namespace libcamera {\n> > > >\n> > > > +/**\n> > > > + * \\class SharedMemObject\n> > > > + * \\brief Helper class for shared memory allocations.\n> > > > + *\n> > > > + * Takes a template T which is used to indicate the\n> > > > + * data type of the object stored.\n> > >\n> > > I'd wrap this to the usual 80 chars. Not critical though.\n> >\n> > Not critical, but no reason not to do so :-)\n> >\n> > > It might be nice to explain here what the class is doing, as the code\n> > > may not be visible to the reader.\n> > >\n> > > The only part I'd add is perhaps something like:\n> > >\n> > > \"\"\"\n> > > Memory is allocated and exposed as a SharedFD for use across IPC\n> > > boundaries.\n> > > \"\"\"\n> >\n> > I would like more documentation too, with an explanation of how this is\n> > supposed to be used.\n> >\n> > > But either way,\n> > >\n> > > Reviewed-by: Kieran Bingham \n> >\n> > Documentation goes to .cpp files. The documentation should also come in\n> > the same patch as the one that adds the class.\n>\n> Just to be clear, as there's no .cpp file yet, I think most of the\n> SharedMemObject constructor should go to a separate init(size_t size)\n> function moved to a .cpp file, as it doesn't otherwise depend on the\n> type T or arguments Args.\n>\n>\n> The way we create a single memfd for every SharedMemObject isn't very\n> nice, as it could cause lots of fd allocations if callers are not aware\n> that they should create a single large object instead. This should be at\n> the very least clearly explained in the documentation. Ideally the\n> class should be refactored to make it possible to allocate multiple\n> objects within a single shared memory segment, but that's out of scope\n> for this series.\n>\n> As for squashing documentation with the patch that introduces the class,\n> I hadn't realized that the class had simply be moved from the rpi code.\n> I'm OK keeping the two patches separate.\n\n\nAdmittedly I've not been following this series, so apologies if I get some\nof the details below wrong.\n\nThe RPi Pi implementation of SharedMemObject is exactly what we want as per\nLaurent's description above, i.e. a single FD per object instance allocated\nand constructed through \"placement new\". This is why the SharedMemObject\nconstructor takes the var-args list for the wrapped object constructor.\nThere are only ever 2 SharedMemObject instances (backend and frontend) at\nruntime shared between the IPA and pipeline handler.\n\nI agree with Laurent that this same mechanism may not be entirely\nappropriate if there are a large number of objects being allocated in\nshared memory, resulting in a large number of FDs. In such cases, perhaps\nSharedMemObject() is not the right tool, and the underlying memfd\nallocations may need to be wrapped differently. Or maybe SharedMemObject\ncan be used as-is, but the object it allocates is a superset of many\nsmaller sub-objects...?\n\nRegards,\nNaush\n\n>\n> > > > + */\n> > > > template\n> > > > class SharedMemObject\n> > > > {\n> > > > public:\n> > > > + /**\n> > > > + * \\brief The size of the object that is going to be stored\nhere.\n> > > > + */\n> > > > static constexpr std::size_t SIZE = sizeof(T);\n>\n> We don't use all uppercase constants. You can name this kSize or size.\n>\n> > > >\n> > > > SharedMemObject()\n> > > > @@ -30,6 +40,11 @@ public:\n> > > > {\n> > > > }\n> > > >\n> > > > + /**\n> > > > + * \\brief Contstructor for the SharedMemObject.\n> > > > + * \\param[in] name The requested name.\n> > > > + * \\param[in] args Any additional args.\n> > > > + */\n>\n> You need to expand the documentation. Consider the point of view of a\n> reader who hasn't seen the code. They can't tell from the documentation\n> what the name and args are for.\n>\n> Also, I'd like unit tests.\n>\n> > > > template\n> > > > SharedMemObject(const std::string &name, Args &&...args)\n> > > > : name_(name), obj_(nullptr)\n> > > > @@ -57,6 +72,10 @@ public:\n> > > > obj_ = new (mem) T(std::forward(args)...);\n> > > > }\n> > > >\n> > > > + /**\n> > > > + * \\brief Move constructor for SharedMemObject.\n> > > > + * \\param[in] rhs The object to move.\n> > > > + */\n> > > > SharedMemObject(SharedMemObject &&rhs)\n> > > > {\n> > > > this->name_ = std::move(rhs.name_);\n> > > > @@ -76,6 +95,10 @@ public:\n> > > > /* Make SharedMemObject non-copyable for now. */\n> > > > LIBCAMERA_DISABLE_COPY(SharedMemObject)\n> > > >\n> > > > + /**\n> > > > + * \\brief Operator= for SharedMemObject.\n> > > > + * \\param[in] rhs The SharedMemObject object to take the\ndata from.\n> > > > + */\n> > > > SharedMemObject &operator=(SharedMemObject &&rhs)\n> > > > {\n> > > > this->name_ = std::move(rhs.name_);\n> > > > @@ -85,31 +108,61 @@ public:\n> > > > return *this;\n> > > > }\n> > > >\n> > > > + /**\n> > > > + * \\brief Operator-> for SharedMemObject.\n> > > > + *\n> > > > + * \\return the object.\n> > > > + */\n> > > > T *operator->()\n> > > > {\n> > > > return obj_;\n> > > > }\n> > > >\n> > > > + /**\n> > > > + * \\brief Operator-> for SharedMemObject.\n> > > > + *\n> > > > + * \\return the object.\n> > > > + */\n> > > > const T *operator->() const\n> > > > {\n> > > > return obj_;\n> > > > }\n> > > >\n> > > > + /**\n> > > > + * \\brief Operator* for SharedMemObject.\n> > > > + *\n> > > > + * \\return the object.\n> > > > + */\n> > > > T &operator*()\n> > > > {\n> > > > return *obj_;\n> > > > }\n> > > >\n> > > > + /**\n> > > > + * \\brief Operator* for SharedMemObject.\n> > > > + *\n> > > > + * \\return the object.\n> > > > + */\n> > > > const T &operator*() const\n> > > > {\n> > > > return *obj_;\n> > > > }\n> > > >\n> > > > + /**\n> > > > + * \\brief Gets the file descriptor for the underlaying\nstorage file.\n> > > > + *\n> > > > + * \\return the file descriptor.\n> > > > + */\n> > > > const SharedFD &fd() const\n> > > > {\n> > > > return fd_;\n> > > > }\n> > > >\n> > > > + /**\n> > > > + * \\brief Operator bool() for SharedMemObject.\n> > > > + *\n> > > > + * \\return true if the object is not null, false otherwise.\n> > > > + */\n> > > > explicit operator bool() const\n> > > > {\n> > > > return !!obj_;\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 2BC83BDC71\n\tfor ;\n\tWed, 24 Jan 2024 11:07:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1F3FB6294D;\n\tWed, 24 Jan 2024 12:07:44 +0100 (CET)","from mail-yb1-xb30.google.com (mail-yb1-xb30.google.com\n\t[IPv6:2607:f8b0:4864:20::b30])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EF3C461D30\n\tfor ;\n\tWed, 24 Jan 2024 12:07:42 +0100 (CET)","by mail-yb1-xb30.google.com with SMTP id\n\t3f1490d57ef6-dbe344a6cf4so4724573276.0\n\tfor ;\n\tWed, 24 Jan 2024 03:07:42 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"sOeXBiLP\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1706094462; x=1706699262;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=3dMx4sXrG3WQvgcSyuN6scWl7VJyz0lQL3ILau210wo=;\n\tb=sOeXBiLP3lJ5LY9C2e98fO/o4BkC0hIVNVd6NJ3AIh2pdHoU/QRDvVDGDk4ePGs2AV\n\tEkL+ij+F/r+ANHsGeiAapXLmAibzuopjexI1ryY9V0bQzM4eiqxVWuldSX4/I7LNsigB\n\tBPP9C/++p8WG9X3+tZAMh5tL9YfJ0rKTIo4FEukGKBSoDqVWbi4sVlLl+lkUpa748H5a\n\tc23NFGIT2H9LtTnEVc+7oah0d8IUWhmF7Hko7p4pi2i069NRcw2yOSc68TZ/GUrIOYBx\n\t73oVWpYlu1p7Lp2DVsUZ2PznR38aqaprRTIDEV/y4UKeXnvDekEp6UkWZZhFxfcdrr+Q\n\tHexQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1706094462; x=1706699262;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=3dMx4sXrG3WQvgcSyuN6scWl7VJyz0lQL3ILau210wo=;\n\tb=VDcwjxQNxDkIMIdNMgU0Am8yk8MgRc1PohJHgd+uEWhHBh6GZeafy8qJKRZ0Anz7Mf\n\tRpMEy/y/aZiVJeol7iwMu2zHetdzHRAeC/4PT89/3sLTqigiC69P6jRDyGXT6I0BExih\n\t2bAJ/oPfcoNHarVevufTf34Ka0QrDEblhxAd6uBddThihKuOOTK2pCCHsAb6x79bY7gQ\n\tOovvuYmPY0aoW4t5Zb8eXJInVxQ6ySMxB5HM3fHYaNdSJDrqyLT4Gl5WVHIxGKIxnT3J\n\te0pvcAjtGlXOJqf1tjrnqIS5b/Zx79fDPUsFaT30R4AVr1TYcnC5gpQpcL33VxCnHhHt\n\tsP1Q==","X-Gm-Message-State":"AOJu0YyGTj5BB2jYqrwOtwd4aKXsj/GG3ljVJCWVC7Bsa+jTLmqzWPFP\n\tDliPWGzO6sq4tfDC+/CMvkBiwP++evuUa4IEcvNicqZHo/NldSe232AvIzdw9v/82PLFsFTOJJ3\n\tG0H/lDoPfkIFf3XufEgB7P7KchZ0MXGJNcIQzZA==","X-Google-Smtp-Source":"AGHT+IFtZloKJPwoCsx4/wPArXFD0k3MZXT8gx3/kJeV15xRZbsw1fRweoQ9ykcXs8KNUqzn93l/2AGiMH1GuAg9ThM=","X-Received":"by 2002:a25:be50:0:b0:dc3:7305:549f with SMTP id\n\td16-20020a25be50000000b00dc37305549fmr480513ybm.16.1706094461703;\n\tWed, 24 Jan 2024 03:07:41 -0800 (PST)","MIME-Version":"1.0","References":"<20240113142218.28063-1-hdegoede@redhat.com>\n\t<20240113142218.28063-6-hdegoede@redhat.com>\n\t<170518591472.3418233.17233321911691749297@ping.linuxembedded.co.uk>\n\t<20240123133453.GD32612@pendragon.ideasonboard.com>\n\t<20240123144327.GD10679@pendragon.ideasonboard.com>","In-Reply-To":"<20240123144327.GD10679@pendragon.ideasonboard.com>","From":"Naushir Patuck ","Date":"Wed, 24 Jan 2024 11:07:35 +0000","Message-ID":"","Subject":"Re: [libcamera-devel] [PATCH v2 05/18] libcamera: internal: Document\n\tthe SharedMemObject class","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"00000000000057ead8060faf123f\"","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":"Maxime Ripard , g.martti@gmail.com,\n\tt.langendam@gmail.com, libcamera-devel@lists.libcamera.org,\n\tsrinivas.kandagatla@linaro.org, Pavel Machek ,\n\tBryan O'Donoghue , admin@dennisbonke.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28637,"web_url":"https://patchwork.libcamera.org/comment/28637/","msgid":"<20240205141614.GA2091@pendragon.ideasonboard.com>","date":"2024-02-05T14:16:14","subject":"Re: [libcamera-devel] [PATCH v2 05/18] libcamera: internal: Document\n\tthe SharedMemObject class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Wed, Jan 24, 2024 at 11:07:35AM +0000, Naushir Patuck wrote:\n> On Tue, 23 Jan 2024 at 14:43, Laurent Pinchart wrote:\n> > On Tue, Jan 23, 2024 at 03:34:53PM +0200, Laurent Pinchart wrote:\n> > > On Sat, Jan 13, 2024 at 10:45:14PM +0000, -dev wrote:\n> > > > Quoting Hans de Goede via libcamera-devel (2024-01-13 14:22:05)\n> > > > > From: Dennis Bonke \n> > > > >\n> > > > > Document the SharedMemObject class.\n> > > > >\n> > > > > Signed-off-by: Dennis Bonke \n> > > > > Signed-off-by: Hans de Goede \n> > > > > Tested-by: Bryan O'Donoghue # sc8280xp Lenovo x13s\n> > > > > Tested-by: Pavel Machek \n> > > > > ---\n> > > > >  .../libcamera/internal/shared_mem_object.h    | 53 +++++++++++++++++++\n> > > > >  1 file changed, 53 insertions(+)\n> > > > >\n> > > > > diff --git a/include/libcamera/internal/shared_mem_object.h b/include/libcamera/internal/shared_mem_object.h\n> > > > > index bfb639ee..e862ce48 100644\n> > > > > --- a/include/libcamera/internal/shared_mem_object.h\n> > > > > +++ b/include/libcamera/internal/shared_mem_object.h\n> > > > > @@ -19,10 +19,20 @@\n> > > > >\n> > > > >  namespace libcamera {\n> > > > >\n> > > > > +/**\n> > > > > + * \\class SharedMemObject\n> > > > > + * \\brief Helper class for shared memory allocations.\n> > > > > + *\n> > > > > + * Takes a template T which is used to indicate the\n> > > > > + * data type of the object stored.\n> > > >\n> > > > I'd wrap this to the usual 80 chars. Not critical though.\n> > >\n> > > Not critical, but no reason not to do so :-)\n> > >\n> > > > It might be nice to explain here what the class is doing, as the code\n> > > > may not be visible to the reader.\n> > > >\n> > > > The only part I'd add is perhaps something like:\n> > > >\n> > > > \"\"\"\n> > > > Memory is allocated and exposed as a SharedFD for use across IPC\n> > > > boundaries.\n> > > > \"\"\"\n> > >\n> > > I would like more documentation too, with an explanation of how this is\n> > > supposed to be used.\n> > >\n> > > > But either way,\n> > > >\n> > > > Reviewed-by: Kieran Bingham \n> > >\n> > > Documentation goes to .cpp files. The documentation should also come in\n> > > the same patch as the one that adds the class.\n> >\n> > Just to be clear, as there's no .cpp file yet, I think most of the\n> > SharedMemObject constructor should go to a separate init(size_t size)\n> > function moved to a .cpp file, as it doesn't otherwise depend on the\n> > type T or arguments Args.\n> >\n> > The way we create a single memfd for every SharedMemObject isn't very\n> > nice, as it could cause lots of fd allocations if callers are not aware\n> > that they should create a single large object instead. This should be at\n> > the very least clearly explained in the documentation. Ideally the\n> > class should be refactored to make it possible to allocate multiple\n> > objects within a single shared memory segment, but that's out of scope\n> > for this series.\n> >\n> > As for squashing documentation with the patch that introduces the class,\n> > I hadn't realized that the class had simply be moved from the rpi code.\n> > I'm OK keeping the two patches separate.\n> \n> Admittedly I've not been following this series, so apologies if I get some of\n> the details below wrong.\n> \n> The RPi Pi implementation of SharedMemObject is exactly what we want as per\n> Laurent's description above, i.e. a single FD per object instance allocated and\n> constructed through \"placement new\".  This is why the SharedMemObject\n> constructor takes the var-args list for the wrapped object constructor.  There\n> are only ever 2 SharedMemObject instances (backend and frontend) at runtime\n> shared between the IPA and pipeline handler.\n> \n> I agree with Laurent that this same mechanism may not be entirely appropriate\n> if there are a large number of objects being allocated in shared memory,\n> resulting in a large number of FDs.  In such cases, perhaps SharedMemObject()\n> is not the right tool, and the underlying memfd allocations may need to be\n> wrapped differently.  Or maybe SharedMemObject can be used as-is, but the\n> object it allocates is a superset of many smaller sub-objects...?\n\nI agree with all that. We may rework the implementation, but from a\nRaspberry Pi point of view, it will have to stay two objects with one fd\neach, with an easy to use API.\n\n> > > > > + */\n> > > > >  template\n> > > > >  class SharedMemObject\n> > > > >  {\n> > > > >  public:\n> > > > > +       /**\n> > > > > +        * \\brief The size of the object that is going to be stored here.\n> > > > > +        */\n> > > > >         static constexpr std::size_t SIZE = sizeof(T);\n> >\n> > We don't use all uppercase constants. You can name this kSize or size.\n> >\n> > > > >\n> > > > >         SharedMemObject()\n> > > > > @@ -30,6 +40,11 @@ public:\n> > > > >         {\n> > > > >         }\n> > > > >\n> > > > > +       /**\n> > > > > +        * \\brief Contstructor for the SharedMemObject.\n> > > > > +        * \\param[in] name The requested name.\n> > > > > +        * \\param[in] args Any additional args.\n> > > > > +        */\n> >\n> > You need to expand the documentation. Consider the point of view of a\n> > reader who hasn't seen the code. They can't tell from the documentation\n> > what the name and args are for.\n> >\n> > Also, I'd like unit tests.\n> >\n> > > > >         template\n> > > > >         SharedMemObject(const std::string &name, Args &&...args)\n> > > > >                 : name_(name), obj_(nullptr)\n> > > > > @@ -57,6 +72,10 @@ public:\n> > > > >                 obj_ = new (mem) T(std::forward(args)...);\n> > > > >         }\n> > > > >\n> > > > > +       /**\n> > > > > +        * \\brief Move constructor for SharedMemObject.\n> > > > > +        * \\param[in] rhs The object to move.\n> > > > > +        */\n> > > > >         SharedMemObject(SharedMemObject &&rhs)\n> > > > >         {\n> > > > >                 this->name_ = std::move(rhs.name_);\n> > > > > @@ -76,6 +95,10 @@ public:\n> > > > >         /* Make SharedMemObject non-copyable for now. */\n> > > > >         LIBCAMERA_DISABLE_COPY(SharedMemObject)\n> > > > >\n> > > > > +       /**\n> > > > > +        * \\brief Operator= for SharedMemObject.\n> > > > > +        * \\param[in] rhs The SharedMemObject object to take the data from.\n> > > > > +        */\n> > > > >         SharedMemObject &operator=(SharedMemObject &&rhs)\n> > > > >         {\n> > > > >                 this->name_ = std::move(rhs.name_);\n> > > > > @@ -85,31 +108,61 @@ public:\n> > > > >                 return *this;\n> > > > >         }\n> > > > >\n> > > > > +       /**\n> > > > > +        * \\brief Operator-> for SharedMemObject.\n> > > > > +        *\n> > > > > +        * \\return the object.\n> > > > > +        */\n> > > > >         T *operator->()\n> > > > >         {\n> > > > >                 return obj_;\n> > > > >         }\n> > > > >\n> > > > > +       /**\n> > > > > +        * \\brief Operator-> for SharedMemObject.\n> > > > > +        *\n> > > > > +        * \\return the object.\n> > > > > +        */\n> > > > >         const T *operator->() const\n> > > > >         {\n> > > > >                 return obj_;\n> > > > >         }\n> > > > >\n> > > > > +       /**\n> > > > > +        * \\brief Operator* for SharedMemObject.\n> > > > > +        *\n> > > > > +        * \\return the object.\n> > > > > +        */\n> > > > >         T &operator*()\n> > > > >         {\n> > > > >                 return *obj_;\n> > > > >         }\n> > > > >\n> > > > > +       /**\n> > > > > +        * \\brief Operator* for SharedMemObject.\n> > > > > +        *\n> > > > > +        * \\return the object.\n> > > > > +        */\n> > > > >         const T &operator*() const\n> > > > >         {\n> > > > >                 return *obj_;\n> > > > >         }\n> > > > >\n> > > > > +       /**\n> > > > > +        * \\brief Gets the file descriptor for the underlaying storage file.\n> > > > > +        *\n> > > > > +        * \\return the file descriptor.\n> > > > > +        */\n> > > > >         const SharedFD &fd() const\n> > > > >         {\n> > > > >                 return fd_;\n> > > > >         }\n> > > > >\n> > > > > +       /**\n> > > > > +        * \\brief Operator bool() for SharedMemObject.\n> > > > > +        *\n> > > > > +        * \\return true if the object is not null, false otherwise.\n> > > > > +        */\n> > > > >         explicit operator bool() const\n> > > > >         {\n> > > > >                 return !!obj_;","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 9C6ABBDB13\n\tfor ;\n\tMon, 5 Feb 2024 14:16:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id ECC2862805;\n\tMon, 5 Feb 2024 15:16:14 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 37C6961D0F\n\tfor ;\n\tMon, 5 Feb 2024 15:16:13 +0100 (CET)","from pendragon.ideasonboard.com\n\t(99.141-128-109.adsl-dyn.isp.belgacom.be [109.128.141.99])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 34BFD2B3;\n\tMon, 5 Feb 2024 15:14:50 +0100 (CET)"],"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=\"pWcFqe2r\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1707142490;\n\tbh=5O3rg7Jk9jaAyJBEn7F78xYwCAJxTTA6Qfdrc9s7Jfs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=pWcFqe2rgtd/WXhS/uRyYszYXBUAwfGvWRBuNg6FjzuJJDDI16g75NP1neYGKHZFR\n\tjEtXJgeGSQe6LVH0lv4C+14qtcpX3Rbe0wLhPXWUukxOmtxEMRXuMXwLu02DuYiarc\n\tdFc/erJUv0DOTG1HVs5w0IJIs0tWzUYyme3MOhZo=","Date":"Mon, 5 Feb 2024 16:16:14 +0200","From":"Laurent Pinchart ","To":"Naushir Patuck ","Subject":"Re: [libcamera-devel] [PATCH v2 05/18] libcamera: internal: Document\n\tthe SharedMemObject class","Message-ID":"<20240205141614.GA2091@pendragon.ideasonboard.com>","References":"<20240113142218.28063-1-hdegoede@redhat.com>\n\t<20240113142218.28063-6-hdegoede@redhat.com>\n\t<170518591472.3418233.17233321911691749297@ping.linuxembedded.co.uk>\n\t<20240123133453.GD32612@pendragon.ideasonboard.com>\n\t<20240123144327.GD10679@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","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":"Maxime Ripard , g.martti@gmail.com,\n\tt.langendam@gmail.com, libcamera-devel@lists.libcamera.org,\n\tsrinivas.kandagatla@linaro.org, Pavel Machek ,\n\tBryan O'Donoghue , admin@dennisbonke.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]