[libcamera-devel,RFC,1/4] pipeline: ipu3: Support IPA tuning file
diff mbox series

Message ID 20220802100955.1546-2-laurent.pinchart@ideasonboard.com
State Accepted
Headers show
Series
  • Add tuning data file support to the IPU3 IPA module
Related show

Commit Message

Laurent Pinchart Aug. 2, 2022, 10:09 a.m. UTC 
Pass the path name of the YAML IPA tuning file to the IPA module. The
file name is derived from the sensor name ("${sensor_name}.yaml"), with
a fallback to "uncalibrated.yaml".

The tuning file name can be manually overridden with the
LIBCAMERA_IPU3_TUNING_FILE environment variable.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/libcamera/pipeline/ipu3/ipu3.cpp | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

Comments

Kieran Bingham Aug. 8, 2022, 10:53 p.m. UTC | #1 
Quoting Laurent Pinchart via libcamera-devel (2022-08-02 11:09:52)
> Pass the path name of the YAML IPA tuning file to the IPA module. The
> file name is derived from the sensor name ("${sensor_name}.yaml"), with
> a fallback to "uncalibrated.yaml".
> 
> The tuning file name can be manually overridden with the
> LIBCAMERA_IPU3_TUNING_FILE environment variable.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 4fe52f74a68c..5c4a0a567ff2 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -1243,8 +1243,26 @@ int IPU3CameraData::loadIPA()
>         if (ret)
>                 return ret;
>  
> -       ret = ipa_->init(IPASettings{ "", sensor->model() }, sensorInfo,
> -                        sensor->controls(), &ipaControls_);
> +       /*
> +        * The API tuning file is made from the sensor name unless the
> +        * environment variable overrides it. If

If ?

> +        */
> +       std::string ipaTuningFile;
> +       char const *configFromEnv = utils::secure_getenv("LIBCAMERA_IPU3_TUNING_FILE");

This is just about OK for RPi where there is usually only a single
camera, but this doesn't really work when there are more such as most IPU3
devices.

I would be tempted to drop this - and move towards having a libcamera
yaml config file that could override this on a per-camera id basis.


