[{"id":15339,"web_url":"https://patchwork.libcamera.org/comment/15339/","msgid":"","date":"2021-02-28T19:03:01","subject":"Re: [libcamera-devel] [PATCH 11/12] android: Introduce\n\tcros_cbm_buffer","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Fri, Feb 26, 2021 at 02:29:31PM +0100, Jacopo Mondi wrote:\n> Introduce the CameraBuffer backend for the ChromeOS operating system.\n> \n> The cros_cbm CameraBuffer implementation uses the ChromeOS\n> CameraBufferManager class to perform mapping of 1 plane and multiplane\n> buffers.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/android/mm/cros_cbm.cpp | 167 ++++++++++++++++++++++++++++++++++++\n> src/android/mm/meson.build | 3 +\n> 2 files changed, 170 insertions(+)\n> create mode 100644 src/android/mm/cros_cbm.cpp\n> \n> diff --git a/src/android/mm/cros_cbm.cpp b/src/android/mm/cros_cbm.cpp\n> new file mode 100644\n> index 000000000000..6c931c99d2d0\n> --- /dev/null\n> +++ b/src/android/mm/cros_cbm.cpp\n> @@ -0,0 +1,167 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Google Inc.\n> + *\n> + * cros_cbm.cpp - ChromiumOS libcbm frame buffer backend\n\ns/ChromiumOS/Chromium OS/\n\ncbm stands for CameraBufferManager, which you use below. Maybe there\nused to be a cbm library, but the class is now part of libcros_camera.\nThere's still libgbm, upon which the cross::CameraBufferManager is\nimplemented, but that's an implementation detail. I'd thus write\n\n * cros_cbm.cpp - Chromium OS CameraBufferManager frame buffer backend\n\nor maybe\n\n * cros_cbm.cpp - Chromium OS frame buffer backend using CameraBufferManager\n\n> + */\n> +\n> +#include \"../camera_buffer.h\"\n> +\n> +#include \"libcamera/internal/log.h\"\n> +\n> +#include \"cros-camera/camera_buffer_manager.h\"\n> +\n> +using namespace libcamera;\n> +\n> +LOG_DECLARE_CATEGORY(HAL)\n> +\n> +class CameraBuffer::CameraBufferImpl : public libcamera::MappedBuffer\n> +{\n> +public:\n> +\tCameraBufferImpl(buffer_handle_t camera3Buffer, int flags);\n> +\t~CameraBufferImpl();\n> +\n> +\tbool isValid() const { return valid_; }\n> +\n> +\tunsigned int numPlanes() const;\n> +\tssize_t planeSize(unsigned int plane) const;\n> +\n> +\tconst uint8_t *plane(unsigned int plane) const;\n> +\tuint8_t *plane(unsigned int plane);\n> +\n> +private:\n> +\tcros::CameraBufferManager *bufferManager_;\n> +\tbuffer_handle_t handle_;\n> +\tunsigned int numPlanes_;\n> +\tbool valid_;\n> +\tunion {\n> +\t\tvoid *addr;\n> +\t\tandroid_ycbcr ycbcr;\n> +\t} mem;\n> +};\n> +\n> +CameraBuffer::CameraBufferImpl::CameraBufferImpl(buffer_handle_t camera3Buffer,\n> +\t\t\t\t\t\t int flags)\n> +\t: handle_(camera3Buffer), numPlanes_(0), valid_(false)\n> +{\n> +\tbufferManager_ = cros::CameraBufferManager::GetInstance();\n> +\n> +\tbufferManager_->Register(camera3Buffer);\n> +\n> +\tnumPlanes_ = bufferManager_->GetNumPlanes(camera3Buffer);\n> +\tswitch (numPlanes_) {\n> +\tcase 1: {\n> +\t\tint ret = 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\nThat's a weird placement for the brace, does it work if you move it\nafter case 3 ?\n\n> +\tcase 3:\n> +\t\tint ret = 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\nI'm tempted to delay the mapping to the first plane() call, in order to\nwrap all buffers we get from the camera service in the CameraBuffer\nclass and use it through the HAL implementation. That's for later\nthough, not this series.\n\n> +\n> +\tvalid_ = true;\n> +}\n> +\n> +CameraBuffer::CameraBufferImpl::~CameraBufferImpl()\n> +{\n> +\tbufferManager_->Unlock(handle_);\n> +\tbufferManager_->Deregister(handle_);\n> +}\n> +\n> +unsigned int CameraBuffer::CameraBufferImpl::numPlanes() const\n> +{\n> +\treturn bufferManager_->GetNumPlanes(handle_);\n> +}\n> +\n> +ssize_t CameraBuffer::CameraBufferImpl::planeSize(unsigned int plane) const\n> +{\n> +\tif (plane >= numPlanes())\n> +\t\treturn -EINVAL;\n> +\n> +\treturn bufferManager_->GetPlaneSize(handle_, plane);\n> +}\n> +\n> +const uint8_t *CameraBuffer::CameraBufferImpl::plane(unsigned int plane) const\n> +{\n> +\tif (plane >= numPlanes())\n> +\t\treturn nullptr;\n> +\n\nYou can drop the check, it's in the next function.\n\n> +\treturn const_cast(this->plane(plane));\n\nI think this will cause an infinite loop as you're calling the same\nfunction.\n\n\tCameraBuffer::CameraBufferImpl *self =\n\t\tconst_cast(this);\n\treturn const_cast(self->plane(plane));\n\n> +}\n> +\n> +uint8_t *CameraBuffer::CameraBufferImpl::plane(unsigned int plane)\n> +{\n> +\tif (plane >= numPlanes())\n> +\t\treturn nullptr;\n> +\n> +\tvoid *addr;\n> +\tswitch (numPlanes()) {\n> +\tcase 1:\n> +\t\taddr = mem.addr;\n> +\t\tbreak;\n> +\tdefault:\n> +\t\tswitch (plane) {\n> +\t\tcase 1:\n> +\t\t\taddr = mem.ycbcr.y;\n> +\t\t\tbreak;\n> +\t\tcase 2:\n> +\t\t\taddr = mem.ycbcr.cb;\n> +\t\t\tbreak;\n> +\t\tcase 3:\n> +\t\t\taddr = mem.ycbcr.cr;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +\n> +\treturn static_cast(addr);\n> +}\n> +\n> +CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags)\n> +\t: impl_(new CameraBuffer::CameraBufferImpl(camera3Buffer, flags))\n> +{\n> +}\n> +\n> +CameraBuffer::~CameraBuffer()\n> +{\n> +\tdelete impl_;\n> +}\n> +\n> +bool CameraBuffer::isValid() const\n> +{\n> +\treturn impl_->isValid();\n> +}\n> +\n> +unsigned int CameraBuffer::numPlanes() const\n> +{\n> +\treturn impl_->numPlanes();\n> +}\n> +\n> +ssize_t CameraBuffer::planeSize(unsigned int plane) const\n> +{\n> +\treturn impl_->planeSize(plane);\n> +}\n> +\n> +const uint8_t *CameraBuffer::plane(unsigned int plane) const\n> +{\n> +\treturn impl_->plane(plane);\n> +}\n> +\n> +uint8_t *CameraBuffer::plane(unsigned int plane)\n> +{\n> +\treturn impl_->plane(plane);\n> +}\n> +\n\nThe blank line isn't needed.\n\n> diff --git a/src/android/mm/meson.build b/src/android/mm/meson.build\n> index 39be8fec8567..a801c54ff4f4 100644\n> --- a/src/android/mm/meson.build\n> +++ b/src/android/mm/meson.build\n> @@ -3,4 +3,7 @@\n> memory_backend = get_option('android_memory_backend')\n> if memory_backend == 'android_generic'\n> android_hal_sources += files(['android_generic_buffer.cpp'])\n> +elif memory_backend == 'cros_cbm'\n> + android_hal_sources += files(['cros_cbm.cpp'])\n> + android_deps += [ dependency('libcros_camera') ]\n\nNo spaces within [ ].\n\n> endif","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 88A37BD808\n\tfor ;\n\tSun, 28 Feb 2021 19:03:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 15CDD68A7E;\n\tSun, 28 Feb 2021 20:03:32 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7CD4768A45\n\tfor ;\n\tSun, 28 Feb 2021 20:03:30 +0100 (CET)","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 E04F080F;\n\tSun, 28 Feb 2021 20:03:29 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"niF7yJNq\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1614539010;\n\tbh=oyXHYnRhHT/IhORCdNUrZ09QET9ViIEWSJ3U9zIizfs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=niF7yJNqn5j4A+UvsndoMGWSWLKykHxHTbwN8EpICj6xQVV7atvZ5hrJOJE22PIPw\n\tNBon5yWnZcuTqPNceHC66/IP30Sb0gOR76Th0uHP6cuj38PWJ1ZDjdDjD+2khVGYWR\n\tihEfLWD3Et2dS1aMpOsyRsBhsmaa72r4WBbtW744=","Date":"Sun, 28 Feb 2021 21:03:01 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210226132932.165484-1-jacopo@jmondi.org>\n\t<20210226132932.165484-12-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210226132932.165484-12-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH 11/12] android: Introduce\n\tcros_cbm_buffer","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]