[libcamera-devel] [RFC PATCH v2] ipa: ipu3: Introduce IPAConfigInfo in IPC
paul.elder at ideasonboard.com
paul.elder at ideasonboard.com
Fri Apr 30 05:22:40 CEST 2021
Hello Umang and Jean-Michel,
On Thu, Apr 29, 2021 at 07:30:47AM +0200, Jean-Michel Hautbois wrote:
>
>
> On 27/04/2021 14:54, Umang Jain wrote:
> > Hi JM
> >
> > On 4/27/21 5:23 PM, Jean-Michel Hautbois wrote:
> >> Hi Umang,
> >>
> >> Thanks for the patch !
> >>
> >> On 27/04/2021 08:35, Umang Jain wrote:
> >>> IPAConfigInfo is a consolidated data structure passed from IPU3
> >>> pipeline-handler to IPU3 IPA. This provides a streamlined and
> >>> extensible way to provide the configuration data to IPA interface.
> >>>
> >>> The IPA interface should be common to both closed-source and
> >>> open-source IPU3 IPAs. Hence, the structure encompasses additional
> >>> configuration information required to init and configure the
> >>> closed-source IPA as well.
> >>>
> >>> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> >>> ---
> >>> Changes in v2:
> >>> - Mark as RFC(do not merge) as this is built upon Paul's
> >>> [Fix support for core.mojom structs v3] - waiting for merge to master
This has been merged already.
> >>> - Drop cropRegion_ from structure - this is provided from
> >>> CameraSensorInfo
> >>> itself.
> >>> - Doxygen documentation of IPAConfigInfo.
> >>> ---
> >>> include/libcamera/ipa/ipu3.mojom | 46 ++++++++++++++++++++++++++--
> >>> src/ipa/ipu3/ipu3.cpp | 14 ++++-----
> >>> src/libcamera/pipeline/ipu3/ipu3.cpp | 9 +++++-
> >>> 3 files changed, 58 insertions(+), 11 deletions(-)
> >>>
> >>> diff --git a/include/libcamera/ipa/ipu3.mojom
> >>> b/include/libcamera/ipa/ipu3.mojom
> >>> index a717b1e6..acd5cfa4 100644
> >>> --- a/include/libcamera/ipa/ipu3.mojom
> >>> +++ b/include/libcamera/ipa/ipu3.mojom
> >>> @@ -25,13 +25,55 @@ struct IPU3Action {
> >>> libcamera.ControlList controls;
> >>> };
> >>> +/**
> >>> + * \struct IPAConfigInfo
> >>> + * \brief Information to be passed from IPU3 pipeline-handler to IPA
> >>> + *
> >>> + * The IPAConfigInfo structure stores the data passed from IPU3
> >>> pipeline-handler
> >>> + * to the IPAIPU3Interface::configure(). The structure provides
> >>> extensibility
> >>> + * for additional configuration data as required, for closed-source or
> >>> + * open-source IPAs' configure() phases.
> >>> + */
> >>> +
> >>> +/**
> >>> + * \var IPAConfigInfo::sensorInfo
> >>> + * \brief Camera sensor information
> >>> + *
> >>> + * Provides the camera sensor information such as model name,
> >>> pixel-rate,
> >>> + * output-size and so on. Refer to libcamera::CameraSensorInfo for
> >>> further
> >>> + * details.
> >>> + */
> >>> +
> >>> + /**
> >>> + * \var IPAConfigInfo::entityControls
> >>> + * \brief Controls supported by the sensor
> >>> + *
> >>> + * A map of V4L2 controls supported by the sensor in order to read
> >>> or write
> >>> + * control values to them.
> >>> + */
> >>> +
> >>> + /**
> >>> + * \var IPAConfigInfo::bdsOutputSize
> >>> + * \brief Size of ImgU BDS rectangle
> >>> + */
> >>> +
> >>> + /**
> >>> + * \var IPAConfigInfo::iif
> >>> + * \brief Size of ImgU input-feeder rectangle
> >>> + */
> >>> +struct IPAConfigInfo {
> >>> + libcamera.CameraSensorInfo sensorInfo;
> >>> + map<uint32, ControlInfoMap> entityControls;
> >>> + libcamera.Size bdsOutputSize;
> >>> + libcamera.Size iif;
> >>> +};
> >>> +
> >> I think it is interesting, and would like your advice: I would need this
> >> same structure for rkisp1 platform, except that the sizes are obviously
> >> not the same.
> >> And so, I am wondering if we could imagine a more general structure like:
> >> struct IPAConfigInfo {
> >> libcamera.CameraSensorInfo sensorInfo;
> >> map<uint32, ControlInfoMap> entityControls;
> >> libcamera.Size ispInputSize;
> >> libcamera.Size ispOutputSize;
> >> };
> >>
> >> names and all may have to be reworked but that is the base idea.
> >> There is no bds in rkisp1 (afaik) but on both IPU3 and RkIsp1 at least
> >> you have an input an output pad size for the ISP node.
> > Certainly, this might be a good idea. I am not opposed to it.
> >
> > The question here is, that is the structure core.mojom worthy, in order
> > to be shared between multiple IPAs right? If not, I think we need to
> > keep them in <platform IPA>.mojom for now.
> >
> > And if we want to share the data-structures, we need to work out how all
> > existing IPAs (for e.g. Raspberry Pi) can be ported to used that
> > IPAConfig structure too. We probably, should address this by looking at
> > all the common parameters we need for all the existing IPAs and have a
> > patch series that ports every IPA to use that data-structure.
> >
> > However, there are some risky? situations that can happen, for e.g.
> > - An IPA X requiring a parameter Y for configuration and the common
> > struct IPAConfig doesn't have a placeholder
> > - Parameter Y is added to common struct IPAConfig
> > - Parameter Y is unused/nullptr for all IPAs except IPA X
> >
> > I do not know if adding placeholder for Y is fine in the common struct,
> > if fine or not, in the codebase purview.
> >
> > Thinking about it, the chances of various IPA, having exact
> > configuration data (common IPAConfig) are slim. Hence, the IPA authors
> > will try hacks for other parameters they need for IPA configuration,
> > either as function-params or introducing new structures and passing
> > alongside with common struct IPAConfig to the IPA. That would not be
> > nice, isn't it?
We could have a greatest common divisor for the IPA config info struct
in core.mojom, and then the IPAs could define their own additional
struct.
> >
> > I might be wrong, so I shall keep an open mind about it. Maybe we can
> > track this as a task somewhere too?
> >
>
> The idea I had is to have a common structure, and having the IPA
> specific ones inherit from this common one.
> You would have something like
> IPAIPU3ConfigInfo: IPAConfigInfo {
> /* myGreatParameters */
> }
Because mojo doesn't support inheritance :/
Well okay, if you *really* wanted to, in theory, we could use an attribute
to declare inheritance, and then I'd have to expand the code generator
for that (just keeping doors and minds open).
Paul
>
> That one may not make sense though :-).
>
> >>
> >>> interface IPAIPU3Interface {
> >>> init(libcamera.IPASettings settings) => (int32 ret);
> >>> start() => (int32 ret);
> >>> stop();
> >>> - configure(map<uint32, libcamera.ControlInfoMap> entityControls,
> >>> - libcamera.Size bdsOutputSize) => ();
> >>> + configure(IPAConfigInfo configInfo);
> >>> mapBuffers(array<libcamera.IPABuffer> buffers);
> >>> unmapBuffers(array<uint32> ids);
> >>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> >>> index f5343547..769c24d3 100644
> >>> --- a/src/ipa/ipu3/ipu3.cpp
> >>> +++ b/src/ipa/ipu3/ipu3.cpp
> >>> @@ -43,8 +43,7 @@ public:
> >>> int start() override;
> >>> void stop() override {}
> >>> - void configure(const std::map<uint32_t, ControlInfoMap>
> >>> &entityControls,
> >>> - const Size &bdsOutputSize) override;
> >>> + void configure(const IPAConfigInfo &configInfo) override;
> >>> void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> >>> void unmapBuffers(const std::vector<unsigned int> &ids) override;
> >>> @@ -139,13 +138,12 @@ void IPAIPU3::calculateBdsGrid(const Size
> >>> &bdsOutputSize)
> >>> << (int)bdsGrid_.height << " << " <<
> >>> (int)bdsGrid_.block_height_log2 << ")";
> >>> }
> >>> -void IPAIPU3::configure(const std::map<uint32_t, ControlInfoMap>
> >>> &entityControls,
> >>> - const Size &bdsOutputSize)
> >>> +void IPAIPU3::configure(const IPAConfigInfo &configInfo)
> >>> {
> >>> - if (entityControls.empty())
> >>> + if (configInfo.entityControls.empty())
> >>> return;
> >>> - ctrls_ = entityControls.at(0);
> >>> + ctrls_ = configInfo.entityControls.at(0);
> >>> const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);
> >>> if (itExp == ctrls_.end()) {
> >>> @@ -169,10 +167,10 @@ void IPAIPU3::configure(const
> >>> std::map<uint32_t, ControlInfoMap> &entityControls
> >>> params_ = {};
> >>> - calculateBdsGrid(bdsOutputSize);
> >>> + calculateBdsGrid(configInfo.bdsOutputSize);
> >>> awbAlgo_ = std::make_unique<IPU3Awb>();
> >>> - awbAlgo_->initialise(params_, bdsOutputSize, bdsGrid_);
> >>> + awbAlgo_->initialise(params_, configInfo.bdsOutputSize, bdsGrid_);
> >>> agcAlgo_ = std::make_unique<IPU3Agc>();
> >>> agcAlgo_->initialise(bdsGrid_);
> >>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp
> >>> b/src/libcamera/pipeline/ipu3/ipu3.cpp
> >>> index 73306cea..b1ff1dbe 100644
> >>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> >>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> >>> @@ -635,7 +635,14 @@ int PipelineHandlerIPU3::configure(Camera
> >>> *camera, CameraConfiguration *c)
> >>> std::map<uint32_t, ControlInfoMap> entityControls;
> >>> entityControls.emplace(0, data->cio2_.sensor()->controls());
> >>> - data->ipa_->configure(entityControls, config->imguConfig().bds);
> >>> +
> >>> + ipa::ipu3::IPAConfigInfo configInfo;
> >>> + configInfo.sensorInfo = sensorInfo;
> >>> + configInfo.entityControls = entityControls;
> >>> + configInfo.bdsOutputSize = config->imguConfig().bds;
> >>> + configInfo.iif = config->imguConfig().iif;
> >>> +
> >>> + data->ipa_->configure(configInfo);
> >>> return 0;
> >>> }
> >>>
> >
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
More information about the libcamera-devel
mailing list