[libcamera-devel] [RFC PATCH v2] ipa: ipu3: Introduce IPAConfigInfo in IPC
Jacopo Mondi
jacopo at jmondi.org
Thu Apr 29 09:45:14 CEST 2021
Hi Umang,
On Tue, Apr 27, 2021 at 12:05:27PM +0530, 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
> - 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
to 'the' 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.
What do you mean with extensibility ? That one can add fields to the
structure, like to all structures ? :)
Or are you thinking at some inheritance mechanism ?
In the former case I would say
"The structure can be extended with additional parameters to
accommodate the requirements of different IPA modules"
> + */
> +
> +/**
> + * \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.
I would just:
\sa CameraSensorInfo
As duplicating the description has limited value imho
> + */
> +
> + /**
> + * \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.
> + */
The existing entityControls is described as
\param[in] entityControls Controls provided by the pipeline entities
and it does not only include the sensor controls (otherwise it would
have been just a ControlInfoMap not a map). The idea was that each ph-IPA
defines it's own mapping. Ie entityControls[0] = sensor controls,
entityControls[1] = someother entity controls, etc etc).
So far I think only this is only been effectively used to transport
sensor controls, and I would be fine making this a ControlList and
name it 'sensorControls'. Even if we later need to pass to IPA the
ControlInfoMap of another entity, we can add it to this structure with
a more explicit name (much better than hiding it in a map and
establishing what an index maps to like we're doing now)
> +
> + /**
> + * \var IPAConfigInfo::bdsOutputSize
> + * \brief Size of ImgU BDS rectangle
> + */
> +
> + /**
> + * \var IPAConfigInfo::iif
> + * \brief Size of ImgU input-feeder rectangle
> + */
Empty line maybe.
I wonder if it wouldn't be better to pass the full
ImgUDevice::PipeConfig. True that the less data we transport over IPC
the better, so this makes sense. Is it worth wrapping thise two sizes
in an 'ImguSize' structure ?
BTW I don't see iif being used currently. Will you need it in future ?
> +struct IPAConfigInfo {
> + libcamera.CameraSensorInfo sensorInfo;
> + map<uint32, ControlInfoMap> entityControls;
> + libcamera.Size bdsOutputSize;
> + libcamera.Size iif;
> +};
> +
> interface IPAIPU3Interface {
> init(libcamera.IPASettings settings) => (int32 ret);
> start() => (int32 ret);
> stop();
>
> - configure(map<uint32, libcamera.ControlInfoMap> entityControls,
> - libcamera.Size bdsOutputSize) => ();
> + configure(IPAConfigInfo configInfo);
No idea how that works for IPC, but if this was a regular function I
would have made this
configure(const 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;
Ah
Why the two prototypes are different ?
>
> 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;
Why a copy when you can populate this one directly ?
Overall the idea is good. I would refrain from premature optimizations
like the one suggested by Jean-Micheal for the moment though, it's a
bit too early in my opinion, and the effort for each pipeline handler
to define its own exchange structures is limited.
Thanks
j
> + configInfo.bdsOutputSize = config->imguConfig().bds;
> + configInfo.iif = config->imguConfig().iif;
> +
> + data->ipa_->configure(configInfo);
>
> return 0;
> }
> --
> 2.26.2
>
> _______________________________________________
> 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