[v10,05/13] config: Look up rpi config path in configuration file
diff mbox series

Message ID 20250616084733.18707-6-mzamazal@redhat.com
State Superseded
Headers show
Series
  • Add global configuration file
Related show

Commit Message

Milan Zamazal June 16, 2025, 8:47 a.m. UTC
Let's introduce a configuration option for rpi configuration file path.
It may be arguable whether pipeline specific configurations belong to
the global configuration file.  But:

- Having a single configuration file is generally easier for the user.
- The original configuration via environment variables can be already
  considered global.
- This option points to other configuration files and it makes little
  sense to add another configuration file to the chain.

The configuration snippet:

  configuration:
    pipeline:
      rpi:
        config_file: FILENAME

Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
---
 src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Laurent Pinchart June 16, 2025, 11:45 p.m. UTC | #1
Hi Milan,

Thank you for the patch.

On Mon, Jun 16, 2025 at 10:47:23AM +0200, Milan Zamazal wrote:
> Let's introduce a configuration option for rpi configuration file path.
> It may be arguable whether pipeline specific configurations belong to
> the global configuration file.  But:
> 
> - Having a single configuration file is generally easier for the user.
> - The original configuration via environment variables can be already
>   considered global.
> - This option points to other configuration files and it makes little
>   sense to add another configuration file to the chain.

I'm not sure to follow you here. With this patch we'll have two
configuration files, a global one, and one for the rpi pipeline handler.
Isn't that exactly "adding another configuration file to the chain" ?

Have you considered including the pipeline-specific configuration to the
global configuration file ? CC'ing Naush and David for feedback.

> The configuration snippet:
> 
>   configuration:
>     pipeline:
>       rpi:
>         config_file: FILENAME
> 
> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> ---
>  src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> index d8c7ca935..850a5e31b 100644
> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> @@ -20,6 +20,7 @@
>  #include <libcamera/property_ids.h>
>  
>  #include "libcamera/internal/camera_lens.h"
> +#include "libcamera/internal/global_configuration.h"
>  #include "libcamera/internal/ipa_manager.h"
>  #include "libcamera/internal/v4l2_subdevice.h"
>  
> @@ -1084,7 +1085,12 @@ int CameraData::loadPipelineConfiguration()
>  	/* Initial configuration of the platform, in case no config file is present */
>  	platformPipelineConfigure({});
>  
> -	char const *configFromEnv = utils::secure_getenv("LIBCAMERA_RPI_CONFIG_FILE");
> +	const GlobalConfiguration &config = pipe()->cameraManager()->_d()->configuration();
> +	std::optional<std::string> configFile =
> +		config.envOption("LIBCAMERA_RPI_CONFIG_FILE", "pipelines.rpi.config_file");
> +	if (!configFile.has_value())
> +		return 0;
> +	char const *configFromEnv = configFile.value().c_str();
>  	if (!configFromEnv || *configFromEnv == '\0')
>  		return 0;
>
Milan Zamazal June 20, 2025, 9:38 p.m. UTC | #2
Hi Laurent,

Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:

> Hi Milan,
>
> Thank you for the patch.
>
> On Mon, Jun 16, 2025 at 10:47:23AM +0200, Milan Zamazal wrote:
>> Let's introduce a configuration option for rpi configuration file path.
>> It may be arguable whether pipeline specific configurations belong to
>> the global configuration file.  But:
>> 
>> - Having a single configuration file is generally easier for the user.
>> - The original configuration via environment variables can be already
>>   considered global.
>> - This option points to other configuration files and it makes little
>>   sense to add another configuration file to the chain.
>
> I'm not sure to follow you here. With this patch we'll have two
> configuration files, a global one, and one for the rpi pipeline handler.
> Isn't that exactly "adding another configuration file to the chain" ?

It is.  IIRC my explanation above was sort of response to some offline
discussion regarding keeping or not keeping pipeline configurations
separate, while being conservative about contingent changes at the given moment.

> Have you considered including the pipeline-specific configuration to the
> global configuration file ? 

Reading my words above, I think so :-).  And "Allow enabling software
ISP in runtime" patch is such a case after all.

> CC'ing Naush and David for feedback.

Let's discuss what's preferable.

>> The configuration snippet:
>> 
>>   configuration:
>>     pipeline:
>>       rpi:
>>         config_file: FILENAME
>> 
>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
>> ---
>>  src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>> 
>> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
>> index d8c7ca935..850a5e31b 100644
>> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
>> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
>> @@ -20,6 +20,7 @@
>>  #include <libcamera/property_ids.h>
>>  
>>  #include "libcamera/internal/camera_lens.h"
>> +#include "libcamera/internal/global_configuration.h"
>>  #include "libcamera/internal/ipa_manager.h"
>>  #include "libcamera/internal/v4l2_subdevice.h"
>>  
>> @@ -1084,7 +1085,12 @@ int CameraData::loadPipelineConfiguration()
>>  	/* Initial configuration of the platform, in case no config file is present */
>>  	platformPipelineConfigure({});
>>  
>> -	char const *configFromEnv = utils::secure_getenv("LIBCAMERA_RPI_CONFIG_FILE");
>> +	const GlobalConfiguration &config = pipe()->cameraManager()->_d()->configuration();
>> +	std::optional<std::string> configFile =
>> +		config.envOption("LIBCAMERA_RPI_CONFIG_FILE", "pipelines.rpi.config_file");
>> +	if (!configFile.has_value())
>> +		return 0;
>> +	char const *configFromEnv = configFile.value().c_str();
>>  	if (!configFromEnv || *configFromEnv == '\0')
>>  		return 0;
>>
Naushir Patuck June 23, 2025, 8 a.m. UTC | #3
Hi all,

