libcamera: pipeline: Move tuning file override handling to IPAProxy
diff mbox series

Message ID 20250123143818.29703-1-laurent.pinchart@ideasonboard.com
State Accepted
Headers show
Series
  • libcamera: pipeline: Move tuning file override handling to IPAProxy
Related show

Commit Message

Laurent Pinchart Jan. 23, 2025, 2:38 p.m. UTC 
The rkisp1 and rpi pipeline handlers duplicate code to handle the
LIBCAMERA_RKISP1_TUNING_FILE and LIBCAMERA_RPI_TUNING_FILE environment
variables that override tuning file selection. Move the common code to
IPAProxy::configurationFile() to avoid the duplication, and make the
feature available to all pipeline handlers with the same behaviour.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 Documentation/environment_variables.rst       |  4 +--
 src/libcamera/ipa_proxy.cpp                   | 27 +++++++++++++++----
 src/libcamera/pipeline/rkisp1/rkisp1.cpp      | 15 +++--------
 .../pipeline/rpi/common/pipeline_base.cpp     | 19 ++++---------
 4 files changed, 32 insertions(+), 33 deletions(-)


base-commit: fdc01dc3e0969423d65ee87f118164ec74373e5d
--
Regards,

Laurent Pinchart

Comments

Stefan Klug Jan. 24, 2025, 6:19 p.m. UTC | #1 
Hi Laurent,

Thank you for the patch.

On Thu, Jan 23, 2025 at 04:38:18PM +0200, Laurent Pinchart wrote:
> The rkisp1 and rpi pipeline handlers duplicate code to handle the
> LIBCAMERA_RKISP1_TUNING_FILE and LIBCAMERA_RPI_TUNING_FILE environment
> variables that override tuning file selection. Move the common code to
> IPAProxy::configurationFile() to avoid the duplication, and make the
> feature available to all pipeline handlers with the same behaviour.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  Documentation/environment_variables.rst       |  4 +--
>  src/libcamera/ipa_proxy.cpp                   | 27 +++++++++++++++----
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp      | 15 +++--------
>  .../pipeline/rpi/common/pipeline_base.cpp     | 19 ++++---------
>  4 files changed, 32 insertions(+), 33 deletions(-)
> 
> diff --git a/Documentation/environment_variables.rst b/Documentation/environment_variables.rst
> index 7da9883a8380..6f1235587a40 100644
> --- a/Documentation/environment_variables.rst
> +++ b/Documentation/environment_variables.rst
> @@ -57,8 +57,8 @@ LIBCAMERA_RPI_CONFIG_FILE
> 
>     Example value: ``/usr/local/share/libcamera/pipeline/rpi/vc4/minimal_mem.yaml``
> 
> -LIBCAMERA_RPI_TUNING_FILE
> -   Define a custom JSON tuning file to use in the Raspberry Pi.
> +LIBCAMERA_<NAME>_TUNING_FILE
> +   Define a custom IPA tuning file to use with the pipeline handler `NAME`.
> 
>     Example value: ``/usr/local/share/libcamera/ipa/rpi/vc4/custom_sensor.json``

Not directly related to this patch but anyways. This variable is quite
useful while developing, but fails if there are multiple sensors
attached to the device. Should we add a LIBCAMERA_<NAME>_TUNING_DIR to
add the ability to run multiple sensors with custom tuning files?

> 
> diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp
> index 85004737c171..25f772a41bf8 100644
> --- a/src/libcamera/ipa_proxy.cpp
> +++ b/src/libcamera/ipa_proxy.cpp
> @@ -98,16 +98,33 @@ IPAProxy::~IPAProxy()
>  std::string IPAProxy::configurationFile(const std::string &name,
>  					const std::string &fallbackName) const
>  {
> -	struct stat statbuf;
> -	int ret;
> -
>  	/*
>  	 * The IPA module name can be used as-is to build directory names as it
>  	 * has been validated when loading the module.
>  	 */
> -	std::string ipaName = ipam_->info().name;
> +	const std::string ipaName = ipam_->info().name;

Should this be a string_view?

> 
> -	/* Check the environment variable first. */
> +	/*
> +	 * Start with any user override through the module-specific environment
> +	 * variable. Use the name of the IPA module up to the first '/' to
> +	 * construct the variable name.
> +	 */
> +	std::string ipaEnvName = ipaName.substr(0, ipaName.find('/'));
> +	std::transform(ipaEnvName.begin(), ipaEnvName.end(), ipaEnvName.begin(),
> +		       [](unsigned char c) { return std::toupper(c); });

Sometimes c++ is really awful :-). Googling for this revealed

