[libcamera-devel] [PATCH 1/6] libcamera: ipu3: Drop entityControls map

[Jacopo Mondi]

Hi Kieran,

On Thu, Sep 02, 2021 at 02:16:27PM +0100, Kieran Bingham wrote:
> On 01/09/2021 15:37, Jacopo Mondi wrote:
> > The IPA::configure() function has an IPAConfigInfo parameters which
> > contains a map of numerical indexes to ControlInfoMap instances.
> >
> > This is a leftover of the old IPA protocol, where it was not possible to
> > specify a rich interface as it is possible today and each entity
> > ControlInfoMap was indexed by a numerical id and stored in a map.
> >
> > Now that the IPA interface allows to specify parameters by name, drop the
> > map and send the sensor's control info map only.
> >
> > If we'll need more ControlInfoMap to be shared with the IPA, a new parameter
> > can be added to IPAConfigInfo.
>
>
> It looks like this will need a patch for
>
>   https://git.libcamera.org/libcamera/ipu3-ipa.git/
>
> as well. But given that it requires manually installed AIQ libraries on
> non CrOS build environments, perhaps it's something myself or Umang
> should handle when this series merges.

I would ofc be happy if you and Umang could take care of that, but as
we evolve the IPA interface on the open source IPA module, the burden
of keeping the two in sync might get considerable on your side.

If I'm not mistaken most the changes to the IPA interface will reflect
only in ipu3.cpp from the above repository.

Is there plan to merge the glue code between the PH and the closed
source IPA module in libcamera ? In this way we could keep
at least the interface in sync when we modify the protocol.

Thanks
   j

>
> I wonder if we should be increasing the IPA ABI / version numbers too
> whenever we make a change.
>
> But alas - it doesn't look like that's fully implemented, The mojo files
> should have a version number in there (or even better, could mojo create
> a hash of the interface to catch when it changes I wonder)?
>
> So, given that you can't update the other IPA, and you can't update the
> IPU3 IPA Version (without further patches) ... I guess this is it ...
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
>
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  include/libcamera/ipa/ipu3.mojom     | 2 +-
> >  src/ipa/ipu3/ipu3.cpp                | 4 ++--
> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +-
> >  3 files changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom
> > index d561c2244907..2045ce909a88 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, libcamera.ControlInfoMap> entityControls;
> > +	libcamera.ControlInfoMap sensorControls;
> >  	libcamera.Size bdsOutputSize;
> >  	libcamera.Size iif;
> >  };
> > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> > index c925cf642611..6a74f7826332 100644
> > --- a/src/ipa/ipu3/ipu3.cpp
> > +++ b/src/ipa/ipu3/ipu3.cpp
> > @@ -337,14 +337,14 @@ void IPAIPU3::calculateBdsGrid(const Size &bdsOutputSize)
> >
> >  int IPAIPU3::configure(const IPAConfigInfo &configInfo)
> >  {
> > -	if (configInfo.entityControls.empty()) {
> > +	if (configInfo.sensorControls.empty()) {
> >  		LOG(IPAIPU3, Error) << "No controls provided";
> >  		return -ENODATA;
> >  	}
> >
> >  	sensorInfo_ = configInfo.sensorInfo;
> >
> > -	ctrls_ = configInfo.entityControls.at(0);
> > +	ctrls_ = configInfo.sensorControls;
> >
> >  	const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);
> >  	if (itExp == ctrls_.end()) {
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index c287bf86e79a..92e869257e53 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -654,7 +654,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
> >  	}
> >
> >  	ipa::ipu3::IPAConfigInfo configInfo;
> > -	configInfo.entityControls.emplace(0, data->cio2_.sensor()->controls());
> > +	configInfo.sensorControls = data->cio2_.sensor()->controls();
> >  	configInfo.sensorInfo = sensorInfo;
> >  	configInfo.bdsOutputSize = config->imguConfig().bds;
> >  	configInfo.iif = config->imguConfig().iif;
> >