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

[Laurent Pinchart]

Hi Kieran,

On Fri, Sep 03, 2021 at 12:30:17PM +0100, Kieran Bingham wrote:
> On 03/09/2021 12:12, Laurent Pinchart wrote:
> > On Fri, Sep 03, 2021 at 06:33:30PM +0900, paul.elder at ideasonboard.com wrote:
> >> 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 wonder if we should be increasing the IPA ABI / version numbers too
> >>> whenever we make a change.
> >>
> >> We don't have a release yet so it should be fine, right? :p
> >>
> >> I guess we gotta start working on this for real... I'll prepare
> >> a summary + discussion points.
> > 
> > Another can of worms :-) It certainly needs to be addressed, but will
> > need to include .mojom versioning, and protocol stability. We can start
> > discussions, but I don't think the implementation is the priority at
> > this point.
> 
> But we /do/ have externally built IPAs already which can get out of sync
> on this - and therefore crash. (I know, it's crashed on me already :D)
> 
> Hiding behind "We don't have a release" only gets us to the point that
> we release. If that is going to be in 5 years, sure we can keep hiding.
> 
> But I think we're doing ourselves a dis-service by not actually putting
> the process in place to use (and therefore test) this before we get to 1.0

The process has to be put in place, it will involve lots of work, which
means that starting the discussion early is good. The implementation,
however, isn't the highest priority.

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

-- 
Regards,

Laurent Pinchart