[RFC PATCH 6/7] ipa: rkisp1: Add a lookahead of one frame when sending sensor controls

[Paul Elder]

On Mon, Jan 13, 2025 at 01:04:36PM +0100, Stefan Klug wrote:
> Hi Laurent,
> 
> Thank you for the review. 
> 
> On Mon, Jan 13, 2025 at 12:12:37AM +0200, Laurent Pinchart wrote:
> > Hi Stefan,
> > 
> > Thank you for the patch.
> > 
> > On Fri, Dec 20, 2024 at 05:26:52PM +0100, Stefan Klug wrote:
> > > When the delayed controls instance of the pipeline is reset, it initializes
> > > the values for frame 0 as being sent out to the sensor (which is
> > > correct). The next sequence number that can be pushed to delayed
> > > controls is therefore number 1. Ensure that the IPA never tries to
> > > queue controls for frame 0.
> > 
> > What happens to the controls passed in request 0 ?
> 
> These were correctly applied to the activeState so they are not lost but
> will get picked up in frame 1.

>From my understanding, 5/7 handles this with ipa_->start() returning the
sensor controls to be set, and then the pipeline handler sets the sensor
controls synchronously after ipa_->start() returns and before
streamOn()ing. Then it resets the delayed controls and so here we are.

> > 
> > I know the existing code doesn't behave correctly, but it feels that
> > this series is piling hacks towards a result that will be even more
> > difficult to untangle :-S I'd like a second opinion on this.
> 
> Is it really more hacks, or merely making the existing deficiencies 
> more visible?

Well it does indeed kind of feel like a hack, but it's not that bad of a
hack...

> Maybe we can take this as discussion basis for the big picture. In my
> first PFC series the lookahead was pushed down to DelayedControls (which
> I maybe should have renamed to LookaheadControls) which didn't get much
> approval. But the changes were also difficult to digest. So I see there
> is a bit of work to be done conceptually also. I still believe we need
> to move the sensor lookahead out of the IPA so I didn't try to implement
> a full fledged lookahead here.

I think it is indeed more visible here, at least compared to in
DelayedControls.


I think it's fine...

Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>

> 
> > 
> > > Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> > > ---
> > >  src/ipa/rkisp1/rkisp1.cpp | 9 ++++++---
> > >  1 file changed, 6 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > > index 799fe0846635..5d439e0d727b 100644
> > > --- a/src/ipa/rkisp1/rkisp1.cpp
> > > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > > @@ -386,10 +386,13 @@ void IPARkISP1::processStats(const uint32_t frame, const uint32_t bufferId,
> > >  
> > >  	/*
> > >  	 * \todo: Here we should do a lookahead that takes the sensor delays
> > > -	 * into account.
> > > +	 * into account. A lookahead of 1 is the smallest lookahead possible to
> > > +	 * ensure we don't try to send the controls for a frame that we already
> > > +	 * received.
> > >  	 */
> > > -	ControlList ctrls = getSensorControls(frame);
> > > -	setSensorControls.emit(frame, ctrls);
> > > +	int lookahead = 1;
> > > +	ControlList ctrls = getSensorControls(frame + lookahead);
> > > +	setSensorControls.emit(frame + lookahead, ctrls);
> > >  
> > >  	context_.debugMetadata.moveEntries(metadata);
> > >  	metadataReady.emit(frame, metadata);
> >