> +       if (!configFromEnv || *configFromEnv == '\0') {
> +               ipaTuningFile = ipa_->configurationFile(sensor->model() + ".yaml");
> +               /*
> +                * If the tuning file isn't found, fall back to the
> +                * 'uncalibrated' configuration file.
> +                */
> +               if (ipaTuningFile.empty())
> +                       ipaTuningFile = ipa_->configurationFile("uncalibrated.yaml");
> +       } else {
> +               ipaTuningFile = std::string(configFromEnv);
> +       }
> +
> +       ret = ipa_->init(IPASettings{ ipaTuningFile, sensor->model() },
> +                        sensorInfo, sensor->controls(), &ipaControls_);
>         if (ret) {
>                 LOG(IPU3, Error) << "Failed to initialise the IPU3 IPA";
>                 return ret;
> -- 
> Regards,
> 
> Laurent Pinchart
>
Laurent Pinchart Aug. 9, 2022, 2:38 p.m. UTC | #2 
Hi Kieran,

On Mon, Aug 08, 2022 at 11:53:52PM +0100, Kieran Bingham wrote:
> Quoting Laurent Pinchart via libcamera-devel (2022-08-02 11:09:52)
> > Pass the path name of the YAML IPA tuning file to the IPA module. The
> > file name is derived from the sensor name ("${sensor_name}.yaml"), with
> > a fallback to "uncalibrated.yaml".
> > 
> > The tuning file name can be manually overridden with the
> > LIBCAMERA_IPU3_TUNING_FILE environment variable.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 22 ++++++++++++++++++++--
> >  1 file changed, 20 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 4fe52f74a68c..5c4a0a567ff2 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -1243,8 +1243,26 @@ int IPU3CameraData::loadIPA()
> >         if (ret)
> >                 return ret;
> >  
> > -       ret = ipa_->init(IPASettings{ "", sensor->model() }, sensorInfo,
> > -                        sensor->controls(), &ipaControls_);
> > +       /*
> > +        * The API tuning file is made from the sensor name unless the
> > +        * environment variable overrides it. If
> 
> If ?

Good question :-)

> > +        */
> > +       std::string ipaTuningFile;
> > +       char const *configFromEnv = utils::secure_getenv("LIBCAMERA_IPU3_TUNING_FILE");
> 
> This is just about OK for RPi where there is usually only a single
> camera, but this doesn't really work when there are more such as most IPU3
> devices.
> 
> I would be tempted to drop this - and move towards having a libcamera
> yaml config file that could override this on a per-camera id basis.

You're right, I'll drop it.

> > +       if (!configFromEnv || *configFromEnv == '\0') {
> > +               ipaTuningFile = ipa_->configurationFile(sensor->model() + ".yaml");
> > +               /*
> > +                * If the tuning file isn't found, fall back to the
> > +                * 'uncalibrated' configuration file.
> > +                */
> > +               if (ipaTuningFile.empty())
> > +                       ipaTuningFile = ipa_->configurationFile("uncalibrated.yaml");
> > +       } else {
> > +               ipaTuningFile = std::string(configFromEnv);
> > +       }
> > +
> > +       ret = ipa_->init(IPASettings{ ipaTuningFile, sensor->model() },
> > +                        sensorInfo, sensor->controls(), &ipaControls_);
> >         if (ret) {
> >                 LOG(IPU3, Error) << "Failed to initialise the IPU3 IPA";
> >                 return ret;
Kieran Bingham Aug. 9, 2022, 7:43 p.m. UTC | #3 
Quoting Laurent Pinchart (2022-08-09 15:38:45)
> Hi Kieran,
> 
> On Mon, Aug 08, 2022 at 11:53:52PM +0100, Kieran Bingham wrote:
> > Quoting Laurent Pinchart via libcamera-devel (2022-08-02 11:09:52)
> > > Pass the path name of the YAML IPA tuning file to the IPA module. The
> > > file name is derived from the sensor name ("${sensor_name}.yaml"), with
> > > a fallback to "uncalibrated.yaml".
> > > 
> > > The tuning file name can be manually overridden with the
> > > LIBCAMERA_IPU3_TUNING_FILE environment variable.
> > > 
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > >  src/libcamera/pipeline/ipu3/ipu3.cpp | 22 ++++++++++++++++++++--
> > >  1 file changed, 20 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > index 4fe52f74a68c..5c4a0a567ff2 100644
> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > @@ -1243,8 +1243,26 @@ int IPU3CameraData::loadIPA()
> > >         if (ret)
> > >                 return ret;
> > >  
> > > -       ret = ipa_->init(IPASettings{ "", sensor->model() }, sensorInfo,
> > > -                        sensor->controls(), &ipaControls_);
> > > +       /*
> > > +        * The API tuning file is made from the sensor name unless the
> > > +        * environment variable overrides it. If
> > 
> > If ?
> 
> Good question :-)

With this answered,

> 
> > > +        */
> > > +       std::string ipaTuningFile;
> > > +       char const *configFromEnv = utils::secure_getenv("LIBCAMERA_IPU3_TUNING_FILE");
> > 
> > This is just about OK for RPi where there is usually only a single
> > camera, but this doesn't really work when there are more such as most IPU3
> > devices.
> > 
> > I would be tempted to drop this - and move towards having a libcamera
> > yaml config file that could override this on a per-camera id basis.
> 
> You're right, I'll drop it.

And this dropped...


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

> 
> > > +       if (!configFromEnv || *configFromEnv == '\0') {
> > > +               ipaTuningFile = ipa_->configurationFile(sensor->model() + ".yaml");
> > > +               /*
> > > +                * If the tuning file isn't found, fall back to the
> > > +                * 'uncalibrated' configuration file.
> > > +                */
> > > +               if (ipaTuningFile.empty())
> > > +                       ipaTuningFile = ipa_->configurationFile("uncalibrated.yaml");
> > > +       } else {
> > > +               ipaTuningFile = std::string(configFromEnv);
> > > +       }
> > > +
> > > +       ret = ipa_->init(IPASettings{ ipaTuningFile, sensor->model() },
> > > +                        sensorInfo, sensor->controls(), &ipaControls_);
> > >         if (ret) {
> > >                 LOG(IPU3, Error) << "Failed to initialise the IPU3 IPA";
> > >                 return ret;
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
index 4fe52f74a68c..5c4a0a567ff2 100644
--- a/src/libcamera/pipeline/ipu3/ipu3.cpp
+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
@@ -1243,8 +1243,26 @@  int IPU3CameraData::loadIPA()
 	if (ret)
 		return ret;
 
-	ret = ipa_->init(IPASettings{ "", sensor->model() }, sensorInfo,
-			 sensor->controls(), &ipaControls_);
+	/*
+	 * The API tuning file is made from the sensor name unless the
+	 * environment variable overrides it. If
+	 */
+	std::string ipaTuningFile;
+	char const *configFromEnv = utils::secure_getenv("LIBCAMERA_IPU3_TUNING_FILE");
+	if (!configFromEnv || *configFromEnv == '\0') {
+		ipaTuningFile = ipa_->configurationFile(sensor->model() + ".yaml");
+		/*
+		 * If the tuning file isn't found, fall back to the
+		 * 'uncalibrated' configuration file.
+		 */
+		if (ipaTuningFile.empty())
+			ipaTuningFile = ipa_->configurationFile("uncalibrated.yaml");
+	} else {
+		ipaTuningFile = std::string(configFromEnv);
+	}
+
+	ret = ipa_->init(IPASettings{ ipaTuningFile, sensor->model() },
+			 sensorInfo, sensor->controls(), &ipaControls_);
 	if (ret) {
 		LOG(IPU3, Error) << "Failed to initialise the IPU3 IPA";
 		return ret;