[libcamera-devel] [PATCH v3 5/6] ipa: ipu3: Introduce IPAConfigInfo in IPC

[Umang Jain]

Hi Paul,

On 5/25/21 2:39 PM, paul.elder at ideasonboard.com wrote:
> Hi Laurent,
>
> On Mon, May 24, 2021 at 11:36:14PM +0300, Laurent Pinchart wrote:
>> Hi Paul,
>>
>> On Mon, May 24, 2021 at 02:43:54PM +0900, paul.elder at ideasonboard.com wrote:
>>> On Mon, May 24, 2021 at 04:00:01AM +0300, Laurent Pinchart wrote:
>>>> On Fri, May 21, 2021 at 06:58:22PM +0530, Umang Jain wrote:
>>>>> IPAConfigInfo is a consolidated data structure passed from IPU3
>>>>> pipeline-handler to IPU3 IPA. The structure can be extended with
>>>>> additional parameters to accommodate the requirements of multiple
>>>>> IPU3 IPA modules.
>>>>>
>>>>> Adapt the in-tree IPU3 IPA to use IPAConfigInfo as well.
>>>>>
>>>>> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
>>>>> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
>>>>> Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
>>>>> Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
>>>>> ---
>>>>>   include/libcamera/ipa/ipu3.mojom     | 10 ++++++++--
>>>>>   src/ipa/ipu3/ipu3.cpp                | 14 ++++++--------
>>>>>   src/libcamera/pipeline/ipu3/ipu3.cpp | 10 +++++++---
>>>>>   3 files changed, 21 insertions(+), 13 deletions(-)
>>>>>
>>>>> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom
>>>>> index 9e3cd885..6b6b431f 100644
>>>>> --- a/include/libcamera/ipa/ipu3.mojom
>>>>> +++ b/include/libcamera/ipa/ipu3.mojom
>>>>> @@ -30,13 +30,19 @@ struct IPU3Action {
>>>>>   	libcamera.ControlList controls;
>>>>>   };
>>>>>   
>>>>> +struct IPAConfigInfo {
>>>>> +	libcamera.IPACameraSensorInfo sensorInfo;
>>>>> +	map<uint32, ControlInfoMap> entityControls;
>>>> Should this be libcamera.ControlInfoMap ? Paul, what's the rule for the
>>>> 'libcamera.' namespace prefix ?
>>> Here it's optional.
>>>
>>> The code generator doesn't actually need the libcamera prefix anywhere
>>> (because all generated files are in the libcamera namespace anyway), but
>>> the mojo parser needs it for top-level types.
>> What do you mean by "top-level types" ?
> Types that are parameters of functions or that are members of structs.
Omitting `libcamera.` for entityControls introduced a FATAL breakage 
while running the IPA from master:

[0:52:11.842213069] [7830] FATAL IPADataSerializer 
ipa_data_serializer.cpp:437 ControlSerializer not provided for 
serialization of ControlInfoMap

Weird that it never was seen before by me (or Kieran (assuming that 
these patches were applied to his tree in development for testing Intel 
IPA))  :-/

The below diff fixes the FATAL error:

diff --git a/include/libcamera/ipa/ipu3.mojom 
b/include/libcamera/ipa/ipu3.mojom
index 6b6b431f..32c046ad 100644
--- a/include/libcamera/ipa/ipu3.mojom
+++ b/include/libcamera/ipa/ipu3.mojom
@@ -32,7 +32,7 @@ struct IPU3Action {

  struct IPAConfigInfo {
         libcamera.IPACameraSensorInfo sensorInfo;
-       map<uint32, ControlInfoMap> entityControls;
+       map<uint32, libcamera.ControlInfoMap> entityControls;
         libcamera.Size bdsOutputSize;
         libcamera.Size iif;


>
>
> Paul
>
>>>> With this address (if needed),
>>>>
>>>> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>>>>
>>>>> +	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) => ();
>>>>>   
>>>>>   	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 98c6160f..5b15ca90 100644
>>>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
>>>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
>>>>> @@ -633,9 +633,13 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
>>>>>   		return ret;
>>>>>   	}
>>>>>   
>>>>> -	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.entityControls.emplace(0, data->cio2_.sensor()->controls());
>>>>> +	configInfo.sensorInfo = sensorInfo;
>>>>> +	configInfo.bdsOutputSize = config->imguConfig().bds;
>>>>> +	configInfo.iif = config->imguConfig().iif;
>>>>> +
>>>>> +	data->ipa_->configure(configInfo);
>>>>>   
>>>>>   	return 0;
>>>>>   }
>> -- 
>> Regards,
>>
>> Laurent Pinchart