| Message ID | 20241220162724.756494-7-stefan.klug@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
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 ? 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. > Signed-off-by: Stefan Klug <stefan.klug@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);
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. > > 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? 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. Cheers, Stefan > > > Signed-off-by: Stefan Klug <stefan.klug@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); > > -- > Regards, > > Laurent Pinchart
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);
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. Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> --- src/ipa/rkisp1/rkisp1.cpp | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)