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
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@ideasonboard.com> > > > > > > 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); > >
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(-)