[{"id":27663,"web_url":"https://patchwork.libcamera.org/comment/27663/","msgid":"<169222015737.695434.4262081504831307329@ping.linuxembedded.co.uk>","date":"2023-08-16T21:09:17","subject":"Re: [libcamera-devel] [RFC PATCH 2/5] libcamera: MappedFrameBuffer:\n\tUse stored plane offset","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Gabby George (2023-08-14 12:28:46)\n> When calling mmap, the mapped frame buffer class hard-codes the offset value to 0. TODO: EDIT ME WHEN YOU HAVE MORE VERBAL REASONING SKILLS\n\nInteresting TODO item ;-)\n\n\n\n> \n> Signed-off-by: Gabby George \n> ---\n> include/libcamera/internal/mapped_framebuffer.h | 2 +-\n> src/libcamera/mapped_framebuffer.cpp | 16 ++++++++++++----\n> 2 files changed, 13 insertions(+), 5 deletions(-)\n> \n> diff --git a/include/libcamera/internal/mapped_framebuffer.h b/include/libcamera/internal/mapped_framebuffer.h\n> index fb39adbf..fac86344 100644\n> --- a/include/libcamera/internal/mapped_framebuffer.h\n> +++ b/include/libcamera/internal/mapped_framebuffer.h\n> @@ -54,7 +54,7 @@ public:\n> \n> using MapFlags = Flags;\n> \n> - MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags);\n> + MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags, bool usePlaneOffset = false);\n> };\n> \n> LIBCAMERA_FLAGS_ENABLE_OPERATORS(MappedFrameBuffer::MapFlag)\n> diff --git a/src/libcamera/mapped_framebuffer.cpp b/src/libcamera/mapped_framebuffer.cpp\n> index 6860069b..fcbb38ec 100644\n> --- a/src/libcamera/mapped_framebuffer.cpp\n> +++ b/src/libcamera/mapped_framebuffer.cpp\n> @@ -172,12 +172,13 @@ MappedBuffer::~MappedBuffer()\n> * \\brief Map all planes of a FrameBuffer\n> * \\param[in] buffer FrameBuffer to be mapped\n> * \\param[in] flags Protection flags to apply to map\n> + * \\param[in] usePlaneOffset Use offset stored in buffer's plane; default false\n> *\n> * Construct an object to map a frame buffer for CPU access. The mapping can be\n> * made as Read only, Write only or support Read and Write operations by setting\n> * the MapFlag flags accordingly.\n> */\n> -MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags)\n> +MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags, bool usePlaneOffset)\n> {\n> ASSERT(!buffer->planes().empty());\n> planes_.reserve(buffer->planes().size());\n> @@ -223,8 +224,14 @@ MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags)\n> const int fd = plane.fd.get();\n> auto &info = mappedBuffers[fd];\n> if (!info.address) {\n> - void *address = mmap(nullptr, info.mapLength, mmapFlags,\n> - MAP_SHARED, fd, 0);\n> + void *address;\n> + if (usePlaneOffset) {\n\nI don't think we should hide this behind a flag. We should trust the\noffset in the FrameBuffer. If the offset is not to be set, or rather is\ntruely 0, it should be correctly initialised to 0 whenever a FrameBuffer\nis constructed.\n\nHrm ... seeing the storedAddress calculation below worries me that what\nI've written above might not be true - but I hope we can work through\nand find what uses the offsets either way and make sure they're all\nconsistent. I still hope we don't need to hide behind a flag here.\n\n> + address = mmap(nullptr, plane.length, mmapFlags,\n> + MAP_SHARED, fd, plane.offset);\n> + } else {\n> + address = mmap(nullptr, info.mapLength, mmapFlags,\n> + MAP_SHARED, fd, 0);\n> + }\n> if (address == MAP_FAILED) {\n> error_ = -errno;\n> LOG(Buffer, Error) << \"Failed to mmap plane: \"\n> @@ -236,7 +243,8 @@ MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags)\n> maps_.emplace_back(info.address, info.mapLength);\n> }\n> \n> - planes_.emplace_back(info.address + plane.offset, plane.length);\n> + uint8_t *storedAddress = usePlaneOffset ? info.address : info.address + plane.offset;\n\nHrm ... what's going on here though.\n\nI suspect if we are always mapping with the plane.offset, then here we\nshould no longer 'add' the offset at all...\n\nDid this trigger anything in the unit tests ?\n\nHave you run the unit tests yet? - if not - please run ...\n\n sudo modprobe vimc\n sudo modprobe vivid\n sudo modprobe vim2m\n\nand then from your build directory:\n ninja -C build test\n\nThough now I check, you might need to make sure your libcamera\nconfiguration has testing enabled: \n\nperhaps (if you need it)\n meson --reconfigure build -Dtest=true\n\n\n> + planes_.emplace_back(storedAddress, plane.length);\n> }\n> }\n> \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 F2A13BE08A\n\tfor ;\n\tWed, 16 Aug 2023 21:09:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5E5C3628D2;\n\tWed, 16 Aug 2023 23:09:22 +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 82ECF628D2\n\tfor ;\n\tWed, 16 Aug 2023 23:09:20 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(aztw-30-b2-v4wan-166917-cust845.vm26.cable.virginm.net\n\t[82.37.23.78])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 10F69223;\n\tWed, 16 Aug 2023 23:08:07 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1692220162;\n\tbh=sQ0iX2xrHfZMqkGOaRt1INjy4xvpt97VqtWsnO7mX2A=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=qH6E+UImKXktAiwnkAoV0EuITMLoroEIvpFJkL/JiWH3eHpjEFbitb++Td8cI/dEQ\n\t88HoulvrF/GBtHlHOLCaO0N02oX1JP25eRsd1HmxheiyuzbEYINw08GCRKBOI8UB1B\n\texvAoYSMHstzh+KGX6YBMYshf2ou7K19jOPhoJ8GmLtnj3gug0zomaiSEbgSH//ADz\n\t4M0IhIBJYISGoUjPPqUBbfaFqq5EEzjh9860ZxLtqhghfjGAm94xpoNBKBj+ll1f/J\n\tQVFcZFBN5W/k/v6egv2TDY8q/Z/yMjOwuWCj0/vSbZ8vIO29E4gu5gjaz3JbkFKZvi\n\t3ibC3vsE71PDw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1692220087;\n\tbh=sQ0iX2xrHfZMqkGOaRt1INjy4xvpt97VqtWsnO7mX2A=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=Gnsma0Vs4us5tsHmDEl/T0La0hgZcNXdFJqWQAZg3m+pPfe8Bdwhem5WS2doOZczG\n\tagvRsAKlRI9vcSv2Ida7oeWWfrbRmr6WsyBgACexGxEBeoXuh1bexIjz4HCy1Y39+J\n\tZaX90hod1G6hiFdfQO1/y5yZ/aZy2Di9/HZxSdIc="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Gnsma0Vs\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20230814112849.176943-3-gabbymg94@gmail.com>","References":"<20230814112849.176943-1-gabbymg94@gmail.com>\n\t<20230814112849.176943-3-gabbymg94@gmail.com>","To":"gabbymg94@gmail.com, libcamera-devel@lists.libcamera.org,\n\tvedantparanjape160201@gmail.com","Date":"Wed, 16 Aug 2023 22:09:17 +0100","Message-ID":"<169222015737.695434.4262081504831307329@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [RFC PATCH 2/5] libcamera: MappedFrameBuffer:\n\tUse stored plane offset","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]