[{"id":23499,"web_url":"https://patchwork.libcamera.org/comment/23499/","msgid":"","date":"2022-06-21T07:29:43","subject":"Re: [libcamera-devel] [PATCH 1/3] pipeline: ipa: raspberrypi: Move\n\tControlInfoMap to the IPA","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi,\n\nPing for a review on this series please.\n\nThanks,\nNaush\n\nOn Fri, 10 Jun 2022 at 13:25, Naushir Patuck wrote:\n\n> Currently the pipeline handler advertises controls handled by the IPA from\n> a\n> static ControlInfoMap defined in the raspberrypi.h header. This change\n> removes\n> this header file, and instead the IPA returns the ControlInfoMap to the\n> pipeline\n> handler from the ipa::init() function. This is done to allow the IPA to\n> adjust\n> the limits of the controls based on the sensor mode in a subsequent change.\n>\n> Signed-off-by: Naushir Patuck \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\n> 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,\n> ControlInfo(controls::AeMeteringModeValues) },\n> - { &controls::AeConstraintMode,\n> ControlInfo(controls::AeConstraintModeValues) },\n> - { &controls::AeExposureMode,\n> ControlInfo(controls::AeExposureModeValues) },\n> - { &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f)\n> },\n> - { &controls::AwbEnable, ControlInfo(false, true) },\n> - { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n> - { &controls::AwbMode, ControlInfo(controls::AwbModeValues)\n> },\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,\n> 16.0f) },\n> - { &controls::ScalerCrop, ControlInfo(Rectangle{},\n> Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },\n> - { &controls::FrameDurationLimits,\n> ControlInfo(INT64_C(1000), INT64_C(1000000000)) },\n> - { &controls::draft::NoiseReductionMode,\n> 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\n> 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\n> 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,\n> ControlInfo(controls::AeMeteringModeValues) },\n> + { &controls::AeConstraintMode,\n> ControlInfo(controls::AeConstraintModeValues) },\n> + { &controls::AeExposureMode,\n> 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,\n> 65535, 65535, 65535), Rectangle{}) },\n> + { &controls::FrameDurationLimits, ControlInfo(INT64_C(1000),\n> INT64_C(1000000000)) },\n> + { &controls::draft::NoiseReductionMode,\n> 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)\n> override;\n> + int init(const IPASettings &settings, IPAInitResult *result)\n> override;\n> void start(const ControlList &controls, StartConfig *startConfig)\n> 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\n> specific stuff\n> @@ -202,15 +223,19 @@ int IPARPi::init(const IPASettings &settings,\n> 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),\n> controls::controls);\n> +\n> return 0;\n> }\n>\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> 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,\n> ipa::RPi::IPAConfigResult *result);\n>\n> void enumerateVideoDevices(MediaLink *link);\n> @@ -1231,15 +1230,15 @@ int PipelineHandlerRPi::registerCamera(MediaDevice\n> *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\n> CamHelper for embedded data usage!\";\n> - sensorConfig.sensorMetadata = false;\n> + result.sensorConfig.sensorMetadata = false;\n> if (unicamEmbedded)\n>\n> data->unicam_[Unicam::Embedded].dev()->bufferReady.disconnect();\n> }\n> @@ -1253,7 +1252,7 @@ int PipelineHandlerRPi::registerCamera(MediaDevice\n> *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\n> *unicam, MediaDevice *isp, Me\n> * gain and exposure delays. Mark VBLANK for priority write.\n> */\n> std::unordered_map\n> params = {\n> - { V4L2_CID_ANALOGUE_GAIN, { sensorConfig.gainDelay, false\n> } },\n> - { V4L2_CID_EXPOSURE, { sensorConfig.exposureDelay, false }\n> },\n> - { V4L2_CID_VBLANK, { sensorConfig.vblankDelay, true } }\n> + { V4L2_CID_ANALOGUE_GAIN, { result.sensorConfig.gainDelay,\n> false } },\n> + { V4L2_CID_EXPOSURE, { result.sensorConfig.exposureDelay,\n> false } },\n> + { V4L2_CID_VBLANK, { result.sensorConfig.vblankDelay, true\n> } }\n> };\n> data->delayedCtrls_ =\n> 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> */\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\n> *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,\n> ipa::RPi::IPAConfigResult *result)\n> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> 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>\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 BDCE9BD808\n\tfor ;\n\tTue, 21 Jun 2022 07:29:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7DBEE65637;\n\tTue, 21 Jun 2022 09:29:53 +0200 (CEST)","from mail-lf1-x133.google.com (mail-lf1-x133.google.com\n\t[IPv6:2a00:1450:4864:20::133])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 550A165631\n\tfor ;\n\tTue, 21 Jun 2022 09:29:51 +0200 (CEST)","by mail-lf1-x133.google.com with SMTP id y32so20889925lfa.6\n\tfor ;\n\tTue, 21 Jun 2022 00:29:51 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1655796593;\n\tbh=FPTFGmJoEYgDP9xKzbj1+ThBClCNCO26R4hYNjPjsfg=;\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:\n\tFrom;\n\tb=g86m6lAeyXkPlM7eppuhhivbeRjv+gN+VBbBA+8KFrIpJmz88wjgN8E519MZb6QGO\n\tUYNZm238+oYz2yitPhmCAiFrUCeLEaVtb+L6vkc3u3su5BMzvQipp8lAkUXodxR8eQ\n\tduxmtI1QZQm2yo/tp6HLn21YL/kgiNabiTId5AblFH3R1L16xZLnue6T+tUkYV48id\n\tiUddCpCIEy/EouYvCe+DxZoeJTUbjQtfhCKoAxqM/6RQuVloseyU7++N0HUc0Wvu+X\n\tDMa+J5tj3jwds/U/2BsMPEr2nxM7PsQ1DJb5v647FLga2A3adyGtaogyb1XLEg/JjW\n\t7nYA0Cgh+pKQA==","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=O1ye5aFpwKqVZrhrSVigxCxbDk/2JmSbwNZQjGUXDsY=;\n\tb=M9UVGhfwDqVT/C/n3gADNmvdEdVlALh8nVa6GcMl7l+BHoWfkMQjn9J3OQJqqPnwpQ\n\tuijvxLxn5iqXN2+ZDfGMTMNUj/fDNPK0K9EF99LlSolQENtjMDjIdfP84m9U5bUrs+aj\n\tuMvCoe79H8cqV7D4eRCE/6CT0ausECzH00xx4fxfJWFUKHOiTJ6/oIEeV1rJ3tUs7LG0\n\t1/24KF4C3jLdVDebAtDlyzRhQt6xRiehC2jsWfZoL8T7+OUmO+/5CprdMWq5jhhY0AN9\n\tA2OG3gCTIZcDOskT+a61GQ2kuAql1dL0d5vmOjR6oUyTfUtDHiO1mlwimpA8Q8VUQejD\n\tlb5Q=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"M9UVGhfw\"; 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=O1ye5aFpwKqVZrhrSVigxCxbDk/2JmSbwNZQjGUXDsY=;\n\tb=JMz9XTOQHAhRIKZ7Um3uqFSOh0K5We3qKvH3cjn4RouCjg+H9ky+dVquDr/LNPZMve\n\teXH4mCsgWZf+X3xCSeRJ8bFHAw7hTZdwsK+JfQ0+wdrlrrRQtUhgRm0ss3l9IAzLqZ1v\n\tDQDeEHrbwVDGeWfbvt/p9Vbq/1sZ2kcxRDcuMGmr7ZpYhm5G1fRD9BSGpf3v/HeFw6kx\n\tgAg9s328W9FDeoTMgOcjV/CoOSYqH9NDty3LtMlv2B0kzib+f3laumDsAo3yEydC3OEo\n\tuhHHaqMW5ipxaAsY5EBndjLdyQE2jhx7bpbAL80uGYiFZ8KRXRvcGeCLG8HRBe8NvF2G\n\tlHIw==","X-Gm-Message-State":"AJIora9IyaGZQH6vfxugSrO0OVjL4PyPUQ0vr5p2316CFlgsPXdnwDaM\n\t5G5qhQclZFzgh0Lng6xlOL/emLpfz+bmDb0uxtVVq1oOdx5YkQ==","X-Google-Smtp-Source":"AGRyM1tpnIkIS9Ppq49ec0bvb02cQuZg3JWSRwY9+gsDXTum1Ni/aHfjhiZlOxO0Mf2+SJf0RgCLEj1baHZavJdwKeU=","X-Received":"by 2002:a05:6512:3d08:b0:47f:6efa:4965 with SMTP id\n\td8-20020a0565123d0800b0047f6efa4965mr5507291lfv.363.1655796589905;\n\tTue, 21 Jun 2022 00:29:49 -0700 (PDT)","MIME-Version":"1.0","References":"<20220610122533.11888-1-naush@raspberrypi.com>","In-Reply-To":"<20220610122533.11888-1-naush@raspberrypi.com>","Date":"Tue, 21 Jun 2022 08:29:43 +0100","Message-ID":"","To":"libcamera devel ","Content-Type":"multipart/alternative; boundary=\"0000000000008fe2a705e1f02f8a\"","Subject":"Re: [libcamera-devel] [PATCH 1/3] pipeline: ipa: raspberrypi: Move\n\tControlInfoMap 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":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23500,"web_url":"https://patchwork.libcamera.org/comment/23500/","msgid":"<165581315804.1149771.781126249639547161@Monstersaurus>","date":"2022-06-21T12:05:58","subject":"Re: [libcamera-devel] [PATCH 1/3] pipeline: ipa: raspberrypi: Move\n\tControlInfoMap to the IPA","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Naushir Patuck via libcamera-devel (2022-06-10 13:25:31)\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> Signed-off-by: Naushir Patuck \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\nWe didn't have it before, but I wonder if result should have an\nassertion that it is not null. But given how tied this is to the PH\nside, I think we can say we 'know' that the two components will behave\nwith each other, so there's enough of a contract that it will be valid.\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\nresult keeps reading like such an odd variable name through here. But\nI'm not sure I've got better alternatives. ipaData? ipaContext ? ...\ndata and config are too overloaded already...\n\nBut it's trivial so it doesn't worry me too much.\n\n\nReviewed-by: Kieran Bingham \n\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 37AA2BD808\n\tfor ;\n\tTue, 21 Jun 2022 12:06:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 141D465634;\n\tTue, 21 Jun 2022 14:06:03 +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 592CB61FB2\n\tfor ;\n\tTue, 21 Jun 2022 14:06:01 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id ED240104;\n\tTue, 21 Jun 2022 14:06:00 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1655813163;\n\tbh=ouk8T+AZBlcLO2oN2ljGUL6ruNC0P/ce6Hyjgn2jufg=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=xS1fsS2uivORQFbeoQrUdgBNOCoVJ1KMbOw7bC6ENjXwxpZMgIqLAsHJq+N/vDKT4\n\tYWTPNYH1iaeS+iyw2fxKx9+nd8FTrW8voStLS55fiGblfUCN7iImgZW0pBGBX/znjZ\n\tVj6fwcb+ouzXB2oBb/rZ/8swKlNHSQNsk7vV+Evx7poDocg/MXzOtWh2yBZJeKH3T5\n\t/SiZGoGmi4qq8CwgWwMV6a05Iy/p+lCgdYhXesvOoUEPkmKSG1++BkbYlm1t5sBqf7\n\t1Bx7dE+HrvfGxIwRD0AQoAJeB4+sAojwIMdlXMln0OcK15tTqIMgntYCaelujRDwRu\n\tnUB98lBrHrD/g==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1655813161;\n\tbh=ouk8T+AZBlcLO2oN2ljGUL6ruNC0P/ce6Hyjgn2jufg=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=WthDf+x4e8ImhZ+2H0lPOAZQXZRoUFbSSPpAopnmXdIWv9MHzBxHxz8DA7djkM2H3\n\tj+kIoW5jPx3k4r8l3dzniLCfGEBdPIXnoCCY1lcqbHLmnYn8Tb8MDQ6uwytBgA06K/\n\tS+BpSLiDyH7OFiPSa8DCFBdE6r1g9W5HaVY0+X0k="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"WthDf+x4\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220610122533.11888-1-naush@raspberrypi.com>","References":"<20220610122533.11888-1-naush@raspberrypi.com>","To":"Naushir Patuck ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 21 Jun 2022 13:05:58 +0100","Message-ID":"<165581315804.1149771.781126249639547161@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH 1/3] pipeline: ipa: raspberrypi: Move\n\tControlInfoMap 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":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23504,"web_url":"https://patchwork.libcamera.org/comment/23504/","msgid":"","date":"2022-06-21T12:35:59","subject":"Re: [libcamera-devel] [PATCH 1/3] pipeline: ipa: raspberrypi: Move\n\tControlInfoMap to the IPA","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nThank you for the patch.\n\nOn Fri, Jun 10, 2022 at 01:25:31PM +0100, Naushir Patuck via libcamera-devel wrote:\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\nThis also fixes a bug :-) You can add the following tag.\n\nBug: https://bugs.libcamera.org/show_bug.cgi?id=81\n\n> Signed-off-by: Naushir Patuck \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> -\t\t{ &controls::AeEnable, ControlInfo(false, true) },\n> -\t\t{ &controls::ExposureTime, ControlInfo(0, 999999) },\n> -\t\t{ &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },\n> -\t\t{ &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },\n> -\t\t{ &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n> -\t\t{ &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },\n> -\t\t{ &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) },\n> -\t\t{ &controls::AwbEnable, ControlInfo(false, true) },\n> -\t\t{ &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n> -\t\t{ &controls::AwbMode, ControlInfo(controls::AwbModeValues) },\n> -\t\t{ &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },\n> -\t\t{ &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },\n> -\t\t{ &controls::Saturation, ControlInfo(0.0f, 32.0f, 1.0f) },\n> -\t\t{ &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n> -\t\t{ &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },\n> -\t\t{ &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },\n> -\t\t{ &controls::FrameDurationLimits, ControlInfo(INT64_C(1000), INT64_C(1000000000)) },\n> -\t\t{ &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }\n> -\t}, 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> \tuint32 sensorMetadata;\n> };\n> \n> +struct IPAInitResult {\n> +\tSensorConfig sensorConfig;\n> +\tlibcamera.ControlInfoMap controlInfo;\n> +};\n> +\n> struct ISPConfig {\n> \tuint32 embeddedBufferId;\n> \tuint32 bayerBufferId;\n> @@ -50,7 +55,7 @@ struct StartConfig {\n> \n> interface IPARPiInterface {\n> \tinit(libcamera.IPASettings settings)\n> -\t\t=> (int32 ret, SensorConfig sensorConfig);\n> +\t\t=> (int32 ret, IPAInitResult result);\n> \tstart(libcamera.ControlList controls) => (StartConfig startConfig);\n> \tstop();\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> +\t{ &controls::AeEnable, ControlInfo(false, true) },\n> +\t{ &controls::ExposureTime, ControlInfo(0, 999999) },\n> +\t{ &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },\n> +\t{ &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },\n> +\t{ &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n> +\t{ &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },\n> +\t{ &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) },\n> +\t{ &controls::AwbEnable, ControlInfo(false, true) },\n> +\t{ &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n> +\t{ &controls::AwbMode, ControlInfo(controls::AwbModeValues) },\n> +\t{ &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },\n> +\t{ &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },\n> +\t{ &controls::Saturation, ControlInfo(0.0f, 32.0f, 1.0f) },\n> +\t{ &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n> +\t{ &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },\n> +\t{ &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },\n> +\t{ &controls::FrameDurationLimits, ControlInfo(INT64_C(1000), INT64_C(1000000000)) },\n> +\t{ &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> \t\t\tmunmap(lsTable_, MaxLsGridSize);\n> \t}\n> \n> -\tint init(const IPASettings &settings, SensorConfig *sensorConfig) override;\n> +\tint init(const IPASettings &settings, IPAInitResult *result) override;\n> \tvoid start(const ControlList &controls, StartConfig *startConfig) override;\n> \tvoid stop() override {}\n> \n> @@ -180,7 +201,7 @@ private:\n> \tuint32_t maxSensorGainCode_;\n> };\n> \n> -int IPARPi::init(const IPASettings &settings, SensorConfig *sensorConfig)\n> +int IPARPi::init(const IPASettings &settings, IPAInitResult *result)\n> {\n> \t/*\n> \t * 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> \thelper_->GetDelays(exposureDelay, gainDelay, vblankDelay);\n> \tsensorMetadata = helper_->SensorEmbeddedDataPresent();\n> \n> -\tsensorConfig->gainDelay = gainDelay;\n> -\tsensorConfig->exposureDelay = exposureDelay;\n> -\tsensorConfig->vblankDelay = vblankDelay;\n> -\tsensorConfig->sensorMetadata = sensorMetadata;\n> +\tresult->sensorConfig.gainDelay = gainDelay;\n> +\tresult->sensorConfig.exposureDelay = exposureDelay;\n> +\tresult->sensorConfig.vblankDelay = vblankDelay;\n> +\tresult->sensorConfig.sensorMetadata = sensorMetadata;\n> \n> \t/* Load the tuning file for this sensor. */\n> \tcontroller_.Read(settings.configurationFile.c_str());\n> \tcontroller_.Initialise();\n> \n> +\t/* Return the controls handled by the IPA */\n> +\tControlInfoMap::Map ctrlMap = ipaControls;\n> +\tresult->controlInfo = ControlInfoMap(std::move(ctrlMap), controls::controls);\n\nThis is a bit weird. I think it could be simplified by keeping\nipaControls a ControlInfoMap. To do so safely, and not depend on link\norder, you can declare it as a local static const variable in this\nfunction instead of a global variable.\n\nReviewed-by: Laurent Pinchart \n\n> +\n> \treturn 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> \tvoid freeBuffers();\n> \tvoid frameStarted(uint32_t sequence);\n> \n> -\tint loadIPA(ipa::RPi::SensorConfig *sensorConfig);\n> +\tint loadIPA(ipa::RPi::IPAInitResult *result);\n> \tint configureIPA(const CameraConfiguration *config, ipa::RPi::IPAConfigResult *result);\n> \n> \tvoid enumerateVideoDevices(MediaLink *link);\n> @@ -1231,15 +1230,15 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> \n> \tdata->sensorFormats_ = populateSensorFormats(data->sensor_);\n> \n> -\tipa::RPi::SensorConfig sensorConfig;\n> -\tif (data->loadIPA(&sensorConfig)) {\n> +\tipa::RPi::IPAInitResult result;\n> +\tif (data->loadIPA(&result)) {\n> \t\tLOG(RPI, Error) << \"Failed to load a suitable IPA library\";\n> \t\treturn -EINVAL;\n> \t}\n> \n> -\tif (sensorConfig.sensorMetadata ^ !!unicamEmbedded) {\n> +\tif (result.sensorConfig.sensorMetadata ^ !!unicamEmbedded) {\n> \t\tLOG(RPI, Warning) << \"Mismatch between Unicam and CamHelper for embedded data usage!\";\n> -\t\tsensorConfig.sensorMetadata = false;\n> +\t\tresult.sensorConfig.sensorMetadata = false;\n> \t\tif (unicamEmbedded)\n> \t\t\tdata->unicam_[Unicam::Embedded].dev()->bufferReady.disconnect();\n> \t}\n> @@ -1253,7 +1252,7 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> \t * iterate over all streams in one go.\n> \t */\n> \tdata->streams_.push_back(&data->unicam_[Unicam::Image]);\n> -\tif (sensorConfig.sensorMetadata)\n> +\tif (result.sensorConfig.sensorMetadata)\n> \t\tdata->streams_.push_back(&data->unicam_[Unicam::Embedded]);\n> \n> \tfor (auto &stream : data->isp_)\n> @@ -1275,15 +1274,16 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> \t * gain and exposure delays. Mark VBLANK for priority write.\n> \t */\n> \tstd::unordered_map params = {\n> -\t\t{ V4L2_CID_ANALOGUE_GAIN, { sensorConfig.gainDelay, false } },\n> -\t\t{ V4L2_CID_EXPOSURE, { sensorConfig.exposureDelay, false } },\n> -\t\t{ V4L2_CID_VBLANK, { sensorConfig.vblankDelay, true } }\n> +\t\t{ V4L2_CID_ANALOGUE_GAIN, { result.sensorConfig.gainDelay, false } },\n> +\t\t{ V4L2_CID_EXPOSURE, { result.sensorConfig.exposureDelay, false } },\n> +\t\t{ V4L2_CID_VBLANK, { result.sensorConfig.vblankDelay, true } }\n> \t};\n> \tdata->delayedCtrls_ = std::make_unique(data->sensor_->device(), params);\n> -\tdata->sensorMetadata_ = sensorConfig.sensorMetadata;\n> +\tdata->sensorMetadata_ = result.sensorConfig.sensorMetadata;\n> +\n> +\t/* Register initial controls that the Raspberry Pi IPA can handle. */\n> +\tdata->controlInfo_ = std::move(result.controlInfo);\n> \n> -\t/* Register the controls that the Raspberry Pi IPA can handle. */\n> -\tdata->controlInfo_ = RPi::Controls;\n> \t/* Initialize the camera properties. */\n> \tdata->properties_ = data->sensor_->properties();\n> \n> @@ -1509,7 +1509,7 @@ void RPiCameraData::frameStarted(uint32_t sequence)\n> \tdelayedCtrls_->applyControls(sequence);\n> }\n> \n> -int RPiCameraData::loadIPA(ipa::RPi::SensorConfig *sensorConfig)\n> +int RPiCameraData::loadIPA(ipa::RPi::IPAInitResult *result)\n> {\n> \tipa_ = IPAManager::createIPA(pipe(), 1, 1);\n> \n> @@ -1535,7 +1535,7 @@ int RPiCameraData::loadIPA(ipa::RPi::SensorConfig *sensorConfig)\n> \n> \tIPASettings settings(configurationFile, sensor_->model());\n> \n> -\treturn ipa_->init(settings, sensorConfig);\n> +\treturn 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>","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 C154DBE173\n\tfor ;\n\tTue, 21 Jun 2022 12:36:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 864536563B;\n\tTue, 21 Jun 2022 14:36:20 +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 6847A65634\n\tfor ;\n\tTue, 21 Jun 2022 14:36:16 +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 E3787104;\n\tTue, 21 Jun 2022 14:36:15 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1655814980;\n\tbh=4mSmIWIhJM1VnOlyyl2pf7aNm9bSC2EkCxNm5aE5Nz8=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=OmJ2phPA6ZTIgGbjdce4u2J0WnQoy6CsUYVrExLMXy/hIzGasrhyfLCTG7aJt60US\n\tfMwJJ4/STCROrWsbBb3nq4yS6lnpkpicL060ZLPcbXLCQy7M1yXx9h1cxkQqtVEXcq\n\tJiXScBGUFotzvqq4gBN5ykRWQQmchqPTteMZsNjcSTPBGmghnr6RAZ/2cjjXnjIrL9\n\tgr7uNv5DKaECx+edfFMDjig9aIkKxpB6nFgFDciklKoKOX5vcFpnDU5EnHQS/lcSz2\n\tcdJ6NitTAi2wYWUYS+bhLmvbZk0IKBMpEvUXJPrjgJ2thx6bMzXijfhULh73DQYJwu\n\tO147p/GQ5WOpw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1655814976;\n\tbh=4mSmIWIhJM1VnOlyyl2pf7aNm9bSC2EkCxNm5aE5Nz8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=cmIsaAH5kjtSApTtMT59Ib5RRnzChoo+S1hj3zkkum2e9XAZeG2l8Z+rSf7oStswb\n\tEKf9YQn/UOAREn4PvkW/vhR591DHKwyCivMTNlln4ifzX8e9t7/j8bya1xIO/QJorv\n\tv/EU8IsKuATsX0tUhTP3Fla4MgY1t9VhKm3ysu90="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"cmIsaAH5\"; dkim-atps=neutral","Date":"Tue, 21 Jun 2022 15:35:59 +0300","To":"Naushir Patuck ","Message-ID":"","References":"<20220610122533.11888-1-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220610122533.11888-1-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH 1/3] pipeline: ipa: raspberrypi: Move\n\tControlInfoMap 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":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23508,"web_url":"https://patchwork.libcamera.org/comment/23508/","msgid":"","date":"2022-06-21T13:07:37","subject":"Re: [libcamera-devel] [PATCH 1/3] pipeline: ipa: raspberrypi: Move\n\tControlInfoMap to the IPA","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Tue, Jun 21, 2022 at 03:35:59PM +0300, Laurent Pinchart via libcamera-devel wrote:\n> Hi Naush,\n> \n> Thank you for the patch.\n> \n> On Fri, Jun 10, 2022 at 01:25:31PM +0100, Naushir Patuck via libcamera-devel wrote:\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> This also fixes a bug :-) You can add the following tag.\n> \n> Bug: https://bugs.libcamera.org/show_bug.cgi?id=81\n\nThe careful reader will have noticed I meant\n\nBug: https://bugs.libcamera.org/show_bug.cgi?id=83\n\n(thanks Kieran for spotting this)\n\n> > Signed-off-by: Naushir Patuck \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> > -\t\t{ &controls::AeEnable, ControlInfo(false, true) },\n> > -\t\t{ &controls::ExposureTime, ControlInfo(0, 999999) },\n> > -\t\t{ &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },\n> > -\t\t{ &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },\n> > -\t\t{ &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n> > -\t\t{ &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },\n> > -\t\t{ &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) },\n> > -\t\t{ &controls::AwbEnable, ControlInfo(false, true) },\n> > -\t\t{ &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n> > -\t\t{ &controls::AwbMode, ControlInfo(controls::AwbModeValues) },\n> > -\t\t{ &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },\n> > -\t\t{ &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },\n> > -\t\t{ &controls::Saturation, ControlInfo(0.0f, 32.0f, 1.0f) },\n> > -\t\t{ &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n> > -\t\t{ &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },\n> > -\t\t{ &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },\n> > -\t\t{ &controls::FrameDurationLimits, ControlInfo(INT64_C(1000), INT64_C(1000000000)) },\n> > -\t\t{ &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }\n> > -\t}, 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> > \tuint32 sensorMetadata;\n> > };\n> > \n> > +struct IPAInitResult {\n> > +\tSensorConfig sensorConfig;\n> > +\tlibcamera.ControlInfoMap controlInfo;\n> > +};\n> > +\n> > struct ISPConfig {\n> > \tuint32 embeddedBufferId;\n> > \tuint32 bayerBufferId;\n> > @@ -50,7 +55,7 @@ struct StartConfig {\n> > \n> > interface IPARPiInterface {\n> > \tinit(libcamera.IPASettings settings)\n> > -\t\t=> (int32 ret, SensorConfig sensorConfig);\n> > +\t\t=> (int32 ret, IPAInitResult result);\n> > \tstart(libcamera.ControlList controls) => (StartConfig startConfig);\n> > \tstop();\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> > +\t{ &controls::AeEnable, ControlInfo(false, true) },\n> > +\t{ &controls::ExposureTime, ControlInfo(0, 999999) },\n> > +\t{ &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },\n> > +\t{ &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },\n> > +\t{ &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n> > +\t{ &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },\n> > +\t{ &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) },\n> > +\t{ &controls::AwbEnable, ControlInfo(false, true) },\n> > +\t{ &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n> > +\t{ &controls::AwbMode, ControlInfo(controls::AwbModeValues) },\n> > +\t{ &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },\n> > +\t{ &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },\n> > +\t{ &controls::Saturation, ControlInfo(0.0f, 32.0f, 1.0f) },\n> > +\t{ &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n> > +\t{ &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },\n> > +\t{ &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },\n> > +\t{ &controls::FrameDurationLimits, ControlInfo(INT64_C(1000), INT64_C(1000000000)) },\n> > +\t{ &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> > \t\t\tmunmap(lsTable_, MaxLsGridSize);\n> > \t}\n> > \n> > -\tint init(const IPASettings &settings, SensorConfig *sensorConfig) override;\n> > +\tint init(const IPASettings &settings, IPAInitResult *result) override;\n> > \tvoid start(const ControlList &controls, StartConfig *startConfig) override;\n> > \tvoid stop() override {}\n> > \n> > @@ -180,7 +201,7 @@ private:\n> > \tuint32_t maxSensorGainCode_;\n> > };\n> > \n> > -int IPARPi::init(const IPASettings &settings, SensorConfig *sensorConfig)\n> > +int IPARPi::init(const IPASettings &settings, IPAInitResult *result)\n> > {\n> > \t/*\n> > \t * 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> > \thelper_->GetDelays(exposureDelay, gainDelay, vblankDelay);\n> > \tsensorMetadata = helper_->SensorEmbeddedDataPresent();\n> > \n> > -\tsensorConfig->gainDelay = gainDelay;\n> > -\tsensorConfig->exposureDelay = exposureDelay;\n> > -\tsensorConfig->vblankDelay = vblankDelay;\n> > -\tsensorConfig->sensorMetadata = sensorMetadata;\n> > +\tresult->sensorConfig.gainDelay = gainDelay;\n> > +\tresult->sensorConfig.exposureDelay = exposureDelay;\n> > +\tresult->sensorConfig.vblankDelay = vblankDelay;\n> > +\tresult->sensorConfig.sensorMetadata = sensorMetadata;\n> > \n> > \t/* Load the tuning file for this sensor. */\n> > \tcontroller_.Read(settings.configurationFile.c_str());\n> > \tcontroller_.Initialise();\n> > \n> > +\t/* Return the controls handled by the IPA */\n> > +\tControlInfoMap::Map ctrlMap = ipaControls;\n> > +\tresult->controlInfo = ControlInfoMap(std::move(ctrlMap), controls::controls);\n> \n> This is a bit weird. I think it could be simplified by keeping\n> ipaControls a ControlInfoMap. To do so safely, and not depend on link\n> order, you can declare it as a local static const variable in this\n> function instead of a global variable.\n> \n> Reviewed-by: Laurent Pinchart \n> \n> > +\n> > \treturn 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> > \tvoid freeBuffers();\n> > \tvoid frameStarted(uint32_t sequence);\n> > \n> > -\tint loadIPA(ipa::RPi::SensorConfig *sensorConfig);\n> > +\tint loadIPA(ipa::RPi::IPAInitResult *result);\n> > \tint configureIPA(const CameraConfiguration *config, ipa::RPi::IPAConfigResult *result);\n> > \n> > \tvoid enumerateVideoDevices(MediaLink *link);\n> > @@ -1231,15 +1230,15 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> > \n> > \tdata->sensorFormats_ = populateSensorFormats(data->sensor_);\n> > \n> > -\tipa::RPi::SensorConfig sensorConfig;\n> > -\tif (data->loadIPA(&sensorConfig)) {\n> > +\tipa::RPi::IPAInitResult result;\n> > +\tif (data->loadIPA(&result)) {\n> > \t\tLOG(RPI, Error) << \"Failed to load a suitable IPA library\";\n> > \t\treturn -EINVAL;\n> > \t}\n> > \n> > -\tif (sensorConfig.sensorMetadata ^ !!unicamEmbedded) {\n> > +\tif (result.sensorConfig.sensorMetadata ^ !!unicamEmbedded) {\n> > \t\tLOG(RPI, Warning) << \"Mismatch between Unicam and CamHelper for embedded data usage!\";\n> > -\t\tsensorConfig.sensorMetadata = false;\n> > +\t\tresult.sensorConfig.sensorMetadata = false;\n> > \t\tif (unicamEmbedded)\n> > \t\t\tdata->unicam_[Unicam::Embedded].dev()->bufferReady.disconnect();\n> > \t}\n> > @@ -1253,7 +1252,7 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> > \t * iterate over all streams in one go.\n> > \t */\n> > \tdata->streams_.push_back(&data->unicam_[Unicam::Image]);\n> > -\tif (sensorConfig.sensorMetadata)\n> > +\tif (result.sensorConfig.sensorMetadata)\n> > \t\tdata->streams_.push_back(&data->unicam_[Unicam::Embedded]);\n> > \n> > \tfor (auto &stream : data->isp_)\n> > @@ -1275,15 +1274,16 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> > \t * gain and exposure delays. Mark VBLANK for priority write.\n> > \t */\n> > \tstd::unordered_map params = {\n> > -\t\t{ V4L2_CID_ANALOGUE_GAIN, { sensorConfig.gainDelay, false } },\n> > -\t\t{ V4L2_CID_EXPOSURE, { sensorConfig.exposureDelay, false } },\n> > -\t\t{ V4L2_CID_VBLANK, { sensorConfig.vblankDelay, true } }\n> > +\t\t{ V4L2_CID_ANALOGUE_GAIN, { result.sensorConfig.gainDelay, false } },\n> > +\t\t{ V4L2_CID_EXPOSURE, { result.sensorConfig.exposureDelay, false } },\n> > +\t\t{ V4L2_CID_VBLANK, { result.sensorConfig.vblankDelay, true } }\n> > \t};\n> > \tdata->delayedCtrls_ = std::make_unique(data->sensor_->device(), params);\n> > -\tdata->sensorMetadata_ = sensorConfig.sensorMetadata;\n> > +\tdata->sensorMetadata_ = result.sensorConfig.sensorMetadata;\n> > +\n> > +\t/* Register initial controls that the Raspberry Pi IPA can handle. */\n> > +\tdata->controlInfo_ = std::move(result.controlInfo);\n> > \n> > -\t/* Register the controls that the Raspberry Pi IPA can handle. */\n> > -\tdata->controlInfo_ = RPi::Controls;\n> > \t/* Initialize the camera properties. */\n> > \tdata->properties_ = data->sensor_->properties();\n> > \n> > @@ -1509,7 +1509,7 @@ void RPiCameraData::frameStarted(uint32_t sequence)\n> > \tdelayedCtrls_->applyControls(sequence);\n> > }\n> > \n> > -int RPiCameraData::loadIPA(ipa::RPi::SensorConfig *sensorConfig)\n> > +int RPiCameraData::loadIPA(ipa::RPi::IPAInitResult *result)\n> > {\n> > \tipa_ = IPAManager::createIPA(pipe(), 1, 1);\n> > \n> > @@ -1535,7 +1535,7 @@ int RPiCameraData::loadIPA(ipa::RPi::SensorConfig *sensorConfig)\n> > \n> > \tIPASettings settings(configurationFile, sensor_->model());\n> > \n> > -\treturn ipa_->init(settings, sensorConfig);\n> > +\treturn 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> >","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 13E80BE173\n\tfor ;\n\tTue, 21 Jun 2022 13:07:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 23AF265634;\n\tTue, 21 Jun 2022 15:07:56 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 28A5761FB2\n\tfor ;\n\tTue, 21 Jun 2022 15:07:54 +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 97136104;\n\tTue, 21 Jun 2022 15:07:53 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1655816876;\n\tbh=Ax3a+mUN6u931PuscTCWwqmfdeFACzdwTY1Rkp2QKE4=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=UZ7b4300OinW+ufU67C/8qWjmHpLMGCcIdXdlgW2ZKfj12Rm55jqyhR0j+7uS2JdJ\n\t7LZdRECz2dcUEil4nbghaVOFrSVPqnVtZGvP5HrFfJrtLkJRH6mYaINQc/yzMpV/6U\n\tq+7B16xwWsYpQ45/8TjVsI0Bvf86nMSQlsEJckzL8nHeCqABc4K3sl55m9OFMglAaF\n\tfyuXaVVXdCuXgVQ9FJMrpP7gddQVD+DlGPI0GdUrj7jWZGkTD96n1mo1+YpAq10vVK\n\tMA2WbAh2QKhOArqoYZs7AOebgPvQVr5qR99CFbPETzb+o5a/67+pLZKkU0UuAixs2x\n\tdwXyB4V5/CI1g==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1655816873;\n\tbh=Ax3a+mUN6u931PuscTCWwqmfdeFACzdwTY1Rkp2QKE4=;\n\th=Date:From:To:Subject:References:In-Reply-To:From;\n\tb=J2zsGm7RoFA9FZrOcHl3pIAlQ5kzmKXLNx+bXc/n8y7h6YVbbv71SJLk+GAxauDmZ\n\tn3U0Tq0Gq7Wngkyv6CJyZs0toKSQILubSUPSSchO8Rw/1AwoP6tiSd7ZOk+dkqoFoJ\n\tBmrFtMFh5iKytc35Q2P6M7toBr8SVLPcR+XYqmnY="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"J2zsGm7R\"; dkim-atps=neutral","Date":"Tue, 21 Jun 2022 16:07:37 +0300","To":"Naushir Patuck ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"","References":"<20220610122533.11888-1-naush@raspberrypi.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 1/3] pipeline: ipa: raspberrypi: Move\n\tControlInfoMap 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":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]