[{"id":21982,"web_url":"https://patchwork.libcamera.org/comment/21982/","msgid":"<164167845677.1917876.5006116835009050963@Monstersaurus>","date":"2022-01-08T21:47:36","subject":"Re: [libcamera-devel] Fix build with LTO enabled","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Victor,\n\nQuoting victor@westerhu.is (2022-01-07 20:51:05)\n> From 4f419bd0310462616a107c89510a4864a3b8db31 Mon Sep 17 00:00:00 2001\n> From: Victor Westerhuis <victor@westerhu.is>\n> Date: Fri, 7 Jan 2022 17:10:53 +0100\n> Subject: [PATCH] Fix build with LTO enabled\n> \n> libcamera::RPi::Controls in raspberrypi.h depends on\n> libcamera::controls::controls in control_ids.cpp, instantiated\n> from control_ids.cpp.in.\n> \n> The order of initialization is not defined between these two\n> namespace scope objects. This patch changes RPi::Controls to a\n> function-level static, initialized on first use and therefore\n> safe to use.\n> \n> This leads to warnings about getControls not being used in\n> src/ipa/raspberrypi/raspberrypi.cpp and\n> src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> (through src/libcamera/pipeline/raspberrypi/rpi_stream.h).\n> This patch therefore drops the include without ill effects.\n> \n> Signed-off-by: Victor Westerhuis <victor@westerhu.is>\n> ---\n> This patch fixes https://bugs.libcamera.org/show_bug.cgi?id=83\n> \n> Perhaps this data should not be in a header at all?\n\nLooking at this patch, it certainly looks like this should be moved to\nthe .cpp file, and doesn't need to be in the .h file.\n\nNaush, what do you think?\n\nMaybe this set of controls should be part of the class too. Would that\nhelp guarantee the order of construction?\n\n> \n> Please CC me when responding, since I'm not subscribed to this mailing\n> list.\n> \n>  include/libcamera/ipa/raspberrypi.h           | 42 ++++++++++---------\n>  src/ipa/raspberrypi/raspberrypi.cpp           |  1 -\n>  .../pipeline/raspberrypi/raspberrypi.cpp      |  2 +-\n>  .../pipeline/raspberrypi/rpi_stream.h         |  1 -\n>  4 files changed, 24 insertions(+), 22 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> index 7f705e49..545df355 100644\n> --- a/include/libcamera/ipa/raspberrypi.h\n> +++ b/include/libcamera/ipa/raspberrypi.h\n> @@ -27,26 +27,30 @@ namespace RPi {\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(0.0f, 16.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) },\n> -               { &controls::Contrast, ControlInfo(0.0f, 32.0f) },\n> -               { &controls::Saturation, ControlInfo(0.0f, 32.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> +static const ControlInfoMap &getControls()\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(0.0f, 16.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) },\n> +                       { &controls::Contrast, ControlInfo(0.0f, 32.0f) },\n> +                       { &controls::Saturation, ControlInfo(0.0f, 32.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> +       return controls;\n> +}\n>  \n>  } /* namespace RPi */\n>  \n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 0ed41385..b2717dfc 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> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 168bbcef..a48f1130 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -1244,7 +1244,7 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n>         data->sensorMetadata_ = sensorConfig.sensorMetadata;\n>  \n>         /* Register the controls that the Raspberry Pi IPA can handle. */\n> -       data->controlInfo_ = RPi::Controls;\n> +       data->controlInfo_ = RPi::getControls();\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 d6f49d34..b0fc1119 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> 2.34.1\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 9FC87BF415\n\tfor <parsemail@patchwork.libcamera.org>;\n\tSat,  8 Jan 2022 21:47:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DA4346093B;\n\tSat,  8 Jan 2022 22:47:40 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BAA5E604F8\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSat,  8 Jan 2022 22:47:39 +0100 (CET)","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 658C48A1;\n\tSat,  8 Jan 2022 22:47:39 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"nbM7mtiY\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1641678459;\n\tbh=0TweubA6m4sGG2t/zNbVzwT73Vlvutl90rmkuKgbbQU=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=nbM7mtiYitkxHorjuusiPUi5V9/hazEs8PL2e1KAiIH+fEfvl3j2QyhYEhPk++7oq\n\tPfWlvWrb/MOtDav1OUSRHhqFTJQKhVnmxFXHST2KRg/MWpGmdT35PK8oU/ZeWit83r\n\tldsni8pY0OGwSde9lE2hHQ0RmLe/i7lXPlZpj7Qw=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<E1n5wCX-0004MQ-3t@mail-02.1984.is>","References":"<E1n5wCX-0004MQ-3t@mail-02.1984.is>","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","To":"victor@westerhu.is, Naushir Patuck <naush@raspberrypi.com>,\n\tlibcamera devel <libcamera-devel@lists.libcamera.org>, ","Date":"Sat, 08 Jan 2022 21:47:36 +0000","Message-ID":"<164167845677.1917876.5006116835009050963@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] Fix build with LTO enabled","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>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":21983,"web_url":"https://patchwork.libcamera.org/comment/21983/","msgid":"<YdoN7CBI129nmoY0@pendragon.ideasonboard.com>","date":"2022-01-08T22:19:24","subject":"Re: [libcamera-devel] Fix build with LTO enabled","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Sat, Jan 08, 2022 at 09:47:36PM +0000, Kieran Bingham wrote:\n> Quoting victor@westerhu.is (2022-01-07 20:51:05)\n> > From 4f419bd0310462616a107c89510a4864a3b8db31 Mon Sep 17 00:00:00 2001\n> > From: Victor Westerhuis <victor@westerhu.is>\n> > Date: Fri, 7 Jan 2022 17:10:53 +0100\n> > Subject: [PATCH] Fix build with LTO enabled\n> > \n> > libcamera::RPi::Controls in raspberrypi.h depends on\n> > libcamera::controls::controls in control_ids.cpp, instantiated\n> > from control_ids.cpp.in.\n> > \n> > The order of initialization is not defined between these two\n> > namespace scope objects. This patch changes RPi::Controls to a\n> > function-level static, initialized on first use and therefore\n> > safe to use.\n> > \n> > This leads to warnings about getControls not being used in\n> > src/ipa/raspberrypi/raspberrypi.cpp and\n> > src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > (through src/libcamera/pipeline/raspberrypi/rpi_stream.h).\n> > This patch therefore drops the include without ill effects.\n> > \n> > Signed-off-by: Victor Westerhuis <victor@westerhu.is>\n> > ---\n> > This patch fixes https://bugs.libcamera.org/show_bug.cgi?id=83\n> > \n> > Perhaps this data should not be in a header at all?\n> \n> Looking at this patch, it certainly looks like this should be moved to\n> the .cpp file, and doesn't need to be in the .h file.\n\nThe reason why the ControlInfoMap is defined in the header file is that\nit's used by the IPA module and the pipeline handler. If you moved it to\na .cpp file on one side, it wouldn't be accessible to the other side.\n\nI think the information should be store din the IPA module, and\ncommunicated to the pipeline handler at init time.\n\n> Naush, what do you think?\n> \n> Maybe this set of controls should be part of the class too. Would that\n> help guarantee the order of construction?\n> \n> > Please CC me when responding, since I'm not subscribed to this mailing\n> > list.\n> > \n> >  include/libcamera/ipa/raspberrypi.h           | 42 ++++++++++---------\n> >  src/ipa/raspberrypi/raspberrypi.cpp           |  1 -\n> >  .../pipeline/raspberrypi/raspberrypi.cpp      |  2 +-\n> >  .../pipeline/raspberrypi/rpi_stream.h         |  1 -\n> >  4 files changed, 24 insertions(+), 22 deletions(-)\n> > \n> > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> > index 7f705e49..545df355 100644\n> > --- a/include/libcamera/ipa/raspberrypi.h\n> > +++ b/include/libcamera/ipa/raspberrypi.h\n> > @@ -27,26 +27,30 @@ namespace RPi {\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(0.0f, 16.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) },\n> > -               { &controls::Contrast, ControlInfo(0.0f, 32.0f) },\n> > -               { &controls::Saturation, ControlInfo(0.0f, 32.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> > +static const ControlInfoMap &getControls()\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(0.0f, 16.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) },\n> > +                       { &controls::Contrast, ControlInfo(0.0f, 32.0f) },\n> > +                       { &controls::Saturation, ControlInfo(0.0f, 32.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> > +       return controls;\n> > +}\n> >  \n> >  } /* namespace RPi */\n> >  \n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index 0ed41385..b2717dfc 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> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 168bbcef..a48f1130 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -1244,7 +1244,7 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> >         data->sensorMetadata_ = sensorConfig.sensorMetadata;\n> >  \n> >         /* Register the controls that the Raspberry Pi IPA can handle. */\n> > -       data->controlInfo_ = RPi::Controls;\n> > +       data->controlInfo_ = RPi::getControls();\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 d6f49d34..b0fc1119 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 821D7BE080\n\tfor <parsemail@patchwork.libcamera.org>;\n\tSat,  8 Jan 2022 22:19:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E756960868;\n\tSat,  8 Jan 2022 23:19:35 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B413D604F8\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSat,  8 Jan 2022 23:19:33 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2B1838A1;\n\tSat,  8 Jan 2022 23:19:33 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"aVy3w7CH\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1641680373;\n\tbh=rKulbJt/uJzY9z4ePcOD3q+YX4FH9OQKX8Ka2GGF7eg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=aVy3w7CHqz9h5eWrcDy+quXWMLV7vFKzmKbxDk/h0UpV32c2tnqzgTmWtsoQKYS64\n\t7t0V0gbdh3JI2trfH0Wp3vmFQ5xTV4EqNly0STbTRo/l7ig6Hm8elrVb0E+TG7HRFM\n\tJN9LAJXw80NGiSFoJJ9apKZ3T28o8X2uMXkAresw=","Date":"Sun, 9 Jan 2022 00:19:24 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Message-ID":"<YdoN7CBI129nmoY0@pendragon.ideasonboard.com>","References":"<E1n5wCX-0004MQ-3t@mail-02.1984.is>\n\t<164167845677.1917876.5006116835009050963@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<164167845677.1917876.5006116835009050963@Monstersaurus>","Subject":"Re: [libcamera-devel] Fix build with LTO enabled","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>","Cc":"victor@westerhu.is, 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":21985,"web_url":"https://patchwork.libcamera.org/comment/21985/","msgid":"<fa2df1b5901d848d50373cbaee9177c9@westerhu.is>","date":"2022-01-09T00:52:53","subject":"Re: [libcamera-devel] Fix build with LTO enabled","submitter":{"id":112,"url":"https://patchwork.libcamera.org/api/people/112/","name":"Victor Westerhuis","email":"victor@westerhu.is"},"content":"Laurent Pinchart schreef op 08.01.2022 23:19:\n> Hello,\n> \n> On Sat, Jan 08, 2022 at 09:47:36PM +0000, Kieran Bingham wrote:\n>> Quoting victor@westerhu.is (2022-01-07 20:51:05)\n>> > From 4f419bd0310462616a107c89510a4864a3b8db31 Mon Sep 17 00:00:00 2001\n>> > From: Victor Westerhuis <victor@westerhu.is>\n>> > Date: Fri, 7 Jan 2022 17:10:53 +0100\n>> > Subject: [PATCH] Fix build with LTO enabled\n>> >\n>> > libcamera::RPi::Controls in raspberrypi.h depends on\n>> > libcamera::controls::controls in control_ids.cpp, instantiated\n>> > from control_ids.cpp.in.\n>> >\n>> > The order of initialization is not defined between these two\n>> > namespace scope objects. This patch changes RPi::Controls to a\n>> > function-level static, initialized on first use and therefore\n>> > safe to use.\n>> >\n>> > This leads to warnings about getControls not being used in\n>> > src/ipa/raspberrypi/raspberrypi.cpp and\n>> > src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n>> > (through src/libcamera/pipeline/raspberrypi/rpi_stream.h).\n>> > This patch therefore drops the include without ill effects.\n>> >\n>> > Signed-off-by: Victor Westerhuis <victor@westerhu.is>\n>> > ---\n>> > This patch fixes https://bugs.libcamera.org/show_bug.cgi?id=83\n>> >\n>> > Perhaps this data should not be in a header at all?\n>> \n>> Looking at this patch, it certainly looks like this should be moved to\n>> the .cpp file, and doesn't need to be in the .h file.\n> \n> The reason why the ControlInfoMap is defined in the header file is that\n> it's used by the IPA module and the pipeline handler. If you moved it \n> to\n> a .cpp file on one side, it wouldn't be accessible to the other side.\n> \nWhile writing my patch, I noticed that the data in the header is not \ncurrently referenced elsewhere. That's why after changing the variable \nto a getter function I had to remove the include from two other files, \nto prevent a warning about unused functions.\n> I think the information should be store din the IPA module, and\n> communicated to the pipeline handler at init time.\n> \n>> Naush, what do you think?\n>> \n>> Maybe this set of controls should be part of the class too. Would that\n>> help guarantee the order of construction?\n>> \n>> > Please CC me when responding, since I'm not subscribed to this mailing\n>> > list.\n>> >\n>> >  include/libcamera/ipa/raspberrypi.h           | 42 ++++++++++---------\n>> >  src/ipa/raspberrypi/raspberrypi.cpp           |  1 -\n>> >  .../pipeline/raspberrypi/raspberrypi.cpp      |  2 +-\n>> >  .../pipeline/raspberrypi/rpi_stream.h         |  1 -\n>> >  4 files changed, 24 insertions(+), 22 deletions(-)\n>> >\n>> > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n>> > index 7f705e49..545df355 100644\n>> > --- a/include/libcamera/ipa/raspberrypi.h\n>> > +++ b/include/libcamera/ipa/raspberrypi.h\n>> > @@ -27,26 +27,30 @@ namespace RPi {\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(0.0f, 16.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) },\n>> > -               { &controls::Contrast, ControlInfo(0.0f, 32.0f) },\n>> > -               { &controls::Saturation, ControlInfo(0.0f, 32.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>> > +static const ControlInfoMap &getControls()\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(0.0f, 16.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) },\n>> > +                       { &controls::Contrast, ControlInfo(0.0f, 32.0f) },\n>> > +                       { &controls::Saturation, ControlInfo(0.0f, 32.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>> > +       return controls;\n>> > +}\n>> >\n>> >  } /* namespace RPi */\n>> >\n>> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n>> > index 0ed41385..b2717dfc 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>> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>> > index 168bbcef..a48f1130 100644\n>> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>> > @@ -1244,7 +1244,7 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n>> >         data->sensorMetadata_ = sensorConfig.sensorMetadata;\n>> >\n>> >         /* Register the controls that the Raspberry Pi IPA can handle. */\n>> > -       data->controlInfo_ = RPi::Controls;\n>> > +       data->controlInfo_ = RPi::getControls();\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 d6f49d34..b0fc1119 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 F09E0BE080\n\tfor <parsemail@patchwork.libcamera.org>;\n\tSun,  9 Jan 2022 00:57:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5A14C60921;\n\tSun,  9 Jan 2022 01:57:41 +0100 (CET)","from mail-02.1984.is (mail-02.1984.is [185.112.145.70])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 006376017E\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun,  9 Jan 2022 01:57:39 +0100 (CET)","from localhost by mail-02.1984.is with esmtpsa\n\t(TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128)\n\t(Exim 4.92) (envelope-from <victor@westerhu.is>)\n\tid 1n6MS5-0002bi-Ab; Sun, 09 Jan 2022 00:52:53 +0000"],"MIME-Version":"1.0","Content-Type":"text/plain; charset=US-ASCII;\n format=flowed","Content-Transfer-Encoding":"7bit","Date":"Sun, 09 Jan 2022 01:52:53 +0100","From":"Victor Westerhuis <victor@westerhu.is>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","In-Reply-To":"<YdoN7CBI129nmoY0@pendragon.ideasonboard.com>","References":"<E1n5wCX-0004MQ-3t@mail-02.1984.is>\n\t<164167845677.1917876.5006116835009050963@Monstersaurus>\n\t<YdoN7CBI129nmoY0@pendragon.ideasonboard.com>","User-Agent":"Roundcube Webmail/1.4.0","Message-ID":"<fa2df1b5901d848d50373cbaee9177c9@westerhu.is>","X-Sender":"victor@westerhu.is","Subject":"Re: [libcamera-devel] Fix build with LTO enabled","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>","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":21988,"web_url":"https://patchwork.libcamera.org/comment/21988/","msgid":"<YdpJOUB5FgK/6bpM@pendragon.ideasonboard.com>","date":"2022-01-09T02:32:25","subject":"Re: [libcamera-devel] Fix build with LTO enabled","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Victor,\n\nOn Sun, Jan 09, 2022 at 01:52:53AM +0100, Victor Westerhuis wrote:\n> Laurent Pinchart schreef op 08.01.2022 23:19:\n> > On Sat, Jan 08, 2022 at 09:47:36PM +0000, Kieran Bingham wrote:\n> >> Quoting victor@westerhu.is (2022-01-07 20:51:05)\n> >> > From 4f419bd0310462616a107c89510a4864a3b8db31 Mon Sep 17 00:00:00 2001\n> >> > From: Victor Westerhuis <victor@westerhu.is>\n> >> > Date: Fri, 7 Jan 2022 17:10:53 +0100\n> >> > Subject: [PATCH] Fix build with LTO enabled\n> >> >\n> >> > libcamera::RPi::Controls in raspberrypi.h depends on\n> >> > libcamera::controls::controls in control_ids.cpp, instantiated\n> >> > from control_ids.cpp.in.\n> >> >\n> >> > The order of initialization is not defined between these two\n> >> > namespace scope objects. This patch changes RPi::Controls to a\n> >> > function-level static, initialized on first use and therefore\n> >> > safe to use.\n> >> >\n> >> > This leads to warnings about getControls not being used in\n> >> > src/ipa/raspberrypi/raspberrypi.cpp and\n> >> > src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> >> > (through src/libcamera/pipeline/raspberrypi/rpi_stream.h).\n> >> > This patch therefore drops the include without ill effects.\n> >> >\n> >> > Signed-off-by: Victor Westerhuis <victor@westerhu.is>\n> >> > ---\n> >> > This patch fixes https://bugs.libcamera.org/show_bug.cgi?id=83\n> >> >\n> >> > Perhaps this data should not be in a header at all?\n> >> \n> >> Looking at this patch, it certainly looks like this should be moved to\n> >> the .cpp file, and doesn't need to be in the .h file.\n> > \n> > The reason why the ControlInfoMap is defined in the header file is that\n> > it's used by the IPA module and the pipeline handler. If you moved it to\n> > a .cpp file on one side, it wouldn't be accessible to the other side.\n>\n> While writing my patch, I noticed that the data in the header is not \n> currently referenced elsewhere. That's why after changing the variable \n> to a getter function I had to remove the include from two other files, \n> to prevent a warning about unused functions.\n\nThat's a good point, but conceptually, this is data that is part of the\nIPA module, and is used by the pipeline handler. We could move it to the\npipeline handler implementation, but it means it would need to change\nwhen the IPA changes, which isn't right. Storing it in the IPA module\nand passing it to the pipeline handler at init time would be better.\n\nHow about the following (untested) patch ?\n\nFrom: Victor Westerhuis <victor@westerhu.is>\nDate: Sun, 9 Jan 2022 04:27:51 +0200\nSubject: [PATCH] ipa: raspberrypi: Fix build with LTO enabled\n\nlibcamera::RPi::Controls in raspberrypi.h depends on\nlibcamera::controls::controls in control_ids.cpp, instantiated from\ncontrol_ids.cpp.in.\n\nThe order of initialization is not defined between these two namespace\nscope objects, resulting in a runtime failure when compiling with LTO\nenabled.\n\nTo solve this, address the long-standing issue of the ControlInfoMap\nbeing defined in a shared header, by moving it to the Raspberry Pi IPA\nand passing it to the pipeline handler through the IPA init() function.\n\nBug: https://bugs.libcamera.org/show_bug.cgi?id=83\nSigned-off-by: Victor Westerhuis <victor@westerhu.is>\nSigned-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n---\n include/libcamera/ipa/raspberrypi.h           | 55 -------------------\n include/libcamera/ipa/raspberrypi.mojom       |  3 +-\n src/ipa/raspberrypi/raspberrypi.cpp           | 39 ++++++++++++-\n .../pipeline/raspberrypi/raspberrypi.cpp      | 15 +++--\n .../pipeline/raspberrypi/rpi_stream.h         |  1 -\n 5 files changed, 47 insertions(+), 66 deletions(-)\n delete mode 100644 include/libcamera/ipa/raspberrypi.h\n\ndiff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\ndeleted file mode 100644\nindex 7f705e49411d..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(0.0f, 16.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) },\n-\t\t{ &controls::Contrast, ControlInfo(0.0f, 32.0f) },\n-\t\t{ &controls::Saturation, ControlInfo(0.0f, 32.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__ */\ndiff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom\nindex acd3cafe6c91..275cf946ee9a 100644\n--- a/include/libcamera/ipa/raspberrypi.mojom\n+++ b/include/libcamera/ipa/raspberrypi.mojom\n@@ -45,7 +45,8 @@ struct StartConfig {\n \n interface IPARPiInterface {\n \tinit(libcamera.IPASettings settings)\n-\t\t=> (int32 ret, SensorConfig sensorConfig);\n+\t\t=> (int32 ret, SensorConfig sensorConfig,\n+\t\t    libcamera.ControlInfoMap controls);\n \tstart(libcamera.ControlList controls) => (StartConfig startConfig);\n \tstop();\n \ndiff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\nindex 677126a3a2b3..e919d91e4c33 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@@ -89,7 +88,8 @@ public:\n \t\t\tmunmap(lsTable_, ipa::RPi::MaxLsGridSize);\n \t}\n \n-\tint init(const IPASettings &settings, ipa::RPi::SensorConfig *sensorConfig) override;\n+\tint init(const IPASettings &settings, ipa::RPi::SensorConfig *sensorConfig,\n+\t\t ControlInfoMap *ipaControls) override;\n \tvoid start(const ControlList &controls, ipa::RPi::StartConfig *startConfig) override;\n \tvoid stop() override {}\n \n@@ -175,8 +175,39 @@ private:\n \tDuration maxFrameDuration_;\n };\n \n-int IPARPi::init(const IPASettings &settings, ipa::RPi::SensorConfig *sensorConfig)\n+int IPARPi::init(const IPASettings &settings, ipa::RPi::SensorConfig *sensorConfig,\n+\t\t ControlInfoMap *ipaControls)\n {\n+\t/*\n+\t * List of controls handled by the Raspberry Pi IPA\n+\t *\n+\t * \\todo This list will need to be built dynamically from the control\n+\t * algorithms loaded by the json file, once this is supported. At that\n+\t * point applications should check first whether a control is supported,\n+\t * and the pipeline handler may be reverted so that it aborts when an\n+\t * unsupported control is encountered.\n+\t */\n+\tstatic const ControlInfoMap rpiControls({\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(0.0f, 16.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) },\n+\t\t{ &controls::Contrast, ControlInfo(0.0f, 32.0f) },\n+\t\t{ &controls::Saturation, ControlInfo(0.0f, 32.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 \t/*\n \t * Load the \"helper\" for this sensor. This tells us all the device specific stuff\n \t * that the kernel driver doesn't. We only do this the first time; we don't need\n@@ -206,6 +237,8 @@ int IPARPi::init(const IPASettings &settings, ipa::RPi::SensorConfig *sensorConf\n \tcontroller_.Read(settings.configurationFile.c_str());\n \tcontroller_.Initialise();\n \n+\t*ipaControls = rpiControls;\n+\n \treturn 0;\n }\n \ndiff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\nindex 49d7ff23209f..c9bffd647068 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@@ -191,7 +190,8 @@ public:\n \n \tvoid frameStarted(uint32_t sequence);\n \n-\tint loadIPA(ipa::RPi::SensorConfig *sensorConfig);\n+\tint loadIPA(ipa::RPi::SensorConfig *sensorConfig,\n+\t\t    ControlInfoMap *ipaControls);\n \tint configureIPA(const CameraConfiguration *config);\n \n \tvoid enumerateVideoDevices(MediaLink *link);\n@@ -1199,7 +1199,9 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n \tdata->sensorFormats_ = populateSensorFormats(data->sensor_);\n \n \tipa::RPi::SensorConfig sensorConfig;\n-\tif (data->loadIPA(&sensorConfig)) {\n+\tControlInfoMap ipaControls;\n+\n+\tif (data->loadIPA(&sensorConfig, &ipaControls)) {\n \t\tLOG(RPI, Error) << \"Failed to load a suitable IPA library\";\n \t\treturn -EINVAL;\n \t}\n@@ -1250,7 +1252,7 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n \tdata->sensorMetadata_ = sensorConfig.sensorMetadata;\n \n \t/* Register the controls that the Raspberry Pi IPA can handle. */\n-\tdata->controlInfo_ = RPi::Controls;\n+\tdata->controlInfo_ = ipaControls;\n \t/* Initialize the camera properties. */\n \tdata->properties_ = data->sensor_->properties();\n \n@@ -1467,7 +1469,8 @@ 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::SensorConfig *sensorConfig,\n+\t\t\t   ControlInfoMap *ipaControls)\n {\n \tipa_ = IPAManager::createIPA<ipa::RPi::IPAProxyRPi>(pipe(), 1, 1);\n \n@@ -1493,7 +1496,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, sensorConfig, ipaControls);\n }\n \n int RPiCameraData::configureIPA(const CameraConfiguration *config)\ndiff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\nindex d6f49d34f8c8..b0fc1119aabb 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> > I think the information should be store din the IPA module, and\n> > communicated to the pipeline handler at init time.\n> > \n> >> Naush, what do you think?\n> >> \n> >> Maybe this set of controls should be part of the class too. Would that\n> >> help guarantee the order of construction?\n> >> \n> >> > Please CC me when responding, since I'm not subscribed to this mailing\n> >> > list.\n> >> >\n> >> >  include/libcamera/ipa/raspberrypi.h           | 42 ++++++++++---------\n> >> >  src/ipa/raspberrypi/raspberrypi.cpp           |  1 -\n> >> >  .../pipeline/raspberrypi/raspberrypi.cpp      |  2 +-\n> >> >  .../pipeline/raspberrypi/rpi_stream.h         |  1 -\n> >> >  4 files changed, 24 insertions(+), 22 deletions(-)\n> >> >\n> >> > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> >> > index 7f705e49..545df355 100644\n> >> > --- a/include/libcamera/ipa/raspberrypi.h\n> >> > +++ b/include/libcamera/ipa/raspberrypi.h\n> >> > @@ -27,26 +27,30 @@ namespace RPi {\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(0.0f, 16.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) },\n> >> > -               { &controls::Contrast, ControlInfo(0.0f, 32.0f) },\n> >> > -               { &controls::Saturation, ControlInfo(0.0f, 32.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> >> > +static const ControlInfoMap &getControls()\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(0.0f, 16.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) },\n> >> > +                       { &controls::Contrast, ControlInfo(0.0f, 32.0f) },\n> >> > +                       { &controls::Saturation, ControlInfo(0.0f, 32.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> >> > +       return controls;\n> >> > +}\n> >> >\n> >> >  } /* namespace RPi */\n> >> >\n> >> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> >> > index 0ed41385..b2717dfc 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> >> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >> > index 168bbcef..a48f1130 100644\n> >> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >> > @@ -1244,7 +1244,7 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> >> >         data->sensorMetadata_ = sensorConfig.sensorMetadata;\n> >> >\n> >> >         /* Register the controls that the Raspberry Pi IPA can handle. */\n> >> > -       data->controlInfo_ = RPi::Controls;\n> >> > +       data->controlInfo_ = RPi::getControls();\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 d6f49d34..b0fc1119 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 967BFBF415\n\tfor <parsemail@patchwork.libcamera.org>;\n\tSun,  9 Jan 2022 02:32:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B5D0E604F5;\n\tSun,  9 Jan 2022 03:32:36 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BDFF66017E\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun,  9 Jan 2022 03:32:34 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E079AA1B;\n\tSun,  9 Jan 2022 03:32:33 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"BBgsmICT\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1641695554;\n\tbh=w759bPzfAjiY/gmbMIT21/vVvrOJ6UfmzbU0E8NMk6I=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=BBgsmICTFd/EWVFwaeOm7sYUWP5a8+SLfI46unukZPNPJ3YnirzerNGZ9cBsTcCfK\n\tlj6y8yA8AKspQf/LC4PQTCFCqI4zo1r0Sd4YeWDcdQiqmP8H/xAwmAGam3w62pFoEP\n\tJyHNxK+6XA3eWAbkGsqn2fBvWQW6EQcwQ0AMiMNM=","Date":"Sun, 9 Jan 2022 04:32:25 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Victor Westerhuis <victor@westerhu.is>","Message-ID":"<YdpJOUB5FgK/6bpM@pendragon.ideasonboard.com>","References":"<E1n5wCX-0004MQ-3t@mail-02.1984.is>\n\t<164167845677.1917876.5006116835009050963@Monstersaurus>\n\t<YdoN7CBI129nmoY0@pendragon.ideasonboard.com>\n\t<fa2df1b5901d848d50373cbaee9177c9@westerhu.is>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<fa2df1b5901d848d50373cbaee9177c9@westerhu.is>","Subject":"Re: [libcamera-devel] Fix build with LTO enabled","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>","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":21990,"web_url":"https://patchwork.libcamera.org/comment/21990/","msgid":"<164180747049.10801.13118379938307030585@Monstersaurus>","date":"2022-01-10T09:37:50","subject":"Re: [libcamera-devel] Fix build with LTO enabled","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nQuoting Laurent Pinchart (2022-01-09 02:32:25)\n> Hi Victor,\n> \n> On Sun, Jan 09, 2022 at 01:52:53AM +0100, Victor Westerhuis wrote:\n> > Laurent Pinchart schreef op 08.01.2022 23:19:\n> > > On Sat, Jan 08, 2022 at 09:47:36PM +0000, Kieran Bingham wrote:\n> > >> Quoting victor@westerhu.is (2022-01-07 20:51:05)\n> > >> > From 4f419bd0310462616a107c89510a4864a3b8db31 Mon Sep 17 00:00:00 2001\n> > >> > From: Victor Westerhuis <victor@westerhu.is>\n> > >> > Date: Fri, 7 Jan 2022 17:10:53 +0100\n> > >> > Subject: [PATCH] Fix build with LTO enabled\n> > >> >\n> > >> > libcamera::RPi::Controls in raspberrypi.h depends on\n> > >> > libcamera::controls::controls in control_ids.cpp, instantiated\n> > >> > from control_ids.cpp.in.\n> > >> >\n> > >> > The order of initialization is not defined between these two\n> > >> > namespace scope objects. This patch changes RPi::Controls to a\n> > >> > function-level static, initialized on first use and therefore\n> > >> > safe to use.\n> > >> >\n> > >> > This leads to warnings about getControls not being used in\n> > >> > src/ipa/raspberrypi/raspberrypi.cpp and\n> > >> > src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > >> > (through src/libcamera/pipeline/raspberrypi/rpi_stream.h).\n> > >> > This patch therefore drops the include without ill effects.\n> > >> >\n> > >> > Signed-off-by: Victor Westerhuis <victor@westerhu.is>\n> > >> > ---\n> > >> > This patch fixes https://bugs.libcamera.org/show_bug.cgi?id=83\n> > >> >\n> > >> > Perhaps this data should not be in a header at all?\n> > >> \n> > >> Looking at this patch, it certainly looks like this should be moved to\n> > >> the .cpp file, and doesn't need to be in the .h file.\n> > > \n> > > The reason why the ControlInfoMap is defined in the header file is that\n> > > it's used by the IPA module and the pipeline handler. If you moved it to\n> > > a .cpp file on one side, it wouldn't be accessible to the other side.\n> >\n> > While writing my patch, I noticed that the data in the header is not \n> > currently referenced elsewhere. That's why after changing the variable \n> > to a getter function I had to remove the include from two other files, \n> > to prevent a warning about unused functions.\n> \n> That's a good point, but conceptually, this is data that is part of the\n> IPA module, and is used by the pipeline handler. We could move it to the\n> pipeline handler implementation, but it means it would need to change\n> when the IPA changes, which isn't right. Storing it in the IPA module\n> and passing it to the pipeline handler at init time would be better.\n> \n> How about the following (untested) patch ?\n> \n> From: Victor Westerhuis <victor@westerhu.is>\n> Date: Sun, 9 Jan 2022 04:27:51 +0200\n> Subject: [PATCH] ipa: raspberrypi: Fix build with LTO enabled\n> \n> libcamera::RPi::Controls in raspberrypi.h depends on\n> libcamera::controls::controls in control_ids.cpp, instantiated from\n> control_ids.cpp.in.\n> \n> The order of initialization is not defined between these two namespace\n> scope objects, resulting in a runtime failure when compiling with LTO\n> enabled.\n> \n> To solve this, address the long-standing issue of the ControlInfoMap\n> being defined in a shared header, by moving it to the Raspberry Pi IPA\n> and passing it to the pipeline handler through the IPA init() function.\n> \n> Bug: https://bugs.libcamera.org/show_bug.cgi?id=83\n> Signed-off-by: Victor Westerhuis <victor@westerhu.is>\n> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> ---\n>  include/libcamera/ipa/raspberrypi.h           | 55 -------------------\n>  include/libcamera/ipa/raspberrypi.mojom       |  3 +-\n>  src/ipa/raspberrypi/raspberrypi.cpp           | 39 ++++++++++++-\n>  .../pipeline/raspberrypi/raspberrypi.cpp      | 15 +++--\n>  .../pipeline/raspberrypi/rpi_stream.h         |  1 -\n>  5 files changed, 47 insertions(+), 66 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 7f705e49411d..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(0.0f, 16.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) },\n> -               { &controls::Contrast, ControlInfo(0.0f, 32.0f) },\n> -               { &controls::Saturation, ControlInfo(0.0f, 32.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 acd3cafe6c91..275cf946ee9a 100644\n> --- a/include/libcamera/ipa/raspberrypi.mojom\n> +++ b/include/libcamera/ipa/raspberrypi.mojom\n> @@ -45,7 +45,8 @@ struct StartConfig {\n>  \n>  interface IPARPiInterface {\n>         init(libcamera.IPASettings settings)\n> -               => (int32 ret, SensorConfig sensorConfig);\n> +               => (int32 ret, SensorConfig sensorConfig,\n> +                   libcamera.ControlInfoMap controls);\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 677126a3a2b3..e919d91e4c33 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> @@ -89,7 +88,8 @@ public:\n>                         munmap(lsTable_, ipa::RPi::MaxLsGridSize);\n>         }\n>  \n> -       int init(const IPASettings &settings, ipa::RPi::SensorConfig *sensorConfig) override;\n> +       int init(const IPASettings &settings, ipa::RPi::SensorConfig *sensorConfig,\n> +                ControlInfoMap *ipaControls) override;\n>         void start(const ControlList &controls, ipa::RPi::StartConfig *startConfig) override;\n>         void stop() override {}\n>  \n> @@ -175,8 +175,39 @@ private:\n>         Duration maxFrameDuration_;\n>  };\n>  \n> -int IPARPi::init(const IPASettings &settings, ipa::RPi::SensorConfig *sensorConfig)\n> +int IPARPi::init(const IPASettings &settings, ipa::RPi::SensorConfig *sensorConfig,\n> +                ControlInfoMap *ipaControls)\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 rpiControls({\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(0.0f, 16.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) },\n> +               { &controls::Contrast, ControlInfo(0.0f, 32.0f) },\n> +               { &controls::Saturation, ControlInfo(0.0f, 32.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\nIs the ScalerCrop an IPA related control?\n\n--\nKieran\n\n\n\n> +               { &controls::FrameDurationLimits, ControlInfo(INT64_C(1000), INT64_C(1000000000)) },\n> +               { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }\n> +       }, controls::controls);\n> +\n>         /*\n>          * Load the \"helper\" for this sensor. This tells us all the device specific stuff\n>          * that the kernel driver doesn't. We only do this the first time; we don't need\n> @@ -206,6 +237,8 @@ int IPARPi::init(const IPASettings &settings, ipa::RPi::SensorConfig *sensorConf\n>         controller_.Read(settings.configurationFile.c_str());\n>         controller_.Initialise();\n>  \n> +       *ipaControls = rpiControls;\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 49d7ff23209f..c9bffd647068 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> @@ -191,7 +190,8 @@ public:\n>  \n>         void frameStarted(uint32_t sequence);\n>  \n> -       int loadIPA(ipa::RPi::SensorConfig *sensorConfig);\n> +       int loadIPA(ipa::RPi::SensorConfig *sensorConfig,\n> +                   ControlInfoMap *ipaControls);\n>         int configureIPA(const CameraConfiguration *config);\n>  \n>         void enumerateVideoDevices(MediaLink *link);\n> @@ -1199,7 +1199,9 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n>         data->sensorFormats_ = populateSensorFormats(data->sensor_);\n>  \n>         ipa::RPi::SensorConfig sensorConfig;\n> -       if (data->loadIPA(&sensorConfig)) {\n> +       ControlInfoMap ipaControls;\n> +\n> +       if (data->loadIPA(&sensorConfig, &ipaControls)) {\n>                 LOG(RPI, Error) << \"Failed to load a suitable IPA library\";\n>                 return -EINVAL;\n>         }\n> @@ -1250,7 +1252,7 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n>         data->sensorMetadata_ = sensorConfig.sensorMetadata;\n>  \n>         /* Register the controls that the Raspberry Pi IPA can handle. */\n> -       data->controlInfo_ = RPi::Controls;\n> +       data->controlInfo_ = ipaControls;\n>         /* Initialize the camera properties. */\n>         data->properties_ = data->sensor_->properties();\n>  \n> @@ -1467,7 +1469,8 @@ 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::SensorConfig *sensorConfig,\n> +                          ControlInfoMap *ipaControls)\n>  {\n>         ipa_ = IPAManager::createIPA<ipa::RPi::IPAProxyRPi>(pipe(), 1, 1);\n>  \n> @@ -1493,7 +1496,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, sensorConfig, ipaControls);\n>  }\n>  \n>  int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> index d6f49d34f8c8..b0fc1119aabb 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> > > I think the information should be store din the IPA module, and\n> > > communicated to the pipeline handler at init time.\n> > > \n> > >> Naush, what do you think?\n> > >> \n> > >> Maybe this set of controls should be part of the class too. Would that\n> > >> help guarantee the order of construction?\n> > >> \n> > >> > Please CC me when responding, since I'm not subscribed to this mailing\n> > >> > list.\n> > >> >\n> > >> >  include/libcamera/ipa/raspberrypi.h           | 42 ++++++++++---------\n> > >> >  src/ipa/raspberrypi/raspberrypi.cpp           |  1 -\n> > >> >  .../pipeline/raspberrypi/raspberrypi.cpp      |  2 +-\n> > >> >  .../pipeline/raspberrypi/rpi_stream.h         |  1 -\n> > >> >  4 files changed, 24 insertions(+), 22 deletions(-)\n> > >> >\n> > >> > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> > >> > index 7f705e49..545df355 100644\n> > >> > --- a/include/libcamera/ipa/raspberrypi.h\n> > >> > +++ b/include/libcamera/ipa/raspberrypi.h\n> > >> > @@ -27,26 +27,30 @@ namespace RPi {\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(0.0f, 16.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) },\n> > >> > -               { &controls::Contrast, ControlInfo(0.0f, 32.0f) },\n> > >> > -               { &controls::Saturation, ControlInfo(0.0f, 32.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> > >> > +static const ControlInfoMap &getControls()\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(0.0f, 16.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) },\n> > >> > +                       { &controls::Contrast, ControlInfo(0.0f, 32.0f) },\n> > >> > +                       { &controls::Saturation, ControlInfo(0.0f, 32.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> > >> > +       return controls;\n> > >> > +}\n> > >> >\n> > >> >  } /* namespace RPi */\n> > >> >\n> > >> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > >> > index 0ed41385..b2717dfc 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> > >> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > >> > index 168bbcef..a48f1130 100644\n> > >> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > >> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > >> > @@ -1244,7 +1244,7 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> > >> >         data->sensorMetadata_ = sensorConfig.sensorMetadata;\n> > >> >\n> > >> >         /* Register the controls that the Raspberry Pi IPA can handle. */\n> > >> > -       data->controlInfo_ = RPi::Controls;\n> > >> > +       data->controlInfo_ = RPi::getControls();\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 d6f49d34..b0fc1119 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 76B24BF415\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 10 Jan 2022 09:37:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8C2A160935;\n\tMon, 10 Jan 2022 10:37:54 +0100 (CET)","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 3719C604F5\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 10 Jan 2022 10:37:53 +0100 (CET)","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 BDD04A50;\n\tMon, 10 Jan 2022 10:37:52 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"iNImzM1L\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1641807472;\n\tbh=HXopNuzfmIlK8c6CDVl2ISX4EwcKMdlbkl0+bZ53ZTc=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=iNImzM1L4yyHLWowWQ8kLheF0oTuHsFs/wA/ClYP7OIxXlNEtEXVX5Z4Ikw2IncQG\n\t29baTJSkzJVJxw67LVIl99sORuNl4zLNYQXrA5XCXhEPxuuVGhuUsZoJdNVKZau5Yq\n\tbq1oJf7R1nPOjAV8pVihkBEY92byakBn5og5ETH0=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<YdpJOUB5FgK/6bpM@pendragon.ideasonboard.com>","References":"<E1n5wCX-0004MQ-3t@mail-02.1984.is>\n\t<164167845677.1917876.5006116835009050963@Monstersaurus>\n\t<YdoN7CBI129nmoY0@pendragon.ideasonboard.com>\n\t<fa2df1b5901d848d50373cbaee9177c9@westerhu.is>\n\t<YdpJOUB5FgK/6bpM@pendragon.ideasonboard.com>","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>,\n\tVictor Westerhuis <victor@westerhu.is>","Date":"Mon, 10 Jan 2022 09:37:50 +0000","Message-ID":"<164180747049.10801.13118379938307030585@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] Fix build with LTO enabled","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>","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":21991,"web_url":"https://patchwork.libcamera.org/comment/21991/","msgid":"<CAEmqJPrGSAxrNCy+UcdpUfvzoxYsGERty=8QATxws+c1SGTw4w@mail.gmail.com>","date":"2022-01-10T10:10:00","subject":"Re: [libcamera-devel] Fix build with LTO enabled","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi everyone,\n\nVictor, thanks for the patch!\n\nOn Mon, 10 Jan 2022 at 09:37, Kieran Bingham <\nkieran.bingham@ideasonboard.com> wrote:\n\n> Hi Laurent,\n>\n> Quoting Laurent Pinchart (2022-01-09 02:32:25)\n> > Hi Victor,\n> >\n> > On Sun, Jan 09, 2022 at 01:52:53AM +0100, Victor Westerhuis wrote:\n> > > Laurent Pinchart schreef op 08.01.2022 23:19:\n> > > > On Sat, Jan 08, 2022 at 09:47:36PM +0000, Kieran Bingham wrote:\n> > > >> Quoting victor@westerhu.is (2022-01-07 20:51:05)\n> > > >> > From 4f419bd0310462616a107c89510a4864a3b8db31 Mon Sep 17 00:00:00\n> 2001\n> > > >> > From: Victor Westerhuis <victor@westerhu.is>\n> > > >> > Date: Fri, 7 Jan 2022 17:10:53 +0100\n> > > >> > Subject: [PATCH] Fix build with LTO enabled\n> > > >> >\n> > > >> > libcamera::RPi::Controls in raspberrypi.h depends on\n> > > >> > libcamera::controls::controls in control_ids.cpp, instantiated\n> > > >> > from control_ids.cpp.in.\n> > > >> >\n> > > >> > The order of initialization is not defined between these two\n> > > >> > namespace scope objects. This patch changes RPi::Controls to a\n> > > >> > function-level static, initialized on first use and therefore\n> > > >> > safe to use.\n> > > >> >\n> > > >> > This leads to warnings about getControls not being used in\n> > > >> > src/ipa/raspberrypi/raspberrypi.cpp and\n> > > >> > src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > > >> > (through src/libcamera/pipeline/raspberrypi/rpi_stream.h).\n> > > >> > This patch therefore drops the include without ill effects.\n> > > >> >\n> > > >> > Signed-off-by: Victor Westerhuis <victor@westerhu.is>\n> > > >> > ---\n> > > >> > This patch fixes https://bugs.libcamera.org/show_bug.cgi?id=83\n> > > >> >\n> > > >> > Perhaps this data should not be in a header at all?\n> > > >>\n> > > >> Looking at this patch, it certainly looks like this should be moved\n> to\n> > > >> the .cpp file, and doesn't need to be in the .h file.\n> > > >\n> > > > The reason why the ControlInfoMap is defined in the header file is\n> that\n> > > > it's used by the IPA module and the pipeline handler. If you moved\n> it to\n> > > > a .cpp file on one side, it wouldn't be accessible to the other side.\n> > >\n> > > While writing my patch, I noticed that the data in the header is not\n> > > currently referenced elsewhere. That's why after changing the variable\n> > > to a getter function I had to remove the include from two other files,\n> > > to prevent a warning about unused functions.\n> >\n> > That's a good point, but conceptually, this is data that is part of the\n> > IPA module, and is used by the pipeline handler. We could move it to the\n> > pipeline handler implementation, but it means it would need to change\n> > when the IPA changes, which isn't right. Storing it in the IPA module\n> > and passing it to the pipeline handler at init time would be better.\n> >\n> > How about the following (untested) patch ?\n> >\n> > From: Victor Westerhuis <victor@westerhu.is>\n> > Date: Sun, 9 Jan 2022 04:27:51 +0200\n> > Subject: [PATCH] ipa: raspberrypi: Fix build with LTO enabled\n> >\n> > libcamera::RPi::Controls in raspberrypi.h depends on\n> > libcamera::controls::controls in control_ids.cpp, instantiated from\n> > control_ids.cpp.in.\n> >\n> > The order of initialization is not defined between these two namespace\n> > scope objects, resulting in a runtime failure when compiling with LTO\n> > enabled.\n> >\n> > To solve this, address the long-standing issue of the ControlInfoMap\n> > being defined in a shared header, by moving it to the Raspberry Pi IPA\n> > and passing it to the pipeline handler through the IPA init() function.\n> >\n> > Bug: https://bugs.libcamera.org/show_bug.cgi?id=83\n> > Signed-off-by: Victor Westerhuis <victor@westerhu.is>\n> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> > ---\n> >  include/libcamera/ipa/raspberrypi.h           | 55 -------------------\n> >  include/libcamera/ipa/raspberrypi.mojom       |  3 +-\n> >  src/ipa/raspberrypi/raspberrypi.cpp           | 39 ++++++++++++-\n> >  .../pipeline/raspberrypi/raspberrypi.cpp      | 15 +++--\n> >  .../pipeline/raspberrypi/rpi_stream.h         |  1 -\n> >  5 files changed, 47 insertions(+), 66 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 7f705e49411d..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(0.0f, 16.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) },\n> > -               { &controls::Contrast, ControlInfo(0.0f, 32.0f) },\n> > -               { &controls::Saturation, ControlInfo(0.0f, 32.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 acd3cafe6c91..275cf946ee9a 100644\n> > --- a/include/libcamera/ipa/raspberrypi.mojom\n> > +++ b/include/libcamera/ipa/raspberrypi.mojom\n> > @@ -45,7 +45,8 @@ struct StartConfig {\n> >\n> >  interface IPARPiInterface {\n> >         init(libcamera.IPASettings settings)\n> > -               => (int32 ret, SensorConfig sensorConfig);\n> > +               => (int32 ret, SensorConfig sensorConfig,\n> > +                   libcamera.ControlInfoMap controls);\n> >         start(libcamera.ControlList controls) => (StartConfig\n> startConfig);\n> >         stop();\n> >\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index 677126a3a2b3..e919d91e4c33 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> > @@ -89,7 +88,8 @@ public:\n> >                         munmap(lsTable_, ipa::RPi::MaxLsGridSize);\n> >         }\n> >\n> > -       int init(const IPASettings &settings, ipa::RPi::SensorConfig\n> *sensorConfig) override;\n> > +       int init(const IPASettings &settings, ipa::RPi::SensorConfig\n> *sensorConfig,\n> > +                ControlInfoMap *ipaControls) override;\n> >         void start(const ControlList &controls, ipa::RPi::StartConfig\n> *startConfig) override;\n> >         void stop() override {}\n> >\n> > @@ -175,8 +175,39 @@ private:\n> >         Duration maxFrameDuration_;\n> >  };\n> >\n> > -int IPARPi::init(const IPASettings &settings, ipa::RPi::SensorConfig\n> *sensorConfig)\n> > +int IPARPi::init(const IPASettings &settings, ipa::RPi::SensorConfig\n> *sensorConfig,\n> > +                ControlInfoMap *ipaControls)\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\n> control\n> > +        * algorithms loaded by the json file, once this is supported.\n> 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\n> when an\n> > +        * unsupported control is encountered.\n> > +        */\n> > +       static const ControlInfoMap rpiControls({\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(0.0f, 16.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) },\n> > +               { &controls::Contrast, ControlInfo(0.0f, 32.0f) },\n> > +               { &controls::Saturation, ControlInfo(0.0f, 32.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>\n> Is the ScalerCrop an IPA related control?\n>\n\nScaler crop is to only control in this list that does not really belong in\nthe IPA. Perhaps\nthere needs to be 2 lists of controls, one for the IPA and one for the\npipeline handler?\nNot the biggest fan of this approach :(\n\nIf one list is acceptable, I am happy to remove it from the header and keep\nin a separate\ncpp file.\n\nThanks,\nNaush\n\n\n\n>\n> --\n> Kieran\n>\n>\n>\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> >         /*\n> >          * Load the \"helper\" for this sensor. This tells us all the\n> device specific stuff\n> >          * that the kernel driver doesn't. We only do this the first\n> time; we don't need\n> > @@ -206,6 +237,8 @@ int IPARPi::init(const IPASettings &settings,\n> ipa::RPi::SensorConfig *sensorConf\n> >         controller_.Read(settings.configurationFile.c_str());\n> >         controller_.Initialise();\n> >\n> > +       *ipaControls = rpiControls;\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 49d7ff23209f..c9bffd647068 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> > @@ -191,7 +190,8 @@ public:\n> >\n> >         void frameStarted(uint32_t sequence);\n> >\n> > -       int loadIPA(ipa::RPi::SensorConfig *sensorConfig);\n> > +       int loadIPA(ipa::RPi::SensorConfig *sensorConfig,\n> > +                   ControlInfoMap *ipaControls);\n> >         int configureIPA(const CameraConfiguration *config);\n> >\n> >         void enumerateVideoDevices(MediaLink *link);\n> > @@ -1199,7 +1199,9 @@ int PipelineHandlerRPi::registerCamera(MediaDevice\n> *unicam, MediaDevice *isp, Me\n> >         data->sensorFormats_ = populateSensorFormats(data->sensor_);\n> >\n> >         ipa::RPi::SensorConfig sensorConfig;\n> > -       if (data->loadIPA(&sensorConfig)) {\n> > +       ControlInfoMap ipaControls;\n> > +\n> > +       if (data->loadIPA(&sensorConfig, &ipaControls)) {\n> >                 LOG(RPI, Error) << \"Failed to load a suitable IPA\n> library\";\n> >                 return -EINVAL;\n> >         }\n> > @@ -1250,7 +1252,7 @@ int PipelineHandlerRPi::registerCamera(MediaDevice\n> *unicam, MediaDevice *isp, Me\n> >         data->sensorMetadata_ = sensorConfig.sensorMetadata;\n> >\n> >         /* Register the controls that the Raspberry Pi IPA can handle. */\n> > -       data->controlInfo_ = RPi::Controls;\n> > +       data->controlInfo_ = ipaControls;\n> >         /* Initialize the camera properties. */\n> >         data->properties_ = data->sensor_->properties();\n> >\n> > @@ -1467,7 +1469,8 @@ 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::SensorConfig *sensorConfig,\n> > +                          ControlInfoMap *ipaControls)\n> >  {\n> >         ipa_ = IPAManager::createIPA<ipa::RPi::IPAProxyRPi>(pipe(), 1,\n> 1);\n> >\n> > @@ -1493,7 +1496,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, sensorConfig, ipaControls);\n> >  }\n> >\n> >  int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> > index d6f49d34f8c8..b0fc1119aabb 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> > > > I think the information should be store din the IPA module, and\n> > > > communicated to the pipeline handler at init time.\n> > > >\n> > > >> Naush, what do you think?\n> > > >>\n> > > >> Maybe this set of controls should be part of the class too. Would\n> that\n> > > >> help guarantee the order of construction?\n> > > >>\n> > > >> > Please CC me when responding, since I'm not subscribed to this\n> mailing\n> > > >> > list.\n> > > >> >\n> > > >> >  include/libcamera/ipa/raspberrypi.h           | 42\n> ++++++++++---------\n> > > >> >  src/ipa/raspberrypi/raspberrypi.cpp           |  1 -\n> > > >> >  .../pipeline/raspberrypi/raspberrypi.cpp      |  2 +-\n> > > >> >  .../pipeline/raspberrypi/rpi_stream.h         |  1 -\n> > > >> >  4 files changed, 24 insertions(+), 22 deletions(-)\n> > > >> >\n> > > >> > diff --git a/include/libcamera/ipa/raspberrypi.h\n> b/include/libcamera/ipa/raspberrypi.h\n> > > >> > index 7f705e49..545df355 100644\n> > > >> > --- a/include/libcamera/ipa/raspberrypi.h\n> > > >> > +++ b/include/libcamera/ipa/raspberrypi.h\n> > > >> > @@ -27,26 +27,30 @@ namespace RPi {\n> > > >> >   * and the pipeline handler may be reverted so that it aborts\n> 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> },\n> > > >> > -               { &controls::AnalogueGain, ControlInfo(1.0f,\n> 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(0.0f,\n> 16.0f) },\n> > > >> > -               { &controls::AwbEnable, ControlInfo(false, true)\n> },\n> > > >> > -               { &controls::ColourGains, ControlInfo(0.0f,\n> 32.0f) },\n> > > >> > -               { &controls::AwbMode,\n> ControlInfo(controls::AwbModeValues) },\n> > > >> > -               { &controls::Brightness, ControlInfo(-1.0f, 1.0f)\n> },\n> > > >> > -               { &controls::Contrast, ControlInfo(0.0f, 32.0f) },\n> > > >> > -               { &controls::Saturation, ControlInfo(0.0f, 32.0f)\n> },\n> > > >> > -               { &controls::Sharpness, ControlInfo(0.0f, 16.0f,\n> 1.0f) },\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> > > >> > +static const ControlInfoMap &getControls()\n> > > >> > +{\n> > > >> > +       static const ControlInfoMap controls({\n> > > >> > +                       { &controls::AeEnable, ControlInfo(false,\n> true) },\n> > > >> > +                       { &controls::ExposureTime, ControlInfo(0,\n> 999999) },\n> > > >> > +                       { &controls::AnalogueGain,\n> 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,\n> ControlInfo(0.0f, 16.0f) },\n> > > >> > +                       { &controls::AwbEnable,\n> ControlInfo(false, true) },\n> > > >> > +                       { &controls::ColourGains,\n> ControlInfo(0.0f, 32.0f) },\n> > > >> > +                       { &controls::AwbMode,\n> ControlInfo(controls::AwbModeValues) },\n> > > >> > +                       { &controls::Brightness,\n> ControlInfo(-1.0f, 1.0f) },\n> > > >> > +                       { &controls::Contrast, ControlInfo(0.0f,\n> 32.0f) },\n> > > >> > +                       { &controls::Saturation,\n> ControlInfo(0.0f, 32.0f) },\n> > > >> > +                       { &controls::Sharpness, ControlInfo(0.0f,\n> 16.0f, 1.0f) },\n> > > >> > +                       { &controls::ColourCorrectionMatrix,\n> ControlInfo(-16.0f, 16.0f) },\n> > > >> > +                       { &controls::ScalerCrop,\n> ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535),\n> 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> > > >> > +       return controls;\n> > > >> > +}\n> > > >> >\n> > > >> >  } /* namespace RPi */\n> > > >> >\n> > > >> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > >> > index 0ed41385..b2717dfc 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> > > >> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > >> > index 168bbcef..a48f1130 100644\n> > > >> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > >> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > >> > @@ -1244,7 +1244,7 @@ int\n> PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> > > >> >         data->sensorMetadata_ = sensorConfig.sensorMetadata;\n> > > >> >\n> > > >> >         /* Register the controls that the Raspberry Pi IPA can\n> handle. */\n> > > >> > -       data->controlInfo_ = RPi::Controls;\n> > > >> > +       data->controlInfo_ = RPi::getControls();\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 d6f49d34..b0fc1119 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 7AA00BE080\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 10 Jan 2022 10:10:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BD51460868;\n\tMon, 10 Jan 2022 11:10:18 +0100 (CET)","from mail-lf1-x131.google.com (mail-lf1-x131.google.com\n\t[IPv6:2a00:1450:4864:20::131])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1007C604F5\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 10 Jan 2022 11:10:17 +0100 (CET)","by mail-lf1-x131.google.com with SMTP id k21so42517254lfu.0\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 10 Jan 2022 02:10:16 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"O4s+Rt+Q\"; dkim-atps=neutral","DKIM-Signature":"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=3LzmT7nj3woLiElBhN4ic/lcaxnCBx7kFSs8dSZjFKs=;\n\tb=O4s+Rt+QCTHFeij6qPd1JDQOF/mMOtHrtLrTdQ9uELKYMVF1xkMaiUk8JU0+2l+QOM\n\tR0EfRBESyU0QjX/eLFOgSW+GfjA1iWJOV7IOLW9lm2FX/QNggbajROjZ7i3TrCKLkTZF\n\tsrpzvzkIL4H1K8Lv/OUIa/d75Kx0hZzThStIUKIeRz9w1z+21qS5MHwZ2/hmjNyTleTN\n\tAaIidfQyQECEHrtScKcZzp9uQXY18WadvGHB41cf1UG/6VwVe04ssV/5Eydteylro3Vr\n\tt8X7+02kJOdL8Pce9xMMbi/o1SDjyt+k535gq12pmcRG15rHEtgjE67FL4NzbzOZRkO2\n\tkV0w==","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=3LzmT7nj3woLiElBhN4ic/lcaxnCBx7kFSs8dSZjFKs=;\n\tb=GYwN/UR+pYiDsvVbXT2aZxFCpuIhj6Kmi8AWsnpGFwMod9IZh3vpnst1hdA2ND2vJM\n\t7CA59EEUte2XPjPb4TQjcTdduhyhSCbIc3f5/fmxCvFuVwCIRFZPF0xMbfeMyJfUyxeE\n\tWbihnJ1klAnoYOMouubZb45Sk1FW5q07Zg1pZYcCDDD+jYmuF4eU8a9tpf84ianorpGe\n\tmUyDC6YN9JA29/xqrjZM9vpLmI9Pmjkhu3s/q7a557VDBpjieR8+GjRFPyL5CyfA4V82\n\t0KkjKe3I+laW1XVRhbNBE1wW/ZXBaLe3yE26vb+aaFCaZze2uHfAHkBxYwp/CBBDaRpT\n\tvZCA==","X-Gm-Message-State":"AOAM530pgEAWMbQiQFb+lsZgg80HUk8p9whlFOIKcbpthVIF1T07atHr\n\tU6rRwhQlUqNJUqPjyk4Ke/D6Zydaf0qlzeRCffqRbA==","X-Google-Smtp-Source":"ABdhPJzwKE7rKkC6uMxZQXoLdSSKFVg3p+YiLT4cfzuoG3SGcd3s5zMpzU0Wb+8CYZf49lcWaJGGZVDXYVAh14Cv5g0=","X-Received":"by 2002:a2e:9659:: with SMTP id\n\tz25mr59094563ljh.16.1641809416022; \n\tMon, 10 Jan 2022 02:10:16 -0800 (PST)","MIME-Version":"1.0","References":"<E1n5wCX-0004MQ-3t@mail-02.1984.is>\n\t<164167845677.1917876.5006116835009050963@Monstersaurus>\n\t<YdoN7CBI129nmoY0@pendragon.ideasonboard.com>\n\t<fa2df1b5901d848d50373cbaee9177c9@westerhu.is>\n\t<YdpJOUB5FgK/6bpM@pendragon.ideasonboard.com>\n\t<164180747049.10801.13118379938307030585@Monstersaurus>","In-Reply-To":"<164180747049.10801.13118379938307030585@Monstersaurus>","From":"Naushir Patuck <naush@raspberrypi.com>","Date":"Mon, 10 Jan 2022 10:10:00 +0000","Message-ID":"<CAEmqJPrGSAxrNCy+UcdpUfvzoxYsGERty=8QATxws+c1SGTw4w@mail.gmail.com>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Content-Type":"multipart/alternative; boundary=\"00000000000007fec405d5378bad\"","Subject":"Re: [libcamera-devel] Fix build with LTO enabled","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>","Cc":"Victor Westerhuis <victor@westerhu.is>,\n\tlibcamera devel <libcamera-devel@lists.libcamera.org>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":21992,"web_url":"https://patchwork.libcamera.org/comment/21992/","msgid":"<YdwTu4GYgSIPQQ/6@pendragon.ideasonboard.com>","date":"2022-01-10T11:08:43","subject":"Re: [libcamera-devel] Fix build with LTO enabled","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush, everyone,\n\nOn Mon, Jan 10, 2022 at 10:10:00AM +0000, Naushir Patuck wrote:\n> On Mon, 10 Jan 2022 at 09:37, Kieran Bingham wrote:\n> > Quoting Laurent Pinchart (2022-01-09 02:32:25)\n> > > On Sun, Jan 09, 2022 at 01:52:53AM +0100, Victor Westerhuis wrote:\n> > > > Laurent Pinchart schreef op 08.01.2022 23:19:\n> > > > > On Sat, Jan 08, 2022 at 09:47:36PM +0000, Kieran Bingham wrote:\n> > > > >> Quoting victor@westerhu.is (2022-01-07 20:51:05)\n> > > > >> > From 4f419bd0310462616a107c89510a4864a3b8db31 Mon Sep 17 00:00:00 2001\n> > > > >> > From: Victor Westerhuis <victor@westerhu.is>\n> > > > >> > Date: Fri, 7 Jan 2022 17:10:53 +0100\n> > > > >> > Subject: [PATCH] Fix build with LTO enabled\n> > > > >> >\n> > > > >> > libcamera::RPi::Controls in raspberrypi.h depends on\n> > > > >> > libcamera::controls::controls in control_ids.cpp, instantiated\n> > > > >> > from control_ids.cpp.in.\n> > > > >> >\n> > > > >> > The order of initialization is not defined between these two\n> > > > >> > namespace scope objects. This patch changes RPi::Controls to a\n> > > > >> > function-level static, initialized on first use and therefore\n> > > > >> > safe to use.\n> > > > >> >\n> > > > >> > This leads to warnings about getControls not being used in\n> > > > >> > src/ipa/raspberrypi/raspberrypi.cpp and\n> > > > >> > src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > > > >> > (through src/libcamera/pipeline/raspberrypi/rpi_stream.h).\n> > > > >> > This patch therefore drops the include without ill effects.\n> > > > >> >\n> > > > >> > Signed-off-by: Victor Westerhuis <victor@westerhu.is>\n> > > > >> > ---\n> > > > >> > This patch fixes https://bugs.libcamera.org/show_bug.cgi?id=83\n> > > > >> >\n> > > > >> > Perhaps this data should not be in a header at all?\n> > > > >>\n> > > > >> Looking at this patch, it certainly looks like this should be moved to\n> > > > >> the .cpp file, and doesn't need to be in the .h file.\n> > > > >\n> > > > > The reason why the ControlInfoMap is defined in the header file is that\n> > > > > it's used by the IPA module and the pipeline handler. If you moved it to\n> > > > > a .cpp file on one side, it wouldn't be accessible to the other side.\n> > > >\n> > > > While writing my patch, I noticed that the data in the header is not\n> > > > currently referenced elsewhere. That's why after changing the variable\n> > > > to a getter function I had to remove the include from two other files,\n> > > > to prevent a warning about unused functions.\n> > >\n> > > That's a good point, but conceptually, this is data that is part of the\n> > > IPA module, and is used by the pipeline handler. We could move it to the\n> > > pipeline handler implementation, but it means it would need to change\n> > > when the IPA changes, which isn't right. Storing it in the IPA module\n> > > and passing it to the pipeline handler at init time would be better.\n> > >\n> > > How about the following (untested) patch ?\n> > >\n> > > From: Victor Westerhuis <victor@westerhu.is>\n> > > Date: Sun, 9 Jan 2022 04:27:51 +0200\n> > > Subject: [PATCH] ipa: raspberrypi: Fix build with LTO enabled\n> > >\n> > > libcamera::RPi::Controls in raspberrypi.h depends on\n> > > libcamera::controls::controls in control_ids.cpp, instantiated from\n> > > control_ids.cpp.in.\n> > >\n> > > The order of initialization is not defined between these two namespace\n> > > scope objects, resulting in a runtime failure when compiling with LTO\n> > > enabled.\n> > >\n> > > To solve this, address the long-standing issue of the ControlInfoMap\n> > > being defined in a shared header, by moving it to the Raspberry Pi IPA\n> > > and passing it to the pipeline handler through the IPA init() function.\n> > >\n> > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=83\n> > > Signed-off-by: Victor Westerhuis <victor@westerhu.is>\n> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> > > ---\n> > >  include/libcamera/ipa/raspberrypi.h           | 55 -------------------\n> > >  include/libcamera/ipa/raspberrypi.mojom       |  3 +-\n> > >  src/ipa/raspberrypi/raspberrypi.cpp           | 39 ++++++++++++-\n> > >  .../pipeline/raspberrypi/raspberrypi.cpp      | 15 +++--\n> > >  .../pipeline/raspberrypi/rpi_stream.h         |  1 -\n> > >  5 files changed, 47 insertions(+), 66 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 7f705e49411d..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(0.0f, 16.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) },\n> > > -               { &controls::Contrast, ControlInfo(0.0f, 32.0f) },\n> > > -               { &controls::Saturation, ControlInfo(0.0f, 32.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 acd3cafe6c91..275cf946ee9a 100644\n> > > --- a/include/libcamera/ipa/raspberrypi.mojom\n> > > +++ b/include/libcamera/ipa/raspberrypi.mojom\n> > > @@ -45,7 +45,8 @@ struct StartConfig {\n> > >\n> > >  interface IPARPiInterface {\n> > >         init(libcamera.IPASettings settings)\n> > > -               => (int32 ret, SensorConfig sensorConfig);\n> > > +               => (int32 ret, SensorConfig sensorConfig,\n> > > +                   libcamera.ControlInfoMap controls);\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 677126a3a2b3..e919d91e4c33 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> > > @@ -89,7 +88,8 @@ public:\n> > >                         munmap(lsTable_, ipa::RPi::MaxLsGridSize);\n> > >         }\n> > >\n> > > -       int init(const IPASettings &settings, ipa::RPi::SensorConfig *sensorConfig) override;\n> > > +       int init(const IPASettings &settings, ipa::RPi::SensorConfig *sensorConfig,\n> > > +                ControlInfoMap *ipaControls) override;\n> > >         void start(const ControlList &controls, ipa::RPi::StartConfig *startConfig) override;\n> > >         void stop() override {}\n> > >\n> > > @@ -175,8 +175,39 @@ private:\n> > >         Duration maxFrameDuration_;\n> > >  };\n> > >\n> > > -int IPARPi::init(const IPASettings &settings, ipa::RPi::SensorConfig *sensorConfig)\n> > > +int IPARPi::init(const IPASettings &settings, ipa::RPi::SensorConfig *sensorConfig,\n> > > +                ControlInfoMap *ipaControls)\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 rpiControls({\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(0.0f, 16.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) },\n> > > +               { &controls::Contrast, ControlInfo(0.0f, 32.0f) },\n> > > +               { &controls::Saturation, ControlInfo(0.0f, 32.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> > Is the ScalerCrop an IPA related control?\n\nGood point.\n\n> Scaler crop is to only control in this list that does not really belong in the IPA. Perhaps\n> there needs to be 2 lists of controls, one for the IPA and one for the pipeline handler?\n> Not the biggest fan of this approach :(\n\nI think we should have two indeed. In the general case, the pipeline\nhandler can control processing steps that the IPA isn't aware of, so it\nwould add controls to the ControlInfoMap, merging its own controls with\nthe one provided by the IPA.\n\nDoes anyone want to give it a try in a v2 of this patch ? :-)\n\n> If one list is acceptable, I am happy to remove it from the header and keep in a separate\n> cpp file.\n> \n> > > +               { &controls::FrameDurationLimits, ControlInfo(INT64_C(1000), INT64_C(1000000000)) },\n> > > +               { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }\n> > > +       }, controls::controls);\n> > > +\n> > >         /*\n> > >          * Load the \"helper\" for this sensor. This tells us all the device specific stuff\n> > >          * that the kernel driver doesn't. We only do this the first time; we don't need\n> > > @@ -206,6 +237,8 @@ int IPARPi::init(const IPASettings &settings, ipa::RPi::SensorConfig *sensorConf\n> > >         controller_.Read(settings.configurationFile.c_str());\n> > >         controller_.Initialise();\n> > >\n> > > +       *ipaControls = rpiControls;\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 49d7ff23209f..c9bffd647068 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> > > @@ -191,7 +190,8 @@ public:\n> > >\n> > >         void frameStarted(uint32_t sequence);\n> > >\n> > > -       int loadIPA(ipa::RPi::SensorConfig *sensorConfig);\n> > > +       int loadIPA(ipa::RPi::SensorConfig *sensorConfig,\n> > > +                   ControlInfoMap *ipaControls);\n> > >         int configureIPA(const CameraConfiguration *config);\n> > >\n> > >         void enumerateVideoDevices(MediaLink *link);\n> > > @@ -1199,7 +1199,9 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> > >         data->sensorFormats_ = populateSensorFormats(data->sensor_);\n> > >\n> > >         ipa::RPi::SensorConfig sensorConfig;\n> > > -       if (data->loadIPA(&sensorConfig)) {\n> > > +       ControlInfoMap ipaControls;\n> > > +\n> > > +       if (data->loadIPA(&sensorConfig, &ipaControls)) {\n> > >                 LOG(RPI, Error) << \"Failed to load a suitable IPA library\";\n> > >                 return -EINVAL;\n> > >         }\n> > > @@ -1250,7 +1252,7 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> > >         data->sensorMetadata_ = sensorConfig.sensorMetadata;\n> > >\n> > >         /* Register the controls that the Raspberry Pi IPA can handle. */\n> > > -       data->controlInfo_ = RPi::Controls;\n> > > +       data->controlInfo_ = ipaControls;\n> > >         /* Initialize the camera properties. */\n> > >         data->properties_ = data->sensor_->properties();\n> > >\n> > > @@ -1467,7 +1469,8 @@ 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::SensorConfig *sensorConfig,\n> > > +                          ControlInfoMap *ipaControls)\n> > >  {\n> > >         ipa_ = IPAManager::createIPA<ipa::RPi::IPAProxyRPi>(pipe(), 1, 1);\n> > >\n> > > @@ -1493,7 +1496,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, sensorConfig, ipaControls);\n> > >  }\n> > >\n> > >  int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> > > index d6f49d34f8c8..b0fc1119aabb 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> > > > > I think the information should be store din the IPA module, and\n> > > > > communicated to the pipeline handler at init time.\n> > > > >\n> > > > >> Naush, what do you think?\n> > > > >>\n> > > > >> Maybe this set of controls should be part of the class too. Would that\n> > > > >> help guarantee the order of construction?\n> > > > >>\n> > > > >> > Please CC me when responding, since I'm not subscribed to this mailing\n> > > > >> > list.\n> > > > >> >\n> > > > >> >  include/libcamera/ipa/raspberrypi.h           | 42 ++++++++++---------\n> > > > >> >  src/ipa/raspberrypi/raspberrypi.cpp           |  1 -\n> > > > >> >  .../pipeline/raspberrypi/raspberrypi.cpp      |  2 +-\n> > > > >> >  .../pipeline/raspberrypi/rpi_stream.h         |  1 -\n> > > > >> >  4 files changed, 24 insertions(+), 22 deletions(-)\n> > > > >> >\n> > > > >> > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> > > > >> > index 7f705e49..545df355 100644\n> > > > >> > --- a/include/libcamera/ipa/raspberrypi.h\n> > > > >> > +++ b/include/libcamera/ipa/raspberrypi.h\n> > > > >> > @@ -27,26 +27,30 @@ namespace RPi {\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(0.0f, 16.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) },\n> > > > >> > -               { &controls::Contrast, ControlInfo(0.0f, 32.0f) },\n> > > > >> > -               { &controls::Saturation, ControlInfo(0.0f, 32.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> > > > >> > +static const ControlInfoMap &getControls()\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(0.0f, 16.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) },\n> > > > >> > +                       { &controls::Contrast, ControlInfo(0.0f, 32.0f) },\n> > > > >> > +                       { &controls::Saturation, ControlInfo(0.0f, 32.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> > > > >> > +       return controls;\n> > > > >> > +}\n> > > > >> >\n> > > > >> >  } /* namespace RPi */\n> > > > >> >\n> > > > >> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > >> > index 0ed41385..b2717dfc 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> > > > >> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > >> > index 168bbcef..a48f1130 100644\n> > > > >> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > >> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > >> > @@ -1244,7 +1244,7 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me\n> > > > >> >         data->sensorMetadata_ = sensorConfig.sensorMetadata;\n> > > > >> >\n> > > > >> >         /* Register the controls that the Raspberry Pi IPA can handle. */\n> > > > >> > -       data->controlInfo_ = RPi::Controls;\n> > > > >> > +       data->controlInfo_ = RPi::getControls();\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 d6f49d34..b0fc1119 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 C0D34BE080\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 10 Jan 2022 11:08:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 180E760921;\n\tMon, 10 Jan 2022 12:08:54 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B60946021A\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 10 Jan 2022 12:08:52 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1A517A50;\n\tMon, 10 Jan 2022 12:08:52 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"tK5cC5SJ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1641812932;\n\tbh=giCC0iUoV686GjIUr/wB5VgOhitMMMc9a9yU1X5jf4M=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=tK5cC5SJ3C0eG/09Ct29Vw9xavXFgzKm3mflS9bjMQk69CmdLrRIV78guwiP1L0CN\n\t/uEL546AKBku76VDy3QRrhw8YAtHdwmymOP8comntztKelWQthBBrH4dZHm2VZ44Pr\n\tq0GgYaJrDIF53C5Qdysr26e5k9nNdWNHn2zW3JR0=","Date":"Mon, 10 Jan 2022 13:08:43 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Naushir Patuck <naush@raspberrypi.com>","Message-ID":"<YdwTu4GYgSIPQQ/6@pendragon.ideasonboard.com>","References":"<E1n5wCX-0004MQ-3t@mail-02.1984.is>\n\t<164167845677.1917876.5006116835009050963@Monstersaurus>\n\t<YdoN7CBI129nmoY0@pendragon.ideasonboard.com>\n\t<fa2df1b5901d848d50373cbaee9177c9@westerhu.is>\n\t<YdpJOUB5FgK/6bpM@pendragon.ideasonboard.com>\n\t<164180747049.10801.13118379938307030585@Monstersaurus>\n\t<CAEmqJPrGSAxrNCy+UcdpUfvzoxYsGERty=8QATxws+c1SGTw4w@mail.gmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<CAEmqJPrGSAxrNCy+UcdpUfvzoxYsGERty=8QATxws+c1SGTw4w@mail.gmail.com>","Subject":"Re: [libcamera-devel] Fix build with LTO enabled","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>","Cc":"Victor Westerhuis <victor@westerhu.is>,\n\tlibcamera devel <libcamera-devel@lists.libcamera.org>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]