[v1,29/33] pipeline: rkisp1: Enable the dewarper based on the tuning file

24528 diff mbox series

To do actual lens dewarping, the dewarper will be configured based on
the tuning file. As a first step implement the basic loading of the
tuning file and enable/disable the dewarper for the given camera based
on the existence of the "Dewarp" algorithm in the tuning file.

Todo: This is an backwards incompatible change in that the dewarper is
currently included in the chain unconditionally. Some users may want to
not use the dewarper, so it is sensible to make that configurable. If it
should be in the algorithms section or in a different one is open for
debate.

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
---
 src/ipa/libipa/module.h                  |  4 ++
 src/libcamera/pipeline/rkisp1/rkisp1.cpp | 51 +++++++++++++++++++++++-
 2 files changed, 54 insertions(+), 1 deletion(-)

Quoting Stefan Klug (2025-09-30 13:26:50)
> To do actual lens dewarping, the dewarper will be configured based on
> the tuning file. As a first step implement the basic loading of the
> tuning file and enable/disable the dewarper for the given camera based
> on the existence of the "Dewarp" algorithm in the tuning file.
> 
> Todo: This is an backwards incompatible change in that the dewarper is
> currently included in the chain unconditionally. Some users may want to
> not use the dewarper, so it is sensible to make that configurable. If it
> should be in the algorithms section or in a different one is open for
> debate.
> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> ---
>  src/ipa/libipa/module.h                  |  4 ++
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 51 +++++++++++++++++++++++-
>  2 files changed, 54 insertions(+), 1 deletion(-)
> 
> diff --git a/src/ipa/libipa/module.h b/src/ipa/libipa/module.h
> index 0fb51916fff6..84386f901534 100644
> --- a/src/ipa/libipa/module.h
> +++ b/src/ipa/libipa/module.h
> @@ -74,6 +74,10 @@ private:
>         int createAlgorithm(Context &context, const YamlObject &data)
>         {
>                 const auto &[name, algoData] = *data.asDict().begin();
> +
> +               if (name == "Dewarp")
> +                       return 0;

I think this means that really - we shouldn't put dewarp config under
the algorithms.

It should be a separate base key.

Perhaps under convertors ?


> +
>                 std::unique_ptr<Algorithm<Module>> algo = createAlgorithm(name);
>                 if (!algo) {
>                         LOG(IPAModuleAlgo, Error)
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 1046930857e1..07dd5dc8d99a 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -16,6 +16,7 @@
>  #include <linux/media-bus-format.h>
>  #include <linux/rkisp1-config.h>
>  
> +#include <libcamera/base/file.h>
>  #include <libcamera/base/log.h>
>  #include <libcamera/base/utils.h>
>  
> @@ -47,6 +48,7 @@
>  #include "libcamera/internal/v4l2_request.h"
>  #include "libcamera/internal/v4l2_subdevice.h"
>  #include "libcamera/internal/v4l2_videodevice.h"
> +#include "libcamera/internal/yaml_parser.h"
>  
>  #include "rkisp1_path.h"
>  
> @@ -123,6 +125,7 @@ public:
>          */
>         MediaPipeline pipe_;
>  
> +       bool canUseDewarper_;
>         bool usesDewarper_;
>  
>  private:
> @@ -131,6 +134,7 @@ private:
>                                const ControlList &sensorControls);
>  
>         void metadataReady(unsigned int frame, const ControlList &metadata);
> +       int loadTuningFile(const std::string &file);
>  };
>  
>  class RkISP1CameraConfiguration : public CameraConfiguration
> @@ -416,6 +420,51 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision, uint32_t supportedBlocks)
>                 return ret;
>         }
>  
> +       ret = loadTuningFile(ipaTuningFile);
> +       if (ret < 0) {
> +               LOG(RkISP1, Error) << "Failed to load tuning file";
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +int RkISP1CameraData::loadTuningFile(const std::string &path)
> +{
> +       if (!pipe()->dewarper_)
> +               /* Nothing to do without dewarper */
> +               return 0;
> +
> +       LOG(RkISP1, Debug) << "Load tuning file " << path;
> +
> +       File file(path);
> +       if (!file.open(File::OpenModeFlag::ReadOnly)) {
> +               int ret = file.error();
> +               LOG(RkISP1, Error)
> +                       << "Failed to open tuning file "
> +                       << path << ": " << strerror(-ret);
> +               return ret;
> +       }
> +
> +       std::unique_ptr<libcamera::YamlObject> data = YamlParser::parse(file);
> +       if (!data)
> +               return -EINVAL;
> +
> +       if (!data->contains("algorithms")) {
> +               LOG(RkISP1, Error)
> +                       << "Tuning file doesn't contain any algorithm";
> +               return -EINVAL;
> +       }
> +
> +       const auto &algos = (*data)["algorithms"].asList();
> +       for (const auto &algo : algos) {
> +               if (algo.contains("Dewarp")) {
> +                       const auto &params = algo["Dewarp"];

params doesn't seem to be used here ?

> +
> +                       canUseDewarper_ = true;
> +               }
> +       }

Somewhat getting towards more modular pipeline handlers - perhaps we
need a list of convertors supported in the pipeline handler - and then
we pass the tuning file down to those to parse.


> +
>         return 0;
>  }
>  
> @@ -572,7 +621,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>          */
>         bool transposeAfterIsp = false;
>         bool useDewarper = false;
> -       if (pipe->dewarper_) {
> +       if (data_->canUseDewarper_) {
>                 /*
>                  * Platforms with dewarper support, such as i.MX8MP, support
>                  * only a single stream. We can inspect config_[0] only here.
> -- 
> 2.48.1
>

Hi Kieran,

Thank you for the review.

Quoting Kieran Bingham (2025-10-02 23:26:09)
> Quoting Stefan Klug (2025-09-30 13:26:50)
> > To do actual lens dewarping, the dewarper will be configured based on
> > the tuning file. As a first step implement the basic loading of the
> > tuning file and enable/disable the dewarper for the given camera based
> > on the existence of the "Dewarp" algorithm in the tuning file.
> > 
> > Todo: This is an backwards incompatible change in that the dewarper is
> > currently included in the chain unconditionally. Some users may want to
> > not use the dewarper, so it is sensible to make that configurable. If it
> > should be in the algorithms section or in a different one is open for
> > debate.
> > 
> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > ---
> >  src/ipa/libipa/module.h                  |  4 ++
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 51 +++++++++++++++++++++++-
> >  2 files changed, 54 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/ipa/libipa/module.h b/src/ipa/libipa/module.h
> > index 0fb51916fff6..84386f901534 100644
> > --- a/src/ipa/libipa/module.h
> > +++ b/src/ipa/libipa/module.h
> > @@ -74,6 +74,10 @@ private:
> >         int createAlgorithm(Context &context, const YamlObject &data)
> >         {
> >                 const auto &[name, algoData] = *data.asDict().begin();
> > +
> > +               if (name == "Dewarp")
> > +                       return 0;
> 
> I think this means that really - we shouldn't put dewarp config under
> the algorithms.
> 
> It should be a separate base key.
> 
> Perhaps under convertors ?

Yes, I didn't like that either. On the other hand it feels a bit
superfluous to add another top level element for this single instance.
For example on the soft-gpu-isp side, implementing this as part of the
gpu pipeline would be quite easy and then it is suddenly an algorithm
again?

One could also argue that algorithms contains all the algorithms
implemented in the pipeline. How that is done hardware wise is an
implementation detail. There are similar issues with the current
algorithm blocks on rkisp1. These are very centric to the
hardware-blocks but could/should potentially be more centered around the
algorithms supported by libipa.

Ahh difficult :-)

> 
> 
> > +
> >                 std::unique_ptr<Algorithm<Module>> algo = createAlgorithm(name);
> >                 if (!algo) {
> >                         LOG(IPAModuleAlgo, Error)
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 1046930857e1..07dd5dc8d99a 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -16,6 +16,7 @@
> >  #include <linux/media-bus-format.h>
> >  #include <linux/rkisp1-config.h>
> >  
> > +#include <libcamera/base/file.h>
> >  #include <libcamera/base/log.h>
> >  #include <libcamera/base/utils.h>
> >  
> > @@ -47,6 +48,7 @@
> >  #include "libcamera/internal/v4l2_request.h"
> >  #include "libcamera/internal/v4l2_subdevice.h"
> >  #include "libcamera/internal/v4l2_videodevice.h"
> > +#include "libcamera/internal/yaml_parser.h"
> >  
> >  #include "rkisp1_path.h"
> >  
> > @@ -123,6 +125,7 @@ public:
> >          */
> >         MediaPipeline pipe_;
> >  
> > +       bool canUseDewarper_;
> >         bool usesDewarper_;
> >  
> >  private:
> > @@ -131,6 +134,7 @@ private:
> >                                const ControlList &sensorControls);
> >  
> >         void metadataReady(unsigned int frame, const ControlList &metadata);
> > +       int loadTuningFile(const std::string &file);
> >  };
> >  
> >  class RkISP1CameraConfiguration : public CameraConfiguration
> > @@ -416,6 +420,51 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision, uint32_t supportedBlocks)
> >                 return ret;
> >         }
> >  
> > +       ret = loadTuningFile(ipaTuningFile);
> > +       if (ret < 0) {
> > +               LOG(RkISP1, Error) << "Failed to load tuning file";
> > +               return ret;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +int RkISP1CameraData::loadTuningFile(const std::string &path)
> > +{
> > +       if (!pipe()->dewarper_)
> > +               /* Nothing to do without dewarper */
> > +               return 0;
> > +
> > +       LOG(RkISP1, Debug) << "Load tuning file " << path;
> > +
> > +       File file(path);
> > +       if (!file.open(File::OpenModeFlag::ReadOnly)) {
> > +               int ret = file.error();
> > +               LOG(RkISP1, Error)
> > +                       << "Failed to open tuning file "
> > +                       << path << ": " << strerror(-ret);
> > +               return ret;
> > +       }
> > +
> > +       std::unique_ptr<libcamera::YamlObject> data = YamlParser::parse(file);
> > +       if (!data)
> > +               return -EINVAL;
> > +
> > +       if (!data->contains("algorithms")) {
> > +               LOG(RkISP1, Error)
> > +                       << "Tuning file doesn't contain any algorithm";
> > +               return -EINVAL;
> > +       }
> > +
> > +       const auto &algos = (*data)["algorithms"].asList();
> > +       for (const auto &algo : algos) {
> > +               if (algo.contains("Dewarp")) {
> > +                       const auto &params = algo["Dewarp"];
> 
> params doesn't seem to be used here ?

Oh yes, that should be moved to the next patch.

> 
> > +
> > +                       canUseDewarper_ = true;
> > +               }
> > +       }
> 
> Somewhat getting towards more modular pipeline handlers - perhaps we
> need a list of convertors supported in the pipeline handler - and then
> we pass the tuning file down to those to parse.
> 

Yes, I feel these are first tiny steps in that direction, but I still
miss a full picture.

Best regards,
Stefan

> 
> > +
> >         return 0;
> >  }
> >  
> > @@ -572,7 +621,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> >          */
> >         bool transposeAfterIsp = false;
> >         bool useDewarper = false;
> > -       if (pipe->dewarper_) {
> > +       if (data_->canUseDewarper_) {
> >                 /*
> >                  * Platforms with dewarper support, such as i.MX8MP, support
> >                  * only a single stream. We can inspect config_[0] only here.
> > -- 
> > 2.48.1
> >