| Message ID | 20250927180004.84620-1-hansg@kernel.org |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hans, Thank you for the patches. On Sat, Sep 27, 2025 at 07:59:58PM +0200, Hans de Goede wrote: > Hi All, > > Due to a combination of not having correct control-delay information for > various sensors as well as the generic nature of the simple-pipeline + > software-ISP meaning that any pipeline delays are unknown it is impossible > to get reliable control-delay values. I'm fine with this patch series as a short term workaround for the time being. The next step is to turn the "impossible" into "done". Delay characterization of sensors is something we need to implement, and that's just of matter of getting it done (i.e. a matter of time). What other issues are there to fix ? > Wrong control-delay values can lead to pretty bad oscilation. See the v1 > cover-letter for more details. > > This series fixes some unrelated issues in the softIPA AGC algorithm and > in the last 2 patches addresses the oscillation issue. Note v2 and later > take a new approach to fixing the oscilation by only generating statistics > for every 4th frame and having the IPA only run its algorithms when there > actually are statistics. This saves CPU time, while at the same time > avoiding the oscillation issue. > > v3 has successfully passed CI, see: > https://gitlab.freedesktop.org/camera/libcamera-softisp/-/commit/76f37231527d10399bc123a546f8e252faaa8310/pipelines?ref=softisp-agc-oscillation-fixes-v3 > > Changes in v3: > - Drop if (!stats_->valid) early exit from IPASoftSimple::processStats() > - Save last AGC calculated new gain and exposure values and re-use these > for frames without statistics, this avoids delayedCtrl history underruns > - Improve SwIspStats.valid documentation > > Changes in v2: > - Skip running IPA algorithms when there are no statistics > - Only generate statistics for every 4th frame > > Note the speed of the AGC algorithm converges on the desired brightness > level is unchanged compared to v1 since in v1 the AGC algorithm would > skip 3 frames after every gain/exposure change. > > Regards, > > Hans > > > > Hans de Goede (6): > ipa: software_isp: Fix context_.configuration.agc.againMin init > ipa: software_isp: AGC: Do not lower gain below 1.0 > ipa: software_isp: AGC: Raise exposure or gain not both at the same > time > ipa: software_isp: AGC: Only use integers for exposure calculations > libcamera: software_isp: Add valid flag to struct SwIspStats > libcamera: software_isp: Run sw-statistics once every 4th frame > > .../internal/software_isp/swisp_stats.h | 5 ++++ > src/ipa/simple/algorithms/agc.cpp | 26 ++++++++++--------- > src/ipa/simple/algorithms/awb.cpp | 3 +++ > src/ipa/simple/algorithms/blc.cpp | 3 +++ > src/ipa/simple/ipa_context.h | 2 +- > src/ipa/simple/soft_simple.cpp | 7 ++++- > src/libcamera/software_isp/debayer_cpu.cpp | 25 +++++++++++------- > src/libcamera/software_isp/debayer_cpu.h | 4 +-- > src/libcamera/software_isp/swstats_cpu.cpp | 9 ++++++- > src/libcamera/software_isp/swstats_cpu.h | 5 +++- > 10 files changed, 61 insertions(+), 28 deletions(-)
Hi, On 30-Sep-25 12:06, Laurent Pinchart wrote: > Hans, > > Thank you for the patches. > > On Sat, Sep 27, 2025 at 07:59:58PM +0200, Hans de Goede wrote: >> Hi All, >> >> Due to a combination of not having correct control-delay information for >> various sensors as well as the generic nature of the simple-pipeline + >> software-ISP meaning that any pipeline delays are unknown it is impossible >> to get reliable control-delay values. > > I'm fine with this patch series as a short term workaround for the time > being. The next step is to turn the "impossible" into "done". Delay > characterization of sensors is something we need to implement, and > that's just of matter of getting it done (i.e. a matter of time). What > other issues are there to fix ? I have the feeling that with some CSI receivers when we get the SOF event and apply new sensor controls the controls do not get applied to the frame we think it does. I think some existing handlers already modify the sensor delays before passing them to delayedctrls for similar reasons ? We may need to do the same in the simple pipeline, adding a per CSI2 receiver driver setting for this to the existing per CSI2-receiver parameters in the simple pipeline. Regards, Hans >> Wrong control-delay values can lead to pretty bad oscilation. See the v1 >> cover-letter for more details. >> >> This series fixes some unrelated issues in the softIPA AGC algorithm and >> in the last 2 patches addresses the oscillation issue. Note v2 and later >> take a new approach to fixing the oscilation by only generating statistics >> for every 4th frame and having the IPA only run its algorithms when there >> actually are statistics. This saves CPU time, while at the same time >> avoiding the oscillation issue. >> >> v3 has successfully passed CI, see: >> https://gitlab.freedesktop.org/camera/libcamera-softisp/-/commit/76f37231527d10399bc123a546f8e252faaa8310/pipelines?ref=softisp-agc-oscillation-fixes-v3 >> >> Changes in v3: >> - Drop if (!stats_->valid) early exit from IPASoftSimple::processStats() >> - Save last AGC calculated new gain and exposure values and re-use these >> for frames without statistics, this avoids delayedCtrl history underruns >> - Improve SwIspStats.valid documentation >> >> Changes in v2: >> - Skip running IPA algorithms when there are no statistics >> - Only generate statistics for every 4th frame >> >> Note the speed of the AGC algorithm converges on the desired brightness >> level is unchanged compared to v1 since in v1 the AGC algorithm would >> skip 3 frames after every gain/exposure change. >> >> Regards, >> >> Hans >> >> >> >> Hans de Goede (6): >> ipa: software_isp: Fix context_.configuration.agc.againMin init >> ipa: software_isp: AGC: Do not lower gain below 1.0 >> ipa: software_isp: AGC: Raise exposure or gain not both at the same >> time >> ipa: software_isp: AGC: Only use integers for exposure calculations >> libcamera: software_isp: Add valid flag to struct SwIspStats >> libcamera: software_isp: Run sw-statistics once every 4th frame >> >> .../internal/software_isp/swisp_stats.h | 5 ++++ >> src/ipa/simple/algorithms/agc.cpp | 26 ++++++++++--------- >> src/ipa/simple/algorithms/awb.cpp | 3 +++ >> src/ipa/simple/algorithms/blc.cpp | 3 +++ >> src/ipa/simple/ipa_context.h | 2 +- >> src/ipa/simple/soft_simple.cpp | 7 ++++- >> src/libcamera/software_isp/debayer_cpu.cpp | 25 +++++++++++------- >> src/libcamera/software_isp/debayer_cpu.h | 4 +-- >> src/libcamera/software_isp/swstats_cpu.cpp | 9 ++++++- >> src/libcamera/software_isp/swstats_cpu.h | 5 +++- >> 10 files changed, 61 insertions(+), 28 deletions(-) >
Hans, On Tue, Sep 30, 2025 at 04:04:13PM +0200, Hans de Goede wrote: > On 30-Sep-25 12:06, Laurent Pinchart wrote: > > On Sat, Sep 27, 2025 at 07:59:58PM +0200, Hans de Goede wrote: > >> Hi All, > >> > >> Due to a combination of not having correct control-delay information for > >> various sensors as well as the generic nature of the simple-pipeline + > >> software-ISP meaning that any pipeline delays are unknown it is impossible > >> to get reliable control-delay values. > > > > I'm fine with this patch series as a short term workaround for the time > > being. The next step is to turn the "impossible" into "done". Delay > > characterization of sensors is something we need to implement, and > > that's just of matter of getting it done (i.e. a matter of time). What > > other issues are there to fix ? > > I have the feeling that with some CSI receivers when we get the SOF > event and apply new sensor controls the controls do not get applied > to the frame we think it does. The SOF interrupt should come at start of frame. If we apply controls right away, they should be applied on the next frame + sensor delays. I of course won't rule out bugs in CSI-2 receiver drivers that would for instance cause the SOF signal to be delivered at end of frame. If that's the case we should fix the drivers. > I think some existing handlers already modify the sensor delays > before passing them to delayedctrls for similar reasons ? Even if controls are applied when expected, what needs to be taken into account is the consumption of the data by the IPA module. When a frame is captured and statstics are computed, the AGC algorithm needs to process those statistics with the sensor parameters that have been applied to the frame. That's an area where bugs can occur in pipeline handlers and IPA modules, using the sensor parameters for the wrong frame. > We may need to do the same in the simple pipeline, adding > a per CSI2 receiver driver setting for this to the existing > per CSI2-receiver parameters in the simple pipeline. Hopefully it won't need to be per CSI-2 receiver. Thanks for the explanation. When Kieran (or someone else) will find time to finish the work he started to port the soft ISP to the AGC algorithm from libipa we'll have to revisit this issue. That will be a good time to drop the hack in the last patch of this series. > >> Wrong control-delay values can lead to pretty bad oscilation. See the v1 > >> cover-letter for more details. > >> > >> This series fixes some unrelated issues in the softIPA AGC algorithm and > >> in the last 2 patches addresses the oscillation issue. Note v2 and later > >> take a new approach to fixing the oscilation by only generating statistics > >> for every 4th frame and having the IPA only run its algorithms when there > >> actually are statistics. This saves CPU time, while at the same time > >> avoiding the oscillation issue. > >> > >> v3 has successfully passed CI, see: > >> https://gitlab.freedesktop.org/camera/libcamera-softisp/-/commit/76f37231527d10399bc123a546f8e252faaa8310/pipelines?ref=softisp-agc-oscillation-fixes-v3 > >> > >> Changes in v3: > >> - Drop if (!stats_->valid) early exit from IPASoftSimple::processStats() > >> - Save last AGC calculated new gain and exposure values and re-use these > >> for frames without statistics, this avoids delayedCtrl history underruns > >> - Improve SwIspStats.valid documentation > >> > >> Changes in v2: > >> - Skip running IPA algorithms when there are no statistics > >> - Only generate statistics for every 4th frame > >> > >> Note the speed of the AGC algorithm converges on the desired brightness > >> level is unchanged compared to v1 since in v1 the AGC algorithm would > >> skip 3 frames after every gain/exposure change. > >> > >> Regards, > >> > >> Hans > >> > >> Hans de Goede (6): > >> ipa: software_isp: Fix context_.configuration.agc.againMin init > >> ipa: software_isp: AGC: Do not lower gain below 1.0 > >> ipa: software_isp: AGC: Raise exposure or gain not both at the same > >> time > >> ipa: software_isp: AGC: Only use integers for exposure calculations > >> libcamera: software_isp: Add valid flag to struct SwIspStats > >> libcamera: software_isp: Run sw-statistics once every 4th frame > >> > >> .../internal/software_isp/swisp_stats.h | 5 ++++ > >> src/ipa/simple/algorithms/agc.cpp | 26 ++++++++++--------- > >> src/ipa/simple/algorithms/awb.cpp | 3 +++ > >> src/ipa/simple/algorithms/blc.cpp | 3 +++ > >> src/ipa/simple/ipa_context.h | 2 +- > >> src/ipa/simple/soft_simple.cpp | 7 ++++- > >> src/libcamera/software_isp/debayer_cpu.cpp | 25 +++++++++++------- > >> src/libcamera/software_isp/debayer_cpu.h | 4 +-- > >> src/libcamera/software_isp/swstats_cpu.cpp | 9 ++++++- > >> src/libcamera/software_isp/swstats_cpu.h | 5 +++- > >> 10 files changed, 61 insertions(+), 28 deletions(-)
Hi All, Due to a combination of not having correct control-delay information for various sensors as well as the generic nature of the simple-pipeline + software-ISP meaning that any pipeline delays are unknown it is impossible to get reliable control-delay values. Wrong control-delay values can lead to pretty bad oscilation. See the v1 cover-letter for more details. This series fixes some unrelated issues in the softIPA AGC algorithm and in the last 2 patches addresses the oscillation issue. Note v2 and later take a new approach to fixing the oscilation by only generating statistics for every 4th frame and having the IPA only run its algorithms when there actually are statistics. This saves CPU time, while at the same time avoiding the oscillation issue. v3 has successfully passed CI, see: https://gitlab.freedesktop.org/camera/libcamera-softisp/-/commit/76f37231527d10399bc123a546f8e252faaa8310/pipelines?ref=softisp-agc-oscillation-fixes-v3 Changes in v3: - Drop if (!stats_->valid) early exit from IPASoftSimple::processStats() - Save last AGC calculated new gain and exposure values and re-use these for frames without statistics, this avoids delayedCtrl history underruns - Improve SwIspStats.valid documentation Changes in v2: - Skip running IPA algorithms when there are no statistics - Only generate statistics for every 4th frame Note the speed of the AGC algorithm converges on the desired brightness level is unchanged compared to v1 since in v1 the AGC algorithm would skip 3 frames after every gain/exposure change. Regards, Hans Hans de Goede (6): ipa: software_isp: Fix context_.configuration.agc.againMin init ipa: software_isp: AGC: Do not lower gain below 1.0 ipa: software_isp: AGC: Raise exposure or gain not both at the same time ipa: software_isp: AGC: Only use integers for exposure calculations libcamera: software_isp: Add valid flag to struct SwIspStats libcamera: software_isp: Run sw-statistics once every 4th frame .../internal/software_isp/swisp_stats.h | 5 ++++ src/ipa/simple/algorithms/agc.cpp | 26 ++++++++++--------- src/ipa/simple/algorithms/awb.cpp | 3 +++ src/ipa/simple/algorithms/blc.cpp | 3 +++ src/ipa/simple/ipa_context.h | 2 +- src/ipa/simple/soft_simple.cpp | 7 ++++- src/libcamera/software_isp/debayer_cpu.cpp | 25 +++++++++++------- src/libcamera/software_isp/debayer_cpu.h | 4 +-- src/libcamera/software_isp/swstats_cpu.cpp | 9 ++++++- src/libcamera/software_isp/swstats_cpu.h | 5 +++- 10 files changed, 61 insertions(+), 28 deletions(-)