[{"id":19038,"web_url":"https://patchwork.libcamera.org/comment/19038/","msgid":"<20210825084635.p3arszpl4sm2qdyn@uno.localdomain>","date":"2021-08-25T08:46:35","subject":"Re: [libcamera-devel] [PATCH v2 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 Wed, Aug 25, 2021 at 01:44:09PM +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 | 85 ++++++++++++++++--------\n> 2 files changed, 101 insertions(+), 55 deletions(-)\n>\n> diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp\n> index 50732637..42546d87 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> +\tbool map();\n\n void map();\n\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,12 @@ unsigned int CameraBuffer::Private::numPlanes() const\n>\n> Span CameraBuffer::Private::plane(unsigned int plane)\n> {\n> +\tif (!mapped_) {\n> +\t\tmapped_ = map();\n> +\t\tif (!mapped_)\n> +\t\t\treturn {};\n> +\t}\n\n if (!mapped_)\n map();\n if (!mapped_);\n return {};\n> +\n> \tvoid *addr;\n>\n> \tswitch (numPlanes()) {\n> @@ -134,4 +117,34 @@ size_t CameraBuffer::Private::jpegBufferSize([[maybe_unused]] size_t maxJpegBuff\n> \treturn bufferManager_->GetPlaneSize(handle_, 0);\n> }\n>\n> +bool CameraBuffer::Private::map()\n\nvoid\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 false;\n\n return;\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 false;\n\nDitto\n\n> +\t\t}\n> +\t\tbreak;\n> +\t}\n> +\tdefault:\n> +\t\tLOG(HAL, Error) << \"Invalid number of planes: \" << numPlanes_;\n> +\t\treturn false;\n\nDitto\n\n> +\t}\n> +\n> +\treturn true;\n\n mapped_ = true;\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 0bfdc2ba..37868d26 100644\n> --- a/src/android/mm/generic_camera_buffer.cpp\n> +++ b/src/android/mm/generic_camera_buffer.cpp\n> @@ -37,6 +37,17 @@ 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> +\tbool map();\n> +\n> +\tint fd_;\n> +\tint flags_;\n> +\toff_t bufferLength_;\n> +\tstd::vector planeInfo_;\n\nCan't you use a mapped_ like the cros implementation ?\n\n> \t/* \\todo Remove planes_ when it will be added to MappedBuffer */\n> \tstd::vector> planes_;\n> };\n> @@ -45,6 +56,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)\n> {\n> \terror_ = 0;\n> \t/*\n> @@ -52,19 +64,18 @@ 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 ||\n> -\t\t camera3Buffer->data[i] == fd) {\n> +\t\t camera3Buffer->data[i] == fd_) {\n> \t\t\tcontinue;\n> \t\t}\n>\n> -\t\tif (fd != -1)\n> +\t\tif (fd_ != -1)\n> \t\t\tLOG(HAL, Fatal) << \"Discontiguous planes are not supported\";\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> @@ -78,23 +89,16 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> \t\treturn;\n> \t}\n>\n> -\toff_t bufferLength = lseek(fd, 0, SEEK_END);\n> +\tsize_t bufferLength = lseek(fd_, 0, SEEK_END);\n> \tif (bufferLength < 0) {\n> -\t\terror_ = -errno;\n> +\t\terror_ = -EINVAL;\n\nWhy this change ?\n\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> @@ -106,17 +110,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\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> @@ -127,11 +131,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 (planes_.empty() && !map())\n> +\t\treturn {};\n> +\n\nAnd repeat the pattern used in cros implementation using a mapped_\nclass variable to keep the two implementation similar ?\n\n> \tif (plane >= planes_.size())\n> \t\treturn {};\n>\n> @@ -140,8 +147,34 @@ 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> +\tif (maps_.empty()) {\n> +\t\tASSERT(bufferLength_ >= 0);\n> +\t\treturn std::min(bufferLength_, maxJpegBufferSize);\n> +\t}\n> +\n> +\treturn std::min(maps_[0].size(), maxJpegBufferSize);\n> +}\n> +\n> +bool CameraBuffer::Private::map()\n> +{\n> +\tASSERT(fd_ != -1);\n\nShouldn't we fail earlier then ?\n\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 false;\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\nCan't this be done on costruction as it just basically transfer the\ninformation stored in planeInfo into planes_ rendering planeInfo_ just\na temporary storage ?\n\n> +\n> +\treturn 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 E6670BD87C\n\tfor ;\n\tWed, 25 Aug 2021 08:45:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6001B688A3;\n\tWed, 25 Aug 2021 10:45:49 +0200 (CEST)","from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[217.70.183.200])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4A5E360259\n\tfor ;\n\tWed, 25 Aug 2021 10:45:48 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay7-d.mail.gandi.net (Postfix) with ESMTPSA id 1DB5720006;\n\tWed, 25 Aug 2021 08:45:46 +0000 (UTC)"],"Date":"Wed, 25 Aug 2021 10:46:35 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20210825084635.p3arszpl4sm2qdyn@uno.localdomain>","References":"<20210825044410.2787433-1-hiroh@chromium.org>\n\t<20210825044410.2787433-3-hiroh@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210825044410.2787433-3-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH v2 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":19054,"web_url":"https://patchwork.libcamera.org/comment/19054/","msgid":"","date":"2021-08-25T11:02:45","subject":"Re: [libcamera-devel] [PATCH v2 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 Jacopo,\n\nOn Wed, Aug 25, 2021 at 5:45 PM Jacopo Mondi wrote:\n>\n> Hi Hiro,\n>\n> On Wed, Aug 25, 2021 at 01:44:09PM +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 | 85 ++++++++++++++++--------\n> > 2 files changed, 101 insertions(+), 55 deletions(-)\n> >\n> > diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp\n> > index 50732637..42546d87 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> > + bool map();\n>\n> void map();\n>\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,12 @@ unsigned int CameraBuffer::Private::numPlanes() const\n> >\n> > Span CameraBuffer::Private::plane(unsigned int plane)\n> > {\n> > + if (!mapped_) {\n> > + mapped_ = map();\n> > + if (!mapped_)\n> > + return {};\n> > + }\n>\n> if (!mapped_)\n> map();\n> if (!mapped_);\n> return {};\n> > +\n> > void *addr;\n> >\n> > switch (numPlanes()) {\n> > @@ -134,4 +117,34 @@ size_t CameraBuffer::Private::jpegBufferSize([[maybe_unused]] size_t maxJpegBuff\n> > return bufferManager_->GetPlaneSize(handle_, 0);\n> > }\n> >\n> > +bool CameraBuffer::Private::map()\n>\n> void\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 false;\n>\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 false;\n>\n> Ditto\n>\n> > + }\n> > + break;\n> > + }\n> > + default:\n> > + LOG(HAL, Error) << \"Invalid number of planes: \" << numPlanes_;\n> > + return false;\n>\n> Ditto\n>\n> > + }\n> > +\n> > + return true;\n>\n> mapped_ = true;\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 0bfdc2ba..37868d26 100644\n> > --- a/src/android/mm/generic_camera_buffer.cpp\n> > +++ b/src/android/mm/generic_camera_buffer.cpp\n> > @@ -37,6 +37,17 @@ 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> > + bool map();\n> > +\n> > + int fd_;\n> > + int flags_;\n> > + off_t bufferLength_;\n> > + std::vector planeInfo_;\n>\n> Can't you use a mapped_ like the cros implementation ?\n>\n> > /* \\todo Remove planes_ when it will be added to MappedBuffer */\n> > std::vector> planes_;\n> > };\n> > @@ -45,6 +56,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)\n> > {\n> > error_ = 0;\n> > /*\n> > @@ -52,19 +64,18 @@ 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 ||\n> > - camera3Buffer->data[i] == fd) {\n> > + camera3Buffer->data[i] == fd_) {\n> > continue;\n> > }\n> >\n> > - if (fd != -1)\n> > + if (fd_ != -1)\n> > LOG(HAL, Fatal) << \"Discontiguous planes are not supported\";\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> > @@ -78,23 +89,16 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> > return;\n> > }\n> >\n> > - off_t bufferLength = lseek(fd, 0, SEEK_END);\n> > + size_t bufferLength = lseek(fd_, 0, SEEK_END);\n> > if (bufferLength < 0) {\n> > - error_ = -errno;\n> > + error_ = -EINVAL;\n>\n> Why this change ?\n>\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> > @@ -106,17 +110,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> > + 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> > @@ -127,11 +131,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 (planes_.empty() && !map())\n> > + return {};\n> > +\n>\n> And repeat the pattern used in cros implementation using a mapped_\n> class variable to keep the two implementation similar ?\n>\n> > if (plane >= planes_.size())\n> > return {};\n> >\n> > @@ -140,8 +147,34 @@ 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> > + if (maps_.empty()) {\n> > + ASSERT(bufferLength_ >= 0);\n> > + return std::min(bufferLength_, maxJpegBufferSize);\n> > + }\n> > +\n> > + return std::min(maps_[0].size(), maxJpegBufferSize);\n> > +}\n> > +\n> > +bool CameraBuffer::Private::map()\n> > +{\n> > + ASSERT(fd_ != -1);\n>\n> Shouldn't we fail earlier then ?\n>\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 false;\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> Can't this be done on costruction as it just basically transfer the\n> information stored in planeInfo into planes_ rendering planeInfo_ just\n> a temporary storage ?\n>\n\naddress is needed to fill planes_ correctly.\nplaneInfo_ is used in stride/offset/size() in the next patch. Since it\nis useful even in this patch, I introduced it from this patch.\n\n-Hiro\n> > +\n> > + return 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 2EBDCBD87C\n\tfor ;\n\tWed, 25 Aug 2021 11:02:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 92CA968893;\n\tWed, 25 Aug 2021 13:02:58 +0200 (CEST)","from mail-ej1-x635.google.com (mail-ej1-x635.google.com\n\t[IPv6:2a00:1450:4864:20::635])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 93E3D60259\n\tfor ;\n\tWed, 25 Aug 2021 13:02:57 +0200 (CEST)","by mail-ej1-x635.google.com with SMTP id me10so22339891ejb.11\n\tfor ;\n\tWed, 25 Aug 2021 04:02:57 -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=\"VZyKoIkq\"; 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=yUXPv1d/TREeI1+URgxZFqJuiVLuCGYCOa1masYzLKI=;\n\tb=VZyKoIkqhOjkiwWgact7eyTRypVmCXrvnWi6swJv87lqIk7oWsZRhwv96PFLRssy5d\n\tu8MkCueBUL8GfwU+X3+GjYhDwqu2zkvvHx+8cSq+jlJVNbbUMHMlH2znbAnBGbgHjHiB\n\t+CT82y2WaYojg0wVTr0ysWYF9dhdsbj3BtQbY=","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=yUXPv1d/TREeI1+URgxZFqJuiVLuCGYCOa1masYzLKI=;\n\tb=guer1VfqTXMYDqDKgYU7kByo46bwlHqJ/aRp8RdmJ4V0GEO4/wTkXx1OCtfyBes4O7\n\t2OvTWLWKiPJt4hPtmzyG6eD6h+CvG5NGTnMqxlKC1PrHrCmWho7rZ5LZXM+QDJVqJsPZ\n\tiAs63u4vIxnl6ktr1+ypx8h9dCKwvVDDAE2ZI9HT5Rib//Ca1G1hrcdT6BrAyh5oi4in\n\tKcSKvQ5RxkbnmV94qE3iOglHsdBy2HqVjFvvR7CcTT7zdtB6lKMLRWKZnSTrxtzBUA95\n\tr89gUKV+0gcrk9U8OFP9v0z0RsVe9e9S5BD6czG7/Oxd/mpuFqH/V/phkwEeHuLTiAb/\n\tCcdA==","X-Gm-Message-State":"AOAM533SKlIwWBZbAoTq81bi/PCpQwnnxElWnlqOe/IGvvQwDnBz8UJi\n\tQF3z5Fj4+Xv/SA9Q9BAps/g/0T/pNZVy+PUrW32OJA==","X-Google-Smtp-Source":"ABdhPJy/ogPvcOcYQthYEW68CymlwEM5YN/H95V0cbnFVqk/CjmjJUrG6GKiE6tuJ0ycwSsaTYpbKMuD11rPeqm3Gi0=","X-Received":"by 2002:a17:907:2168:: with SMTP id\n\trl8mr24106427ejb.42.1629889376809; \n\tWed, 25 Aug 2021 04:02:56 -0700 (PDT)","MIME-Version":"1.0","References":"<20210825044410.2787433-1-hiroh@chromium.org>\n\t<20210825044410.2787433-3-hiroh@chromium.org>\n\t<20210825084635.p3arszpl4sm2qdyn@uno.localdomain>","In-Reply-To":"<20210825084635.p3arszpl4sm2qdyn@uno.localdomain>","From":"Hirokazu Honda ","Date":"Wed, 25 Aug 2021 20:02:45 +0900","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 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":19073,"web_url":"https://patchwork.libcamera.org/comment/19073/","msgid":"","date":"2021-08-25T20:33:23","subject":"Re: [libcamera-devel] [PATCH v2 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 Wed, Aug 25, 2021 at 08:02:45PM +0900, Hirokazu Honda wrote:\n> On Wed, Aug 25, 2021 at 5:45 PM Jacopo Mondi wrote:\n> > On Wed, Aug 25, 2021 at 01:44:09PM +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 | 85 ++++++++++++++++--------\n> > > 2 files changed, 101 insertions(+), 55 deletions(-)\n> > >\n> > > diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp\n> > > index 50732637..42546d87 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> > > + bool map();\n> >\n> > void map();\n> >\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,12 @@ unsigned int CameraBuffer::Private::numPlanes() const\n> > >\n> > > Span CameraBuffer::Private::plane(unsigned int plane)\n> > > {\n> > > + if (!mapped_) {\n> > > + mapped_ = map();\n> > > + if (!mapped_)\n> > > + return {};\n> > > + }\n> >\n> > if (!mapped_)\n> > map();\n> > if (!mapped_);\n> > return {};\n> > > +\n> > > void *addr;\n> > >\n> > > switch (numPlanes()) {\n> > > @@ -134,4 +117,34 @@ size_t CameraBuffer::Private::jpegBufferSize([[maybe_unused]] size_t maxJpegBuff\n> > > return bufferManager_->GetPlaneSize(handle_, 0);\n> > > }\n> > >\n> > > +bool CameraBuffer::Private::map()\n> >\n> > void\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 false;\n> >\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 false;\n> >\n> > Ditto\n> >\n> > > + }\n> > > + break;\n> > > + }\n> > > + default:\n> > > + LOG(HAL, Error) << \"Invalid number of planes: \" << numPlanes_;\n> > > + return false;\n> >\n> > Ditto\n> >\n> > > + }\n> > > +\n> > > + return true;\n> >\n> > mapped_ = true;\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 0bfdc2ba..37868d26 100644\n> > > --- a/src/android/mm/generic_camera_buffer.cpp\n> > > +++ b/src/android/mm/generic_camera_buffer.cpp\n> > > @@ -37,6 +37,17 @@ 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> > > + bool map();\n> > > +\n> > > + int fd_;\n> > > + int flags_;\n> > > + off_t bufferLength_;\n> > > + std::vector planeInfo_;\n> >\n> > Can't you use a mapped_ like the cros implementation ?\n> >\n> > > /* \\todo Remove planes_ when it will be added to MappedBuffer */\n> > > std::vector> planes_;\n> > > };\n> > > @@ -45,6 +56,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)\n> > > {\n> > > error_ = 0;\n> > > /*\n> > > @@ -52,19 +64,18 @@ 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 ||\n> > > - camera3Buffer->data[i] == fd) {\n> > > + camera3Buffer->data[i] == fd_) {\n> > > continue;\n> > > }\n> > >\n> > > - if (fd != -1)\n> > > + if (fd_ != -1)\n> > > LOG(HAL, Fatal) << \"Discontiguous planes are not supported\";\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> > > @@ -78,23 +89,16 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> > > return;\n> > > }\n> > >\n> > > - off_t bufferLength = lseek(fd, 0, SEEK_END);\n> > > + size_t bufferLength = lseek(fd_, 0, SEEK_END);\n\ns/size_t/off_t/ or you'll have a warning with the next line.\n\n> > > if (bufferLength < 0) {\n> > > - error_ = -errno;\n> > > + error_ = -EINVAL;\n> >\n> > Why this change ?\n> >\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> > > @@ -106,17 +110,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> > > + 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> > > @@ -127,11 +131,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 (planes_.empty() && !map())\n> > > + return {};\n> > > +\n> >\n> > And repeat the pattern used in cros implementation using a mapped_\n> > class variable to keep the two implementation similar ?\n> >\n> > > if (plane >= planes_.size())\n> > > return {};\n> > >\n> > > @@ -140,8 +147,34 @@ 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> > > + if (maps_.empty()) {\n> > > + ASSERT(bufferLength_ >= 0);\n> > > + return std::min(bufferLength_, maxJpegBufferSize);\n> > > + }\n> > > +\n> > > + return std::min(maps_[0].size(), maxJpegBufferSize);\n\nGiven that maps_[0].size() == bufferLength_, I think this whole function\ncan be simplified to\n\n\treturn std::min(bufferLength_, maxJpegBufferSize);\n\n> > > +}\n> > > +\n> > > +bool CameraBuffer::Private::map()\n> > > +{\n> > > + ASSERT(fd_ != -1);\n> >\n> > Shouldn't we fail earlier then ?\n> >\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 false;\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> > Can't this be done on costruction as it just basically transfer the\n> > information stored in planeInfo into planes_ rendering planeInfo_ just\n> > a temporary storage ?\n> \n> address is needed to fill planes_ correctly.\n> planeInfo_ is used in stride/offset/size() in the next patch. Since it\n> is useful even in this patch, I introduced it from this patch.\n\nAnother option would be to store the mmap()ed address in a single void\n*address_ class member, drop maps_, and change the plane() function to\n\nSpan CameraBuffer::Private::plane(unsigned int plane)\n{\n if (plane >= planeInfo_.size())\n return {};\n\n\tconst PlaneInfo &info = planeInfo_[plane];\n\treturn { static_cast(address_) + info.offset, info.size };\n}\n\nYet another option would be to store { info.offset, info.size } in planes_\nin the constructor, and then add address to the offset in this function.\n\n> > > +\n> > > + return 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 E58FFBD87D\n\tfor ;\n\tWed, 25 Aug 2021 20:33:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 58AB168893;\n\tWed, 25 Aug 2021 22:33:37 +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 9431860288\n\tfor ;\n\tWed, 25 Aug 2021 22:33:35 +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 0235E24F;\n\tWed, 25 Aug 2021 22:33:34 +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=\"YJu8PtwL\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629923615;\n\tbh=xdB4ygMEphrGY77i6UdYP6FfjRlOp5Ti54+ETwnAUXA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=YJu8PtwLazJUigFOEhPpNub5X0uV7a7vZ7K3pNTLDd7axiryePa1UGDfBHwCK0WGV\n\tIyhTobuZt/Nx/pQiVy71WGTSW1ubXm4R0qPNVdL7a44+rRHrftpZLzS6YADbYule9S\n\tWMEhX+zjNOtollfNj7Lo+F2mWKA8w1quWNTLRwwo=","Date":"Wed, 25 Aug 2021 23:33:23 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210825044410.2787433-1-hiroh@chromium.org>\n\t<20210825044410.2787433-3-hiroh@chromium.org>\n\t<20210825084635.p3arszpl4sm2qdyn@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 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":19075,"web_url":"https://patchwork.libcamera.org/comment/19075/","msgid":"","date":"2021-08-25T20:38:58","subject":"Re: [libcamera-devel] [PATCH v2 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":"On Wed, Aug 25, 2021 at 11:33:24PM +0300, Laurent Pinchart wrote:\n> Hi Hiro,\n> \n> Thank you for the patch.\n> \n> On Wed, Aug 25, 2021 at 08:02:45PM +0900, Hirokazu Honda wrote:\n> > On Wed, Aug 25, 2021 at 5:45 PM Jacopo Mondi wrote:\n> > > On Wed, Aug 25, 2021 at 01:44:09PM +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 | 85 ++++++++++++++++--------\n> > > > 2 files changed, 101 insertions(+), 55 deletions(-)\n> > > >\n> > > > diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp\n> > > > index 50732637..42546d87 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> > > > + bool map();\n> > >\n> > > void map();\n> > >\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,12 @@ unsigned int CameraBuffer::Private::numPlanes() const\n> > > >\n> > > > Span CameraBuffer::Private::plane(unsigned int plane)\n> > > > {\n> > > > + if (!mapped_) {\n> > > > + mapped_ = map();\n> > > > + if (!mapped_)\n> > > > + return {};\n> > > > + }\n> > >\n> > > if (!mapped_)\n> > > map();\n> > > if (!mapped_);\n> > > return {};\n> > > > +\n> > > > void *addr;\n> > > >\n> > > > switch (numPlanes()) {\n> > > > @@ -134,4 +117,34 @@ size_t CameraBuffer::Private::jpegBufferSize([[maybe_unused]] size_t maxJpegBuff\n> > > > return bufferManager_->GetPlaneSize(handle_, 0);\n> > > > }\n> > > >\n> > > > +bool CameraBuffer::Private::map()\n> > >\n> > > void\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 false;\n> > >\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 false;\n> > >\n> > > Ditto\n> > >\n> > > > + }\n> > > > + break;\n> > > > + }\n> > > > + default:\n> > > > + LOG(HAL, Error) << \"Invalid number of planes: \" << numPlanes_;\n> > > > + return false;\n> > >\n> > > Ditto\n> > >\n> > > > + }\n> > > > +\n> > > > + return true;\n> > >\n> > > mapped_ = true;\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 0bfdc2ba..37868d26 100644\n> > > > --- a/src/android/mm/generic_camera_buffer.cpp\n> > > > +++ b/src/android/mm/generic_camera_buffer.cpp\n> > > > @@ -37,6 +37,17 @@ 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> > > > + bool map();\n> > > > +\n> > > > + int fd_;\n> > > > + int flags_;\n> > > > + off_t bufferLength_;\n> > > > + std::vector planeInfo_;\n> > >\n> > > Can't you use a mapped_ like the cros implementation ?\n> > >\n> > > > /* \\todo Remove planes_ when it will be added to MappedBuffer */\n> > > > std::vector> planes_;\n> > > > };\n> > > > @@ -45,6 +56,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)\n> > > > {\n> > > > error_ = 0;\n> > > > /*\n> > > > @@ -52,19 +64,18 @@ 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 ||\n> > > > - camera3Buffer->data[i] == fd) {\n> > > > + camera3Buffer->data[i] == fd_) {\n> > > > continue;\n> > > > }\n> > > >\n> > > > - if (fd != -1)\n> > > > + if (fd_ != -1)\n> > > > LOG(HAL, Fatal) << \"Discontiguous planes are not supported\";\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> > > > @@ -78,23 +89,16 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> > > > return;\n> > > > }\n> > > >\n> > > > - off_t bufferLength = lseek(fd, 0, SEEK_END);\n> > > > + size_t bufferLength = lseek(fd_, 0, SEEK_END);\n> \n> s/size_t/off_t/ or you'll have a warning with the next line.\n> \n> > > > if (bufferLength < 0) {\n> > > > - error_ = -errno;\n> > > > + error_ = -EINVAL;\n> > >\n> > > Why this change ?\n> > >\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> > > > @@ -106,17 +110,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> > > > + 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> > > > @@ -127,11 +131,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 (planes_.empty() && !map())\n> > > > + return {};\n> > > > +\n> > >\n> > > And repeat the pattern used in cros implementation using a mapped_\n> > > class variable to keep the two implementation similar ?\n> > >\n> > > > if (plane >= planes_.size())\n> > > > return {};\n> > > >\n> > > > @@ -140,8 +147,34 @@ 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> > > > + if (maps_.empty()) {\n> > > > + ASSERT(bufferLength_ >= 0);\n> > > > + return std::min(bufferLength_, maxJpegBufferSize);\n> > > > + }\n> > > > +\n> > > > + return std::min(maps_[0].size(), maxJpegBufferSize);\n> \n> Given that maps_[0].size() == bufferLength_, I think this whole function\n> can be simplified to\n> \n> \treturn std::min(bufferLength_, maxJpegBufferSize);\n> \n> > > > +}\n> > > > +\n> > > > +bool CameraBuffer::Private::map()\n> > > > +{\n> > > > + ASSERT(fd_ != -1);\n> > >\n> > > Shouldn't we fail earlier then ?\n> > >\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 false;\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> > > Can't this be done on costruction as it just basically transfer the\n> > > information stored in planeInfo into planes_ rendering planeInfo_ just\n> > > a temporary storage ?\n> > \n> > address is needed to fill planes_ correctly.\n> > planeInfo_ is used in stride/offset/size() in the next patch. Since it\n> > is useful even in this patch, I introduced it from this patch.\n> \n> Another option would be to store the mmap()ed address in a single void\n> *address_ class member, drop maps_, and change the plane() function to\n> \n> Span CameraBuffer::Private::plane(unsigned int plane)\n> {\n> if (plane >= planeInfo_.size())\n> return {};\n> \n> \tconst PlaneInfo &info = planeInfo_[plane];\n> \treturn { static_cast(address_) + info.offset, info.size };\n> }\n> \n> Yet another option would be to store { info.offset, info.size } in planes_\n> in the constructor, and then add address to the offset in this function.\n\nNow that I've read patch 3/3, I think there will be a bit of additional\nrefactoring of the MappedBuffer class that I'd like to do on top of\nthis, so I'm fine with any of the above options.\n\n> > > > +\n> > > > + return 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 78EC0BD87C\n\tfor ;\n\tWed, 25 Aug 2021 20:39:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CE5C668893;\n\tWed, 25 Aug 2021 22:39:12 +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 5BB7860288\n\tfor ;\n\tWed, 25 Aug 2021 22:39:11 +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 B676824F;\n\tWed, 25 Aug 2021 22:39:10 +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=\"X9cdbH0a\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629923950;\n\tbh=RKQvNbCkVooXTeZkaxTjn1B7oGagdEBPz0Dx4dWY5f8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=X9cdbH0axG+8zJs4dZpGlSrtr0ll641J/RlF3psyBDN2phrdPmczKYG09mXvWU7YO\n\tqMIBQywmG3PP+zKMv/13c+B6UpFM043BYwtzSh29S9qY5qglrxhNt9chX4W28ohFrF\n\tJ1PsZUPuNsB+fMmG9dIETK3DjqoTULBpnoxDQDP0=","Date":"Wed, 25 Aug 2021 23:38:58 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210825044410.2787433-1-hiroh@chromium.org>\n\t<20210825044410.2787433-3-hiroh@chromium.org>\n\t<20210825084635.p3arszpl4sm2qdyn@uno.localdomain>\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 v2 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":19087,"web_url":"https://patchwork.libcamera.org/comment/19087/","msgid":"","date":"2021-08-26T08:00:19","subject":"Re: [libcamera-devel] [PATCH v2 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 5:39 AM Laurent Pinchart\n wrote:\n>\n> On Wed, Aug 25, 2021 at 11:33:24PM +0300, Laurent Pinchart wrote:\n> > Hi Hiro,\n> >\n> > Thank you for the patch.\n> >\n> > On Wed, Aug 25, 2021 at 08:02:45PM +0900, Hirokazu Honda wrote:\n> > > On Wed, Aug 25, 2021 at 5:45 PM Jacopo Mondi wrote:\n> > > > On Wed, Aug 25, 2021 at 01:44:09PM +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 | 85 ++++++++++++++++--------\n> > > > > 2 files changed, 101 insertions(+), 55 deletions(-)\n> > > > >\n> > > > > diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp\n> > > > > index 50732637..42546d87 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> > > > > + bool map();\n> > > >\n> > > > void map();\n> > > >\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,12 @@ unsigned int CameraBuffer::Private::numPlanes() const\n> > > > >\n> > > > > Span CameraBuffer::Private::plane(unsigned int plane)\n> > > > > {\n> > > > > + if (!mapped_) {\n> > > > > + mapped_ = map();\n> > > > > + if (!mapped_)\n> > > > > + return {};\n> > > > > + }\n> > > >\n> > > > if (!mapped_)\n> > > > map();\n> > > > if (!mapped_);\n> > > > return {};\n> > > > > +\n> > > > > void *addr;\n> > > > >\n> > > > > switch (numPlanes()) {\n> > > > > @@ -134,4 +117,34 @@ size_t CameraBuffer::Private::jpegBufferSize([[maybe_unused]] size_t maxJpegBuff\n> > > > > return bufferManager_->GetPlaneSize(handle_, 0);\n> > > > > }\n> > > > >\n> > > > > +bool CameraBuffer::Private::map()\n> > > >\n> > > > void\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 false;\n> > > >\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 false;\n> > > >\n> > > > Ditto\n> > > >\n> > > > > + }\n> > > > > + break;\n> > > > > + }\n> > > > > + default:\n> > > > > + LOG(HAL, Error) << \"Invalid number of planes: \" << numPlanes_;\n> > > > > + return false;\n> > > >\n> > > > Ditto\n> > > >\n> > > > > + }\n> > > > > +\n> > > > > + return true;\n> > > >\n> > > > mapped_ = true;\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 0bfdc2ba..37868d26 100644\n> > > > > --- a/src/android/mm/generic_camera_buffer.cpp\n> > > > > +++ b/src/android/mm/generic_camera_buffer.cpp\n> > > > > @@ -37,6 +37,17 @@ 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> > > > > + bool map();\n> > > > > +\n> > > > > + int fd_;\n> > > > > + int flags_;\n> > > > > + off_t bufferLength_;\n> > > > > + std::vector planeInfo_;\n> > > >\n> > > > Can't you use a mapped_ like the cros implementation ?\n> > > >\n> > > > > /* \\todo Remove planes_ when it will be added to MappedBuffer */\n> > > > > std::vector> planes_;\n> > > > > };\n> > > > > @@ -45,6 +56,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)\n> > > > > {\n> > > > > error_ = 0;\n> > > > > /*\n> > > > > @@ -52,19 +64,18 @@ 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 ||\n> > > > > - camera3Buffer->data[i] == fd) {\n> > > > > + camera3Buffer->data[i] == fd_) {\n> > > > > continue;\n> > > > > }\n> > > > >\n> > > > > - if (fd != -1)\n> > > > > + if (fd_ != -1)\n> > > > > LOG(HAL, Fatal) << \"Discontiguous planes are not supported\";\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> > > > > @@ -78,23 +89,16 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> > > > > return;\n> > > > > }\n> > > > >\n> > > > > - off_t bufferLength = lseek(fd, 0, SEEK_END);\n> > > > > + size_t bufferLength = lseek(fd_, 0, SEEK_END);\n> >\n> > s/size_t/off_t/ or you'll have a warning with the next line.\n> >\n> > > > > if (bufferLength < 0) {\n> > > > > - error_ = -errno;\n> > > > > + error_ = -EINVAL;\n> > > >\n> > > > Why this change ?\n> > > >\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> > > > > @@ -106,17 +110,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> > > > > + 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> > > > > @@ -127,11 +131,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 (planes_.empty() && !map())\n> > > > > + return {};\n> > > > > +\n> > > >\n> > > > And repeat the pattern used in cros implementation using a mapped_\n> > > > class variable to keep the two implementation similar ?\n> > > >\n> > > > > if (plane >= planes_.size())\n> > > > > return {};\n> > > > >\n> > > > > @@ -140,8 +147,34 @@ 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> > > > > + if (maps_.empty()) {\n> > > > > + ASSERT(bufferLength_ >= 0);\n> > > > > + return std::min(bufferLength_, maxJpegBufferSize);\n> > > > > + }\n> > > > > +\n> > > > > + return std::min(maps_[0].size(), maxJpegBufferSize);\n> >\n> > Given that maps_[0].size() == bufferLength_, I think this whole function\n> > can be simplified to\n> >\n> > return std::min(bufferLength_, maxJpegBufferSize);\n> >\n> > > > > +}\n> > > > > +\n> > > > > +bool CameraBuffer::Private::map()\n> > > > > +{\n> > > > > + ASSERT(fd_ != -1);\n> > > >\n> > > > Shouldn't we fail earlier then ?\n> > > >\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 false;\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> > > > Can't this be done on costruction as it just basically transfer the\n> > > > information stored in planeInfo into planes_ rendering planeInfo_ just\n> > > > a temporary storage ?\n> > >\n> > > address is needed to fill planes_ correctly.\n> > > planeInfo_ is used in stride/offset/size() in the next patch. Since it\n> > > is useful even in this patch, I introduced it from this patch.\n> >\n> > Another option would be to store the mmap()ed address in a single void\n> > *address_ class member, drop maps_, and change the plane() function to\n> >\n> > Span CameraBuffer::Private::plane(unsigned int plane)\n> > {\n> > if (plane >= planeInfo_.size())\n> > return {};\n> >\n> > const PlaneInfo &info = planeInfo_[plane];\n> > return { static_cast(address_) + info.offset, info.size };\n> > }\n> >\n> > Yet another option would be to store { info.offset, info.size } in planes_\n> > in the constructor, and then add address to the offset in this function.\n>\n> Now that I've read patch 3/3, I think there will be a bit of additional\n> refactoring of the MappedBuffer class that I'd like to do on top of\n> this, so I'm fine with any of the above options.\n>\n\nCameraBuffer::Private in generic_camera_buffer.cpp inherits MappedBuffer.\nIt will have planes for address in my another patch series. I think it\nis better to temporarily have planes_ here and store it addresses.\n\n-Hiro\n> > > > > +\n> > > > > + return 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 E10C5BD87C\n\tfor ;\n\tThu, 26 Aug 2021 08:00:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 98122688AC;\n\tThu, 26 Aug 2021 10:00:33 +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 F02CF68892\n\tfor ;\n\tThu, 26 Aug 2021 10:00:30 +0200 (CEST)","by mail-ej1-x634.google.com with SMTP id u14so4242319ejf.13\n\tfor ;\n\tThu, 26 Aug 2021 01:00:30 -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=\"C2B9IbiA\"; 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=VJeRnlj6Lu4OgMxltoBwJ4DsT7gjfkrRthdLrJyPsg4=;\n\tb=C2B9IbiAETqN7ZXWjOW0GYnqApDnsGLQbPyZRzo+0vghMO8+o8RhzRh/1/DlDxCbQb\n\tR438FtgNUbdv+AjzLq7kqeh1sSdG+wkPqGFz1savdEt69dEzAYMVb0Acn1hhdlRio52Z\n\tsowfa3uv1ZBW/TVY0wu0WIbJtL/x7gNZ/ciRA=","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=VJeRnlj6Lu4OgMxltoBwJ4DsT7gjfkrRthdLrJyPsg4=;\n\tb=tWj5ohlBLli/crG4s1VkaBLxRy31NmfKjdzXsubOyBEdA5hiCV7N02Xl/TfnZd9jno\n\tw/zlHM5jvdNjkp128u7DeGy/e9bTclNOtAT7M/bsvykBAdYCuvx+FIw7f5IlLeTb6c5j\n\tp57INhmjwTBs/z5ncI2JGsi2xLq9m1pWChI09nkBxlCw+EjDL01GMIc3/9hIQH6hnq6S\n\tYtbppIFNH2JhEA/yG8sljJu1zJznBBGtvnYugchtLVF0Dqudf9LsxPSVL+aKkR1j2b6C\n\tPteZtBEG0ntzQ1uVZtE0QoDIG7rcJvoDmdio3eq9E66W9kOxJwPR8fTrKtFb982CHAAM\n\tYgiA==","X-Gm-Message-State":"AOAM533+4OyV5GujQbmuyJBLr9agqdCEMIFoLJ5o+8gnpYX8M2+jl1Vk\n\ttWl1Po82ImuyShnHV8/7jedlX1IavqkTdANsC5l8+w==","X-Google-Smtp-Source":"ABdhPJzUPECP5bCTg7LNmSnn5Nou3onZkAR4ioNAwTeVpd8C4HjuHodhYYQ0wT0tyPSRzznO23eBXh7D06dKglACyLw=","X-Received":"by 2002:a17:907:969f:: with SMTP id\n\thd31mr2860452ejc.475.1629964830388; \n\tThu, 26 Aug 2021 01:00:30 -0700 (PDT)","MIME-Version":"1.0","References":"<20210825044410.2787433-1-hiroh@chromium.org>\n\t<20210825044410.2787433-3-hiroh@chromium.org>\n\t<20210825084635.p3arszpl4sm2qdyn@uno.localdomain>\n\t\n\t\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Thu, 26 Aug 2021 17:00:19 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 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\" "}}]