On Fri, 20 Jun 2025 at 22:38, Milan Zamazal <mzamazal@redhat.com> wrote:
>
> Hi Laurent,
>
> Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:
>
> > Hi Milan,
> >
> > Thank you for the patch.
> >
> > On Mon, Jun 16, 2025 at 10:47:23AM +0200, Milan Zamazal wrote:
> >> Let's introduce a configuration option for rpi configuration file path.
> >> It may be arguable whether pipeline specific configurations belong to
> >> the global configuration file.  But:
> >>
> >> - Having a single configuration file is generally easier for the user.
> >> - The original configuration via environment variables can be already
> >>   considered global.
> >> - This option points to other configuration files and it makes little
> >>   sense to add another configuration file to the chain.
> >
> > I'm not sure to follow you here. With this patch we'll have two
> > configuration files, a global one, and one for the rpi pipeline handler.
> > Isn't that exactly "adding another configuration file to the chain" ?
>
> It is.  IIRC my explanation above was sort of response to some offline
> discussion regarding keeping or not keeping pipeline configurations
> separate, while being conservative about contingent changes at the given moment.
>
> > Have you considered including the pipeline-specific configuration to the
> > global configuration file ?
>
> Reading my words above, I think so :-).  And "Allow enabling software
> ISP in runtime" patch is such a case after all.
>
> > CC'ing Naush and David for feedback.
>
> Let's discuss what's preferable.

I would be happy to add the RPi pipeline specific configuration
parameters directly to the global configuration file as an end goal.
If there needs to be an interim step where we add the rpi config file
to the global config file, that's ok with me.

Naush


>
> >> The configuration snippet:
> >>
> >>   configuration:
> >>     pipeline:
> >>       rpi:
> >>         config_file: FILENAME
> >>
> >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> >> ---
> >>  src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 8 +++++++-
> >>  1 file changed, 7 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> >> index d8c7ca935..850a5e31b 100644
> >> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> >> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> >> @@ -20,6 +20,7 @@
> >>  #include <libcamera/property_ids.h>
> >>
> >>  #include "libcamera/internal/camera_lens.h"
> >> +#include "libcamera/internal/global_configuration.h"
> >>  #include "libcamera/internal/ipa_manager.h"
> >>  #include "libcamera/internal/v4l2_subdevice.h"
> >>
> >> @@ -1084,7 +1085,12 @@ int CameraData::loadPipelineConfiguration()
> >>      /* Initial configuration of the platform, in case no config file is present */
> >>      platformPipelineConfigure({});
> >>
> >> -    char const *configFromEnv = utils::secure_getenv("LIBCAMERA_RPI_CONFIG_FILE");
> >> +    const GlobalConfiguration &config = pipe()->cameraManager()->_d()->configuration();
> >> +    std::optional<std::string> configFile =
> >> +            config.envOption("LIBCAMERA_RPI_CONFIG_FILE", "pipelines.rpi.config_file");
> >> +    if (!configFile.has_value())
> >> +            return 0;
> >> +    char const *configFromEnv = configFile.value().c_str();
> >>      if (!configFromEnv || *configFromEnv == '\0')
> >>              return 0;
> >>
>
Milan Zamazal June 24, 2025, 8:41 a.m. UTC | #4
Hi Naush,

Naushir Patuck <naush@raspberrypi.com> writes:

> I would be happy to add the RPi pipeline specific configuration
> parameters directly to the global configuration file as an end goal.
> If there needs to be an interim step where we add the rpi config file
> to the global config file, that's ok with me.

Thank you for your opinion.  Rather than making an interim step, I
put the rpi configuration to the global configuration file directly in
v11 (LIBCAMERA_RPI_CONFIG_FILE still works unchanged).  Not sure about
the exact structure there, but let's discuss that in review.

Regards,
Milan

Patch
diff mbox series

diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
index d8c7ca935..850a5e31b 100644
--- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
+++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
@@ -20,6 +20,7 @@ 
 #include <libcamera/property_ids.h>
 
 #include "libcamera/internal/camera_lens.h"
+#include "libcamera/internal/global_configuration.h"
 #include "libcamera/internal/ipa_manager.h"
 #include "libcamera/internal/v4l2_subdevice.h"
 
@@ -1084,7 +1085,12 @@  int CameraData::loadPipelineConfiguration()
 	/* Initial configuration of the platform, in case no config file is present */
 	platformPipelineConfigure({});
 
-	char const *configFromEnv = utils::secure_getenv("LIBCAMERA_RPI_CONFIG_FILE");
+	const GlobalConfiguration &config = pipe()->cameraManager()->_d()->configuration();
+	std::optional<std::string> configFile =
+		config.envOption("LIBCAMERA_RPI_CONFIG_FILE", "pipelines.rpi.config_file");
+	if (!configFile.has_value())
+		return 0;
+	char const *configFromEnv = configFile.value().c_str();
 	if (!configFromEnv || *configFromEnv == '\0')
 		return 0;