[{"id":19092,"web_url":"https://patchwork.libcamera.org/comment/19092/","msgid":"","date":"2021-08-26T12:06:01","subject":"Re: [libcamera-devel] [PATCH v3 2/3] android: camera_buffer: Map\n\tbuffer in the first plane() call","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 Thu, Aug 26, 2021 at 05:00:20PM +0900, Hirokazu Honda wrote:\n> CameraBuffer implementation maps a given buffer_handle_t in\n> constructor. Mapping is redundant to only know the plane info like\n> stride and offset. Mapping should be executed rater in the first\n> plane() call.\n> \n> Signed-off-by: Hirokazu Honda \n> ---\n> src/android/mm/cros_camera_buffer.cpp | 71 +++++++++++--------\n> src/android/mm/generic_camera_buffer.cpp | 86 ++++++++++++++++--------\n> 2 files changed, 100 insertions(+), 57 deletions(-)\n> \n> diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp\n> index 50732637..ba6650cf 100644\n> --- a/src/android/mm/cros_camera_buffer.cpp\n> +++ b/src/android/mm/cros_camera_buffer.cpp\n> @@ -25,7 +25,7 @@ public:\n> \t\tint flags);\n> \t~Private();\n> \n> -\tbool isValid() const { return valid_; }\n> +\tbool isValid() const { return registered_; }\n> \n> \tunsigned int numPlanes() const;\n> \n> @@ -34,10 +34,12 @@ public:\n> \tsize_t jpegBufferSize(size_t maxJpegBufferSize) const;\n> \n> private:\n> +\tvoid map();\n> +\n> \tcros::CameraBufferManager *bufferManager_;\n> \tbuffer_handle_t handle_;\n> \tunsigned int numPlanes_;\n> -\tbool valid_;\n> +\tbool mapped_;\n> \tbool registered_;\n> \tunion {\n> \t\tvoid *addr;\n> @@ -50,7 +52,7 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> \t\t\t [[maybe_unused]] libcamera::PixelFormat pixelFormat,\n> \t\t\t [[maybe_unused]] const libcamera::Size &size,\n> \t\t\t [[maybe_unused]] int flags)\n> -\t: handle_(camera3Buffer), numPlanes_(0), valid_(false),\n> +\t: handle_(camera3Buffer), numPlanes_(0), mapped_(false),\n> \t registered_(false)\n> {\n> \tbufferManager_ = cros::CameraBufferManager::GetInstance();\n> @@ -63,36 +65,11 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> \n> \tregistered_ = true;\n> \tnumPlanes_ = bufferManager_->GetNumPlanes(camera3Buffer);\n> -\tswitch (numPlanes_) {\n> -\tcase 1: {\n> -\t\tret = bufferManager_->Lock(handle_, 0, 0, 0, 0, 0, &mem.addr);\n> -\t\tif (ret) {\n> -\t\t\tLOG(HAL, Error) << \"Single plane buffer mapping failed\";\n> -\t\t\treturn;\n> -\t\t}\n> -\t\tbreak;\n> -\t}\n> -\tcase 2:\n> -\tcase 3: {\n> -\t\tret = bufferManager_->LockYCbCr(handle_, 0, 0, 0, 0, 0,\n> -\t\t\t\t\t\t&mem.ycbcr);\n> -\t\tif (ret) {\n> -\t\t\tLOG(HAL, Error) << \"YCbCr buffer mapping failed\";\n> -\t\t\treturn;\n> -\t\t}\n> -\t\tbreak;\n> -\t}\n> -\tdefault:\n> -\t\tLOG(HAL, Error) << \"Invalid number of planes: \" << numPlanes_;\n> -\t\treturn;\n> -\t}\n> -\n> -\tvalid_ = true;\n> }\n> \n> CameraBuffer::Private::~Private()\n> {\n> -\tif (valid_)\n> +\tif (mapped_)\n> \t\tbufferManager_->Unlock(handle_);\n> \tif (registered_)\n> \t\tbufferManager_->Deregister(handle_);\n> @@ -105,6 +82,11 @@ unsigned int CameraBuffer::Private::numPlanes() const\n> \n> Span CameraBuffer::Private::plane(unsigned int plane)\n> {\n> +\tif (!mapped_)\n> +\t\tmap();\n> +\tif (!mapped_)\n> +\t\treturn {};\n> +\n> \tvoid *addr;\n> \n> \tswitch (numPlanes()) {\n> @@ -134,4 +116,35 @@ size_t CameraBuffer::Private::jpegBufferSize([[maybe_unused]] size_t maxJpegBuff\n> \treturn bufferManager_->GetPlaneSize(handle_, 0);\n> }\n> \n> +void CameraBuffer::Private::map()\n> +{\n> +\tint ret;\n> +\tswitch (numPlanes_) {\n> +\tcase 1: {\n> +\t\tret = bufferManager_->Lock(handle_, 0, 0, 0, 0, 0, &mem.addr);\n> +\t\tif (ret) {\n> +\t\t\tLOG(HAL, Error) << \"Single plane buffer mapping failed\";\n> +\t\t\treturn;\n> +\t\t}\n> +\t\tbreak;\n> +\t}\n> +\tcase 2:\n> +\tcase 3: {\n> +\t\tret = bufferManager_->LockYCbCr(handle_, 0, 0, 0, 0, 0,\n> +\t\t\t\t\t\t&mem.ycbcr);\n> +\t\tif (ret) {\n> +\t\t\tLOG(HAL, Error) << \"YCbCr buffer mapping failed\";\n> +\t\t\treturn;\n> +\t\t}\n> +\t\tbreak;\n> +\t}\n> +\tdefault:\n> +\t\tLOG(HAL, Error) << \"Invalid number of planes: \" << numPlanes_;\n> +\t\treturn;\n> +\t}\n> +\n> +\tmapped_ = true;\n> +\treturn;\n> +}\n> +\n> PUBLIC_CAMERA_BUFFER_IMPLEMENTATION\n> diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp\n> index 22753490..cfe55b69 100644\n> --- a/src/android/mm/generic_camera_buffer.cpp\n> +++ b/src/android/mm/generic_camera_buffer.cpp\n> @@ -37,6 +37,18 @@ public:\n> \tsize_t jpegBufferSize(size_t maxJpegBufferSize) const;\n> \n> private:\n> +\tstruct PlaneInfo {\n> +\t\tunsigned int offset;\n> +\t\tunsigned int size;\n> +\t};\n> +\n> +\tvoid map();\n> +\n> +\tint fd_;\n> +\tint flags_;\n> +\toff_t bufferLength_;\n> +\tbool mapped_;\n> +\tstd::vector planeInfo_;\n> \t/* \\todo Remove planes_ when it will be added to MappedBuffer */\n> \tstd::vector> planes_;\n> };\n> @@ -45,6 +57,7 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> \t\t\t buffer_handle_t camera3Buffer,\n> \t\t\t libcamera::PixelFormat pixelFormat,\n> \t\t\t const libcamera::Size &size, int flags)\n> +\t: fd_(-1), flags_(flags), mapped_(false)\n\nThis should initialize bufferLength_ to 0, otherwise the ASSERT()s that\nchecks the variable will be pointless if the constructor returns an\nerror before setting bufferLength_.\n\nAnd it will also avoid a Coverity warning :-)\n\nI can handle this when pushing the series.\n\nReviewed-by: Laurent Pinchart \n\n> {\n> \terror_ = 0;\n> \n> @@ -61,43 +74,35 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> \t * now that the buffer is backed by a single dmabuf, with planes being\n> \t * stored contiguously.\n> \t */\n> -\tint fd = -1;\n> \tfor (int i = 0; i < camera3Buffer->numFds; i++) {\n> -\t\tif (camera3Buffer->data[i] == -1 || camera3Buffer->data[i] == fd)\n> +\t\tif (camera3Buffer->data[i] == -1 || camera3Buffer->data[i] == fd_)\n> \t\t\tcontinue;\n> \n> -\t\tif (fd != -1) {\n> +\t\tif (fd_ != -1) {\n> \t\t\terror_ = -EINVAL;\n> \t\t\tLOG(HAL, Error) << \"Discontiguous planes are not supported\";\n> \t\t\treturn;\n> \t\t}\n> \n> -\t\tfd = camera3Buffer->data[i];\n> +\t\tfd_ = camera3Buffer->data[i];\n> \t}\n> \n> -\tif (fd == -1) {\n> +\tif (fd_ == -1) {\n> \t\terror_ = -EINVAL;\n> \t\tLOG(HAL, Error) << \"No valid file descriptor\";\n> \t\treturn;\n> \t}\n> \n> -\toff_t bufferLength = lseek(fd, 0, SEEK_END);\n> -\tif (bufferLength < 0) {\n> +\tbufferLength_ = lseek(fd_, 0, SEEK_END);\n> +\tif (bufferLength_ < 0) {\n> \t\terror_ = -errno;\n> \t\tLOG(HAL, Error) << \"Failed to get buffer length\";\n> \t\treturn;\n> \t}\n> \n> -\tvoid *address = mmap(nullptr, bufferLength, flags, MAP_SHARED, fd, 0);\n> -\tif (address == MAP_FAILED) {\n> -\t\terror_ = -errno;\n> -\t\tLOG(HAL, Error) << \"Failed to mmap plane\";\n> -\t\treturn;\n> -\t}\n> -\tmaps_.emplace_back(static_cast(address), bufferLength);\n> -\n> \tconst unsigned int numPlanes = info.numPlanes();\n> -\tplanes_.resize(numPlanes);\n> +\tplaneInfo_.resize(numPlanes);\n> +\n> \tunsigned int offset = 0;\n> \tfor (unsigned int i = 0; i < numPlanes; ++i) {\n> \t\t/*\n> @@ -109,17 +114,17 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> \t\tconst unsigned int planeSize =\n> \t\t\tstride * ((size.height + vertSubSample - 1) / vertSubSample);\n> \n> -\t\tplanes_[i] = libcamera::Span(\n> -\t\t\tstatic_cast(address) + offset, planeSize);\n> +\t\tplaneInfo_[i].offset = offset;\n> +\t\tplaneInfo_[i].size = planeSize;\n> \n> -\t\tif (bufferLength < offset + planeSize) {\n> -\t\t\terror_ = -EINVAL;\n> -\t\t\tLOG(HAL, Error) << \"Plane \" << i << \" is out of buffer\"\n> -\t\t\t\t\t<< \", buffer length=\" << bufferLength\n> -\t\t\t\t\t<< \", offset=\" << offset\n> -\t\t\t\t\t<< \", size=\" << planeSize;\n> +\t\tif (bufferLength_ < offset + planeSize) {\n> +\t\t\tLOG(HAL, Error) << \"Plane \" << i << \" is out of buffer:\"\n> +\t\t\t\t\t<< \" plane offset=\" << offset\n> +\t\t\t\t\t<< \", plane size=\" << planeSize\n> +\t\t\t\t\t<< \", buffer length=\" << bufferLength_;\n> \t\t\treturn;\n> \t\t}\n> +\n> \t\toffset += planeSize;\n> \t}\n> }\n> @@ -130,12 +135,14 @@ CameraBuffer::Private::~Private()\n> \n> unsigned int CameraBuffer::Private::numPlanes() const\n> {\n> -\treturn planes_.size();\n> +\treturn planeInfo_.size();\n> }\n> \n> Span CameraBuffer::Private::plane(unsigned int plane)\n> {\n> -\tif (plane >= planes_.size())\n> +\tif (!mapped_)\n> +\t\tmap();\n> +\tif (!mapped_)\n> \t\treturn {};\n> \n> \treturn planes_[plane];\n> @@ -143,8 +150,31 @@ Span CameraBuffer::Private::plane(unsigned int plane)\n> \n> size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const\n> {\n> -\treturn std::min(maps_[0].size(),\n> -\t\t\t\t maxJpegBufferSize);\n> +\tASSERT(bufferLength_ >= 0);\n> +\n> +\treturn std::min(bufferLength_, maxJpegBufferSize);\n> +}\n> +\n> +void CameraBuffer::Private::map()\n> +{\n> +\tASSERT(fd_ != -1);\n> +\tASSERT(bufferLength_ >= 0);\n> +\n> +\tvoid *address = mmap(nullptr, bufferLength_, flags_, MAP_SHARED, fd_, 0);\n> +\tif (address == MAP_FAILED) {\n> +\t\terror_ = -errno;\n> +\t\tLOG(HAL, Error) << \"Failed to mmap plane\";\n> +\t\treturn;\n> +\t}\n> +\tmaps_.emplace_back(static_cast(address), bufferLength_);\n> +\n> +\tplanes_.reserve(planeInfo_.size());\n> +\tfor (const auto &info : planeInfo_) {\n> +\t\tplanes_.emplace_back(\n> +\t\t\tstatic_cast(address) + info.offset, info.size);\n> +\t}\n> +\n> +\tmapped_ = true;\n> }\n> \n> PUBLIC_CAMERA_BUFFER_IMPLEMENTATION","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 6BA81BD87D\n\tfor ;\n\tThu, 26 Aug 2021 12:06:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B9B4F688E5;\n\tThu, 26 Aug 2021 14:06:15 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3DEBE6888F\n\tfor ;\n\tThu, 26 Aug 2021 14:06:14 +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 B8FCD191F;\n\tThu, 26 Aug 2021 14:06:13 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"SuN7qR2k\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629979573;\n\tbh=2gPYoHauBSl/fjtQkWc6C9D/CJEnT7x3dF9MENWnnsk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=SuN7qR2kk4LPbWF+O7D12V9CmqYGxHKR1l8oEL1pWAiUVtwXhWYCEmMgh8dDSFbZw\n\tMaLZnQQ0DbM1G3ReyI5hmkybopIxYOtcxMZ4rUnmAnptxcCBoHgs1qHxcIxfE/0wDt\n\tCpqPSaDStWuN5XaVwC8UcAI8R4opH24bvWDIg+A8=","Date":"Thu, 26 Aug 2021 15:06:01 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210826080021.3454520-1-hiroh@chromium.org>\n\t<20210826080021.3454520-3-hiroh@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210826080021.3454520-3-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH v3 2/3] android: camera_buffer: Map\n\tbuffer in the first plane() call","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":19116,"web_url":"https://patchwork.libcamera.org/comment/19116/","msgid":"<20210826214552.wfm42xvtqoz3vmf2@uno.localdomain>","date":"2021-08-26T21:45:52","subject":"Re: [libcamera-devel] [PATCH v3 2/3] android: camera_buffer: Map\n\tbuffer in the first plane() call","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro,\n\nOn Thu, Aug 26, 2021 at 05:00:20PM +0900, Hirokazu Honda wrote:\n> CameraBuffer implementation maps a given buffer_handle_t in\n> constructor. Mapping is redundant to only know the plane info like\n> stride and offset. Mapping should be executed rater in the first\n> plane() call.\n\nWith the issue pointed out by Laurent\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n>\n> Signed-off-by: Hirokazu Honda \n> ---\n> src/android/mm/cros_camera_buffer.cpp | 71 +++++++++++--------\n> src/android/mm/generic_camera_buffer.cpp | 86 ++++++++++++++++--------\n> 2 files changed, 100 insertions(+), 57 deletions(-)\n>\n> diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp\n> index 50732637..ba6650cf 100644\n> --- a/src/android/mm/cros_camera_buffer.cpp\n> +++ b/src/android/mm/cros_camera_buffer.cpp\n> @@ -25,7 +25,7 @@ public:\n> \t\tint flags);\n> \t~Private();\n>\n> -\tbool isValid() const { return valid_; }\n> +\tbool isValid() const { return registered_; }\n>\n> \tunsigned int numPlanes() const;\n>\n> @@ -34,10 +34,12 @@ public:\n> \tsize_t jpegBufferSize(size_t maxJpegBufferSize) const;\n>\n> private:\n> +\tvoid map();\n> +\n> \tcros::CameraBufferManager *bufferManager_;\n> \tbuffer_handle_t handle_;\n> \tunsigned int numPlanes_;\n> -\tbool valid_;\n> +\tbool mapped_;\n> \tbool registered_;\n> \tunion {\n> \t\tvoid *addr;\n> @@ -50,7 +52,7 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> \t\t\t [[maybe_unused]] libcamera::PixelFormat pixelFormat,\n> \t\t\t [[maybe_unused]] const libcamera::Size &size,\n> \t\t\t [[maybe_unused]] int flags)\n> -\t: handle_(camera3Buffer), numPlanes_(0), valid_(false),\n> +\t: handle_(camera3Buffer), numPlanes_(0), mapped_(false),\n> \t registered_(false)\n> {\n> \tbufferManager_ = cros::CameraBufferManager::GetInstance();\n> @@ -63,36 +65,11 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n>\n> \tregistered_ = true;\n> \tnumPlanes_ = bufferManager_->GetNumPlanes(camera3Buffer);\n> -\tswitch (numPlanes_) {\n> -\tcase 1: {\n> -\t\tret = bufferManager_->Lock(handle_, 0, 0, 0, 0, 0, &mem.addr);\n> -\t\tif (ret) {\n> -\t\t\tLOG(HAL, Error) << \"Single plane buffer mapping failed\";\n> -\t\t\treturn;\n> -\t\t}\n> -\t\tbreak;\n> -\t}\n> -\tcase 2:\n> -\tcase 3: {\n> -\t\tret = bufferManager_->LockYCbCr(handle_, 0, 0, 0, 0, 0,\n> -\t\t\t\t\t\t&mem.ycbcr);\n> -\t\tif (ret) {\n> -\t\t\tLOG(HAL, Error) << \"YCbCr buffer mapping failed\";\n> -\t\t\treturn;\n> -\t\t}\n> -\t\tbreak;\n> -\t}\n> -\tdefault:\n> -\t\tLOG(HAL, Error) << \"Invalid number of planes: \" << numPlanes_;\n> -\t\treturn;\n> -\t}\n> -\n> -\tvalid_ = true;\n> }\n>\n> CameraBuffer::Private::~Private()\n> {\n> -\tif (valid_)\n> +\tif (mapped_)\n> \t\tbufferManager_->Unlock(handle_);\n> \tif (registered_)\n> \t\tbufferManager_->Deregister(handle_);\n> @@ -105,6 +82,11 @@ unsigned int CameraBuffer::Private::numPlanes() const\n>\n> Span CameraBuffer::Private::plane(unsigned int plane)\n> {\n> +\tif (!mapped_)\n> +\t\tmap();\n> +\tif (!mapped_)\n> +\t\treturn {};\n> +\n> \tvoid *addr;\n>\n> \tswitch (numPlanes()) {\n> @@ -134,4 +116,35 @@ size_t CameraBuffer::Private::jpegBufferSize([[maybe_unused]] size_t maxJpegBuff\n> \treturn bufferManager_->GetPlaneSize(handle_, 0);\n> }\n>\n> +void CameraBuffer::Private::map()\n> +{\n> +\tint ret;\n> +\tswitch (numPlanes_) {\n> +\tcase 1: {\n> +\t\tret = bufferManager_->Lock(handle_, 0, 0, 0, 0, 0, &mem.addr);\n> +\t\tif (ret) {\n> +\t\t\tLOG(HAL, Error) << \"Single plane buffer mapping failed\";\n> +\t\t\treturn;\n> +\t\t}\n> +\t\tbreak;\n> +\t}\n> +\tcase 2:\n> +\tcase 3: {\n> +\t\tret = bufferManager_->LockYCbCr(handle_, 0, 0, 0, 0, 0,\n> +\t\t\t\t\t\t&mem.ycbcr);\n> +\t\tif (ret) {\n> +\t\t\tLOG(HAL, Error) << \"YCbCr buffer mapping failed\";\n> +\t\t\treturn;\n> +\t\t}\n> +\t\tbreak;\n> +\t}\n> +\tdefault:\n> +\t\tLOG(HAL, Error) << \"Invalid number of planes: \" << numPlanes_;\n> +\t\treturn;\n> +\t}\n> +\n> +\tmapped_ = true;\n> +\treturn;\n> +}\n> +\n> PUBLIC_CAMERA_BUFFER_IMPLEMENTATION\n> diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp\n> index 22753490..cfe55b69 100644\n> --- a/src/android/mm/generic_camera_buffer.cpp\n> +++ b/src/android/mm/generic_camera_buffer.cpp\n> @@ -37,6 +37,18 @@ public:\n> \tsize_t jpegBufferSize(size_t maxJpegBufferSize) const;\n>\n> private:\n> +\tstruct PlaneInfo {\n> +\t\tunsigned int offset;\n> +\t\tunsigned int size;\n> +\t};\n> +\n> +\tvoid map();\n> +\n> +\tint fd_;\n> +\tint flags_;\n> +\toff_t bufferLength_;\n> +\tbool mapped_;\n> +\tstd::vector planeInfo_;\n> \t/* \\todo Remove planes_ when it will be added to MappedBuffer */\n> \tstd::vector> planes_;\n> };\n> @@ -45,6 +57,7 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> \t\t\t buffer_handle_t camera3Buffer,\n> \t\t\t libcamera::PixelFormat pixelFormat,\n> \t\t\t const libcamera::Size &size, int flags)\n> +\t: fd_(-1), flags_(flags), mapped_(false)\n> {\n> \terror_ = 0;\n>\n> @@ -61,43 +74,35 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> \t * now that the buffer is backed by a single dmabuf, with planes being\n> \t * stored contiguously.\n> \t */\n> -\tint fd = -1;\n> \tfor (int i = 0; i < camera3Buffer->numFds; i++) {\n> -\t\tif (camera3Buffer->data[i] == -1 || camera3Buffer->data[i] == fd)\n> +\t\tif (camera3Buffer->data[i] == -1 || camera3Buffer->data[i] == fd_)\n> \t\t\tcontinue;\n>\n> -\t\tif (fd != -1) {\n> +\t\tif (fd_ != -1) {\n> \t\t\terror_ = -EINVAL;\n> \t\t\tLOG(HAL, Error) << \"Discontiguous planes are not supported\";\n> \t\t\treturn;\n> \t\t}\n>\n> -\t\tfd = camera3Buffer->data[i];\n> +\t\tfd_ = camera3Buffer->data[i];\n> \t}\n>\n> -\tif (fd == -1) {\n> +\tif (fd_ == -1) {\n> \t\terror_ = -EINVAL;\n> \t\tLOG(HAL, Error) << \"No valid file descriptor\";\n> \t\treturn;\n> \t}\n>\n> -\toff_t bufferLength = lseek(fd, 0, SEEK_END);\n> -\tif (bufferLength < 0) {\n> +\tbufferLength_ = lseek(fd_, 0, SEEK_END);\n> +\tif (bufferLength_ < 0) {\n> \t\terror_ = -errno;\n> \t\tLOG(HAL, Error) << \"Failed to get buffer length\";\n> \t\treturn;\n> \t}\n>\n> -\tvoid *address = mmap(nullptr, bufferLength, flags, MAP_SHARED, fd, 0);\n> -\tif (address == MAP_FAILED) {\n> -\t\terror_ = -errno;\n> -\t\tLOG(HAL, Error) << \"Failed to mmap plane\";\n> -\t\treturn;\n> -\t}\n> -\tmaps_.emplace_back(static_cast(address), bufferLength);\n> -\n> \tconst unsigned int numPlanes = info.numPlanes();\n> -\tplanes_.resize(numPlanes);\n> +\tplaneInfo_.resize(numPlanes);\n> +\n> \tunsigned int offset = 0;\n> \tfor (unsigned int i = 0; i < numPlanes; ++i) {\n> \t\t/*\n> @@ -109,17 +114,17 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> \t\tconst unsigned int planeSize =\n> \t\t\tstride * ((size.height + vertSubSample - 1) / vertSubSample);\n>\n> -\t\tplanes_[i] = libcamera::Span(\n> -\t\t\tstatic_cast(address) + offset, planeSize);\n> +\t\tplaneInfo_[i].offset = offset;\n> +\t\tplaneInfo_[i].size = planeSize;\n>\n> -\t\tif (bufferLength < offset + planeSize) {\n> -\t\t\terror_ = -EINVAL;\n> -\t\t\tLOG(HAL, Error) << \"Plane \" << i << \" is out of buffer\"\n> -\t\t\t\t\t<< \", buffer length=\" << bufferLength\n> -\t\t\t\t\t<< \", offset=\" << offset\n> -\t\t\t\t\t<< \", size=\" << planeSize;\n> +\t\tif (bufferLength_ < offset + planeSize) {\n> +\t\t\tLOG(HAL, Error) << \"Plane \" << i << \" is out of buffer:\"\n> +\t\t\t\t\t<< \" plane offset=\" << offset\n> +\t\t\t\t\t<< \", plane size=\" << planeSize\n> +\t\t\t\t\t<< \", buffer length=\" << bufferLength_;\n> \t\t\treturn;\n> \t\t}\n> +\n> \t\toffset += planeSize;\n> \t}\n> }\n> @@ -130,12 +135,14 @@ CameraBuffer::Private::~Private()\n>\n> unsigned int CameraBuffer::Private::numPlanes() const\n> {\n> -\treturn planes_.size();\n> +\treturn planeInfo_.size();\n> }\n>\n> Span CameraBuffer::Private::plane(unsigned int plane)\n> {\n> -\tif (plane >= planes_.size())\n> +\tif (!mapped_)\n> +\t\tmap();\n> +\tif (!mapped_)\n> \t\treturn {};\n>\n> \treturn planes_[plane];\n> @@ -143,8 +150,31 @@ Span CameraBuffer::Private::plane(unsigned int plane)\n>\n> size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const\n> {\n> -\treturn std::min(maps_[0].size(),\n> -\t\t\t\t maxJpegBufferSize);\n> +\tASSERT(bufferLength_ >= 0);\n> +\n> +\treturn std::min(bufferLength_, maxJpegBufferSize);\n> +}\n> +\n> +void CameraBuffer::Private::map()\n> +{\n> +\tASSERT(fd_ != -1);\n> +\tASSERT(bufferLength_ >= 0);\n> +\n> +\tvoid *address = mmap(nullptr, bufferLength_, flags_, MAP_SHARED, fd_, 0);\n> +\tif (address == MAP_FAILED) {\n> +\t\terror_ = -errno;\n> +\t\tLOG(HAL, Error) << \"Failed to mmap plane\";\n> +\t\treturn;\n> +\t}\n> +\tmaps_.emplace_back(static_cast(address), bufferLength_);\n> +\n> +\tplanes_.reserve(planeInfo_.size());\n> +\tfor (const auto &info : planeInfo_) {\n> +\t\tplanes_.emplace_back(\n> +\t\t\tstatic_cast(address) + info.offset, info.size);\n> +\t}\n> +\n> +\tmapped_ = true;\n> }\n>\n> PUBLIC_CAMERA_BUFFER_IMPLEMENTATION\n> --\n> 2.33.0.rc2.250.ged5fa647cd-goog\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 9765BBD87D\n\tfor ;\n\tThu, 26 Aug 2021 21:45:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1555768939;\n\tThu, 26 Aug 2021 23:45:06 +0200 (CEST)","from relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[217.70.178.232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DD46D68928\n\tfor ;\n\tThu, 26 Aug 2021 23:45:04 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay12.mail.gandi.net (Postfix) with ESMTPSA id EA6D1200004;\n\tThu, 26 Aug 2021 21:45:03 +0000 (UTC)"],"Date":"Thu, 26 Aug 2021 23:45:52 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20210826214552.wfm42xvtqoz3vmf2@uno.localdomain>","References":"<20210826080021.3454520-1-hiroh@chromium.org>\n\t<20210826080021.3454520-3-hiroh@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210826080021.3454520-3-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH v3 2/3] android: camera_buffer: Map\n\tbuffer in the first plane() call","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":19123,"web_url":"https://patchwork.libcamera.org/comment/19123/","msgid":"","date":"2021-08-27T06:57:08","subject":"Re: [libcamera-devel] [PATCH v3 2/3] android: camera_buffer: Map\n\tbuffer in the first plane() call","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent,\n\nOn Thu, Aug 26, 2021 at 9:06 PM Laurent Pinchart\n wrote:\n>\n> Hi Hiro,\n>\n> Thank you for the patch.\n>\n> On Thu, Aug 26, 2021 at 05:00:20PM +0900, Hirokazu Honda wrote:\n> > CameraBuffer implementation maps a given buffer_handle_t in\n> > constructor. Mapping is redundant to only know the plane info like\n> > stride and offset. Mapping should be executed rater in the first\n> > plane() call.\n> >\n> > Signed-off-by: Hirokazu Honda \n> > ---\n> > src/android/mm/cros_camera_buffer.cpp | 71 +++++++++++--------\n> > src/android/mm/generic_camera_buffer.cpp | 86 ++++++++++++++++--------\n> > 2 files changed, 100 insertions(+), 57 deletions(-)\n> >\n> > diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp\n> > index 50732637..ba6650cf 100644\n> > --- a/src/android/mm/cros_camera_buffer.cpp\n> > +++ b/src/android/mm/cros_camera_buffer.cpp\n> > @@ -25,7 +25,7 @@ public:\n> > int flags);\n> > ~Private();\n> >\n> > - bool isValid() const { return valid_; }\n> > + bool isValid() const { return registered_; }\n> >\n> > unsigned int numPlanes() const;\n> >\n> > @@ -34,10 +34,12 @@ public:\n> > size_t jpegBufferSize(size_t maxJpegBufferSize) const;\n> >\n> > private:\n> > + void map();\n> > +\n> > cros::CameraBufferManager *bufferManager_;\n> > buffer_handle_t handle_;\n> > unsigned int numPlanes_;\n> > - bool valid_;\n> > + bool mapped_;\n> > bool registered_;\n> > union {\n> > void *addr;\n> > @@ -50,7 +52,7 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> > [[maybe_unused]] libcamera::PixelFormat pixelFormat,\n> > [[maybe_unused]] const libcamera::Size &size,\n> > [[maybe_unused]] int flags)\n> > - : handle_(camera3Buffer), numPlanes_(0), valid_(false),\n> > + : handle_(camera3Buffer), numPlanes_(0), mapped_(false),\n> > registered_(false)\n> > {\n> > bufferManager_ = cros::CameraBufferManager::GetInstance();\n> > @@ -63,36 +65,11 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> >\n> > registered_ = true;\n> > numPlanes_ = bufferManager_->GetNumPlanes(camera3Buffer);\n> > - switch (numPlanes_) {\n> > - case 1: {\n> > - ret = bufferManager_->Lock(handle_, 0, 0, 0, 0, 0, &mem.addr);\n> > - if (ret) {\n> > - LOG(HAL, Error) << \"Single plane buffer mapping failed\";\n> > - return;\n> > - }\n> > - break;\n> > - }\n> > - case 2:\n> > - case 3: {\n> > - ret = bufferManager_->LockYCbCr(handle_, 0, 0, 0, 0, 0,\n> > - &mem.ycbcr);\n> > - if (ret) {\n> > - LOG(HAL, Error) << \"YCbCr buffer mapping failed\";\n> > - return;\n> > - }\n> > - break;\n> > - }\n> > - default:\n> > - LOG(HAL, Error) << \"Invalid number of planes: \" << numPlanes_;\n> > - return;\n> > - }\n> > -\n> > - valid_ = true;\n> > }\n> >\n> > CameraBuffer::Private::~Private()\n> > {\n> > - if (valid_)\n> > + if (mapped_)\n> > bufferManager_->Unlock(handle_);\n> > if (registered_)\n> > bufferManager_->Deregister(handle_);\n> > @@ -105,6 +82,11 @@ unsigned int CameraBuffer::Private::numPlanes() const\n> >\n> > Span CameraBuffer::Private::plane(unsigned int plane)\n> > {\n> > + if (!mapped_)\n> > + map();\n> > + if (!mapped_)\n> > + return {};\n> > +\n> > void *addr;\n> >\n> > switch (numPlanes()) {\n> > @@ -134,4 +116,35 @@ size_t CameraBuffer::Private::jpegBufferSize([[maybe_unused]] size_t maxJpegBuff\n> > return bufferManager_->GetPlaneSize(handle_, 0);\n> > }\n> >\n> > +void CameraBuffer::Private::map()\n> > +{\n> > + int ret;\n> > + switch (numPlanes_) {\n> > + case 1: {\n> > + ret = bufferManager_->Lock(handle_, 0, 0, 0, 0, 0, &mem.addr);\n> > + if (ret) {\n> > + LOG(HAL, Error) << \"Single plane buffer mapping failed\";\n> > + return;\n> > + }\n> > + break;\n> > + }\n> > + case 2:\n> > + case 3: {\n> > + ret = bufferManager_->LockYCbCr(handle_, 0, 0, 0, 0, 0,\n> > + &mem.ycbcr);\n> > + if (ret) {\n> > + LOG(HAL, Error) << \"YCbCr buffer mapping failed\";\n> > + return;\n> > + }\n> > + break;\n> > + }\n> > + default:\n> > + LOG(HAL, Error) << \"Invalid number of planes: \" << numPlanes_;\n> > + return;\n> > + }\n> > +\n> > + mapped_ = true;\n> > + return;\n> > +}\n> > +\n> > PUBLIC_CAMERA_BUFFER_IMPLEMENTATION\n> > diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp\n> > index 22753490..cfe55b69 100644\n> > --- a/src/android/mm/generic_camera_buffer.cpp\n> > +++ b/src/android/mm/generic_camera_buffer.cpp\n> > @@ -37,6 +37,18 @@ public:\n> > size_t jpegBufferSize(size_t maxJpegBufferSize) const;\n> >\n> > private:\n> > + struct PlaneInfo {\n> > + unsigned int offset;\n> > + unsigned int size;\n> > + };\n> > +\n> > + void map();\n> > +\n> > + int fd_;\n> > + int flags_;\n> > + off_t bufferLength_;\n> > + bool mapped_;\n> > + std::vector planeInfo_;\n> > /* \\todo Remove planes_ when it will be added to MappedBuffer */\n> > std::vector> planes_;\n> > };\n> > @@ -45,6 +57,7 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> > buffer_handle_t camera3Buffer,\n> > libcamera::PixelFormat pixelFormat,\n> > const libcamera::Size &size, int flags)\n> > + : fd_(-1), flags_(flags), mapped_(false)\n>\n> This should initialize bufferLength_ to 0, otherwise the ASSERT()s that\n> checks the variable will be pointless if the constructor returns an\n> error before setting bufferLength_.\n>\n\nShall I set bufferLength to -1 and then ASSERT(bufferLength_ >= 0), or\nset to 0 and ASSERT(bufferLength > 0)?\n\n-Hiro\n> And it will also avoid a Coverity warning :-)\n>\n> I can handle this when pushing the series.\n>\n> Reviewed-by: Laurent Pinchart \n>\n> > {\n> > error_ = 0;\n> >\n> > @@ -61,43 +74,35 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> > * now that the buffer is backed by a single dmabuf, with planes being\n> > * stored contiguously.\n> > */\n> > - int fd = -1;\n> > for (int i = 0; i < camera3Buffer->numFds; i++) {\n> > - if (camera3Buffer->data[i] == -1 || camera3Buffer->data[i] == fd)\n> > + if (camera3Buffer->data[i] == -1 || camera3Buffer->data[i] == fd_)\n> > continue;\n> >\n> > - if (fd != -1) {\n> > + if (fd_ != -1) {\n> > error_ = -EINVAL;\n> > LOG(HAL, Error) << \"Discontiguous planes are not supported\";\n> > return;\n> > }\n> >\n> > - fd = camera3Buffer->data[i];\n> > + fd_ = camera3Buffer->data[i];\n> > }\n> >\n> > - if (fd == -1) {\n> > + if (fd_ == -1) {\n> > error_ = -EINVAL;\n> > LOG(HAL, Error) << \"No valid file descriptor\";\n> > return;\n> > }\n> >\n> > - off_t bufferLength = lseek(fd, 0, SEEK_END);\n> > - if (bufferLength < 0) {\n> > + bufferLength_ = lseek(fd_, 0, SEEK_END);\n> > + if (bufferLength_ < 0) {\n> > error_ = -errno;\n> > LOG(HAL, Error) << \"Failed to get buffer length\";\n> > return;\n> > }\n> >\n> > - void *address = mmap(nullptr, bufferLength, flags, MAP_SHARED, fd, 0);\n> > - if (address == MAP_FAILED) {\n> > - error_ = -errno;\n> > - LOG(HAL, Error) << \"Failed to mmap plane\";\n> > - return;\n> > - }\n> > - maps_.emplace_back(static_cast(address), bufferLength);\n> > -\n> > const unsigned int numPlanes = info.numPlanes();\n> > - planes_.resize(numPlanes);\n> > + planeInfo_.resize(numPlanes);\n> > +\n> > unsigned int offset = 0;\n> > for (unsigned int i = 0; i < numPlanes; ++i) {\n> > /*\n> > @@ -109,17 +114,17 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> > const unsigned int planeSize =\n> > stride * ((size.height + vertSubSample - 1) / vertSubSample);\n> >\n> > - planes_[i] = libcamera::Span(\n> > - static_cast(address) + offset, planeSize);\n> > + planeInfo_[i].offset = offset;\n> > + planeInfo_[i].size = planeSize;\n> >\n> > - if (bufferLength < offset + planeSize) {\n> > - error_ = -EINVAL;\n> > - LOG(HAL, Error) << \"Plane \" << i << \" is out of buffer\"\n> > - << \", buffer length=\" << bufferLength\n> > - << \", offset=\" << offset\n> > - << \", size=\" << planeSize;\n> > + if (bufferLength_ < offset + planeSize) {\n> > + LOG(HAL, Error) << \"Plane \" << i << \" is out of buffer:\"\n> > + << \" plane offset=\" << offset\n> > + << \", plane size=\" << planeSize\n> > + << \", buffer length=\" << bufferLength_;\n> > return;\n> > }\n> > +\n> > offset += planeSize;\n> > }\n> > }\n> > @@ -130,12 +135,14 @@ CameraBuffer::Private::~Private()\n> >\n> > unsigned int CameraBuffer::Private::numPlanes() const\n> > {\n> > - return planes_.size();\n> > + return planeInfo_.size();\n> > }\n> >\n> > Span CameraBuffer::Private::plane(unsigned int plane)\n> > {\n> > - if (plane >= planes_.size())\n> > + if (!mapped_)\n> > + map();\n> > + if (!mapped_)\n> > return {};\n> >\n> > return planes_[plane];\n> > @@ -143,8 +150,31 @@ Span CameraBuffer::Private::plane(unsigned int plane)\n> >\n> > size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const\n> > {\n> > - return std::min(maps_[0].size(),\n> > - maxJpegBufferSize);\n> > + ASSERT(bufferLength_ >= 0);\n> > +\n> > + return std::min(bufferLength_, maxJpegBufferSize);\n> > +}\n> > +\n> > +void CameraBuffer::Private::map()\n> > +{\n> > + ASSERT(fd_ != -1);\n> > + ASSERT(bufferLength_ >= 0);\n> > +\n> > + void *address = mmap(nullptr, bufferLength_, flags_, MAP_SHARED, fd_, 0);\n> > + if (address == MAP_FAILED) {\n> > + error_ = -errno;\n> > + LOG(HAL, Error) << \"Failed to mmap plane\";\n> > + return;\n> > + }\n> > + maps_.emplace_back(static_cast(address), bufferLength_);\n> > +\n> > + planes_.reserve(planeInfo_.size());\n> > + for (const auto &info : planeInfo_) {\n> > + planes_.emplace_back(\n> > + static_cast(address) + info.offset, info.size);\n> > + }\n> > +\n> > + mapped_ = true;\n> > }\n> >\n> > PUBLIC_CAMERA_BUFFER_IMPLEMENTATION\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 2A9C9BD87D\n\tfor ;\n\tFri, 27 Aug 2021 06:57:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 96B1468932;\n\tFri, 27 Aug 2021 08:57:20 +0200 (CEST)","from mail-ej1-x62b.google.com (mail-ej1-x62b.google.com\n\t[IPv6:2a00:1450:4864:20::62b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5F01560288\n\tfor ;\n\tFri, 27 Aug 2021 08:57:19 +0200 (CEST)","by mail-ej1-x62b.google.com with SMTP id n27so11745652eja.5\n\tfor ;\n\tThu, 26 Aug 2021 23:57:19 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"F9X3FJ6X\"; 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=FbuOP2w57u21bQRlol14NXqoS8AK6Vgs6vuFqx/KZzk=;\n\tb=F9X3FJ6X1RpfEr2fius+VYWVphnCjxkxoQeLmRsUqnJc0cN6g0njUaHg2UdkMSij9J\n\twpyLb7neM9cozMDQeh06480XT6jMnPSSj3jYkSKHOw9lUbS+wtGGzPO3zm55KGz9SEVU\n\tDopR6grRkE3gLIIB0j68qDmL+o18m7L559D6Y=","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=FbuOP2w57u21bQRlol14NXqoS8AK6Vgs6vuFqx/KZzk=;\n\tb=UvoLA+eLLNLlC1YaUwlP27AjKRdZr1xFHMEi5AEPVvQPe/w/3neqgL45O0LShEpl49\n\tCih6caqsysYazBY0zwfH/YIkv7OLhQ40b4d/+BeFi+iUFDRcjwX162roBtDtsV9QkX4v\n\tfEizW58xlFBHVXmn0Wjf28GctWSyCFAMwX61b0p4UWUImERNOO5OaoX1/e8eqajb9qPW\n\taoU292DF/M1MmdjWvRQuG/jrgtGGgfKtlqbf07YoLjSAltjtA+O6m29mDdMlCT1zo6tM\n\tk2JhwCbNGMj6SWUdW+i1fRnr9MjdRddUcXGH4u4q3LqVQdxK2ftYDtuAZcmkQAC0RWBh\n\t8jLA==","X-Gm-Message-State":"AOAM533dMNp18dv/SPxksqa4xueZ03guO9CGjvds7mb5yQansdppNJee\n\twrNGHfQGC+9orG18Wl2Cmg1R06hCWOpWlmhRvQrS02xbJAs=","X-Google-Smtp-Source":"ABdhPJw3dYKgqy6l48jbwicVugCZF8FG3ffLAM+iihihRMUBkcbyY98BOPLAHmuU2FbQEQYn/PYkBk46z2nNTDBwkuc=","X-Received":"by 2002:a17:906:e104:: with SMTP id\n\tgj4mr8152181ejb.306.1630047438904; \n\tThu, 26 Aug 2021 23:57:18 -0700 (PDT)","MIME-Version":"1.0","References":"<20210826080021.3454520-1-hiroh@chromium.org>\n\t<20210826080021.3454520-3-hiroh@chromium.org>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Fri, 27 Aug 2021 15:57:08 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v3 2/3] android: camera_buffer: Map\n\tbuffer in the first plane() call","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":19124,"web_url":"https://patchwork.libcamera.org/comment/19124/","msgid":"","date":"2021-08-27T07:00:10","subject":"Re: [libcamera-devel] [PATCH v3 2/3] android: camera_buffer: Map\n\tbuffer in the first plane() call","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"On Fri, Aug 27, 2021 at 3:57 PM Hirokazu Honda wrote:\n>\n> Hi Laurent,\n>\n> On Thu, Aug 26, 2021 at 9:06 PM Laurent Pinchart\n> wrote:\n> >\n> > Hi Hiro,\n> >\n> > Thank you for the patch.\n> >\n> > On Thu, Aug 26, 2021 at 05:00:20PM +0900, Hirokazu Honda wrote:\n> > > CameraBuffer implementation maps a given buffer_handle_t in\n> > > constructor. Mapping is redundant to only know the plane info like\n> > > stride and offset. Mapping should be executed rater in the first\n> > > plane() call.\n> > >\n> > > Signed-off-by: Hirokazu Honda \n> > > ---\n> > > src/android/mm/cros_camera_buffer.cpp | 71 +++++++++++--------\n> > > src/android/mm/generic_camera_buffer.cpp | 86 ++++++++++++++++--------\n> > > 2 files changed, 100 insertions(+), 57 deletions(-)\n> > >\n> > > diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp\n> > > index 50732637..ba6650cf 100644\n> > > --- a/src/android/mm/cros_camera_buffer.cpp\n> > > +++ b/src/android/mm/cros_camera_buffer.cpp\n> > > @@ -25,7 +25,7 @@ public:\n> > > int flags);\n> > > ~Private();\n> > >\n> > > - bool isValid() const { return valid_; }\n> > > + bool isValid() const { return registered_; }\n> > >\n> > > unsigned int numPlanes() const;\n> > >\n> > > @@ -34,10 +34,12 @@ public:\n> > > size_t jpegBufferSize(size_t maxJpegBufferSize) const;\n> > >\n> > > private:\n> > > + void map();\n> > > +\n> > > cros::CameraBufferManager *bufferManager_;\n> > > buffer_handle_t handle_;\n> > > unsigned int numPlanes_;\n> > > - bool valid_;\n> > > + bool mapped_;\n> > > bool registered_;\n> > > union {\n> > > void *addr;\n> > > @@ -50,7 +52,7 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> > > [[maybe_unused]] libcamera::PixelFormat pixelFormat,\n> > > [[maybe_unused]] const libcamera::Size &size,\n> > > [[maybe_unused]] int flags)\n> > > - : handle_(camera3Buffer), numPlanes_(0), valid_(false),\n> > > + : handle_(camera3Buffer), numPlanes_(0), mapped_(false),\n> > > registered_(false)\n> > > {\n> > > bufferManager_ = cros::CameraBufferManager::GetInstance();\n> > > @@ -63,36 +65,11 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> > >\n> > > registered_ = true;\n> > > numPlanes_ = bufferManager_->GetNumPlanes(camera3Buffer);\n> > > - switch (numPlanes_) {\n> > > - case 1: {\n> > > - ret = bufferManager_->Lock(handle_, 0, 0, 0, 0, 0, &mem.addr);\n> > > - if (ret) {\n> > > - LOG(HAL, Error) << \"Single plane buffer mapping failed\";\n> > > - return;\n> > > - }\n> > > - break;\n> > > - }\n> > > - case 2:\n> > > - case 3: {\n> > > - ret = bufferManager_->LockYCbCr(handle_, 0, 0, 0, 0, 0,\n> > > - &mem.ycbcr);\n> > > - if (ret) {\n> > > - LOG(HAL, Error) << \"YCbCr buffer mapping failed\";\n> > > - return;\n> > > - }\n> > > - break;\n> > > - }\n> > > - default:\n> > > - LOG(HAL, Error) << \"Invalid number of planes: \" << numPlanes_;\n> > > - return;\n> > > - }\n> > > -\n> > > - valid_ = true;\n> > > }\n> > >\n> > > CameraBuffer::Private::~Private()\n> > > {\n> > > - if (valid_)\n> > > + if (mapped_)\n> > > bufferManager_->Unlock(handle_);\n> > > if (registered_)\n> > > bufferManager_->Deregister(handle_);\n> > > @@ -105,6 +82,11 @@ unsigned int CameraBuffer::Private::numPlanes() const\n> > >\n> > > Span CameraBuffer::Private::plane(unsigned int plane)\n> > > {\n> > > + if (!mapped_)\n> > > + map();\n> > > + if (!mapped_)\n> > > + return {};\n> > > +\n> > > void *addr;\n> > >\n> > > switch (numPlanes()) {\n> > > @@ -134,4 +116,35 @@ size_t CameraBuffer::Private::jpegBufferSize([[maybe_unused]] size_t maxJpegBuff\n> > > return bufferManager_->GetPlaneSize(handle_, 0);\n> > > }\n> > >\n> > > +void CameraBuffer::Private::map()\n> > > +{\n> > > + int ret;\n> > > + switch (numPlanes_) {\n> > > + case 1: {\n> > > + ret = bufferManager_->Lock(handle_, 0, 0, 0, 0, 0, &mem.addr);\n> > > + if (ret) {\n> > > + LOG(HAL, Error) << \"Single plane buffer mapping failed\";\n> > > + return;\n> > > + }\n> > > + break;\n> > > + }\n> > > + case 2:\n> > > + case 3: {\n> > > + ret = bufferManager_->LockYCbCr(handle_, 0, 0, 0, 0, 0,\n> > > + &mem.ycbcr);\n> > > + if (ret) {\n> > > + LOG(HAL, Error) << \"YCbCr buffer mapping failed\";\n> > > + return;\n> > > + }\n> > > + break;\n> > > + }\n> > > + default:\n> > > + LOG(HAL, Error) << \"Invalid number of planes: \" << numPlanes_;\n> > > + return;\n> > > + }\n> > > +\n> > > + mapped_ = true;\n> > > + return;\n> > > +}\n> > > +\n> > > PUBLIC_CAMERA_BUFFER_IMPLEMENTATION\n> > > diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp\n> > > index 22753490..cfe55b69 100644\n> > > --- a/src/android/mm/generic_camera_buffer.cpp\n> > > +++ b/src/android/mm/generic_camera_buffer.cpp\n> > > @@ -37,6 +37,18 @@ public:\n> > > size_t jpegBufferSize(size_t maxJpegBufferSize) const;\n> > >\n> > > private:\n> > > + struct PlaneInfo {\n> > > + unsigned int offset;\n> > > + unsigned int size;\n> > > + };\n> > > +\n> > > + void map();\n> > > +\n> > > + int fd_;\n> > > + int flags_;\n> > > + off_t bufferLength_;\n> > > + bool mapped_;\n> > > + std::vector planeInfo_;\n> > > /* \\todo Remove planes_ when it will be added to MappedBuffer */\n> > > std::vector> planes_;\n> > > };\n> > > @@ -45,6 +57,7 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> > > buffer_handle_t camera3Buffer,\n> > > libcamera::PixelFormat pixelFormat,\n> > > const libcamera::Size &size, int flags)\n> > > + : fd_(-1), flags_(flags), mapped_(false)\n> >\n> > This should initialize bufferLength_ to 0, otherwise the ASSERT()s that\n> > checks the variable will be pointless if the constructor returns an\n> > error before setting bufferLength_.\n> >\n>\n> Shall I set bufferLength to -1 and then ASSERT(bufferLength_ >= 0), or\n> set to 0 and ASSERT(bufferLength > 0)?\n>\n\nUploaded with the former. If you think the latter is correct, please\nfix it upon applying.\nThanks in advance.\n\n-Hiro\n\n> -Hiro\n> > And it will also avoid a Coverity warning :-)\n> >\n> > I can handle this when pushing the series.\n> >\n> > Reviewed-by: Laurent Pinchart \n> >\n> > > {\n> > > error_ = 0;\n> > >\n> > > @@ -61,43 +74,35 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> > > * now that the buffer is backed by a single dmabuf, with planes being\n> > > * stored contiguously.\n> > > */\n> > > - int fd = -1;\n> > > for (int i = 0; i < camera3Buffer->numFds; i++) {\n> > > - if (camera3Buffer->data[i] == -1 || camera3Buffer->data[i] == fd)\n> > > + if (camera3Buffer->data[i] == -1 || camera3Buffer->data[i] == fd_)\n> > > continue;\n> > >\n> > > - if (fd != -1) {\n> > > + if (fd_ != -1) {\n> > > error_ = -EINVAL;\n> > > LOG(HAL, Error) << \"Discontiguous planes are not supported\";\n> > > return;\n> > > }\n> > >\n> > > - fd = camera3Buffer->data[i];\n> > > + fd_ = camera3Buffer->data[i];\n> > > }\n> > >\n> > > - if (fd == -1) {\n> > > + if (fd_ == -1) {\n> > > error_ = -EINVAL;\n> > > LOG(HAL, Error) << \"No valid file descriptor\";\n> > > return;\n> > > }\n> > >\n> > > - off_t bufferLength = lseek(fd, 0, SEEK_END);\n> > > - if (bufferLength < 0) {\n> > > + bufferLength_ = lseek(fd_, 0, SEEK_END);\n> > > + if (bufferLength_ < 0) {\n> > > error_ = -errno;\n> > > LOG(HAL, Error) << \"Failed to get buffer length\";\n> > > return;\n> > > }\n> > >\n> > > - void *address = mmap(nullptr, bufferLength, flags, MAP_SHARED, fd, 0);\n> > > - if (address == MAP_FAILED) {\n> > > - error_ = -errno;\n> > > - LOG(HAL, Error) << \"Failed to mmap plane\";\n> > > - return;\n> > > - }\n> > > - maps_.emplace_back(static_cast(address), bufferLength);\n> > > -\n> > > const unsigned int numPlanes = info.numPlanes();\n> > > - planes_.resize(numPlanes);\n> > > + planeInfo_.resize(numPlanes);\n> > > +\n> > > unsigned int offset = 0;\n> > > for (unsigned int i = 0; i < numPlanes; ++i) {\n> > > /*\n> > > @@ -109,17 +114,17 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> > > const unsigned int planeSize =\n> > > stride * ((size.height + vertSubSample - 1) / vertSubSample);\n> > >\n> > > - planes_[i] = libcamera::Span(\n> > > - static_cast(address) + offset, planeSize);\n> > > + planeInfo_[i].offset = offset;\n> > > + planeInfo_[i].size = planeSize;\n> > >\n> > > - if (bufferLength < offset + planeSize) {\n> > > - error_ = -EINVAL;\n> > > - LOG(HAL, Error) << \"Plane \" << i << \" is out of buffer\"\n> > > - << \", buffer length=\" << bufferLength\n> > > - << \", offset=\" << offset\n> > > - << \", size=\" << planeSize;\n> > > + if (bufferLength_ < offset + planeSize) {\n> > > + LOG(HAL, Error) << \"Plane \" << i << \" is out of buffer:\"\n> > > + << \" plane offset=\" << offset\n> > > + << \", plane size=\" << planeSize\n> > > + << \", buffer length=\" << bufferLength_;\n> > > return;\n> > > }\n> > > +\n> > > offset += planeSize;\n> > > }\n> > > }\n> > > @@ -130,12 +135,14 @@ CameraBuffer::Private::~Private()\n> > >\n> > > unsigned int CameraBuffer::Private::numPlanes() const\n> > > {\n> > > - return planes_.size();\n> > > + return planeInfo_.size();\n> > > }\n> > >\n> > > Span CameraBuffer::Private::plane(unsigned int plane)\n> > > {\n> > > - if (plane >= planes_.size())\n> > > + if (!mapped_)\n> > > + map();\n> > > + if (!mapped_)\n> > > return {};\n> > >\n> > > return planes_[plane];\n> > > @@ -143,8 +150,31 @@ Span CameraBuffer::Private::plane(unsigned int plane)\n> > >\n> > > size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const\n> > > {\n> > > - return std::min(maps_[0].size(),\n> > > - maxJpegBufferSize);\n> > > + ASSERT(bufferLength_ >= 0);\n> > > +\n> > > + return std::min(bufferLength_, maxJpegBufferSize);\n> > > +}\n> > > +\n> > > +void CameraBuffer::Private::map()\n> > > +{\n> > > + ASSERT(fd_ != -1);\n> > > + ASSERT(bufferLength_ >= 0);\n> > > +\n> > > + void *address = mmap(nullptr, bufferLength_, flags_, MAP_SHARED, fd_, 0);\n> > > + if (address == MAP_FAILED) {\n> > > + error_ = -errno;\n> > > + LOG(HAL, Error) << \"Failed to mmap plane\";\n> > > + return;\n> > > + }\n> > > + maps_.emplace_back(static_cast(address), bufferLength_);\n> > > +\n> > > + planes_.reserve(planeInfo_.size());\n> > > + for (const auto &info : planeInfo_) {\n> > > + planes_.emplace_back(\n> > > + static_cast(address) + info.offset, info.size);\n> > > + }\n> > > +\n> > > + mapped_ = true;\n> > > }\n> > >\n> > > PUBLIC_CAMERA_BUFFER_IMPLEMENTATION\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 98908BD87D\n\tfor ;\n\tFri, 27 Aug 2021 07:00:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5DAFF68932;\n\tFri, 27 Aug 2021 09:00:22 +0200 (CEST)","from mail-ej1-x634.google.com (mail-ej1-x634.google.com\n\t[IPv6:2a00:1450:4864:20::634])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8E53868891\n\tfor ;\n\tFri, 27 Aug 2021 09:00:21 +0200 (CEST)","by mail-ej1-x634.google.com with SMTP id mf2so11701208ejb.9\n\tfor ;\n\tFri, 27 Aug 2021 00:00:21 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"l0MoxbQ5\"; 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=gZlpxiV2O79V+7zsRDvPA4JVCV99sVASvxaFgw4LJE8=;\n\tb=l0MoxbQ5+LMZ6KqdPNCrdpfAWS368amwebVuV85zmcrKomaZ+Oyp/OQgwkAw8MQ25E\n\tOW6MO6WHTqv29CAraJmF0eVJ45aMt8kl4aZ4zzrrLahCyb3FElBCjGyklJZLI94KS8ZA\n\tD6ikpNh/r73PD3pz+ekapF9VSXRBJoop/lFro=","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=gZlpxiV2O79V+7zsRDvPA4JVCV99sVASvxaFgw4LJE8=;\n\tb=aGcdl1k/eygKcua/GV5xRBHtPqDwbAR0zNwRXkE3De1dW6/SxIvhVC8RSkvlXYGyUn\n\tqiJEttQ0VWLvOBmdRQURzMnUw55v++3jZWCRTAiTDhocC/ofeqtRsuZZPqUAV5m0+9Ky\n\t4RRUS6gm+wI/oFm3COjmU5WUDVNUpybY0qBRNbM1M2Y+Nui40b8IIpatngpxg1r3rKvK\n\todKmbT7+EIN2sZk2i1o2SwkcTCOiGndoIMpq3pXOxYYzpaTMLA1UtOBcbfbigJGgkxwZ\n\tR7DVXJ7oOU66RIRjyLg9RkKD9TvG+ax/ZtCgnnPRyGYMU5vnEGzAsYdyDUs5S+lN/gA9\n\tMJMQ==","X-Gm-Message-State":"AOAM532guNZDKRb0GAmJwUcgHXkLcJJSbp/ad7OjKoLHuZo/POxG3hDO\n\tK/TZN8+1Ngcg3w6m/siRZ8MFpuGMUMTuWxXJWs8myW7zh/s=","X-Google-Smtp-Source":"ABdhPJwKIkqqnDdnh9gSx/or+jx8yir5ihgG97cHz0isq6mbJzZJ+0BQ7a9bqBytYh6pFCNiSjZbgonh3zjXXeGoxCc=","X-Received":"by 2002:a17:906:ec9:: with SMTP id\n\tu9mr8021695eji.243.1630047621112; \n\tFri, 27 Aug 2021 00:00:21 -0700 (PDT)","MIME-Version":"1.0","References":"<20210826080021.3454520-1-hiroh@chromium.org>\n\t<20210826080021.3454520-3-hiroh@chromium.org>\n\t\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Fri, 27 Aug 2021 16:00:10 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v3 2/3] android: camera_buffer: Map\n\tbuffer in the first plane() call","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":19177,"web_url":"https://patchwork.libcamera.org/comment/19177/","msgid":"","date":"2021-08-30T15:00:55","subject":"Re: [libcamera-devel] [PATCH v3 2/3] android: camera_buffer: Map\n\tbuffer in the first plane() call","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 27, 2021 at 04:00:10PM +0900, Hirokazu Honda wrote:\n> On Fri, Aug 27, 2021 at 3:57 PM Hirokazu Honda wrote:\n> > On Thu, Aug 26, 2021 at 9:06 PM Laurent Pinchart wrote:\n> > > On Thu, Aug 26, 2021 at 05:00:20PM +0900, Hirokazu Honda wrote:\n> > > > CameraBuffer implementation maps a given buffer_handle_t in\n> > > > constructor. Mapping is redundant to only know the plane info like\n> > > > stride and offset. Mapping should be executed rater in the first\n> > > > plane() call.\n> > > >\n> > > > Signed-off-by: Hirokazu Honda \n> > > > ---\n> > > > src/android/mm/cros_camera_buffer.cpp | 71 +++++++++++--------\n> > > > src/android/mm/generic_camera_buffer.cpp | 86 ++++++++++++++++--------\n> > > > 2 files changed, 100 insertions(+), 57 deletions(-)\n> > > >\n> > > > diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp\n> > > > index 50732637..ba6650cf 100644\n> > > > --- a/src/android/mm/cros_camera_buffer.cpp\n> > > > +++ b/src/android/mm/cros_camera_buffer.cpp\n> > > > @@ -25,7 +25,7 @@ public:\n> > > > int flags);\n> > > > ~Private();\n> > > >\n> > > > - bool isValid() const { return valid_; }\n> > > > + bool isValid() const { return registered_; }\n> > > >\n> > > > unsigned int numPlanes() const;\n> > > >\n> > > > @@ -34,10 +34,12 @@ public:\n> > > > size_t jpegBufferSize(size_t maxJpegBufferSize) const;\n> > > >\n> > > > private:\n> > > > + void map();\n> > > > +\n> > > > cros::CameraBufferManager *bufferManager_;\n> > > > buffer_handle_t handle_;\n> > > > unsigned int numPlanes_;\n> > > > - bool valid_;\n> > > > + bool mapped_;\n> > > > bool registered_;\n> > > > union {\n> > > > void *addr;\n> > > > @@ -50,7 +52,7 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> > > > [[maybe_unused]] libcamera::PixelFormat pixelFormat,\n> > > > [[maybe_unused]] const libcamera::Size &size,\n> > > > [[maybe_unused]] int flags)\n> > > > - : handle_(camera3Buffer), numPlanes_(0), valid_(false),\n> > > > + : handle_(camera3Buffer), numPlanes_(0), mapped_(false),\n> > > > registered_(false)\n> > > > {\n> > > > bufferManager_ = cros::CameraBufferManager::GetInstance();\n> > > > @@ -63,36 +65,11 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> > > >\n> > > > registered_ = true;\n> > > > numPlanes_ = bufferManager_->GetNumPlanes(camera3Buffer);\n> > > > - switch (numPlanes_) {\n> > > > - case 1: {\n> > > > - ret = bufferManager_->Lock(handle_, 0, 0, 0, 0, 0, &mem.addr);\n> > > > - if (ret) {\n> > > > - LOG(HAL, Error) << \"Single plane buffer mapping failed\";\n> > > > - return;\n> > > > - }\n> > > > - break;\n> > > > - }\n> > > > - case 2:\n> > > > - case 3: {\n> > > > - ret = bufferManager_->LockYCbCr(handle_, 0, 0, 0, 0, 0,\n> > > > - &mem.ycbcr);\n> > > > - if (ret) {\n> > > > - LOG(HAL, Error) << \"YCbCr buffer mapping failed\";\n> > > > - return;\n> > > > - }\n> > > > - break;\n> > > > - }\n> > > > - default:\n> > > > - LOG(HAL, Error) << \"Invalid number of planes: \" << numPlanes_;\n> > > > - return;\n> > > > - }\n> > > > -\n> > > > - valid_ = true;\n> > > > }\n> > > >\n> > > > CameraBuffer::Private::~Private()\n> > > > {\n> > > > - if (valid_)\n> > > > + if (mapped_)\n> > > > bufferManager_->Unlock(handle_);\n> > > > if (registered_)\n> > > > bufferManager_->Deregister(handle_);\n> > > > @@ -105,6 +82,11 @@ unsigned int CameraBuffer::Private::numPlanes() const\n> > > >\n> > > > Span CameraBuffer::Private::plane(unsigned int plane)\n> > > > {\n> > > > + if (!mapped_)\n> > > > + map();\n> > > > + if (!mapped_)\n> > > > + return {};\n> > > > +\n> > > > void *addr;\n> > > >\n> > > > switch (numPlanes()) {\n> > > > @@ -134,4 +116,35 @@ size_t CameraBuffer::Private::jpegBufferSize([[maybe_unused]] size_t maxJpegBuff\n> > > > return bufferManager_->GetPlaneSize(handle_, 0);\n> > > > }\n> > > >\n> > > > +void CameraBuffer::Private::map()\n> > > > +{\n> > > > + int ret;\n> > > > + switch (numPlanes_) {\n> > > > + case 1: {\n> > > > + ret = bufferManager_->Lock(handle_, 0, 0, 0, 0, 0, &mem.addr);\n> > > > + if (ret) {\n> > > > + LOG(HAL, Error) << \"Single plane buffer mapping failed\";\n> > > > + return;\n> > > > + }\n> > > > + break;\n> > > > + }\n> > > > + case 2:\n> > > > + case 3: {\n> > > > + ret = bufferManager_->LockYCbCr(handle_, 0, 0, 0, 0, 0,\n> > > > + &mem.ycbcr);\n> > > > + if (ret) {\n> > > > + LOG(HAL, Error) << \"YCbCr buffer mapping failed\";\n> > > > + return;\n> > > > + }\n> > > > + break;\n> > > > + }\n> > > > + default:\n> > > > + LOG(HAL, Error) << \"Invalid number of planes: \" << numPlanes_;\n> > > > + return;\n> > > > + }\n> > > > +\n> > > > + mapped_ = true;\n> > > > + return;\n> > > > +}\n> > > > +\n> > > > PUBLIC_CAMERA_BUFFER_IMPLEMENTATION\n> > > > diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp\n> > > > index 22753490..cfe55b69 100644\n> > > > --- a/src/android/mm/generic_camera_buffer.cpp\n> > > > +++ b/src/android/mm/generic_camera_buffer.cpp\n> > > > @@ -37,6 +37,18 @@ public:\n> > > > size_t jpegBufferSize(size_t maxJpegBufferSize) const;\n> > > >\n> > > > private:\n> > > > + struct PlaneInfo {\n> > > > + unsigned int offset;\n> > > > + unsigned int size;\n> > > > + };\n> > > > +\n> > > > + void map();\n> > > > +\n> > > > + int fd_;\n> > > > + int flags_;\n> > > > + off_t bufferLength_;\n> > > > + bool mapped_;\n> > > > + std::vector planeInfo_;\n> > > > /* \\todo Remove planes_ when it will be added to MappedBuffer */\n> > > > std::vector> planes_;\n> > > > };\n> > > > @@ -45,6 +57,7 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> > > > buffer_handle_t camera3Buffer,\n> > > > libcamera::PixelFormat pixelFormat,\n> > > > const libcamera::Size &size, int flags)\n> > > > + : fd_(-1), flags_(flags), mapped_(false)\n> > >\n> > > This should initialize bufferLength_ to 0, otherwise the ASSERT()s that\n> > > checks the variable will be pointless if the constructor returns an\n> > > error before setting bufferLength_.\n> >\n> > Shall I set bufferLength to -1 and then ASSERT(bufferLength_ >= 0), or\n> > set to 0 and ASSERT(bufferLength > 0)?\n> \n> Uploaded with the former. If you think the latter is correct, please\n> fix it upon applying.\n> Thanks in advance.\n\nI've mistakenly pushed this patch already. I'll turn yours into a fix\nand will send it to the list. Sorry about the mistake.\n\n> > > And it will also avoid a Coverity warning :-)\n> > >\n> > > I can handle this when pushing the series.\n> > >\n> > > Reviewed-by: Laurent Pinchart \n> > >\n> > > > {\n> > > > error_ = 0;\n> > > >\n> > > > @@ -61,43 +74,35 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> > > > * now that the buffer is backed by a single dmabuf, with planes being\n> > > > * stored contiguously.\n> > > > */\n> > > > - int fd = -1;\n> > > > for (int i = 0; i < camera3Buffer->numFds; i++) {\n> > > > - if (camera3Buffer->data[i] == -1 || camera3Buffer->data[i] == fd)\n> > > > + if (camera3Buffer->data[i] == -1 || camera3Buffer->data[i] == fd_)\n> > > > continue;\n> > > >\n> > > > - if (fd != -1) {\n> > > > + if (fd_ != -1) {\n> > > > error_ = -EINVAL;\n> > > > LOG(HAL, Error) << \"Discontiguous planes are not supported\";\n> > > > return;\n> > > > }\n> > > >\n> > > > - fd = camera3Buffer->data[i];\n> > > > + fd_ = camera3Buffer->data[i];\n> > > > }\n> > > >\n> > > > - if (fd == -1) {\n> > > > + if (fd_ == -1) {\n> > > > error_ = -EINVAL;\n> > > > LOG(HAL, Error) << \"No valid file descriptor\";\n> > > > return;\n> > > > }\n> > > >\n> > > > - off_t bufferLength = lseek(fd, 0, SEEK_END);\n> > > > - if (bufferLength < 0) {\n> > > > + bufferLength_ = lseek(fd_, 0, SEEK_END);\n> > > > + if (bufferLength_ < 0) {\n> > > > error_ = -errno;\n> > > > LOG(HAL, Error) << \"Failed to get buffer length\";\n> > > > return;\n> > > > }\n> > > >\n> > > > - void *address = mmap(nullptr, bufferLength, flags, MAP_SHARED, fd, 0);\n> > > > - if (address == MAP_FAILED) {\n> > > > - error_ = -errno;\n> > > > - LOG(HAL, Error) << \"Failed to mmap plane\";\n> > > > - return;\n> > > > - }\n> > > > - maps_.emplace_back(static_cast(address), bufferLength);\n> > > > -\n> > > > const unsigned int numPlanes = info.numPlanes();\n> > > > - planes_.resize(numPlanes);\n> > > > + planeInfo_.resize(numPlanes);\n> > > > +\n> > > > unsigned int offset = 0;\n> > > > for (unsigned int i = 0; i < numPlanes; ++i) {\n> > > > /*\n> > > > @@ -109,17 +114,17 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> > > > const unsigned int planeSize =\n> > > > stride * ((size.height + vertSubSample - 1) / vertSubSample);\n> > > >\n> > > > - planes_[i] = libcamera::Span(\n> > > > - static_cast(address) + offset, planeSize);\n> > > > + planeInfo_[i].offset = offset;\n> > > > + planeInfo_[i].size = planeSize;\n> > > >\n> > > > - if (bufferLength < offset + planeSize) {\n> > > > - error_ = -EINVAL;\n> > > > - LOG(HAL, Error) << \"Plane \" << i << \" is out of buffer\"\n> > > > - << \", buffer length=\" << bufferLength\n> > > > - << \", offset=\" << offset\n> > > > - << \", size=\" << planeSize;\n> > > > + if (bufferLength_ < offset + planeSize) {\n> > > > + LOG(HAL, Error) << \"Plane \" << i << \" is out of buffer:\"\n> > > > + << \" plane offset=\" << offset\n> > > > + << \", plane size=\" << planeSize\n> > > > + << \", buffer length=\" << bufferLength_;\n> > > > return;\n> > > > }\n> > > > +\n> > > > offset += planeSize;\n> > > > }\n> > > > }\n> > > > @@ -130,12 +135,14 @@ CameraBuffer::Private::~Private()\n> > > >\n> > > > unsigned int CameraBuffer::Private::numPlanes() const\n> > > > {\n> > > > - return planes_.size();\n> > > > + return planeInfo_.size();\n> > > > }\n> > > >\n> > > > Span CameraBuffer::Private::plane(unsigned int plane)\n> > > > {\n> > > > - if (plane >= planes_.size())\n> > > > + if (!mapped_)\n> > > > + map();\n> > > > + if (!mapped_)\n> > > > return {};\n> > > >\n> > > > return planes_[plane];\n> > > > @@ -143,8 +150,31 @@ Span CameraBuffer::Private::plane(unsigned int plane)\n> > > >\n> > > > size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const\n> > > > {\n> > > > - return std::min(maps_[0].size(),\n> > > > - maxJpegBufferSize);\n> > > > + ASSERT(bufferLength_ >= 0);\n> > > > +\n> > > > + return std::min(bufferLength_, maxJpegBufferSize);\n> > > > +}\n> > > > +\n> > > > +void CameraBuffer::Private::map()\n> > > > +{\n> > > > + ASSERT(fd_ != -1);\n> > > > + ASSERT(bufferLength_ >= 0);\n> > > > +\n> > > > + void *address = mmap(nullptr, bufferLength_, flags_, MAP_SHARED, fd_, 0);\n> > > > + if (address == MAP_FAILED) {\n> > > > + error_ = -errno;\n> > > > + LOG(HAL, Error) << \"Failed to mmap plane\";\n> > > > + return;\n> > > > + }\n> > > > + maps_.emplace_back(static_cast(address), bufferLength_);\n> > > > +\n> > > > + planes_.reserve(planeInfo_.size());\n> > > > + for (const auto &info : planeInfo_) {\n> > > > + planes_.emplace_back(\n> > > > + static_cast(address) + info.offset, info.size);\n> > > > + }\n> > > > +\n> > > > + mapped_ = true;\n> > > > }\n> > > >\n> > > > PUBLIC_CAMERA_BUFFER_IMPLEMENTATION","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 D14B6BD87D\n\tfor ;\n\tMon, 30 Aug 2021 15:01:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 285896916A;\n\tMon, 30 Aug 2021 17:01:11 +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 2227E60258\n\tfor ;\n\tMon, 30 Aug 2021 17:01:10 +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 897365A7;\n\tMon, 30 Aug 2021 17:01:09 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Z7mPleWU\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630335669;\n\tbh=pEr0j2QbMUPshYm1sWoBFlqBsp7V08uCw5a3iJmSUWw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Z7mPleWUUMQLWeoe6+pS955dLEJe2Rt7cwFoNqp7HIJgopjtBRhBlZ+eWO2QGjbxn\n\tW/8oJQJtXCp7MAPWWFPxCGb2QRd5k6OnD7Lbpv/WHGtgY4BMq3t9j4TQih215MCgjj\n\tPsBE2DqfaMeFJ4WB/tDcowfRdm8rbbscz7Yb8ZvA=","Date":"Mon, 30 Aug 2021 18:00:55 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210826080021.3454520-1-hiroh@chromium.org>\n\t<20210826080021.3454520-3-hiroh@chromium.org>\n\t\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] [PATCH v3 2/3] android: camera_buffer: Map\n\tbuffer in the first plane() call","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\" "}}]