[libcamera-devel] [PATCH 1/6] libcamera: ipu3: Drop entityControls map
Kieran Bingham
kieran.bingham at ideasonboard.com
Fri Sep 3 13:30:17 CEST 2021
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
>>> 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;
>
More information about the libcamera-devel
mailing list