[{"id":13722,"web_url":"https://patchwork.libcamera.org/comment/13722/","msgid":"<20201116122808.GH6540@pendragon.ideasonboard.com>","date":"2020-11-16T12:28:08","subject":"Re: [libcamera-devel] [PATCH v2 1/2] pipeline: raspberrypi: Use\n\tMappedFrameBuffer for embedded data buffers","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nThank you for the patch.\n\nOn Mon, Nov 16, 2020 at 11:24:57AM +0000, Naushir Patuck wrote:\n> Use a MappedFrameBuffer to mmap embedded data buffers for the pipeline\n> handler to use in the cases where the sensor does not fill it in. This\n> avoids the need to mmap and unmap on every frame.\n> \n> Signed-off-by: Naushir Patuck \n> Reviewed-by: Kieran Bingham \n> ---\n> .../pipeline/raspberrypi/raspberrypi.cpp | 33 ++++++++++++++-----\n> 1 file changed, 25 insertions(+), 8 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index dd62dfc7..faa06c00 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -24,6 +24,7 @@\n> #include \n> \n> #include \"libcamera/internal/bayer_format.h\"\n> +#include \"libcamera/internal/buffer.h\"\n> #include \"libcamera/internal/camera_sensor.h\"\n> #include \"libcamera/internal/device_enumerator.h\"\n> #include \"libcamera/internal/ipa_manager.h\"\n> @@ -165,6 +166,12 @@ public:\n> \t/* Stores the ids of the buffers mapped in the IPA. */\n> \tstd::unordered_set ipaBuffers_;\n> \n> +\t/*\n> +\t * Map of (internal) mmaped embedded data buffers, to avoid having to\n> +\t * map/unmap on every frame.\n> +\t */\n> +\tstd::map mappedEmbeddedBuffers_;\n> +\n> \t/* DMAHEAP allocation helper. */\n> \tRPi::DmaHeap dmaHeap_;\n> \tFileDescriptor lsTable_;\n> @@ -1040,6 +1047,13 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n> \t\t\treturn ret;\n> \t}\n> \n> +\tif (!data->sensorMetadata_) {\n> +\t\tfor (auto const &it : data->unicam_[Unicam::Embedded].getBuffers()) {\n> +\t\t\tdata->mappedEmbeddedBuffers_.emplace(it.first,\n> +\t\t\t\t\t\t\t MappedFrameBuffer(it.second, PROT_READ | PROT_WRITE));\n\nThat's a fairly long line, could we write\n\n\t\t\tMappedFrameBuffer fb(it.second, PROT_READ | PROT_WRITE);\n\t\t\tdata->mappedEmbeddedBuffers_.emplace(it.first, std::move(fb));\n\nAlternatively, if you want to avoid the move construction of the\nelement, you could write\n\n\t\t\tdata->mappedEmbeddedBuffers_.emplace(std::piecewise_construct,\n\t\t\t\t\t\t\t std::forward_as_tuple(it.first),\n\t\t\t\t\t\t\t std::forward_as_tuple(it.second,\n\t\t\t\t\t\t\t\t\t\t PROT_READ | PROT_WRITE));\n\n> +\t\t}\n> +\t}\n> +\n> \t/*\n> \t * Pass the stats and embedded data buffers to the IPA. No other\n> \t * buffers need to be passed.\n> @@ -1078,6 +1092,7 @@ void PipelineHandlerRPi::freeBuffers(Camera *camera)\n> \tstd::vector ipaBuffers(data->ipaBuffers_.begin(), data->ipaBuffers_.end());\n> \tdata->ipa_->unmapBuffers(ipaBuffers);\n> \tdata->ipaBuffers_.clear();\n> +\tdata->mappedEmbeddedBuffers_.clear();\n> \n> \tfor (auto const stream : data->streams_)\n> \t\tstream->releaseBuffers();\n> @@ -1310,14 +1325,16 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)\n> \t\t * metadata buffer.\n> \t\t */\n> \t\tif (!sensorMetadata_) {\n> -\t\t\tconst FrameBuffer &fb = buffer->planes();\n> -\t\t\tuint32_t *mem = static_cast(::mmap(nullptr, fb.planes()[0].length,\n> -\t\t\t\t\t\t\t\t PROT_READ | PROT_WRITE,\n> -\t\t\t\t\t\t\t\t MAP_SHARED,\n> -\t\t\t\t\t\t\t\t fb.planes()[0].fd.fd(), 0));\n> -\t\t\tmem[0] = ctrl[V4L2_CID_EXPOSURE];\n> -\t\t\tmem[1] = ctrl[V4L2_CID_ANALOGUE_GAIN];\n> -\t\t\tmunmap(mem, fb.planes()[0].length);\n> +\t\t\tunsigned int bufferId = unicam_[Unicam::Embedded].getBufferId(buffer);\n> +\t\t\tauto it = mappedEmbeddedBuffers_.find(bufferId);\n> +\t\t\tif (it != mappedEmbeddedBuffers_.end()) {\n> +\t\t\t\tuint32_t *mem = reinterpret_cast(it->second.maps()[0].data());\n> +\t\t\t\tmem[0] = ctrl[V4L2_CID_EXPOSURE];\n> +\t\t\t\tmem[1] = ctrl[V4L2_CID_ANALOGUE_GAIN];\n> +\t\t\t} else {\n> +\t\t\t\tLOG(RPI, Warning) << \"Failed to find embedded buffer \"\n> +\t\t\t\t\t\t << bufferId;\n> +\t\t\t}\n\nOut of scope for this series, but I wonder if we shouldn't improve this\nwith a cleaner API to the IPA instead of faking an embedded data buffer.\n\nReviewed-by: Laurent Pinchart \n\n> \t\t}\n> \t}\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 EA6A8BE081\n\tfor ;\n\tMon, 16 Nov 2020 12:28:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7A69F632C3;\n\tMon, 16 Nov 2020 13:28:17 +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 61AAC631B5\n\tfor ;\n\tMon, 16 Nov 2020 13:28:16 +0100 (CET)","from pendragon.ideasonboard.com (85-76-51-86-nat.elisa-mobile.fi\n\t[85.76.51.86])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BD3B3A1B;\n\tMon, 16 Nov 2020 13:28:15 +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=\"PqVAR9pB\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1605529696;\n\tbh=/xLiTGpC1KLO/CHsQin+46BKK82hC0lHgJPsfQ7xnE8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=PqVAR9pBZPQmEkLByODzOxZcCJjezLocDVNmSryYh0GyZ3jIAWoURChnHhVu6nBL4\n\tJWkcTnGviYrq211ZdU4WZ8AF0o+SB2wgqfAWzcV1cKOkvpTIBnVRCWKyCEgA+aFRkU\n\tFJlO6TIpUXBqXSKsVSEgu+luDZB88T28ZWdtxmcY=","Date":"Mon, 16 Nov 2020 14:28:08 +0200","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"<20201116122808.GH6540@pendragon.ideasonboard.com>","References":"<20201116112458.148477-1-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201116112458.148477-1-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v2 1/2] pipeline: raspberrypi: Use\n\tMappedFrameBuffer for embedded data buffers","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13723,"web_url":"https://patchwork.libcamera.org/comment/13723/","msgid":"","date":"2020-11-16T12:34:16","subject":"Re: [libcamera-devel] [PATCH v2 1/2] pipeline: raspberrypi: Use\n\tMappedFrameBuffer for embedded data buffers","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nThank you for your comments. I will apply your suggestions for both\npatches in this series and submit a v3 to merge.\n\nRegards,\nNaush\n\n\nOn Mon, 16 Nov 2020 at 12:28, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> Thank you for the patch.\n>\n> On Mon, Nov 16, 2020 at 11:24:57AM +0000, Naushir Patuck wrote:\n> > Use a MappedFrameBuffer to mmap embedded data buffers for the pipeline\n> > handler to use in the cases where the sensor does not fill it in. This\n> > avoids the need to mmap and unmap on every frame.\n> >\n> > Signed-off-by: Naushir Patuck \n> > Reviewed-by: Kieran Bingham \n> > ---\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 33 ++++++++++++++-----\n> > 1 file changed, 25 insertions(+), 8 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index dd62dfc7..faa06c00 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -24,6 +24,7 @@\n> > #include \n> >\n> > #include \"libcamera/internal/bayer_format.h\"\n> > +#include \"libcamera/internal/buffer.h\"\n> > #include \"libcamera/internal/camera_sensor.h\"\n> > #include \"libcamera/internal/device_enumerator.h\"\n> > #include \"libcamera/internal/ipa_manager.h\"\n> > @@ -165,6 +166,12 @@ public:\n> > /* Stores the ids of the buffers mapped in the IPA. */\n> > std::unordered_set ipaBuffers_;\n> >\n> > + /*\n> > + * Map of (internal) mmaped embedded data buffers, to avoid having\n> to\n> > + * map/unmap on every frame.\n> > + */\n> > + std::map mappedEmbeddedBuffers_;\n> > +\n> > /* DMAHEAP allocation helper. */\n> > RPi::DmaHeap dmaHeap_;\n> > FileDescriptor lsTable_;\n> > @@ -1040,6 +1047,13 @@ int PipelineHandlerRPi::prepareBuffers(Camera\n> *camera)\n> > return ret;\n> > }\n> >\n> > + if (!data->sensorMetadata_) {\n> > + for (auto const &it :\n> data->unicam_[Unicam::Embedded].getBuffers()) {\n> > + data->mappedEmbeddedBuffers_.emplace(it.first,\n> > +\n> MappedFrameBuffer(it.second, PROT_READ | PROT_WRITE));\n>\n> That's a fairly long line, could we write\n>\n> MappedFrameBuffer fb(it.second, PROT_READ |\n> PROT_WRITE);\n> data->mappedEmbeddedBuffers_.emplace(it.first,\n> std::move(fb));\n>\n> Alternatively, if you want to avoid the move construction of the\n> element, you could write\n>\n>\n> data->mappedEmbeddedBuffers_.emplace(std::piecewise_construct,\n>\n> std::forward_as_tuple(it.first),\n>\n> std::forward_as_tuple(it.second,\n>\n> PROT_READ | PROT_WRITE));\n>\n> > + }\n> > + }\n> > +\n> > /*\n> > * Pass the stats and embedded data buffers to the IPA. No other\n> > * buffers need to be passed.\n> > @@ -1078,6 +1092,7 @@ void PipelineHandlerRPi::freeBuffers(Camera\n> *camera)\n> > std::vector ipaBuffers(data->ipaBuffers_.begin(),\n> data->ipaBuffers_.end());\n> > data->ipa_->unmapBuffers(ipaBuffers);\n> > data->ipaBuffers_.clear();\n> > + data->mappedEmbeddedBuffers_.clear();\n> >\n> > for (auto const stream : data->streams_)\n> > stream->releaseBuffers();\n> > @@ -1310,14 +1325,16 @@ void\n> RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)\n> > * metadata buffer.\n> > */\n> > if (!sensorMetadata_) {\n> > - const FrameBuffer &fb = buffer->planes();\n> > - uint32_t *mem = static_cast *>(::mmap(nullptr, fb.planes()[0].length,\n> > -\n> PROT_READ | PROT_WRITE,\n> > -\n> MAP_SHARED,\n> > -\n> fb.planes()[0].fd.fd(), 0));\n> > - mem[0] = ctrl[V4L2_CID_EXPOSURE];\n> > - mem[1] = ctrl[V4L2_CID_ANALOGUE_GAIN];\n> > - munmap(mem, fb.planes()[0].length);\n> > + unsigned int bufferId =\n> unicam_[Unicam::Embedded].getBufferId(buffer);\n> > + auto it = mappedEmbeddedBuffers_.find(bufferId);\n> > + if (it != mappedEmbeddedBuffers_.end()) {\n> > + uint32_t *mem = reinterpret_cast *>(it->second.maps()[0].data());\n> > + mem[0] = ctrl[V4L2_CID_EXPOSURE];\n> > + mem[1] = ctrl[V4L2_CID_ANALOGUE_GAIN];\n> > + } else {\n> > + LOG(RPI, Warning) << \"Failed to find\n> embedded buffer \"\n> > + << bufferId;\n> > + }\n>\n> Out of scope for this series, but I wonder if we shouldn't improve this\n> with a cleaner API to the IPA instead of faking an embedded data buffer.\n>\n> Reviewed-by: Laurent Pinchart \n>\n> > }\n> > }\n> >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 CB71BBE082\n\tfor ;\n\tMon, 16 Nov 2020 12:34:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 50FE4632C5;\n\tMon, 16 Nov 2020 13:34:37 +0100 (CET)","from mail-lj1-x22d.google.com (mail-lj1-x22d.google.com\n\t[IPv6:2a00:1450:4864:20::22d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9D9C5631B5\n\tfor ;\n\tMon, 16 Nov 2020 13:34:35 +0100 (CET)","by mail-lj1-x22d.google.com with SMTP id 142so6284290ljj.10\n\tfor ;\n\tMon, 16 Nov 2020 04:34:35 -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=\"OkF9hFDn\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=bfW42hYc32WOSHIQlXMmOSjvyjlvs3NyN4CkrLn/oAc=;\n\tb=OkF9hFDn5FiNYl2XeV72Zi4+9Z/NaegEe7U2TRO2exteUK5Cqp1dCOek/KAhbHxWE7\n\t/eLi/JAlkNpR+cym6zrZAiELgbAgb0QePVmWUPFpPee1G5tt8W2VfIxJecUp+rLymSrs\n\t5GAG8uVLs8/A/bUHS8Y3XNKks/lBL2/BJPJ7GnjDbCtafOO+dBRv0u2EYAkk4hQ6l5i4\n\tjdOms55TXulbFP0eWYNgBWcLCL/sGMUETJzR5MQPW/NNG/la7Q9e4/l2tHFzBd6/ueit\n\t5NMt2HSDVXEijtEp5lc5gaknTORezMCc45piWZQlxA1CKfqDyBya4pLuyQv4CImem3gG\n\tTkWw==","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=bfW42hYc32WOSHIQlXMmOSjvyjlvs3NyN4CkrLn/oAc=;\n\tb=K/wFBJgrUySdt6uS2kGqJ6ToWke7It4QoziUDM+QpOa6zUOj8E1Qm2oQ/2/KbdqAIk\n\tHlH8juwvoUlYqFcJcyl8SyRxi13kDTKjOxMMvHiXO6VPgS05AJpEIImCqrVQXBf2x/OU\n\t6rOFFAdLhe1DiYJzpkZCuoqcqVYYFrmr5MMqjzBIK0JoQifnV4yN8aweB8YEVkVAswCz\n\tawYw/GkrElG8Qf37rpC4OG5lPIu3PXdpmNxJU6Pm0zPmw9JBbCShQqXBD0IkQtGISErb\n\tvGnI0wGynJ2jdRye0LYeIrEj1WozjmupkM8UYBGRcNoxV0Fkbf9rtA3+joB1FesxqKJM\n\tS2fw==","X-Gm-Message-State":"AOAM531LBq2cd5yIZhNN2Yq2z1FT23OMpj2cAVbNGQHBDuRP3/CDmZr6\n\tKb2QpeaiOm2v28aPr5nmIh13A+WkclHOO7LEAvE/r/9b5aw=","X-Google-Smtp-Source":"ABdhPJwbKqgfuk4SDE6yLZVpYqq0SKV8p3fWfN5mcEy6hqGYj2XHjhhnfdH13Suq+SJjCkntPvIRM54xjCMLL761oLA=","X-Received":"by 2002:a05:651c:1b7:: with SMTP id\n\tc23mr5718374ljn.112.1605530074703; \n\tMon, 16 Nov 2020 04:34:34 -0800 (PST)","MIME-Version":"1.0","References":"<20201116112458.148477-1-naush@raspberrypi.com>\n\t<20201116122808.GH6540@pendragon.ideasonboard.com>","In-Reply-To":"<20201116122808.GH6540@pendragon.ideasonboard.com>","From":"Naushir Patuck ","Date":"Mon, 16 Nov 2020 12:34:16 +0000","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v2 1/2] pipeline: raspberrypi: Use\n\tMappedFrameBuffer for embedded data buffers","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"multipart/mixed;\n\tboundary=\"===============4061224058815890828==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]