| Message ID | 20251024085130.995967-24-stefan.klug@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Stefan On Fri, Oct 24, 2025 at 10:50:47AM +0200, Stefan Klug wrote: > For per frame control we want to tick the IPA by the sensor frame > sequence instead of the request frame sequence. This has the side effect > that the IPA must be able to cope with situations where a frame context > is required for a frame that was not queued before (computeParams is > called without a corresponding request) or processStats is called for an > unexpected sequence number (because a scratch buffer was used on kernel > side) > > Prepare for that by allowing the frame context to be initialized on demand. > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > --- > src/ipa/rkisp1/algorithms/awb.cpp | 2 ++ > src/ipa/rkisp1/ipa_context.h | 2 ++ > src/ipa/rkisp1/rkisp1.cpp | 6 ++++++ > 3 files changed, 10 insertions(+) > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp > index 399fb51be414..27109478c340 100644 > --- a/src/ipa/rkisp1/algorithms/awb.cpp > +++ b/src/ipa/rkisp1/algorithms/awb.cpp > @@ -172,6 +172,8 @@ void Awb::queueRequest(IPAContext &context, > awbAlgo_->handleControls(controls); > > frameContext.awb.autoEnabled = awb.autoEnabled; > + frameContext.awb.gains = awb.automatic.gains; > + frameContext.awb.temperatureK = awb.automatic.temperatureK; unrelated ? > > if (awb.autoEnabled) > return; > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h > index f85a130d9c23..185951fb8286 100644 > --- a/src/ipa/rkisp1/ipa_context.h > +++ b/src/ipa/rkisp1/ipa_context.h > @@ -214,6 +214,8 @@ struct IPAFrameContext : public FrameContext { > double strength; > double gain; > } wdr; > + > + bool initialised; As already pointed out, the base struct already has an initialised member, an indication that something could maybe be improved. the here introduced initializeFrameContext() is called from 4 places: - start() [where you should call alloc() imho; with valid controls] - queueRequest() [which calls alloc(); with valid controls] - computeParams() [which calls get(); without controls] - processStats() [which calls get(); without controls) If I'm not mistaken - start() will always be called before queueRequest and the frame context won't be initialized, but controls need to be queued to algorithms - queueRequest() will be called with valid controls, but the frame context could already be initialized by the FCQ - computeParams/processStats won't have valid controls but the frame context might already be initialised. Here I'm not sure if we need to even try to call algo->queueRequest() because there won't be controls associated with this frame context. All in all, I'm wondering if initializeFrameContext() is not actually about only queueing controls to the algos, and we can even skip calling it from computeParams/processStats for which the correct initialisation of the frame context is already performed by the FCQueue. > }; > > struct IPAContext { > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > index 01b30c947a0a..23d80bc43c5d 100644 > --- a/src/ipa/rkisp1/rkisp1.cpp > +++ b/src/ipa/rkisp1/rkisp1.cpp > @@ -343,6 +343,10 @@ void IPARkISP1::initializeFrameContext(const uint32_t frame, > IPAFrameContext &frameContext, > const ControlList &controls) > { > + if (frameContext.initialised) > + return; > + > + frameContext.initialised = true; > for (auto const &a : algorithms()) { > Algorithm *algo = static_cast<Algorithm *>(a.get()); > if (algo->disabled_) > @@ -354,6 +358,7 @@ void IPARkISP1::initializeFrameContext(const uint32_t frame, > void IPARkISP1::computeParams(const uint32_t frame, const uint32_t bufferId) > { > IPAFrameContext &frameContext = context_.frameContexts.get(frame); > + initializeFrameContext(frame, frameContext, {}); > > /* > * \todo: This needs discussion. In raw mode, computeParams is > @@ -383,6 +388,7 @@ void IPARkISP1::processStats(const uint32_t frame, const uint32_t bufferId, > const ControlList &sensorControls) > { > IPAFrameContext &frameContext = context_.frameContexts.get(frame); > + initializeFrameContext(frame, frameContext, {}); > > /* > * In raw capture mode, the ISP is bypassed and no statistics buffer is > -- > 2.48.1 >
Hi again Stefan On Thu, Nov 06, 2025 at 07:44:57PM +0100, Jacopo Mondi wrote: > Hi Stefan > > On Fri, Oct 24, 2025 at 10:50:47AM +0200, Stefan Klug wrote: > > For per frame control we want to tick the IPA by the sensor frame > > sequence instead of the request frame sequence. This has the side effect > > that the IPA must be able to cope with situations where a frame context > > is required for a frame that was not queued before (computeParams is > > called without a corresponding request) or processStats is called for an > > unexpected sequence number (because a scratch buffer was used on kernel > > side) > > > > Prepare for that by allowing the frame context to be initialized on demand. > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > --- > > src/ipa/rkisp1/algorithms/awb.cpp | 2 ++ > > src/ipa/rkisp1/ipa_context.h | 2 ++ > > src/ipa/rkisp1/rkisp1.cpp | 6 ++++++ > > 3 files changed, 10 insertions(+) > > > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp > > index 399fb51be414..27109478c340 100644 > > --- a/src/ipa/rkisp1/algorithms/awb.cpp > > +++ b/src/ipa/rkisp1/algorithms/awb.cpp > > @@ -172,6 +172,8 @@ void Awb::queueRequest(IPAContext &context, > > awbAlgo_->handleControls(controls); > > > > frameContext.awb.autoEnabled = awb.autoEnabled; > > + frameContext.awb.gains = awb.automatic.gains; > > + frameContext.awb.temperatureK = awb.automatic.temperatureK; > > unrelated ? > > > > > if (awb.autoEnabled) > > return; > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h > > index f85a130d9c23..185951fb8286 100644 > > --- a/src/ipa/rkisp1/ipa_context.h > > +++ b/src/ipa/rkisp1/ipa_context.h > > @@ -214,6 +214,8 @@ struct IPAFrameContext : public FrameContext { > > double strength; > > double gain; > > } wdr; > > + > > + bool initialised; > > As already pointed out, the base struct already has an initialised > member, an indication that something could maybe be improved. > > the here introduced initializeFrameContext() > is called from 4 places: > > - start() [where you should call alloc() imho; with valid controls] > - queueRequest() [which calls alloc(); with valid controls] > - computeParams() [which calls get(); without controls] > - processStats() [which calls get(); without controls) > > If I'm not mistaken > > - start() will always be called before queueRequest and the frame > context won't be initialized, but controls need to be queued to > algorithms > - queueRequest() will be called with valid controls, but the frame > context could already be initialized by the FCQ > - computeParams/processStats won't have valid controls but the frame > context might already be initialised. Here I'm not sure if we need this should be "might have not been initialised" as we're dealing with the case where prepare/process for frame X are called before the corresponding request X has been queued. > to even try to call algo->queueRequest() because there won't be > controls associated with this frame context. > > All in all, I'm wondering if initializeFrameContext() is not actually about only > queueing controls to the algos, and we can even skip calling it from > computeParams/processStats for which the correct initialisation of the frame > context is already performed by the FCQueue. I slept over this a bit.. I think we should slightly re-work the FCQ to make this a bit nicer. I think the first thing to do would be to propagate to IPAs the "error" conditions from FCQueue::alloc() and ::get(). This would help you detect if an unexpected sequence of events has happened and act accordingly. Let's leave out start() for a bit (I would like to clarify, here on in the other patch if start() should call alloc() or not, but that's unrelated). Let's only consider queueRequest()/process()/prepare(). In normal operating conditions, with user queueing requests fast enough to sustain the sensor frame rate, as we now clock the IPA with frames from the sensor, and we have no guarantee that requests will be queued fast enough to sustain this. - IPA::queueRequest(Request:X) - FCQueue::alloc(X): - frame context already initialized - This means that prepare/process have already been called - The Request is late and the corresponding frame has already been produced - We need to accummulate the Request's controls in the list of controls to apply to the "next" algo->queueRequest() loop - frame context not initialized - Normal operating conditions - Call algo->queueRequest(request->controls()) - computeParams(Frame:X) - FCQeueue::get(X) - frame context already initialized - Normal operating conditions - call algos->prepare() - frame context not initialized - User request underrun - if any pending controls for late requests are present we should handle them: call algo->queueRequest() to provide them the pending controls we have accumulated - processStats(Frame:X) - same as computeParams(X) This is what I think it is implemented in this series ? Do you agree or I missed anything ? If that's the case, wouldn't it be nicer if we could do IPA::queueRequest(request) { auto { fc, err } = context_.frameContexts.alloc(request->sequece); if (err == REQUEST_UNDERRUN) { /* Accumulate controls in the list of pending controls * because the request is late */ pendingControls.merge(request->controls); } queueRequestToAlgo(request->controls); } IPA::computeParams(frame) { auto { fc, err } = context_.frameContexts.get(frame); if (err == REQUEST_UNDERRUN) { queueRequestToAlgo(pendingControls); } for (algo : algos) { algo->prepare(); } setSensorControls(); } IPA::processStats(frame, sensorControls) { auto { fc, err } = context_.frameContexts.get(frame); if (err == REQUEST_UNDERRUN) { queueRequestToAlgo(pendingControls); } updateSensorControls(sensorControls); for (algo : algos) { algo->process(); } metadataReady.emit(); } The only error condition from FCQueue is then a request underrun (apart from the Fatal 'overwrite' error), which corresponds to the two conditions now demoted to Debug messages in alloc() and get() in case of underruns we handle the list of pending controls, either by accummulating them in queueRequest() or by sending them to algos in computeParams and processStats. Does this match the mental model you have implemented in this series or have I missed some parts ? One part it is not totally clear to me in this series is if computeParams/processStats for a frame will always be called in sequence or not, and if not, if there's anything to do to address this condition. Thanks j > > > }; > > > > struct IPAContext { > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > > index 01b30c947a0a..23d80bc43c5d 100644 > > --- a/src/ipa/rkisp1/rkisp1.cpp > > +++ b/src/ipa/rkisp1/rkisp1.cpp > > @@ -343,6 +343,10 @@ void IPARkISP1::initializeFrameContext(const uint32_t frame, > > IPAFrameContext &frameContext, > > const ControlList &controls) > > { > > + if (frameContext.initialised) > > + return; > > + > > + frameContext.initialised = true; > > for (auto const &a : algorithms()) { > > Algorithm *algo = static_cast<Algorithm *>(a.get()); > > if (algo->disabled_) > > @@ -354,6 +358,7 @@ void IPARkISP1::initializeFrameContext(const uint32_t frame, > > void IPARkISP1::computeParams(const uint32_t frame, const uint32_t bufferId) > > { > > IPAFrameContext &frameContext = context_.frameContexts.get(frame); > > + initializeFrameContext(frame, frameContext, {}); > > > > /* > > * \todo: This needs discussion. In raw mode, computeParams is > > @@ -383,6 +388,7 @@ void IPARkISP1::processStats(const uint32_t frame, const uint32_t bufferId, > > const ControlList &sensorControls) > > { > > IPAFrameContext &frameContext = context_.frameContexts.get(frame); > > + initializeFrameContext(frame, frameContext, {}); > > > > /* > > * In raw capture mode, the ISP is bypassed and no statistics buffer is > > -- > > 2.48.1 > >
Hi Jacopo, Thanks for looking deep into this. Quoting Jacopo Mondi (2025-11-07 09:02:48) > Hi again Stefan > > On Thu, Nov 06, 2025 at 07:44:57PM +0100, Jacopo Mondi wrote: > > Hi Stefan > > > > On Fri, Oct 24, 2025 at 10:50:47AM +0200, Stefan Klug wrote: > > > For per frame control we want to tick the IPA by the sensor frame > > > sequence instead of the request frame sequence. This has the side effect > > > that the IPA must be able to cope with situations where a frame context > > > is required for a frame that was not queued before (computeParams is > > > called without a corresponding request) or processStats is called for an > > > unexpected sequence number (because a scratch buffer was used on kernel > > > side) > > > > > > Prepare for that by allowing the frame context to be initialized on demand. > > > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > > --- > > > src/ipa/rkisp1/algorithms/awb.cpp | 2 ++ > > > src/ipa/rkisp1/ipa_context.h | 2 ++ > > > src/ipa/rkisp1/rkisp1.cpp | 6 ++++++ > > > 3 files changed, 10 insertions(+) > > > > > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp > > > index 399fb51be414..27109478c340 100644 > > > --- a/src/ipa/rkisp1/algorithms/awb.cpp > > > +++ b/src/ipa/rkisp1/algorithms/awb.cpp > > > @@ -172,6 +172,8 @@ void Awb::queueRequest(IPAContext &context, > > > awbAlgo_->handleControls(controls); > > > > > > frameContext.awb.autoEnabled = awb.autoEnabled; > > > + frameContext.awb.gains = awb.automatic.gains; > > > + frameContext.awb.temperatureK = awb.automatic.temperatureK; > > > > unrelated ? Ooops :-) > > > > > > > > if (awb.autoEnabled) > > > return; > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h > > > index f85a130d9c23..185951fb8286 100644 > > > --- a/src/ipa/rkisp1/ipa_context.h > > > +++ b/src/ipa/rkisp1/ipa_context.h > > > @@ -214,6 +214,8 @@ struct IPAFrameContext : public FrameContext { > > > double strength; > > > double gain; > > > } wdr; > > > + > > > + bool initialised; > > > > As already pointed out, the base struct already has an initialised > > member, an indication that something could maybe be improved. > > > > the here introduced initializeFrameContext() > > is called from 4 places: > > > > - start() [where you should call alloc() imho; with valid controls] > > - queueRequest() [which calls alloc(); with valid controls] > > - computeParams() [which calls get(); without controls] > > - processStats() [which calls get(); without controls) > > > > If I'm not mistaken > > > > - start() will always be called before queueRequest and the frame > > context won't be initialized, but controls need to be queued to > > algorithms > > - queueRequest() will be called with valid controls, but the frame > > context could already be initialized by the FCQ > > - computeParams/processStats won't have valid controls but the frame > > context might already be initialised. Here I'm not sure if we need > > this should be "might have not been initialised" as we're dealing with > the case where prepare/process for frame X are called before the > corresponding request X has been queued. > > > to even try to call algo->queueRequest() because there won't be > > controls associated with this frame context. > > > > All in all, I'm wondering if initializeFrameContext() is not actually about only > > queueing controls to the algos, and we can even skip calling it from > > computeParams/processStats for which the correct initialisation of the frame > > context is already performed by the FCQueue. I think I miss something here. I don't think we can skip calling initializeFrameContext() for computeParams/processStats because it ensures that algo->queueRequest() is called for the FC. I think algo->queueRequest() should be renamed to algo->initializeRequest() which is the actual intent. The FCQueue only zeroes the frame context but actual initialization happens in algo->queueRequest(). > > I slept over this a bit.. I think we should slightly re-work the FCQ > to make this a bit nicer. I completely agree. I'm not happy with the design of the FCQueue. I tried to apply modifications in baby steps to ensure I didn't miss anything in the big picture. But there is still room for further improvements. > > I think the first thing to do would be to propagate to IPAs the > "error" conditions from FCQueue::alloc() and ::get(). > > This would help you detect if an unexpected sequence of events has > happened and act accordingly. > > Let's leave out start() for a bit (I would like to clarify, here on in > the other patch if start() should call alloc() or not, but that's > unrelated). Let's only consider queueRequest()/process()/prepare(). > > In normal operating conditions, with user queueing requests fast > enough to sustain the sensor frame rate, as we now clock the IPA with > frames from the sensor, and we have no guarantee that requests will be > queued fast enough to sustain this. > > > - IPA::queueRequest(Request:X) > - FCQueue::alloc(X): > - frame context already initialized > - This means that prepare/process have already been called > - The Request is late and the corresponding frame has already > been produced > - We need to accummulate the Request's controls in the list of > controls to apply to the "next" algo->queueRequest() loop > - frame context not initialized > - Normal operating conditions > - Call algo->queueRequest(request->controls()) > > - computeParams(Frame:X) > - FCQeueue::get(X) > - frame context already initialized > - Normal operating conditions > - call algos->prepare() > - frame context not initialized > - User request underrun > - if any pending controls for late requests are present we > should handle them: call algo->queueRequest() to provide > them the pending controls we have accumulated > > - processStats(Frame:X) > - same as computeParams(X) > > This is what I think it is implemented in this series ? Do you agree > or I missed anything ? I think that's it, yes. > > If that's the case, wouldn't it be nicer if we could do > > IPA::queueRequest(request) { > auto { fc, err } = > context_.frameContexts.alloc(request->sequece); This is a tiny bit misleading as it is not the request sequence, but sensor frame and controls. > if (err == REQUEST_UNDERRUN) { > /* Accumulate controls in the list of pending controls > * because the request is late */ > pendingControls.merge(request->controls); > } > > queueRequestToAlgo(request->controls); > } > > > IPA::computeParams(frame) { > auto { fc, err } = context_.frameContexts.get(frame); > if (err == REQUEST_UNDERRUN) { > queueRequestToAlgo(pendingControls); > } > > for (algo : algos) { > algo->prepare(); > } > > setSensorControls(); > } > > IPA::processStats(frame, sensorControls) { > auto { fc, err } = context_.frameContexts.get(frame); > if (err == REQUEST_UNDERRUN) { > queueRequestToAlgo(pendingControls); > } > > updateSensorControls(sensorControls); > > for (algo : algos) { > algo->process(); > } > > metadataReady.emit(); > } > > The only error condition from FCQueue is then a request underrun > (apart from the Fatal 'overwrite' error), which corresponds to the two > conditions now demoted to Debug messages in alloc() and get() > > in case of underruns we handle the list of pending controls, either by > accummulating them in queueRequest() or by sending them to algos in > computeParams and processStats. > > Does this match the mental model you have implemented in this series > or have I missed some parts ? I think you got that all right. I'm not sure if I like that style better as it adds more control logic into computeParams()/processStats() and assumes that the FCQueue has knowledge of the expected order of actions. But I agree that you could get rid of the outer initialized member in that case. I'd like the FCQueue to be as simple as possible. Continuing on your thoughts what about removing the FCQueue::alloc all together? If we replace alloc+get by a single get(): auto { fc, did_allocate } = context_.frameContexts.get(frame); we could move all the debug messages out of the FCQueue into the IPA. We can then pass the did_allocate to initializeFrameContext() which would be close to your proposal or stick to the FC.initialized member which I somehow still like better. But that is a technical nuance. > > One part it is not totally clear to me in this series is if > computeParams/processStats for a frame will always be called in > sequence or not, and if not, if there's anything to do to address this > condition. I wouldn't make much guarantees here. But I can't come up with a condition where processStats is called without a prior computeParams. A computeParams() on a frame after processStats was called on that frame should never happen. Best regards, Stefan > > Thanks > j > > > > > > }; > > > > > > struct IPAContext { > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > > > index 01b30c947a0a..23d80bc43c5d 100644 > > > --- a/src/ipa/rkisp1/rkisp1.cpp > > > +++ b/src/ipa/rkisp1/rkisp1.cpp > > > @@ -343,6 +343,10 @@ void IPARkISP1::initializeFrameContext(const uint32_t frame, > > > IPAFrameContext &frameContext, > > > const ControlList &controls) > > > { > > > + if (frameContext.initialised) > > > + return; > > > + > > > + frameContext.initialised = true; > > > for (auto const &a : algorithms()) { > > > Algorithm *algo = static_cast<Algorithm *>(a.get()); > > > if (algo->disabled_) > > > @@ -354,6 +358,7 @@ void IPARkISP1::initializeFrameContext(const uint32_t frame, > > > void IPARkISP1::computeParams(const uint32_t frame, const uint32_t bufferId) > > > { > > > IPAFrameContext &frameContext = context_.frameContexts.get(frame); > > > + initializeFrameContext(frame, frameContext, {}); > > > > > > /* > > > * \todo: This needs discussion. In raw mode, computeParams is > > > @@ -383,6 +388,7 @@ void IPARkISP1::processStats(const uint32_t frame, const uint32_t bufferId, > > > const ControlList &sensorControls) > > > { > > > IPAFrameContext &frameContext = context_.frameContexts.get(frame); > > > + initializeFrameContext(frame, frameContext, {}); > > > > > > /* > > > * In raw capture mode, the ISP is bypassed and no statistics buffer is > > > -- > > > 2.48.1 > > >
Hi Stefan On Fri, Nov 07, 2025 at 11:41:15AM +0100, Stefan Klug wrote: > Hi Jacopo, > > Thanks for looking deep into this. > > Quoting Jacopo Mondi (2025-11-07 09:02:48) > > Hi again Stefan > > > > On Thu, Nov 06, 2025 at 07:44:57PM +0100, Jacopo Mondi wrote: > > > Hi Stefan > > > > > > On Fri, Oct 24, 2025 at 10:50:47AM +0200, Stefan Klug wrote: > > > > For per frame control we want to tick the IPA by the sensor frame > > > > sequence instead of the request frame sequence. This has the side effect > > > > that the IPA must be able to cope with situations where a frame context > > > > is required for a frame that was not queued before (computeParams is > > > > called without a corresponding request) or processStats is called for an > > > > unexpected sequence number (because a scratch buffer was used on kernel > > > > side) > > > > > > > > Prepare for that by allowing the frame context to be initialized on demand. > > > > > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > > > --- > > > > src/ipa/rkisp1/algorithms/awb.cpp | 2 ++ > > > > src/ipa/rkisp1/ipa_context.h | 2 ++ > > > > src/ipa/rkisp1/rkisp1.cpp | 6 ++++++ > > > > 3 files changed, 10 insertions(+) > > > > > > > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp > > > > index 399fb51be414..27109478c340 100644 > > > > --- a/src/ipa/rkisp1/algorithms/awb.cpp > > > > +++ b/src/ipa/rkisp1/algorithms/awb.cpp > > > > @@ -172,6 +172,8 @@ void Awb::queueRequest(IPAContext &context, > > > > awbAlgo_->handleControls(controls); > > > > > > > > frameContext.awb.autoEnabled = awb.autoEnabled; > > > > + frameContext.awb.gains = awb.automatic.gains; > > > > + frameContext.awb.temperatureK = awb.automatic.temperatureK; > > > > > > unrelated ? > > Ooops :-) > > > > > > > > > > > > if (awb.autoEnabled) > > > > return; > > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h > > > > index f85a130d9c23..185951fb8286 100644 > > > > --- a/src/ipa/rkisp1/ipa_context.h > > > > +++ b/src/ipa/rkisp1/ipa_context.h > > > > @@ -214,6 +214,8 @@ struct IPAFrameContext : public FrameContext { > > > > double strength; > > > > double gain; > > > > } wdr; > > > > + > > > > + bool initialised; > > > > > > As already pointed out, the base struct already has an initialised > > > member, an indication that something could maybe be improved. > > > > > > the here introduced initializeFrameContext() > > > is called from 4 places: > > > > > > - start() [where you should call alloc() imho; with valid controls] > > > - queueRequest() [which calls alloc(); with valid controls] > > > - computeParams() [which calls get(); without controls] > > > - processStats() [which calls get(); without controls) > > > > > > If I'm not mistaken > > > > > > - start() will always be called before queueRequest and the frame > > > context won't be initialized, but controls need to be queued to > > > algorithms > > > - queueRequest() will be called with valid controls, but the frame > > > context could already be initialized by the FCQ > > > - computeParams/processStats won't have valid controls but the frame > > > context might already be initialised. Here I'm not sure if we need > > > > this should be "might have not been initialised" as we're dealing with > > the case where prepare/process for frame X are called before the > > corresponding request X has been queued. > > > > > to even try to call algo->queueRequest() because there won't be > > > controls associated with this frame context. > > > > > > All in all, I'm wondering if initializeFrameContext() is not actually about only > > > queueing controls to the algos, and we can even skip calling it from > > > computeParams/processStats for which the correct initialisation of the frame > > > context is already performed by the FCQueue. > > I think I miss something here. I don't think we can skip calling > initializeFrameContext() for computeParams/processStats because it > ensures that algo->queueRequest() is called for the FC. I think Right, I think I realized that in my last reply > algo->queueRequest() should be renamed to algo->initializeRequest() > which is the actual intent. The FCQueue only zeroes the frame context > but actual initialization happens in algo->queueRequest(). > > > > > I slept over this a bit.. I think we should slightly re-work the FCQ > > to make this a bit nicer. > > I completely agree. I'm not happy with the design of the FCQueue. I > tried to apply modifications in baby steps to ensure I didn't miss > anything in the big picture. But there is still room for further > improvements. > > > > > I think the first thing to do would be to propagate to IPAs the > > "error" conditions from FCQueue::alloc() and ::get(). > > > > This would help you detect if an unexpected sequence of events has > > happened and act accordingly. > > > > Let's leave out start() for a bit (I would like to clarify, here on in > > the other patch if start() should call alloc() or not, but that's > > unrelated). Let's only consider queueRequest()/process()/prepare(). > > > > In normal operating conditions, with user queueing requests fast > > enough to sustain the sensor frame rate, as we now clock the IPA with > > frames from the sensor, and we have no guarantee that requests will be > > queued fast enough to sustain this. > > > > > > - IPA::queueRequest(Request:X) > > - FCQueue::alloc(X): > > - frame context already initialized > > - This means that prepare/process have already been called > > - The Request is late and the corresponding frame has already > > been produced > > - We need to accummulate the Request's controls in the list of > > controls to apply to the "next" algo->queueRequest() loop > > - frame context not initialized > > - Normal operating conditions > > - Call algo->queueRequest(request->controls()) > > > > - computeParams(Frame:X) > > - FCQeueue::get(X) > > - frame context already initialized > > - Normal operating conditions > > - call algos->prepare() > > - frame context not initialized > > - User request underrun > > - if any pending controls for late requests are present we > > should handle them: call algo->queueRequest() to provide > > them the pending controls we have accumulated > > > > - processStats(Frame:X) > > - same as computeParams(X) > > > > This is what I think it is implemented in this series ? Do you agree > > or I missed anything ? > > I think that's it, yes. > > > > > If that's the case, wouldn't it be nicer if we could do > > > > IPA::queueRequest(request) { > > auto { fc, err } = > > context_.frameContexts.alloc(request->sequece); > > This is a tiny bit misleading as it is not the request sequence, but > sensor frame and controls. > That's something else that I think we should better clarify. Params and stats have the frame sequence as it comes from the kernel as identifier. Request has a sequence number that is unrelated to the frame sequence number. I have not absorbed patch 27 yet, but I presume that's where you associated a request with the most up to date frame sequence ? if that's not the case, how can requests be associated with frame numbers as the two sequences are not related in any way ? > > if (err == REQUEST_UNDERRUN) { > > /* Accumulate controls in the list of pending controls > > * because the request is late */ > > pendingControls.merge(request->controls); > > } > > > > queueRequestToAlgo(request->controls); > > } > > > > > > IPA::computeParams(frame) { > > auto { fc, err } = context_.frameContexts.get(frame); > > if (err == REQUEST_UNDERRUN) { > > queueRequestToAlgo(pendingControls); > > } > > > > for (algo : algos) { > > algo->prepare(); > > } > > > > setSensorControls(); > > } > > > > IPA::processStats(frame, sensorControls) { > > auto { fc, err } = context_.frameContexts.get(frame); > > if (err == REQUEST_UNDERRUN) { > > queueRequestToAlgo(pendingControls); > > } > > > > updateSensorControls(sensorControls); > > > > for (algo : algos) { > > algo->process(); > > } > > > > metadataReady.emit(); > > } > > > > The only error condition from FCQueue is then a request underrun > > (apart from the Fatal 'overwrite' error), which corresponds to the two > > conditions now demoted to Debug messages in alloc() and get() > > > > in case of underruns we handle the list of pending controls, either by > > accummulating them in queueRequest() or by sending them to algos in > > computeParams and processStats. > > > > Does this match the mental model you have implemented in this series > > or have I missed some parts ? > > I think you got that all right. I'm not sure if I like that style better > as it adds more control logic into computeParams()/processStats() and > assumes that the FCQueue has knowledge of the expected order of actions. My main goal here is to push as much of the design in the common libipa part and make the IPA module as easy to implement as possible. The amount of logic implemented in IPARkISP1::initializeFrameContext() in this series is quite dense if (frameContext.initialised) { if (!controls.empty()) { } return; } const ControlList *controls2 = &controls; if (!controlsToApply_.empty()) { } frameContext.initialised = true; for (auto const &a : algorithms()) { Algorithm *algo = static_cast<Algorithm *>(a.get()); if (algo->disabled_) continue; algo->queueRequest(context_, frame, frameContext, *controls2); } controlsToApply_.clear(); And will have to be verbatim replicated in each IPA (I guess). I would be curious to check if the above proposed model which revolves around the concept of queuing controls to the algos with a single queueRequestToAlgo() function that has to be called by prepare()/process() only in case of FCQueue error would result in less logic to replicate in other IPAs. > But I agree that you could get rid of the outer initialized member in > that case. > > I'd like the FCQueue to be as simple as possible. Continuing on your ehehe I'm fine paying the price of a more complex FCQueue if the logic in the single IPAs is reduced :) > thoughts what about removing the FCQueue::alloc all together? > > If we replace alloc+get by a single get(): > > auto { fc, did_allocate } = context_.frameContexts.get(frame); > we could move all the debug messages out of the FCQueue into the IPA. I considered this as well. However alloc() and get() have different semantics in my opinion, mostly around the idea that alloc() should be called before get() in "normal" operations. All our PFC designs revolves around the idea that there is a normal operating condition where requests are queued fast enough to sustain the frame rate. In case this doesn't happen we have to handle an "underrun" as an error condition. Now, we can re-discuss this, but seeing that in case of underrun the IPA has to take counter-measures to either to accumulate controls for late requests or feed the algos with any pending controls for early frames [*] makes me think as these two as "error" conditions. [*] I quite like the idea of collapsing the FCQueue error to a single underrun condition. In the end a late request at queueRequest() time means we had a computeParams() for a frame for which the application has not supplied a Request (yet). And the error paths are effectively the ones described above: - late request: accumulate the controls in a pendingControls_ list - early computeParams(): queue any pendingControls_ to algos to maintain their status up-to-date with users' request In the end, if we have an early computeParams() we'll always have a late queueRequest() and we can only mitigate this by pushing the missed controls to algos as soon as we can. There might be an argument here if a late queueRequest() should push pendingControls_ (before merging its own controls there) > > We can then pass the did_allocate to initializeFrameContext() which > would be close to your proposal or stick to the FC.initialized member > which I somehow still like better. But that is a technical nuance. > > > > > One part it is not totally clear to me in this series is if > > computeParams/processStats for a frame will always be called in > > sequence or not, and if not, if there's anything to do to address this > > condition. > > I wouldn't make much guarantees here. But I can't come up with a > condition where processStats is called without a prior computeParams. A If a pipeline is designed correctly, this shouldn't happen, right ? We can even assert if we detect this condition and check if it ever happens at runtime ? > computeParams() on a frame after processStats was called on that frame > should never happen. > > Best regards, > Stefan > > > > > Thanks > > j > > > > > > > > > }; > > > > > > > > struct IPAContext { > > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > > > > index 01b30c947a0a..23d80bc43c5d 100644 > > > > --- a/src/ipa/rkisp1/rkisp1.cpp > > > > +++ b/src/ipa/rkisp1/rkisp1.cpp > > > > @@ -343,6 +343,10 @@ void IPARkISP1::initializeFrameContext(const uint32_t frame, > > > > IPAFrameContext &frameContext, > > > > const ControlList &controls) > > > > { > > > > + if (frameContext.initialised) > > > > + return; > > > > + > > > > + frameContext.initialised = true; > > > > for (auto const &a : algorithms()) { > > > > Algorithm *algo = static_cast<Algorithm *>(a.get()); > > > > if (algo->disabled_) > > > > @@ -354,6 +358,7 @@ void IPARkISP1::initializeFrameContext(const uint32_t frame, > > > > void IPARkISP1::computeParams(const uint32_t frame, const uint32_t bufferId) > > > > { > > > > IPAFrameContext &frameContext = context_.frameContexts.get(frame); > > > > + initializeFrameContext(frame, frameContext, {}); > > > > > > > > /* > > > > * \todo: This needs discussion. In raw mode, computeParams is > > > > @@ -383,6 +388,7 @@ void IPARkISP1::processStats(const uint32_t frame, const uint32_t bufferId, > > > > const ControlList &sensorControls) > > > > { > > > > IPAFrameContext &frameContext = context_.frameContexts.get(frame); > > > > + initializeFrameContext(frame, frameContext, {}); > > > > > > > > /* > > > > * In raw capture mode, the ISP is bypassed and no statistics buffer is > > > > -- > > > > 2.48.1 > > > >
Hi Jacopo, Quoting Jacopo Mondi (2025-11-08 11:56:53) > Hi Stefan > > On Fri, Nov 07, 2025 at 11:41:15AM +0100, Stefan Klug wrote: > > Hi Jacopo, > > > > Thanks for looking deep into this. > > > > Quoting Jacopo Mondi (2025-11-07 09:02:48) > > > Hi again Stefan > > > > > > On Thu, Nov 06, 2025 at 07:44:57PM +0100, Jacopo Mondi wrote: > > > > Hi Stefan > > > > > > > > On Fri, Oct 24, 2025 at 10:50:47AM +0200, Stefan Klug wrote: > > > > > For per frame control we want to tick the IPA by the sensor frame > > > > > sequence instead of the request frame sequence. This has the side effect > > > > > that the IPA must be able to cope with situations where a frame context > > > > > is required for a frame that was not queued before (computeParams is > > > > > called without a corresponding request) or processStats is called for an > > > > > unexpected sequence number (because a scratch buffer was used on kernel > > > > > side) > > > > > > > > > > Prepare for that by allowing the frame context to be initialized on demand. > > > > > > > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > > > > --- > > > > > src/ipa/rkisp1/algorithms/awb.cpp | 2 ++ > > > > > src/ipa/rkisp1/ipa_context.h | 2 ++ > > > > > src/ipa/rkisp1/rkisp1.cpp | 6 ++++++ > > > > > 3 files changed, 10 insertions(+) > > > > > > > > > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp > > > > > index 399fb51be414..27109478c340 100644 > > > > > --- a/src/ipa/rkisp1/algorithms/awb.cpp > > > > > +++ b/src/ipa/rkisp1/algorithms/awb.cpp > > > > > @@ -172,6 +172,8 @@ void Awb::queueRequest(IPAContext &context, > > > > > awbAlgo_->handleControls(controls); > > > > > > > > > > frameContext.awb.autoEnabled = awb.autoEnabled; > > > > > + frameContext.awb.gains = awb.automatic.gains; > > > > > + frameContext.awb.temperatureK = awb.automatic.temperatureK; > > > > > > > > unrelated ? > > > > Ooops :-) > > > > > > > > > > > > > > > > if (awb.autoEnabled) > > > > > return; > > > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h > > > > > index f85a130d9c23..185951fb8286 100644 > > > > > --- a/src/ipa/rkisp1/ipa_context.h > > > > > +++ b/src/ipa/rkisp1/ipa_context.h > > > > > @@ -214,6 +214,8 @@ struct IPAFrameContext : public FrameContext { > > > > > double strength; > > > > > double gain; > > > > > } wdr; > > > > > + > > > > > + bool initialised; > > > > > > > > As already pointed out, the base struct already has an initialised > > > > member, an indication that something could maybe be improved. > > > > > > > > the here introduced initializeFrameContext() > > > > is called from 4 places: > > > > > > > > - start() [where you should call alloc() imho; with valid controls] > > > > - queueRequest() [which calls alloc(); with valid controls] > > > > - computeParams() [which calls get(); without controls] > > > > - processStats() [which calls get(); without controls) > > > > > > > > If I'm not mistaken > > > > > > > > - start() will always be called before queueRequest and the frame > > > > context won't be initialized, but controls need to be queued to > > > > algorithms > > > > - queueRequest() will be called with valid controls, but the frame > > > > context could already be initialized by the FCQ > > > > - computeParams/processStats won't have valid controls but the frame > > > > context might already be initialised. Here I'm not sure if we need > > > > > > this should be "might have not been initialised" as we're dealing with > > > the case where prepare/process for frame X are called before the > > > corresponding request X has been queued. > > > > > > > to even try to call algo->queueRequest() because there won't be > > > > controls associated with this frame context. > > > > > > > > All in all, I'm wondering if initializeFrameContext() is not actually about only > > > > queueing controls to the algos, and we can even skip calling it from > > > > computeParams/processStats for which the correct initialisation of the frame > > > > context is already performed by the FCQueue. > > > > I think I miss something here. I don't think we can skip calling > > initializeFrameContext() for computeParams/processStats because it > > ensures that algo->queueRequest() is called for the FC. I think > > Right, I think I realized that in my last reply > > > algo->queueRequest() should be renamed to algo->initializeRequest() > > which is the actual intent. The FCQueue only zeroes the frame context > > but actual initialization happens in algo->queueRequest(). > > > > > > > > I slept over this a bit.. I think we should slightly re-work the FCQ > > > to make this a bit nicer. > > > > I completely agree. I'm not happy with the design of the FCQueue. I > > tried to apply modifications in baby steps to ensure I didn't miss > > anything in the big picture. But there is still room for further > > improvements. > > > > > > > > I think the first thing to do would be to propagate to IPAs the > > > "error" conditions from FCQueue::alloc() and ::get(). > > > > > > This would help you detect if an unexpected sequence of events has > > > happened and act accordingly. > > > > > > Let's leave out start() for a bit (I would like to clarify, here on in > > > the other patch if start() should call alloc() or not, but that's > > > unrelated). Let's only consider queueRequest()/process()/prepare(). > > > > > > In normal operating conditions, with user queueing requests fast > > > enough to sustain the sensor frame rate, as we now clock the IPA with > > > frames from the sensor, and we have no guarantee that requests will be > > > queued fast enough to sustain this. > > > > > > > > > - IPA::queueRequest(Request:X) > > > - FCQueue::alloc(X): > > > - frame context already initialized > > > - This means that prepare/process have already been called > > > - The Request is late and the corresponding frame has already > > > been produced > > > - We need to accummulate the Request's controls in the list of > > > controls to apply to the "next" algo->queueRequest() loop > > > - frame context not initialized > > > - Normal operating conditions > > > - Call algo->queueRequest(request->controls()) > > > > > > - computeParams(Frame:X) > > > - FCQeueue::get(X) > > > - frame context already initialized > > > - Normal operating conditions > > > - call algos->prepare() > > > - frame context not initialized > > > - User request underrun > > > - if any pending controls for late requests are present we > > > should handle them: call algo->queueRequest() to provide > > > them the pending controls we have accumulated > > > > > > - processStats(Frame:X) > > > - same as computeParams(X) > > > > > > This is what I think it is implemented in this series ? Do you agree > > > or I missed anything ? > > > > I think that's it, yes. > > > > > > > > If that's the case, wouldn't it be nicer if we could do > > > > > > IPA::queueRequest(request) { > > > auto { fc, err } = > > > context_.frameContexts.alloc(request->sequece); > > > > This is a tiny bit misleading as it is not the request sequence, but > > sensor frame and controls. > > > > That's something else that I think we should better clarify. > > Params and stats have the frame sequence as it comes from the kernel > as identifier. > > Request has a sequence number that is unrelated to the frame sequence > number. > > I have not absorbed patch 27 yet, but I presume that's where you > associated a request with the most up to date frame sequence ? > > if that's not the case, how can requests be associated with frame > numbers as the two sequences are not related in any way ? Yes, that is done in that patch. For the happy path we can also assume that request->sequence == frame sequence. For me the important aspect is that the IPA has no notion of a request (aside from that function beeing called queueRequest and probably should be renamed to initializeFrameContext). So the IPA handles only sensor sequence numbers and controls. > > > > > if (err == REQUEST_UNDERRUN) { > > > /* Accumulate controls in the list of pending controls > > > * because the request is late */ > > > pendingControls.merge(request->controls); > > > } > > > > > > queueRequestToAlgo(request->controls); > > > } > > > > > > > > > IPA::computeParams(frame) { > > > auto { fc, err } = context_.frameContexts.get(frame); > > > if (err == REQUEST_UNDERRUN) { > > > queueRequestToAlgo(pendingControls); > > > } > > > > > > for (algo : algos) { > > > algo->prepare(); > > > } > > > > > > setSensorControls(); > > > } > > > > > > IPA::processStats(frame, sensorControls) { > > > auto { fc, err } = context_.frameContexts.get(frame); > > > if (err == REQUEST_UNDERRUN) { > > > queueRequestToAlgo(pendingControls); > > > } > > > > > > updateSensorControls(sensorControls); > > > > > > for (algo : algos) { > > > algo->process(); > > > } > > > > > > metadataReady.emit(); > > > } > > > > > > The only error condition from FCQueue is then a request underrun > > > (apart from the Fatal 'overwrite' error), which corresponds to the two > > > conditions now demoted to Debug messages in alloc() and get() > > > > > > in case of underruns we handle the list of pending controls, either by > > > accummulating them in queueRequest() or by sending them to algos in > > > computeParams and processStats. > > > > > > Does this match the mental model you have implemented in this series > > > or have I missed some parts ? > > > > I think you got that all right. I'm not sure if I like that style better > > as it adds more control logic into computeParams()/processStats() and > > assumes that the FCQueue has knowledge of the expected order of actions. > > My main goal here is to push as much of the design in the common > libipa part and make the IPA module as easy to implement as possible. > > The amount of logic implemented in > IPARkISP1::initializeFrameContext() in this series is quite dense > > if (frameContext.initialised) { > if (!controls.empty()) { > } > return; > } > > const ControlList *controls2 = &controls; > if (!controlsToApply_.empty()) { > } > > frameContext.initialised = true; > for (auto const &a : algorithms()) { > Algorithm *algo = static_cast<Algorithm *>(a.get()); > if (algo->disabled_) > continue; > algo->queueRequest(context_, frame, frameContext, *controls2); > } > > controlsToApply_.clear(); > > And will have to be verbatim replicated in each IPA (I guess). > > I would be curious to check if the above proposed model which > revolves around the concept of queuing controls to the algos with a > single queueRequestToAlgo() function that has to be called by > prepare()/process() only in case of FCQueue error would result in less > logic to replicate in other IPAs. Ok, I see. Given that target it makes absolute sense to keep logic inside the FCQueue. > > > > But I agree that you could get rid of the outer initialized member in > > that case. > > > > I'd like the FCQueue to be as simple as possible. Continuing on your > > ehehe I'm fine paying the price of a more complex FCQueue if the logic > in the single IPAs is reduced :) Then why not going one step further? If we allow the FCQueue to call queueRequestToAlgo() we could move all the pending controls handling into the FCQueue. Then we could drop the repetitive error handling from the IPA implementations. Only aspect I don't like too much in that case is that the initialized-state handling of the frame context is then inside the FCQueue, so you can't use a frame context alone (as is currently done on start()). That is maybe not that big of a deal but requires the FCQueue to accept alloc beeing called 2 times for frame 0. Something like: IPA::IPA() { context_.frameContexts.setQueueCallback(IPA::queueRequestToAlgo) } IPA::start(controls) { auto fc = context_.frameContexts.alloc(0, controls); ... } IPA::queueRequest(frame, controls) { /* Note that controls is passed into FCQueue */ auto fc = context_.frameContexts.alloc(frame, controls); } IPA::computeParams(frame) { auto fc = context_.frameContexts.get(frame); for (algo : algos) { algo->prepare(); } setSensorControls(); } IPA::processStats(frame, sensorControls) { auto fc = context_.frameContexts.get(frame); for (algo : algos) { algo->process(); } metadataReady.emit(); } What do you think? Best regards, Stefan > > > thoughts what about removing the FCQueue::alloc all together? > > > > If we replace alloc+get by a single get(): > > > > auto { fc, did_allocate } = context_.frameContexts.get(frame); > > we could move all the debug messages out of the FCQueue into the IPA. > > I considered this as well. > > However alloc() and get() have different semantics in my opinion, > mostly around the idea that alloc() should be called before get() in > "normal" operations. > > All our PFC designs revolves around the idea that there is a normal > operating condition where requests are queued fast enough to sustain > the frame rate. In case this doesn't happen we have to handle an > "underrun" as an error condition. Now, we can re-discuss this, but > seeing that in case of underrun the IPA has to take counter-measures > to either to accumulate controls for late requests or feed the algos > with any pending controls for early frames [*] makes me think as these > two as "error" conditions. > > [*] I quite like the idea of collapsing the FCQueue error to a single > underrun condition. In the end a late request at queueRequest() time > means we had a computeParams() for a frame for which the application > has not supplied a Request (yet). And the error paths are > effectively the ones described above: > - late request: accumulate the controls in a pendingControls_ list > - early computeParams(): queue any pendingControls_ to algos to > maintain their status up-to-date with users' request > > In the end, if we have an early computeParams() we'll always have a late > queueRequest() and we can only mitigate this by pushing the missed > controls to algos as soon as we can. There might be an argument here > if a late queueRequest() should push pendingControls_ (before merging > its own controls there) > > > > > We can then pass the did_allocate to initializeFrameContext() which > > would be close to your proposal or stick to the FC.initialized member > > which I somehow still like better. But that is a technical nuance. > > > > > > > > One part it is not totally clear to me in this series is if > > > computeParams/processStats for a frame will always be called in > > > sequence or not, and if not, if there's anything to do to address this > > > condition. > > > > I wouldn't make much guarantees here. But I can't come up with a > > condition where processStats is called without a prior computeParams. A > > If a pipeline is designed correctly, this shouldn't happen, right ? > > We can even assert if we detect this condition and check if it ever > happens at runtime ? > > > computeParams() on a frame after processStats was called on that frame > > should never happen. > > > > Best regards, > > Stefan > > > > > > > > Thanks > > > j > > > > > > > > > > > > }; > > > > > > > > > > struct IPAContext { > > > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > > > > > index 01b30c947a0a..23d80bc43c5d 100644 > > > > > --- a/src/ipa/rkisp1/rkisp1.cpp > > > > > +++ b/src/ipa/rkisp1/rkisp1.cpp > > > > > @@ -343,6 +343,10 @@ void IPARkISP1::initializeFrameContext(const uint32_t frame, > > > > > IPAFrameContext &frameContext, > > > > > const ControlList &controls) > > > > > { > > > > > + if (frameContext.initialised) > > > > > + return; > > > > > + > > > > > + frameContext.initialised = true; > > > > > for (auto const &a : algorithms()) { > > > > > Algorithm *algo = static_cast<Algorithm *>(a.get()); > > > > > if (algo->disabled_) > > > > > @@ -354,6 +358,7 @@ void IPARkISP1::initializeFrameContext(const uint32_t frame, > > > > > void IPARkISP1::computeParams(const uint32_t frame, const uint32_t bufferId) > > > > > { > > > > > IPAFrameContext &frameContext = context_.frameContexts.get(frame); > > > > > + initializeFrameContext(frame, frameContext, {}); > > > > > > > > > > /* > > > > > * \todo: This needs discussion. In raw mode, computeParams is > > > > > @@ -383,6 +388,7 @@ void IPARkISP1::processStats(const uint32_t frame, const uint32_t bufferId, > > > > > const ControlList &sensorControls) > > > > > { > > > > > IPAFrameContext &frameContext = context_.frameContexts.get(frame); > > > > > + initializeFrameContext(frame, frameContext, {}); > > > > > > > > > > /* > > > > > * In raw capture mode, the ISP is bypassed and no statistics buffer is > > > > > -- > > > > > 2.48.1 > > > > >
diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp index 399fb51be414..27109478c340 100644 --- a/src/ipa/rkisp1/algorithms/awb.cpp +++ b/src/ipa/rkisp1/algorithms/awb.cpp @@ -172,6 +172,8 @@ void Awb::queueRequest(IPAContext &context, awbAlgo_->handleControls(controls); frameContext.awb.autoEnabled = awb.autoEnabled; + frameContext.awb.gains = awb.automatic.gains; + frameContext.awb.temperatureK = awb.automatic.temperatureK; if (awb.autoEnabled) return; diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h index f85a130d9c23..185951fb8286 100644 --- a/src/ipa/rkisp1/ipa_context.h +++ b/src/ipa/rkisp1/ipa_context.h @@ -214,6 +214,8 @@ struct IPAFrameContext : public FrameContext { double strength; double gain; } wdr; + + bool initialised; }; struct IPAContext { diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp index 01b30c947a0a..23d80bc43c5d 100644 --- a/src/ipa/rkisp1/rkisp1.cpp +++ b/src/ipa/rkisp1/rkisp1.cpp @@ -343,6 +343,10 @@ void IPARkISP1::initializeFrameContext(const uint32_t frame, IPAFrameContext &frameContext, const ControlList &controls) { + if (frameContext.initialised) + return; + + frameContext.initialised = true; for (auto const &a : algorithms()) { Algorithm *algo = static_cast<Algorithm *>(a.get()); if (algo->disabled_) @@ -354,6 +358,7 @@ void IPARkISP1::initializeFrameContext(const uint32_t frame, void IPARkISP1::computeParams(const uint32_t frame, const uint32_t bufferId) { IPAFrameContext &frameContext = context_.frameContexts.get(frame); + initializeFrameContext(frame, frameContext, {}); /* * \todo: This needs discussion. In raw mode, computeParams is @@ -383,6 +388,7 @@ void IPARkISP1::processStats(const uint32_t frame, const uint32_t bufferId, const ControlList &sensorControls) { IPAFrameContext &frameContext = context_.frameContexts.get(frame); + initializeFrameContext(frame, frameContext, {}); /* * In raw capture mode, the ISP is bypassed and no statistics buffer is
For per frame control we want to tick the IPA by the sensor frame sequence instead of the request frame sequence. This has the side effect that the IPA must be able to cope with situations where a frame context is required for a frame that was not queued before (computeParams is called without a corresponding request) or processStats is called for an unexpected sequence number (because a scratch buffer was used on kernel side) Prepare for that by allowing the frame context to be initialized on demand. Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> --- src/ipa/rkisp1/algorithms/awb.cpp | 2 ++ src/ipa/rkisp1/ipa_context.h | 2 ++ src/ipa/rkisp1/rkisp1.cpp | 6 ++++++ 3 files changed, 10 insertions(+)