for (auto & c: ipaEnvName) c = std::toupper(c);

as short alternative.

> +	ipaEnvName = "LIBCAMERA_" + ipaEnvName + "_TUNING_FILE";
> +
> +	char const *configFromEnv = utils::secure_getenv(ipaEnvName.c_str());
> +	if (configFromEnv && *configFromEnv == '\0')
> +		return { configFromEnv };
> +
> +	struct stat statbuf;
> +	int ret;
> +
> +	/*
> +	 * Check the directory pointed to by the IPA config path environment
> +	 * variable next.
> +	 */
>  	const char *confPaths = utils::secure_getenv("LIBCAMERA_IPA_CONFIG_PATH");
>  	if (confPaths) {
>  		for (const auto &dir : utils::split(confPaths, ":")) {
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 35c793da9bba..1ac8d8ae7ed9 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -380,18 +380,9 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)
>  	ipa_->paramsComputed.connect(this, &RkISP1CameraData::paramsComputed);
>  	ipa_->metadataReady.connect(this, &RkISP1CameraData::metadataReady);
> 
> -	/*
> -	 * The API tuning file is made from the sensor name unless the
> -	 * environment variable overrides it.
> -	 */
> -	std::string ipaTuningFile;
> -	char const *configFromEnv = utils::secure_getenv("LIBCAMERA_RKISP1_TUNING_FILE");
> -	if (!configFromEnv || *configFromEnv == '\0') {
> -		ipaTuningFile =
> -			ipa_->configurationFile(sensor_->model() + ".yaml", "uncalibrated.yaml");
> -	} else {
> -		ipaTuningFile = std::string(configFromEnv);
> -	}
> +	/* The IPA tuning file is made from the sensor name. */
> +	std::string ipaTuningFile =
> +		ipa_->configurationFile(sensor_->model() + ".yaml", "uncalibrated.yaml");

Oh I like this file getting shorter.

> 
>  	IPACameraSensorInfo sensorInfo{};
>  	int ret = sensor_->sensorInfo(&sensorInfo);
> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> index 4b147fdb379a..1f13e5230fae 100644
> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> @@ -1156,20 +1156,11 @@ int CameraData::loadIPA(ipa::RPi::InitResult *result)
>  	if (!ipa_)
>  		return -ENOENT;
> 
> -	/*
> -	 * The configuration (tuning file) is made from the sensor name unless
> -	 * the environment variable overrides it.
> -	 */
> -	std::string configurationFile;
> -	char const *configFromEnv = utils::secure_getenv("LIBCAMERA_RPI_TUNING_FILE");
> -	if (!configFromEnv || *configFromEnv == '\0') {
> -		std::string model = sensor_->model();
> -		if (isMonoSensor(sensor_))
> -			model += "_mono";
> -		configurationFile = ipa_->configurationFile(model + ".json");
> -	} else {
> -		configurationFile = std::string(configFromEnv);
> -	}
> +	/* The configuration (tuning file) is made from the sensor name. */
> +	std::string model = sensor_->model();
> +	if (isMonoSensor(sensor_))
> +		model += "_mono";
> +	std::string configurationFile = ipa_->configurationFile(model + ".json");
> 
>  	IPASettings settings(configurationFile, sensor_->model());
>  	ipa::RPi::InitParams params;

The other comments were nits, so
Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> 

Cheers,
Stefan

> 
> base-commit: fdc01dc3e0969423d65ee87f118164ec74373e5d
> --
> Regards,
> 
> Laurent Pinchart
>
Laurent Pinchart Jan. 24, 2025, 8:33 p.m. UTC | #2 
Hi Stefan,

On Fri, Jan 24, 2025 at 07:19:47PM +0100, Stefan Klug wrote:
> On Thu, Jan 23, 2025 at 04:38:18PM +0200, Laurent Pinchart wrote:
> > The rkisp1 and rpi pipeline handlers duplicate code to handle the
> > LIBCAMERA_RKISP1_TUNING_FILE and LIBCAMERA_RPI_TUNING_FILE environment
> > variables that override tuning file selection. Move the common code to
> > IPAProxy::configurationFile() to avoid the duplication, and make the
> > feature available to all pipeline handlers with the same behaviour.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  Documentation/environment_variables.rst       |  4 +--
> >  src/libcamera/ipa_proxy.cpp                   | 27 +++++++++++++++----
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp      | 15 +++--------
> >  .../pipeline/rpi/common/pipeline_base.cpp     | 19 ++++---------
> >  4 files changed, 32 insertions(+), 33 deletions(-)
> > 
> > diff --git a/Documentation/environment_variables.rst b/Documentation/environment_variables.rst
> > index 7da9883a8380..6f1235587a40 100644
> > --- a/Documentation/environment_variables.rst
> > +++ b/Documentation/environment_variables.rst
> > @@ -57,8 +57,8 @@ LIBCAMERA_RPI_CONFIG_FILE
> > 
> >     Example value: ``/usr/local/share/libcamera/pipeline/rpi/vc4/minimal_mem.yaml``
> > 
> > -LIBCAMERA_RPI_TUNING_FILE
> > -   Define a custom JSON tuning file to use in the Raspberry Pi.
> > +LIBCAMERA_<NAME>_TUNING_FILE
> > +   Define a custom IPA tuning file to use with the pipeline handler `NAME`.
> > 
> >     Example value: ``/usr/local/share/libcamera/ipa/rpi/vc4/custom_sensor.json``
> 
> Not directly related to this patch but anyways. This variable is quite
> useful while developing, but fails if there are multiple sensors
> attached to the device. Should we add a LIBCAMERA_<NAME>_TUNING_DIR to
> add the ability to run multiple sensors with custom tuning files?

We could, but I think I would prefer exploring a configuration file
instead of environment variables.

> > diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp
> > index 85004737c171..25f772a41bf8 100644
> > --- a/src/libcamera/ipa_proxy.cpp
> > +++ b/src/libcamera/ipa_proxy.cpp
> > @@ -98,16 +98,33 @@ IPAProxy::~IPAProxy()
> >  std::string IPAProxy::configurationFile(const std::string &name,
> >  					const std::string &fallbackName) const
> >  {
> > -	struct stat statbuf;
> > -	int ret;
> > -
> >  	/*
> >  	 * The IPA module name can be used as-is to build directory names as it
> >  	 * has been validated when loading the module.
> >  	 */
> > -	std::string ipaName = ipam_->info().name;
> > +	const std::string ipaName = ipam_->info().name;
> 
> Should this be a string_view?

We can't, because the operators that concatenate a std::string and a
std::string_view have only been added in C++20 :-( See
https://en.cppreference.com/w/cpp/string/basic_string/operator%2B.
Workarounds would be possible by defining our own operators (I actually
have a patch that does just that), but that's out of scope for this
patch.

> > -	/* Check the environment variable first. */
> > +	/*
> > +	 * Start with any user override through the module-specific environment
> > +	 * variable. Use the name of the IPA module up to the first '/' to
> > +	 * construct the variable name.
> > +	 */
> > +	std::string ipaEnvName = ipaName.substr(0, ipaName.find('/'));
> > +	std::transform(ipaEnvName.begin(), ipaEnvName.end(), ipaEnvName.begin(),
> > +		       [](unsigned char c) { return std::toupper(c); });
> 
> Sometimes c++ is really awful :-). Googling for this revealed
> 
> for (auto & c: ipaEnvName) c = std::toupper(c);
> 
> as short alternative.

I kind of like std::transform(), but it may be a case of Stockholm
syndrome :-)

> > +	ipaEnvName = "LIBCAMERA_" + ipaEnvName + "_TUNING_FILE";
> > +
> > +	char const *configFromEnv = utils::secure_getenv(ipaEnvName.c_str());
> > +	if (configFromEnv && *configFromEnv == '\0')
> > +		return { configFromEnv };
> > +
> > +	struct stat statbuf;
> > +	int ret;
> > +
> > +	/*
> > +	 * Check the directory pointed to by the IPA config path environment
> > +	 * variable next.
> > +	 */
> >  	const char *confPaths = utils::secure_getenv("LIBCAMERA_IPA_CONFIG_PATH");
> >  	if (confPaths) {
> >  		for (const auto &dir : utils::split(confPaths, ":")) {
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 35c793da9bba..1ac8d8ae7ed9 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -380,18 +380,9 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)
> >  	ipa_->paramsComputed.connect(this, &RkISP1CameraData::paramsComputed);
> >  	ipa_->metadataReady.connect(this, &RkISP1CameraData::metadataReady);
> > 
> > -	/*
> > -	 * The API tuning file is made from the sensor name unless the
> > -	 * environment variable overrides it.
> > -	 */
> > -	std::string ipaTuningFile;
> > -	char const *configFromEnv = utils::secure_getenv("LIBCAMERA_RKISP1_TUNING_FILE");
> > -	if (!configFromEnv || *configFromEnv == '\0') {
> > -		ipaTuningFile =
> > -			ipa_->configurationFile(sensor_->model() + ".yaml", "uncalibrated.yaml");
> > -	} else {
> > -		ipaTuningFile = std::string(configFromEnv);
> > -	}
> > +	/* The IPA tuning file is made from the sensor name. */
> > +	std::string ipaTuningFile =
> > +		ipa_->configurationFile(sensor_->model() + ".yaml", "uncalibrated.yaml");
> 
> Oh I like this file getting shorter.
> 
> > 
> >  	IPACameraSensorInfo sensorInfo{};
> >  	int ret = sensor_->sensorInfo(&sensorInfo);
> > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > index 4b147fdb379a..1f13e5230fae 100644
> > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > @@ -1156,20 +1156,11 @@ int CameraData::loadIPA(ipa::RPi::InitResult *result)
> >  	if (!ipa_)
> >  		return -ENOENT;
> > 
> > -	/*
> > -	 * The configuration (tuning file) is made from the sensor name unless
> > -	 * the environment variable overrides it.
> > -	 */
> > -	std::string configurationFile;
> > -	char const *configFromEnv = utils::secure_getenv("LIBCAMERA_RPI_TUNING_FILE");
> > -	if (!configFromEnv || *configFromEnv == '\0') {
> > -		std::string model = sensor_->model();
> > -		if (isMonoSensor(sensor_))
> > -			model += "_mono";
> > -		configurationFile = ipa_->configurationFile(model + ".json");
> > -	} else {
> > -		configurationFile = std::string(configFromEnv);
> > -	}
> > +	/* The configuration (tuning file) is made from the sensor name. */
> > +	std::string model = sensor_->model();
> > +	if (isMonoSensor(sensor_))
> > +		model += "_mono";
> > +	std::string configurationFile = ipa_->configurationFile(model + ".json");
> > 
> >  	IPASettings settings(configurationFile, sensor_->model());
> >  	ipa::RPi::InitParams params;
> 
> The other comments were nits, so
> Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> 
> 
> > base-commit: fdc01dc3e0969423d65ee87f118164ec74373e5d
Kieran Bingham Jan. 25, 2025, 8:05 a.m. UTC | #3 
Quoting Laurent Pinchart (2025-01-24 20:33:17)
> Hi Stefan,
> 
> On Fri, Jan 24, 2025 at 07:19:47PM +0100, Stefan Klug wrote:
> > On Thu, Jan 23, 2025 at 04:38:18PM +0200, Laurent Pinchart wrote:
> > > The rkisp1 and rpi pipeline handlers duplicate code to handle the
> > > LIBCAMERA_RKISP1_TUNING_FILE and LIBCAMERA_RPI_TUNING_FILE environment
> > > variables that override tuning file selection. Move the common code to
> > > IPAProxy::configurationFile() to avoid the duplication, and make the
> > > feature available to all pipeline handlers with the same behaviour.
> > > 

Centralising all this was going to be my review comment to Stefan's
patch, 

> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > >  Documentation/environment_variables.rst       |  4 +--
> > >  src/libcamera/ipa_proxy.cpp                   | 27 +++++++++++++++----
> > >  src/libcamera/pipeline/rkisp1/rkisp1.cpp      | 15 +++--------
> > >  .../pipeline/rpi/common/pipeline_base.cpp     | 19 ++++---------
> > >  4 files changed, 32 insertions(+), 33 deletions(-)
> > > 
> > > diff --git a/Documentation/environment_variables.rst b/Documentation/environment_variables.rst
> > > index 7da9883a8380..6f1235587a40 100644
> > > --- a/Documentation/environment_variables.rst
> > > +++ b/Documentation/environment_variables.rst
> > > @@ -57,8 +57,8 @@ LIBCAMERA_RPI_CONFIG_FILE
> > > 
> > >     Example value: ``/usr/local/share/libcamera/pipeline/rpi/vc4/minimal_mem.yaml``
> > > 
> > > -LIBCAMERA_RPI_TUNING_FILE
> > > -   Define a custom JSON tuning file to use in the Raspberry Pi.
> > > +LIBCAMERA_<NAME>_TUNING_FILE
> > > +   Define a custom IPA tuning file to use with the pipeline handler `NAME`.
> > > 
> > >     Example value: ``/usr/local/share/libcamera/ipa/rpi/vc4/custom_sensor.json``
> > 
> > Not directly related to this patch but anyways. This variable is quite
> > useful while developing, but fails if there are multiple sensors
> > attached to the device. Should we add a LIBCAMERA_<NAME>_TUNING_DIR to
> > add the ability to run multiple sensors with custom tuning files?
> 
> We could, but I think I would prefer exploring a configuration file
> instead of environment variables.
> 
> > > diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp
> > > index 85004737c171..25f772a41bf8 100644
> > > --- a/src/libcamera/ipa_proxy.cpp
> > > +++ b/src/libcamera/ipa_proxy.cpp
> > > @@ -98,16 +98,33 @@ IPAProxy::~IPAProxy()
> > >  std::string IPAProxy::configurationFile(const std::string &name,
> > >                                     const std::string &fallbackName) const
> > >  {
> > > -   struct stat statbuf;
> > > -   int ret;
> > > -
> > >     /*
> > >      * The IPA module name can be used as-is to build directory names as it
> > >      * has been validated when loading the module.
> > >      */
> > > -   std::string ipaName = ipam_->info().name;
> > > +   const std::string ipaName = ipam_->info().name;
> > 
> > Should this be a string_view?
> 
> We can't, because the operators that concatenate a std::string and a
> std::string_view have only been added in C++20 :-( See
> https://en.cppreference.com/w/cpp/string/basic_string/operator%2B.
> Workarounds would be possible by defining our own operators (I actually
> have a patch that does just that), but that's out of scope for this
> patch.
> 
> > > -   /* Check the environment variable first. */
> > > +   /*
> > > +    * Start with any user override through the module-specific environment
> > > +    * variable. Use the name of the IPA module up to the first '/' to
> > > +    * construct the variable name.
> > > +    */
> > > +   std::string ipaEnvName = ipaName.substr(0, ipaName.find('/'));
> > > +   std::transform(ipaEnvName.begin(), ipaEnvName.end(), ipaEnvName.begin(),
> > > +                  [](unsigned char c) { return std::toupper(c); });
> > 
> > Sometimes c++ is really awful :-). Googling for this revealed
> > 
> > for (auto & c: ipaEnvName) c = std::toupper(c);
> > 
> > as short alternative.
> 
> I kind of like std::transform(), but it may be a case of Stockholm
> syndrome :-)

I find almost every use of std::transform in libcamera hard to read :_(

but whatever works,


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> 
> > > +   ipaEnvName = "LIBCAMERA_" + ipaEnvName + "_TUNING_FILE";
> > > +
> > > +   char const *configFromEnv = utils::secure_getenv(ipaEnvName.c_str());
> > > +   if (configFromEnv && *configFromEnv == '\0')
> > > +           return { configFromEnv };
> > > +
> > > +   struct stat statbuf;
> > > +   int ret;
> > > +
> > > +   /*
> > > +    * Check the directory pointed to by the IPA config path environment
> > > +    * variable next.
> > > +    */
> > >     const char *confPaths = utils::secure_getenv("LIBCAMERA_IPA_CONFIG_PATH");
> > >     if (confPaths) {
> > >             for (const auto &dir : utils::split(confPaths, ":")) {
> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > index 35c793da9bba..1ac8d8ae7ed9 100644
> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > @@ -380,18 +380,9 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)
> > >     ipa_->paramsComputed.connect(this, &RkISP1CameraData::paramsComputed);
> > >     ipa_->metadataReady.connect(this, &RkISP1CameraData::metadataReady);
> > > 
> > > -   /*
> > > -    * The API tuning file is made from the sensor name unless the
> > > -    * environment variable overrides it.
> > > -    */
> > > -   std::string ipaTuningFile;
> > > -   char const *configFromEnv = utils::secure_getenv("LIBCAMERA_RKISP1_TUNING_FILE");
> > > -   if (!configFromEnv || *configFromEnv == '\0') {
> > > -           ipaTuningFile =
> > > -                   ipa_->configurationFile(sensor_->model() + ".yaml", "uncalibrated.yaml");
> > > -   } else {
> > > -           ipaTuningFile = std::string(configFromEnv);
> > > -   }
> > > +   /* The IPA tuning file is made from the sensor name. */
> > > +   std::string ipaTuningFile =
> > > +           ipa_->configurationFile(sensor_->model() + ".yaml", "uncalibrated.yaml");
> > 
> > Oh I like this file getting shorter.
> > 
> > > 
> > >     IPACameraSensorInfo sensorInfo{};
> > >     int ret = sensor_->sensorInfo(&sensorInfo);
> > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > index 4b147fdb379a..1f13e5230fae 100644
> > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > @@ -1156,20 +1156,11 @@ int CameraData::loadIPA(ipa::RPi::InitResult *result)
> > >     if (!ipa_)
> > >             return -ENOENT;
> > > 
> > > -   /*
> > > -    * The configuration (tuning file) is made from the sensor name unless
> > > -    * the environment variable overrides it.
> > > -    */
> > > -   std::string configurationFile;
> > > -   char const *configFromEnv = utils::secure_getenv("LIBCAMERA_RPI_TUNING_FILE");
> > > -   if (!configFromEnv || *configFromEnv == '\0') {
> > > -           std::string model = sensor_->model();
> > > -           if (isMonoSensor(sensor_))
> > > -                   model += "_mono";
> > > -           configurationFile = ipa_->configurationFile(model + ".json");
> > > -   } else {
> > > -           configurationFile = std::string(configFromEnv);
> > > -   }
> > > +   /* The configuration (tuning file) is made from the sensor name. */
> > > +   std::string model = sensor_->model();
> > > +   if (isMonoSensor(sensor_))
> > > +           model += "_mono";
> > > +   std::string configurationFile = ipa_->configurationFile(model + ".json");
> > > 
> > >     IPASettings settings(configurationFile, sensor_->model());
> > >     ipa::RPi::InitParams params;
> > 
> > The other comments were nits, so
> > Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> 
> > 
> > > base-commit: fdc01dc3e0969423d65ee87f118164ec74373e5d
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/Documentation/environment_variables.rst b/Documentation/environment_variables.rst
index 7da9883a8380..6f1235587a40 100644
--- a/Documentation/environment_variables.rst
+++ b/Documentation/environment_variables.rst
@@ -57,8 +57,8 @@  LIBCAMERA_RPI_CONFIG_FILE

    Example value: ``/usr/local/share/libcamera/pipeline/rpi/vc4/minimal_mem.yaml``

-LIBCAMERA_RPI_TUNING_FILE
-   Define a custom JSON tuning file to use in the Raspberry Pi.
+LIBCAMERA_<NAME>_TUNING_FILE
+   Define a custom IPA tuning file to use with the pipeline handler `NAME`.

    Example value: ``/usr/local/share/libcamera/ipa/rpi/vc4/custom_sensor.json``

diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp
index 85004737c171..25f772a41bf8 100644
--- a/src/libcamera/ipa_proxy.cpp
+++ b/src/libcamera/ipa_proxy.cpp
@@ -98,16 +98,33 @@  IPAProxy::~IPAProxy()
 std::string IPAProxy::configurationFile(const std::string &name,
 					const std::string &fallbackName) const
 {
-	struct stat statbuf;
-	int ret;
-
 	/*
 	 * The IPA module name can be used as-is to build directory names as it
 	 * has been validated when loading the module.
 	 */
-	std::string ipaName = ipam_->info().name;
+	const std::string ipaName = ipam_->info().name;

-	/* Check the environment variable first. */
+	/*
+	 * Start with any user override through the module-specific environment
+	 * variable. Use the name of the IPA module up to the first '/' to
+	 * construct the variable name.
+	 */
+	std::string ipaEnvName = ipaName.substr(0, ipaName.find('/'));
+	std::transform(ipaEnvName.begin(), ipaEnvName.end(), ipaEnvName.begin(),
+		       [](unsigned char c) { return std::toupper(c); });
+	ipaEnvName = "LIBCAMERA_" + ipaEnvName + "_TUNING_FILE";
+
+	char const *configFromEnv = utils::secure_getenv(ipaEnvName.c_str());
+	if (configFromEnv && *configFromEnv == '\0')
+		return { configFromEnv };
+
+	struct stat statbuf;
+	int ret;
+
+	/*
+	 * Check the directory pointed to by the IPA config path environment
+	 * variable next.
+	 */
 	const char *confPaths = utils::secure_getenv("LIBCAMERA_IPA_CONFIG_PATH");
 	if (confPaths) {
 		for (const auto &dir : utils::split(confPaths, ":")) {
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
index 35c793da9bba..1ac8d8ae7ed9 100644
--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
@@ -380,18 +380,9 @@  int RkISP1CameraData::loadIPA(unsigned int hwRevision)
 	ipa_->paramsComputed.connect(this, &RkISP1CameraData::paramsComputed);
 	ipa_->metadataReady.connect(this, &RkISP1CameraData::metadataReady);

-	/*
-	 * The API tuning file is made from the sensor name unless the
-	 * environment variable overrides it.
-	 */
-	std::string ipaTuningFile;
-	char const *configFromEnv = utils::secure_getenv("LIBCAMERA_RKISP1_TUNING_FILE");
-	if (!configFromEnv || *configFromEnv == '\0') {
-		ipaTuningFile =
-			ipa_->configurationFile(sensor_->model() + ".yaml", "uncalibrated.yaml");
-	} else {
-		ipaTuningFile = std::string(configFromEnv);
-	}
+	/* The IPA tuning file is made from the sensor name. */
+	std::string ipaTuningFile =
+		ipa_->configurationFile(sensor_->model() + ".yaml", "uncalibrated.yaml");

 	IPACameraSensorInfo sensorInfo{};
 	int ret = sensor_->sensorInfo(&sensorInfo);
diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
index 4b147fdb379a..1f13e5230fae 100644
--- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
+++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
@@ -1156,20 +1156,11 @@  int CameraData::loadIPA(ipa::RPi::InitResult *result)
 	if (!ipa_)
 		return -ENOENT;

-	/*
-	 * The configuration (tuning file) is made from the sensor name unless
-	 * the environment variable overrides it.
-	 */
-	std::string configurationFile;
-	char const *configFromEnv = utils::secure_getenv("LIBCAMERA_RPI_TUNING_FILE");
-	if (!configFromEnv || *configFromEnv == '\0') {
-		std::string model = sensor_->model();
-		if (isMonoSensor(sensor_))
-			model += "_mono";
-		configurationFile = ipa_->configurationFile(model + ".json");
-	} else {
-		configurationFile = std::string(configFromEnv);
-	}
+	/* The configuration (tuning file) is made from the sensor name. */
+	std::string model = sensor_->model();
+	if (isMonoSensor(sensor_))
+		model += "_mono";
+	std::string configurationFile = ipa_->configurationFile(model + ".json");

 	IPASettings settings(configurationFile, sensor_->model());
 	ipa::RPi::InitParams params;