| Message ID | 20250616084733.18707-6-mzamazal@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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; >
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; >>
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; > >> >
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
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;
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(-)