[{"id":23639,"web_url":"https://patchwork.libcamera.org/comment/23639/","msgid":"","date":"2022-06-28T08:39:22","subject":"Re: [libcamera-devel] [PATCH v3 1/3] pipeline: ipa: raspberrypi:\n\tMove ControlInfoMap to the IPA","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Naush\n\nThanks for this patch! Very pleased that we'll be able to get accurate\nframerate limits and so on for each mode.\n\nOn Wed, 22 Jun 2022 at 11:21, Naushir Patuck via libcamera-devel\n wrote:\n>\n> Currently the pipeline handler advertises controls handled by the IPA from a\n> static ControlInfoMap defined in the raspberrypi.h header. This change removes\n> this header file, and instead the IPA returns the ControlInfoMap to the pipeline\n> handler from the ipa::init() function. This is done to allow the IPA to adjust\n> the limits of the controls based on the sensor mode in a subsequent change.\n>\n> Bug: https://bugs.libcamera.org/show_bug.cgi?id=83\n> Signed-off-by: Naushir Patuck \n> Reviewed-by: Kieran Bingham \n\nReviewed-by: David Plowman \n\nThanks\nDavid\n\n> ---\n> include/libcamera/ipa/raspberrypi.h | 55 -------------------\n> include/libcamera/ipa/raspberrypi.mojom | 7 ++-\n> src/ipa/raspberrypi/raspberrypi.cpp | 39 ++++++++++---\n> .../pipeline/raspberrypi/raspberrypi.cpp | 30 +++++-----\n> .../pipeline/raspberrypi/rpi_stream.h | 1 -\n> 5 files changed, 53 insertions(+), 79 deletions(-)\n> delete mode 100644 include/libcamera/ipa/raspberrypi.h\n>\n> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> deleted file mode 100644\n> index 6a56b0083b85..000000000000\n> --- a/include/libcamera/ipa/raspberrypi.h\n> +++ /dev/null\n> @@ -1,55 +0,0 @@\n> -/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> -/*\n> - * Copyright (C) 2019-2020, Raspberry Pi Ltd.\n> - *\n> - * raspberrypi.h - Image Processing Algorithm interface for Raspberry Pi\n> - */\n> -\n> -#pragma once\n> -\n> -#include \n> -\n> -#include \n> -#include \n> -\n> -#ifndef __DOXYGEN__\n> -\n> -namespace libcamera {\n> -\n> -namespace RPi {\n> -\n> -/*\n> - * List of controls handled by the Raspberry Pi IPA\n> - *\n> - * \\todo This list will need to be built dynamically from the control\n> - * algorithms loaded by the json file, once this is supported. At that\n> - * point applications should check first whether a control is supported,\n> - * and the pipeline handler may be reverted so that it aborts when an\n> - * unsupported control is encountered.\n> - */\n> -static const ControlInfoMap Controls({\n> - { &controls::AeEnable, ControlInfo(false, true) },\n> - { &controls::ExposureTime, ControlInfo(0, 999999) },\n> - { &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },\n> - { &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },\n> - { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n> - { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },\n> - { &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) },\n> - { &controls::AwbEnable, ControlInfo(false, true) },\n> - { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n> - { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },\n> - { &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },\n> - { &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },\n> - { &controls::Saturation, ControlInfo(0.0f, 32.0f, 1.0f) },\n> - { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n> - { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },\n> - { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },\n> - { &controls::FrameDurationLimits, ControlInfo(INT64_C(1000), INT64_C(1000000000)) },\n> - { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }\n> - }, controls::controls);\n> -\n> -} /* namespace RPi */\n> -\n> -} /* namespace libcamera */\n> -\n> -#endif /* __DOXYGEN__ */\n> diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom\n> index a60c3bb43d3c..77f52c282b0f 100644\n> --- a/include/libcamera/ipa/raspberrypi.mojom\n> +++ b/include/libcamera/ipa/raspberrypi.mojom\n> @@ -26,6 +26,11 @@ struct SensorConfig {\n> uint32 sensorMetadata;\n> };\n>\n> +struct IPAInitResult {\n> + SensorConfig sensorConfig;\n> + libcamera.ControlInfoMap controlInfo;\n> +};\n> +\n> struct ISPConfig {\n> uint32 embeddedBufferId;\n> uint32 bayerBufferId;\n> @@ -50,7 +55,7 @@ struct StartConfig {\n>\n> interface IPARPiInterface {\n> init(libcamera.IPASettings settings)\n> - => (int32 ret, SensorConfig sensorConfig);\n> + => (int32 ret, IPAInitResult result);\n> start(libcamera.ControlList controls) => (StartConfig startConfig);\n> stop();\n>\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 3b126bb5175e..089528f5e126 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -24,7 +24,6 @@\n> #include \n> #include \n> #include \n> -#include \n> #include \n> #include \n>\n> @@ -72,6 +71,28 @@ constexpr Duration defaultMaxFrameDuration = 250.0s;\n> */\n> constexpr Duration controllerMinFrameDuration = 1.0s / 30.0;\n>\n> +/* List of controls handled by the Raspberry Pi IPA */\n> +static const ControlInfoMap::Map ipaControls{\n> + { &controls::AeEnable, ControlInfo(false, true) },\n> + { &controls::ExposureTime, ControlInfo(0, 999999) },\n> + { &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },\n> + { &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },\n> + { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n> + { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },\n> + { &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) },\n> + { &controls::AwbEnable, ControlInfo(false, true) },\n> + { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n> + { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },\n> + { &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },\n> + { &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },\n> + { &controls::Saturation, ControlInfo(0.0f, 32.0f, 1.0f) },\n> + { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n> + { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },\n> + { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },\n> + { &controls::FrameDurationLimits, ControlInfo(INT64_C(1000), INT64_C(1000000000)) },\n> + { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }\n> +};\n> +\n> LOG_DEFINE_CATEGORY(IPARPI)\n>\n> namespace ipa::RPi {\n> @@ -91,7 +112,7 @@ public:\n> munmap(lsTable_, MaxLsGridSize);\n> }\n>\n> - int init(const IPASettings &settings, SensorConfig *sensorConfig) override;\n> + int init(const IPASettings &settings, IPAInitResult *result) override;\n> void start(const ControlList &controls, StartConfig *startConfig) override;\n> void stop() override {}\n>\n> @@ -180,7 +201,7 @@ private:\n> uint32_t maxSensorGainCode_;\n> };\n>\n> -int IPARPi::init(const IPASettings &settings, SensorConfig *sensorConfig)\n> +int IPARPi::init(const IPASettings &settings, IPAInitResult *result)\n> {\n> /*\n> * Load the \"helper\" for this sensor. This tells us all the device specific stuff\n> @@ -202,15 +223,19 @@ int IPARPi::init(const IPASettings &settings, SensorConfig *sensorConfig)\n> helper_->GetDelays(exposureDelay, gainDelay, vblankDelay);\n> sensorMetadata = helper_->SensorEmbeddedDataPresent();\n>\n> - sensorConfig->gainDelay = gainDelay;\n> - sensorConfig->exposureDelay = exposureDelay;\n> - sensorConfig->vblankDelay = vblankDelay;\n> - sensorConfig->sensorMetadata = sensorMetadata;\n> + result->sensorConfig.gainDelay = gainDelay;\n> + result->sensorConfig.exposureDelay = exposureDelay;\n> + result->sensorConfig.vblankDelay = vblankDelay;\n> + result->sensorConfig.sensorMetadata = sensorMetadata;\n>\n> /* Load the tuning file for this sensor. */\n> controller_.Read(settings.configurationFile.c_str());\n> controller_.Initialise();\n>\n> + /* Return the controls handled by the IPA */\n> + ControlInfoMap::Map ctrlMap = ipaControls;\n> + result->controlInfo = ControlInfoMap(std::move(ctrlMap), controls::controls);\n> +\n> return 0;\n> }\n>\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index adc397e8aabd..d980c1a71dd8 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -20,7 +20,6 @@\n> #include \n> #include \n> #include \n> -#include \n> #include \n> #include \n> #include \n> @@ -199,7 +198,7 @@ public:\n> void freeBuffers();\n> void frameStarted(uint32_t sequence);\n>\n> - int loadIPA(ipa::RPi::SensorConfig *sensorConfig);\n> + int loadIPA(ipa::RPi::IPAInitResult *result);\n> int configureIPA(const CameraConfiguration *config, ipa::RPi::IPAConfigResult *result);\n>\n> void enumerateVideoDevices(MediaLink *link);\n> @@ -1231,15 +1230,15 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n>\n> data->sensorFormats_ = populateSensorFormats(data->sensor_);\n>\n> - ipa::RPi::SensorConfig sensorConfig;\n> - if (data->loadIPA(&sensorConfig)) {\n> + ipa::RPi::IPAInitResult result;\n> + if (data->loadIPA(&result)) {\n> LOG(RPI, Error) << \"Failed to load a suitable IPA library\";\n> return -EINVAL;\n> }\n>\n> - if (sensorConfig.sensorMetadata ^ !!unicamEmbedded) {\n> + if (result.sensorConfig.sensorMetadata ^ !!unicamEmbedded) {\n> LOG(RPI, Warning) << \"Mismatch between Unicam and CamHelper for embedded data usage!\";\n> - sensorConfig.sensorMetadata = false;\n> + result.sensorConfig.sensorMetadata = false;\n> if (unicamEmbedded)\n> data->unicam_[Unicam::Embedded].dev()->bufferReady.disconnect();\n> }\n> @@ -1253,7 +1252,7 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> * iterate over all streams in one go.\n> */\n> data->streams_.push_back(&data->unicam_[Unicam::Image]);\n> - if (sensorConfig.sensorMetadata)\n> + if (result.sensorConfig.sensorMetadata)\n> data->streams_.push_back(&data->unicam_[Unicam::Embedded]);\n>\n> for (auto &stream : data->isp_)\n> @@ -1275,15 +1274,16 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> * gain and exposure delays. Mark VBLANK for priority write.\n> */\n> std::unordered_map params = {\n> - { V4L2_CID_ANALOGUE_GAIN, { sensorConfig.gainDelay, false } },\n> - { V4L2_CID_EXPOSURE, { sensorConfig.exposureDelay, false } },\n> - { V4L2_CID_VBLANK, { sensorConfig.vblankDelay, true } }\n> + { V4L2_CID_ANALOGUE_GAIN, { result.sensorConfig.gainDelay, false } },\n> + { V4L2_CID_EXPOSURE, { result.sensorConfig.exposureDelay, false } },\n> + { V4L2_CID_VBLANK, { result.sensorConfig.vblankDelay, true } }\n> };\n> data->delayedCtrls_ = std::make_unique(data->sensor_->device(), params);\n> - data->sensorMetadata_ = sensorConfig.sensorMetadata;\n> + data->sensorMetadata_ = result.sensorConfig.sensorMetadata;\n> +\n> + /* Register initial controls that the Raspberry Pi IPA can handle. */\n> + data->controlInfo_ = std::move(result.controlInfo);\n>\n> - /* Register the controls that the Raspberry Pi IPA can handle. */\n> - data->controlInfo_ = RPi::Controls;\n> /* Initialize the camera properties. */\n> data->properties_ = data->sensor_->properties();\n>\n> @@ -1509,7 +1509,7 @@ void RPiCameraData::frameStarted(uint32_t sequence)\n> delayedCtrls_->applyControls(sequence);\n> }\n>\n> -int RPiCameraData::loadIPA(ipa::RPi::SensorConfig *sensorConfig)\n> +int RPiCameraData::loadIPA(ipa::RPi::IPAInitResult *result)\n> {\n> ipa_ = IPAManager::createIPA(pipe(), 1, 1);\n>\n> @@ -1535,7 +1535,7 @@ int RPiCameraData::loadIPA(ipa::RPi::SensorConfig *sensorConfig)\n>\n> IPASettings settings(configurationFile, sensor_->model());\n>\n> - return ipa_->init(settings, sensorConfig);\n> + return ipa_->init(settings, result);\n> }\n>\n> int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::IPAConfigResult *result)\n> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> index c37f7e82eef6..fe01110019b7 100644\n> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> @@ -12,7 +12,6 @@\n> #include \n> #include \n>\n> -#include \n> #include \n> #include \n>\n> --\n> 2.25.1\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 8E25ABE173\n\tfor ;\n\tTue, 28 Jun 2022 08:39:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3976465637;\n\tTue, 28 Jun 2022 10:39:35 +0200 (CEST)","from mail-ej1-x631.google.com (mail-ej1-x631.google.com\n\t[IPv6:2a00:1450:4864:20::631])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1D3A96559A\n\tfor ;\n\tTue, 28 Jun 2022 10:39:34 +0200 (CEST)","by mail-ej1-x631.google.com with SMTP id u12so24185438eja.8\n\tfor ;\n\tTue, 28 Jun 2022 01:39:34 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1656405575;\n\tbh=T02sHQL1ro/rmovPsdHwW1rCo/Gcif02sVDBLE1IOl0=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=bT8WVECftQKivZf1BT1znJwcAq2juhYCvR70aXCfTHOhqD8h5pjrb97XN83YgKSIW\n\tcKpxFK8MfRZ4Bnec9ZIW0UyV20vi3Ab99mPJHwhUmLi56sSWXMVqsYBM5W8kYL39Dl\n\tRIIAMRVlA7HNIqS7hM6RUy7yem6e/Rl3lU4ZtOTdQr5VXUInGe+r/Cjw3nWcxjjma7\n\t7scMocyPXDrbpzJ0Zci0Q6ve36s1JICXyqSESG0fk9Z6G5Yajuqanqy18SYhzQ5zs3\n\txH6LQBLk2fBeBISt39YVkcGhVnhFUDbQKg4nUQgjl1lq+uaj2ja5U6BPDNcNWhVEGV\n\tsgkVQ1ueyji1g==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=yjRZcXV78GuTI5ok0LtFDwlnKOHmPSU5x7c1Vanrr1Y=;\n\tb=VDR+J9i/NFxpsvgXcPPhGA55u2vV0UzqnqE1Lba2wC578hZztOWbYMKPJYVH/abV0/\n\tNdUJPztaCHDFDIddPGMvn6gVXZ7xhI8EOOsQ0ug4g8CbzQ2c30Sb3ds/wgUKe8UdTyQb\n\twjNie9U3qVTjnSvY9FvlUgDAPTtDsSwmj9IAVjZTu4BFGnVWHs6cgw7qsDE+UWhLYyny\n\tXoEcmLVx96wTMDtlDzGJgJZ5QfEHQ6jv4ng3rS/5p4yb8FYy2uAgyXVgQvOje/kTqPzM\n\tbZJi/q6bOKFiTM3cacba1Fcq3bHDcLk3yEjtrFx2Sg8eQkqmSM8guzPpNCWYuIDWuuB5\n\tsSRw=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"VDR+J9i/\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=yjRZcXV78GuTI5ok0LtFDwlnKOHmPSU5x7c1Vanrr1Y=;\n\tb=SR9CBdNeDO8Enve+KqUOxPHpImu8RYDZotNXs180V0FRtXEEHjdqMQ/POVMmejwilh\n\tj/FH85mpSWODhKIW9COg9DueWdVJ3Zhx2idCtiIiurCGiolZRORefEOf29kpy7SlGAfS\n\t1UCaMLy4Hq7EN9tApTyBVe4+VIk1cRKR+5I8xwsdPI9vl1UMlwMX+SzD+JS1i35kVi0L\n\tlGSyw2Ksxseh6I5zfZ5FXCVycDttQC5SqUe0HIt/luEd6fEyF941KBp4havlDlhxu24A\n\tcDI0gZJRyO/4MMmpDUHIR7wsUKQ8OfWPIVF0rP8IrBjpvBYwXX0N9cAOiChuXsClfdsN\n\tYMvg==","X-Gm-Message-State":"AJIora9r2cKGVNZDNbwzty0krUyi42dvsccirdKDjnG9pQuypgNrz2qj\n\tjycvwQFL9UDQUsghGNv+boIgLriGCeXWIR2t1V/t1MJwAmdkEQ==","X-Google-Smtp-Source":"AGRyM1t5E8y3/D9XTT/z64GwgSXAwYxohy6CwUb0NfR9Ntj/ZsA5QR+Z7zTaLqK9Prjb2hdq91KFJqRkOUGFEtbj6ZI=","X-Received":"by 2002:a17:906:2086:b0:712:1257:77bf with SMTP id\n\t6-20020a170906208600b00712125777bfmr17574448ejq.655.1656405573654;\n\tTue, 28 Jun 2022 01:39:33 -0700 (PDT)","MIME-Version":"1.0","References":"<20220622102047.22492-1-naush@raspberrypi.com>\n\t<20220622102047.22492-2-naush@raspberrypi.com>","In-Reply-To":"<20220622102047.22492-2-naush@raspberrypi.com>","Date":"Tue, 28 Jun 2022 09:39:22 +0100","Message-ID":"","To":"Naushir Patuck ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v3 1/3] pipeline: ipa: raspberrypi:\n\tMove ControlInfoMap to the IPA","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","From":"David Plowman via libcamera-devel ","Reply-To":"David Plowman ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]