[{"id":15374,"web_url":"https://patchwork.libcamera.org/comment/15374/","msgid":"","date":"2021-03-02T00:15:11","subject":"Re: [libcamera-devel] [PATCH 10/10] android: Introduce Chromium OS\n\tbuffer manager","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 Mon, Mar 01, 2021 at 04:01:11PM +0100, Jacopo Mondi wrote:\n> Introduce the CameraBuffer backend for the Chromium OS operating system\n> and the associated meson option.\n> \n> The Chromium OS CameraBuffer implementation uses the\n> cros::CameraBufferManager class to perform mapping of 1 plane and multiplane\n> buffers and to retrieve size information.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> meson_options.txt | 2 +-\n> src/android/mm/cros_camera_buffer.cpp | 141 ++++++++++++++++++++++++++\n> src/android/mm/meson.build | 3 +\n> 3 files changed, 145 insertions(+), 1 deletion(-)\n> create mode 100644 src/android/mm/cros_camera_buffer.cpp\n> \n> diff --git a/meson_options.txt b/meson_options.txt\n> index d840543b01f5..870914662f3e 100644\n> --- a/meson_options.txt\n> +++ b/meson_options.txt\n> @@ -7,7 +7,7 @@ option('android',\n> \n> option('android_platform',\n> type : 'combo',\n> - choices : ['generic'],\n> + choices : ['generic', 'cros'],\n\nAlphabetical order ?\n\n> value : 'generic',\n> description : 'Select the Android platform to compile for')\n> \n> diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp\n> new file mode 100644\n> index 000000000000..1103f4423c2f\n> --- /dev/null\n> +++ b/src/android/mm/cros_camera_buffer.cpp\n> @@ -0,0 +1,141 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021, Google Inc.\n> + *\n> + * cros_camera_buffer.cpp - Chromium OS buffer backend using CameraBufferManager\n> + */\n> +\n> +#include \"../camera_buffer.h\"\n> +\n> +#include \"libcamera/internal/log.h\"\n> +#include \n\nI think you can drop span.h, it's provided by camera_buffer.h as part of\nthe CameraBuffer interface..\n\n> +\n> +#include \"cros-camera/camera_buffer_manager.h\"\n> +\n> +using namespace libcamera;\n> +\n> +LOG_DECLARE_CATEGORY(HAL)\n> +\n> +class CameraBuffer::Private : public Extensible::Private\n> +{\n> +\tLIBCAMERA_DECLARE_PUBLIC(CameraBuffer)\n> +\n> +public:\n> +\tPrivate(CameraBuffer *cameraBuffer,\n> +\t\tbuffer_handle_t camera3Buffer, int flags);\n> +\t~Private();\n> +\n> +\tbool isValid() const { return valid_; }\n> +\n> +\tunsigned int numPlanes() const;\n> +\n> +\tSpan plane(unsigned int plane) const;\n> +\tSpan 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> +\tconst uint8_t *planeAddr(unsigned int plane) const;\n> +\tuint8_t *planeAddr(unsigned int plane);\n> +};\n> +\n> +CameraBuffer::Private::Private(CameraBuffer *cameraBuffer,\n> +\t\t\t buffer_handle_t camera3Buffer, int flags)\n> +\t: Extensible::Private(cameraBuffer), handle_(camera3Buffer),\n> +\t 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> +\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> +\n> +\tvalid_ = true;\n> +}\n> +\n> +CameraBuffer::Private::~Private()\n> +{\n> +\tbufferManager_->Unlock(handle_);\n> +\tbufferManager_->Deregister(handle_);\n> +}\n> +\n> +unsigned int CameraBuffer::Private::numPlanes() const\n> +{\n> +\treturn bufferManager_->GetNumPlanes(handle_);\n> +}\n> +\n> +Span CameraBuffer::Private::plane(unsigned int plane) const\n> +{\n> +\tconst uint8_t *addr = planeAddr(plane);\n> +\treturn { addr, bufferManager_->GetPlaneSize(handle_, plane) };\n> +}\n> +\n> +Span CameraBuffer::Private::plane(unsigned int plane)\n> +{\n> +\tuint8_t *addr = planeAddr(plane);\n> +\treturn { addr, bufferManager_->GetPlaneSize(handle_, plane) };\n> +}\n> +\n> +const uint8_t *CameraBuffer::Private::planeAddr(unsigned int plane) const\n> +{\n> +\tconst void *addr;\n> +\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> +uint8_t *CameraBuffer::Private::planeAddr(unsigned int plane)\n> +{\n> +\tconst CameraBuffer::Private *self =\n> +\t\tconst_cast(this);\n> +\treturn const_cast(self->planeAddr(plane));\n> +}\n\nThese functions are only called in plane(), I'd inline them there (and\nfollowing the comments on previous patches, you can get rid of the\nnon-const version).\n\nReviewed-by: Laurent Pinchart \n\n> +\n> +PUBLIC_CAMERA_BUFFER\n> diff --git a/src/android/mm/meson.build b/src/android/mm/meson.build\n> index 97f83f2a7380..eeb5cc2e6a31 100644\n> --- a/src/android/mm/meson.build\n> +++ b/src/android/mm/meson.build\n> @@ -3,4 +3,7 @@\n> platform = get_option('android_platform')\n> if platform == 'generic'\n> android_hal_sources += files(['generic_camera_buffer.cpp'])\n> +elif platform == 'cros'\n> + android_hal_sources += files(['cros_camera_buffer.cpp'])\n> + android_deps += [dependency('libcros_camera')]\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 07E5CBD1F1\n\tfor ;\n\tTue, 2 Mar 2021 00:15:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 834EF68A93;\n\tTue, 2 Mar 2021 01:15:40 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DED97602E9\n\tfor ;\n\tTue, 2 Mar 2021 01:15:39 +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 4C9DB45D;\n\tTue, 2 Mar 2021 01:15:39 +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=\"AziF87Gu\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1614644139;\n\tbh=L3B4v731kC0UILdjoNHuLHuoK0pJw5njSFdBy7VCRnQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=AziF87GuVATdjal3wFUNrLOwzHYJlBWq2izdwpWydp03QsDstuldYHYAcJeLLLfda\n\tOCIWPShYTfU0XYDMIdwkaw3av1SLqZxozuPfyfOzn9HryXm/uoJhaSn3PER2qnK027\n\tL1rKV7kE4B8mk10vikly7u/FaEi6E7dZkcdKbRMA=","Date":"Tue, 2 Mar 2021 02:15:11 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210301150111.61791-1-jacopo@jmondi.org>\n\t<20210301150111.61791-11-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210301150111.61791-11-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH 10/10] android: Introduce Chromium OS\n\tbuffer manager","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":"Han-lin Chen ,\n\tlibcamera-devel@lists.libcamera.org, Daniel Hung-yu Wu ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]