| Message ID | 20260720-libipa-algorithms-v6-0-ececb73f97cb@ideasonboard.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Jacopo Mondi <jacopo.mondi@ideasonboard.com> writes: > The series is identical to v5 up to patch 27. The only minor difference > is that I back-tracked on making functions class members but in the > RkISP1 IPA I kept them as static helpers. > > Patches 28 and 29 are new. > > They remove the templating of the LscAlgorithm class hierarchy and > instead implement quantization in the LscAlgorithm class. > > So we now expand polynomials and parse tables as floats, store them > in a map and in LscAlgorithm we re-iterate over it to either quantized > them in the polynomial case, or simply cast floats to the register bit > width if table. > > Timing the duration of the whole RkISP1 configure operation, it now > takes 3 msec compared to 600 usec. > > RkISP1Lsc lsc.cpp:171 Configure took = 3081[µs] > > RkISP1Lsc lsc.cpp:172 Configure took = 583[µs] > > Also, now each IPA has to define their own Interpolator::interpolate() > overload (see rkisp1 IPA as an example of this). > > As the interface of LscAlgorithm has changed, patches on the list that > port the LSC algorithm of other platforms based on v5 will have to be > rebased. I'm having trouble to understand the exact motivation for the change. From the libipa consumer side, it means that for `simple' pipeline, I had to copy & substitute & paste code from rkisp1 for no very good reason. Which looks like a step against the code unification goal, while it doesn't seem to simplify rkisp1 that much. The float representation could be useful for software ISP, where there is no inherent reason to use quantized values (although they may still have its use for portable textures, as pointed out by Bryan). But it seems using quantized values is still imposed on libipa LSC consumers? > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > --- > Changes in v6: > - Address comments received on v5, mostly on documentation > - Remove templating of LscPolynomial and perform quantization in > LscAlgorithm > > Changes in v5: > - Add "ipa: libipa: lsc_polynomial: Fix polynomial parsing error" > to fix and exiting potential issue with Polynomial parsing > - Fix Simple, which I broke in v4, thanks Milan > > Changes in v4: > - s/awb/AWB in comments everywhere, same for CCM and LSC > - Awb: changelog per patch > - Make AwbStats pure virtual and move stats parsing logic to the IPA > modules > - Reduce comments/renames to reduce patch size and comments > - Collect tags > > Changes in v3: > - Address comments on Awb from Stefan: > - Introduce AwbImplementation::Result > - Move AwbStats implementation to IPAs > - Drop AwbStats::rg()/bg() and re-implement AwbGray accordingly > - Fix Saturation bug in Simple Ccm algorithm > - Fix Simple Ccm gain limits using Q<2, 8> > - Update Mali CCM and Gamma to latest uAPI patch version > - Fix Mali CCM gain masking as reported by Dan > - Break down libIPA Lsc implementation as requested by Stefan > > Changes in v2: > - Introduce awb::Context to simplify ActiveState and FrameContext > definition > - Add mali uAPI header for CCM and Gamma > - Do not rename Mali functions for hardware configuration in awb and lsc > - Spelling and documentation > > --- > Jacopo Mondi (28): > ipa: libipa: awb: Reimplement AwbAlgorithm > ipa: mali-c55: awb: Port to use libipa AwbAlgorithm > ipa: libipa: Add CcmAlgorithm to libipa > ipa: rkisp1: ccm: Port to use CcmAlgorithm > include: linux: mali-c55: Update to support CCM and Gamma > ipa: mali-c55: Implement Ccm algorithm > ipa: rkisp1: lsc: Re-architecture Lsc support > ipa: libipa: Introduce lsc_base.h > ipa: libipa: lsc_polynomial: Fix polynomial parsing error > ipa: libipa: lsc_polynomial: Rename LscPolynomial > ipa: libipa: lsc_polynomial: Do not inline functions > ipa: libipa: lsc: Move Interpolator<lsc::Components> to lsc_base > ipa: rkisp1: lsc: Remove rogue empty line > ipa: libipa: lsc_polynomial: Move LscPolynomial from RkISP1 > ipa: libipa: lsc_table: Move LscTable from RkISP1 > ipa: libipa: lsc_base: Add sampleForCrop documentation > ipa: libipa: lsc: Introduce LscAlgorithm > ipa: libipa: lsc: Move sizesListToPositions() back to rkisp1 > ipa: rkisp1: lsc: Port to use LscAlgorithm > ipa: libipa: lsc: Make Components a map<> > ipa: libipa: lsc: Document Components parsing > ipa: libipa: lsc_polynomial: Remove sensorSize_ > ipa: libipa: lsc: Document LscDescriptor::sensorSize usage > ipa: libipa: lsc: Quantize lsc gains in IPA > ipa: libipa: lsc: Quantize gains in LscAlgorithm > ipa: libipa: lsc: Re-sort LscAlgorithmBase documentation > ipa: mali-c55: Add sensorInfo to IPAContext > ipa: mali-c55: Port to use LscAlgorithm > > Kieran Bingham (2): > ipa: simple: awb: Port to use libipa AwbAlgorithm > ipa: simple: Port to use libipa CcmAlgorithm > > Laurent Pinchart (1): > ipa: libipa: lsc_polynomial: Don't inline ValueNode accessor > > .../internal/software_isp/debayer_params.h | 4 +- > include/linux/media/arm/mali-c55-config.h | 86 ++++- > src/ipa/libipa/awb.cpp | 418 +++++++++++++++++--- > src/ipa/libipa/awb.h | 100 ++++- > src/ipa/libipa/awb_bayes.cpp | 52 +-- > src/ipa/libipa/awb_bayes.h | 12 +- > src/ipa/libipa/awb_grey.cpp | 16 +- > src/ipa/libipa/awb_grey.h | 6 +- > src/ipa/libipa/ccm.cpp | 245 ++++++++++++ > src/ipa/libipa/ccm.h | 77 ++++ > src/ipa/libipa/lsc.cpp | 372 ++++++++++++++++++ > src/ipa/libipa/lsc.h | 137 +++++++ > src/ipa/libipa/lsc_base.cpp | 136 +++++++ > src/ipa/libipa/lsc_base.h | 56 +++ > src/ipa/libipa/lsc_polynomial.cpp | 208 +++++++++- > src/ipa/libipa/lsc_polynomial.h | 100 ++--- > src/ipa/libipa/lsc_table.cpp | 107 +++++ > src/ipa/libipa/lsc_table.h | 55 +++ > src/ipa/libipa/meson.build | 8 + > src/ipa/mali-c55/algorithms/awb.cpp | 223 ++++++----- > src/ipa/mali-c55/algorithms/awb.h | 28 +- > src/ipa/mali-c55/algorithms/ccm.cpp | 173 +++++++++ > src/ipa/mali-c55/algorithms/ccm.h | 66 ++++ > src/ipa/mali-c55/algorithms/lsc.cpp | 156 +++++--- > src/ipa/mali-c55/algorithms/lsc.h | 32 +- > src/ipa/mali-c55/algorithms/meson.build | 1 + > src/ipa/mali-c55/ipa_context.cpp | 3 + > src/ipa/mali-c55/ipa_context.h | 20 +- > src/ipa/mali-c55/mali-c55.cpp | 2 + > src/ipa/mali-c55/params.h | 2 + > src/ipa/rkisp1/algorithms/awb.cpp | 191 ++------- > src/ipa/rkisp1/algorithms/awb.h | 18 +- > src/ipa/rkisp1/algorithms/ccm.cpp | 97 +---- > src/ipa/rkisp1/algorithms/ccm.h | 17 +- > src/ipa/rkisp1/algorithms/lsc.cpp | 429 +++------------------ > src/ipa/rkisp1/algorithms/lsc.h | 74 ++-- > src/ipa/rkisp1/ipa_context.h | 49 +-- > src/ipa/simple/algorithms/awb.cpp | 140 +++++-- > src/ipa/simple/algorithms/awb.h | 28 ++ > src/ipa/simple/algorithms/blc.cpp | 2 +- > src/ipa/simple/algorithms/ccm.cpp | 74 ++-- > src/ipa/simple/algorithms/ccm.h | 22 +- > src/ipa/simple/ipa_context.h | 15 +- > src/libcamera/software_isp/debayer_cpu.cpp | 12 +- > 44 files changed, 2934 insertions(+), 1135 deletions(-) > --- > base-commit: 709ad59a8d90af3570be1c8d02dee0ec9e011954 > change-id: 20260615-libipa-algorithms-59666bca0a2f > > Best regards,
Hi Milan thanks for the review On Tue, Jul 21, 2026 at 02:25:56PM +0200, Milan Zamazal wrote: > Hi Jacopo, > > Jacopo Mondi <jacopo.mondi@ideasonboard.com> writes: > > > The series is identical to v5 up to patch 27. The only minor difference > > is that I back-tracked on making functions class members but in the > > RkISP1 IPA I kept them as static helpers. > > > > Patches 28 and 29 are new. > > > > They remove the templating of the LscAlgorithm class hierarchy and > > instead implement quantization in the LscAlgorithm class. > > > > So we now expand polynomials and parse tables as floats, store them > > in a map and in LscAlgorithm we re-iterate over it to either quantized > > them in the polynomial case, or simply cast floats to the register bit > > width if table. > > > > Timing the duration of the whole RkISP1 configure operation, it now > > takes 3 msec compared to 600 usec. > > > > RkISP1Lsc lsc.cpp:171 Configure took = 3081[µs] > > > > RkISP1Lsc lsc.cpp:172 Configure took = 583[µs] > > > > Also, now each IPA has to define their own Interpolator::interpolate() > > overload (see rkisp1 IPA as an example of this). > > > > As the interface of LscAlgorithm has changed, patches on the list that > > port the LSC algorithm of other platforms based on v5 will have to be > > rebased. > > I'm having trouble to understand the exact motivation for the change. I've been suggested to move quantization to LscAlgorithm to remove templating of LscPolynomial and LscTable. > From the libipa consumer side, it means that for `simple' pipeline, I > had to copy & substitute & paste code from rkisp1 for no very good > reason. Which looks like a step against the code unification goal, > while it doesn't seem to simplify rkisp1 that much. Agreed > > The float representation could be useful for software ISP, where there > is no inherent reason to use quantized values (although they may still > have its use for portable textures, as pointed out by Bryan). But it > seems using quantized values is still imposed on libipa LSC consumers? Let's put LscPolynomial aside for a second and only consider LscTable. Gains in the tuning files are currently expressed in register format (iow already quantized). This means Lsc tables are not portable across platforms. I think there is consensus on the desire to express gains as floats in tuning files, so that we can share lsc tables among platforms. This is good and will be done on top along with table re-scaling to adapt them to different sensor's resolutions. On quantization: up until v5 I templated the whole LscImplementation class hierarchy with the platform's Q format, so that when the gain vectors are populated (when a polynomial is expanded or a table-resampled in future) they are stored in register format ready to be consumed by the IPA. This required to template a few classes, which was not that bad to me. I've been suggested to remove quantization from the LscImplementation hierarchy, so that it's simpler and instead perform quantization either in the IPA module itself or in LscAlgorithm class. This requires to go over the already populated arrays and quantize the entries one by one before passing them to the IPA. I don't think it's ideal. However having floats as an intermediate values might make interpolation more accurate (I don't know and I don't see why, I'm just speculating) and the implementation is simpler. v6 requires however that each IPA defines its own Interpolator::interpolate() overload (maybe this could be generalized?). Is this the code that you needed to add to your IPA ? On the gain format: simple is kind of special. It doesn't require quantization and you could maybe do with floats. But the same reasoning applies to AWB, gamma, CCM and all other algorithms, where you don't have any register format to translate to. If you want floats in your IPA to work with, maybe using UQ<0, 32> as register format would do ? > > > > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > --- > > Changes in v6: > > - Address comments received on v5, mostly on documentation > > - Remove templating of LscPolynomial and perform quantization in > > LscAlgorithm > > > > Changes in v5: > > - Add "ipa: libipa: lsc_polynomial: Fix polynomial parsing error" > > to fix and exiting potential issue with Polynomial parsing > > - Fix Simple, which I broke in v4, thanks Milan > > > > Changes in v4: > > - s/awb/AWB in comments everywhere, same for CCM and LSC > > - Awb: changelog per patch > > - Make AwbStats pure virtual and move stats parsing logic to the IPA > > modules > > - Reduce comments/renames to reduce patch size and comments > > - Collect tags > > > > Changes in v3: > > - Address comments on Awb from Stefan: > > - Introduce AwbImplementation::Result > > - Move AwbStats implementation to IPAs > > - Drop AwbStats::rg()/bg() and re-implement AwbGray accordingly > > - Fix Saturation bug in Simple Ccm algorithm > > - Fix Simple Ccm gain limits using Q<2, 8> > > - Update Mali CCM and Gamma to latest uAPI patch version > > - Fix Mali CCM gain masking as reported by Dan > > - Break down libIPA Lsc implementation as requested by Stefan > > > > Changes in v2: > > - Introduce awb::Context to simplify ActiveState and FrameContext > > definition > > - Add mali uAPI header for CCM and Gamma > > - Do not rename Mali functions for hardware configuration in awb and lsc > > - Spelling and documentation > > > > --- > > Jacopo Mondi (28): > > ipa: libipa: awb: Reimplement AwbAlgorithm > > ipa: mali-c55: awb: Port to use libipa AwbAlgorithm > > ipa: libipa: Add CcmAlgorithm to libipa > > ipa: rkisp1: ccm: Port to use CcmAlgorithm > > include: linux: mali-c55: Update to support CCM and Gamma > > ipa: mali-c55: Implement Ccm algorithm > > ipa: rkisp1: lsc: Re-architecture Lsc support > > ipa: libipa: Introduce lsc_base.h > > ipa: libipa: lsc_polynomial: Fix polynomial parsing error > > ipa: libipa: lsc_polynomial: Rename LscPolynomial > > ipa: libipa: lsc_polynomial: Do not inline functions > > ipa: libipa: lsc: Move Interpolator<lsc::Components> to lsc_base > > ipa: rkisp1: lsc: Remove rogue empty line > > ipa: libipa: lsc_polynomial: Move LscPolynomial from RkISP1 > > ipa: libipa: lsc_table: Move LscTable from RkISP1 > > ipa: libipa: lsc_base: Add sampleForCrop documentation > > ipa: libipa: lsc: Introduce LscAlgorithm > > ipa: libipa: lsc: Move sizesListToPositions() back to rkisp1 > > ipa: rkisp1: lsc: Port to use LscAlgorithm > > ipa: libipa: lsc: Make Components a map<> > > ipa: libipa: lsc: Document Components parsing > > ipa: libipa: lsc_polynomial: Remove sensorSize_ > > ipa: libipa: lsc: Document LscDescriptor::sensorSize usage > > ipa: libipa: lsc: Quantize lsc gains in IPA > > ipa: libipa: lsc: Quantize gains in LscAlgorithm > > ipa: libipa: lsc: Re-sort LscAlgorithmBase documentation > > ipa: mali-c55: Add sensorInfo to IPAContext > > ipa: mali-c55: Port to use LscAlgorithm > > > > Kieran Bingham (2): > > ipa: simple: awb: Port to use libipa AwbAlgorithm > > ipa: simple: Port to use libipa CcmAlgorithm > > > > Laurent Pinchart (1): > > ipa: libipa: lsc_polynomial: Don't inline ValueNode accessor > > > > .../internal/software_isp/debayer_params.h | 4 +- > > include/linux/media/arm/mali-c55-config.h | 86 ++++- > > src/ipa/libipa/awb.cpp | 418 +++++++++++++++++--- > > src/ipa/libipa/awb.h | 100 ++++- > > src/ipa/libipa/awb_bayes.cpp | 52 +-- > > src/ipa/libipa/awb_bayes.h | 12 +- > > src/ipa/libipa/awb_grey.cpp | 16 +- > > src/ipa/libipa/awb_grey.h | 6 +- > > src/ipa/libipa/ccm.cpp | 245 ++++++++++++ > > src/ipa/libipa/ccm.h | 77 ++++ > > src/ipa/libipa/lsc.cpp | 372 ++++++++++++++++++ > > src/ipa/libipa/lsc.h | 137 +++++++ > > src/ipa/libipa/lsc_base.cpp | 136 +++++++ > > src/ipa/libipa/lsc_base.h | 56 +++ > > src/ipa/libipa/lsc_polynomial.cpp | 208 +++++++++- > > src/ipa/libipa/lsc_polynomial.h | 100 ++--- > > src/ipa/libipa/lsc_table.cpp | 107 +++++ > > src/ipa/libipa/lsc_table.h | 55 +++ > > src/ipa/libipa/meson.build | 8 + > > src/ipa/mali-c55/algorithms/awb.cpp | 223 ++++++----- > > src/ipa/mali-c55/algorithms/awb.h | 28 +- > > src/ipa/mali-c55/algorithms/ccm.cpp | 173 +++++++++ > > src/ipa/mali-c55/algorithms/ccm.h | 66 ++++ > > src/ipa/mali-c55/algorithms/lsc.cpp | 156 +++++--- > > src/ipa/mali-c55/algorithms/lsc.h | 32 +- > > src/ipa/mali-c55/algorithms/meson.build | 1 + > > src/ipa/mali-c55/ipa_context.cpp | 3 + > > src/ipa/mali-c55/ipa_context.h | 20 +- > > src/ipa/mali-c55/mali-c55.cpp | 2 + > > src/ipa/mali-c55/params.h | 2 + > > src/ipa/rkisp1/algorithms/awb.cpp | 191 ++------- > > src/ipa/rkisp1/algorithms/awb.h | 18 +- > > src/ipa/rkisp1/algorithms/ccm.cpp | 97 +---- > > src/ipa/rkisp1/algorithms/ccm.h | 17 +- > > src/ipa/rkisp1/algorithms/lsc.cpp | 429 +++------------------ > > src/ipa/rkisp1/algorithms/lsc.h | 74 ++-- > > src/ipa/rkisp1/ipa_context.h | 49 +-- > > src/ipa/simple/algorithms/awb.cpp | 140 +++++-- > > src/ipa/simple/algorithms/awb.h | 28 ++ > > src/ipa/simple/algorithms/blc.cpp | 2 +- > > src/ipa/simple/algorithms/ccm.cpp | 74 ++-- > > src/ipa/simple/algorithms/ccm.h | 22 +- > > src/ipa/simple/ipa_context.h | 15 +- > > src/libcamera/software_isp/debayer_cpu.cpp | 12 +- > > 44 files changed, 2934 insertions(+), 1135 deletions(-) > > --- > > base-commit: 709ad59a8d90af3570be1c8d02dee0ec9e011954 > > change-id: 20260615-libipa-algorithms-59666bca0a2f > > > > Best regards, >
Jacopo Mondi <jacopo.mondi@ideasonboard.com> writes: > Hi Milan > thanks for the review > > On Tue, Jul 21, 2026 at 02:25:56PM +0200, Milan Zamazal wrote: >> Hi Jacopo, >> >> Jacopo Mondi <jacopo.mondi@ideasonboard.com> writes: >> >> > The series is identical to v5 up to patch 27. The only minor difference >> > is that I back-tracked on making functions class members but in the >> > RkISP1 IPA I kept them as static helpers. >> > >> > Patches 28 and 29 are new. >> > >> > They remove the templating of the LscAlgorithm class hierarchy and >> > instead implement quantization in the LscAlgorithm class. >> > >> > So we now expand polynomials and parse tables as floats, store them >> > in a map and in LscAlgorithm we re-iterate over it to either quantized >> > them in the polynomial case, or simply cast floats to the register bit >> > width if table. >> > >> > Timing the duration of the whole RkISP1 configure operation, it now >> > takes 3 msec compared to 600 usec. >> > >> > RkISP1Lsc lsc.cpp:171 Configure took = 3081[µs] >> > >> > RkISP1Lsc lsc.cpp:172 Configure took = 583[µs] >> > >> > Also, now each IPA has to define their own Interpolator::interpolate() >> > overload (see rkisp1 IPA as an example of this). >> > >> > As the interface of LscAlgorithm has changed, patches on the list that >> > port the LSC algorithm of other platforms based on v5 will have to be >> > rebased. >> >> I'm having trouble to understand the exact motivation for the change. > > I've been suggested to move quantization to LscAlgorithm to remove > templating of LscPolynomial and LscTable. > >> From the libipa consumer side, it means that for `simple' pipeline, I >> had to copy & substitute & paste code from rkisp1 for no very good >> reason. Which looks like a step against the code unification goal, >> while it doesn't seem to simplify rkisp1 that much. > > Agreed > >> >> The float representation could be useful for software ISP, where there >> is no inherent reason to use quantized values (although they may still >> have its use for portable textures, as pointed out by Bryan). But it >> seems using quantized values is still imposed on libipa LSC consumers? > > Let's put LscPolynomial aside for a second and only consider LscTable. > > Gains in the tuning files are currently expressed in register format > (iow already quantized). This means Lsc tables are not portable across > platforms. I think there is consensus on the desire to express gains > as floats in tuning files, so that we can share lsc tables among > platforms. This is good and will be done on top along with table > re-scaling to adapt them to different sensor's resolutions. Yes. > On quantization: up until v5 I templated the whole LscImplementation > class hierarchy with the platform's Q format, so that when the gain > vectors are populated (when a polynomial is expanded or a > table-resampled in future) they are stored in register format > ready to be consumed by the IPA. > > This required to template a few classes, which was not that bad to me. > I've been suggested to remove quantization from the LscImplementation > hierarchy, so that it's simpler and instead perform quantization > either in the IPA module itself or in LscAlgorithm class. This > requires to go over the already populated arrays and quantize the > entries one by one before passing them to the IPA. I don't think it's ideal. > > However having floats as an intermediate values might make > interpolation more accurate (I don't know and I don't see why, > I'm just speculating) and the implementation is simpler. Hmm, the whole stuff sounds like enough opportunities for different opinions... > v6 requires however that each IPA defines its own > Interpolator::interpolate() overload (maybe this could be > generalized?). Is this the code that you needed to add to your IPA ? Yes. If Interpolator::interpolate() specialisation for the map could be added then the IPAs would be happy either way. If not, I'd personally prefer IPA simplicity over libipa simplicity. > On the gain format: simple is kind of special. It doesn't require > quantization and you could maybe do with floats. But the same > reasoning applies to AWB, gamma, CCM and all other algorithms, where > you don't have any register format to translate to. If you want floats > in your IPA to work with, maybe using UQ<0, 32> as register format > would do ? It looks weird but it's not an obstacle. `simple' can live with the quantized values; I asked just in case I was missing something. Thanks for clarification. >> >> > >> > >> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> >> > --- >> > Changes in v6: >> > - Address comments received on v5, mostly on documentation >> > - Remove templating of LscPolynomial and perform quantization in >> > LscAlgorithm >> > >> > Changes in v5: >> > - Add "ipa: libipa: lsc_polynomial: Fix polynomial parsing error" >> > to fix and exiting potential issue with Polynomial parsing >> > - Fix Simple, which I broke in v4, thanks Milan >> > >> > Changes in v4: >> > - s/awb/AWB in comments everywhere, same for CCM and LSC >> > - Awb: changelog per patch >> > - Make AwbStats pure virtual and move stats parsing logic to the IPA >> > modules >> > - Reduce comments/renames to reduce patch size and comments >> > - Collect tags >> > >> > Changes in v3: >> > - Address comments on Awb from Stefan: >> > - Introduce AwbImplementation::Result >> > - Move AwbStats implementation to IPAs >> > - Drop AwbStats::rg()/bg() and re-implement AwbGray accordingly >> > - Fix Saturation bug in Simple Ccm algorithm >> > - Fix Simple Ccm gain limits using Q<2, 8> >> > - Update Mali CCM and Gamma to latest uAPI patch version >> > - Fix Mali CCM gain masking as reported by Dan >> > - Break down libIPA Lsc implementation as requested by Stefan >> > >> > Changes in v2: >> > - Introduce awb::Context to simplify ActiveState and FrameContext >> > definition >> > - Add mali uAPI header for CCM and Gamma >> > - Do not rename Mali functions for hardware configuration in awb and lsc >> > - Spelling and documentation >> > >> > --- >> > Jacopo Mondi (28): >> > ipa: libipa: awb: Reimplement AwbAlgorithm >> > ipa: mali-c55: awb: Port to use libipa AwbAlgorithm >> > ipa: libipa: Add CcmAlgorithm to libipa >> > ipa: rkisp1: ccm: Port to use CcmAlgorithm >> > include: linux: mali-c55: Update to support CCM and Gamma >> > ipa: mali-c55: Implement Ccm algorithm >> > ipa: rkisp1: lsc: Re-architecture Lsc support >> > ipa: libipa: Introduce lsc_base.h >> > ipa: libipa: lsc_polynomial: Fix polynomial parsing error >> > ipa: libipa: lsc_polynomial: Rename LscPolynomial >> > ipa: libipa: lsc_polynomial: Do not inline functions >> > ipa: libipa: lsc: Move Interpolator<lsc::Components> to lsc_base >> > ipa: rkisp1: lsc: Remove rogue empty line >> > ipa: libipa: lsc_polynomial: Move LscPolynomial from RkISP1 >> > ipa: libipa: lsc_table: Move LscTable from RkISP1 >> > ipa: libipa: lsc_base: Add sampleForCrop documentation >> > ipa: libipa: lsc: Introduce LscAlgorithm >> > ipa: libipa: lsc: Move sizesListToPositions() back to rkisp1 >> > ipa: rkisp1: lsc: Port to use LscAlgorithm >> > ipa: libipa: lsc: Make Components a map<> >> > ipa: libipa: lsc: Document Components parsing >> > ipa: libipa: lsc_polynomial: Remove sensorSize_ >> > ipa: libipa: lsc: Document LscDescriptor::sensorSize usage >> > ipa: libipa: lsc: Quantize lsc gains in IPA >> > ipa: libipa: lsc: Quantize gains in LscAlgorithm >> > ipa: libipa: lsc: Re-sort LscAlgorithmBase documentation >> > ipa: mali-c55: Add sensorInfo to IPAContext >> > ipa: mali-c55: Port to use LscAlgorithm >> > >> > Kieran Bingham (2): >> > ipa: simple: awb: Port to use libipa AwbAlgorithm >> > ipa: simple: Port to use libipa CcmAlgorithm >> > >> > Laurent Pinchart (1): >> > ipa: libipa: lsc_polynomial: Don't inline ValueNode accessor >> > >> > .../internal/software_isp/debayer_params.h | 4 +- >> > include/linux/media/arm/mali-c55-config.h | 86 ++++- >> > src/ipa/libipa/awb.cpp | 418 +++++++++++++++++--- >> > src/ipa/libipa/awb.h | 100 ++++- >> > src/ipa/libipa/awb_bayes.cpp | 52 +-- >> > src/ipa/libipa/awb_bayes.h | 12 +- >> > src/ipa/libipa/awb_grey.cpp | 16 +- >> > src/ipa/libipa/awb_grey.h | 6 +- >> > src/ipa/libipa/ccm.cpp | 245 ++++++++++++ >> > src/ipa/libipa/ccm.h | 77 ++++ >> > src/ipa/libipa/lsc.cpp | 372 ++++++++++++++++++ >> > src/ipa/libipa/lsc.h | 137 +++++++ >> > src/ipa/libipa/lsc_base.cpp | 136 +++++++ >> > src/ipa/libipa/lsc_base.h | 56 +++ >> > src/ipa/libipa/lsc_polynomial.cpp | 208 +++++++++- >> > src/ipa/libipa/lsc_polynomial.h | 100 ++--- >> > src/ipa/libipa/lsc_table.cpp | 107 +++++ >> > src/ipa/libipa/lsc_table.h | 55 +++ >> > src/ipa/libipa/meson.build | 8 + >> > src/ipa/mali-c55/algorithms/awb.cpp | 223 ++++++----- >> > src/ipa/mali-c55/algorithms/awb.h | 28 +- >> > src/ipa/mali-c55/algorithms/ccm.cpp | 173 +++++++++ >> > src/ipa/mali-c55/algorithms/ccm.h | 66 ++++ >> > src/ipa/mali-c55/algorithms/lsc.cpp | 156 +++++--- >> > src/ipa/mali-c55/algorithms/lsc.h | 32 +- >> > src/ipa/mali-c55/algorithms/meson.build | 1 + >> > src/ipa/mali-c55/ipa_context.cpp | 3 + >> > src/ipa/mali-c55/ipa_context.h | 20 +- >> > src/ipa/mali-c55/mali-c55.cpp | 2 + >> > src/ipa/mali-c55/params.h | 2 + >> > src/ipa/rkisp1/algorithms/awb.cpp | 191 ++------- >> > src/ipa/rkisp1/algorithms/awb.h | 18 +- >> > src/ipa/rkisp1/algorithms/ccm.cpp | 97 +---- >> > src/ipa/rkisp1/algorithms/ccm.h | 17 +- >> > src/ipa/rkisp1/algorithms/lsc.cpp | 429 +++------------------ >> > src/ipa/rkisp1/algorithms/lsc.h | 74 ++-- >> > src/ipa/rkisp1/ipa_context.h | 49 +-- >> > src/ipa/simple/algorithms/awb.cpp | 140 +++++-- >> > src/ipa/simple/algorithms/awb.h | 28 ++ >> > src/ipa/simple/algorithms/blc.cpp | 2 +- >> > src/ipa/simple/algorithms/ccm.cpp | 74 ++-- >> > src/ipa/simple/algorithms/ccm.h | 22 +- >> > src/ipa/simple/ipa_context.h | 15 +- >> > src/libcamera/software_isp/debayer_cpu.cpp | 12 +- >> > 44 files changed, 2934 insertions(+), 1135 deletions(-) >> > --- >> > base-commit: 709ad59a8d90af3570be1c8d02dee0ec9e011954 >> > change-id: 20260615-libipa-algorithms-59666bca0a2f >> > >> > Best regards, >>
Hi 2026. 07. 20. 16:59 keltezéssel, Jacopo Mondi írta: > The series is identical to v5 up to patch 27. The only minor difference > is that I back-tracked on making functions class members but in the > RkISP1 IPA I kept them as static helpers. > > Patches 28 and 29 are new. > > They remove the templating of the LscAlgorithm class hierarchy and > instead implement quantization in the LscAlgorithm class. I have spent some time on this v5 vs. v6 question, and to me it seems there are no real significant differences. The interpolation is done on the quantized type in either case, the only real difference is whether the quantization happen in the implementation or in the main algorithm. And I think that's largely an implementation detail that doesn't affect users much. One potential thing that was already brought up is whether it is even possible to do the interpolation on the quantized type? I think as long as we are dealing with unsigned types, it should be fine (so maybe it would be best to add a `static_assert` to `LscAlgorithm` enforcing that), but if a platform appears that needs some kid of "special" quantization from float, then `LscAlgorithm` will have to be adjusted. Initially I thought v6 would defer quantization even more by doing everything with floats and only quantizing the final interpolated data. I feel like that's a nice approach that avoids any details of quantization until the last point, but of course it has the largest performance penalty as now each time the lsc tables change float interpolation and quantization is carried out, instead of just the integer interpolation of v5 and v6. > > So we now expand polynomials and parse tables as floats, store them > in a map and in LscAlgorithm we re-iterate over it to either quantized > them in the polynomial case, or simply cast floats to the register bit > width if table. > > Timing the duration of the whole RkISP1 configure operation, it now > takes 3 msec compared to 600 usec. > > RkISP1Lsc lsc.cpp:171 Configure took = 3081[µs] > > RkISP1Lsc lsc.cpp:172 Configure took = 583[µs] I'm honestly a bit surprised that there is a 5x time penalty here... is there any chance you could attach the tuning file that you used to measure the time? > > Also, now each IPA has to define their own Interpolator::interpolate() > overload (see rkisp1 IPA as an example of this). It seems to me that the it should be possible to provide the specializations in a common place like in v5 without making every ipa specialize it itself (which will eventually lead to duplicates). The "Components" type only depends on the quantized type, but currently there is an unnecessary dependency on the fixed point representation because it is defined inside `LscAlgorithm<U>`, moving it out would remove this dependency and would allow specialization in a common place, like in v5. > > As the interface of LscAlgorithm has changed, patches on the list that > port the LSC algorithm of other platforms based on v5 will have to be rebased. > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > --- So in summary, assuming the plan is to move the table based tuning files to floating point number, I think my preference would be deferring the quantization as much as possible (until after interpolation) and dealing with floats. However, although I have not tested it, I feel the runtime cost of that would be too big compared to the other approaches. So my (very) slight preference is for v6 with two caveats: * the interpolation can be moved to a common place like in v5 (as mentioned above); * I would like take a look at the 5x runtime gap. > Changes in v6: > - Address comments received on v5, mostly on documentation > - Remove templating of LscPolynomial and perform quantization in > LscAlgorithm > > Changes in v5: > - Add "ipa: libipa: lsc_polynomial: Fix polynomial parsing error" > to fix and exiting potential issue with Polynomial parsing > - Fix Simple, which I broke in v4, thanks Milan > > Changes in v4: > - s/awb/AWB in comments everywhere, same for CCM and LSC > - Awb: changelog per patch > - Make AwbStats pure virtual and move stats parsing logic to the IPA > modules > - Reduce comments/renames to reduce patch size and comments > - Collect tags > > Changes in v3: > - Address comments on Awb from Stefan: > - Introduce AwbImplementation::Result > - Move AwbStats implementation to IPAs > - Drop AwbStats::rg()/bg() and re-implement AwbGray accordingly > - Fix Saturation bug in Simple Ccm algorithm > - Fix Simple Ccm gain limits using Q<2, 8> > - Update Mali CCM and Gamma to latest uAPI patch version > - Fix Mali CCM gain masking as reported by Dan > - Break down libIPA Lsc implementation as requested by Stefan > > Changes in v2: > - Introduce awb::Context to simplify ActiveState and FrameContext > definition > - Add mali uAPI header for CCM and Gamma > - Do not rename Mali functions for hardware configuration in awb and lsc > - Spelling and documentation > > --- > Jacopo Mondi (28): > ipa: libipa: awb: Reimplement AwbAlgorithm > ipa: mali-c55: awb: Port to use libipa AwbAlgorithm > ipa: libipa: Add CcmAlgorithm to libipa > ipa: rkisp1: ccm: Port to use CcmAlgorithm > include: linux: mali-c55: Update to support CCM and Gamma > ipa: mali-c55: Implement Ccm algorithm > ipa: rkisp1: lsc: Re-architecture Lsc support > ipa: libipa: Introduce lsc_base.h > ipa: libipa: lsc_polynomial: Fix polynomial parsing error > ipa: libipa: lsc_polynomial: Rename LscPolynomial > ipa: libipa: lsc_polynomial: Do not inline functions > ipa: libipa: lsc: Move Interpolator<lsc::Components> to lsc_base > ipa: rkisp1: lsc: Remove rogue empty line > ipa: libipa: lsc_polynomial: Move LscPolynomial from RkISP1 > ipa: libipa: lsc_table: Move LscTable from RkISP1 > ipa: libipa: lsc_base: Add sampleForCrop documentation > ipa: libipa: lsc: Introduce LscAlgorithm > ipa: libipa: lsc: Move sizesListToPositions() back to rkisp1 > ipa: rkisp1: lsc: Port to use LscAlgorithm > ipa: libipa: lsc: Make Components a map<> > ipa: libipa: lsc: Document Components parsing > ipa: libipa: lsc_polynomial: Remove sensorSize_ > ipa: libipa: lsc: Document LscDescriptor::sensorSize usage > ipa: libipa: lsc: Quantize lsc gains in IPA > ipa: libipa: lsc: Quantize gains in LscAlgorithm > ipa: libipa: lsc: Re-sort LscAlgorithmBase documentation > ipa: mali-c55: Add sensorInfo to IPAContext > ipa: mali-c55: Port to use LscAlgorithm > > Kieran Bingham (2): > ipa: simple: awb: Port to use libipa AwbAlgorithm > ipa: simple: Port to use libipa CcmAlgorithm > > Laurent Pinchart (1): > ipa: libipa: lsc_polynomial: Don't inline ValueNode accessor > > .../internal/software_isp/debayer_params.h | 4 +- > include/linux/media/arm/mali-c55-config.h | 86 ++++- > src/ipa/libipa/awb.cpp | 418 +++++++++++++++++--- > src/ipa/libipa/awb.h | 100 ++++- > src/ipa/libipa/awb_bayes.cpp | 52 +-- > src/ipa/libipa/awb_bayes.h | 12 +- > src/ipa/libipa/awb_grey.cpp | 16 +- > src/ipa/libipa/awb_grey.h | 6 +- > src/ipa/libipa/ccm.cpp | 245 ++++++++++++ > src/ipa/libipa/ccm.h | 77 ++++ > src/ipa/libipa/lsc.cpp | 372 ++++++++++++++++++ > src/ipa/libipa/lsc.h | 137 +++++++ > src/ipa/libipa/lsc_base.cpp | 136 +++++++ > src/ipa/libipa/lsc_base.h | 56 +++ > src/ipa/libipa/lsc_polynomial.cpp | 208 +++++++++- > src/ipa/libipa/lsc_polynomial.h | 100 ++--- > src/ipa/libipa/lsc_table.cpp | 107 +++++ > src/ipa/libipa/lsc_table.h | 55 +++ > src/ipa/libipa/meson.build | 8 + > src/ipa/mali-c55/algorithms/awb.cpp | 223 ++++++----- > src/ipa/mali-c55/algorithms/awb.h | 28 +- > src/ipa/mali-c55/algorithms/ccm.cpp | 173 +++++++++ > src/ipa/mali-c55/algorithms/ccm.h | 66 ++++ > src/ipa/mali-c55/algorithms/lsc.cpp | 156 +++++--- > src/ipa/mali-c55/algorithms/lsc.h | 32 +- > src/ipa/mali-c55/algorithms/meson.build | 1 + > src/ipa/mali-c55/ipa_context.cpp | 3 + > src/ipa/mali-c55/ipa_context.h | 20 +- > src/ipa/mali-c55/mali-c55.cpp | 2 + > src/ipa/mali-c55/params.h | 2 + > src/ipa/rkisp1/algorithms/awb.cpp | 191 ++------- > src/ipa/rkisp1/algorithms/awb.h | 18 +- > src/ipa/rkisp1/algorithms/ccm.cpp | 97 +---- > src/ipa/rkisp1/algorithms/ccm.h | 17 +- > src/ipa/rkisp1/algorithms/lsc.cpp | 429 +++------------------ > src/ipa/rkisp1/algorithms/lsc.h | 74 ++-- > src/ipa/rkisp1/ipa_context.h | 49 +-- > src/ipa/simple/algorithms/awb.cpp | 140 +++++-- > src/ipa/simple/algorithms/awb.h | 28 ++ > src/ipa/simple/algorithms/blc.cpp | 2 +- > src/ipa/simple/algorithms/ccm.cpp | 74 ++-- > src/ipa/simple/algorithms/ccm.h | 22 +- > src/ipa/simple/ipa_context.h | 15 +- > src/libcamera/software_isp/debayer_cpu.cpp | 12 +- > 44 files changed, 2934 insertions(+), 1135 deletions(-) > --- > base-commit: 709ad59a8d90af3570be1c8d02dee0ec9e011954 > change-id: 20260615-libipa-algorithms-59666bca0a2f > > Best regards,
Hi Barnabás On Tue, Jul 28, 2026 at 11:48:51AM +0200, Barnabás Pőcze wrote: > Hi > > 2026. 07. 20. 16:59 keltezéssel, Jacopo Mondi írta: > > The series is identical to v5 up to patch 27. The only minor difference > > is that I back-tracked on making functions class members but in the > > RkISP1 IPA I kept them as static helpers. > > > > Patches 28 and 29 are new. > > > > They remove the templating of the LscAlgorithm class hierarchy and > > instead implement quantization in the LscAlgorithm class. > > I have spent some time on this v5 vs. v6 question, and to me it seems there Thanks a lot > are no real significant differences. The interpolation is done on the quantized > type in either case, the only real difference is whether the quantization happen > in the implementation or in the main algorithm. And I think that's largely an Note there is a third option suggested by Stefan to move quantization in the single IPA after the interpolation in the float domain > implementation detail that doesn't affect users much. > > One potential thing that was already brought up is whether it is even possible > to do the interpolation on the quantized type? I think as long as we are dealing > with unsigned types, it should be fine (so maybe it would be best to add a `static_assert` > to `LscAlgorithm` enforcing that), but if a platform appears that needs some kid of > "special" quantization from float, then `LscAlgorithm` will have to be adjusted. > > Initially I thought v6 would defer quantization even more by doing everything with > floats and only quantizing the final interpolated data. I feel like that's a nice That would require moving quantization completely to the IPAs, right ? > approach that avoids any details of quantization until the last point, but of course > it has the largest performance penalty as now each time the lsc tables change float > interpolation and quantization is carried out, instead of just the integer interpolation > of v5 and v6. > > > > > So we now expand polynomials and parse tables as floats, store them > > in a map and in LscAlgorithm we re-iterate over it to either quantized > > them in the polynomial case, or simply cast floats to the register bit > > width if table. > > > > Timing the duration of the whole RkISP1 configure operation, it now > > takes 3 msec compared to 600 usec. > > > > RkISP1Lsc lsc.cpp:171 Configure took = 3081[µs] > > > > RkISP1Lsc lsc.cpp:172 Configure took = 583[µs] > > I'm honestly a bit surprised that there is a 5x time penalty here... is there > any chance you could attach the tuning file that you used to measure the time? Sure, I think it's just the imx219 tuning file we have in the mainline tree From the branch I sent to the ml: https://gitlab.freedesktop.org/camera/libcamera/-/blob/patchwork/6067/src/ipa/rkisp1/data/imx219.yaml > > > > > > Also, now each IPA has to define their own Interpolator::interpolate() > > overload (see rkisp1 IPA as an example of this). > > It seems to me that the it should be possible to provide the specializations > in a common place like in v5 without making every ipa specialize it itself > (which will eventually lead to duplicates). The "Components" type only > depends on the quantized type, but currently there is an unnecessary dependency > on the fixed point representation because it is defined inside `LscAlgorithm<U>`, > moving it out would remove this dependency and would allow specialization in a > common place, like in v5. > > > > > > As the interface of LscAlgorithm has changed, patches on the list that > > port the LSC algorithm of other platforms based on v5 will have to be rebased. > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > --- > > So in summary, assuming the plan is to move the table based tuning files to > floating point number, I think my preference would be deferring the quantization > as much as possible (until after interpolation) and dealing with floats. However, > although I have not tested it, I feel the runtime cost of that would be too big > compared to the other approaches. So my (very) slight preference is for v6 with two caveats: > * the interpolation can be moved to a common place like in v5 (as mentioned above); I presume there is way to do that by moving Components<> out of LscAlgorithm. As you noted, I have defined it inside the class because of the template dependency. > * I would like take a look at the 5x runtime gap. The latest version is available in the above branch while v5 is available in https://gitlab.freedesktop.org/camera/libcamera/-/tree/patchwork/6048/ The timing measurement was performed with: ------------------------------------------------------------------------------- --- a/src/ipa/rkisp1/algorithms/lsc.cpp +++ b/src/ipa/rkisp1/algorithms/lsc.cpp @@ -140,6 +140,8 @@ int LensShadingCorrection::configure(IPAContext &context, const Size &size = context.configuration.sensor.size; Size totalSize{}; +std::chrono::high_resolution_clock::time_point begin = std::chrono::high_resolution_clock::now(); + for (unsigned int i = 0; i < RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE; ++i) { xSizes_[i] = xSize_[i] * size.width; ySizes_[i] = ySize_[i] * size.height; @@ -162,8 +164,13 @@ int LensShadingCorrection::configure(IPAContext &context, yGrad_[i] = std::round(32768 / ySizes_[i]); } - return lscAlgo_.configure(context.activeState.lsc, configInfo.analogCrop, + lscAlgo_.configure(context.activeState.lsc, configInfo.analogCrop, xPos_, yPos_); + std::chrono::high_resolution_clock::time_point end = std::chrono::high_resolution_clock::now(); + + LOG(RkISP1Lsc, Error) << "Configure took = " << std::chrono::duration_cast<std::chrono::microseconds>(end - begin).count() << "[µs]"; + + return 0; } void LensShadingCorrection::setParameters(rkisp1_cif_isp_lsc_config &config) ------------------------------------------------------------------------------- > > > > Changes in v6: > > - Address comments received on v5, mostly on documentation > > - Remove templating of LscPolynomial and perform quantization in > > LscAlgorithm > > > > Changes in v5: > > - Add "ipa: libipa: lsc_polynomial: Fix polynomial parsing error" > > to fix and exiting potential issue with Polynomial parsing > > - Fix Simple, which I broke in v4, thanks Milan > > > > Changes in v4: > > - s/awb/AWB in comments everywhere, same for CCM and LSC > > - Awb: changelog per patch > > - Make AwbStats pure virtual and move stats parsing logic to the IPA > > modules > > - Reduce comments/renames to reduce patch size and comments > > - Collect tags > > > > Changes in v3: > > - Address comments on Awb from Stefan: > > - Introduce AwbImplementation::Result > > - Move AwbStats implementation to IPAs > > - Drop AwbStats::rg()/bg() and re-implement AwbGray accordingly > > - Fix Saturation bug in Simple Ccm algorithm > > - Fix Simple Ccm gain limits using Q<2, 8> > > - Update Mali CCM and Gamma to latest uAPI patch version > > - Fix Mali CCM gain masking as reported by Dan > > - Break down libIPA Lsc implementation as requested by Stefan > > > > Changes in v2: > > - Introduce awb::Context to simplify ActiveState and FrameContext > > definition > > - Add mali uAPI header for CCM and Gamma > > - Do not rename Mali functions for hardware configuration in awb and lsc > > - Spelling and documentation > > > > --- > > Jacopo Mondi (28): > > ipa: libipa: awb: Reimplement AwbAlgorithm > > ipa: mali-c55: awb: Port to use libipa AwbAlgorithm > > ipa: libipa: Add CcmAlgorithm to libipa > > ipa: rkisp1: ccm: Port to use CcmAlgorithm > > include: linux: mali-c55: Update to support CCM and Gamma > > ipa: mali-c55: Implement Ccm algorithm > > ipa: rkisp1: lsc: Re-architecture Lsc support > > ipa: libipa: Introduce lsc_base.h > > ipa: libipa: lsc_polynomial: Fix polynomial parsing error > > ipa: libipa: lsc_polynomial: Rename LscPolynomial > > ipa: libipa: lsc_polynomial: Do not inline functions > > ipa: libipa: lsc: Move Interpolator<lsc::Components> to lsc_base > > ipa: rkisp1: lsc: Remove rogue empty line > > ipa: libipa: lsc_polynomial: Move LscPolynomial from RkISP1 > > ipa: libipa: lsc_table: Move LscTable from RkISP1 > > ipa: libipa: lsc_base: Add sampleForCrop documentation > > ipa: libipa: lsc: Introduce LscAlgorithm > > ipa: libipa: lsc: Move sizesListToPositions() back to rkisp1 > > ipa: rkisp1: lsc: Port to use LscAlgorithm > > ipa: libipa: lsc: Make Components a map<> > > ipa: libipa: lsc: Document Components parsing > > ipa: libipa: lsc_polynomial: Remove sensorSize_ > > ipa: libipa: lsc: Document LscDescriptor::sensorSize usage > > ipa: libipa: lsc: Quantize lsc gains in IPA > > ipa: libipa: lsc: Quantize gains in LscAlgorithm > > ipa: libipa: lsc: Re-sort LscAlgorithmBase documentation > > ipa: mali-c55: Add sensorInfo to IPAContext > > ipa: mali-c55: Port to use LscAlgorithm > > > > Kieran Bingham (2): > > ipa: simple: awb: Port to use libipa AwbAlgorithm > > ipa: simple: Port to use libipa CcmAlgorithm > > > > Laurent Pinchart (1): > > ipa: libipa: lsc_polynomial: Don't inline ValueNode accessor > > > > .../internal/software_isp/debayer_params.h | 4 +- > > include/linux/media/arm/mali-c55-config.h | 86 ++++- > > src/ipa/libipa/awb.cpp | 418 +++++++++++++++++--- > > src/ipa/libipa/awb.h | 100 ++++- > > src/ipa/libipa/awb_bayes.cpp | 52 +-- > > src/ipa/libipa/awb_bayes.h | 12 +- > > src/ipa/libipa/awb_grey.cpp | 16 +- > > src/ipa/libipa/awb_grey.h | 6 +- > > src/ipa/libipa/ccm.cpp | 245 ++++++++++++ > > src/ipa/libipa/ccm.h | 77 ++++ > > src/ipa/libipa/lsc.cpp | 372 ++++++++++++++++++ > > src/ipa/libipa/lsc.h | 137 +++++++ > > src/ipa/libipa/lsc_base.cpp | 136 +++++++ > > src/ipa/libipa/lsc_base.h | 56 +++ > > src/ipa/libipa/lsc_polynomial.cpp | 208 +++++++++- > > src/ipa/libipa/lsc_polynomial.h | 100 ++--- > > src/ipa/libipa/lsc_table.cpp | 107 +++++ > > src/ipa/libipa/lsc_table.h | 55 +++ > > src/ipa/libipa/meson.build | 8 + > > src/ipa/mali-c55/algorithms/awb.cpp | 223 ++++++----- > > src/ipa/mali-c55/algorithms/awb.h | 28 +- > > src/ipa/mali-c55/algorithms/ccm.cpp | 173 +++++++++ > > src/ipa/mali-c55/algorithms/ccm.h | 66 ++++ > > src/ipa/mali-c55/algorithms/lsc.cpp | 156 +++++--- > > src/ipa/mali-c55/algorithms/lsc.h | 32 +- > > src/ipa/mali-c55/algorithms/meson.build | 1 + > > src/ipa/mali-c55/ipa_context.cpp | 3 + > > src/ipa/mali-c55/ipa_context.h | 20 +- > > src/ipa/mali-c55/mali-c55.cpp | 2 + > > src/ipa/mali-c55/params.h | 2 + > > src/ipa/rkisp1/algorithms/awb.cpp | 191 ++------- > > src/ipa/rkisp1/algorithms/awb.h | 18 +- > > src/ipa/rkisp1/algorithms/ccm.cpp | 97 +---- > > src/ipa/rkisp1/algorithms/ccm.h | 17 +- > > src/ipa/rkisp1/algorithms/lsc.cpp | 429 +++------------------ > > src/ipa/rkisp1/algorithms/lsc.h | 74 ++-- > > src/ipa/rkisp1/ipa_context.h | 49 +-- > > src/ipa/simple/algorithms/awb.cpp | 140 +++++-- > > src/ipa/simple/algorithms/awb.h | 28 ++ > > src/ipa/simple/algorithms/blc.cpp | 2 +- > > src/ipa/simple/algorithms/ccm.cpp | 74 ++-- > > src/ipa/simple/algorithms/ccm.h | 22 +- > > src/ipa/simple/ipa_context.h | 15 +- > > src/libcamera/software_isp/debayer_cpu.cpp | 12 +- > > 44 files changed, 2934 insertions(+), 1135 deletions(-) > > --- > > base-commit: 709ad59a8d90af3570be1c8d02dee0ec9e011954 > > change-id: 20260615-libipa-algorithms-59666bca0a2f > > > > Best regards, >
2026. 07. 28. 14:12 keltezéssel, Jacopo Mondi írta: > Hi Barnabás > > On Tue, Jul 28, 2026 at 11:48:51AM +0200, Barnabás Pőcze wrote: >> Hi >> >> 2026. 07. 20. 16:59 keltezéssel, Jacopo Mondi írta: >>> The series is identical to v5 up to patch 27. The only minor difference >>> is that I back-tracked on making functions class members but in the >>> RkISP1 IPA I kept them as static helpers. >>> >>> Patches 28 and 29 are new. >>> >>> They remove the templating of the LscAlgorithm class hierarchy and >>> instead implement quantization in the LscAlgorithm class. >> >> I have spent some time on this v5 vs. v6 question, and to me it seems there > > Thanks a lot > >> are no real significant differences. The interpolation is done on the quantized >> type in either case, the only real difference is whether the quantization happen >> in the implementation or in the main algorithm. And I think that's largely an > > Note there is a third option suggested by Stefan to move quantization > in the single IPA after the interpolation in the float domain That sounds like what I'm suggesting: load and interpolate with `float`, quantize the interpolated data "in the IPA module" right before applying. > >> implementation detail that doesn't affect users much. >> >> One potential thing that was already brought up is whether it is even possible >> to do the interpolation on the quantized type? I think as long as we are dealing >> with unsigned types, it should be fine (so maybe it would be best to add a `static_assert` >> to `LscAlgorithm` enforcing that), but if a platform appears that needs some kid of >> "special" quantization from float, then `LscAlgorithm` will have to be adjusted. >> >> Initially I thought v6 would defer quantization even more by doing everything with >> floats and only quantizing the final interpolated data. I feel like that's a nice > > That would require moving quantization completely to the IPAs, right ? I was thinking of something like this: struct Components { std::map<std::string, std::vector<float>, std::less<>> sets; template<typename T> void quantize(std::string_key key, typename T::QuantizedType *dst) { const auto &s = sets.at(key); std::transform(s.begin(), s.end(), dst, [](float x) { return T(x).quantized(); }); } }; ... void LensShadingCorrection::copyTable(rkisp1_cif_isp_lsc_config &config, const lsc::Components &set) { set.quantize<UQ<2, 10>>("r", &config.r_data_tbl[0][0]); ... } > >> approach that avoids any details of quantization until the last point, but of course >> it has the largest performance penalty as now each time the lsc tables change float >> interpolation and quantization is carried out, instead of just the integer interpolation >> of v5 and v6. >> >>> >>> So we now expand polynomials and parse tables as floats, store them >>> in a map and in LscAlgorithm we re-iterate over it to either quantized >>> them in the polynomial case, or simply cast floats to the register bit >>> width if table. >>> >>> Timing the duration of the whole RkISP1 configure operation, it now >>> takes 3 msec compared to 600 usec. >>> >>> RkISP1Lsc lsc.cpp:171 Configure took = 3081[µs] >>> >>> RkISP1Lsc lsc.cpp:172 Configure took = 583[µs] >> >> I'm honestly a bit surprised that there is a 5x time penalty here... is there >> any chance you could attach the tuning file that you used to measure the time? > > Sure, I think it's just the imx219 tuning file we have in the mainline > tree > > From the branch I sent to the ml: > https://gitlab.freedesktop.org/camera/libcamera/-/blob/patchwork/6067/src/ipa/rkisp1/data/imx219.yaml > >> >> >>> >>> Also, now each IPA has to define their own Interpolator::interpolate() >>> overload (see rkisp1 IPA as an example of this). >> >> It seems to me that the it should be possible to provide the specializations >> in a common place like in v5 without making every ipa specialize it itself >> (which will eventually lead to duplicates). The "Components" type only >> depends on the quantized type, but currently there is an unnecessary dependency >> on the fixed point representation because it is defined inside `LscAlgorithm<U>`, >> moving it out would remove this dependency and would allow specialization in a >> common place, like in v5. >> >> >>> >>> As the interface of LscAlgorithm has changed, patches on the list that >>> port the LSC algorithm of other platforms based on v5 will have to be rebased. >>> >>> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> >>> --- >> >> So in summary, assuming the plan is to move the table based tuning files to >> floating point number, I think my preference would be deferring the quantization >> as much as possible (until after interpolation) and dealing with floats. However, >> although I have not tested it, I feel the runtime cost of that would be too big >> compared to the other approaches. So my (very) slight preference is for v6 with two caveats: >> * the interpolation can be moved to a common place like in v5 (as mentioned above); > > I presume there is way to do that by moving Components<> out of > LscAlgorithm. As you noted, I have defined it inside the class because > of the template dependency. My point is that I don't think there is a real dependency, I'm attaching a proof of concept patch that should apply to v6. > >> * I would like take a look at the 5x runtime gap. > > The latest version is available in the above branch while v5 is > available in > https://gitlab.freedesktop.org/camera/libcamera/-/tree/patchwork/6048/ > > The timing measurement was performed with: > > ------------------------------------------------------------------------------- > --- a/src/ipa/rkisp1/algorithms/lsc.cpp > +++ b/src/ipa/rkisp1/algorithms/lsc.cpp > @@ -140,6 +140,8 @@ int LensShadingCorrection::configure(IPAContext &context, > const Size &size = context.configuration.sensor.size; > Size totalSize{}; > > +std::chrono::high_resolution_clock::time_point begin = std::chrono::high_resolution_clock::now(); > + > for (unsigned int i = 0; i < RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE; ++i) { > xSizes_[i] = xSize_[i] * size.width; > ySizes_[i] = ySize_[i] * size.height; > @@ -162,8 +164,13 @@ int LensShadingCorrection::configure(IPAContext &context, > yGrad_[i] = std::round(32768 / ySizes_[i]); > } > > - return lscAlgo_.configure(context.activeState.lsc, configInfo.analogCrop, > + lscAlgo_.configure(context.activeState.lsc, configInfo.analogCrop, > xPos_, yPos_); > + std::chrono::high_resolution_clock::time_point end = std::chrono::high_resolution_clock::now(); > + > + LOG(RkISP1Lsc, Error) << "Configure took = " << std::chrono::duration_cast<std::chrono::microseconds>(end - begin).count() << "[µs]"; > + > + return 0; > } Interesting, I'm only measuring the `lscAlgo_.configure()` call because the parts before seem to be the same, and I'm getting around ~120us for v5 and ~140us for v6 with imx219.yml. > > void LensShadingCorrection::setParameters(rkisp1_cif_isp_lsc_config &config) > ------------------------------------------------------------------------------- > > > >> >> >>> Changes in v6: >>> - Address comments received on v5, mostly on documentation >>> - Remove templating of LscPolynomial and perform quantization in >>> LscAlgorithm >>> >>> Changes in v5: >>> - Add "ipa: libipa: lsc_polynomial: Fix polynomial parsing error" >>> to fix and exiting potential issue with Polynomial parsing >>> - Fix Simple, which I broke in v4, thanks Milan >>> >>> Changes in v4: >>> - s/awb/AWB in comments everywhere, same for CCM and LSC >>> - Awb: changelog per patch >>> - Make AwbStats pure virtual and move stats parsing logic to the IPA >>> modules >>> - Reduce comments/renames to reduce patch size and comments >>> - Collect tags >>> >>> Changes in v3: >>> - Address comments on Awb from Stefan: >>> - Introduce AwbImplementation::Result >>> - Move AwbStats implementation to IPAs >>> - Drop AwbStats::rg()/bg() and re-implement AwbGray accordingly >>> - Fix Saturation bug in Simple Ccm algorithm >>> - Fix Simple Ccm gain limits using Q<2, 8> >>> - Update Mali CCM and Gamma to latest uAPI patch version >>> - Fix Mali CCM gain masking as reported by Dan >>> - Break down libIPA Lsc implementation as requested by Stefan >>> >>> Changes in v2: >>> - Introduce awb::Context to simplify ActiveState and FrameContext >>> definition >>> - Add mali uAPI header for CCM and Gamma >>> - Do not rename Mali functions for hardware configuration in awb and lsc >>> - Spelling and documentation >>> >>> --- > [...]
On Tue, Jul 28, 2026 at 03:10:48PM +0200, Barnabás Pőcze wrote: > 2026. 07. 28. 14:12 keltezéssel, Jacopo Mondi írta: > > Hi Barnabás > > > > On Tue, Jul 28, 2026 at 11:48:51AM +0200, Barnabás Pőcze wrote: > > > Hi > > > > > > 2026. 07. 20. 16:59 keltezéssel, Jacopo Mondi írta: > > > > The series is identical to v5 up to patch 27. The only minor difference > > > > is that I back-tracked on making functions class members but in the > > > > RkISP1 IPA I kept them as static helpers. > > > > > > > > Patches 28 and 29 are new. > > > > > > > > They remove the templating of the LscAlgorithm class hierarchy and > > > > instead implement quantization in the LscAlgorithm class. > > > > > > I have spent some time on this v5 vs. v6 question, and to me it seems there > > > > Thanks a lot > > > > > are no real significant differences. The interpolation is done on the quantized > > > type in either case, the only real difference is whether the quantization happen > > > in the implementation or in the main algorithm. And I think that's largely an > > > > Note there is a third option suggested by Stefan to move quantization > > in the single IPA after the interpolation in the float domain > > That sounds like what I'm suggesting: load and interpolate with `float`, > quantize the interpolated data "in the IPA module" right before applying. > Even if we use std::transform<> doesn't it mean we have to walk all the vectors and quantize them, while if we quantize once storing the values in the vector before interpolation we avoid that ? That's the bulk of the difference between the two versions, isn't it ? > > > > > > implementation detail that doesn't affect users much. > > > > > > One potential thing that was already brought up is whether it is even possible > > > to do the interpolation on the quantized type? I think as long as we are dealing > > > with unsigned types, it should be fine (so maybe it would be best to add a `static_assert` > > > to `LscAlgorithm` enforcing that), but if a platform appears that needs some kid of > > > "special" quantization from float, then `LscAlgorithm` will have to be adjusted. > > > > > > Initially I thought v6 would defer quantization even more by doing everything with > > > floats and only quantizing the final interpolated data. I feel like that's a nice > > > > That would require moving quantization completely to the IPAs, right ? > > I was thinking of something like this: > > struct Components { > std::map<std::string, std::vector<float>, std::less<>> sets; > > template<typename T> > void quantize(std::string_key key, typename T::QuantizedType *dst) > { > const auto &s = sets.at(key); > std::transform(s.begin(), s.end(), dst, [](float x) { return T(x).quantized(); }); > } > }; > > ... > > void LensShadingCorrection::copyTable(rkisp1_cif_isp_lsc_config &config, > const lsc::Components &set) > { > set.quantize<UQ<2, 10>>("r", &config.r_data_tbl[0][0]); > ... > } > That's nice. I had starting by defining Components with 'using' instead of defining them as a type. I can expand their definition to also do the quantization. > > > > > approach that avoids any details of quantization until the last point, but of course > > > it has the largest performance penalty as now each time the lsc tables change float > > > interpolation and quantization is carried out, instead of just the integer interpolation > > > of v5 and v6. Yes, and let me point out that v5 did the quantization when populating the vector of gains (when expanding polynomial or (in the future) re-sampling the lsc table), while v6 does that by walking the vectors after they have been populated by LscPolynomial/LscTable. > > > > > > > > > > > So we now expand polynomials and parse tables as floats, store them > > > > in a map and in LscAlgorithm we re-iterate over it to either quantized > > > > them in the polynomial case, or simply cast floats to the register bit > > > > width if table. > > > > > > > > Timing the duration of the whole RkISP1 configure operation, it now > > > > takes 3 msec compared to 600 usec. > > > > > > > > RkISP1Lsc lsc.cpp:171 Configure took = 3081[µs] > > > > > > > > RkISP1Lsc lsc.cpp:172 Configure took = 583[µs] > > > > > > I'm honestly a bit surprised that there is a 5x time penalty here... is there > > > any chance you could attach the tuning file that you used to measure the time? > > > > Sure, I think it's just the imx219 tuning file we have in the mainline > > tree > > > > From the branch I sent to the ml: > > https://gitlab.freedesktop.org/camera/libcamera/-/blob/patchwork/6067/src/ipa/rkisp1/data/imx219.yaml > > > > > > > > > > > > > > > > Also, now each IPA has to define their own Interpolator::interpolate() > > > > overload (see rkisp1 IPA as an example of this). > > > > > > It seems to me that the it should be possible to provide the specializations > > > in a common place like in v5 without making every ipa specialize it itself > > > (which will eventually lead to duplicates). The "Components" type only > > > depends on the quantized type, but currently there is an unnecessary dependency > > > on the fixed point representation because it is defined inside `LscAlgorithm<U>`, > > > moving it out would remove this dependency and would allow specialization in a > > > common place, like in v5. > > > > > > > > > > > > > > As the interface of LscAlgorithm has changed, patches on the list that > > > > port the LSC algorithm of other platforms based on v5 will have to be rebased. > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > --- > > > > > > So in summary, assuming the plan is to move the table based tuning files to > > > floating point number, I think my preference would be deferring the quantization > > > as much as possible (until after interpolation) and dealing with floats. However, > > > although I have not tested it, I feel the runtime cost of that would be too big > > > compared to the other approaches. So my (very) slight preference is for v6 with two caveats: > > > * the interpolation can be moved to a common place like in v5 (as mentioned above); > > > > I presume there is way to do that by moving Components<> out of > > LscAlgorithm. As you noted, I have defined it inside the class because > > of the template dependency. > > My point is that I don't think there is a real dependency, I'm attaching a proof > of concept patch that should apply to v6. There isn't if you have the IPA provide to the Components.quantize() function the Q format set.quantize<UQ<2, 10>>("r", &config.r_data_tbl[0][0]); As LscAlgorithm is already templated with the Q format it would be nice to re-use this instead of having to specify. Alternatively, let's remove the templating completely from LscAlgorithm and only template quantize() with the Q format. This still requires you to 1) LscImplementation: expand polynomial/resample grid in float 2) IPA module: interpolate on floats 3) IPA module: quantize the interpolated values while v5: 1) LscImplementation: expand polynomial/resample grid and quantize 2) IPA module: interpolate on quantized in v6: 1) LscImplementation: expand polynomial/resample grid in floats 2) LscAlgorithm: quantize values 3) IPA module: interpolate on quantized > > > > > > > * I would like take a look at the 5x runtime gap. > > > > The latest version is available in the above branch while v5 is > > available in > > https://gitlab.freedesktop.org/camera/libcamera/-/tree/patchwork/6048/ > > > > The timing measurement was performed with: > > > > ------------------------------------------------------------------------------- > > --- a/src/ipa/rkisp1/algorithms/lsc.cpp > > +++ b/src/ipa/rkisp1/algorithms/lsc.cpp > > @@ -140,6 +140,8 @@ int LensShadingCorrection::configure(IPAContext &context, > > const Size &size = context.configuration.sensor.size; > > Size totalSize{}; > > > > +std::chrono::high_resolution_clock::time_point begin = std::chrono::high_resolution_clock::now(); > > + > > for (unsigned int i = 0; i < RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE; ++i) { > > xSizes_[i] = xSize_[i] * size.width; > > ySizes_[i] = ySize_[i] * size.height; > > @@ -162,8 +164,13 @@ int LensShadingCorrection::configure(IPAContext &context, > > yGrad_[i] = std::round(32768 / ySizes_[i]); > > } > > > > - return lscAlgo_.configure(context.activeState.lsc, configInfo.analogCrop, > > + lscAlgo_.configure(context.activeState.lsc, configInfo.analogCrop, > > xPos_, yPos_); > > + std::chrono::high_resolution_clock::time_point end = std::chrono::high_resolution_clock::now(); > > + > > + LOG(RkISP1Lsc, Error) << "Configure took = " << std::chrono::duration_cast<std::chrono::microseconds>(end - begin).count() << "[µs]"; > > + > > + return 0; > > } > > Interesting, I'm only measuring the `lscAlgo_.configure()` call because the parts before > seem to be the same, and I'm getting around ~120us for v5 and ~140us for v6 > with imx219.yml. With the above patch applied on top of v6: ERROR RkISP1Lsc lsc.cpp:171 Configure took = 3052[µs] What am I doing differently ? > > > > > > void LensShadingCorrection::setParameters(rkisp1_cif_isp_lsc_config &config) > > ------------------------------------------------------------------------------- > > > > > > > > > > > > > > > > Changes in v6: > > > > - Address comments received on v5, mostly on documentation > > > > - Remove templating of LscPolynomial and perform quantization in > > > > LscAlgorithm > > > > > > > > Changes in v5: > > > > - Add "ipa: libipa: lsc_polynomial: Fix polynomial parsing error" > > > > to fix and exiting potential issue with Polynomial parsing > > > > - Fix Simple, which I broke in v4, thanks Milan > > > > > > > > Changes in v4: > > > > - s/awb/AWB in comments everywhere, same for CCM and LSC > > > > - Awb: changelog per patch > > > > - Make AwbStats pure virtual and move stats parsing logic to the IPA > > > > modules > > > > - Reduce comments/renames to reduce patch size and comments > > > > - Collect tags > > > > > > > > Changes in v3: > > > > - Address comments on Awb from Stefan: > > > > - Introduce AwbImplementation::Result > > > > - Move AwbStats implementation to IPAs > > > > - Drop AwbStats::rg()/bg() and re-implement AwbGray accordingly > > > > - Fix Saturation bug in Simple Ccm algorithm > > > > - Fix Simple Ccm gain limits using Q<2, 8> > > > > - Update Mali CCM and Gamma to latest uAPI patch version > > > > - Fix Mali CCM gain masking as reported by Dan > > > > - Break down libIPA Lsc implementation as requested by Stefan > > > > > > > > Changes in v2: > > > > - Introduce awb::Context to simplify ActiveState and FrameContext > > > > definition > > > > - Add mali uAPI header for CCM and Gamma > > > > - Do not rename Mali functions for hardware configuration in awb and lsc > > > > - Spelling and documentation > > > > > > > > --- > > [...]
2026. 07. 28. 15:42 keltezéssel, Jacopo Mondi írta: > On Tue, Jul 28, 2026 at 03:10:48PM +0200, Barnabás Pőcze wrote: >> 2026. 07. 28. 14:12 keltezéssel, Jacopo Mondi írta: >>> Hi Barnabás >>> >>> On Tue, Jul 28, 2026 at 11:48:51AM +0200, Barnabás Pőcze wrote: >>>> Hi >>>> >>>> 2026. 07. 20. 16:59 keltezéssel, Jacopo Mondi írta: >>>>> The series is identical to v5 up to patch 27. The only minor difference >>>>> is that I back-tracked on making functions class members but in the >>>>> RkISP1 IPA I kept them as static helpers. >>>>> >>>>> Patches 28 and 29 are new. >>>>> >>>>> They remove the templating of the LscAlgorithm class hierarchy and >>>>> instead implement quantization in the LscAlgorithm class. >>>> >>>> I have spent some time on this v5 vs. v6 question, and to me it seems there >>> >>> Thanks a lot >>> >>>> are no real significant differences. The interpolation is done on the quantized >>>> type in either case, the only real difference is whether the quantization happen >>>> in the implementation or in the main algorithm. And I think that's largely an >>> >>> Note there is a third option suggested by Stefan to move quantization >>> in the single IPA after the interpolation in the float domain >> >> That sounds like what I'm suggesting: load and interpolate with `float`, >> quantize the interpolated data "in the IPA module" right before applying. >> > > Even if we use std::transform<> doesn't it mean we have to walk all > the vectors and quantize them, while if we quantize once storing the > values in the vector before interpolation we avoid that ? > > That's the bulk of the difference between the two versions, isn't it ? Yes, and that's why I have mostly disregarded this approach in my first reply because while I kind of prefer the design, I think the runtime cost is probably not worth it -- assuming interpolation on the quantized values is possible, which it seems to be. > >> >>> >>>> implementation detail that doesn't affect users much. >>>> >>>> One potential thing that was already brought up is whether it is even possible >>>> to do the interpolation on the quantized type? I think as long as we are dealing >>>> with unsigned types, it should be fine (so maybe it would be best to add a `static_assert` >>>> to `LscAlgorithm` enforcing that), but if a platform appears that needs some kid of >>>> "special" quantization from float, then `LscAlgorithm` will have to be adjusted. >>>> >>>> Initially I thought v6 would defer quantization even more by doing everything with >>>> floats and only quantizing the final interpolated data. I feel like that's a nice >>> >>> That would require moving quantization completely to the IPAs, right ? >> >> I was thinking of something like this: >> >> struct Components { >> std::map<std::string, std::vector<float>, std::less<>> sets; >> >> template<typename T> >> void quantize(std::string_key key, typename T::QuantizedType *dst) >> { >> const auto &s = sets.at(key); >> std::transform(s.begin(), s.end(), dst, [](float x) { return T(x).quantized(); }); >> } >> }; >> >> ... >> >> void LensShadingCorrection::copyTable(rkisp1_cif_isp_lsc_config &config, >> const lsc::Components &set) >> { >> set.quantize<UQ<2, 10>>("r", &config.r_data_tbl[0][0]); >> ... >> } >> > > That's nice. > > I had starting by defining Components with 'using' instead of defining > them as a type. I can expand their definition to also do the > quantization. > >>> >>>> approach that avoids any details of quantization until the last point, but of course >>>> it has the largest performance penalty as now each time the lsc tables change float >>>> interpolation and quantization is carried out, instead of just the integer interpolation >>>> of v5 and v6. > > Yes, and let me point out that v5 did the quantization when populating > the vector of gains (when expanding polynomial or (in the future) > re-sampling the lsc table), while v6 does that by walking the vectors > after they have been populated by LscPolynomial/LscTable. > > >>>> >>>>> >>>>> So we now expand polynomials and parse tables as floats, store them >>>>> in a map and in LscAlgorithm we re-iterate over it to either quantized >>>>> them in the polynomial case, or simply cast floats to the register bit >>>>> width if table. >>>>> >>>>> Timing the duration of the whole RkISP1 configure operation, it now >>>>> takes 3 msec compared to 600 usec. >>>>> >>>>> RkISP1Lsc lsc.cpp:171 Configure took = 3081[µs] >>>>> >>>>> RkISP1Lsc lsc.cpp:172 Configure took = 583[µs] >>>> >>>> I'm honestly a bit surprised that there is a 5x time penalty here... is there >>>> any chance you could attach the tuning file that you used to measure the time? >>> >>> Sure, I think it's just the imx219 tuning file we have in the mainline >>> tree >>> >>> From the branch I sent to the ml: >>> https://gitlab.freedesktop.org/camera/libcamera/-/blob/patchwork/6067/src/ipa/rkisp1/data/imx219.yaml >>> >>>> >>>> >>>>> >>>>> Also, now each IPA has to define their own Interpolator::interpolate() >>>>> overload (see rkisp1 IPA as an example of this). >>>> >>>> It seems to me that the it should be possible to provide the specializations >>>> in a common place like in v5 without making every ipa specialize it itself >>>> (which will eventually lead to duplicates). The "Components" type only >>>> depends on the quantized type, but currently there is an unnecessary dependency >>>> on the fixed point representation because it is defined inside `LscAlgorithm<U>`, >>>> moving it out would remove this dependency and would allow specialization in a >>>> common place, like in v5. >>>> >>>> >>>>> >>>>> As the interface of LscAlgorithm has changed, patches on the list that >>>>> port the LSC algorithm of other platforms based on v5 will have to be rebased. >>>>> >>>>> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> >>>>> --- >>>> >>>> So in summary, assuming the plan is to move the table based tuning files to >>>> floating point number, I think my preference would be deferring the quantization >>>> as much as possible (until after interpolation) and dealing with floats. However, >>>> although I have not tested it, I feel the runtime cost of that would be too big >>>> compared to the other approaches. So my (very) slight preference is for v6 with two caveats: >>>> * the interpolation can be moved to a common place like in v5 (as mentioned above); >>> >>> I presume there is way to do that by moving Components<> out of >>> LscAlgorithm. As you noted, I have defined it inside the class because >>> of the template dependency. >> >> My point is that I don't think there is a real dependency, I'm attaching a proof >> of concept patch that should apply to v6. > > There isn't if you have the IPA provide to the Components.quantize() > function the Q format > > set.quantize<UQ<2, 10>>("r", &config.r_data_tbl[0][0]); > > As LscAlgorithm is already templated with the Q format it would be > nice to re-use this instead of having to specify. The way I see it, this `quantize` function in `Components` is only needed in the "third" (not currently implemented approach) that uses floats everywhere. In that case `LscAlgorithm` wouldn't be a template, and it would work with floats all the way. > > Alternatively, let's remove the templating completely from > LscAlgorithm and only template quantize() with the Q format. > > This still requires you to > > 1) LscImplementation: expand polynomial/resample grid in float > 2) IPA module: interpolate on floats > 3) IPA module: quantize the interpolated values > > while v5: > > 1) LscImplementation: expand polynomial/resample grid and quantize > 2) IPA module: interpolate on quantized > > in v6: > > 1) LscImplementation: expand polynomial/resample grid in floats > 2) LscAlgorithm: quantize values > 3) IPA module: interpolate on quantized > > >> >> >>> >>>> * I would like take a look at the 5x runtime gap. >>> >>> The latest version is available in the above branch while v5 is >>> available in >>> https://gitlab.freedesktop.org/camera/libcamera/-/tree/patchwork/6048/ >>> >>> The timing measurement was performed with: >>> >>> ------------------------------------------------------------------------------- >>> --- a/src/ipa/rkisp1/algorithms/lsc.cpp >>> +++ b/src/ipa/rkisp1/algorithms/lsc.cpp >>> @@ -140,6 +140,8 @@ int LensShadingCorrection::configure(IPAContext &context, >>> const Size &size = context.configuration.sensor.size; >>> Size totalSize{}; >>> >>> +std::chrono::high_resolution_clock::time_point begin = std::chrono::high_resolution_clock::now(); >>> + >>> for (unsigned int i = 0; i < RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE; ++i) { >>> xSizes_[i] = xSize_[i] * size.width; >>> ySizes_[i] = ySize_[i] * size.height; >>> @@ -162,8 +164,13 @@ int LensShadingCorrection::configure(IPAContext &context, >>> yGrad_[i] = std::round(32768 / ySizes_[i]); >>> } >>> >>> - return lscAlgo_.configure(context.activeState.lsc, configInfo.analogCrop, >>> + lscAlgo_.configure(context.activeState.lsc, configInfo.analogCrop, >>> xPos_, yPos_); >>> + std::chrono::high_resolution_clock::time_point end = std::chrono::high_resolution_clock::now(); >>> + >>> + LOG(RkISP1Lsc, Error) << "Configure took = " << std::chrono::duration_cast<std::chrono::microseconds>(end - begin).count() << "[µs]"; >>> + >>> + return 0; >>> } >> >> Interesting, I'm only measuring the `lscAlgo_.configure()` call because the parts before >> seem to be the same, and I'm getting around ~120us for v5 and ~140us for v6 >> with imx219.yml. > > With the above patch applied on top of v6: > > ERROR RkISP1Lsc lsc.cpp:171 Configure took = 3052[µs] > > What am I doing differently ? I'll test again. > >> >> >>> >>> void LensShadingCorrection::setParameters(rkisp1_cif_isp_lsc_config &config) >>> ------------------------------------------------------------------------------- >>> >>> >>> >>>> >>>> >>>>> Changes in v6: >>>>> - Address comments received on v5, mostly on documentation >>>>> - Remove templating of LscPolynomial and perform quantization in >>>>> LscAlgorithm >>>>> >>>>> Changes in v5: >>>>> - Add "ipa: libipa: lsc_polynomial: Fix polynomial parsing error" >>>>> to fix and exiting potential issue with Polynomial parsing >>>>> - Fix Simple, which I broke in v4, thanks Milan >>>>> >>>>> Changes in v4: >>>>> - s/awb/AWB in comments everywhere, same for CCM and LSC >>>>> - Awb: changelog per patch >>>>> - Make AwbStats pure virtual and move stats parsing logic to the IPA >>>>> modules >>>>> - Reduce comments/renames to reduce patch size and comments >>>>> - Collect tags >>>>> >>>>> Changes in v3: >>>>> - Address comments on Awb from Stefan: >>>>> - Introduce AwbImplementation::Result >>>>> - Move AwbStats implementation to IPAs >>>>> - Drop AwbStats::rg()/bg() and re-implement AwbGray accordingly >>>>> - Fix Saturation bug in Simple Ccm algorithm >>>>> - Fix Simple Ccm gain limits using Q<2, 8> >>>>> - Update Mali CCM and Gamma to latest uAPI patch version >>>>> - Fix Mali CCM gain masking as reported by Dan >>>>> - Break down libIPA Lsc implementation as requested by Stefan >>>>> >>>>> Changes in v2: >>>>> - Introduce awb::Context to simplify ActiveState and FrameContext >>>>> definition >>>>> - Add mali uAPI header for CCM and Gamma >>>>> - Do not rename Mali functions for hardware configuration in awb and lsc >>>>> - Spelling and documentation >>>>> >>>>> --- >>> [...] > >
Hi Barnabás On Tue, Jul 28, 2026 at 03:57:31PM +0200, Barnabás Pőcze wrote: > 2026. 07. 28. 15:42 keltezéssel, Jacopo Mondi írta: > > On Tue, Jul 28, 2026 at 03:10:48PM +0200, Barnabás Pőcze wrote: > > > 2026. 07. 28. 14:12 keltezéssel, Jacopo Mondi írta: > > > > Hi Barnabás > > > > > > > > On Tue, Jul 28, 2026 at 11:48:51AM +0200, Barnabás Pőcze wrote: > > > > > Hi > > > > > > > > > > 2026. 07. 20. 16:59 keltezéssel, Jacopo Mondi írta: > > > > > > The series is identical to v5 up to patch 27. The only minor difference > > > > > > is that I back-tracked on making functions class members but in the > > > > > > RkISP1 IPA I kept them as static helpers. > > > > > > > > > > > > Patches 28 and 29 are new. > > > > > > > > > > > > They remove the templating of the LscAlgorithm class hierarchy and > > > > > > instead implement quantization in the LscAlgorithm class. > > > > > > > > > > I have spent some time on this v5 vs. v6 question, and to me it seems there > > > > > > > > Thanks a lot > > > > > > > > > are no real significant differences. The interpolation is done on the quantized > > > > > type in either case, the only real difference is whether the quantization happen > > > > > in the implementation or in the main algorithm. And I think that's largely an > > > > > > > > Note there is a third option suggested by Stefan to move quantization > > > > in the single IPA after the interpolation in the float domain > > > > > > That sounds like what I'm suggesting: load and interpolate with `float`, > > > quantize the interpolated data "in the IPA module" right before applying. > > > > > > > Even if we use std::transform<> doesn't it mean we have to walk all > > the vectors and quantize them, while if we quantize once storing the > > values in the vector before interpolation we avoid that ? > > > > That's the bulk of the difference between the two versions, isn't it ? > > Yes, and that's why I have mostly disregarded this approach in my first > reply because while I kind of prefer the design, I think the runtime > cost is probably not worth it -- assuming interpolation on the quantized > values is possible, which it seems to be. > > > > > > > > > > > > > > > > implementation detail that doesn't affect users much. > > > > > > > > > > One potential thing that was already brought up is whether it is even possible > > > > > to do the interpolation on the quantized type? I think as long as we are dealing > > > > > with unsigned types, it should be fine (so maybe it would be best to add a `static_assert` > > > > > to `LscAlgorithm` enforcing that), but if a platform appears that needs some kid of > > > > > "special" quantization from float, then `LscAlgorithm` will have to be adjusted. > > > > > > > > > > Initially I thought v6 would defer quantization even more by doing everything with > > > > > floats and only quantizing the final interpolated data. I feel like that's a nice > > > > > > > > That would require moving quantization completely to the IPAs, right ? > > > > > > I was thinking of something like this: > > > > > > struct Components { > > > std::map<std::string, std::vector<float>, std::less<>> sets; > > > > > > template<typename T> > > > void quantize(std::string_key key, typename T::QuantizedType *dst) > > > { > > > const auto &s = sets.at(key); > > > std::transform(s.begin(), s.end(), dst, [](float x) { return T(x).quantized(); }); > > > } > > > }; > > > > > > ... > > > > > > void LensShadingCorrection::copyTable(rkisp1_cif_isp_lsc_config &config, > > > const lsc::Components &set) > > > { > > > set.quantize<UQ<2, 10>>("r", &config.r_data_tbl[0][0]); > > > ... > > > } > > > > > > > That's nice. > > > > I had starting by defining Components with 'using' instead of defining > > them as a type. I can expand their definition to also do the > > quantization. > > > > > > > > > > > approach that avoids any details of quantization until the last point, but of course > > > > > it has the largest performance penalty as now each time the lsc tables change float > > > > > interpolation and quantization is carried out, instead of just the integer interpolation > > > > > of v5 and v6. > > > > Yes, and let me point out that v5 did the quantization when populating > > the vector of gains (when expanding polynomial or (in the future) > > re-sampling the lsc table), while v6 does that by walking the vectors > > after they have been populated by LscPolynomial/LscTable. > > > > > > > > > > > > > > > > > > > > > So we now expand polynomials and parse tables as floats, store them > > > > > > in a map and in LscAlgorithm we re-iterate over it to either quantized > > > > > > them in the polynomial case, or simply cast floats to the register bit > > > > > > width if table. > > > > > > > > > > > > Timing the duration of the whole RkISP1 configure operation, it now > > > > > > takes 3 msec compared to 600 usec. > > > > > > > > > > > > RkISP1Lsc lsc.cpp:171 Configure took = 3081[µs] > > > > > > > > > > > > RkISP1Lsc lsc.cpp:172 Configure took = 583[µs] > > > > > > > > > > I'm honestly a bit surprised that there is a 5x time penalty here... is there > > > > > any chance you could attach the tuning file that you used to measure the time? > > > > > > > > Sure, I think it's just the imx219 tuning file we have in the mainline > > > > tree > > > > > > > > From the branch I sent to the ml: > > > > https://gitlab.freedesktop.org/camera/libcamera/-/blob/patchwork/6067/src/ipa/rkisp1/data/imx219.yaml > > > > > > > > > > > > > > > > > > > > > > > > > > Also, now each IPA has to define their own Interpolator::interpolate() > > > > > > overload (see rkisp1 IPA as an example of this). > > > > > > > > > > It seems to me that the it should be possible to provide the specializations > > > > > in a common place like in v5 without making every ipa specialize it itself > > > > > (which will eventually lead to duplicates). The "Components" type only > > > > > depends on the quantized type, but currently there is an unnecessary dependency > > > > > on the fixed point representation because it is defined inside `LscAlgorithm<U>`, > > > > > moving it out would remove this dependency and would allow specialization in a > > > > > common place, like in v5. > > > > > > > > > > > > > > > > > > > > > > As the interface of LscAlgorithm has changed, patches on the list that > > > > > > port the LSC algorithm of other platforms based on v5 will have to be rebased. > > > > > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > > > --- > > > > > > > > > > So in summary, assuming the plan is to move the table based tuning files to > > > > > floating point number, I think my preference would be deferring the quantization > > > > > as much as possible (until after interpolation) and dealing with floats. However, > > > > > although I have not tested it, I feel the runtime cost of that would be too big > > > > > compared to the other approaches. So my (very) slight preference is for v6 with two caveats: > > > > > * the interpolation can be moved to a common place like in v5 (as mentioned above); > > > > > > > > I presume there is way to do that by moving Components<> out of > > > > LscAlgorithm. As you noted, I have defined it inside the class because > > > > of the template dependency. > > > > > > My point is that I don't think there is a real dependency, I'm attaching a proof > > > of concept patch that should apply to v6. > > > > There isn't if you have the IPA provide to the Components.quantize() > > function the Q format > > > > set.quantize<UQ<2, 10>>("r", &config.r_data_tbl[0][0]); > > > > As LscAlgorithm is already templated with the Q format it would be > > nice to re-use this instead of having to specify. > > The way I see it, this `quantize` function in `Components` is only needed in the > "third" (not currently implemented approach) that uses floats everywhere. In that > case `LscAlgorithm` wouldn't be a template, and it would work with floats all the way. > > > > > > Alternatively, let's remove the templating completely from > > LscAlgorithm and only template quantize() with the Q format. > > > > This still requires you to > > > > 1) LscImplementation: expand polynomial/resample grid in float > > 2) IPA module: interpolate on floats > > 3) IPA module: quantize the interpolated values > > So basically you mean this one.. I can give it a go, so we can compare all three designs as v5, v6 and v7 :) > > while v5: > > > > 1) LscImplementation: expand polynomial/resample grid and quantize > > 2) IPA module: interpolate on quantized > > > > in v6: > > > > 1) LscImplementation: expand polynomial/resample grid in floats > > 2) LscAlgorithm: quantize values > > 3) IPA module: interpolate on quantized > > > > > > > > > > > > > > > > > > > * I would like take a look at the 5x runtime gap. > > > > > > > > The latest version is available in the above branch while v5 is > > > > available in > > > > https://gitlab.freedesktop.org/camera/libcamera/-/tree/patchwork/6048/ > > > > > > > > The timing measurement was performed with: > > > > > > > > ------------------------------------------------------------------------------- > > > > --- a/src/ipa/rkisp1/algorithms/lsc.cpp > > > > +++ b/src/ipa/rkisp1/algorithms/lsc.cpp > > > > @@ -140,6 +140,8 @@ int LensShadingCorrection::configure(IPAContext &context, > > > > const Size &size = context.configuration.sensor.size; > > > > Size totalSize{}; > > > > > > > > +std::chrono::high_resolution_clock::time_point begin = std::chrono::high_resolution_clock::now(); > > > > + > > > > for (unsigned int i = 0; i < RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE; ++i) { > > > > xSizes_[i] = xSize_[i] * size.width; > > > > ySizes_[i] = ySize_[i] * size.height; > > > > @@ -162,8 +164,13 @@ int LensShadingCorrection::configure(IPAContext &context, > > > > yGrad_[i] = std::round(32768 / ySizes_[i]); > > > > } > > > > > > > > - return lscAlgo_.configure(context.activeState.lsc, configInfo.analogCrop, > > > > + lscAlgo_.configure(context.activeState.lsc, configInfo.analogCrop, > > > > xPos_, yPos_); > > > > + std::chrono::high_resolution_clock::time_point end = std::chrono::high_resolution_clock::now(); > > > > + > > > > + LOG(RkISP1Lsc, Error) << "Configure took = " << std::chrono::duration_cast<std::chrono::microseconds>(end - begin).count() << "[µs]"; > > > > + > > > > + return 0; > > > > } > > > > > > Interesting, I'm only measuring the `lscAlgo_.configure()` call because the parts before > > > seem to be the same, and I'm getting around ~120us for v5 and ~140us for v6 > > > with imx219.yml. > > > > With the above patch applied on top of v6: > > > > ERROR RkISP1Lsc lsc.cpp:171 Configure took = 3052[µs] > > > > What am I doing differently ? > > I'll test again. > > > > > > > > > > > > > > > > > > void LensShadingCorrection::setParameters(rkisp1_cif_isp_lsc_config &config) > > > > ------------------------------------------------------------------------------- > > > > > > > > > > > > > > > > > > > > > > > > > > > > Changes in v6: > > > > > > - Address comments received on v5, mostly on documentation > > > > > > - Remove templating of LscPolynomial and perform quantization in > > > > > > LscAlgorithm > > > > > > > > > > > > Changes in v5: > > > > > > - Add "ipa: libipa: lsc_polynomial: Fix polynomial parsing error" > > > > > > to fix and exiting potential issue with Polynomial parsing > > > > > > - Fix Simple, which I broke in v4, thanks Milan > > > > > > > > > > > > Changes in v4: > > > > > > - s/awb/AWB in comments everywhere, same for CCM and LSC > > > > > > - Awb: changelog per patch > > > > > > - Make AwbStats pure virtual and move stats parsing logic to the IPA > > > > > > modules > > > > > > - Reduce comments/renames to reduce patch size and comments > > > > > > - Collect tags > > > > > > > > > > > > Changes in v3: > > > > > > - Address comments on Awb from Stefan: > > > > > > - Introduce AwbImplementation::Result > > > > > > - Move AwbStats implementation to IPAs > > > > > > - Drop AwbStats::rg()/bg() and re-implement AwbGray accordingly > > > > > > - Fix Saturation bug in Simple Ccm algorithm > > > > > > - Fix Simple Ccm gain limits using Q<2, 8> > > > > > > - Update Mali CCM and Gamma to latest uAPI patch version > > > > > > - Fix Mali CCM gain masking as reported by Dan > > > > > > - Break down libIPA Lsc implementation as requested by Stefan > > > > > > > > > > > > Changes in v2: > > > > > > - Introduce awb::Context to simplify ActiveState and FrameContext > > > > > > definition > > > > > > - Add mali uAPI header for CCM and Gamma > > > > > > - Do not rename Mali functions for hardware configuration in awb and lsc > > > > > > - Spelling and documentation > > > > > > > > > > > > --- > > > > [...] > > > > >
One small correction On Tue, Jul 28, 2026 at 04:04:52PM +0200, Jacopo Mondi wrote: > Hi Barnabás > > On Tue, Jul 28, 2026 at 03:57:31PM +0200, Barnabás Pőcze wrote: > > 2026. 07. 28. 15:42 keltezéssel, Jacopo Mondi írta: > > > On Tue, Jul 28, 2026 at 03:10:48PM +0200, Barnabás Pőcze wrote: > > > > 2026. 07. 28. 14:12 keltezéssel, Jacopo Mondi írta: > > > > > Hi Barnabás > > > > > > > > > > On Tue, Jul 28, 2026 at 11:48:51AM +0200, Barnabás Pőcze wrote: > > > > > > Hi > > > > > > > > > > > > 2026. 07. 20. 16:59 keltezéssel, Jacopo Mondi írta: > > > > > > > The series is identical to v5 up to patch 27. The only minor difference > > > > > > > is that I back-tracked on making functions class members but in the > > > > > > > RkISP1 IPA I kept them as static helpers. > > > > > > > > > > > > > > Patches 28 and 29 are new. > > > > > > > > > > > > > > They remove the templating of the LscAlgorithm class hierarchy and > > > > > > > instead implement quantization in the LscAlgorithm class. > > > > > > > > > > > > I have spent some time on this v5 vs. v6 question, and to me it seems there > > > > > > > > > > Thanks a lot > > > > > > > > > > > are no real significant differences. The interpolation is done on the quantized > > > > > > type in either case, the only real difference is whether the quantization happen > > > > > > in the implementation or in the main algorithm. And I think that's largely an > > > > > > > > > > Note there is a third option suggested by Stefan to move quantization > > > > > in the single IPA after the interpolation in the float domain > > > > > > > > That sounds like what I'm suggesting: load and interpolate with `float`, > > > > quantize the interpolated data "in the IPA module" right before applying. > > > > > > > > > > Even if we use std::transform<> doesn't it mean we have to walk all > > > the vectors and quantize them, while if we quantize once storing the > > > values in the vector before interpolation we avoid that ? > > > > > > That's the bulk of the difference between the two versions, isn't it ? > > > > Yes, and that's why I have mostly disregarded this approach in my first > > reply because while I kind of prefer the design, I think the runtime > > cost is probably not worth it -- assuming interpolation on the quantized > > values is possible, which it seems to be. > > > > > > > > > > > > > > > > > > > > > > implementation detail that doesn't affect users much. > > > > > > > > > > > > One potential thing that was already brought up is whether it is even possible > > > > > > to do the interpolation on the quantized type? I think as long as we are dealing > > > > > > with unsigned types, it should be fine (so maybe it would be best to add a `static_assert` > > > > > > to `LscAlgorithm` enforcing that), but if a platform appears that needs some kid of > > > > > > "special" quantization from float, then `LscAlgorithm` will have to be adjusted. > > > > > > > > > > > > Initially I thought v6 would defer quantization even more by doing everything with > > > > > > floats and only quantizing the final interpolated data. I feel like that's a nice > > > > > > > > > > That would require moving quantization completely to the IPAs, right ? > > > > > > > > I was thinking of something like this: > > > > > > > > struct Components { > > > > std::map<std::string, std::vector<float>, std::less<>> sets; > > > > > > > > template<typename T> > > > > void quantize(std::string_key key, typename T::QuantizedType *dst) > > > > { > > > > const auto &s = sets.at(key); > > > > std::transform(s.begin(), s.end(), dst, [](float x) { return T(x).quantized(); }); > > > > } > > > > }; > > > > > > > > ... > > > > > > > > void LensShadingCorrection::copyTable(rkisp1_cif_isp_lsc_config &config, > > > > const lsc::Components &set) > > > > { > > > > set.quantize<UQ<2, 10>>("r", &config.r_data_tbl[0][0]); > > > > ... > > > > } > > > > > > > > > > That's nice. > > > > > > I had starting by defining Components with 'using' instead of defining > > > them as a type. I can expand their definition to also do the > > > quantization. > > > > > > > > > > > > > > approach that avoids any details of quantization until the last point, but of course > > > > > > it has the largest performance penalty as now each time the lsc tables change float > > > > > > interpolation and quantization is carried out, instead of just the integer interpolation > > > > > > of v5 and v6. > > > > > > Yes, and let me point out that v5 did the quantization when populating > > > the vector of gains (when expanding polynomial or (in the future) > > > re-sampling the lsc table), while v6 does that by walking the vectors > > > after they have been populated by LscPolynomial/LscTable. > > > > > > > > > > > > > > > > > > > > > > > > > > So we now expand polynomials and parse tables as floats, store them > > > > > > > in a map and in LscAlgorithm we re-iterate over it to either quantized > > > > > > > them in the polynomial case, or simply cast floats to the register bit > > > > > > > width if table. > > > > > > > > > > > > > > Timing the duration of the whole RkISP1 configure operation, it now > > > > > > > takes 3 msec compared to 600 usec. > > > > > > > > > > > > > > RkISP1Lsc lsc.cpp:171 Configure took = 3081[µs] > > > > > > > > > > > > > > RkISP1Lsc lsc.cpp:172 Configure took = 583[µs] > > > > > > > > > > > > I'm honestly a bit surprised that there is a 5x time penalty here... is there > > > > > > any chance you could attach the tuning file that you used to measure the time? > > > > > > > > > > Sure, I think it's just the imx219 tuning file we have in the mainline > > > > > tree > > > > > > > > > > From the branch I sent to the ml: > > > > > https://gitlab.freedesktop.org/camera/libcamera/-/blob/patchwork/6067/src/ipa/rkisp1/data/imx219.yaml > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Also, now each IPA has to define their own Interpolator::interpolate() > > > > > > > overload (see rkisp1 IPA as an example of this). > > > > > > > > > > > > It seems to me that the it should be possible to provide the specializations > > > > > > in a common place like in v5 without making every ipa specialize it itself > > > > > > (which will eventually lead to duplicates). The "Components" type only > > > > > > depends on the quantized type, but currently there is an unnecessary dependency > > > > > > on the fixed point representation because it is defined inside `LscAlgorithm<U>`, > > > > > > moving it out would remove this dependency and would allow specialization in a > > > > > > common place, like in v5. > > > > > > > > > > > > > > > > > > > > > > > > > > As the interface of LscAlgorithm has changed, patches on the list that > > > > > > > port the LSC algorithm of other platforms based on v5 will have to be rebased. > > > > > > > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > > > > --- > > > > > > > > > > > > So in summary, assuming the plan is to move the table based tuning files to > > > > > > floating point number, I think my preference would be deferring the quantization > > > > > > as much as possible (until after interpolation) and dealing with floats. However, > > > > > > although I have not tested it, I feel the runtime cost of that would be too big > > > > > > compared to the other approaches. So my (very) slight preference is for v6 with two caveats: > > > > > > * the interpolation can be moved to a common place like in v5 (as mentioned above); > > > > > > > > > > I presume there is way to do that by moving Components<> out of > > > > > LscAlgorithm. As you noted, I have defined it inside the class because > > > > > of the template dependency. > > > > > > > > My point is that I don't think there is a real dependency, I'm attaching a proof > > > > of concept patch that should apply to v6. > > > > > > There isn't if you have the IPA provide to the Components.quantize() > > > function the Q format > > > > > > set.quantize<UQ<2, 10>>("r", &config.r_data_tbl[0][0]); > > > > > > As LscAlgorithm is already templated with the Q format it would be > > > nice to re-use this instead of having to specify. > > > > The way I see it, this `quantize` function in `Components` is only needed in the > > "third" (not currently implemented approach) that uses floats everywhere. In that > > case `LscAlgorithm` wouldn't be a template, and it would work with floats all the way. > > > > > > > > > > Alternatively, let's remove the templating completely from > > > LscAlgorithm and only template quantize() with the Q format. > > > > > > This still requires you to > > > > > > 1) LscImplementation: expand polynomial/resample grid in float > > > 2) IPA module: interpolate on floats > > > 3) IPA module: quantize the interpolated values > > > > > So basically you mean this one.. > > I can give it a go, so we can compare all three designs as v5, v6 and v7 :) > As long as we have tables in quantized format, we can't have LscAlgorithm work with floats. So we need to quantize the result of the polynomial expansion before sending them to the IPA, which means we're left with the two below possibilities: > > > while v5: > > > > > > 1) LscImplementation: expand polynomial/resample grid and quantize > > > 2) IPA module: interpolate on quantized > > > > > > in v6: > > > > > > 1) LscImplementation: expand polynomial/resample grid in floats > > > 2) LscAlgorithm: quantize values > > > 3) IPA module: interpolate on quantized > > > These ones ^ Note that "resample grid" above is not yet implemented and we simply return the (quantized) values as parsed from the tuning file Does it make sense ? Thanks j > > > > > > > > > > > > > > > > > > > > > > * I would like take a look at the 5x runtime gap. > > > > > > > > > > The latest version is available in the above branch while v5 is > > > > > available in > > > > > https://gitlab.freedesktop.org/camera/libcamera/-/tree/patchwork/6048/ > > > > > > > > > > The timing measurement was performed with: > > > > > > > > > > ------------------------------------------------------------------------------- > > > > > --- a/src/ipa/rkisp1/algorithms/lsc.cpp > > > > > +++ b/src/ipa/rkisp1/algorithms/lsc.cpp > > > > > @@ -140,6 +140,8 @@ int LensShadingCorrection::configure(IPAContext &context, > > > > > const Size &size = context.configuration.sensor.size; > > > > > Size totalSize{}; > > > > > > > > > > +std::chrono::high_resolution_clock::time_point begin = std::chrono::high_resolution_clock::now(); > > > > > + > > > > > for (unsigned int i = 0; i < RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE; ++i) { > > > > > xSizes_[i] = xSize_[i] * size.width; > > > > > ySizes_[i] = ySize_[i] * size.height; > > > > > @@ -162,8 +164,13 @@ int LensShadingCorrection::configure(IPAContext &context, > > > > > yGrad_[i] = std::round(32768 / ySizes_[i]); > > > > > } > > > > > > > > > > - return lscAlgo_.configure(context.activeState.lsc, configInfo.analogCrop, > > > > > + lscAlgo_.configure(context.activeState.lsc, configInfo.analogCrop, > > > > > xPos_, yPos_); > > > > > + std::chrono::high_resolution_clock::time_point end = std::chrono::high_resolution_clock::now(); > > > > > + > > > > > + LOG(RkISP1Lsc, Error) << "Configure took = " << std::chrono::duration_cast<std::chrono::microseconds>(end - begin).count() << "[µs]"; > > > > > + > > > > > + return 0; > > > > > } > > > > > > > > Interesting, I'm only measuring the `lscAlgo_.configure()` call because the parts before > > > > seem to be the same, and I'm getting around ~120us for v5 and ~140us for v6 > > > > with imx219.yml. > > > > > > With the above patch applied on top of v6: > > > > > > ERROR RkISP1Lsc lsc.cpp:171 Configure took = 3052[µs] > > > > > > What am I doing differently ? > > > > I'll test again. > > > > > > > > > > > > > > > > > > > > > > > > > void LensShadingCorrection::setParameters(rkisp1_cif_isp_lsc_config &config) > > > > > ------------------------------------------------------------------------------- > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Changes in v6: > > > > > > > - Address comments received on v5, mostly on documentation > > > > > > > - Remove templating of LscPolynomial and perform quantization in > > > > > > > LscAlgorithm > > > > > > > > > > > > > > Changes in v5: > > > > > > > - Add "ipa: libipa: lsc_polynomial: Fix polynomial parsing error" > > > > > > > to fix and exiting potential issue with Polynomial parsing > > > > > > > - Fix Simple, which I broke in v4, thanks Milan > > > > > > > > > > > > > > Changes in v4: > > > > > > > - s/awb/AWB in comments everywhere, same for CCM and LSC > > > > > > > - Awb: changelog per patch > > > > > > > - Make AwbStats pure virtual and move stats parsing logic to the IPA > > > > > > > modules > > > > > > > - Reduce comments/renames to reduce patch size and comments > > > > > > > - Collect tags > > > > > > > > > > > > > > Changes in v3: > > > > > > > - Address comments on Awb from Stefan: > > > > > > > - Introduce AwbImplementation::Result > > > > > > > - Move AwbStats implementation to IPAs > > > > > > > - Drop AwbStats::rg()/bg() and re-implement AwbGray accordingly > > > > > > > - Fix Saturation bug in Simple Ccm algorithm > > > > > > > - Fix Simple Ccm gain limits using Q<2, 8> > > > > > > > - Update Mali CCM and Gamma to latest uAPI patch version > > > > > > > - Fix Mali CCM gain masking as reported by Dan > > > > > > > - Break down libIPA Lsc implementation as requested by Stefan > > > > > > > > > > > > > > Changes in v2: > > > > > > > - Introduce awb::Context to simplify ActiveState and FrameContext > > > > > > > definition > > > > > > > - Add mali uAPI header for CCM and Gamma > > > > > > > - Do not rename Mali functions for hardware configuration in awb and lsc > > > > > > > - Spelling and documentation > > > > > > > > > > > > > > --- > > > > > [...] > > > > > > > >
HI Barnabas On Tue, Jul 28, 2026 at 03:10:48PM +0200, Barnabás Pőcze wrote: > 2026. 07. 28. 14:12 keltezéssel, Jacopo Mondi írta: > > Hi Barnabás > > > > On Tue, Jul 28, 2026 at 11:48:51AM +0200, Barnabás Pőcze wrote: > > > Hi > > > > > > 2026. 07. 20. 16:59 keltezéssel, Jacopo Mondi írta: > > > > The series is identical to v5 up to patch 27. The only minor difference > > > > is that I back-tracked on making functions class members but in the > > > > RkISP1 IPA I kept them as static helpers. > > > > > > > > Patches 28 and 29 are new. > > > > > > > > They remove the templating of the LscAlgorithm class hierarchy and > > > > instead implement quantization in the LscAlgorithm class. > > > > > > I have spent some time on this v5 vs. v6 question, and to me it seems there > > > > Thanks a lot > > > > > are no real significant differences. The interpolation is done on the quantized > > > type in either case, the only real difference is whether the quantization happen > > > in the implementation or in the main algorithm. And I think that's largely an > > > > Note there is a third option suggested by Stefan to move quantization > > in the single IPA after the interpolation in the float domain > > That sounds like what I'm suggesting: load and interpolate with `float`, > quantize the interpolated data "in the IPA module" right before applying. > As of now, tables are already quantized, so was can't assume LscImplementation::ComponentsMap<float> as your patch does. That's where I got stuck when trying to remove the Components definition from LscAlgorithm. I feel like I've tried hard enough to explore all possible options and still I didn't get any real argument against the v5 version if not generic complaints against templates. Or have I missed what your opinion is on that subject.. Should we simply resume v5 and re-consider when we can work with floats in LscImplementation ? (iow when we'll switch format in all tuning files). What do you think ? > > > > > > implementation detail that doesn't affect users much. > > > > > > One potential thing that was already brought up is whether it is even possible > > > to do the interpolation on the quantized type? I think as long as we are dealing > > > with unsigned types, it should be fine (so maybe it would be best to add a `static_assert` > > > to `LscAlgorithm` enforcing that), but if a platform appears that needs some kid of > > > "special" quantization from float, then `LscAlgorithm` will have to be adjusted. > > > > > > Initially I thought v6 would defer quantization even more by doing everything with > > > floats and only quantizing the final interpolated data. I feel like that's a nice > > > > That would require moving quantization completely to the IPAs, right ? > > I was thinking of something like this: > > struct Components { > std::map<std::string, std::vector<float>, std::less<>> sets; > > template<typename T> > void quantize(std::string_key key, typename T::QuantizedType *dst) > { > const auto &s = sets.at(key); > std::transform(s.begin(), s.end(), dst, [](float x) { return T(x).quantized(); }); > } > }; > > ... > > void LensShadingCorrection::copyTable(rkisp1_cif_isp_lsc_config &config, > const lsc::Components &set) > { > set.quantize<UQ<2, 10>>("r", &config.r_data_tbl[0][0]); > ... > } > > > > > > approach that avoids any details of quantization until the last point, but of course > > > it has the largest performance penalty as now each time the lsc tables change float > > > interpolation and quantization is carried out, instead of just the integer interpolation > > > of v5 and v6. > > > > > > > > > > > So we now expand polynomials and parse tables as floats, store them > > > > in a map and in LscAlgorithm we re-iterate over it to either quantized > > > > them in the polynomial case, or simply cast floats to the register bit > > > > width if table. > > > > > > > > Timing the duration of the whole RkISP1 configure operation, it now > > > > takes 3 msec compared to 600 usec. > > > > > > > > RkISP1Lsc lsc.cpp:171 Configure took = 3081[µs] > > > > > > > > RkISP1Lsc lsc.cpp:172 Configure took = 583[µs] > > > > > > I'm honestly a bit surprised that there is a 5x time penalty here... is there > > > any chance you could attach the tuning file that you used to measure the time? > > > > Sure, I think it's just the imx219 tuning file we have in the mainline > > tree > > > > From the branch I sent to the ml: > > https://gitlab.freedesktop.org/camera/libcamera/-/blob/patchwork/6067/src/ipa/rkisp1/data/imx219.yaml > > > > > > > > > > > > > > > > Also, now each IPA has to define their own Interpolator::interpolate() > > > > overload (see rkisp1 IPA as an example of this). > > > > > > It seems to me that the it should be possible to provide the specializations > > > in a common place like in v5 without making every ipa specialize it itself > > > (which will eventually lead to duplicates). The "Components" type only > > > depends on the quantized type, but currently there is an unnecessary dependency > > > on the fixed point representation because it is defined inside `LscAlgorithm<U>`, > > > moving it out would remove this dependency and would allow specialization in a > > > common place, like in v5. > > > > > > > > > > > > > > As the interface of LscAlgorithm has changed, patches on the list that > > > > port the LSC algorithm of other platforms based on v5 will have to be rebased. > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > --- > > > > > > So in summary, assuming the plan is to move the table based tuning files to > > > floating point number, I think my preference would be deferring the quantization > > > as much as possible (until after interpolation) and dealing with floats. However, > > > although I have not tested it, I feel the runtime cost of that would be too big > > > compared to the other approaches. So my (very) slight preference is for v6 with two caveats: > > > * the interpolation can be moved to a common place like in v5 (as mentioned above); > > > > I presume there is way to do that by moving Components<> out of > > LscAlgorithm. As you noted, I have defined it inside the class because > > of the template dependency. > > My point is that I don't think there is a real dependency, I'm attaching a proof > of concept patch that should apply to v6. > > > > > > > * I would like take a look at the 5x runtime gap. > > > > The latest version is available in the above branch while v5 is > > available in > > https://gitlab.freedesktop.org/camera/libcamera/-/tree/patchwork/6048/ > > > > The timing measurement was performed with: > > > > ------------------------------------------------------------------------------- > > --- a/src/ipa/rkisp1/algorithms/lsc.cpp > > +++ b/src/ipa/rkisp1/algorithms/lsc.cpp > > @@ -140,6 +140,8 @@ int LensShadingCorrection::configure(IPAContext &context, > > const Size &size = context.configuration.sensor.size; > > Size totalSize{}; > > > > +std::chrono::high_resolution_clock::time_point begin = std::chrono::high_resolution_clock::now(); > > + > > for (unsigned int i = 0; i < RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE; ++i) { > > xSizes_[i] = xSize_[i] * size.width; > > ySizes_[i] = ySize_[i] * size.height; > > @@ -162,8 +164,13 @@ int LensShadingCorrection::configure(IPAContext &context, > > yGrad_[i] = std::round(32768 / ySizes_[i]); > > } > > > > - return lscAlgo_.configure(context.activeState.lsc, configInfo.analogCrop, > > + lscAlgo_.configure(context.activeState.lsc, configInfo.analogCrop, > > xPos_, yPos_); > > + std::chrono::high_resolution_clock::time_point end = std::chrono::high_resolution_clock::now(); > > + > > + LOG(RkISP1Lsc, Error) << "Configure took = " << std::chrono::duration_cast<std::chrono::microseconds>(end - begin).count() << "[µs]"; > > + > > + return 0; > > } > > Interesting, I'm only measuring the `lscAlgo_.configure()` call because the parts before > seem to be the same, and I'm getting around ~120us for v5 and ~140us for v6 > with imx219.yml. > > > > > > void LensShadingCorrection::setParameters(rkisp1_cif_isp_lsc_config &config) > > ------------------------------------------------------------------------------- > > > > > > > > > > > > > > > > Changes in v6: > > > > - Address comments received on v5, mostly on documentation > > > > - Remove templating of LscPolynomial and perform quantization in > > > > LscAlgorithm > > > > > > > > Changes in v5: > > > > - Add "ipa: libipa: lsc_polynomial: Fix polynomial parsing error" > > > > to fix and exiting potential issue with Polynomial parsing > > > > - Fix Simple, which I broke in v4, thanks Milan > > > > > > > > Changes in v4: > > > > - s/awb/AWB in comments everywhere, same for CCM and LSC > > > > - Awb: changelog per patch > > > > - Make AwbStats pure virtual and move stats parsing logic to the IPA > > > > modules > > > > - Reduce comments/renames to reduce patch size and comments > > > > - Collect tags > > > > > > > > Changes in v3: > > > > - Address comments on Awb from Stefan: > > > > - Introduce AwbImplementation::Result > > > > - Move AwbStats implementation to IPAs > > > > - Drop AwbStats::rg()/bg() and re-implement AwbGray accordingly > > > > - Fix Saturation bug in Simple Ccm algorithm > > > > - Fix Simple Ccm gain limits using Q<2, 8> > > > > - Update Mali CCM and Gamma to latest uAPI patch version > > > > - Fix Mali CCM gain masking as reported by Dan > > > > - Break down libIPA Lsc implementation as requested by Stefan > > > > > > > > Changes in v2: > > > > - Introduce awb::Context to simplify ActiveState and FrameContext > > > > definition > > > > - Add mali uAPI header for CCM and Gamma > > > > - Do not rename Mali functions for hardware configuration in awb and lsc > > > > - Spelling and documentation > > > > > > > > --- > > [...]
The series is identical to v5 up to patch 27. The only minor difference is that I back-tracked on making functions class members but in the RkISP1 IPA I kept them as static helpers. Patches 28 and 29 are new. They remove the templating of the LscAlgorithm class hierarchy and instead implement quantization in the LscAlgorithm class. So we now expand polynomials and parse tables as floats, store them in a map and in LscAlgorithm we re-iterate over it to either quantized them in the polynomial case, or simply cast floats to the register bit width if table. Timing the duration of the whole RkISP1 configure operation, it now takes 3 msec compared to 600 usec. RkISP1Lsc lsc.cpp:171 Configure took = 3081[µs] RkISP1Lsc lsc.cpp:172 Configure took = 583[µs] Also, now each IPA has to define their own Interpolator::interpolate() overload (see rkisp1 IPA as an example of this). As the interface of LscAlgorithm has changed, patches on the list that port the LSC algorithm of other platforms based on v5 will have to be rebased. Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> --- Changes in v6: - Address comments received on v5, mostly on documentation - Remove templating of LscPolynomial and perform quantization in LscAlgorithm Changes in v5: - Add "ipa: libipa: lsc_polynomial: Fix polynomial parsing error" to fix and exiting potential issue with Polynomial parsing - Fix Simple, which I broke in v4, thanks Milan Changes in v4: - s/awb/AWB in comments everywhere, same for CCM and LSC - Awb: changelog per patch - Make AwbStats pure virtual and move stats parsing logic to the IPA modules - Reduce comments/renames to reduce patch size and comments - Collect tags Changes in v3: - Address comments on Awb from Stefan: - Introduce AwbImplementation::Result - Move AwbStats implementation to IPAs - Drop AwbStats::rg()/bg() and re-implement AwbGray accordingly - Fix Saturation bug in Simple Ccm algorithm - Fix Simple Ccm gain limits using Q<2, 8> - Update Mali CCM and Gamma to latest uAPI patch version - Fix Mali CCM gain masking as reported by Dan - Break down libIPA Lsc implementation as requested by Stefan Changes in v2: - Introduce awb::Context to simplify ActiveState and FrameContext definition - Add mali uAPI header for CCM and Gamma - Do not rename Mali functions for hardware configuration in awb and lsc - Spelling and documentation --- Jacopo Mondi (28): ipa: libipa: awb: Reimplement AwbAlgorithm ipa: mali-c55: awb: Port to use libipa AwbAlgorithm ipa: libipa: Add CcmAlgorithm to libipa ipa: rkisp1: ccm: Port to use CcmAlgorithm include: linux: mali-c55: Update to support CCM and Gamma ipa: mali-c55: Implement Ccm algorithm ipa: rkisp1: lsc: Re-architecture Lsc support ipa: libipa: Introduce lsc_base.h ipa: libipa: lsc_polynomial: Fix polynomial parsing error ipa: libipa: lsc_polynomial: Rename LscPolynomial ipa: libipa: lsc_polynomial: Do not inline functions ipa: libipa: lsc: Move Interpolator<lsc::Components> to lsc_base ipa: rkisp1: lsc: Remove rogue empty line ipa: libipa: lsc_polynomial: Move LscPolynomial from RkISP1 ipa: libipa: lsc_table: Move LscTable from RkISP1 ipa: libipa: lsc_base: Add sampleForCrop documentation ipa: libipa: lsc: Introduce LscAlgorithm ipa: libipa: lsc: Move sizesListToPositions() back to rkisp1 ipa: rkisp1: lsc: Port to use LscAlgorithm ipa: libipa: lsc: Make Components a map<> ipa: libipa: lsc: Document Components parsing ipa: libipa: lsc_polynomial: Remove sensorSize_ ipa: libipa: lsc: Document LscDescriptor::sensorSize usage ipa: libipa: lsc: Quantize lsc gains in IPA ipa: libipa: lsc: Quantize gains in LscAlgorithm ipa: libipa: lsc: Re-sort LscAlgorithmBase documentation ipa: mali-c55: Add sensorInfo to IPAContext ipa: mali-c55: Port to use LscAlgorithm Kieran Bingham (2): ipa: simple: awb: Port to use libipa AwbAlgorithm ipa: simple: Port to use libipa CcmAlgorithm Laurent Pinchart (1): ipa: libipa: lsc_polynomial: Don't inline ValueNode accessor .../internal/software_isp/debayer_params.h | 4 +- include/linux/media/arm/mali-c55-config.h | 86 ++++- src/ipa/libipa/awb.cpp | 418 +++++++++++++++++--- src/ipa/libipa/awb.h | 100 ++++- src/ipa/libipa/awb_bayes.cpp | 52 +-- src/ipa/libipa/awb_bayes.h | 12 +- src/ipa/libipa/awb_grey.cpp | 16 +- src/ipa/libipa/awb_grey.h | 6 +- src/ipa/libipa/ccm.cpp | 245 ++++++++++++ src/ipa/libipa/ccm.h | 77 ++++ src/ipa/libipa/lsc.cpp | 372 ++++++++++++++++++ src/ipa/libipa/lsc.h | 137 +++++++ src/ipa/libipa/lsc_base.cpp | 136 +++++++ src/ipa/libipa/lsc_base.h | 56 +++ src/ipa/libipa/lsc_polynomial.cpp | 208 +++++++++- src/ipa/libipa/lsc_polynomial.h | 100 ++--- src/ipa/libipa/lsc_table.cpp | 107 +++++ src/ipa/libipa/lsc_table.h | 55 +++ src/ipa/libipa/meson.build | 8 + src/ipa/mali-c55/algorithms/awb.cpp | 223 ++++++----- src/ipa/mali-c55/algorithms/awb.h | 28 +- src/ipa/mali-c55/algorithms/ccm.cpp | 173 +++++++++ src/ipa/mali-c55/algorithms/ccm.h | 66 ++++ src/ipa/mali-c55/algorithms/lsc.cpp | 156 +++++--- src/ipa/mali-c55/algorithms/lsc.h | 32 +- src/ipa/mali-c55/algorithms/meson.build | 1 + src/ipa/mali-c55/ipa_context.cpp | 3 + src/ipa/mali-c55/ipa_context.h | 20 +- src/ipa/mali-c55/mali-c55.cpp | 2 + src/ipa/mali-c55/params.h | 2 + src/ipa/rkisp1/algorithms/awb.cpp | 191 ++------- src/ipa/rkisp1/algorithms/awb.h | 18 +- src/ipa/rkisp1/algorithms/ccm.cpp | 97 +---- src/ipa/rkisp1/algorithms/ccm.h | 17 +- src/ipa/rkisp1/algorithms/lsc.cpp | 429 +++------------------ src/ipa/rkisp1/algorithms/lsc.h | 74 ++-- src/ipa/rkisp1/ipa_context.h | 49 +-- src/ipa/simple/algorithms/awb.cpp | 140 +++++-- src/ipa/simple/algorithms/awb.h | 28 ++ src/ipa/simple/algorithms/blc.cpp | 2 +- src/ipa/simple/algorithms/ccm.cpp | 74 ++-- src/ipa/simple/algorithms/ccm.h | 22 +- src/ipa/simple/ipa_context.h | 15 +- src/libcamera/software_isp/debayer_cpu.cpp | 12 +- 44 files changed, 2934 insertions(+), 1135 deletions(-) --- base-commit: 709ad59a8d90af3570be1c8d02dee0ec9e011954 change-id: 20260615-libipa-algorithms-59666bca0a2f Best regards,