[libcamera-devel] [RFC PATCH v2] ipa: ipu3: Introduce IPAConfigInfo in IPC

[Jean-Michel Hautbois]

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
>>> - 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?
> 
> 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 */
}

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;
>>>   }
>>>
>