[{"id":23370,"web_url":"https://patchwork.libcamera.org/comment/23370/","msgid":"<YqIa+LPXL1zapH/g@pendragon.ideasonboard.com>","date":"2022-06-09T16:08:24","subject":"Re: [libcamera-devel] [PATCH] pipeline: ipa: raspberrypi: Correctly\n\treport available controls","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 Thu, Jun 09, 2022 at 01:58:30PM +0100, Naushir Patuck via libcamera-devel wrote:\n> The pipeline handler currently advertises a static ControlInfoMap to specify the\n> available controls and their limits. Fix this limitation by having the IPA\n> populate a ControlInfoMap during Camera::Configure() that is then returned from\n> the pipeline handle. This will allow the ExposureTime, AnalogueGain, and\n> FrameDurationLimits controls to advertise the correct limits for the programmed\n> mode.\n> \n> Note that with this change, the ControlInfoMap provided by Camera::Controls()\n> is only valid after a successful Camera::Configure() call.\n\nThat's an issue, we need to initialize it earlier. You can update it at\nconfigure() time, but it has to bet set at init() time.\n\n> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>\n> ---\n>  include/libcamera/ipa/raspberrypi.h           | 55 -------------------\n>  include/libcamera/ipa/raspberrypi.mojom       |  1 +\n>  src/ipa/raspberrypi/raspberrypi.cpp           | 42 +++++++++++++-\n>  .../pipeline/raspberrypi/raspberrypi.cpp      |  6 +-\n>  .../pipeline/raspberrypi/rpi_stream.h         |  1 -\n>  5 files changed, 45 insertions(+), 60 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 <stdint.h>\n> -\n> -#include <libcamera/control_ids.h>\n> -#include <libcamera/controls.h>\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..ed7adebce1b8 100644\n> --- a/include/libcamera/ipa/raspberrypi.mojom\n> +++ b/include/libcamera/ipa/raspberrypi.mojom\n> @@ -40,6 +40,7 @@ struct IPAConfig {\n>  \n>  struct IPAConfigResult {\n>         float modeSensitivity;\n> +       libcamera.ControlInfoMap controlInfo;\n>  };\n>  \n>  struct StartConfig {\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 3b126bb5175e..7eb04a24c41e 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -24,7 +24,6 @@\n>  #include <libcamera/framebuffer.h>\n>  #include <libcamera/ipa/ipa_interface.h>\n>  #include <libcamera/ipa/ipa_module_info.h>\n> -#include <libcamera/ipa/raspberrypi.h>\n>  #include <libcamera/ipa/raspberrypi_ipa_interface.h>\n>  #include <libcamera/request.h>\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 Controls({\n\nWhile at it, could you rename the variable to start with a lowercase\nletter ? \"controls\" is a bit too generic, so you could call it\n\"rpiControls\" or anything similar.\n\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\nAs you've said yourself in\nhttps://lists.libcamera.org/pipermail/libcamera-devel/2022-January/028735.html,\nthis one doesn't belong to the IPA side :-) It can be fixed on top\nthough.\n\nIf it helps moving forward, I can resubmit the patch from\nhttps://lists.libcamera.org/pipermail/libcamera-devel/2022-January/028731.html\nas a proper patch, and then you can update the frame duration limits and\nexposure time controls at configure time on top ?\n\n> +\t{ &controls::FrameDurationLimits, ControlInfo(INT64_C(1000), INT64_C(1000000000)) },\n> +\t{ &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }\n> +}, controls::controls);\n> +\n>  LOG_DEFINE_CATEGORY(IPARPI)\n>  \n>  namespace ipa::RPi {\n> @@ -421,6 +442,25 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,\n>  \tASSERT(controls);\n>  \t*controls = std::move(ctrls);\n>  \n> +\t/*\n> +\t * Apply the correct limits to the exposure, gain and frame duration controls\n> +\t * based on the current sensor mode.\n> +\t */\n> +\tresult->controlInfo = Controls;\n> +\tconst Duration minSensorFrameDuration = mode_.min_frame_length * mode_.line_length;\n> +\tconst Duration maxSensorFrameDuration = mode_.max_frame_length * mode_.line_length;\n> +\tresult->controlInfo.at(controls::FrameDurationLimits.id()) =\n> +\t\t\tControlInfo(static_cast<int64_t>(minSensorFrameDuration.get<std::micro>()),\n> +\t\t\t\t    static_cast<int64_t>(maxSensorFrameDuration.get<std::micro>()));\n> +\n> +\tresult->controlInfo.at(controls::AnalogueGain.id()) =\n> +\t\t\tControlInfo(1.0f, static_cast<float>(helper_->Gain(maxSensorGainCode_)));\n> +\n> +\tconst uint32_t exposureMin = sensorCtrls_.at(V4L2_CID_EXPOSURE).min().get<int32_t>();\n> +\tconst uint32_t exposureMax = sensorCtrls_.at(V4L2_CID_EXPOSURE).max().get<int32_t>();\n> +\tresult->controlInfo.at(controls::ExposureTime.id()) =\n> +\t\t\tControlInfo(static_cast<int32_t>(helper_->Exposure(exposureMin).get<std::micro>()),\n> +\t\t\t\t    static_cast<int32_t>(helper_->Exposure(exposureMax).get<std::micro>()));\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..abb29a8d24b9 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -20,7 +20,6 @@\n>  #include <libcamera/camera.h>\n>  #include <libcamera/control_ids.h>\n>  #include <libcamera/formats.h>\n> -#include <libcamera/ipa/raspberrypi.h>\n>  #include <libcamera/ipa/raspberrypi_ipa_interface.h>\n>  #include <libcamera/ipa/raspberrypi_ipa_proxy.h>\n>  #include <libcamera/logging.h>\n> @@ -941,6 +940,9 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n>  \t/* Store the mode sensitivity for the application. */\n>  \tdata->properties_.set(properties::SensorSensitivity, result.modeSensitivity);\n>  \n> +\t/* Register the controls that the Raspberry Pi IPA can handle. */\n> +\tdata->controlInfo_ = result.controlInfo;\n> +\n>  \t/* Setup the Video Mux/Bridge entities. */\n>  \tfor (auto &[device, link] : data->bridgeDevices_) {\n>  \t\t/*\n> @@ -1282,8 +1284,6 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n>  \tdata->delayedCtrls_ = std::make_unique<DelayedControls>(data->sensor_->device(), params);\n>  \tdata->sensorMetadata_ = sensorConfig.sensorMetadata;\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> 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 <unordered_map>\n>  #include <vector>\n>  \n> -#include <libcamera/ipa/raspberrypi.h>\n>  #include <libcamera/ipa/raspberrypi_ipa_interface.h>\n>  #include <libcamera/stream.h>\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 B3ECDBD160\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu,  9 Jun 2022 16:08:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0D29C65637;\n\tThu,  9 Jun 2022 18:08:32 +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 2FC0E6559A\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu,  9 Jun 2022 18:08:31 +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 9AA362F3;\n\tThu,  9 Jun 2022 18:08:30 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1654790912;\n\tbh=13LxSCvrktevvzoScCqgAimZMEE6+o8RyiiXC7zwptY=;\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=Vv4FTXx71mYLUZz21xK1DkGDy4/wcZ+1b+xbrkJDPEpehA3SqaTavYlP67aIYt/fg\n\tfW5IdkEWKS5MUdk+7n9YDKDzawEx8TBm+U4poCz6uxVFzK/1zw6KsPwEmDLvgbdXKD\n\tWva5b9lKH+ieJPnZ1SFxL6Hy3nWqGdgl+7rXnbbdhOND36O/Tw53wCtFOE3yjDrAss\n\tp0RGYXdONucIM8mue93dhzQLzgjd5CX4vdsROteMgiA1ApWEC4fqC7C2iCNSmChLla\n\tK7HoVDBfr1uloFi73x6eLHRi55IsJ7S5Szaxk4/r4TSrSJCCjY1yBfIs83j5f2diGC\n\tkX31a3JINGvTg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1654790910;\n\tbh=13LxSCvrktevvzoScCqgAimZMEE6+o8RyiiXC7zwptY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=oYu18lSJN/kdNHM+AiHPVgJWL/qERn6aoOVzUy0gLQ4W2iRbiq/ZxJQ3WsIhvMozH\n\tvHGz3sWM2xlZxx/7+4ZNUNRC5s6Gv3tieF2dkcIGKzjomtgGyzO7atPGpgKI6Y6mbp\n\tJBWli15arnWc0+zhxr8wolUV6/gQAhoEENzgfir0="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"oYu18lSJ\"; dkim-atps=neutral","Date":"Thu, 9 Jun 2022 19:08:24 +0300","To":"Naushir Patuck <naush@raspberrypi.com>","Message-ID":"<YqIa+LPXL1zapH/g@pendragon.ideasonboard.com>","References":"<20220609125830.4325-1-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220609125830.4325-1-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH] pipeline: ipa: raspberrypi: Correctly\n\treport available controls","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Laurent Pinchart via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":23376,"web_url":"https://patchwork.libcamera.org/comment/23376/","msgid":"<CAHW6GYLFSXeddC5P4b0UjYRZN87Oioj11EHc_uLUUz=tCrXAww@mail.gmail.com>","date":"2022-06-10T08:03:35","subject":"Re: [libcamera-devel] [PATCH] pipeline: ipa: raspberrypi: Correctly\n\treport available controls","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Naush\n\nThanks very much for this patch. I'm very happy with the functionality\nadded here!\n\nOn Thu, 9 Jun 2022 at 17:08, Laurent Pinchart via libcamera-devel\n<libcamera-devel@lists.libcamera.org> wrote:\n>\n> Hi Naush,\n>\n> Thank you for the patch.\n>\n> On Thu, Jun 09, 2022 at 01:58:30PM +0100, Naushir Patuck via libcamera-devel wrote:\n> > The pipeline handler currently advertises a static ControlInfoMap to specify the\n> > available controls and their limits. Fix this limitation by having the IPA\n> > populate a ControlInfoMap during Camera::Configure() that is then returned from\n> > the pipeline handle. This will allow the ExposureTime, AnalogueGain, and\n> > FrameDurationLimits controls to advertise the correct limits for the programmed\n> > mode.\n> >\n> > Note that with this change, the ControlInfoMap provided by Camera::Controls()\n> > is only valid after a successful Camera::Configure() call.\n>\n> That's an issue, we need to initialize it earlier. You can update it at\n> configure() time, but it has to bet set at init() time.\n>\n> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>\n\nThe only minor thought I had looking through this was whether we could\nsupply some better min/max values for the ScalerCrop (for example, all\nzeroes wouldn't be valid), but I think that's a separate\ndiscussion/patch.\n\nReviewed-by: David Plowman <david.plowman@raspberrypi.com>\n\nThanks\nDavid\n\n>\n> > ---\n> >  include/libcamera/ipa/raspberrypi.h           | 55 -------------------\n> >  include/libcamera/ipa/raspberrypi.mojom       |  1 +\n> >  src/ipa/raspberrypi/raspberrypi.cpp           | 42 +++++++++++++-\n> >  .../pipeline/raspberrypi/raspberrypi.cpp      |  6 +-\n> >  .../pipeline/raspberrypi/rpi_stream.h         |  1 -\n> >  5 files changed, 45 insertions(+), 60 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 <stdint.h>\n> > -\n> > -#include <libcamera/control_ids.h>\n> > -#include <libcamera/controls.h>\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..ed7adebce1b8 100644\n> > --- a/include/libcamera/ipa/raspberrypi.mojom\n> > +++ b/include/libcamera/ipa/raspberrypi.mojom\n> > @@ -40,6 +40,7 @@ struct IPAConfig {\n> >\n> >  struct IPAConfigResult {\n> >         float modeSensitivity;\n> > +       libcamera.ControlInfoMap controlInfo;\n> >  };\n> >\n> >  struct StartConfig {\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index 3b126bb5175e..7eb04a24c41e 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -24,7 +24,6 @@\n> >  #include <libcamera/framebuffer.h>\n> >  #include <libcamera/ipa/ipa_interface.h>\n> >  #include <libcamera/ipa/ipa_module_info.h>\n> > -#include <libcamera/ipa/raspberrypi.h>\n> >  #include <libcamera/ipa/raspberrypi_ipa_interface.h>\n> >  #include <libcamera/request.h>\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 Controls({\n>\n> While at it, could you rename the variable to start with a lowercase\n> letter ? \"controls\" is a bit too generic, so you could call it\n> \"rpiControls\" or anything similar.\n>\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>\n> As you've said yourself in\n> https://lists.libcamera.org/pipermail/libcamera-devel/2022-January/028735.html,\n> this one doesn't belong to the IPA side :-) It can be fixed on top\n> though.\n>\n> If it helps moving forward, I can resubmit the patch from\n> https://lists.libcamera.org/pipermail/libcamera-devel/2022-January/028731.html\n> as a proper patch, and then you can update the frame duration limits and\n> exposure time controls at configure time on top ?\n>\n> > +     { &controls::FrameDurationLimits, ControlInfo(INT64_C(1000), INT64_C(1000000000)) },\n> > +     { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }\n> > +}, controls::controls);\n> > +\n> >  LOG_DEFINE_CATEGORY(IPARPI)\n> >\n> >  namespace ipa::RPi {\n> > @@ -421,6 +442,25 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,\n> >       ASSERT(controls);\n> >       *controls = std::move(ctrls);\n> >\n> > +     /*\n> > +      * Apply the correct limits to the exposure, gain and frame duration controls\n> > +      * based on the current sensor mode.\n> > +      */\n> > +     result->controlInfo = Controls;\n> > +     const Duration minSensorFrameDuration = mode_.min_frame_length * mode_.line_length;\n> > +     const Duration maxSensorFrameDuration = mode_.max_frame_length * mode_.line_length;\n> > +     result->controlInfo.at(controls::FrameDurationLimits.id()) =\n> > +                     ControlInfo(static_cast<int64_t>(minSensorFrameDuration.get<std::micro>()),\n> > +                                 static_cast<int64_t>(maxSensorFrameDuration.get<std::micro>()));\n> > +\n> > +     result->controlInfo.at(controls::AnalogueGain.id()) =\n> > +                     ControlInfo(1.0f, static_cast<float>(helper_->Gain(maxSensorGainCode_)));\n> > +\n> > +     const uint32_t exposureMin = sensorCtrls_.at(V4L2_CID_EXPOSURE).min().get<int32_t>();\n> > +     const uint32_t exposureMax = sensorCtrls_.at(V4L2_CID_EXPOSURE).max().get<int32_t>();\n> > +     result->controlInfo.at(controls::ExposureTime.id()) =\n> > +                     ControlInfo(static_cast<int32_t>(helper_->Exposure(exposureMin).get<std::micro>()),\n> > +                                 static_cast<int32_t>(helper_->Exposure(exposureMax).get<std::micro>()));\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..abb29a8d24b9 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -20,7 +20,6 @@\n> >  #include <libcamera/camera.h>\n> >  #include <libcamera/control_ids.h>\n> >  #include <libcamera/formats.h>\n> > -#include <libcamera/ipa/raspberrypi.h>\n> >  #include <libcamera/ipa/raspberrypi_ipa_interface.h>\n> >  #include <libcamera/ipa/raspberrypi_ipa_proxy.h>\n> >  #include <libcamera/logging.h>\n> > @@ -941,6 +940,9 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> >       /* Store the mode sensitivity for the application. */\n> >       data->properties_.set(properties::SensorSensitivity, result.modeSensitivity);\n> >\n> > +     /* Register the controls that the Raspberry Pi IPA can handle. */\n> > +     data->controlInfo_ = result.controlInfo;\n> > +\n> >       /* Setup the Video Mux/Bridge entities. */\n> >       for (auto &[device, link] : data->bridgeDevices_) {\n> >               /*\n> > @@ -1282,8 +1284,6 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> >       data->delayedCtrls_ = std::make_unique<DelayedControls>(data->sensor_->device(), params);\n> >       data->sensorMetadata_ = sensorConfig.sensorMetadata;\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> > 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 <unordered_map>\n> >  #include <vector>\n> >\n> > -#include <libcamera/ipa/raspberrypi.h>\n> >  #include <libcamera/ipa/raspberrypi_ipa_interface.h>\n> >  #include <libcamera/stream.h>\n> >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 EA1E6BD160\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 10 Jun 2022 08:03:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 22FE365635;\n\tFri, 10 Jun 2022 10:03:49 +0200 (CEST)","from mail-ed1-x52b.google.com (mail-ed1-x52b.google.com\n\t[IPv6:2a00:1450:4864:20::52b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DEF51600F4\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 10 Jun 2022 10:03:46 +0200 (CEST)","by mail-ed1-x52b.google.com with SMTP id z7so34180951edm.13\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 10 Jun 2022 01:03:46 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1654848229;\n\tbh=Gv8Kgo2DkIR5TtoyGjH2axL84rsHSuWfWaNiq9OLR98=;\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=izf3tuhCNAHdb9T31uHVaLz6bJLSk1RyAqIQTQnXYJ4WoXGC1JmIr82eWyssh5+8m\n\tYKF1vXjNU21vv6xMrXd3o4M1UsNrmLHdMavu+RW/VWmwyMHUF5pfCjUWhy+sm5ut7k\n\tqlxpZ9JzWL7U2FJ6DvtSPgv+ys0YoCM4uL7K6DrSpr31z3SwGtNbu9UrILI/jS11i3\n\tQbmT08fgUBNxJEqDmfh9dudGgXJFBpJ3Xhph5lJ9ta+8dz9xGgqbTw5n+oLuXiywAw\n\tJv3kITF8Q0paDz3IQkcsyd4GLTbnGd07PUuVw1OlqyViFAM7jC9LvSwmwugt6bzKX0\n\tem9T9oJKiy8xw==","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=SCBJQv+0e8nVq+Wjj4i1kKtpyRsVCUTjvM/ZpT2Qk2c=;\n\tb=sKKx94xZfs4sJEOWLnkmD8FXsUstSQZkdIsbW4K0qYOaZ+NBKMwZvOGzRL1mjQ8x+q\n\tgz6VhYolYAGuBE/ldS/TdYW1b0WbepqBQ08rXBvFWlR9ekAEZrdhCpAoebVz7PAOh3bv\n\tszcSe8w2LumoHJZXCMzI7NjVwKQMqnnqe1Qw39z34NBiiCH5ViKxBtpD1rWNwLbd9waP\n\tcgXgEZKSn/Wf6T4Y4yE/IWYEjFfXJuCUySkpi3aLbeAs9Ns2A6gJ4IL6BUvjWd25K3wi\n\t2ujYpqqPX7EXU6UFAYc1LG2Gi9Omyl3wLVZFh0oXRmoT0cskwbBwfiETazicYnH16jEf\n\tb9Dg=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"sKKx94xZ\"; 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=SCBJQv+0e8nVq+Wjj4i1kKtpyRsVCUTjvM/ZpT2Qk2c=;\n\tb=q7cEHOfohtyu9/Z4V85Er6VCDLVFOyc4SH0f2pZLSxY22kgCxNdvJahjvtIdKERtjw\n\tRB0yIHzb/BDnX5kCVVK7zX6NCXgSxHZQKwOsNcDs1NC7c7oy+GLVCEybQntmciygKd9G\n\tu6JiUyV0CYlQNRJbc/zB2GvLju1EXJebLfVrsw+XQO22Plmgu5vZFSMkzunc++oq9rQk\n\tlLPEW11xbfsFVg0F5nYFckUPH5i1FoXODUTF/9rPRY8isis6cO23SN+yoJplTLeTc8Pn\n\ts6IBkEAtls1MhvM/BS3TVi6R7hsRW406OLsIv06Hee6ujRhN3jmfHwQT3twCa1PSIJmy\n\tBVQg==","X-Gm-Message-State":"AOAM531Jk0kfKqT1bHPqnXydmlVCZClRchYaMimYV2LaH45HGliCFk0n\n\tJJi257FNia8I5M8VnD1KPiDmG2U1DKlIdXqshgfsTYQdskve6A==","X-Google-Smtp-Source":"ABdhPJx08HzdGpG0HB4S6TS8fHVw3BhUuUI0nL/WTrg0uvtadjueaIwKpBneAq1TOKETY1xFwbsCwRMC36z0a0yqN1c=","X-Received":"by 2002:a05:6402:3482:b0:42d:e063:7c1d with SMTP id\n\tv2-20020a056402348200b0042de0637c1dmr49721053edc.40.1654848226405;\n\tFri, 10 Jun 2022 01:03:46 -0700 (PDT)","MIME-Version":"1.0","References":"<20220609125830.4325-1-naush@raspberrypi.com>\n\t<YqIa+LPXL1zapH/g@pendragon.ideasonboard.com>","In-Reply-To":"<YqIa+LPXL1zapH/g@pendragon.ideasonboard.com>","Date":"Fri, 10 Jun 2022 09:03:35 +0100","Message-ID":"<CAHW6GYLFSXeddC5P4b0UjYRZN87Oioj11EHc_uLUUz=tCrXAww@mail.gmail.com>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH] pipeline: ipa: raspberrypi: Correctly\n\treport available controls","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"David Plowman via libcamera-devel <libcamera-devel@lists.libcamera.org>","Reply-To":"David Plowman <david.plowman@raspberrypi.com>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":23378,"web_url":"https://patchwork.libcamera.org/comment/23378/","msgid":"<CAEmqJPpbpuyybLAMKEqWX47wX5-B-Pmv5FFN=NC_t_9vv5LqxQ@mail.gmail.com>","date":"2022-06-10T08:50:04","subject":"Re: [libcamera-devel] [PATCH] pipeline: ipa: raspberrypi: Correctly\n\treport available controls","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nThank you for your feedback.\n\nOn Thu, 9 Jun 2022 at 17:08, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> Thank you for the patch.\n>\n> On Thu, Jun 09, 2022 at 01:58:30PM +0100, Naushir Patuck via\n> libcamera-devel wrote:\n> > The pipeline handler currently advertises a static ControlInfoMap to\n> specify the\n> > available controls and their limits. Fix this limitation by having the\n> IPA\n> > populate a ControlInfoMap during Camera::Configure() that is then\n> returned from\n> > the pipeline handle. This will allow the ExposureTime, AnalogueGain, and\n> > FrameDurationLimits controls to advertise the correct limits for the\n> programmed\n> > mode.\n> >\n> > Note that with this change, the ControlInfoMap provided by\n> Camera::Controls()\n> > is only valid after a successful Camera::Configure() call.\n>\n> That's an issue, we need to initialize it earlier. You can update it at\n> configure() time, but it has to bet set at init() time.\n>\n\nSure I can do that.  What do you suggest we do about\nthe AnalogueGain, ExposureTime\nand FrameDurationLimits controls during the init() phase?  Should I exclude\nthem from\nthe list, or include time, with min/max values set to 0 or something else?\nI'm leaning towards\nsimply excluding them from the list until the limits are actually known.\n\n\n>\n> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>\n> > ---\n> >  include/libcamera/ipa/raspberrypi.h           | 55 -------------------\n> >  include/libcamera/ipa/raspberrypi.mojom       |  1 +\n> >  src/ipa/raspberrypi/raspberrypi.cpp           | 42 +++++++++++++-\n> >  .../pipeline/raspberrypi/raspberrypi.cpp      |  6 +-\n> >  .../pipeline/raspberrypi/rpi_stream.h         |  1 -\n> >  5 files changed, 45 insertions(+), 60 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 <stdint.h>\n> > -\n> > -#include <libcamera/control_ids.h>\n> > -#include <libcamera/controls.h>\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..ed7adebce1b8 100644\n> > --- a/include/libcamera/ipa/raspberrypi.mojom\n> > +++ b/include/libcamera/ipa/raspberrypi.mojom\n> > @@ -40,6 +40,7 @@ struct IPAConfig {\n> >\n> >  struct IPAConfigResult {\n> >         float modeSensitivity;\n> > +       libcamera.ControlInfoMap controlInfo;\n> >  };\n> >\n> >  struct StartConfig {\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index 3b126bb5175e..7eb04a24c41e 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -24,7 +24,6 @@\n> >  #include <libcamera/framebuffer.h>\n> >  #include <libcamera/ipa/ipa_interface.h>\n> >  #include <libcamera/ipa/ipa_module_info.h>\n> > -#include <libcamera/ipa/raspberrypi.h>\n> >  #include <libcamera/ipa/raspberrypi_ipa_interface.h>\n> >  #include <libcamera/request.h>\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 Controls({\n>\n> While at it, could you rename the variable to start with a lowercase\n> letter ? \"controls\" is a bit too generic, so you could call it\n> \"rpiControls\" or anything similar.\n>\n\nAck.\n\n\n>\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>\n> As you've said yourself in\n>\n> https://lists.libcamera.org/pipermail/libcamera-devel/2022-January/028735.html\n> ,\n> this one doesn't belong to the IPA side :-) It can be fixed on top\n> though.\n>\n\nI can add a patch on top to move ScalerCrop to the pipeline handler.  It's\na bit awkward to code because\nI cannot add elements to a ControlInfoMap after initialisation since it\nderives from a private unordered_map.\nInstead, I will have to construct a new ControlInfoMap::Map with the\ncontents of the ControlInfoMap provided\nby the IPA, add ScalerCrop to the ControlInfoMap::Map, and then convert\nback to a ControlInfoMap.\nCan you suggest a more elegant way to do this?\n\nRegards,\nNaush\n\n\n>\n> If it helps moving forward, I can resubmit the patch from\n>\n> https://lists.libcamera.org/pipermail/libcamera-devel/2022-January/028731.html\n> as a proper patch, and then you can update the frame duration limits and\n> exposure time controls at configure time on top ?\n>\n\n> > +     { &controls::FrameDurationLimits, ControlInfo(INT64_C(1000),\n> INT64_C(1000000000)) },\n> > +     { &controls::draft::NoiseReductionMode,\n> ControlInfo(controls::draft::NoiseReductionModeValues) }\n> > +}, controls::controls);\n> > +\n> >  LOG_DEFINE_CATEGORY(IPARPI)\n> >\n> >  namespace ipa::RPi {\n> > @@ -421,6 +442,25 @@ int IPARPi::configure(const IPACameraSensorInfo\n> &sensorInfo,\n> >       ASSERT(controls);\n> >       *controls = std::move(ctrls);\n> >\n> > +     /*\n> > +      * Apply the correct limits to the exposure, gain and frame\n> duration controls\n> > +      * based on the current sensor mode.\n> > +      */\n> > +     result->controlInfo = Controls;\n> > +     const Duration minSensorFrameDuration = mode_.min_frame_length *\n> mode_.line_length;\n> > +     const Duration maxSensorFrameDuration = mode_.max_frame_length *\n> mode_.line_length;\n> > +     result->controlInfo.at(controls::FrameDurationLimits.id()) =\n> > +\n>  ControlInfo(static_cast<int64_t>(minSensorFrameDuration.get<std::micro>()),\n> > +\n>  static_cast<int64_t>(maxSensorFrameDuration.get<std::micro>()));\n> > +\n> > +     result->controlInfo.at(controls::AnalogueGain.id()) =\n> > +                     ControlInfo(1.0f,\n> static_cast<float>(helper_->Gain(maxSensorGainCode_)));\n> > +\n> > +     const uint32_t exposureMin =\n> sensorCtrls_.at(V4L2_CID_EXPOSURE).min().get<int32_t>();\n> > +     const uint32_t exposureMax =\n> sensorCtrls_.at(V4L2_CID_EXPOSURE).max().get<int32_t>();\n> > +     result->controlInfo.at(controls::ExposureTime.id()) =\n> > +\n>  ControlInfo(static_cast<int32_t>(helper_->Exposure(exposureMin).get<std::micro>()),\n> > +\n>  static_cast<int32_t>(helper_->Exposure(exposureMax).get<std::micro>()));\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..abb29a8d24b9 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -20,7 +20,6 @@\n> >  #include <libcamera/camera.h>\n> >  #include <libcamera/control_ids.h>\n> >  #include <libcamera/formats.h>\n> > -#include <libcamera/ipa/raspberrypi.h>\n> >  #include <libcamera/ipa/raspberrypi_ipa_interface.h>\n> >  #include <libcamera/ipa/raspberrypi_ipa_proxy.h>\n> >  #include <libcamera/logging.h>\n> > @@ -941,6 +940,9 @@ int PipelineHandlerRPi::configure(Camera *camera,\n> CameraConfiguration *config)\n> >       /* Store the mode sensitivity for the application. */\n> >       data->properties_.set(properties::SensorSensitivity,\n> result.modeSensitivity);\n> >\n> > +     /* Register the controls that the Raspberry Pi IPA can handle. */\n> > +     data->controlInfo_ = result.controlInfo;\n> > +\n> >       /* Setup the Video Mux/Bridge entities. */\n> >       for (auto &[device, link] : data->bridgeDevices_) {\n> >               /*\n> > @@ -1282,8 +1284,6 @@ int PipelineHandlerRPi::registerCamera(MediaDevice\n> *unicam, MediaDevice *isp, Me\n> >       data->delayedCtrls_ =\n> std::make_unique<DelayedControls>(data->sensor_->device(), params);\n> >       data->sensorMetadata_ = sensorConfig.sensorMetadata;\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> > 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 <unordered_map>\n> >  #include <vector>\n> >\n> > -#include <libcamera/ipa/raspberrypi.h>\n> >  #include <libcamera/ipa/raspberrypi_ipa_interface.h>\n> >  #include <libcamera/stream.h>\n> >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 9E54CBD160\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 10 Jun 2022 08:50:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DD66765637;\n\tFri, 10 Jun 2022 10:50:22 +0200 (CEST)","from mail-qt1-x82b.google.com (mail-qt1-x82b.google.com\n\t[IPv6:2607:f8b0:4864:20::82b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 10861600F4\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 10 Jun 2022 10:50:21 +0200 (CEST)","by mail-qt1-x82b.google.com with SMTP id c8so18908884qtj.1\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 10 Jun 2022 01:50:20 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1654851022;\n\tbh=acRcEJ+frl+V19zlta+whCfqKT0IfAk3hlgHOhxWDuA=;\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=alTvHy0KlWscHNWfOPI/KZa9zj9sNSVNoEsTKzmEM6TBrBUaVfKvmwZn529MOUB2Z\n\tpuzUBJRYhgiWQ51aLyFfWuncQjrnAKKHc1e0VTX3rGCeUpqp+UpWICEr1sQqudmQ1t\n\tGTrxwoeB5NGXYj5nrMBjNyc1+2lrFegbpwsh9clvzkzDt0iHDiOP+o0bubn/yCBNHj\n\tsDwbJxmuVPdtHVwu9HTfxOEyPsJ+f5A08HhXduxB/QWTg6Le0nVMpAVtNIRUSbMO+A\n\tSToee0QR8iWmc2s/5Vz7v4nkcvolX6JKtRRrMSjpEy2x/6tW2hbO69rrj9aOkmmk8Q\n\tlOZkoq8NZv5Ug==","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=380K056tfv8VTSz9cgmaIBwsWwRpF15k6/x+fjoZAB8=;\n\tb=c4M6P+AyrNao8nYVSe9uVn9a7cfhmP/oGgGPZPSrEkOWZyqLWs7sOe5ZmLuah+U4ai\n\tBeaqsA0Uq0aw/lJ2VbynbbcN3brY6f72PUpM5+m8KkHg1CbWuqqcV8qu23LNscDbc+Ew\n\tm8p6pz3pOTBUAhkClk25mOLGZVI9uSbrbr+bVcOIm06bJklr+PbYJAVWu0Nhg8tgcln6\n\ty/HCql8xSNHJvqoORl4+nTpW79DUQY8n0fQCq/em5S2rgd4AH4Yr9VatyS9Hormol59s\n\twaqwiDOItkuqNp/SmWyX0Ola2dS5/9XAwmpAotSvfyJ5s2DsZ49KdLvxOr/Gdbr30ARn\n\tr6Jw=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"c4M6P+Ay\"; 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=380K056tfv8VTSz9cgmaIBwsWwRpF15k6/x+fjoZAB8=;\n\tb=vV+FPV68zLxVYxXZZN0q7kro6Hb5C8UV0ELgzEkSRfKnUMfCUvAlMpx80HF8e+hLWF\n\ttBiNtYWrdNm8xdJZCI27l7aQxLMzgJOLQn4EOmgbMTXof/UpbJTlnBKUPG/pZjzU/oBV\n\taSLo63CqOBZ6KlDH7cxktVTshk6NlnL6OIcOWZmbdQyHichGbmoNeRJ9HXJRXRDoXGP7\n\tQ/D32fW6mHfp5Pgw+FyRBecaqH6aJRHircwQO7QTJUq3e1eo5mXzedtdk/fUwBc3Bnvi\n\tHEd2hd+7yG1oAPxNG9pIaUrjn6T7HsJdLLHqrNP1q9VMuatalh6kLwqTZsNjJTyEgYxU\n\tycMQ==","X-Gm-Message-State":"AOAM532pYQVXdQmHymtQLZ2Ht3/k7gEfEw50arc5jP4APU13TEfC/J8k\n\trMFQsZLPHF1FcdbcQkNdjVePtDn9nidVXWUNV28u4Aybe9WLX8Fz","X-Google-Smtp-Source":"ABdhPJz3qytWQoJaYKLI/fb58SEifQRasP7EFbzeW6Xd4P9eyGZPI4EaIg6n3SdkSV85OeU5pApBIs9E9/PGv35qc18=","X-Received":"by 2002:ac8:7f0d:0:b0:304:fe93:8e77 with SMTP id\n\tf13-20020ac87f0d000000b00304fe938e77mr13058846qtk.70.1654851019774;\n\tFri, 10 Jun 2022 01:50:19 -0700 (PDT)","MIME-Version":"1.0","References":"<20220609125830.4325-1-naush@raspberrypi.com>\n\t<YqIa+LPXL1zapH/g@pendragon.ideasonboard.com>","In-Reply-To":"<YqIa+LPXL1zapH/g@pendragon.ideasonboard.com>","Date":"Fri, 10 Jun 2022 09:50:04 +0100","Message-ID":"<CAEmqJPpbpuyybLAMKEqWX47wX5-B-Pmv5FFN=NC_t_9vv5LqxQ@mail.gmail.com>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Content-Type":"multipart/alternative; boundary=\"00000000000030b02b05e1140791\"","Subject":"Re: [libcamera-devel] [PATCH] pipeline: ipa: raspberrypi: Correctly\n\treport available controls","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Naushir Patuck via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Naushir Patuck <naush@raspberrypi.com>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":23382,"web_url":"https://patchwork.libcamera.org/comment/23382/","msgid":"<CAEmqJPr4c4ZRAGh=i6ym0=KZDV2YGaf2LAM5--b=ux5yHTWVug@mail.gmail.com>","date":"2022-06-10T09:27:23","subject":"Re: [libcamera-devel] [PATCH] pipeline: ipa: raspberrypi: Correctly\n\treport available controls","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi David,\n\nThank you for your feedback.\n\nOn Fri, 10 Jun 2022 at 09:04, David Plowman <david.plowman@raspberrypi.com>\nwrote:\n\n> Hi Naush\n>\n> Thanks very much for this patch. I'm very happy with the functionality\n> added here!\n>\n> On Thu, 9 Jun 2022 at 17:08, Laurent Pinchart via libcamera-devel\n> <libcamera-devel@lists.libcamera.org> wrote:\n> >\n> > Hi Naush,\n> >\n> > Thank you for the patch.\n> >\n> > On Thu, Jun 09, 2022 at 01:58:30PM +0100, Naushir Patuck via\n> libcamera-devel wrote:\n> > > The pipeline handler currently advertises a static ControlInfoMap to\n> specify the\n> > > available controls and their limits. Fix this limitation by having the\n> IPA\n> > > populate a ControlInfoMap during Camera::Configure() that is then\n> returned from\n> > > the pipeline handle. This will allow the ExposureTime, AnalogueGain,\n> and\n> > > FrameDurationLimits controls to advertise the correct limits for the\n> programmed\n> > > mode.\n> > >\n> > > Note that with this change, the ControlInfoMap provided by\n> Camera::Controls()\n> > > is only valid after a successful Camera::Configure() call.\n> >\n> > That's an issue, we need to initialize it earlier. You can update it at\n> > configure() time, but it has to bet set at init() time.\n> >\n> > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>\n>\n> The only minor thought I had looking through this was whether we could\n> supply some better min/max values for the ScalerCrop (for example, all\n> zeroes wouldn't be valid), but I think that's a separate\n> discussion/patch.\n>\n\nI can add a patch that does this since I will be moving the ScalerCrop\ncontrol\nsetup to the pipeline handler.  About setting the limits, presumably the max\nwants to be set to the same value as properties::ScalerCropMaximum, which\nin turn is the value of the \"analgCrop\" from the sensor.  Does this mean\nthat\nproperties::ScalerCropMaximum might become redundant?\n\nRegards,\nNaush\n\n\n\n>\n> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>\n>\n> Thanks\n> David\n>\n> >\n> > > ---\n> > >  include/libcamera/ipa/raspberrypi.h           | 55 -------------------\n> > >  include/libcamera/ipa/raspberrypi.mojom       |  1 +\n> > >  src/ipa/raspberrypi/raspberrypi.cpp           | 42 +++++++++++++-\n> > >  .../pipeline/raspberrypi/raspberrypi.cpp      |  6 +-\n> > >  .../pipeline/raspberrypi/rpi_stream.h         |  1 -\n> > >  5 files changed, 45 insertions(+), 60 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\n> Pi\n> > > - */\n> > > -\n> > > -#pragma once\n> > > -\n> > > -#include <stdint.h>\n> > > -\n> > > -#include <libcamera/control_ids.h>\n> > > -#include <libcamera/controls.h>\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\n> 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,\n> 0.0f) },\n> > > -             { &controls::AwbEnable, ControlInfo(false, true) },\n> > > -             { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n> > > -             { &controls::AwbMode,\n> ControlInfo(controls::AwbModeValues) },\n> > > -             { &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f)\n> },\n> > > -             { &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },\n> > > -             { &controls::Saturation, ControlInfo(0.0f, 32.0f, 1.0f)\n> },\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..ed7adebce1b8 100644\n> > > --- a/include/libcamera/ipa/raspberrypi.mojom\n> > > +++ b/include/libcamera/ipa/raspberrypi.mojom\n> > > @@ -40,6 +40,7 @@ struct IPAConfig {\n> > >\n> > >  struct IPAConfigResult {\n> > >         float modeSensitivity;\n> > > +       libcamera.ControlInfoMap controlInfo;\n> > >  };\n> > >\n> > >  struct StartConfig {\n> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > index 3b126bb5175e..7eb04a24c41e 100644\n> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > @@ -24,7 +24,6 @@\n> > >  #include <libcamera/framebuffer.h>\n> > >  #include <libcamera/ipa/ipa_interface.h>\n> > >  #include <libcamera/ipa/ipa_module_info.h>\n> > > -#include <libcamera/ipa/raspberrypi.h>\n> > >  #include <libcamera/ipa/raspberrypi_ipa_interface.h>\n> > >  #include <libcamera/request.h>\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 Controls({\n> >\n> > While at it, could you rename the variable to start with a lowercase\n> > letter ? \"controls\" is a bit too generic, so you could call it\n> > \"rpiControls\" or anything similar.\n> >\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> },\n> > > +     { &controls::ScalerCrop, ControlInfo(Rectangle{},\n> Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },\n> >\n> > As you've said yourself in\n> >\n> https://lists.libcamera.org/pipermail/libcamera-devel/2022-January/028735.html\n> ,\n> > this one doesn't belong to the IPA side :-) It can be fixed on top\n> > though.\n> >\n> > If it helps moving forward, I can resubmit the patch from\n> >\n> https://lists.libcamera.org/pipermail/libcamera-devel/2022-January/028731.html\n> > as a proper patch, and then you can update the frame duration limits and\n> > exposure time controls at configure time on top ?\n> >\n> > > +     { &controls::FrameDurationLimits, ControlInfo(INT64_C(1000),\n> INT64_C(1000000000)) },\n> > > +     { &controls::draft::NoiseReductionMode,\n> ControlInfo(controls::draft::NoiseReductionModeValues) }\n> > > +}, controls::controls);\n> > > +\n> > >  LOG_DEFINE_CATEGORY(IPARPI)\n> > >\n> > >  namespace ipa::RPi {\n> > > @@ -421,6 +442,25 @@ int IPARPi::configure(const IPACameraSensorInfo\n> &sensorInfo,\n> > >       ASSERT(controls);\n> > >       *controls = std::move(ctrls);\n> > >\n> > > +     /*\n> > > +      * Apply the correct limits to the exposure, gain and frame\n> duration controls\n> > > +      * based on the current sensor mode.\n> > > +      */\n> > > +     result->controlInfo = Controls;\n> > > +     const Duration minSensorFrameDuration = mode_.min_frame_length *\n> mode_.line_length;\n> > > +     const Duration maxSensorFrameDuration = mode_.max_frame_length *\n> mode_.line_length;\n> > > +     result->controlInfo.at(controls::FrameDurationLimits.id()) =\n> > > +\n>  ControlInfo(static_cast<int64_t>(minSensorFrameDuration.get<std::micro>()),\n> > > +\n>  static_cast<int64_t>(maxSensorFrameDuration.get<std::micro>()));\n> > > +\n> > > +     result->controlInfo.at(controls::AnalogueGain.id()) =\n> > > +                     ControlInfo(1.0f,\n> static_cast<float>(helper_->Gain(maxSensorGainCode_)));\n> > > +\n> > > +     const uint32_t exposureMin =\n> sensorCtrls_.at(V4L2_CID_EXPOSURE).min().get<int32_t>();\n> > > +     const uint32_t exposureMax =\n> sensorCtrls_.at(V4L2_CID_EXPOSURE).max().get<int32_t>();\n> > > +     result->controlInfo.at(controls::ExposureTime.id()) =\n> > > +\n>  ControlInfo(static_cast<int32_t>(helper_->Exposure(exposureMin).get<std::micro>()),\n> > > +\n>  static_cast<int32_t>(helper_->Exposure(exposureMax).get<std::micro>()));\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..abb29a8d24b9 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > @@ -20,7 +20,6 @@\n> > >  #include <libcamera/camera.h>\n> > >  #include <libcamera/control_ids.h>\n> > >  #include <libcamera/formats.h>\n> > > -#include <libcamera/ipa/raspberrypi.h>\n> > >  #include <libcamera/ipa/raspberrypi_ipa_interface.h>\n> > >  #include <libcamera/ipa/raspberrypi_ipa_proxy.h>\n> > >  #include <libcamera/logging.h>\n> > > @@ -941,6 +940,9 @@ int PipelineHandlerRPi::configure(Camera *camera,\n> CameraConfiguration *config)\n> > >       /* Store the mode sensitivity for the application. */\n> > >       data->properties_.set(properties::SensorSensitivity,\n> result.modeSensitivity);\n> > >\n> > > +     /* Register the controls that the Raspberry Pi IPA can handle. */\n> > > +     data->controlInfo_ = result.controlInfo;\n> > > +\n> > >       /* Setup the Video Mux/Bridge entities. */\n> > >       for (auto &[device, link] : data->bridgeDevices_) {\n> > >               /*\n> > > @@ -1282,8 +1284,6 @@ int\n> PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> > >       data->delayedCtrls_ =\n> std::make_unique<DelayedControls>(data->sensor_->device(), params);\n> > >       data->sensorMetadata_ = sensorConfig.sensorMetadata;\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> > > 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 <unordered_map>\n> > >  #include <vector>\n> > >\n> > > -#include <libcamera/ipa/raspberrypi.h>\n> > >  #include <libcamera/ipa/raspberrypi_ipa_interface.h>\n> > >  #include <libcamera/stream.h>\n> > >\n> >\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 D1FCCBD160\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 10 Jun 2022 09:27:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8516F65635;\n\tFri, 10 Jun 2022 11:27:41 +0200 (CEST)","from mail-lf1-x12c.google.com (mail-lf1-x12c.google.com\n\t[IPv6:2a00:1450:4864:20::12c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 05F87600F4\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 10 Jun 2022 11:27:39 +0200 (CEST)","by mail-lf1-x12c.google.com with SMTP id c4so9871423lfj.12\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 10 Jun 2022 02:27:38 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1654853261;\n\tbh=QtyDdN64yDdUu3qH0T1fM8u3LRANtemUFOdh/0Ig2+4=;\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=CxTpjP3qB0eiha6UpBFIvt+lfwAIV4i+hd7WV3VAvA9Pur5s9iBh3YnM9EMgaOXCR\n\tHmfCK7Ksio3WUaR7KdAaHdIwlSOMhUj0uqhhwzytINk/Ia83p9TCOQWvtI1o3qKZki\n\t6r6CD1ouX3Z9cVyRhDOH6fUjSOlwjjIScSSbynpZbSIztQjHn/b1VpDGg8vWFqeZXv\n\tghkyAynV2e541qtRIDJQVQFzEU6Sipih1V57qr08Wk5VfwQScqhwQKqAbQS4kkE57E\n\tjICr/FzGjn29P2u+wfemJJNFaQDlkoqxSjTbCgD/DTr0KdAaOtZwsCNYdfbM2xF93O\n\t5R5Cef12WkYvQ==","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=bUSevDE1GKplz2H2ocxDi1rGET/Wwx9P75FbkioFdJs=;\n\tb=sGqeCB35i3eT9ZEupkJcEHcq+vHuaVATVigXikfabchcMFm1nh5yrIUU1szPPEkGJo\n\t6GT4x4ywoY5etUVi46y1h+7qCya9wAHCni79vRZuVHy83ZA31UBJCJroEy6P5u73iynQ\n\txqBX4XoYY8n4wo221hrd2sAAoc5ohKhdFFlFcLaSw2vlF3qLHZic/M2fOS3pydzLFXrg\n\tpMnGcSuc2xjg8IBHKRzt4Ka2IUUBdp8/5hvRJFkFXtwhInILlAgIjQMJ2l+opRLuGr9R\n\tOHYiQxZVUvXEMsQhtJ7gMuDVQdQiOoatar2kmnU+zg8M+GGVjzkGfK1MMdddBjH+cPHE\n\tTMlw=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"sGqeCB35\"; 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=bUSevDE1GKplz2H2ocxDi1rGET/Wwx9P75FbkioFdJs=;\n\tb=IhWHR+wGQpfU3cQjNpLk/Ml8cVSit3g2zCAxisGSVurPlUtovVeu2DasdbtKTKEv1h\n\tJ/RVNxjhCySYi11/N66fTWsM/HjCT6q3+aY2ybwoKGrQaRLbVpCGEqQFJguq8VdYmLee\n\t30iPD+Vo8kJEkK5ifivNPYHIr+uTXsgJn+NF0+Xw8iyTZf1l/0x0w4KLQWbgTeQ5Di34\n\tzMfILKWNOye24rmqaMbUnbBMVo8FWyoOgPNKLgyWR5ksEg1A83iVgDRrX0sgkxJlWB0R\n\tCSYecbz6uXfJ88nEVwafobk2OmHMlAugDqvUwLJFKYzkvv5eSjuNDP6i0iOFIlS1YiHr\n\t0lAw==","X-Gm-Message-State":"AOAM533X8xtIOXKMGcMOkIqFvyTQGN2PPvkTSMZvkQxiuUQr8CgbeLph\n\tFE1t7kl7uqjQQVlSsKQ+SfA0nTgxerTRfSgIuzkHPcXvqzyxGtSC","X-Google-Smtp-Source":"ABdhPJxp8l0gmHBFDxR3ILoh6RqjxEHTCrhzkvp/tBxzYcDQF1P6NFk0m7DBO/ZmO9wkMe7iJ+MxgugzWv5z9jM7dKM=","X-Received":"by 2002:ac2:4c02:0:b0:478:f664:d909 with SMTP id\n\tt2-20020ac24c02000000b00478f664d909mr26313568lfq.330.1654853258197;\n\tFri, 10 Jun 2022 02:27:38 -0700 (PDT)","MIME-Version":"1.0","References":"<20220609125830.4325-1-naush@raspberrypi.com>\n\t<YqIa+LPXL1zapH/g@pendragon.ideasonboard.com>\n\t<CAHW6GYLFSXeddC5P4b0UjYRZN87Oioj11EHc_uLUUz=tCrXAww@mail.gmail.com>","In-Reply-To":"<CAHW6GYLFSXeddC5P4b0UjYRZN87Oioj11EHc_uLUUz=tCrXAww@mail.gmail.com>","Date":"Fri, 10 Jun 2022 10:27:23 +0100","Message-ID":"<CAEmqJPr4c4ZRAGh=i6ym0=KZDV2YGaf2LAM5--b=ux5yHTWVug@mail.gmail.com>","To":"David Plowman <david.plowman@raspberrypi.com>","Content-Type":"multipart/alternative; boundary=\"0000000000009c788b05e1148c84\"","Subject":"Re: [libcamera-devel] [PATCH] pipeline: ipa: raspberrypi: Correctly\n\treport available controls","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Naushir Patuck via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Naushir Patuck <naush@raspberrypi.com>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":23384,"web_url":"https://patchwork.libcamera.org/comment/23384/","msgid":"<CAEmqJPo9_+LTvgfc35bjm_sK=-WvkEQT6aNSNGwoFbODRhGQPw@mail.gmail.com>","date":"2022-06-10T10:02:26","subject":"Re: [libcamera-devel] [PATCH] pipeline: ipa: raspberrypi: Correctly\n\treport available controls","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"On Fri, 10 Jun 2022 at 10:27, Naushir Patuck <naush@raspberrypi.com> wrote:\n\n> Hi David,\n>\n> Thank you for your feedback.\n>\n> On Fri, 10 Jun 2022 at 09:04, David Plowman <david.plowman@raspberrypi.com>\n> wrote:\n>\n>> Hi Naush\n>>\n>> Thanks very much for this patch. I'm very happy with the functionality\n>> added here!\n>>\n>> On Thu, 9 Jun 2022 at 17:08, Laurent Pinchart via libcamera-devel\n>> <libcamera-devel@lists.libcamera.org> wrote:\n>> >\n>> > Hi Naush,\n>> >\n>> > Thank you for the patch.\n>> >\n>> > On Thu, Jun 09, 2022 at 01:58:30PM +0100, Naushir Patuck via\n>> libcamera-devel wrote:\n>> > > The pipeline handler currently advertises a static ControlInfoMap to\n>> specify the\n>> > > available controls and their limits. Fix this limitation by having\n>> the IPA\n>> > > populate a ControlInfoMap during Camera::Configure() that is then\n>> returned from\n>> > > the pipeline handle. This will allow the ExposureTime, AnalogueGain,\n>> and\n>> > > FrameDurationLimits controls to advertise the correct limits for the\n>> programmed\n>> > > mode.\n>> > >\n>> > > Note that with this change, the ControlInfoMap provided by\n>> Camera::Controls()\n>> > > is only valid after a successful Camera::Configure() call.\n>> >\n>> > That's an issue, we need to initialize it earlier. You can update it at\n>> > configure() time, but it has to bet set at init() time.\n>> >\n>> > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>\n>>\n>> The only minor thought I had looking through this was whether we could\n>> supply some better min/max values for the ScalerCrop (for example, all\n>> zeroes wouldn't be valid), but I think that's a separate\n>> discussion/patch.\n>>\n>\n> I can add a patch that does this since I will be moving the ScalerCrop\n> control\n> setup to the pipeline handler.  About setting the limits, presumably the\n> max\n> wants to be set to the same value as properties::ScalerCropMaximum, which\n> in turn is the value of the \"analgCrop\" from the sensor.  Does this mean\n> that\n> properties::ScalerCropMaximum might become redundant?\n>\n\nOops, I got that wrong.  properties::ScalerCropMaximum is the sensor crop of\nvisible pixels, i.e. the full resolution readout.  I need the current mode\nreadout,\nwhich might be different!\n\n\n\n>\n> Regards,\n> Naush\n>\n>\n>\n>>\n>> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>\n>>\n>> Thanks\n>> David\n>>\n>> >\n>> > > ---\n>> > >  include/libcamera/ipa/raspberrypi.h           | 55\n>> -------------------\n>> > >  include/libcamera/ipa/raspberrypi.mojom       |  1 +\n>> > >  src/ipa/raspberrypi/raspberrypi.cpp           | 42 +++++++++++++-\n>> > >  .../pipeline/raspberrypi/raspberrypi.cpp      |  6 +-\n>> > >  .../pipeline/raspberrypi/rpi_stream.h         |  1 -\n>> > >  5 files changed, 45 insertions(+), 60 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\n>> Raspberry Pi\n>> > > - */\n>> > > -\n>> > > -#pragma once\n>> > > -\n>> > > -#include <stdint.h>\n>> > > -\n>> > > -#include <libcamera/control_ids.h>\n>> > > -#include <libcamera/controls.h>\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\n>> that\n>> > > - * point applications should check first whether a control is\n>> 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,\n>> 0.0f) },\n>> > > -             { &controls::AwbEnable, ControlInfo(false, true) },\n>> > > -             { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n>> > > -             { &controls::AwbMode,\n>> ControlInfo(controls::AwbModeValues) },\n>> > > -             { &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f)\n>> },\n>> > > -             { &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },\n>> > > -             { &controls::Saturation, ControlInfo(0.0f, 32.0f, 1.0f)\n>> },\n>> > > -             { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f)\n>> },\n>> > > -             { &controls::ColourCorrectionMatrix,\n>> ControlInfo(-16.0f, 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..ed7adebce1b8 100644\n>> > > --- a/include/libcamera/ipa/raspberrypi.mojom\n>> > > +++ b/include/libcamera/ipa/raspberrypi.mojom\n>> > > @@ -40,6 +40,7 @@ struct IPAConfig {\n>> > >\n>> > >  struct IPAConfigResult {\n>> > >         float modeSensitivity;\n>> > > +       libcamera.ControlInfoMap controlInfo;\n>> > >  };\n>> > >\n>> > >  struct StartConfig {\n>> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n>> b/src/ipa/raspberrypi/raspberrypi.cpp\n>> > > index 3b126bb5175e..7eb04a24c41e 100644\n>> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n>> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n>> > > @@ -24,7 +24,6 @@\n>> > >  #include <libcamera/framebuffer.h>\n>> > >  #include <libcamera/ipa/ipa_interface.h>\n>> > >  #include <libcamera/ipa/ipa_module_info.h>\n>> > > -#include <libcamera/ipa/raspberrypi.h>\n>> > >  #include <libcamera/ipa/raspberrypi_ipa_interface.h>\n>> > >  #include <libcamera/request.h>\n>> > >\n>> > > @@ -72,6 +71,28 @@ constexpr Duration defaultMaxFrameDuration =\n>> 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 Controls({\n>> >\n>> > While at it, could you rename the variable to start with a lowercase\n>> > letter ? \"controls\" is a bit too generic, so you could call it\n>> > \"rpiControls\" or anything similar.\n>> >\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>> },\n>> > > +     { &controls::ScalerCrop, ControlInfo(Rectangle{},\n>> Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },\n>> >\n>> > As you've said yourself in\n>> >\n>> https://lists.libcamera.org/pipermail/libcamera-devel/2022-January/028735.html\n>> ,\n>> > this one doesn't belong to the IPA side :-) It can be fixed on top\n>> > though.\n>> >\n>> > If it helps moving forward, I can resubmit the patch from\n>> >\n>> https://lists.libcamera.org/pipermail/libcamera-devel/2022-January/028731.html\n>> > as a proper patch, and then you can update the frame duration limits and\n>> > exposure time controls at configure time on top ?\n>> >\n>> > > +     { &controls::FrameDurationLimits, ControlInfo(INT64_C(1000),\n>> INT64_C(1000000000)) },\n>> > > +     { &controls::draft::NoiseReductionMode,\n>> ControlInfo(controls::draft::NoiseReductionModeValues) }\n>> > > +}, controls::controls);\n>> > > +\n>> > >  LOG_DEFINE_CATEGORY(IPARPI)\n>> > >\n>> > >  namespace ipa::RPi {\n>> > > @@ -421,6 +442,25 @@ int IPARPi::configure(const IPACameraSensorInfo\n>> &sensorInfo,\n>> > >       ASSERT(controls);\n>> > >       *controls = std::move(ctrls);\n>> > >\n>> > > +     /*\n>> > > +      * Apply the correct limits to the exposure, gain and frame\n>> duration controls\n>> > > +      * based on the current sensor mode.\n>> > > +      */\n>> > > +     result->controlInfo = Controls;\n>> > > +     const Duration minSensorFrameDuration = mode_.min_frame_length\n>> * mode_.line_length;\n>> > > +     const Duration maxSensorFrameDuration = mode_.max_frame_length\n>> * mode_.line_length;\n>> > > +     result->controlInfo.at(controls::FrameDurationLimits.id()) =\n>> > > +\n>>  ControlInfo(static_cast<int64_t>(minSensorFrameDuration.get<std::micro>()),\n>> > > +\n>>  static_cast<int64_t>(maxSensorFrameDuration.get<std::micro>()));\n>> > > +\n>> > > +     result->controlInfo.at(controls::AnalogueGain.id()) =\n>> > > +                     ControlInfo(1.0f,\n>> static_cast<float>(helper_->Gain(maxSensorGainCode_)));\n>> > > +\n>> > > +     const uint32_t exposureMin =\n>> sensorCtrls_.at(V4L2_CID_EXPOSURE).min().get<int32_t>();\n>> > > +     const uint32_t exposureMax =\n>> sensorCtrls_.at(V4L2_CID_EXPOSURE).max().get<int32_t>();\n>> > > +     result->controlInfo.at(controls::ExposureTime.id()) =\n>> > > +\n>>  ControlInfo(static_cast<int32_t>(helper_->Exposure(exposureMin).get<std::micro>()),\n>> > > +\n>>  static_cast<int32_t>(helper_->Exposure(exposureMax).get<std::micro>()));\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..abb29a8d24b9 100644\n>> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>> > > @@ -20,7 +20,6 @@\n>> > >  #include <libcamera/camera.h>\n>> > >  #include <libcamera/control_ids.h>\n>> > >  #include <libcamera/formats.h>\n>> > > -#include <libcamera/ipa/raspberrypi.h>\n>> > >  #include <libcamera/ipa/raspberrypi_ipa_interface.h>\n>> > >  #include <libcamera/ipa/raspberrypi_ipa_proxy.h>\n>> > >  #include <libcamera/logging.h>\n>> > > @@ -941,6 +940,9 @@ int PipelineHandlerRPi::configure(Camera *camera,\n>> CameraConfiguration *config)\n>> > >       /* Store the mode sensitivity for the application. */\n>> > >       data->properties_.set(properties::SensorSensitivity,\n>> result.modeSensitivity);\n>> > >\n>> > > +     /* Register the controls that the Raspberry Pi IPA can handle.\n>> */\n>> > > +     data->controlInfo_ = result.controlInfo;\n>> > > +\n>> > >       /* Setup the Video Mux/Bridge entities. */\n>> > >       for (auto &[device, link] : data->bridgeDevices_) {\n>> > >               /*\n>> > > @@ -1282,8 +1284,6 @@ int\n>> PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n>> > >       data->delayedCtrls_ =\n>> std::make_unique<DelayedControls>(data->sensor_->device(), params);\n>> > >       data->sensorMetadata_ = sensorConfig.sensorMetadata;\n>> > >\n>> > > -     /* Register the controls that the Raspberry Pi IPA can handle.\n>> */\n>> > > -     data->controlInfo_ = RPi::Controls;\n>> > >       /* Initialize the camera properties. */\n>> > >       data->properties_ = data->sensor_->properties();\n>> > >\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 <unordered_map>\n>> > >  #include <vector>\n>> > >\n>> > > -#include <libcamera/ipa/raspberrypi.h>\n>> > >  #include <libcamera/ipa/raspberrypi_ipa_interface.h>\n>> > >  #include <libcamera/stream.h>\n>> > >\n>> >\n>> > --\n>> > Regards,\n>> >\n>> > Laurent Pinchart\n>>\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 594DCBD160\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 10 Jun 2022 10:02:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4B84865637;\n\tFri, 10 Jun 2022 12:02:43 +0200 (CEST)","from mail-lf1-x12d.google.com (mail-lf1-x12d.google.com\n\t[IPv6:2a00:1450:4864:20::12d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 909A9633A4\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 10 Jun 2022 12:02:42 +0200 (CEST)","by mail-lf1-x12d.google.com with SMTP id a15so42025644lfb.9\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 10 Jun 2022 03:02:42 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1654855363;\n\tbh=qxu+0YGOCpf60tykcE/5VUpm0rl5EGdhKTOOFMu6R3Y=;\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=TfsuIpHEPyW0eNXJ1xccy/3iz12JF1Yx3yfY9gs5i2lIDhhEkkc65iv9JwAC8Zu6c\n\t/2fPnb2q1a3fEe5RGHg4OBED/MeRP9bHPTqGWNSqQai0SVwbdrlI4ZxqjwYcItDy2p\n\tlpFCGF982rPp7NAA6VyEYfvm2UkRK2fO98ZjSSK7F8Qp57Z8Hl1Z7EC2Bzodf/MI2C\n\tfYktp54qWQ+2QONQOJZ/YjtPhhaeO9NV5CKi77V7h1t1uMs9qLcskWu5cvX24R4XyA\n\tOarT2xYuVblGQ6xfcYwOSzJSue7YpPEvojn1wQE5JqKplfuT5R175GfLKafcrpIWLW\n\t47B3s6+0hRB3Q==","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=7Q84PCC9pkw+3QMiA7QT53d5ljXcsB2XljmsBOFtWBE=;\n\tb=WQh+nlMATyDNRUMg9D4bJOrEzimxJ05DbkywtMev7/6hGrZy+dcU4YljrEK1aitt8u\n\tjgqgiGKJuiZi2FvZuhBc9xQ8HYGEfYsVBlbEYM1WwsOe7s5nr3TWIKyPGVilKaE2UwoZ\n\tJ3jKNEJZSgkA2q27h9dALRlEtGSHO/x3NAcqM3KMB3JjXoPpBF+Kt1uNW3XG/PzQeMDa\n\tKAqmHeI/zJXpnfEwcJT/xZnQI3P+fNLuOSt5QkOY+JRmDDnEhv6q4d9KWppbLjcNrXy3\n\tb2rmjemD0IfdfDUM90tOdA8ZdmmlDCYXprVTLRBtY2008YNl0356uEnuLA2QGkXHQr18\n\tKEBg=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"WQh+nlMA\"; 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=7Q84PCC9pkw+3QMiA7QT53d5ljXcsB2XljmsBOFtWBE=;\n\tb=CamPxyju8U+fPfF+QLHIFMwGAvnWwBuZ4wMEo3ZdYXLKKXlVTNXBP3BhH6rUk2tfOr\n\te+bXXSSNi6qt4l2HjYN+RuSeFeoWTQNN7F1xD1BnYuAzeh0clxmWNxB7Zja+8deAJ+kv\n\tr98/8N3V8YEZe/dL8v/j89BwTiTGAJy2+Wba8HOztp/6mcmNH2AszXhkMYjhu5RY+htH\n\tAmc4m0zRdbzzC+2RIj6DBJSFfQ3Nmg4kIqDXcwTIvnvvVzPrkT1Asdm2erl73nkebO2Y\n\ticWk1uGDY4Yvh186SACdiWkFWOi6LsggRY2EDBh9ZCFfRcxMmIVFPn1RLVQ4EMIrMlQF\n\tHduA==","X-Gm-Message-State":"AOAM5330lPiR18m3MJ0aLvGlqQH54owkaesnFCLy+S5f6j9y41RDPlqx\n\tQwrWOJ/KLVYef+p0PBJqlKcnsttp9RPyEEgU9rgZPw==","X-Google-Smtp-Source":"ABdhPJziuojhlBa0evSn3O/0s/xIBDdowz2veMlPKsP442UL+kIfN6Rp6kPkwLtEOYKJasxotAxOksrxcH2zWw0XT8s=","X-Received":"by 2002:a05:6512:12c5:b0:479:28ec:c04a with SMTP id\n\tp5-20020a05651212c500b0047928ecc04amr19335065lfg.510.1654855361603;\n\tFri, 10 Jun 2022 03:02:41 -0700 (PDT)","MIME-Version":"1.0","References":"<20220609125830.4325-1-naush@raspberrypi.com>\n\t<YqIa+LPXL1zapH/g@pendragon.ideasonboard.com>\n\t<CAHW6GYLFSXeddC5P4b0UjYRZN87Oioj11EHc_uLUUz=tCrXAww@mail.gmail.com>\n\t<CAEmqJPr4c4ZRAGh=i6ym0=KZDV2YGaf2LAM5--b=ux5yHTWVug@mail.gmail.com>","In-Reply-To":"<CAEmqJPr4c4ZRAGh=i6ym0=KZDV2YGaf2LAM5--b=ux5yHTWVug@mail.gmail.com>","Date":"Fri, 10 Jun 2022 11:02:26 +0100","Message-ID":"<CAEmqJPo9_+LTvgfc35bjm_sK=-WvkEQT6aNSNGwoFbODRhGQPw@mail.gmail.com>","To":"David Plowman <david.plowman@raspberrypi.com>","Content-Type":"multipart/alternative; boundary=\"000000000000fbc11005e11509d6\"","Subject":"Re: [libcamera-devel] [PATCH] pipeline: ipa: raspberrypi: Correctly\n\treport available controls","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Naushir Patuck via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Naushir Patuck <naush@raspberrypi.com>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]