[4/4] ipa: rpi: controller: Ignore algorithms starting with disable
diff mbox series

Message ID 20251024144049.3311-5-david.plowman@raspberrypi.com
State Superseded
Headers show
Series
  • Raspberry Pi AWB using neural networks
Related show

Commit Message

David Plowman Oct. 24, 2025, 2:16 p.m. UTC 
From: Peter Bailey <peter.bailey@raspberrypi.com>

Prevent an algorithm starting with "disable" from being loaded.

Signed-off-by: Peter Bailey <peter.bailey@raspberrypi.com>
---
 src/ipa/rpi/controller/controller.cpp | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Naushir Patuck Dec. 10, 2025, 2:12 p.m. UTC | #1 
Hi David,

On Fri, 24 Oct 2025 at 15:41, David Plowman
<david.plowman@raspberrypi.com> wrote:
>
> From: Peter Bailey <peter.bailey@raspberrypi.com>
>
> Prevent an algorithm starting with "disable" from being loaded.
>
> Signed-off-by: Peter Bailey <peter.bailey@raspberrypi.com>
> ---
>  src/ipa/rpi/controller/controller.cpp | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/src/ipa/rpi/controller/controller.cpp b/src/ipa/rpi/controller/controller.cpp
> index df45dcd3..5eee0693 100644
> --- a/src/ipa/rpi/controller/controller.cpp
> +++ b/src/ipa/rpi/controller/controller.cpp
> @@ -145,6 +145,12 @@ int Controller::read(char const *filename)
>
>  int Controller::createAlgorithm(const std::string &name, const YamlObject &params)
>  {
> +       if (name.find("disable") == 0) {

Is it worth using name.find("blah") != std::string::npos here so we
can also do "rpi.nn_awb.disable"?

With or without that

Reviewed-by: Naushir Patuck <naush@raspberrypi.com>


> +               LOG(RPiController, Debug)
> +                       << "Algorithm \"" << name << "\" is disabled";
> +               return 0;
> +       }
> +
>         auto it = getAlgorithms().find(name);
>         if (it == getAlgorithms().end()) {
>                 LOG(RPiController, Warning)
> --
> 2.47.3
>
Barnabás Pőcze Dec. 10, 2025, 4:47 p.m. UTC | #2 
Hi

2025. 10. 24. 16:16 keltezéssel, David Plowman írta:
> From: Peter Bailey <peter.bailey@raspberrypi.com>
> 
> Prevent an algorithm starting with "disable" from being loaded.
> 
> Signed-off-by: Peter Bailey <peter.bailey@raspberrypi.com>
> ---

A similar feature has been introduced to libipa recentl; maybe it could
make sense to have them both work the same way: https://patchwork.libcamera.org/patch/25151/

The difference currently is that the libipa variant checks the "enabled"
algorithm parameter, not the name.


Regards,
Barnabás Pőcze

>   src/ipa/rpi/controller/controller.cpp | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/src/ipa/rpi/controller/controller.cpp b/src/ipa/rpi/controller/controller.cpp
> index df45dcd3..5eee0693 100644
> --- a/src/ipa/rpi/controller/controller.cpp
> +++ b/src/ipa/rpi/controller/controller.cpp
> @@ -145,6 +145,12 @@ int Controller::read(char const *filename)
>   
>   int Controller::createAlgorithm(const std::string &name, const YamlObject &params)
>   {
> +	if (name.find("disable") == 0) {
> +		LOG(RPiController, Debug)
> +			<< "Algorithm \"" << name << "\" is disabled";
> +		return 0;
> +	}
> +
>   	auto it = getAlgorithms().find(name);
>   	if (it == getAlgorithms().end()) {
>   		LOG(RPiController, Warning)
Naushir Patuck Dec. 11, 2025, 7:47 a.m. UTC | #3 
Hi Barnabás,

On Wed, 10 Dec 2025 at 16:47, Barnabás Pőcze
<barnabas.pocze@ideasonboard.com> wrote:
>
> Hi
>
> 2025. 10. 24. 16:16 keltezéssel, David Plowman írta:
> > From: Peter Bailey <peter.bailey@raspberrypi.com>
> >
> > Prevent an algorithm starting with "disable" from being loaded.
> >
> > Signed-off-by: Peter Bailey <peter.bailey@raspberrypi.com>
> > ---
>
> A similar feature has been introduced to libipa recentl; maybe it could
> make sense to have them both work the same way: https://patchwork.libcamera.org/patch/25151/
>
> The difference currently is that the libipa variant checks the "enabled"
> algorithm parameter, not the name.

This looks like a good solution, I'm ok to change this to match.
David, what do you think?

Naush

>
>
> Regards,
> Barnabás Pőcze
>
> >   src/ipa/rpi/controller/controller.cpp | 6 ++++++
> >   1 file changed, 6 insertions(+)
> >
> > diff --git a/src/ipa/rpi/controller/controller.cpp b/src/ipa/rpi/controller/controller.cpp
> > index df45dcd3..5eee0693 100644
> > --- a/src/ipa/rpi/controller/controller.cpp
> > +++ b/src/ipa/rpi/controller/controller.cpp
> > @@ -145,6 +145,12 @@ int Controller::read(char const *filename)
> >
> >   int Controller::createAlgorithm(const std::string &name, const YamlObject &params)
> >   {
> > +     if (name.find("disable") == 0) {
> > +             LOG(RPiController, Debug)
> > +                     << "Algorithm \"" << name << "\" is disabled";
> > +             return 0;
> > +     }
> > +
> >       auto it = getAlgorithms().find(name);
> >       if (it == getAlgorithms().end()) {
> >               LOG(RPiController, Warning)
>
David Plowman Dec. 11, 2025, 11:28 a.m. UTC | #4 
Hi Naush

Thanks for the review!

On Wed, 10 Dec 2025 at 14:13, Naushir Patuck <naush@raspberrypi.com> wrote:
>
> Hi David,
>
> On Fri, 24 Oct 2025 at 15:41, David Plowman
> <david.plowman@raspberrypi.com> wrote:
> >
> > From: Peter Bailey <peter.bailey@raspberrypi.com>
> >
> > Prevent an algorithm starting with "disable" from being loaded.
> >
> > Signed-off-by: Peter Bailey <peter.bailey@raspberrypi.com>
> > ---
> >  src/ipa/rpi/controller/controller.cpp | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/src/ipa/rpi/controller/controller.cpp b/src/ipa/rpi/controller/controller.cpp
> > index df45dcd3..5eee0693 100644
> > --- a/src/ipa/rpi/controller/controller.cpp
> > +++ b/src/ipa/rpi/controller/controller.cpp
> > @@ -145,6 +145,12 @@ int Controller::read(char const *filename)
> >
> >  int Controller::createAlgorithm(const std::string &name, const YamlObject &params)
> >  {
> > +       if (name.find("disable") == 0) {
>
> Is it worth using name.find("blah") != std::string::npos here so we
> can also do "rpi.nn_awb.disable"?

Yes, that's a good suggestion. However, I'm slightly inclined to stick
with our documented design where the final token in the string (so
after the last period) names the algorithm type (hence our code has
calls like "find_algorithm("awb")). So putting "disable" at the end
looks a bit like a "disable" algorithm. Conversely, adding tokens
before the algorithm type is already documented as the way to add
alternative versions of algorithms so it feels slightly less like an
abuse of the naming convention. Maybe? I agree it's all a bit
marginal, and overall I'm not too bothered...

(Of course, I haven't looked at Barnabas' suggestion yet, which may
change things again.)

Thanks
David

>
> With or without that
>
> Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
>
>
> > +               LOG(RPiController, Debug)
> > +                       << "Algorithm \"" << name << "\" is disabled";
> > +               return 0;
> > +       }
> > +
> >         auto it = getAlgorithms().find(name);
> >         if (it == getAlgorithms().end()) {
> >                 LOG(RPiController, Warning)
> > --
> > 2.47.3
> >

Patch
diff mbox series 

diff --git a/src/ipa/rpi/controller/controller.cpp b/src/ipa/rpi/controller/controller.cpp
index df45dcd3..5eee0693 100644
--- a/src/ipa/rpi/controller/controller.cpp
+++ b/src/ipa/rpi/controller/controller.cpp
@@ -145,6 +145,12 @@  int Controller::read(char const *filename)
 
 int Controller::createAlgorithm(const std::string &name, const YamlObject &params)
 {
+	if (name.find("disable") == 0) {
+		LOG(RPiController, Debug)
+			<< "Algorithm \"" << name << "\" is disabled";
+		return 0;
+	}
+
 	auto it = getAlgorithms().find(name);
 	if (it == getAlgorithms().end()) {
 		LOG(RPiController, Warning)