[libcamera-devel] [RFC PATCH] ipa: ipu3: Retain request controls in the IPA

[Umang Jain]

Hi Paul

On 5/20/22 11:14, Paul Elder via libcamera-devel wrote:
> On Thu, May 19, 2022 at 04:16:42PM +0200, Kieran Bingham via libcamera-devel wrote:
>> To ensure consistency in processing control lists, an application does
>> not have to continually set controls that were already set, but not
>> expected to change.
>>
>> To ensure we keep that state, merge incoming request control lists with
>> a stored ControlList that is owned by the IPA and store the state of
>> that merged list to the per frame FrameContext so that algorithms know
>> the full expected control state at that time.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> Oops, I forgot:
>
> Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
>
> I think this patch as-is is totally fine. Assuming merge() behaves
> properly, this does exactly what we want. The "trigger" (though not
> really trigger) controls for AE we can implement them
> differently/separately/on top, so this patch is fine.


Ack.

As long as we are on the same page design-wise, I don't have any 
objections to the patch, but I'll hold my R-B for now since the design 
work needs to be solidified in coming days.

>
>> ---
>>
>> RFC:
>>
>> 1)
>>
>> If ControlList.update() were implemented like .merge, but with
>> overriding existing values, instead of leaving them then this could be
>> simplified to :
>>
>> +	retainedControls_.update(controls);
>> +	context_.frameContexts[frame % kMaxFrameContexts] = { frame, std::copy(retainedControls_) };
>>
>> Not yet sure if that's more efficient or if it doesn't make much
>> difference.
>>
>> 2)
>>
>> This will not support triggered controls that are set once to
>> 'fire'... Do we have / plan to have any of those?
>>
>> 3)
>>
>> Do we want to keep both a delta control list (the incoming controls) as
>> well as the merged/retained list for full state so IPA's can know if
>> something was changed?
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>> ---
>>   src/ipa/ipu3/ipu3.cpp | 23 ++++++++++++++++++++++-
>>   1 file changed, 22 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
>> index 2f6bb672f7bb..89fc34f86e46 100644
>> --- a/src/ipa/ipu3/ipu3.cpp
>> +++ b/src/ipa/ipu3/ipu3.cpp
>> @@ -176,6 +176,9 @@ private:
>>   
>>   	/* Local parameter storage */
>>   	struct IPAContext context_;
>> +
>> +	/* Control retention to maintain mode states */
>> +	ControlList retainedControls_;
>>   };
>>   
>>   /**
>> @@ -456,6 +459,8 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
>>   
>>   	/* Clean IPAActiveState at each reconfiguration. */
>>   	context_.activeState = {};
>> +	retainedControls_.clear();
>> +
>>   	IPAFrameContext initFrameContext;
>>   	context_.frameContexts.fill(initFrameContext);
>>   
>> @@ -617,8 +622,24 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,
>>    */
>>   void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)
>>   {
>> +	/*
>> +	 * Controls are retained, to ensure any mode updates are persistant.
>> +	 * We merge them into our class control storage before assigning to
>> +	 * the frame context.
>> +	 *
>> +	 * \todo This will not support trigger controls and may need rework if
>> +	 *	 we add any to prevent continually retriggering.
>> +	 *
>> +	 * \todo We may wish to store both the full merged control list, as well
>> +	 *	 as the delta (\a controls) to facilitate algorithms identifying
>> +	 *	 when things have changed.
>> +	 */
>> +	ControlList mergeControls = controls;
>> +	mergeControls.merge(retainedControls_);
>> +	retainedControls_ = mergeControls;
>> +
>>   	/* \todo Start processing for 'frame' based on 'controls'. */
>> -	context_.frameContexts[frame % kMaxFrameContexts] = { frame, controls };
>> +	context_.frameContexts[frame % kMaxFrameContexts] = { frame, std::move(mergeControls) };
>>   }
>>   
>>   /**
>> -- 
>> 2.25.1
>>