[libcamera-devel] [RFC PATCH v2] ipa: ipu3: Introduce IPAConfigInfo in IPC
Umang Jain
umang.jain at ideasonboard.com
Thu Apr 29 10:25:57 CEST 2021
Hi Jacopo,
Thanks for comments,
On 4/29/21 1:15 PM, Jacopo Mondi wrote:
> 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"
Ok, I meant the former, I shall re-word in next iteration
>
>
>> + */
>> +
>> +/**
>> + * \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
Agreed
>
>> + */
>> +
>> + /**
>> + * \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)
Oh, thanks for the context. I'll take a note of it and tinker on how we
can address this change and adapt the documentation. Might need some
cycle of discussion in general with Kieran too!
>
>> +
>> + /**
>> + * \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 ?
Maybe - I see this as an optimization point, so not my highest priority
as of now.
>
> BTW I don't see iif being used currently. Will you need it in future ?
`iif` is used for closed source IPA. So, the current issue is we have
two competing IPAs - Open IPU3 IPA (already merged in-tree) and a closed
one(probably will be residing in it's own git repo). But the ph-IPA
interface has to be common to both. This is why you don't see `iif`
usage here(in open IPA), but is need for the closed one.
>
>> +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)
These are mojom declarations - hence I believe the framework by-default
enforces that all function parameters are `const` and passed
by-reference. These mojom files are used to generated the .cpp interface
code (see include/libcamera/ipa/meson.build), which matches the
prototype below (where you have comment (`Why the two prototypes are
different`).
>
>> 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 ?
Originally it was your suggested way only, but Laurent wanted to use
named initializers for safety - as there are couple of `Sizes`
parameters and one can get the ordering wrong in directly populating
them. IPAConfigInfo is generated from mojom framework and doesn't yet
support named initiazers hence, this is what I ended up as closest
implementation.
>
> 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.
Yes - overall idea is good, but I would be comfortable doing that when
we have got some level of stability for the closed source IPA
>
> 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