[{"id":27940,"web_url":"https://patchwork.libcamera.org/comment/27940/","msgid":"","date":"2023-10-12T08:08:37","subject":"Re: [libcamera-devel] [PATCH 03/20] pipeline: rpi: Add\n\tSharedMemObject class","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Cc-ing Andrey, Bryan and Hans for the SoftISP work\n\nOn Fri, Oct 06, 2023 at 02:19:43PM +0100, Naushir Patuck via libcamera-devel wrote:\n> Add new SharedMemObject class that wraps a memfd memory allocation and\n> constructs a templated object in the memory. With appropriate locking,\n> this object can then be shared across different processes using the\n> associated allocation file handle.\n>\n> Signed-off-by: Naushir Patuck \n> Reviewed-by: David Plowman \n> ---\n> .../pipeline/rpi/common/shared_mem_object.h | 118 ++++++++++++++++++\n> 1 file changed, 118 insertions(+)\n> create mode 100644 src/libcamera/pipeline/rpi/common/shared_mem_object.h\n>\n> diff --git a/src/libcamera/pipeline/rpi/common/shared_mem_object.h b/src/libcamera/pipeline/rpi/common/shared_mem_object.h\n> new file mode 100644\n> index 000000000000..69051ecc3f52\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/rpi/common/shared_mem_object.h\n> @@ -0,0 +1,118 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2023, Raspberry Pi Ltd\n> + *\n> + * shared_mem_object.h - Helper class for shared memory allocations\n> + */\n> +#pragma once\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n\n#include for std::forward\n\n> +\n> +#include \n> +#include \n> +\n> +namespace libcamera {\n> +\n> +namespace RPi {\n> +\n> +template\n> +class SharedMemObject\n> +{\n> +public:\n> +\tstatic constexpr std::size_t SIZE = sizeof(T);\n\nand for std::size_t ?\n\n> +\n> +\tSharedMemObject()\n> +\t\t: obj_(nullptr)\n> +\t{\n> +\t}\n> +\n> +\ttemplate\n> +\tSharedMemObject(const std::string &name, Args &&...args)\n> +\t\t: name_(name), obj_(nullptr)\n> +\t{\n> +\t\tvoid *mem;\n> +\t\tint ret;\n> +\n> +\t\tret = memfd_create(name_.c_str(), MFD_CLOEXEC);\n> +\t\tif (ret < 0)\n> +\t\t\treturn;\n> +\n> +\t\tfd_ = SharedFD(std::move(ret));\n> +\t\tif (!fd_.isValid())\n> +\t\t\treturn;\n> +\n> +\t\tret = ftruncate(fd_.get(), SIZE);\n> +\t\tif (ret < 0)\n> +\t\t\treturn;\n> +\n> +\t\tmem = mmap(nullptr, SIZE, PROT_READ | PROT_WRITE, MAP_SHARED,\n> +\t\t\t fd_.get(), 0);\n> +\t\tif (mem == MAP_FAILED)\n> +\t\t\treturn;\n> +\n> +\t\tobj_ = new (mem) T(std::forward(args)...);\n> +\t}\n> +\n> +\t~SharedMemObject()\n> +\t{\n> +\t\tif (obj_) {\n> +\t\t\tobj_->~T();\n> +\t\t\tmunmap(obj_, SIZE);\n> +\t\t}\n> +\t}\n> +\n> +\t/* Make SharedMemObject non-copyable for now. */\n> +\tLIBCAMERA_DISABLE_COPY(SharedMemObject)\n> +\n> +\tSharedMemObject &operator=(SharedMemObject &&rhs)\n\nDo you need a move constructor as well, since you have a move\nassignment operator ?\n\n\tSharedMemObject(SharedMemObject &&rhs)\n\t{\n\t\tthis->name_ = std::move(rhs.name_);\n\t\tthis->fd_ = std::move(rhs.fd_);\n\t\tthis->obj_ = rhs.obj_;\n\t\trhs.obj_ = nullptr;\n\t}\n\n\n> +\t\tthis->name_ = std::move(rhs.name_);\n> +\t\tthis->fd_ = std::move(rhs.fd_);\n> +\t\tthis->obj_ = rhs.obj_;\n> +\t\trhs.obj_ = nullptr;\n> +\t\treturn *this;\n> +\t}\n> +\n> +\tT *operator->()\n> +\t{\n> +\t\treturn obj_;\n> +\t}\n> +\n> +\tconst T *operator->() const\n> +\t{\n> +\t\treturn obj_;\n> +\t}\n> +\n> +\tT &operator*()\n> +\t{\n> +\t\treturn *obj_;\n> +\t}\n> +\n> +\tconst T &operator*() const\n> +\t{\n> +\t\treturn *obj_;\n> +\t}\n> +\n> +\tconst SharedFD &fd() const\n> +\t{\n> +\t\treturn fd_;\n> +\t}\n> +\n> +\texplicit operator bool() const\n> +\t{\n> +\t\treturn !!obj_;\n> +\t}\n> +\n> +private:\n> +\tstd::string name_;\n> +\tSharedFD fd_;\n> +\tT *obj_;\n> +};\n\nThe rest looks good.. making this a library component on top shouldn't be\nhard..\n\nReviewed-by: Jacopo Mondi \n\n> +\n> +} /* namespace RPi */\n> +\n> +} /* namespace libcamera */\n> --\n> 2.34.1\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 7FA91C3272\n\tfor ;\n\tThu, 12 Oct 2023 08:08:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F38396297F;\n\tThu, 12 Oct 2023 10:08:41 +0200 (CEST)","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 35C8962962\n\tfor ;\n\tThu, 12 Oct 2023 10:08:41 +0200 (CEST)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 074AB583;\n\tThu, 12 Oct 2023 10:08:37 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1697098122;\n\tbh=uqL47Suh/RpqnAB3exqqIk79G0Bxs90PvYqK9e4gbGY=;\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=A52rGeNvCYAo1vFsY7VFSZnPZhFLM4jxeHQ5q/YQojYUWkE+X2FuoucvIJ0KUdjmr\n\tk6seqGbW5f9ytUIEz/au1PP6i8KJDrRMgMm3yA63cH4bnbWbYbJffVPo/tgig2lLMb\n\tacWYctmgEjZOl2cEJ4R3r4L4KOCfeW9z1MAvr869SypknPNUB5URoy89RZhmdTFWIp\n\t0iZ1juGs6ThrBU2z7M0aE3TLF3za6tfiD7h+5Ff9tm9vT+UPQTrKm+o/rx552bnUmr\n\t+XfsnEVCgKyse/36Ym9+klDWEKzexmhOTuYra6t2c9ozSL7W60X+qGRDgGX4owa5OY\n\tV9WVxcDetzviA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1697098118;\n\tbh=uqL47Suh/RpqnAB3exqqIk79G0Bxs90PvYqK9e4gbGY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=nn20erZoPpkiuHk1B04JylR5lMjm3VQXeuenE+by1ZKE+WzGPAxsmujOkdO5jR+8U\n\t48rYhYPv5u1eV7kAbYobZ/lI8DaN9I2vSQGs/Bj8KpenUG1qCSgq1LI9n+xamoibpp\n\tMJNa6ZcmNtB4AEXOJzfydoZ8haUqx3rKC8+QQ3bc="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"nn20erZo\"; dkim-atps=neutral","Date":"Thu, 12 Oct 2023 10:08:37 +0200","To":"Naushir Patuck ","Message-ID":"","References":"<20231006132000.23504-1-naush@raspberrypi.com>\n\t<20231006132000.23504-4-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20231006132000.23504-4-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH 03/20] pipeline: rpi: Add\n\tSharedMemObject 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":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"bryan.odonoghue@linaro.org, libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27944,"web_url":"https://patchwork.libcamera.org/comment/27944/","msgid":"","date":"2023-10-12T08:25:13","subject":"Re: [libcamera-devel] [PATCH 03/20] pipeline: rpi: Add\n\tSharedMemObject class","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Jacopo,\n\nThank you for the feedback.\n\nOn Thu, 12 Oct 2023 at 09:08, Jacopo Mondi\n wrote:\n>\n> Cc-ing Andrey, Bryan and Hans for the SoftISP work\n>\n> On Fri, Oct 06, 2023 at 02:19:43PM +0100, Naushir Patuck via libcamera-devel wrote:\n> > Add new SharedMemObject class that wraps a memfd memory allocation and\n> > constructs a templated object in the memory. With appropriate locking,\n> > this object can then be shared across different processes using the\n> > associated allocation file handle.\n> >\n> > Signed-off-by: Naushir Patuck \n> > Reviewed-by: David Plowman \n> > ---\n> > .../pipeline/rpi/common/shared_mem_object.h | 118 ++++++++++++++++++\n> > 1 file changed, 118 insertions(+)\n> > create mode 100644 src/libcamera/pipeline/rpi/common/shared_mem_object.h\n> >\n> > diff --git a/src/libcamera/pipeline/rpi/common/shared_mem_object.h b/src/libcamera/pipeline/rpi/common/shared_mem_object.h\n> > new file mode 100644\n> > index 000000000000..69051ecc3f52\n> > --- /dev/null\n> > +++ b/src/libcamera/pipeline/rpi/common/shared_mem_object.h\n> > @@ -0,0 +1,118 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2023, Raspberry Pi Ltd\n> > + *\n> > + * shared_mem_object.h - Helper class for shared memory allocations\n> > + */\n> > +#pragma once\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n>\n> #include for std::forward\n>\n> > +\n> > +#include \n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +namespace RPi {\n> > +\n> > +template\n> > +class SharedMemObject\n> > +{\n> > +public:\n> > + static constexpr std::size_t SIZE = sizeof(T);\n>\n> and for std::size_t ?\n\nAck on both.\n\n>\n> > +\n> > + SharedMemObject()\n> > + : obj_(nullptr)\n> > + {\n> > + }\n> > +\n> > + template\n> > + SharedMemObject(const std::string &name, Args &&...args)\n> > + : name_(name), obj_(nullptr)\n> > + {\n> > + void *mem;\n> > + int ret;\n> > +\n> > + ret = memfd_create(name_.c_str(), MFD_CLOEXEC);\n> > + if (ret < 0)\n> > + return;\n> > +\n> > + fd_ = SharedFD(std::move(ret));\n> > + if (!fd_.isValid())\n> > + return;\n> > +\n> > + ret = ftruncate(fd_.get(), SIZE);\n> > + if (ret < 0)\n> > + return;\n> > +\n> > + mem = mmap(nullptr, SIZE, PROT_READ | PROT_WRITE, MAP_SHARED,\n> > + fd_.get(), 0);\n> > + if (mem == MAP_FAILED)\n> > + return;\n> > +\n> > + obj_ = new (mem) T(std::forward(args)...);\n> > + }\n> > +\n> > + ~SharedMemObject()\n> > + {\n> > + if (obj_) {\n> > + obj_->~T();\n> > + munmap(obj_, SIZE);\n> > + }\n> > + }\n> > +\n> > + /* Make SharedMemObject non-copyable for now. */\n> > + LIBCAMERA_DISABLE_COPY(SharedMemObject)\n> > +\n> > + SharedMemObject &operator=(SharedMemObject &&rhs)\n>\n> Do you need a move constructor as well, since you have a move\n> assignment operator ?\n>\n> SharedMemObject(SharedMemObject &&rhs)\n> {\n> this->name_ = std::move(rhs.name_);\n> this->fd_ = std::move(rhs.fd_);\n> this->obj_ = rhs.obj_;\n> rhs.obj_ = nullptr;\n> }\n\nYes, it seems reasonable to add this.\n\n>\n>\n> > + this->name_ = std::move(rhs.name_);\n> > + this->fd_ = std::move(rhs.fd_);\n> > + this->obj_ = rhs.obj_;\n> > + rhs.obj_ = nullptr;\n> > + return *this;\n> > + }\n> > +\n> > + T *operator->()\n> > + {\n> > + return obj_;\n> > + }\n> > +\n> > + const T *operator->() const\n> > + {\n> > + return obj_;\n> > + }\n> > +\n> > + T &operator*()\n> > + {\n> > + return *obj_;\n> > + }\n> > +\n> > + const T &operator*() const\n> > + {\n> > + return *obj_;\n> > + }\n> > +\n> > + const SharedFD &fd() const\n> > + {\n> > + return fd_;\n> > + }\n> > +\n> > + explicit operator bool() const\n> > + {\n> > + return !!obj_;\n> > + }\n> > +\n> > +private:\n> > + std::string name_;\n> > + SharedFD fd_;\n> > + T *obj_;\n> > +};\n>\n> The rest looks good.. making this a library component on top shouldn't be\n> hard..\n>\n> Reviewed-by: Jacopo Mondi \n\nThanks!\nI'll post an update once I've collected all the feedback.\n\nRegards,\nNaush\n\n>\n> > +\n> > +} /* namespace RPi */\n> > +\n> > +} /* namespace libcamera */\n> > --\n> > 2.34.1\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 D8405C3272\n\tfor ;\n\tThu, 12 Oct 2023 08:25:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4E3C66297C;\n\tThu, 12 Oct 2023 10:25:44 +0200 (CEST)","from mail-yb1-xb2b.google.com (mail-yb1-xb2b.google.com\n\t[IPv6:2607:f8b0:4864:20::b2b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7D7F56297B\n\tfor ;\n\tThu, 12 Oct 2023 10:25:42 +0200 (CEST)","by mail-yb1-xb2b.google.com with SMTP id\n\t3f1490d57ef6-d9a4c0d89f7so743298276.1\n\tfor ;\n\tThu, 12 Oct 2023 01:25:42 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1697099144;\n\tbh=E79eoGiY8mXnehAIQQQCRoZ7uFur9ryMKaKBfwqTxjE=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=NOAjB9mc8rbnOSkfmQsLp4dbhfB5c4oVnX6ToZgN4ieO8ofnhPcj3SSuliAPGdK8P\n\tYaMSOKO3Vnpp7++xRQIhE6U+7XoApmTN1FqkxLKLrfLBjku3FgXV9yzIJNIhtS1qXA\n\tWm1t+IRtZL0sDxuYWBqVEFFEH4AfNHZFZfddfiBzNTP6rychnqB8LtPbc+kv2JNohE\n\tyyBQ3zR2XpSke+jzJHPu4d5szWtk9NWDYXwbx+vRcxZ8Nszv5gEZGFK3VBYy3oK2l2\n\t8pc+skOI7cX+zz+r9zNr3shRtZ1fSR+HQyCyJVqyhQln6V3vQsf/OTvT7ABJlbfPQV\n\thb2i8Zxmss2PA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1697099141; x=1697703941;\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=WUjWj0iYv++RzxGYLedP5Y/ntiyO/fYOaoHPhzEblUk=;\n\tb=EM1O0I6tYtrFEjiuOjuV7BBLjUMQDND8F4ncgCGvb5HAa+fYg10aE6Z+WNip4/lFkH\n\t7MuN7sbRRtpdr8BdMbsQ6UpDByah2wXYQLLPp9difwL/oWyJtL0+Z48JgNxPSEPv3e0q\n\tLHfuQ298zggSVWOEoh8vrfZ/ipIBWW6ccql2GlsuZsM75a7nYrck6HD7dMWqPeXeqmnd\n\tqBzHmMWtbL0ktLKPjNsnnqNitNJJX5s9xqfroE0YYQb5T6agf4W0bDutydU7rkndFqHg\n\tVFTNMln6sf3HJlTjdP6Y8GXpx4iZLNZSUThYMOvGCg64B9ofwTFNHMUxpVUJRudXqtN8\n\tTefw=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"EM1O0I6t\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1697099141; x=1697703941;\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=WUjWj0iYv++RzxGYLedP5Y/ntiyO/fYOaoHPhzEblUk=;\n\tb=tmLxC2FYZPQQbXvCrT99nTM6p1MXTQeonaGjFr7q6s4Cse0YA27jlGFgZ+EG7ju3xL\n\tOnQCbfMztuUdpoDkVPXMkb50SaCFQjMNQomF+ZLexh8tc/d2m3F85msqaWBsYB6Sb5FP\n\tG25C7q76rTOtFPSmiPj2CsX1t8yi6hKthYeSi5+ftBfs4f2unJGClUh13Pdo9BvEj2pf\n\tphlPZTN4oPYJt9HTo9L57Xnoj+u+BFYWqmAZIgFDB5lVOqcA73a6Jes5fJPRYqBwqCOX\n\t3ta+d6DzK/mGz1TGubyxFV5+TUnPlyRXqMbsWrOTjPsdTnGSdSOIdmFOrvoUrYLJ/Kvi\n\twCuw==","X-Gm-Message-State":"AOJu0Yx7NeYYWOHjfSjN5QMSqlMzdSwErcKg7sIMaTw3GMGWADrAq3Nq\n\tbrEx6Gf59rqix/p1788XjYqOnGb3r4fhPFXDQFKmRQ==","X-Google-Smtp-Source":"AGHT+IEHAv1V8GQXKRbrPMZHy6Uxk145WpNRkLGNhV5Xe2u8+PcibU1Jbkjrx4MrPK0Tk5n3cIeBrs0GWFbOCDXT7JU=","X-Received":"by 2002:a25:4d07:0:b0:d85:df88:a7c4 with SMTP id\n\ta7-20020a254d07000000b00d85df88a7c4mr19805104ybb.18.1697099141217;\n\tThu, 12 Oct 2023 01:25:41 -0700 (PDT)","MIME-Version":"1.0","References":"<20231006132000.23504-1-naush@raspberrypi.com>\n\t<20231006132000.23504-4-naush@raspberrypi.com>\n\t","In-Reply-To":"","Date":"Thu, 12 Oct 2023 09:25:13 +0100","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 03/20] pipeline: rpi: Add\n\tSharedMemObject 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":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"bryan.odonoghue@linaro.org, libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]