| Message ID | 20250617082956.5699-1-david.plowman@raspberrypi.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Quoting David Plowman (2025-06-17 09:29:48) > Hi everyone > > Here's a patch set that originated way back at the end of 2023! I > remember thinking "oh, I'll send these patches once Pi 5 support has > been merged", thinking it would only be a few weeks. How young and > foolish I was! > > Anyway, it was so long ago now, that I felt rather like I was > reviewing a patch set from another person - "past David", I suppose. > > The first 3 patches are small technical or cosmetic improvements, not > of any particularly great consequence. Each is standalone. I think I'm > happy that past David seemed to know what he was doing. That's practically a review by tag from you for the series right ;-) I think reviewing your own patches more than three months later counts as a different person really :-) > The remaining 4 patches are the important ones. Our AEC/AGC has always > had a problem because the digital gain was set in its prepare() > method. The catch is that if we're running at high framerates, we > frequently skip the IPAs altogether, meaning the digital gain that is > required - because it's making up for a deficit in exposure - doesn't > get applied. This causes some sometimes very obvious "winks". > > In these 4 last patches, calculation of the digital gain is moved out > of prepare() to process(), meaning it's always available. And the IPA > code is refactored slightly so that the digital gain is always > applied, even when the rest of AEC/AGC is skipped. > > I hope it makes reasonable sense - there is some fiddly stuff going on > here. Hats off to past David! Awesome ;-) Are these already existing in the RPi fork ? (I.e. are they so thoroughly tested, they should just be merged already?) Do you prefer an explicit ack/rb tag from Naush or anyone else from RPi as it's under the RPi namespace? - Nothing looks scary in the series to me ... -- Kieran > > Thanks! > > Present-day David > > > David Plowman (7): > ipa: rpi: agc: Change handling of colour gains less than 1 > ipa: rpi: agc: Make the maximum digital gain configurable > ipa: rpi: agc: Rename "analogue gain" to "gain" where appropriate > ipa: rpi: Advance the delay context counter even when IPAs don't run > ipa: rpi: agc: Calculate digital gain in process() > ipa: rpi: Update digital gain handling in IPA base and derived classes > ipa: rpi: agc: Remove digital gain from AgcPrepareStatus > > src/ipa/rpi/common/ipa_base.cpp | 72 ++++--- > src/ipa/rpi/common/ipa_base.h | 6 +- > src/ipa/rpi/controller/agc_algorithm.h | 2 +- > src/ipa/rpi/controller/agc_status.h | 4 +- > src/ipa/rpi/controller/rpi/agc.cpp | 6 +- > src/ipa/rpi/controller/rpi/agc.h | 4 +- > src/ipa/rpi/controller/rpi/agc_channel.cpp | 236 +++++++++------------ > src/ipa/rpi/controller/rpi/agc_channel.h | 6 +- > src/ipa/rpi/pisp/pisp.cpp | 81 ++++--- > src/ipa/rpi/vc4/vc4.cpp | 42 +++- > 10 files changed, 248 insertions(+), 211 deletions(-) > > -- > 2.39.5 >
On Thu, 10 Jul 2025 at 08:56, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Quoting David Plowman (2025-06-17 09:29:48) > > Hi everyone > > > > Here's a patch set that originated way back at the end of 2023! I > > remember thinking "oh, I'll send these patches once Pi 5 support has > > been merged", thinking it would only be a few weeks. How young and > > foolish I was! > > > > Anyway, it was so long ago now, that I felt rather like I was > > reviewing a patch set from another person - "past David", I suppose. > > > > The first 3 patches are small technical or cosmetic improvements, not > > of any particularly great consequence. Each is standalone. I think I'm > > happy that past David seemed to know what he was doing. > > That's practically a review by tag from you for the series right ;-) I > think reviewing your own patches more than three months later counts as > a different person really :-) > > > > The remaining 4 patches are the important ones. Our AEC/AGC has always > > had a problem because the digital gain was set in its prepare() > > method. The catch is that if we're running at high framerates, we > > frequently skip the IPAs altogether, meaning the digital gain that is > > required - because it's making up for a deficit in exposure - doesn't > > get applied. This causes some sometimes very obvious "winks". > > > > In these 4 last patches, calculation of the digital gain is moved out > > of prepare() to process(), meaning it's always available. And the IPA > > code is refactored slightly so that the digital gain is always > > applied, even when the rest of AEC/AGC is skipped. > > > > I hope it makes reasonable sense - there is some fiddly stuff going on > > here. Hats off to past David! > > Awesome ;-) > > Are these already existing in the RPi fork ? (I.e. are they so > thoroughly tested, they should just be merged already?) > > Do you prefer an explicit ack/rb tag from Naush or anyone else from RPi > as it's under the RPi namespace? - Nothing looks scary in the series to > me ... Oops, I thought I had already added my tag. For the series: Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > > -- > Kieran > > > > > > Thanks! > > > > Present-day David > > > > > > David Plowman (7): > > ipa: rpi: agc: Change handling of colour gains less than 1 > > ipa: rpi: agc: Make the maximum digital gain configurable > > ipa: rpi: agc: Rename "analogue gain" to "gain" where appropriate > > ipa: rpi: Advance the delay context counter even when IPAs don't run > > ipa: rpi: agc: Calculate digital gain in process() > > ipa: rpi: Update digital gain handling in IPA base and derived classes > > ipa: rpi: agc: Remove digital gain from AgcPrepareStatus > > > > src/ipa/rpi/common/ipa_base.cpp | 72 ++++--- > > src/ipa/rpi/common/ipa_base.h | 6 +- > > src/ipa/rpi/controller/agc_algorithm.h | 2 +- > > src/ipa/rpi/controller/agc_status.h | 4 +- > > src/ipa/rpi/controller/rpi/agc.cpp | 6 +- > > src/ipa/rpi/controller/rpi/agc.h | 4 +- > > src/ipa/rpi/controller/rpi/agc_channel.cpp | 236 +++++++++------------ > > src/ipa/rpi/controller/rpi/agc_channel.h | 6 +- > > src/ipa/rpi/pisp/pisp.cpp | 81 ++++--- > > src/ipa/rpi/vc4/vc4.cpp | 42 +++- > > 10 files changed, 248 insertions(+), 211 deletions(-) > > > > -- > > 2.39.5 > >
Hi Kieran Yes, these have been running in a branch in the Pi fork for a while, though I don't think they've been on "main". But anyway, personally I think they count as well tested, though of course, until the entire world is using the new stuff in all their idiosyncratic environments you can never know 100%. Don't mind about any other tags... Naush, do you want to "ack" this set as you have tried it out as well? Thanks David On Thu, 10 Jul 2025 at 08:56, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Quoting David Plowman (2025-06-17 09:29:48) > > Hi everyone > > > > Here's a patch set that originated way back at the end of 2023! I > > remember thinking "oh, I'll send these patches once Pi 5 support has > > been merged", thinking it would only be a few weeks. How young and > > foolish I was! > > > > Anyway, it was so long ago now, that I felt rather like I was > > reviewing a patch set from another person - "past David", I suppose. > > > > The first 3 patches are small technical or cosmetic improvements, not > > of any particularly great consequence. Each is standalone. I think I'm > > happy that past David seemed to know what he was doing. > > That's practically a review by tag from you for the series right ;-) I > think reviewing your own patches more than three months later counts as > a different person really :-) > > > > The remaining 4 patches are the important ones. Our AEC/AGC has always > > had a problem because the digital gain was set in its prepare() > > method. The catch is that if we're running at high framerates, we > > frequently skip the IPAs altogether, meaning the digital gain that is > > required - because it's making up for a deficit in exposure - doesn't > > get applied. This causes some sometimes very obvious "winks". > > > > In these 4 last patches, calculation of the digital gain is moved out > > of prepare() to process(), meaning it's always available. And the IPA > > code is refactored slightly so that the digital gain is always > > applied, even when the rest of AEC/AGC is skipped. > > > > I hope it makes reasonable sense - there is some fiddly stuff going on > > here. Hats off to past David! > > Awesome ;-) > > Are these already existing in the RPi fork ? (I.e. are they so > thoroughly tested, they should just be merged already?) > > Do you prefer an explicit ack/rb tag from Naush or anyone else from RPi > as it's under the RPi namespace? - Nothing looks scary in the series to > me ... > > -- > Kieran > > > > > > Thanks! > > > > Present-day David > > > > > > David Plowman (7): > > ipa: rpi: agc: Change handling of colour gains less than 1 > > ipa: rpi: agc: Make the maximum digital gain configurable > > ipa: rpi: agc: Rename "analogue gain" to "gain" where appropriate > > ipa: rpi: Advance the delay context counter even when IPAs don't run > > ipa: rpi: agc: Calculate digital gain in process() > > ipa: rpi: Update digital gain handling in IPA base and derived classes > > ipa: rpi: agc: Remove digital gain from AgcPrepareStatus > > > > src/ipa/rpi/common/ipa_base.cpp | 72 ++++--- > > src/ipa/rpi/common/ipa_base.h | 6 +- > > src/ipa/rpi/controller/agc_algorithm.h | 2 +- > > src/ipa/rpi/controller/agc_status.h | 4 +- > > src/ipa/rpi/controller/rpi/agc.cpp | 6 +- > > src/ipa/rpi/controller/rpi/agc.h | 4 +- > > src/ipa/rpi/controller/rpi/agc_channel.cpp | 236 +++++++++------------ > > src/ipa/rpi/controller/rpi/agc_channel.h | 6 +- > > src/ipa/rpi/pisp/pisp.cpp | 81 ++++--- > > src/ipa/rpi/vc4/vc4.cpp | 42 +++- > > 10 files changed, 248 insertions(+), 211 deletions(-) > > > > -- > > 2.39.5 > >