[{"id":19016,"web_url":"https://patchwork.libcamera.org/comment/19016/","msgid":"","date":"2021-08-23T22:55:54","subject":"Re: [libcamera-devel] [RFC PATCH v2 05/10] ipa: rkisp1: Use offset\n\tin mapping IPABuffer","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nThank you for the patch.\n\nOn Mon, Aug 23, 2021 at 10:12:16PM +0900, Hirokazu Honda wrote:\n> IPABuffer is represented by FrameBuffer. FrameBuffer::Plane has\n> now an offset. This uses the offset variable to map the IPABuffer.\n\nThe commit message and subject line should be updated, this patch now\nmoves to MappedFrameBuffer.\n\n> Signed-off-by: Hirokazu Honda \n> ---\n> .../libcamera/internal/mapped_framebuffer.h | 1 +\n> src/ipa/rkisp1/rkisp1.cpp | 31 +++++++------------\n> src/libcamera/mapped_framebuffer.cpp | 7 +++++\n> 3 files changed, 19 insertions(+), 20 deletions(-)\n> \n> diff --git a/include/libcamera/internal/mapped_framebuffer.h b/include/libcamera/internal/mapped_framebuffer.h\n> index 42479541..ee0583d0 100644\n> --- a/include/libcamera/internal/mapped_framebuffer.h\n> +++ b/include/libcamera/internal/mapped_framebuffer.h\n> @@ -55,6 +55,7 @@ public:\n> \n> \tusing MapFlags = Flags;\n> \n> +\tMappedFrameBuffer();\n> \tMappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags);\n> };\n> \n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index 06fb9640..54cf2885 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -10,7 +10,6 @@\n> #include \n> #include \n> #include \n> -#include \n> \n> #include \n> #include \n> @@ -24,6 +23,8 @@\n> #include \n> #include \n> \n> +#include \n> +\n> namespace libcamera {\n> \n> LOG_DEFINE_CATEGORY(IPARkISP1)\n> @@ -54,7 +55,7 @@ private:\n> \tvoid metadataReady(unsigned int frame, unsigned int aeState);\n> \n> \tstd::map buffers_;\n> -\tstd::map buffersMemory_;\n> +\tstd::map mappedBuffers_;\n> \n> \tControlInfoMap ctrls_;\n> \n> @@ -160,21 +161,10 @@ void IPARkISP1::mapBuffers(const std::vector &buffers)\n> \t\t\t\t\t std::forward_as_tuple(buffer.planes));\n> \t\tconst FrameBuffer &fb = elem.first->second;\n> \n> -\t\t/*\n> -\t\t * \\todo Provide a helper to mmap() buffers (possibly exposed\n> -\t\t * to applications).\n> -\t\t */\n> -\t\tbuffersMemory_[buffer.id] = mmap(NULL,\n> -\t\t\t\t\t\t fb.planes()[0].length,\n> -\t\t\t\t\t\t PROT_READ | PROT_WRITE,\n> -\t\t\t\t\t\t MAP_SHARED,\n> -\t\t\t\t\t\t fb.planes()[0].fd.fd(),\n> -\t\t\t\t\t\t 0);\n> -\n> -\t\tif (buffersMemory_[buffer.id] == MAP_FAILED) {\n> -\t\t\tint ret = -errno;\n> +\t\tmappedBuffers_[buffer.id] = MappedFrameBuffer(&fb, MappedFrameBuffer::MapFlag::ReadWrite);\n\nWe could use emplace() here to avoid the need for a default constructor,\nbut we wouldn't be able to use operator[]() below, which isn't nice. I\nwonder if we could maybe store a std::unique_ptr in\nmappedBuffers_, to avoid the default constructor ?\n\nKieran, you've designed the MappedFrameBuffer class, what do you think ?\n\nApart from this,\n\nReviewed-by: Laurent Pinchart \n\n> +\t\tif (!mappedBuffers_[buffer.id].isValid()) {\n> \t\t\tLOG(IPARkISP1, Fatal) << \"Failed to mmap buffer: \"\n> -\t\t\t\t\t << strerror(-ret);\n> +\t\t\t\t\t << strerror(mappedBuffers_[buffer.id].error());\n> \t\t}\n> \t}\n> }\n> @@ -186,8 +176,7 @@ void IPARkISP1::unmapBuffers(const std::vector &ids)\n> \t\tif (fb == buffers_.end())\n> \t\t\tcontinue;\n> \n> -\t\tmunmap(buffersMemory_[id], fb->second.planes()[0].length);\n> -\t\tbuffersMemory_.erase(id);\n> +\t\tmappedBuffers_.erase(id);\n> \t\tbuffers_.erase(id);\n> \t}\n> }\n> @@ -200,7 +189,8 @@ void IPARkISP1::processEvent(const RkISP1Event &event)\n> \t\tunsigned int bufferId = event.bufferId;\n> \n> \t\tconst rkisp1_stat_buffer *stats =\n> -\t\t\tstatic_cast(buffersMemory_[bufferId]);\n> +\t\t\treinterpret_cast(\n> +\t\t\t\tmappedBuffers_[bufferId].maps()[0].data());\n> \n> \t\tupdateStatistics(frame, stats);\n> \t\tbreak;\n> @@ -210,7 +200,8 @@ void IPARkISP1::processEvent(const RkISP1Event &event)\n> \t\tunsigned int bufferId = event.bufferId;\n> \n> \t\trkisp1_params_cfg *params =\n> -\t\t\tstatic_cast(buffersMemory_[bufferId]);\n> +\t\t\treinterpret_cast(\n> +\t\t\t\tmappedBuffers_[bufferId].maps()[0].data());\n> \n> \t\tqueueRequest(frame, params, event.controls);\n> \t\tbreak;\n> diff --git a/src/libcamera/mapped_framebuffer.cpp b/src/libcamera/mapped_framebuffer.cpp\n> index 03425dea..34d9564d 100644\n> --- a/src/libcamera/mapped_framebuffer.cpp\n> +++ b/src/libcamera/mapped_framebuffer.cpp\n> @@ -168,6 +168,13 @@ MappedBuffer::~MappedBuffer()\n> * \\brief A bitwise combination of MappedFrameBuffer::MapFlag values\n> */\n> \n> +/**\n> + * \\brief Construct an empty MappedFrameBuffer\n> + */\n> +MappedFrameBuffer::MappedFrameBuffer()\n> +\t: MappedBuffer()\n> +{\n> +}\n> /**\n> * \\brief Map all planes of a FrameBuffer\n> * \\param[in] buffer FrameBuffer to be mapped","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 901EABD87D\n\tfor ;\n\tMon, 23 Aug 2021 22:56:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D9617688A3;\n\tTue, 24 Aug 2021 00:56:05 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6622868890\n\tfor ;\n\tTue, 24 Aug 2021 00:56:04 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CD1472A5;\n\tTue, 24 Aug 2021 00:56:03 +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=\"H9e324qF\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629759364;\n\tbh=zEe32GZp/wOsy+/IMqG0M5ORBeEF498Scn6vUliv070=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=H9e324qF1rDHie6gpLnERi/3ixm3R2WgRcSJwhYCNGx4dZB2aGg3gpv0FBSRJwBJI\n\te4twjfXXtFt6GGTWaBd1sff7JUir53GstyRPg6cFT4Lz6h+7/k5JkA/2FNSirNB3t4\n\tpDZ9YcmLvn6NPwC3LULNeoV9YAbEzmQv686J+eOA=","Date":"Tue, 24 Aug 2021 01:55:54 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210823131221.1034059-1-hiroh@chromium.org>\n\t<20210823131221.1034059-6-hiroh@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210823131221.1034059-6-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [RFC PATCH v2 05/10] ipa: rkisp1: Use offset\n\tin mapping IPABuffer","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19025,"web_url":"https://patchwork.libcamera.org/comment/19025/","msgid":"","date":"2021-08-24T12:44:15","subject":"Re: [libcamera-devel] [RFC PATCH v2 05/10] ipa: rkisp1: Use offset\n\tin mapping IPABuffer","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi,\n\nOn 23/08/2021 23:55, Laurent Pinchart wrote:\n> Hi Hiro,\n> \n> Thank you for the patch.\n> \n> On Mon, Aug 23, 2021 at 10:12:16PM +0900, Hirokazu Honda wrote:\n>> IPABuffer is represented by FrameBuffer. FrameBuffer::Plane has\n>> now an offset. This uses the offset variable to map the IPABuffer.\n> \n> The commit message and subject line should be updated, this patch now\n> moves to MappedFrameBuffer.\n> \n>> Signed-off-by: Hirokazu Honda \n>> ---\n>> .../libcamera/internal/mapped_framebuffer.h | 1 +\n>> src/ipa/rkisp1/rkisp1.cpp | 31 +++++++------------\n>> src/libcamera/mapped_framebuffer.cpp | 7 +++++\n>> 3 files changed, 19 insertions(+), 20 deletions(-)\n>>\n>> diff --git a/include/libcamera/internal/mapped_framebuffer.h b/include/libcamera/internal/mapped_framebuffer.h\n>> index 42479541..ee0583d0 100644\n>> --- a/include/libcamera/internal/mapped_framebuffer.h\n>> +++ b/include/libcamera/internal/mapped_framebuffer.h\n>> @@ -55,6 +55,7 @@ public:\n>> \n>> \tusing MapFlags = Flags;\n>> \n>> +\tMappedFrameBuffer();\n>> \tMappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags);\n>> };\n>> \n>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n>> index 06fb9640..54cf2885 100644\n>> --- a/src/ipa/rkisp1/rkisp1.cpp\n>> +++ b/src/ipa/rkisp1/rkisp1.cpp\n>> @@ -10,7 +10,6 @@\n>> #include \n>> #include \n>> #include \n>> -#include \n>> \n>> #include \n>> #include \n>> @@ -24,6 +23,8 @@\n>> #include \n>> #include \n>> \n>> +#include \n>> +\n>> namespace libcamera {\n>> \n>> LOG_DEFINE_CATEGORY(IPARkISP1)\n>> @@ -54,7 +55,7 @@ private:\n>> \tvoid metadataReady(unsigned int frame, unsigned int aeState);\n>> \n>> \tstd::map buffers_;\n>> -\tstd::map buffersMemory_;\n>> +\tstd::map mappedBuffers_;\n>> \n>> \tControlInfoMap ctrls_;\n>> \n>> @@ -160,21 +161,10 @@ void IPARkISP1::mapBuffers(const std::vector &buffers)\n>> \t\t\t\t\t std::forward_as_tuple(buffer.planes));\n>> \t\tconst FrameBuffer &fb = elem.first->second;\n>> \n>> -\t\t/*\n>> -\t\t * \\todo Provide a helper to mmap() buffers (possibly exposed\n>> -\t\t * to applications).\n>> -\t\t */\n>> -\t\tbuffersMemory_[buffer.id] = mmap(NULL,\n>> -\t\t\t\t\t\t fb.planes()[0].length,\n>> -\t\t\t\t\t\t PROT_READ | PROT_WRITE,\n>> -\t\t\t\t\t\t MAP_SHARED,\n>> -\t\t\t\t\t\t fb.planes()[0].fd.fd(),\n>> -\t\t\t\t\t\t 0);\n>> -\n>> -\t\tif (buffersMemory_[buffer.id] == MAP_FAILED) {\n>> -\t\t\tint ret = -errno;\n>> +\t\tmappedBuffers_[buffer.id] = MappedFrameBuffer(&fb, MappedFrameBuffer::MapFlag::ReadWrite);\n> \n> We could use emplace() here to avoid the need for a default constructor,\n> but we wouldn't be able to use operator[]() below, which isn't nice. I\n> wonder if we could maybe store a std::unique_ptr in\n> mappedBuffers_, to avoid the default constructor ?\n> \n> Kieran, you've designed the MappedFrameBuffer class, what do you think ?\n\nHrm, firstly the error_ flag needs to be fixed up if there's a default\nconstructor - mentioned below in the Constructor code.\n\nBut otherwise, even though we do have the isValid() check, I don't think\nwe have many users that actually check it.\n\nThe aim was to make it simple and easy to create the mapping, which is\nwhy it happens on construction.\n\nIf this is causing problems, we can separate construction and mapping...\nbut then I'd expect to have map()/unmap() as well.\n\n\nLooking at existing users of MappedFrameBuffer, I can see that:\n\n IPAVimc::mapBuffers() currently uses emplace, (with a\npiecewise_construct, to be able to specify the mapping flags as\nread-only), and only uses find() to get them.\n\n\nSo does IPU3. ...\n\nAnd unsurprisingly, so does RPi.\n\n\nHowever none of those implementations are checking the isValid() flag.\nSo perhaps they need to be updated to do so.\n\nAnd if we mandate that the isValid() should be checked, then it's not\nmuch different to separating construction and mapping...\n\n\nThe main goal for the current design is to allow other buffer types to\nbe constructed, so perhaps there could be a MappedDRMBuffer or\nMappedDMABuffer or such.\n\nAs long as that's kept in mind, I don't mind if the main interface\nchanges to suit needs as we determine them.\n\nWould using [], require checking the .isValid() flag at every usage?\n(Because we might be given a new empty mapping, if the search failed?)\n\nThat might become less convenient ... ?\n\n\n\n> Apart from this,\n> \n> Reviewed-by: Laurent Pinchart \n> \n>> +\t\tif (!mappedBuffers_[buffer.id].isValid()) {\n>> \t\t\tLOG(IPARkISP1, Fatal) << \"Failed to mmap buffer: \"\n>> -\t\t\t\t\t << strerror(-ret);\n>> +\t\t\t\t\t << strerror(mappedBuffers_[buffer.id].error());\n>> \t\t}\n>> \t}\n>> }\n>> @@ -186,8 +176,7 @@ void IPARkISP1::unmapBuffers(const std::vector &ids)\n>> \t\tif (fb == buffers_.end())\n>> \t\t\tcontinue;\n>> \n>> -\t\tmunmap(buffersMemory_[id], fb->second.planes()[0].length);\n>> -\t\tbuffersMemory_.erase(id);\n>> +\t\tmappedBuffers_.erase(id);\n>> \t\tbuffers_.erase(id);\n>> \t}\n>> }\n>> @@ -200,7 +189,8 @@ void IPARkISP1::processEvent(const RkISP1Event &event)\n>> \t\tunsigned int bufferId = event.bufferId;\n>> \n>> \t\tconst rkisp1_stat_buffer *stats =\n>> -\t\t\tstatic_cast(buffersMemory_[bufferId]);\n>> +\t\t\treinterpret_cast(\n>> +\t\t\t\tmappedBuffers_[bufferId].maps()[0].data());\n>> \n>> \t\tupdateStatistics(frame, stats);\n>> \t\tbreak;\n>> @@ -210,7 +200,8 @@ void IPARkISP1::processEvent(const RkISP1Event &event)\n>> \t\tunsigned int bufferId = event.bufferId;\n>> \n>> \t\trkisp1_params_cfg *params =\n>> -\t\t\tstatic_cast(buffersMemory_[bufferId]);\n>> +\t\t\treinterpret_cast(\n>> +\t\t\t\tmappedBuffers_[bufferId].maps()[0].data());\n>> \n>> \t\tqueueRequest(frame, params, event.controls);\n>> \t\tbreak;\n>> diff --git a/src/libcamera/mapped_framebuffer.cpp b/src/libcamera/mapped_framebuffer.cpp\n>> index 03425dea..34d9564d 100644\n>> --- a/src/libcamera/mapped_framebuffer.cpp\n>> +++ b/src/libcamera/mapped_framebuffer.cpp\n>> @@ -168,6 +168,13 @@ MappedBuffer::~MappedBuffer()\n>> * \\brief A bitwise combination of MappedFrameBuffer::MapFlag values\n>> */\n>> \n>> +/**\n>> + * \\brief Construct an empty MappedFrameBuffer\n>> + */\n>> +MappedFrameBuffer::MappedFrameBuffer()\n>> +\t: MappedBuffer()\n>> +{\n\nIf this is created, error_ would have to be set to an ... error here.\n\nPerhaps something like -ENOMEM, so that 'default constructed' empty\nmappings are not 'valid'.\n\nThat would then necessitate changine the ordering of how error_ is used,\ncurrently it's only set when an error occurs on mapping.\n\nIt would have to be set to 0 on successful mappings as well/instead.\n\n\n\n>> +}\n>> /**\n>> * \\brief Map all planes of a FrameBuffer\n>> * \\param[in] buffer FrameBuffer to be mapped\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 943E6BD87C\n\tfor ;\n\tTue, 24 Aug 2021 12:44:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DAD85688A3;\n\tTue, 24 Aug 2021 14:44:19 +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 EE49D60505\n\tfor ;\n\tTue, 24 Aug 2021 14:44:18 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2F5452A5;\n\tTue, 24 Aug 2021 14:44:18 +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=\"Ln/jNkR3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629809058;\n\tbh=4nna+59Zkycmwyv4JLAf3CZjl7+lKSAT8vwNnUZy0iU=;\n\th=From:To:Cc:References:Subject:Date:In-Reply-To:From;\n\tb=Ln/jNkR3W/7LHDxD5f7c4wGGlDIRJEOSVu33XrqKRqF7zQrAgeisYl3pzGgql88yK\n\tLc9L8fzy/hxRJmOccxRNOhtyueI7b/TvC7cH2DwhiJ2wFG9Llw6/2TzGGtVS02aiLY\n\tgqN8Pv+TmDqAk8z7WKEoDErQbkMlFFOdIY6vMStE=","From":"Kieran Bingham ","To":"Laurent Pinchart ,\n\tHirokazu Honda ","References":"<20210823131221.1034059-1-hiroh@chromium.org>\n\t<20210823131221.1034059-6-hiroh@chromium.org>\n\t","Message-ID":"","Date":"Tue, 24 Aug 2021 13:44:15 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [RFC PATCH v2 05/10] ipa: rkisp1: Use offset\n\tin mapping IPABuffer","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19033,"web_url":"https://patchwork.libcamera.org/comment/19033/","msgid":"","date":"2021-08-25T06:13:43","subject":"Re: [libcamera-devel] [RFC PATCH v2 05/10] ipa: rkisp1: Use offset\n\tin mapping IPABuffer","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent and Kieran, thank you for reviewing.\n\nOn Tue, Aug 24, 2021 at 9:44 PM Kieran Bingham\n wrote:\n>\n> Hi,\n>\n> On 23/08/2021 23:55, Laurent Pinchart wrote:\n> > Hi Hiro,\n> >\n> > Thank you for the patch.\n> >\n> > On Mon, Aug 23, 2021 at 10:12:16PM +0900, Hirokazu Honda wrote:\n> >> IPABuffer is represented by FrameBuffer. FrameBuffer::Plane has\n> >> now an offset. This uses the offset variable to map the IPABuffer.\n> >\n> > The commit message and subject line should be updated, this patch now\n> > moves to MappedFrameBuffer.\n> >\n> >> Signed-off-by: Hirokazu Honda \n> >> ---\n> >> .../libcamera/internal/mapped_framebuffer.h | 1 +\n> >> src/ipa/rkisp1/rkisp1.cpp | 31 +++++++------------\n> >> src/libcamera/mapped_framebuffer.cpp | 7 +++++\n> >> 3 files changed, 19 insertions(+), 20 deletions(-)\n> >>\n> >> diff --git a/include/libcamera/internal/mapped_framebuffer.h b/include/libcamera/internal/mapped_framebuffer.h\n> >> index 42479541..ee0583d0 100644\n> >> --- a/include/libcamera/internal/mapped_framebuffer.h\n> >> +++ b/include/libcamera/internal/mapped_framebuffer.h\n> >> @@ -55,6 +55,7 @@ public:\n> >>\n> >> using MapFlags = Flags;\n> >>\n> >> + MappedFrameBuffer();\n> >> MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags);\n> >> };\n> >>\n> >> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> >> index 06fb9640..54cf2885 100644\n> >> --- a/src/ipa/rkisp1/rkisp1.cpp\n> >> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> >> @@ -10,7 +10,6 @@\n> >> #include \n> >> #include \n> >> #include \n> >> -#include \n> >>\n> >> #include \n> >> #include \n> >> @@ -24,6 +23,8 @@\n> >> #include \n> >> #include \n> >>\n> >> +#include \n> >> +\n> >> namespace libcamera {\n> >>\n> >> LOG_DEFINE_CATEGORY(IPARkISP1)\n> >> @@ -54,7 +55,7 @@ private:\n> >> void metadataReady(unsigned int frame, unsigned int aeState);\n> >>\n> >> std::map buffers_;\n> >> - std::map buffersMemory_;\n> >> + std::map mappedBuffers_;\n> >>\n> >> ControlInfoMap ctrls_;\n> >>\n> >> @@ -160,21 +161,10 @@ void IPARkISP1::mapBuffers(const std::vector &buffers)\n> >> std::forward_as_tuple(buffer.planes));\n> >> const FrameBuffer &fb = elem.first->second;\n> >>\n> >> - /*\n> >> - * \\todo Provide a helper to mmap() buffers (possibly exposed\n> >> - * to applications).\n> >> - */\n> >> - buffersMemory_[buffer.id] = mmap(NULL,\n> >> - fb.planes()[0].length,\n> >> - PROT_READ | PROT_WRITE,\n> >> - MAP_SHARED,\n> >> - fb.planes()[0].fd.fd(),\n> >> - 0);\n> >> -\n> >> - if (buffersMemory_[buffer.id] == MAP_FAILED) {\n> >> - int ret = -errno;\n> >> + mappedBuffers_[buffer.id] = MappedFrameBuffer(&fb, MappedFrameBuffer::MapFlag::ReadWrite);\n> >\n> > We could use emplace() here to avoid the need for a default constructor,\n> > but we wouldn't be able to use operator[]() below, which isn't nice. I\n> > wonder if we could maybe store a std::unique_ptr in\n> > mappedBuffers_, to avoid the default constructor ?\n> >\n> > Kieran, you've designed the MappedFrameBuffer class, what do you think ?\n>\n> Hrm, firstly the error_ flag needs to be fixed up if there's a default\n> constructor - mentioned below in the Constructor code.\n>\n> But otherwise, even though we do have the isValid() check, I don't think\n> we have many users that actually check it.\n>\n> The aim was to make it simple and easy to create the mapping, which is\n> why it happens on construction.\n>\n> If this is causing problems, we can separate construction and mapping...\n> but then I'd expect to have map()/unmap() as well.\n>\n>\n> Looking at existing users of MappedFrameBuffer, I can see that:\n>\n> IPAVimc::mapBuffers() currently uses emplace, (with a\n> piecewise_construct, to be able to specify the mapping flags as\n> read-only), and only uses find() to get them.\n>\n>\n> So does IPU3. ...\n>\n> And unsurprisingly, so does RPi.\n>\n>\n> However none of those implementations are checking the isValid() flag.\n> So perhaps they need to be updated to do so.\n>\n> And if we mandate that the isValid() should be checked, then it's not\n> much different to separating construction and mapping...\n>\n>\n> The main goal for the current design is to allow other buffer types to\n> be constructed, so perhaps there could be a MappedDRMBuffer or\n> MappedDMABuffer or such.\n>\n> As long as that's kept in mind, I don't mind if the main interface\n> changes to suit needs as we determine them.\n>\n> Would using [], require checking the .isValid() flag at every usage?\n> (Because we might be given a new empty mapping, if the search failed?)\n>\n> That might become less convenient ... ?\n>\n>\n\nI store std::unique_ptr to map as Laurent suggested.\nSo I can remove the default constructor of MappedFrameBuffer.\n\nRegards,\n-Hiro\n>\n> > Apart from this,\n> >\n> > Reviewed-by: Laurent Pinchart \n> >\n> >> + if (!mappedBuffers_[buffer.id].isValid()) {\n> >> LOG(IPARkISP1, Fatal) << \"Failed to mmap buffer: \"\n> >> - << strerror(-ret);\n> >> + << strerror(mappedBuffers_[buffer.id].error());\n> >> }\n> >> }\n> >> }\n> >> @@ -186,8 +176,7 @@ void IPARkISP1::unmapBuffers(const std::vector &ids)\n> >> if (fb == buffers_.end())\n> >> continue;\n> >>\n> >> - munmap(buffersMemory_[id], fb->second.planes()[0].length);\n> >> - buffersMemory_.erase(id);\n> >> + mappedBuffers_.erase(id);\n> >> buffers_.erase(id);\n> >> }\n> >> }\n> >> @@ -200,7 +189,8 @@ void IPARkISP1::processEvent(const RkISP1Event &event)\n> >> unsigned int bufferId = event.bufferId;\n> >>\n> >> const rkisp1_stat_buffer *stats =\n> >> - static_cast(buffersMemory_[bufferId]);\n> >> + reinterpret_cast(\n> >> + mappedBuffers_[bufferId].maps()[0].data());\n> >>\n> >> updateStatistics(frame, stats);\n> >> break;\n> >> @@ -210,7 +200,8 @@ void IPARkISP1::processEvent(const RkISP1Event &event)\n> >> unsigned int bufferId = event.bufferId;\n> >>\n> >> rkisp1_params_cfg *params =\n> >> - static_cast(buffersMemory_[bufferId]);\n> >> + reinterpret_cast(\n> >> + mappedBuffers_[bufferId].maps()[0].data());\n> >>\n> >> queueRequest(frame, params, event.controls);\n> >> break;\n> >> diff --git a/src/libcamera/mapped_framebuffer.cpp b/src/libcamera/mapped_framebuffer.cpp\n> >> index 03425dea..34d9564d 100644\n> >> --- a/src/libcamera/mapped_framebuffer.cpp\n> >> +++ b/src/libcamera/mapped_framebuffer.cpp\n> >> @@ -168,6 +168,13 @@ MappedBuffer::~MappedBuffer()\n> >> * \\brief A bitwise combination of MappedFrameBuffer::MapFlag values\n> >> */\n> >>\n> >> +/**\n> >> + * \\brief Construct an empty MappedFrameBuffer\n> >> + */\n> >> +MappedFrameBuffer::MappedFrameBuffer()\n> >> + : MappedBuffer()\n> >> +{\n>\n> If this is created, error_ would have to be set to an ... error here.\n>\n> Perhaps something like -ENOMEM, so that 'default constructed' empty\n> mappings are not 'valid'.\n>\n> That would then necessitate changine the ordering of how error_ is used,\n> currently it's only set when an error occurs on mapping.\n>\n> It would have to be set to 0 on successful mappings as well/instead.\n>\n>\n>\n> >> +}\n> >> /**\n> >> * \\brief Map all planes of a FrameBuffer\n> >> * \\param[in] buffer FrameBuffer to be mapped\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 B9B0BBD87D\n\tfor ;\n\tWed, 25 Aug 2021 06:13:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EF95A688A3;\n\tWed, 25 Aug 2021 08:13:56 +0200 (CEST)","from mail-ej1-x629.google.com (mail-ej1-x629.google.com\n\t[IPv6:2a00:1450:4864:20::629])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1A9C960257\n\tfor ;\n\tWed, 25 Aug 2021 08:13:56 +0200 (CEST)","by mail-ej1-x629.google.com with SMTP id e21so33305836ejz.12\n\tfor ;\n\tTue, 24 Aug 2021 23:13:56 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"MC9Gv958\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=OgvM35foGVe+raNw/dRb8MJiBtE7AIRObdpgwiJP2sA=;\n\tb=MC9Gv958pQ3Tgd3dK3oQWYdmHQCHvt8Hmp7HsKQpYSUHDmaZ3UzZrmC2ezMrGFOEQj\n\t4MKPP7gRsxteHp5zPgs/72Q25zrY8yJU7rrbS5cCwUZ9Uq2/e9v0cqval2LKpYkkuEYa\n\t19048JCuW20aCkaLueBVXeuS+/GV+4B5b5STk=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=OgvM35foGVe+raNw/dRb8MJiBtE7AIRObdpgwiJP2sA=;\n\tb=hh9HMMCSHYEzYIY1CFOa4h2UAzs34R+h1IEIgvguBWi1ADvBlinFzEbq4NssuSXYuU\n\t4JfH+LqKVCkAW9isa7/+e8gk/zK1SVS4cxPPURY5adw0UOSsJ4Cyq0iOICWXxpAa2zUh\n\tizeCmK03yVvJFF1lGTuWKXxizKf5fUPuQLac+mfJ00vpyfyJgoYHv3p5Y9MvEdiBwT/j\n\tcgndeTf/kanmDj3X3uakyWD4HZXnnPRPB6kG1Z8/WBuww7lJsoUNn7foCRGCwzsVHG1B\n\tfy1tz1h7zqfL6u2kzUPnD2oSTIHI3vBvM4sT+aUagA2fovxiLZ29sP8GaUIjQJjeXmFV\n\tTg+Q==","X-Gm-Message-State":"AOAM533xpbzH0CcPm1JbXI2onn7ZO0a8sij2932t+/8yTnnTbrETfSX0\n\to0fSoUXlhLptnlnAVV36XGYuZm/HV3vKP4rk9Wi4cA==","X-Google-Smtp-Source":"ABdhPJwYBavVvG7M5ARCOFiV+vWwV5jx8kzIxWKGYL5bLZXmTOJeBymMzxMSynGdU2bQbrPAg/P05c/pt5KFsoOLmgI=","X-Received":"by 2002:a17:906:b052:: with SMTP id\n\tbj18mr45235371ejb.55.1629872035463; \n\tTue, 24 Aug 2021 23:13:55 -0700 (PDT)","MIME-Version":"1.0","References":"<20210823131221.1034059-1-hiroh@chromium.org>\n\t<20210823131221.1034059-6-hiroh@chromium.org>\n\t\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Wed, 25 Aug 2021 15:13:43 +0900","Message-ID":"","To":"Kieran Bingham ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [RFC PATCH v2 05/10] ipa: rkisp1: Use offset\n\tin mapping IPABuffer","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19036,"web_url":"https://patchwork.libcamera.org/comment/19036/","msgid":"","date":"2021-08-25T08:06:12","subject":"Re: [libcamera-devel] [RFC PATCH v2 05/10] ipa: rkisp1: Use offset\n\tin mapping IPABuffer","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Hiro,\n\nOn 25/08/2021 07:13, Hirokazu Honda wrote:\n> Hi Laurent and Kieran, thank you for reviewing.\n\n\n\n>>>\n>>> We could use emplace() here to avoid the need for a default constructor,\n>>> but we wouldn't be able to use operator[]() below, which isn't nice. I\n>>> wonder if we could maybe store a std::unique_ptr in\n>>> mappedBuffers_, to avoid the default constructor ?\n>>>\n>>> Kieran, you've designed the MappedFrameBuffer class, what do you think ?\n>>\n>> Hrm, firstly the error_ flag needs to be fixed up if there's a default\n>> constructor - mentioned below in the Constructor code.\n>>\n>> But otherwise, even though we do have the isValid() check, I don't think\n>> we have many users that actually check it.\n>>\n>> The aim was to make it simple and easy to create the mapping, which is\n>> why it happens on construction.\n>>\n>> If this is causing problems, we can separate construction and mapping...\n>> but then I'd expect to have map()/unmap() as well.\n>>\n>>\n>> Looking at existing users of MappedFrameBuffer, I can see that:\n>>\n>> IPAVimc::mapBuffers() currently uses emplace, (with a\n>> piecewise_construct, to be able to specify the mapping flags as\n>> read-only), and only uses find() to get them.\n>>\n>>\n>> So does IPU3. ...\n>>\n>> And unsurprisingly, so does RPi.\n>>\n>>\n>> However none of those implementations are checking the isValid() flag.\n>> So perhaps they need to be updated to do so.\n>>\n>> And if we mandate that the isValid() should be checked, then it's not\n>> much different to separating construction and mapping...\n>>\n>>\n>> The main goal for the current design is to allow other buffer types to\n>> be constructed, so perhaps there could be a MappedDRMBuffer or\n>> MappedDMABuffer or such.\n>>\n>> As long as that's kept in mind, I don't mind if the main interface\n>> changes to suit needs as we determine them.\n>>\n>> Would using [], require checking the .isValid() flag at every usage?\n>> (Because we might be given a new empty mapping, if the search failed?)\n>>\n>> That might become less convenient ... ?\n>>\n>>\n> \n> I store std::unique_ptr to map as Laurent suggested.\n> So I can remove the default constructor of MappedFrameBuffer.\n\nDo we need to change all the other IPA's to use the same pattern ?\n\n--\nKieran","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 4C00FBD87C\n\tfor ;\n\tWed, 25 Aug 2021 08:06:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8425A688A3;\n\tWed, 25 Aug 2021 10:06:17 +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 E7FC060259\n\tfor ;\n\tWed, 25 Aug 2021 10:06:15 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 550D424F;\n\tWed, 25 Aug 2021 10:06:15 +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=\"os1IDWn+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629878775;\n\tbh=SgyGgR70seJV8alxNKmH8Hx5cYNeVDSHjpFFwiqPYJo=;\n\th=From:Subject:To:Cc:References:Date:In-Reply-To:From;\n\tb=os1IDWn+FWYQBBP2wl5T8BX9oMD1OPVcEVs6Ax/5OE9GoErDDRFggUHLFBr3r8WYw\n\tY9KWd6D+6NSLBE/agI8PPy2CUB8UYbIZuOJk7xMKQTjpGmZVIm7J93SJ1eEoK3LO+P\n\tkCfCB7VAzGbH0uejiPCQ2Ss8hTBknYBOlHn58O+w=","From":"Kieran Bingham ","To":"Hirokazu Honda ","References":"<20210823131221.1034059-1-hiroh@chromium.org>\n\t<20210823131221.1034059-6-hiroh@chromium.org>\n\t\n\t\n\t","Message-ID":"","Date":"Wed, 25 Aug 2021 09:06:12 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [RFC PATCH v2 05/10] ipa: rkisp1: Use offset\n\tin mapping IPABuffer","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19055,"web_url":"https://patchwork.libcamera.org/comment/19055/","msgid":"","date":"2021-08-25T11:03:59","subject":"Re: [libcamera-devel] [RFC PATCH v2 05/10] ipa: rkisp1: Use offset\n\tin mapping IPABuffer","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Kieran,\n\n\nOn Wed, Aug 25, 2021 at 5:06 PM Kieran Bingham\n wrote:\n>\n> Hi Hiro,\n>\n> On 25/08/2021 07:13, Hirokazu Honda wrote:\n> > Hi Laurent and Kieran, thank you for reviewing.\n>\n> \n>\n> >>>\n> >>> We could use emplace() here to avoid the need for a default constructor,\n> >>> but we wouldn't be able to use operator[]() below, which isn't nice. I\n> >>> wonder if we could maybe store a std::unique_ptr in\n> >>> mappedBuffers_, to avoid the default constructor ?\n> >>>\n> >>> Kieran, you've designed the MappedFrameBuffer class, what do you think ?\n> >>\n> >> Hrm, firstly the error_ flag needs to be fixed up if there's a default\n> >> constructor - mentioned below in the Constructor code.\n> >>\n> >> But otherwise, even though we do have the isValid() check, I don't think\n> >> we have many users that actually check it.\n> >>\n> >> The aim was to make it simple and easy to create the mapping, which is\n> >> why it happens on construction.\n> >>\n> >> If this is causing problems, we can separate construction and mapping...\n> >> but then I'd expect to have map()/unmap() as well.\n> >>\n> >>\n> >> Looking at existing users of MappedFrameBuffer, I can see that:\n> >>\n> >> IPAVimc::mapBuffers() currently uses emplace, (with a\n> >> piecewise_construct, to be able to specify the mapping flags as\n> >> read-only), and only uses find() to get them.\n> >>\n> >>\n> >> So does IPU3. ...\n> >>\n> >> And unsurprisingly, so does RPi.\n> >>\n> >>\n> >> However none of those implementations are checking the isValid() flag.\n> >> So perhaps they need to be updated to do so.\n> >>\n> >> And if we mandate that the isValid() should be checked, then it's not\n> >> much different to separating construction and mapping...\n> >>\n> >>\n> >> The main goal for the current design is to allow other buffer types to\n> >> be constructed, so perhaps there could be a MappedDRMBuffer or\n> >> MappedDMABuffer or such.\n> >>\n> >> As long as that's kept in mind, I don't mind if the main interface\n> >> changes to suit needs as we determine them.\n> >>\n> >> Would using [], require checking the .isValid() flag at every usage?\n> >> (Because we might be given a new empty mapping, if the search failed?)\n> >>\n> >> That might become less convenient ... ?\n> >>\n> >>\n> >\n> > I store std::unique_ptr to map as Laurent suggested.\n> > So I can remove the default constructor of MappedFrameBuffer.\n>\n> Do we need to change all the other IPA's to use the same pattern ?\n>\n\nYes, that should be good.\nWhich do you prefer, storing MappedFrameBuffer as unique_ptr or having\nMappedFrameBuffer constructor?\n\n-Hiro\n> --\n> Kieran\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 AFBD5BD87C\n\tfor ;\n\tWed, 25 Aug 2021 11:04:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6EA07688A5;\n\tWed, 25 Aug 2021 13:04:11 +0200 (CEST)","from mail-ed1-x530.google.com (mail-ed1-x530.google.com\n\t[IPv6:2a00:1450:4864:20::530])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3F94760259\n\tfor ;\n\tWed, 25 Aug 2021 13:04:10 +0200 (CEST)","by mail-ed1-x530.google.com with SMTP id i6so36322518edu.1\n\tfor ;\n\tWed, 25 Aug 2021 04:04:10 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"hzcykmQt\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=er9n5nA3Ior+KQ0zHKjH69sNmb1bky8Z3pbz4PoPMGs=;\n\tb=hzcykmQtLY78E5O0Y0F5VkPzfCSYHR57D7lCuccds9bQ2EwnsVeRqRWpxvYV+0fB3B\n\timAnxQUJYJL6MT4sZdMfNCxlRGTMb6LfA1qYqFLUufFu4jezdZ+Wegz7k69OWM/sG9Lj\n\tlryhpqWiOi+LUhJ097UtSxAUJ3UA0C/n1l6Hs=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=er9n5nA3Ior+KQ0zHKjH69sNmb1bky8Z3pbz4PoPMGs=;\n\tb=JtFXF7vzsGTSGb1rxP1VnpYzN6neisVS0IYznYXEEpqGfXxwhxmR7OWQVrvMABT7hh\n\tYzgPCnE+Ej0zLWLtwHEHsjG2rt9+H8rJGRuLi8dYp2kjvI3YcKLJFfB/eL+pErxH/HgH\n\tFpruHsuvQugd+t5s7gCPdMqKrlsiO+9AMQDvspTAQPlr9g2xV0rbLmx1rUHiKcrw9K7V\n\t7NKSyTykj6o7RhK0Pz4H/99tiapCKq8sQJjgtz275OW0LogNx4M2nb1JdykUWieNAEwt\n\twupXqaZ7bWX96ONT6DSUeevw+hfcyK3jx7vCjhvg3M2jkbw8zqzvl2kNySnEEmSvVRF/\n\te8cw==","X-Gm-Message-State":"AOAM530+Kz/IsiNn+Y2mQbgXNbfmse0SIqnJFcVtN/yWNnR9Yd53mied\n\tlv1wClM/gYbpXXSD8XQYwCQhEdg2JUShQwNbvUZ7Gg==","X-Google-Smtp-Source":"ABdhPJyo68PK8f0f2Qtd8tEXYpMFDOd0Hw4saT/clz4skslIjJGE7e1TMCdp3lzAS94UHivZUOiH0f0159iT8GmnG84=","X-Received":"by 2002:a50:ef11:: with SMTP id\n\tm17mr47386011eds.233.1629889449927; \n\tWed, 25 Aug 2021 04:04:09 -0700 (PDT)","MIME-Version":"1.0","References":"<20210823131221.1034059-1-hiroh@chromium.org>\n\t<20210823131221.1034059-6-hiroh@chromium.org>\n\t\n\t\n\t\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Wed, 25 Aug 2021 20:03:59 +0900","Message-ID":"","To":"Kieran Bingham ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [RFC PATCH v2 05/10] ipa: rkisp1: Use offset\n\tin mapping IPABuffer","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19077,"web_url":"https://patchwork.libcamera.org/comment/19077/","msgid":"","date":"2021-08-25T20:49:22","subject":"Re: [libcamera-devel] [RFC PATCH v2 05/10] ipa: rkisp1: Use offset\n\tin mapping IPABuffer","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Tue, Aug 24, 2021 at 01:44:15PM +0100, Kieran Bingham wrote:\n> On 23/08/2021 23:55, Laurent Pinchart wrote:\n> > On Mon, Aug 23, 2021 at 10:12:16PM +0900, Hirokazu Honda wrote:\n> >> IPABuffer is represented by FrameBuffer. FrameBuffer::Plane has\n> >> now an offset. This uses the offset variable to map the IPABuffer.\n> > \n> > The commit message and subject line should be updated, this patch now\n> > moves to MappedFrameBuffer.\n> > \n> >> Signed-off-by: Hirokazu Honda \n> >> ---\n> >> .../libcamera/internal/mapped_framebuffer.h | 1 +\n> >> src/ipa/rkisp1/rkisp1.cpp | 31 +++++++------------\n> >> src/libcamera/mapped_framebuffer.cpp | 7 +++++\n> >> 3 files changed, 19 insertions(+), 20 deletions(-)\n> >>\n> >> diff --git a/include/libcamera/internal/mapped_framebuffer.h b/include/libcamera/internal/mapped_framebuffer.h\n> >> index 42479541..ee0583d0 100644\n> >> --- a/include/libcamera/internal/mapped_framebuffer.h\n> >> +++ b/include/libcamera/internal/mapped_framebuffer.h\n> >> @@ -55,6 +55,7 @@ public:\n> >> \n> >> \tusing MapFlags = Flags;\n> >> \n> >> +\tMappedFrameBuffer();\n> >> \tMappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags);\n> >> };\n> >> \n> >> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> >> index 06fb9640..54cf2885 100644\n> >> --- a/src/ipa/rkisp1/rkisp1.cpp\n> >> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> >> @@ -10,7 +10,6 @@\n> >> #include \n> >> #include \n> >> #include \n> >> -#include \n> >> \n> >> #include \n> >> #include \n> >> @@ -24,6 +23,8 @@\n> >> #include \n> >> #include \n> >> \n> >> +#include \n> >> +\n> >> namespace libcamera {\n> >> \n> >> LOG_DEFINE_CATEGORY(IPARkISP1)\n> >> @@ -54,7 +55,7 @@ private:\n> >> \tvoid metadataReady(unsigned int frame, unsigned int aeState);\n> >> \n> >> \tstd::map buffers_;\n> >> -\tstd::map buffersMemory_;\n> >> +\tstd::map mappedBuffers_;\n> >> \n> >> \tControlInfoMap ctrls_;\n> >> \n> >> @@ -160,21 +161,10 @@ void IPARkISP1::mapBuffers(const std::vector &buffers)\n> >> \t\t\t\t\t std::forward_as_tuple(buffer.planes));\n> >> \t\tconst FrameBuffer &fb = elem.first->second;\n> >> \n> >> -\t\t/*\n> >> -\t\t * \\todo Provide a helper to mmap() buffers (possibly exposed\n> >> -\t\t * to applications).\n> >> -\t\t */\n> >> -\t\tbuffersMemory_[buffer.id] = mmap(NULL,\n> >> -\t\t\t\t\t\t fb.planes()[0].length,\n> >> -\t\t\t\t\t\t PROT_READ | PROT_WRITE,\n> >> -\t\t\t\t\t\t MAP_SHARED,\n> >> -\t\t\t\t\t\t fb.planes()[0].fd.fd(),\n> >> -\t\t\t\t\t\t 0);\n> >> -\n> >> -\t\tif (buffersMemory_[buffer.id] == MAP_FAILED) {\n> >> -\t\t\tint ret = -errno;\n> >> +\t\tmappedBuffers_[buffer.id] = MappedFrameBuffer(&fb, MappedFrameBuffer::MapFlag::ReadWrite);\n> > \n> > We could use emplace() here to avoid the need for a default constructor,\n> > but we wouldn't be able to use operator[]() below, which isn't nice. I\n> > wonder if we could maybe store a std::unique_ptr in\n> > mappedBuffers_, to avoid the default constructor ?\n> > \n> > Kieran, you've designed the MappedFrameBuffer class, what do you think ?\n> \n> Hrm, firstly the error_ flag needs to be fixed up if there's a default\n> constructor - mentioned below in the Constructor code.\n> \n> But otherwise, even though we do have the isValid() check, I don't think\n> we have many users that actually check it.\n> \n> The aim was to make it simple and easy to create the mapping, which is\n> why it happens on construction.\n> \n> If this is causing problems, we can separate construction and mapping...\n> but then I'd expect to have map()/unmap() as well.\n> Looking at existing users of MappedFrameBuffer, I can see that:\n> \n> IPAVimc::mapBuffers() currently uses emplace, (with a\n> piecewise_construct, to be able to specify the mapping flags as\n> read-only), and only uses find() to get them.\n> \n> \n> So does IPU3. ...\n> \n> And unsurprisingly, so does RPi.\n> \n> \n> However none of those implementations are checking the isValid() flag.\n> So perhaps they need to be updated to do so.\n> \n> And if we mandate that the isValid() should be checked, then it's not\n> much different to separating construction and mapping...\n\nI like having construction and mapping grouped together, but you're\nright that separating them could indeed force users to check for errors\n(if we annotate the map() function with __nodiscard).\n\n> The main goal for the current design is to allow other buffer types to\n> be constructed, so perhaps there could be a MappedDRMBuffer or\n> MappedDMABuffer or such.\n> \n> As long as that's kept in mind, I don't mind if the main interface\n> changes to suit needs as we determine them.\n> \n> Would using [], require checking the .isValid() flag at every usage?\n> (Because we might be given a new empty mapping, if the search failed?)\n\nNot really, a user could be trusted to pass valid keys to operator[]().\nStoring the instances in a std::map>\nas I've proposed would result in a null pointer dereference if the code\ntried to access an entry with an invalid key, which wouldn't be very\ndifferent than using a default-constructed MappedFrameBuffer. We would\n\"just\" crash later in that case :-)\n\n> That might become less convenient ... ?\n> \n> > Apart from this,\n> > \n> > Reviewed-by: Laurent Pinchart \n> > \n> >> +\t\tif (!mappedBuffers_[buffer.id].isValid()) {\n> >> \t\t\tLOG(IPARkISP1, Fatal) << \"Failed to mmap buffer: \"\n> >> -\t\t\t\t\t << strerror(-ret);\n> >> +\t\t\t\t\t << strerror(mappedBuffers_[buffer.id].error());\n> >> \t\t}\n> >> \t}\n> >> }\n> >> @@ -186,8 +176,7 @@ void IPARkISP1::unmapBuffers(const std::vector &ids)\n> >> \t\tif (fb == buffers_.end())\n> >> \t\t\tcontinue;\n> >> \n> >> -\t\tmunmap(buffersMemory_[id], fb->second.planes()[0].length);\n> >> -\t\tbuffersMemory_.erase(id);\n> >> +\t\tmappedBuffers_.erase(id);\n> >> \t\tbuffers_.erase(id);\n> >> \t}\n> >> }\n> >> @@ -200,7 +189,8 @@ void IPARkISP1::processEvent(const RkISP1Event &event)\n> >> \t\tunsigned int bufferId = event.bufferId;\n> >> \n> >> \t\tconst rkisp1_stat_buffer *stats =\n> >> -\t\t\tstatic_cast(buffersMemory_[bufferId]);\n> >> +\t\t\treinterpret_cast(\n> >> +\t\t\t\tmappedBuffers_[bufferId].maps()[0].data());\n> >> \n> >> \t\tupdateStatistics(frame, stats);\n> >> \t\tbreak;\n> >> @@ -210,7 +200,8 @@ void IPARkISP1::processEvent(const RkISP1Event &event)\n> >> \t\tunsigned int bufferId = event.bufferId;\n> >> \n> >> \t\trkisp1_params_cfg *params =\n> >> -\t\t\tstatic_cast(buffersMemory_[bufferId]);\n> >> +\t\t\treinterpret_cast(\n> >> +\t\t\t\tmappedBuffers_[bufferId].maps()[0].data());\n> >> \n> >> \t\tqueueRequest(frame, params, event.controls);\n> >> \t\tbreak;\n> >> diff --git a/src/libcamera/mapped_framebuffer.cpp b/src/libcamera/mapped_framebuffer.cpp\n> >> index 03425dea..34d9564d 100644\n> >> --- a/src/libcamera/mapped_framebuffer.cpp\n> >> +++ b/src/libcamera/mapped_framebuffer.cpp\n> >> @@ -168,6 +168,13 @@ MappedBuffer::~MappedBuffer()\n> >> * \\brief A bitwise combination of MappedFrameBuffer::MapFlag values\n> >> */\n> >> \n> >> +/**\n> >> + * \\brief Construct an empty MappedFrameBuffer\n> >> + */\n> >> +MappedFrameBuffer::MappedFrameBuffer()\n> >> +\t: MappedBuffer()\n> >> +{\n> \n> If this is created, error_ would have to be set to an ... error here.\n> \n> Perhaps something like -ENOMEM, so that 'default constructed' empty\n> mappings are not 'valid'.\n> \n> That would then necessitate changine the ordering of how error_ is used,\n> currently it's only set when an error occurs on mapping.\n> \n> It would have to be set to 0 on successful mappings as well/instead.\n> \n> >> +}\n> >> /**\n> >> * \\brief Map all planes of a FrameBuffer\n> >> * \\param[in] buffer FrameBuffer to be mapped\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 2F792BD87C\n\tfor ;\n\tWed, 25 Aug 2021 20:49:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A264B688A5;\n\tWed, 25 Aug 2021 22:49:36 +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 9692760288\n\tfor ;\n\tWed, 25 Aug 2021 22:49:34 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id F302F24F;\n\tWed, 25 Aug 2021 22:49:33 +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=\"b1ELhOap\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629924574;\n\tbh=dshYQ3sPg8LOf6eoo8SxrWgbH3HcgqJ2rUUhbeLwy7s=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=b1ELhOaplY0a5yD4MEDHpfMw4cpgLzATqgLgcQkTy5SH2+AHoqZpODGVabJwm4Fty\n\tG150H2qJvAtwqQMtlx3Hpm/KHHtwBOG4RdjvGX/AUTbvxzlMOtpIzUWUubD4TV81jY\n\tnfblo6H++ApX8MKe9kL2DzFhcSRRn/7MHELPtGXY=","Date":"Wed, 25 Aug 2021 23:49:22 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210823131221.1034059-1-hiroh@chromium.org>\n\t<20210823131221.1034059-6-hiroh@chromium.org>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [RFC PATCH v2 05/10] ipa: rkisp1: Use offset\n\tin mapping IPABuffer","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19078,"web_url":"https://patchwork.libcamera.org/comment/19078/","msgid":"","date":"2021-08-25T21:05:03","subject":"Re: [libcamera-devel] [RFC PATCH v2 05/10] ipa: rkisp1: Use offset\n\tin mapping IPABuffer","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nOn Mon, Aug 23, 2021 at 10:12:16PM +0900, Hirokazu Honda wrote:\n> IPABuffer is represented by FrameBuffer. FrameBuffer::Plane has\n> now an offset. This uses the offset variable to map the IPABuffer.\n> \n> Signed-off-by: Hirokazu Honda \n> ---\n> .../libcamera/internal/mapped_framebuffer.h | 1 +\n> src/ipa/rkisp1/rkisp1.cpp | 31 +++++++------------\n> src/libcamera/mapped_framebuffer.cpp | 7 +++++\n> 3 files changed, 19 insertions(+), 20 deletions(-)\n> \n> diff --git a/include/libcamera/internal/mapped_framebuffer.h b/include/libcamera/internal/mapped_framebuffer.h\n> index 42479541..ee0583d0 100644\n> --- a/include/libcamera/internal/mapped_framebuffer.h\n> +++ b/include/libcamera/internal/mapped_framebuffer.h\n> @@ -55,6 +55,7 @@ public:\n> \n> \tusing MapFlags = Flags;\n> \n> +\tMappedFrameBuffer();\n> \tMappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags);\n> };\n> \n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index 06fb9640..54cf2885 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -10,7 +10,6 @@\n> #include \n> #include \n> #include \n> -#include \n> \n> #include \n> #include \n> @@ -24,6 +23,8 @@\n> #include \n> #include \n> \n> +#include \n> +\n> namespace libcamera {\n> \n> LOG_DEFINE_CATEGORY(IPARkISP1)\n> @@ -54,7 +55,7 @@ private:\n> \tvoid metadataReady(unsigned int frame, unsigned int aeState);\n> \n> \tstd::map buffers_;\n> -\tstd::map buffersMemory_;\n> +\tstd::map mappedBuffers_;\n> \n> \tControlInfoMap ctrls_;\n> \n> @@ -160,21 +161,10 @@ void IPARkISP1::mapBuffers(const std::vector &buffers)\n> \t\t\t\t\t std::forward_as_tuple(buffer.planes));\n> \t\tconst FrameBuffer &fb = elem.first->second;\n> \n> -\t\t/*\n> -\t\t * \\todo Provide a helper to mmap() buffers (possibly exposed\n> -\t\t * to applications).\n> -\t\t */\n> -\t\tbuffersMemory_[buffer.id] = mmap(NULL,\n> -\t\t\t\t\t\t fb.planes()[0].length,\n> -\t\t\t\t\t\t PROT_READ | PROT_WRITE,\n> -\t\t\t\t\t\t MAP_SHARED,\n> -\t\t\t\t\t\t fb.planes()[0].fd.fd(),\n> -\t\t\t\t\t\t 0);\n> -\n> -\t\tif (buffersMemory_[buffer.id] == MAP_FAILED) {\n> -\t\t\tint ret = -errno;\n> +\t\tmappedBuffers_[buffer.id] = MappedFrameBuffer(&fb, MappedFrameBuffer::MapFlag::ReadWrite);\n> +\t\tif (!mappedBuffers_[buffer.id].isValid()) {\n> \t\t\tLOG(IPARkISP1, Fatal) << \"Failed to mmap buffer: \"\n> -\t\t\t\t\t << strerror(-ret);\n> +\t\t\t\t\t << strerror(mappedBuffers_[buffer.id].error());\n> \t\t}\n\nAnother point I wanted to mention is that we could also fix the default\nconstructor issue by avoiding operator[](). Here we could have\n\n\t\tMappedFrameBuffer mappedBuffer{ &fb, MappedFrameBuffer::MapFlag::ReadWrite };\n\t\tif (!mappedBuffer.isValid()) {\n\t\t\tLOG(IPARkISP1, Fatal)\n\t\t\t\t<< \"Failed to mmap buffer: \"\n\t\t\t\t<< strerror(mappedBuffer.error());\n\t\t}\n\n\t\tmappedBuffers_.emplace(buffer.id, std::move(mappedBuffer));\n\n> \t}\n> }\n> @@ -186,8 +176,7 @@ void IPARkISP1::unmapBuffers(const std::vector &ids)\n> \t\tif (fb == buffers_.end())\n> \t\t\tcontinue;\n> \n> -\t\tmunmap(buffersMemory_[id], fb->second.planes()[0].length);\n> -\t\tbuffersMemory_.erase(id);\n> +\t\tmappedBuffers_.erase(id);\n> \t\tbuffers_.erase(id);\n> \t}\n> }\n> @@ -200,7 +189,8 @@ void IPARkISP1::processEvent(const RkISP1Event &event)\n> \t\tunsigned int bufferId = event.bufferId;\n> \n> \t\tconst rkisp1_stat_buffer *stats =\n> -\t\t\tstatic_cast(buffersMemory_[bufferId]);\n> +\t\t\treinterpret_cast(\n> +\t\t\t\tmappedBuffers_[bufferId].maps()[0].data());\n\nAnd here and below, you could use\n\n\t\t\t\tmappedBuffers_.at(bufferId).maps()[0].data());\n\n> \n> \t\tupdateStatistics(frame, stats);\n> \t\tbreak;\n> @@ -210,7 +200,8 @@ void IPARkISP1::processEvent(const RkISP1Event &event)\n> \t\tunsigned int bufferId = event.bufferId;\n> \n> \t\trkisp1_params_cfg *params =\n> -\t\t\tstatic_cast(buffersMemory_[bufferId]);\n> +\t\t\treinterpret_cast(\n> +\t\t\t\tmappedBuffers_[bufferId].maps()[0].data());\n> \n> \t\tqueueRequest(frame, params, event.controls);\n> \t\tbreak;\n> diff --git a/src/libcamera/mapped_framebuffer.cpp b/src/libcamera/mapped_framebuffer.cpp\n> index 03425dea..34d9564d 100644\n> --- a/src/libcamera/mapped_framebuffer.cpp\n> +++ b/src/libcamera/mapped_framebuffer.cpp\n> @@ -168,6 +168,13 @@ MappedBuffer::~MappedBuffer()\n> * \\brief A bitwise combination of MappedFrameBuffer::MapFlag values\n> */\n> \n> +/**\n> + * \\brief Construct an empty MappedFrameBuffer\n> + */\n> +MappedFrameBuffer::MappedFrameBuffer()\n> +\t: MappedBuffer()\n> +{\n> +}\n> /**\n> * \\brief Map all planes of a FrameBuffer\n> * \\param[in] buffer FrameBuffer to be mapped","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 E24D8BD87D\n\tfor ;\n\tWed, 25 Aug 2021 21:05:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4064A68893;\n\tWed, 25 Aug 2021 23:05:17 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2C60A60288\n\tfor ;\n\tWed, 25 Aug 2021 23:05:16 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A2C9724F;\n\tWed, 25 Aug 2021 23:05:15 +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=\"cMkD/5Zb\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629925515;\n\tbh=OSBC1R9/8ymRfD0tPTL2AqAlwrmDy54nj7KHHGIlqhY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=cMkD/5ZbQeQhe5L7rsn/KAs4demRGYhq87Hrlh2V0GsSa9QGRG/VRuGOZ4bt/DayH\n\tV30Bd3GsA3c8Mq4uyyx8berAgW8mau4NgByVwe03qSfaQivsni16dJh/8hp7ob6gkt\n\takehggixdccEjN42BKiYad45fxzDY7f3js5yl8Ck=","Date":"Thu, 26 Aug 2021 00:05:03 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210823131221.1034059-1-hiroh@chromium.org>\n\t<20210823131221.1034059-6-hiroh@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210823131221.1034059-6-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [RFC PATCH v2 05/10] ipa: rkisp1: Use offset\n\tin mapping IPABuffer","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19080,"web_url":"https://patchwork.libcamera.org/comment/19080/","msgid":"","date":"2021-08-25T21:19:19","subject":"Re: [libcamera-devel] [RFC PATCH v2 05/10] ipa: rkisp1: Use offset\n\tin mapping IPABuffer","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On 25/08/2021 21:49, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> On Tue, Aug 24, 2021 at 01:44:15PM +0100, Kieran Bingham wrote:\n>> On 23/08/2021 23:55, Laurent Pinchart wrote:\n>>> On Mon, Aug 23, 2021 at 10:12:16PM +0900, Hirokazu Honda wrote:\n>>>> IPABuffer is represented by FrameBuffer. FrameBuffer::Plane has\n>>>> now an offset. This uses the offset variable to map the IPABuffer.\n>>>\n>>> The commit message and subject line should be updated, this patch now\n>>> moves to MappedFrameBuffer.\n>>>\n>>>> Signed-off-by: Hirokazu Honda \n>>>> ---\n>>>> .../libcamera/internal/mapped_framebuffer.h | 1 +\n>>>> src/ipa/rkisp1/rkisp1.cpp | 31 +++++++------------\n>>>> src/libcamera/mapped_framebuffer.cpp | 7 +++++\n>>>> 3 files changed, 19 insertions(+), 20 deletions(-)\n>>>>\n>>>> diff --git a/include/libcamera/internal/mapped_framebuffer.h b/include/libcamera/internal/mapped_framebuffer.h\n>>>> index 42479541..ee0583d0 100644\n>>>> --- a/include/libcamera/internal/mapped_framebuffer.h\n>>>> +++ b/include/libcamera/internal/mapped_framebuffer.h\n>>>> @@ -55,6 +55,7 @@ public:\n>>>> \n>>>> \tusing MapFlags = Flags;\n>>>> \n>>>> +\tMappedFrameBuffer();\n>>>> \tMappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags);\n>>>> };\n>>>> \n>>>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n>>>> index 06fb9640..54cf2885 100644\n>>>> --- a/src/ipa/rkisp1/rkisp1.cpp\n>>>> +++ b/src/ipa/rkisp1/rkisp1.cpp\n>>>> @@ -10,7 +10,6 @@\n>>>> #include \n>>>> #include \n>>>> #include \n>>>> -#include \n>>>> \n>>>> #include \n>>>> #include \n>>>> @@ -24,6 +23,8 @@\n>>>> #include \n>>>> #include \n>>>> \n>>>> +#include \n>>>> +\n>>>> namespace libcamera {\n>>>> \n>>>> LOG_DEFINE_CATEGORY(IPARkISP1)\n>>>> @@ -54,7 +55,7 @@ private:\n>>>> \tvoid metadataReady(unsigned int frame, unsigned int aeState);\n>>>> \n>>>> \tstd::map buffers_;\n>>>> -\tstd::map buffersMemory_;\n>>>> +\tstd::map mappedBuffers_;\n>>>> \n>>>> \tControlInfoMap ctrls_;\n>>>> \n>>>> @@ -160,21 +161,10 @@ void IPARkISP1::mapBuffers(const std::vector &buffers)\n>>>> \t\t\t\t\t std::forward_as_tuple(buffer.planes));\n>>>> \t\tconst FrameBuffer &fb = elem.first->second;\n>>>> \n>>>> -\t\t/*\n>>>> -\t\t * \\todo Provide a helper to mmap() buffers (possibly exposed\n>>>> -\t\t * to applications).\n>>>> -\t\t */\n>>>> -\t\tbuffersMemory_[buffer.id] = mmap(NULL,\n>>>> -\t\t\t\t\t\t fb.planes()[0].length,\n>>>> -\t\t\t\t\t\t PROT_READ | PROT_WRITE,\n>>>> -\t\t\t\t\t\t MAP_SHARED,\n>>>> -\t\t\t\t\t\t fb.planes()[0].fd.fd(),\n>>>> -\t\t\t\t\t\t 0);\n>>>> -\n>>>> -\t\tif (buffersMemory_[buffer.id] == MAP_FAILED) {\n>>>> -\t\t\tint ret = -errno;\n>>>> +\t\tmappedBuffers_[buffer.id] = MappedFrameBuffer(&fb, MappedFrameBuffer::MapFlag::ReadWrite);\n>>>\n>>> We could use emplace() here to avoid the need for a default constructor,\n>>> but we wouldn't be able to use operator[]() below, which isn't nice. I\n>>> wonder if we could maybe store a std::unique_ptr in\n>>> mappedBuffers_, to avoid the default constructor ?\n>>>\n>>> Kieran, you've designed the MappedFrameBuffer class, what do you think ?\n>>\n>> Hrm, firstly the error_ flag needs to be fixed up if there's a default\n>> constructor - mentioned below in the Constructor code.\n>>\n>> But otherwise, even though we do have the isValid() check, I don't think\n>> we have many users that actually check it.\n>>\n>> The aim was to make it simple and easy to create the mapping, which is\n>> why it happens on construction.\n>>\n>> If this is causing problems, we can separate construction and mapping...\n>> but then I'd expect to have map()/unmap() as well.\n>> Looking at existing users of MappedFrameBuffer, I can see that:\n>>\n>> IPAVimc::mapBuffers() currently uses emplace, (with a\n>> piecewise_construct, to be able to specify the mapping flags as\n>> read-only), and only uses find() to get them.\n>>\n>>\n>> So does IPU3. ...\n>>\n>> And unsurprisingly, so does RPi.\n>>\n>>\n>> However none of those implementations are checking the isValid() flag.\n>> So perhaps they need to be updated to do so.\n>>\n>> And if we mandate that the isValid() should be checked, then it's not\n>> much different to separating construction and mapping...\n> \n> I like having construction and mapping grouped together, but you're\n> right that separating them could indeed force users to check for errors\n> (if we annotate the map() function with __nodiscard).\n\nYes, the point was to make it easy to use.\n\nBut if it's verging on promotion to an actual API exposed to\napplications, it's worth considering correctness\n\n>> The main goal for the current design is to allow other buffer types to\n>> be constructed, so perhaps there could be a MappedDRMBuffer or\n>> MappedDMABuffer or such.\n>>\n>> As long as that's kept in mind, I don't mind if the main interface\n>> changes to suit needs as we determine them.\n>>\n>> Would using [], require checking the .isValid() flag at every usage?\n>> (Because we might be given a new empty mapping, if the search failed?)\n> \n> Not really, a user could be trusted to pass valid keys to operator[]().\n> Storing the instances in a std::map>\n> as I've proposed would result in a null pointer dereference if the code\n> tried to access an entry with an invalid key, which wouldn't be very\n> different than using a default-constructed MappedFrameBuffer. We would\n> \"just\" crash later in that case :-)\n\nSo I'm not opposed to supporting the [], as long as it doesn't lead to\npotential mis-usages of a MappedFrameBuffer which aren't valid.\n\nAnd the .at() works too.\n\n> \n>> That might become less convenient ... ?\n>>\n>>> Apart from this,\n>>>\n>>> Reviewed-by: Laurent Pinchart \n>>>\n>>>> +\t\tif (!mappedBuffers_[buffer.id].isValid()) {\n>>>> \t\t\tLOG(IPARkISP1, Fatal) << \"Failed to mmap buffer: \"\n>>>> -\t\t\t\t\t << strerror(-ret);\n>>>> +\t\t\t\t\t << strerror(mappedBuffers_[buffer.id].error());\n>>>> \t\t}\n>>>> \t}\n>>>> }\n>>>> @@ -186,8 +176,7 @@ void IPARkISP1::unmapBuffers(const std::vector &ids)\n>>>> \t\tif (fb == buffers_.end())\n>>>> \t\t\tcontinue;\n>>>> \n>>>> -\t\tmunmap(buffersMemory_[id], fb->second.planes()[0].length);\n>>>> -\t\tbuffersMemory_.erase(id);\n>>>> +\t\tmappedBuffers_.erase(id);\n>>>> \t\tbuffers_.erase(id);\n>>>> \t}\n>>>> }\n>>>> @@ -200,7 +189,8 @@ void IPARkISP1::processEvent(const RkISP1Event &event)\n>>>> \t\tunsigned int bufferId = event.bufferId;\n>>>> \n>>>> \t\tconst rkisp1_stat_buffer *stats =\n>>>> -\t\t\tstatic_cast(buffersMemory_[bufferId]);\n>>>> +\t\t\treinterpret_cast(\n>>>> +\t\t\t\tmappedBuffers_[bufferId].maps()[0].data());\n>>>> \n>>>> \t\tupdateStatistics(frame, stats);\n>>>> \t\tbreak;\n>>>> @@ -210,7 +200,8 @@ void IPARkISP1::processEvent(const RkISP1Event &event)\n>>>> \t\tunsigned int bufferId = event.bufferId;\n>>>> \n>>>> \t\trkisp1_params_cfg *params =\n>>>> -\t\t\tstatic_cast(buffersMemory_[bufferId]);\n>>>> +\t\t\treinterpret_cast(\n>>>> +\t\t\t\tmappedBuffers_[bufferId].maps()[0].data());\n>>>> \n>>>> \t\tqueueRequest(frame, params, event.controls);\n>>>> \t\tbreak;\n>>>> diff --git a/src/libcamera/mapped_framebuffer.cpp b/src/libcamera/mapped_framebuffer.cpp\n>>>> index 03425dea..34d9564d 100644\n>>>> --- a/src/libcamera/mapped_framebuffer.cpp\n>>>> +++ b/src/libcamera/mapped_framebuffer.cpp\n>>>> @@ -168,6 +168,13 @@ MappedBuffer::~MappedBuffer()\n>>>> * \\brief A bitwise combination of MappedFrameBuffer::MapFlag values\n>>>> */\n>>>> \n>>>> +/**\n>>>> + * \\brief Construct an empty MappedFrameBuffer\n>>>> + */\n>>>> +MappedFrameBuffer::MappedFrameBuffer()\n>>>> +\t: MappedBuffer()\n>>>> +{\n>>\n>> If this is created, error_ would have to be set to an ... error here.\n>>\n>> Perhaps something like -ENOMEM, so that 'default constructed' empty\n>> mappings are not 'valid'.\n>>\n>> That would then necessitate changine the ordering of how error_ is used,\n>> currently it's only set when an error occurs on mapping.\n>>\n>> It would have to be set to 0 on successful mappings as well/instead.\n>>\n>>>> +}\n>>>> /**\n>>>> * \\brief Map all planes of a FrameBuffer\n>>>> * \\param[in] buffer FrameBuffer to be mapped\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 2E0D5BD87D\n\tfor ;\n\tWed, 25 Aug 2021 21:19:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E876C688A3;\n\tWed, 25 Aug 2021 23:19:23 +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 1442C60288\n\tfor ;\n\tWed, 25 Aug 2021 23:19:22 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7480C24F;\n\tWed, 25 Aug 2021 23:19:21 +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=\"s0VTF3E0\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629926361;\n\tbh=HNtFBsf5rjjbpP7LNQ3WA0XUOYq+GmE7PEPxGvFR/HI=;\n\th=From:To:Cc:References:Subject:Date:In-Reply-To:From;\n\tb=s0VTF3E018hWmhqT9Y8Xw9o3VvRFrLGhG/A2FzLts1/+95mkdOve1b9bvaHNcRMl2\n\tQGiyxo2Uz3b8CqN6smfDlUB1XWvQZDGClBZJzbcCeakltVQozmy5OrU5D70AMqZr1l\n\tSbtGCNvfuw/fjQjiIy0Xc0w+wgjOetSvK5tn2PEY=","From":"Kieran Bingham ","To":"Laurent Pinchart ","References":"<20210823131221.1034059-1-hiroh@chromium.org>\n\t<20210823131221.1034059-6-hiroh@chromium.org>\n\t\n\t\n\t","Message-ID":"","Date":"Wed, 25 Aug 2021 22:19:19 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [RFC PATCH v2 05/10] ipa: rkisp1: Use offset\n\tin mapping IPABuffer","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]