[libcamera-devel] [PATCH] ipa: ipu3: Rectify ControlInfoMap matching in IPAConfigInfo

[Laurent Pinchart]

Hi Paul,

On Wed, May 26, 2021 at 03:07:37PM +0300, Laurent Pinchart wrote:
> On Wed, May 26, 2021 at 05:18:14PM +0530, Umang Jain wrote:
> > The ControlInfoMap of entityControls member in IPAConfigInfo struct,
> > was not able to correctly match to the ControlInfoMap defined in
> > core.mojom. This resulted in a FATAL breakage when IPU3 IPA is meant to
> > run:
> > 
> >  FATAL IPADataSerializer ipa_data_serializer.cpp:437 ControlSerializer
> >  not provided for serialization of ControlInfoMap
> 
> Fixes: c76ca01323d8 ("ipa: ipu3: Introduce IPAConfigInfo in IPC")
> 
> > Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> As this fixes a breakage in master, I'll merge it right away.
> 
> Paul, is there a way this issue could have been detected at compile time
> ? If that's not doable, could you send at least a documentation patch ?

Beside the ControlInfoMap vs. libcamera::ControlInfoMap, here's the diff
in the generated code.

diff -Naur x86-gcc-10.2.0-source/include/libcamera/ipa/ipu3_ipa_serializer.h x86-gcc-11.1.0-source/include/libcamera/ipa/ipu3_ipa_serializer.h
--- x86-gcc-10.2.0-source/include/libcamera/ipa/ipu3_ipa_serializer.h	2021-05-26 15:33:16.154762388 +0300
+++ x86-gcc-11.1.0-source/include/libcamera/ipa/ipu3_ipa_serializer.h	2021-05-25 12:19:18.146693517 +0300
@@ -265,7 +265,7 @@
 public:
 	static std::tuple<std::vector<uint8_t>, std::vector<int32_t>>
 	serialize(const ipa::ipu3::IPAConfigInfo &data,
+		  [[maybe_unused]] ControlSerializer *cs = nullptr)
-		  ControlSerializer *cs)
 	{
 		std::vector<uint8_t> retData;

@@ -298,7 +298,7 @@

 	static ipa::ipu3::IPAConfigInfo
 	deserialize(std::vector<uint8_t> &data,
+		    ControlSerializer *cs = nullptr)
-		    ControlSerializer *cs)
 	{
 		return IPADataSerializer<ipa::ipu3::IPAConfigInfo>::deserialize(data.cbegin(), data.cend(), cs);
 	}
@@ -307,7 +307,7 @@
 	static ipa::ipu3::IPAConfigInfo
 	deserialize(std::vector<uint8_t>::const_iterator dataBegin,
 		    std::vector<uint8_t>::const_iterator dataEnd,
+		    [[maybe_unused]] ControlSerializer *cs = nullptr)
-		    ControlSerializer *cs)
 	{
 		ipa::ipu3::IPAConfigInfo ret;
 		std::vector<uint8_t>::const_iterator m = dataBegin;
diff -Naur x86-gcc-10.2.0-source/src/libcamera/proxy/ipu3_ipa_proxy.cpp x86-gcc-11.1.0-source/src/libcamera/proxy/ipu3_ipa_proxy.cpp
--- x86-gcc-10.2.0-source/src/libcamera/proxy/ipu3_ipa_proxy.cpp	2021-05-26 15:33:16.226762395 +0300
+++ x86-gcc-11.1.0-source/src/libcamera/proxy/ipu3_ipa_proxy.cpp	2021-05-25 12:19:18.262693528 +0300
@@ -272,7 +272,7 @@

 	std::vector<uint8_t> configInfoBuf;
 	std::tie(configInfoBuf, std::ignore) =
+		IPADataSerializer<IPAConfigInfo>::serialize(configInfo);
-		IPADataSerializer<IPAConfigInfo>::serialize(configInfo, &controlSerializer_);
 	_ipcInputBuf.data().insert(_ipcInputBuf.data().end(), configInfoBuf.begin(), configInfoBuf.end());


diff -Naur x86-gcc-10.2.0-source/src/libcamera/proxy/worker/ipu3_ipa_proxy_worker.cpp x86-gcc-11.1.0-source/src/libcamera/proxy/worker/ipu3_ipa_proxy_worker.cpp
--- x86-gcc-10.2.0-source/src/libcamera/proxy/worker/ipu3_ipa_proxy_worker.cpp	2021-05-26 15:33:16.224762394 +0300
+++ x86-gcc-11.1.0-source/src/libcamera/proxy/worker/ipu3_ipa_proxy_worker.cpp	2021-05-25 12:19:18.389693540 +0300
@@ -146,7 +146,8 @@

         	IPAConfigInfo configInfo = IPADataSerializer<IPAConfigInfo>::deserialize(
                 	_ipcMessage.data().cbegin() + configInfoStart,
-                	_ipcMessage.data().cend());
+                	_ipcMessage.data().cend(),
+                	&controlSerializer_);

 ipa_->configure(configInfo);

> > ---
> >  include/libcamera/ipa/ipu3.mojom | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > 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;
> >  };

-- 
Regards,

Laurent Pinchart