[libcamera-devel] [PATCH] ipa: ipu3: Don't call SetControls for each frame

[Kieran Bingham]

Hi Laurent,

On 12/02/2021 16:50, Laurent Pinchart wrote:
> Hi Jean-Michel,
> 
> On Fri, Feb 12, 2021 at 04:59:01PM +0100, Jean-Michel Hautbois wrote:
>> On 12/02/2021 16:36, Kieran Bingham wrote:
>>> On 09/02/2021 15:03, Jean-Michel Hautbois wrote:
>>>> Setting a default EXPOSURE and ANALOGUE_GAIN makes sense for the first
>>>> frame but there is no need to force it for all frames.
>>>> It will be called later by the 3A algorithms.
>>>>
>>>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
>>>> ---
>>>>  src/ipa/ipu3/ipu3.cpp | 3 ---
>>>>  1 file changed, 3 deletions(-)
>>>>
>>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
>>>> index b11b03ef..ae7e8c0b 100644
>>>> --- a/src/ipa/ipu3/ipu3.cpp
>>>> +++ b/src/ipa/ipu3/ipu3.cpp
>>>> @@ -188,9 +188,6 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)
>>>>  	op.operation = IPU3_IPA_ACTION_PARAM_FILLED;
>>>>  
>>>>  	queueFrameAction.emit(frame, op);
>>>> -
>>>> -	/* \todo Calculate new values for exposure_ and gain_. */
>>>> -	setControls(frame);
>>>
>>> Does this leave the setControls() function unused? I wonder if it should
>>> be removed in that case.
>>
>> It is still called at configure()...
>>
>>> I hear that this patch is anticipated to be able to stop the exposure
>>> being set to max, without any means of setting it so I'm not opposed to
>>> this going in as a fix anyway.
>>
>> Well, Laurent pointed that we could implement support for
>> controls::Exposure and it would definitely be better, but I can't find
>> enough time for it this week :-/.
> 
> For the record, I'm fine with this patch as a very short term interim
> measure.

I'll push this soon then, as even when we have IPA patches posted on
top, we'll expect more time for review cycles etc, so this can fill the
gap for a couple of weeks, as I'm aware it may be affecting current
developers working on sensor drivers attached to the IPU3.

--
Kieran


>>> Especially as this will all change soon enough with your other work on top.
>>>
>>> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>>>
>>>>  }
>>>>  
>>>>  void IPAIPU3::parseStatistics(unsigned int frame,
>>>>
> 

-- 
Regards
--
Kieran