[{"id":18882,"web_url":"https://patchwork.libcamera.org/comment/18882/","msgid":"","date":"2021-08-17T23:46:25","subject":"Re: [libcamera-devel] [RFC PATCH 03/10] libcamera:\n\tmapped_framebuffer: Return plane begin address by\n\tMappedBuffer::maps()","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 16, 2021 at 01:31:31PM +0900, Hirokazu Honda wrote:\n> MappedBuffer::maps() returns std::vector.\n> Plane has the address, but the address points the beginning of the\n> buffer containing the plane.\n> This makes the Plane point the beginning of the plane. So\n> MappedBuffer::maps()[i].data() returns the address of i-th plane.\n> \n> Signed-off-by: Hirokazu Honda \n> ---\n> .../libcamera/internal/mapped_framebuffer.h | 4 +-\n> src/libcamera/mapped_framebuffer.cpp | 51 +++++++++++++++----\n> 2 files changed, 44 insertions(+), 11 deletions(-)\n> \n> diff --git a/include/libcamera/internal/mapped_framebuffer.h b/include/libcamera/internal/mapped_framebuffer.h\n> index 3401a9fc..42479541 100644\n> --- a/include/libcamera/internal/mapped_framebuffer.h\n> +++ b/include/libcamera/internal/mapped_framebuffer.h\n> @@ -30,12 +30,14 @@ public:\n> \n> \tbool isValid() const { return error_ == 0; }\n> \tint error() const { return error_; }\n> -\tconst std::vector &maps() const { return maps_; }\n> +\t/* \\todo rename to planes(). */\n\nI'm fine with a todo comment in this patch, but could you do the rename\nas part of the series, maybe as a last patch on top ?\n\n> +\tconst std::vector &maps() const { return planes_; }\n> \n> protected:\n> \tMappedBuffer();\n> \n> \tint error_;\n> +\tstd::vector planes_;\n> \tstd::vector maps_;\n> \n> private:\n> diff --git a/src/libcamera/mapped_framebuffer.cpp b/src/libcamera/mapped_framebuffer.cpp\n> index 2ebe9fdb..b0ba89b0 100644\n> --- a/src/libcamera/mapped_framebuffer.cpp\n> +++ b/src/libcamera/mapped_framebuffer.cpp\n> @@ -9,6 +9,7 @@\n> \n> #include \n> #include \n> +#include \n> \n> #include \n> \n> @@ -79,6 +80,7 @@ MappedBuffer::MappedBuffer(MappedBuffer &&other)\n> MappedBuffer &MappedBuffer::operator=(MappedBuffer &&other)\n> {\n> \terror_ = other.error_;\n> +\tplanes_ = std::move(other.planes_);\n> \tmaps_ = std::move(other.maps_);\n> \tother.error_ = -ENOENT;\n> \n> @@ -127,10 +129,18 @@ MappedBuffer::~MappedBuffer()\n> */\n> \n> /**\n> - * \\var MappedBuffer::maps_\n> + * \\var MappedBuffer::planes_\n> * \\brief Stores the internal mapped planes\n> *\n> * MappedBuffer derived classes shall store the mappings they create in this\n> + * vector which points the beginning of mapped plane addresses.\n> + */\n> +\n> +/**\n> + * \\var MappedBuffer::maps_\n> + * \\brief Stores the mapped buffer\n> + *\n> + * MappedBuffer derived classes shall store the mappings they create in this\n> * vector which is parsed during destruct to unmap any memory mappings which\n> * completed successfully.\n> */\n> @@ -167,7 +177,8 @@ MappedBuffer::~MappedBuffer()\n> */\n> MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags)\n> {\n> -\tmaps_.reserve(buffer->planes().size());\n> +\tASSERT(!buffer->planes().empty());\n> +\tplanes_.reserve(buffer->planes().size());\n> \n> \tint mmapFlags = 0;\n> \n> @@ -177,18 +188,38 @@ MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags)\n> \tif (flags & MapFlag::Write)\n> \t\tmmapFlags |= PROT_WRITE;\n> \n> +\tint prevFd = -1;\n> \tfor (const FrameBuffer::Plane &plane : buffer->planes()) {\n> -\t\tvoid *address = mmap(nullptr, plane.length, mmapFlags,\n> -\t\t\t\t MAP_SHARED, plane.fd.fd(), 0);\n> -\t\tif (address == MAP_FAILED) {\n> -\t\t\terror_ = -errno;\n> -\t\t\tLOG(Buffer, Error) << \"Failed to mmap plane: \"\n> -\t\t\t\t\t << strerror(-error_);\n> -\t\t\tbreak;\n> +\t\tconst int fd = plane.fd.fd();\n> +\t\tif (prevFd != fd) {\n\nCould there be a case where plane 1 and 3 share the same dmabuf but\nplane 2 uses a different once ? It may be a bit far-fetched, but how\nabout turning the maps_ vector into a std::map ? That way\nwe'll correctly support this case, with no overhead and an\nimplementation that shouldn't be more complicated.\n\n> +\t\t\tconst size_t length = lseek(fd, 0, SEEK_END);\n> +\t\t\tvoid *address = mmap(nullptr, length, mmapFlags,\n> +\t\t\t\t\t MAP_SHARED, fd, 0);\n\nHmmm... I could imagine that in some case the dmabuf would be larger\nthan the sum of the planes, and it may then be possible to only map part\nof the dmabuf. Let's consider this for a possible future enhancement, as\nvirtual address space should hopefully not be a limiting factor in most\ncase. Maybe a todo comment would be appropriate ?\n\n\t\t\t/**\n\t\t\t * \\todo Should we try to only map the portions of the\n\t\t\t * dmabuf that are used by planes ?\n\t\t\t */\n\n> +\t\t\tif (address == MAP_FAILED) {\n> +\t\t\t\terror_ = -errno;\n> +\t\t\t\tLOG(Buffer, Error) << \"Failed to mmap plane: \"\n> +\t\t\t\t\t\t << strerror(-error_);\n> +\t\t\t\treturn;\n> +\t\t\t}\n> +\t\t\tmaps_.emplace_back(static_cast(address),\n> +\t\t\t\t\t length);\n> +\t\t\tprevFd = fd;\n> \t\t}\n> \n> -\t\tmaps_.emplace_back(static_cast(address), plane.length);\n> +\t\tconst size_t length = maps_.back().size();\n> +\t\tif (plane.offset + plane.length > length) {\n\nShould we protect ourselves against arithmetic overflows here ?\n\n\t\tif (plane.offset > length ||\n\t\t plane.offset + plane.length > length) {\n\n> +\t\t\tLOG(Buffer, Fatal) << \"plane is out of buffer: \"\n> +\t\t\t\t\t << \"buffer length=\" << length\n> +\t\t\t\t\t << \", plane offset=\" << plane.offset\n> +\t\t\t\t\t << \", plane length=\" << plane.length;\n> +\t\t\treturn;\n> +\t\t}\n> +\n> +\t\tuint8_t *buf = maps_.back().data();\n> +\t\tplanes_.emplace_back(buf + plane.offset, plane.length);\n> \t}\n> +\n> +\tASSERT(maps_.size() == 1u);\n\nI'm not sure to understand this. Won't a multi-planar frame buffer with\ndifferent dmabufs fail this assertion ?\n\n> }\n> \n> } /* namespace libcamera */","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 7C535BD87D\n\tfor ;\n\tTue, 17 Aug 2021 23:46:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id ED7CA68894;\n\tWed, 18 Aug 2021 01:46:34 +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 7A2A16025D\n\tfor ;\n\tWed, 18 Aug 2021 01:46:33 +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 E8F92499;\n\tWed, 18 Aug 2021 01:46:32 +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=\"U3eiiF3j\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629243993;\n\tbh=cTqLLolq1I/EhEQZzNGNC/TGBW8IX/084PtzqnP48g4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=U3eiiF3jmHk+T3I9DrauszUIxubnR1sd2TZbTZ/LmTp6nmXerzNA7NAvTCy4VCWBN\n\tpf7whq2+Oki72OvEVni7Kt6sRS2UxCLfj6iH9V6KkMWrKA8XRnpLzJlnVzKngaI1Bw\n\tgqiqQU3Isxj95xtD2D2891K00qG9nNV5ODboqCLY=","Date":"Wed, 18 Aug 2021 02:46:25 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210816043138.957984-1-hiroh@chromium.org>\n\t<20210816043138.957984-4-hiroh@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210816043138.957984-4-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [RFC PATCH 03/10] libcamera:\n\tmapped_framebuffer: Return plane begin address by\n\tMappedBuffer::maps()","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":18966,"web_url":"https://patchwork.libcamera.org/comment/18966/","msgid":"","date":"2021-08-20T08:19:59","subject":"Re: [libcamera-devel] [RFC PATCH 03/10] libcamera:\n\tmapped_framebuffer: Return plane begin address by\n\tMappedBuffer::maps()","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent, thank you for the comment.\n\nOn Wed, Aug 18, 2021 at 8:46 AM Laurent Pinchart\n wrote:\n>\n> Hi Hiro,\n>\n> Thank you for the patch.\n>\n> On Mon, Aug 16, 2021 at 01:31:31PM +0900, Hirokazu Honda wrote:\n> > MappedBuffer::maps() returns std::vector.\n> > Plane has the address, but the address points the beginning of the\n> > buffer containing the plane.\n> > This makes the Plane point the beginning of the plane. So\n> > MappedBuffer::maps()[i].data() returns the address of i-th plane.\n> >\n> > Signed-off-by: Hirokazu Honda \n> > ---\n> > .../libcamera/internal/mapped_framebuffer.h | 4 +-\n> > src/libcamera/mapped_framebuffer.cpp | 51 +++++++++++++++----\n> > 2 files changed, 44 insertions(+), 11 deletions(-)\n> >\n> > diff --git a/include/libcamera/internal/mapped_framebuffer.h b/include/libcamera/internal/mapped_framebuffer.h\n> > index 3401a9fc..42479541 100644\n> > --- a/include/libcamera/internal/mapped_framebuffer.h\n> > +++ b/include/libcamera/internal/mapped_framebuffer.h\n> > @@ -30,12 +30,14 @@ public:\n> >\n> > bool isValid() const { return error_ == 0; }\n> > int error() const { return error_; }\n> > - const std::vector &maps() const { return maps_; }\n> > + /* \\todo rename to planes(). */\n>\n> I'm fine with a todo comment in this patch, but could you do the rename\n> as part of the series, maybe as a last patch on top ?\n>\n> > + const std::vector &maps() const { return planes_; }\n> >\n> > protected:\n> > MappedBuffer();\n> >\n> > int error_;\n> > + std::vector planes_;\n> > std::vector maps_;\n> >\n> > private:\n> > diff --git a/src/libcamera/mapped_framebuffer.cpp b/src/libcamera/mapped_framebuffer.cpp\n> > index 2ebe9fdb..b0ba89b0 100644\n> > --- a/src/libcamera/mapped_framebuffer.cpp\n> > +++ b/src/libcamera/mapped_framebuffer.cpp\n> > @@ -9,6 +9,7 @@\n> >\n> > #include \n> > #include \n> > +#include \n> >\n> > #include \n> >\n> > @@ -79,6 +80,7 @@ MappedBuffer::MappedBuffer(MappedBuffer &&other)\n> > MappedBuffer &MappedBuffer::operator=(MappedBuffer &&other)\n> > {\n> > error_ = other.error_;\n> > + planes_ = std::move(other.planes_);\n> > maps_ = std::move(other.maps_);\n> > other.error_ = -ENOENT;\n> >\n> > @@ -127,10 +129,18 @@ MappedBuffer::~MappedBuffer()\n> > */\n> >\n> > /**\n> > - * \\var MappedBuffer::maps_\n> > + * \\var MappedBuffer::planes_\n> > * \\brief Stores the internal mapped planes\n> > *\n> > * MappedBuffer derived classes shall store the mappings they create in this\n> > + * vector which points the beginning of mapped plane addresses.\n> > + */\n> > +\n> > +/**\n> > + * \\var MappedBuffer::maps_\n> > + * \\brief Stores the mapped buffer\n> > + *\n> > + * MappedBuffer derived classes shall store the mappings they create in this\n> > * vector which is parsed during destruct to unmap any memory mappings which\n> > * completed successfully.\n> > */\n> > @@ -167,7 +177,8 @@ MappedBuffer::~MappedBuffer()\n> > */\n> > MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags)\n> > {\n> > - maps_.reserve(buffer->planes().size());\n> > + ASSERT(!buffer->planes().empty());\n> > + planes_.reserve(buffer->planes().size());\n> >\n> > int mmapFlags = 0;\n> >\n> > @@ -177,18 +188,38 @@ MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags)\n> > if (flags & MapFlag::Write)\n> > mmapFlags |= PROT_WRITE;\n> >\n> > + int prevFd = -1;\n> > for (const FrameBuffer::Plane &plane : buffer->planes()) {\n> > - void *address = mmap(nullptr, plane.length, mmapFlags,\n> > - MAP_SHARED, plane.fd.fd(), 0);\n> > - if (address == MAP_FAILED) {\n> > - error_ = -errno;\n> > - LOG(Buffer, Error) << \"Failed to mmap plane: \"\n> > - << strerror(-error_);\n> > - break;\n> > + const int fd = plane.fd.fd();\n> > + if (prevFd != fd) {\n>\n> Could there be a case where plane 1 and 3 share the same dmabuf but\n> plane 2 uses a different once ? It may be a bit far-fetched, but how\n> about turning the maps_ vector into a std::map ? That way\n> we'll correctly support this case, with no overhead and an\n> implementation that shouldn't be more complicated.\n>\n> > + const size_t length = lseek(fd, 0, SEEK_END);\n> > + void *address = mmap(nullptr, length, mmapFlags,\n> > + MAP_SHARED, fd, 0);\n>\n> Hmmm... I could imagine that in some case the dmabuf would be larger\n> than the sum of the planes, and it may then be possible to only map part\n> of the dmabuf. Let's consider this for a possible future enhancement, as\n> virtual address space should hopefully not be a limiting factor in most\n> case. Maybe a todo comment would be appropriate ?\n>\n> /**\n> * \\todo Should we try to only map the portions of the\n> * dmabuf that are used by planes ?\n> */\n>\n> > + if (address == MAP_FAILED) {\n> > + error_ = -errno;\n> > + LOG(Buffer, Error) << \"Failed to mmap plane: \"\n> > + << strerror(-error_);\n> > + return;\n> > + }\n> > + maps_.emplace_back(static_cast(address),\n> > + length);\n> > + prevFd = fd;\n> > }\n> >\n> > - maps_.emplace_back(static_cast(address), plane.length);\n> > + const size_t length = maps_.back().size();\n> > + if (plane.offset + plane.length > length) {\n>\n> Should we protect ourselves against arithmetic overflows here ?\n>\n> if (plane.offset > length ||\n> plane.offset + plane.length > length) {\n>\n\nThanks. By the way, ideally we should introduce something like\nbase/numerics in chromium.\nhttps://source.chromium.org/chromium/chromium/src/+/main:base/numerics/README.md\n\nSo I can do like\nsize_t planeEnd;\nif (!CheckAdd(plane.offset, plane.length).AssignIfValid(planeEnd)) {\n // Overflow!\n}\nOr more aggressively if (base::checked_cast(plane.offset,\nplane.length) > length), which causes process crash if overflow\nhappens.\n\n> > + LOG(Buffer, Fatal) << \"plane is out of buffer: \"\n> > + << \"buffer length=\" << length\n> > + << \", plane offset=\" << plane.offset\n> > + << \", plane length=\" << plane.length;\n> > + return;\n> > + }\n> > +\n> > + uint8_t *buf = maps_.back().data();\n> > + planes_.emplace_back(buf + plane.offset, plane.length);\n> > }\n> > +\n> > + ASSERT(maps_.size() == 1u);\n>\n> I'm not sure to understand this. Won't a multi-planar frame buffer with\n> different dmabufs fail this assertion ?\n>\n\nRight, this is brought from your comment to my previous solution.\nWe should remove now.\n\nBest Regards,\n-Hiro\n\n> > }\n> >\n> > } /* namespace libcamera */\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 7EA19BD87C\n\tfor ;\n\tFri, 20 Aug 2021 08:20:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E0A9568891;\n\tFri, 20 Aug 2021 10:20:13 +0200 (CEST)","from mail-ed1-x534.google.com (mail-ed1-x534.google.com\n\t[IPv6:2a00:1450:4864:20::534])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E1F0968890\n\tfor ;\n\tFri, 20 Aug 2021 10:20:11 +0200 (CEST)","by mail-ed1-x534.google.com with SMTP id r19so12774759eds.13\n\tfor ;\n\tFri, 20 Aug 2021 01:20:11 -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=\"E9ZEGSKu\"; 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=7YYOjvRM+VHXCHjEUEGbApEjr2mhiH5ljjdHBFvkT0I=;\n\tb=E9ZEGSKursljvasdv4g58Ylkfcr7Jn6I5tljpljy+52MsZPPNAxu19NxkLwCU4l+fY\n\tc140SR5SJdOMUJTfr93JPoUI9FdWVxnhXd3IcjZZUeRgjCp5uKFl0ORJCGuT7KroIdmY\n\tQfA7SkoxEg/7ko+EWLXhd2F3b0zYY0G6pBoHM=","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=7YYOjvRM+VHXCHjEUEGbApEjr2mhiH5ljjdHBFvkT0I=;\n\tb=C3eJU65tDz2qDYnA/c3lnt1TpIlv8nXL27P9ewepIu/3aLh3h/k3cUfY+idDI6wYUN\n\taSnRz2N9bl4vp3j4+EiaB8OrFqytDnaSKuz5ltwbrqp5U88+1pxl+Y75qK2r3h58UJCq\n\tGCqS9RDuokaYrWtblWaNcdHM6WdxZiZKpliLQfCumTi3bM8VlHpRe60dL7TdlMi3iOJo\n\t0YtAD+P4c73w3wjPoxD2ddj4MwYzGUjGSr+1MMscJqjOVMlLcPXGeZfrILw5F5KtP4e8\n\tPdE3TVaGAI4g/1loJcxavrdlmNtWRSqE6vpWl6d23eRO2r3SvWJExc7wbQ1RlL00ntfE\n\tejqg==","X-Gm-Message-State":"AOAM5300RN1qi3k6mo3m0AMhRcf6YXGNe7+s59YEkkWegR2L9e7MLpWP\n\tJvNyEy6OtLcXL/kL7wRo94qaPmhJrn+AxDXXAGMY1w==","X-Google-Smtp-Source":"ABdhPJyD6UenPUWVuYFcCgsz6GF2mP8URjT3XSY6Ja42QRlXEoPRcIIHE2f7YIFB7qOHn2aYEVyzlZWlVIpGzb2AyWA=","X-Received":"by 2002:a05:6402:202:: with SMTP id\n\tt2mr21085416edv.116.1629447611493; \n\tFri, 20 Aug 2021 01:20:11 -0700 (PDT)","MIME-Version":"1.0","References":"<20210816043138.957984-1-hiroh@chromium.org>\n\t<20210816043138.957984-4-hiroh@chromium.org>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Fri, 20 Aug 2021 17:19:59 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [RFC PATCH 03/10] libcamera:\n\tmapped_framebuffer: Return plane begin address by\n\tMappedBuffer::maps()","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":18978,"web_url":"https://patchwork.libcamera.org/comment/18978/","msgid":"","date":"2021-08-20T12:10:21","subject":"Re: [libcamera-devel] [RFC PATCH 03/10] libcamera:\n\tmapped_framebuffer: Return plane begin address by\n\tMappedBuffer::maps()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nOn Fri, Aug 20, 2021 at 05:19:59PM +0900, Hirokazu Honda wrote:\n> On Wed, Aug 18, 2021 at 8:46 AM Laurent Pinchart wrote:\n> > On Mon, Aug 16, 2021 at 01:31:31PM +0900, Hirokazu Honda wrote:\n> > > MappedBuffer::maps() returns std::vector.\n> > > Plane has the address, but the address points the beginning of the\n> > > buffer containing the plane.\n> > > This makes the Plane point the beginning of the plane. So\n> > > MappedBuffer::maps()[i].data() returns the address of i-th plane.\n> > >\n> > > Signed-off-by: Hirokazu Honda \n> > > ---\n> > > .../libcamera/internal/mapped_framebuffer.h | 4 +-\n> > > src/libcamera/mapped_framebuffer.cpp | 51 +++++++++++++++----\n> > > 2 files changed, 44 insertions(+), 11 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/internal/mapped_framebuffer.h b/include/libcamera/internal/mapped_framebuffer.h\n> > > index 3401a9fc..42479541 100644\n> > > --- a/include/libcamera/internal/mapped_framebuffer.h\n> > > +++ b/include/libcamera/internal/mapped_framebuffer.h\n> > > @@ -30,12 +30,14 @@ public:\n> > >\n> > > bool isValid() const { return error_ == 0; }\n> > > int error() const { return error_; }\n> > > - const std::vector &maps() const { return maps_; }\n> > > + /* \\todo rename to planes(). */\n> >\n> > I'm fine with a todo comment in this patch, but could you do the rename\n> > as part of the series, maybe as a last patch on top ?\n> >\n> > > + const std::vector &maps() const { return planes_; }\n> > >\n> > > protected:\n> > > MappedBuffer();\n> > >\n> > > int error_;\n> > > + std::vector planes_;\n> > > std::vector maps_;\n> > >\n> > > private:\n> > > diff --git a/src/libcamera/mapped_framebuffer.cpp b/src/libcamera/mapped_framebuffer.cpp\n> > > index 2ebe9fdb..b0ba89b0 100644\n> > > --- a/src/libcamera/mapped_framebuffer.cpp\n> > > +++ b/src/libcamera/mapped_framebuffer.cpp\n> > > @@ -9,6 +9,7 @@\n> > >\n> > > #include \n> > > #include \n> > > +#include \n> > >\n> > > #include \n> > >\n> > > @@ -79,6 +80,7 @@ MappedBuffer::MappedBuffer(MappedBuffer &&other)\n> > > MappedBuffer &MappedBuffer::operator=(MappedBuffer &&other)\n> > > {\n> > > error_ = other.error_;\n> > > + planes_ = std::move(other.planes_);\n> > > maps_ = std::move(other.maps_);\n> > > other.error_ = -ENOENT;\n> > >\n> > > @@ -127,10 +129,18 @@ MappedBuffer::~MappedBuffer()\n> > > */\n> > >\n> > > /**\n> > > - * \\var MappedBuffer::maps_\n> > > + * \\var MappedBuffer::planes_\n> > > * \\brief Stores the internal mapped planes\n> > > *\n> > > * MappedBuffer derived classes shall store the mappings they create in this\n> > > + * vector which points the beginning of mapped plane addresses.\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\var MappedBuffer::maps_\n> > > + * \\brief Stores the mapped buffer\n> > > + *\n> > > + * MappedBuffer derived classes shall store the mappings they create in this\n> > > * vector which is parsed during destruct to unmap any memory mappings which\n> > > * completed successfully.\n> > > */\n> > > @@ -167,7 +177,8 @@ MappedBuffer::~MappedBuffer()\n> > > */\n> > > MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags)\n> > > {\n> > > - maps_.reserve(buffer->planes().size());\n> > > + ASSERT(!buffer->planes().empty());\n> > > + planes_.reserve(buffer->planes().size());\n> > >\n> > > int mmapFlags = 0;\n> > >\n> > > @@ -177,18 +188,38 @@ MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags)\n> > > if (flags & MapFlag::Write)\n> > > mmapFlags |= PROT_WRITE;\n> > >\n> > > + int prevFd = -1;\n> > > for (const FrameBuffer::Plane &plane : buffer->planes()) {\n> > > - void *address = mmap(nullptr, plane.length, mmapFlags,\n> > > - MAP_SHARED, plane.fd.fd(), 0);\n> > > - if (address == MAP_FAILED) {\n> > > - error_ = -errno;\n> > > - LOG(Buffer, Error) << \"Failed to mmap plane: \"\n> > > - << strerror(-error_);\n> > > - break;\n> > > + const int fd = plane.fd.fd();\n> > > + if (prevFd != fd) {\n> >\n> > Could there be a case where plane 1 and 3 share the same dmabuf but\n> > plane 2 uses a different once ? It may be a bit far-fetched, but how\n> > about turning the maps_ vector into a std::map ? That way\n> > we'll correctly support this case, with no overhead and an\n> > implementation that shouldn't be more complicated.\n> >\n> > > + const size_t length = lseek(fd, 0, SEEK_END);\n> > > + void *address = mmap(nullptr, length, mmapFlags,\n> > > + MAP_SHARED, fd, 0);\n> >\n> > Hmmm... I could imagine that in some case the dmabuf would be larger\n> > than the sum of the planes, and it may then be possible to only map part\n> > of the dmabuf. Let's consider this for a possible future enhancement, as\n> > virtual address space should hopefully not be a limiting factor in most\n> > case. Maybe a todo comment would be appropriate ?\n> >\n> > /**\n> > * \\todo Should we try to only map the portions of the\n> > * dmabuf that are used by planes ?\n> > */\n> >\n> > > + if (address == MAP_FAILED) {\n> > > + error_ = -errno;\n> > > + LOG(Buffer, Error) << \"Failed to mmap plane: \"\n> > > + << strerror(-error_);\n> > > + return;\n> > > + }\n> > > + maps_.emplace_back(static_cast(address),\n> > > + length);\n> > > + prevFd = fd;\n> > > }\n> > >\n> > > - maps_.emplace_back(static_cast(address), plane.length);\n> > > + const size_t length = maps_.back().size();\n> > > + if (plane.offset + plane.length > length) {\n> >\n> > Should we protect ourselves against arithmetic overflows here ?\n> >\n> > if (plane.offset > length ||\n> > plane.offset + plane.length > length) {\n> >\n> \n> Thanks. By the way, ideally we should introduce something like\n> base/numerics in chromium.\n> https://source.chromium.org/chromium/chromium/src/+/main:base/numerics/README.md\n\nI was actually thinking about adding helpers to utils.h for this when\nreviewing your patch :-) I think we'll end up doing so at some point.\n\n> So I can do like\n> size_t planeEnd;\n> if (!CheckAdd(plane.offset, plane.length).AssignIfValid(planeEnd)) {\n> // Overflow!\n> }\n> Or more aggressively if (base::checked_cast(plane.offset,\n> plane.length) > length), which causes process crash if overflow\n> happens.\n> \n> > > + LOG(Buffer, Fatal) << \"plane is out of buffer: \"\n> > > + << \"buffer length=\" << length\n> > > + << \", plane offset=\" << plane.offset\n> > > + << \", plane length=\" << plane.length;\n> > > + return;\n> > > + }\n> > > +\n> > > + uint8_t *buf = maps_.back().data();\n> > > + planes_.emplace_back(buf + plane.offset, plane.length);\n> > > }\n> > > +\n> > > + ASSERT(maps_.size() == 1u);\n> >\n> > I'm not sure to understand this. Won't a multi-planar frame buffer with\n> > different dmabufs fail this assertion ?\n> \n> Right, this is brought from your comment to my previous solution.\n> We should remove now.\n> \n> > > }\n> > >\n> > > } /* namespace libcamera */","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 3B81BBD87D\n\tfor ;\n\tFri, 20 Aug 2021 12:10:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5FD0468895;\n\tFri, 20 Aug 2021 14:10:32 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9A4DE6025E\n\tfor ;\n\tFri, 20 Aug 2021 14:10:30 +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 2BB468C8;\n\tFri, 20 Aug 2021 14:10:30 +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=\"FiLpw+ds\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629461430;\n\tbh=W5zndrVrjamwX+qc6CgJwE6Vao9aI+e5YcFxbfcWHSU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=FiLpw+dsP7z6rURQakxh93jjHxlJvnSpJIX3SLw3n5PjgTkXjzkTiDS+rcMcngJiN\n\tSu+1py3RwL8QcLXHtdmnu7cnUif26t15VFqCtv0Zw9E8Q+SyEs2IUaBeIONHQxlC39\n\txQfYsDuJ/siTPQztHPcEmQxiJWRDgnUgjXFsebq8=","Date":"Fri, 20 Aug 2021 15:10:21 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210816043138.957984-1-hiroh@chromium.org>\n\t<20210816043138.957984-4-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 03/10] libcamera:\n\tmapped_framebuffer: Return plane begin address by\n\tMappedBuffer::maps()","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\" "}}]