[libcamera-devel] [PATCH 00/11] libcamera: introduce Software ISP and Software IPA

[Bryan O'Donoghue]

On 14/12/2023 16:25, Hans de Goede wrote:
> Hi,
> 
> On 12/14/23 17:02, Hans de Goede wrote:
>> Hi Bryan,
>>
>> On 12/14/23 16:16, Bryan O'Donoghue wrote:
>>> On 14/12/2023 14:47, Hans de Goede wrote:
>>
>> <snip>
>>
>>>> BTW I'm currently working on adding support for 10bpp unpacked
>>>> bayer and while working on this I noticed a small bug in the swstats
>>>> code, you may want to squash in this fix:
>>>
>>> Applied that change.
>>>
>>> I'm comparing to your earlier branch
>>>
>>> commit ae92fa44991ded151c63d5d202efdacc9d640aff (HEAD -> SoftwareISP-v01-hans1, softisp/SoftwareISP-v01-hans1)
>>> Author: Hans de Goede <hdegoede at redhat.com>
>>> Date:   Thu Nov 30 20:13:29 2023 +0100
>>>
>>>
>>> On the earlier branch I get 30fps @ 68% CPU usage. On this branch I'm getting 18fps @ 100%.
>>> Seems to be eating alot more cycles
>>
>> Ok, that is no good.
>>
>> Can you try:
>>
>> https://gitlab.freedesktop.org/camera/libcamera-softisp/-/commits/SoftwareISP-v02-hans3

So this branch has the same performance issue 18fps, pinked-out display 
and ~ 100% on one of the cores.

>>
>> And then start with checking out:
>>
>> 64c9bc0f55f459b9722fe78dfd35a07fb35d2a7b ("libcamera: software_isp: Add debayer-line callbacks for bayer patterns repeating every 4 lines")

I get 30fps here with 60-9x% cpu usage. Looks a bit dark but not pink.

Looks like a reasonable baseline.

>>
>> that is basically softisp/SoftwareISP-v01-hans1 rebased on top of SoftwareISP-v02,
>> so that should give you to same performance as before.
>>
>> First please check this indeed restores performance ?
>>
>> And then after that try newer commits from that branch in this order
>> (I'm skipping commits which should not have a performance impact here):
>>
>> 7136d4d59aafb2564a24a2d4be773ca220257fdc

brighter but pink, lower cpu occupancy 40%, 30fps

>> e3c2a5931dd825c58f626da8c12429b70d20219b (not really expecting a performance impact from this one but maybe)


Way brighter - too much IMO in comparsion to the previous, occupancy 
100% fps 21.

This looks like your wayward commit to me.

>> b3aa4e4f781e881746953177dacbce8c943cb5a3 (bugfix but one which is expected to have some performance impact)

Same result as previous 21 fps, 100% cpu.


>> ec75339a5bff8b8e9a0031b9408bd47020905a4f

28fps, 130% cpu

>> f62ed5df54850a24406bf3a762271a5fa3c0303d

28fps, 100% cpu

>>
>> The reason why I'm asking you to test these instead of do a bisect
>> is since there might be a number of smaller performance regressions
>> all adding up ...
>>
>> If you can let me know the results of this then I'll see if I can
>> restore the old performance for you.
>>
>> Note that as mentioned above b3aa4e4f781e is a bug fix which
>> some potentially big performance implications but the old code
>> really was wrong there ...
>>
>>> and producing a more pinkish result on my hw.
>>
>> As part of my work I more or less rewrote white-balancing, but I thought I fixed
>> the pinkish thing. It seems the new white-balancing is somewhat sensitive to
>> over-exposure and your image does look a bit overexposed.
> 
> p.s.
> 
> It would also be good to compare:
> 
> https://gitlab.freedesktop.org/camera/libcamera-softisp/-/commits/SoftwareISP-v02-hans2

27fps, 100% cpu occupancy, bright and pink

> https://gitlab.freedesktop.org/camera/libcamera-softisp/-/commits/SoftwareISP-v02-hans3

18fps, 100% cpu occupancy maybe less pink than previous but still 
noticeably too red

> 
> (just the last commit of each)
> 
> hans2 -> hans3 is mostly prep work for merging things with SoftwareISP-v03,
> but one significant change in hans3 is that I added Pavel's code changes
> to avoid macro-use (in swstats) resp reduce code duplication (in debayer).


> 
> Pavel's changes rely on the compiler being smart enough to inline *all*
> the helpers and then replace the ctxt struct with using registers for each
> of the struct members. Maybe that is not working for you and we need
> to undo Pavel's changes.

Nothing special there. Stock Debian aarch64 GCC.

Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/libexec/gcc/aarch64-linux-gnu/13/lto-wrapper
Target: aarch64-linux-gnu
Configured with: ../src/configure -v --with-pkgversion='Debian 13.2.0-6' 
--with-bugurl=file:///usr/share/doc/gcc-13/README.Bugs 
--enable-languages=c,ada,c++,go,d,fortran,objc,obj-c++,m2 --prefix=/usr 
--with-gcc-major-version-only --program-suffix=-13 
--program-prefix=aarch64-linux-gnu- --enable-shared 
--enable-linker-build-id --libexecdir=/usr/libexec 
--without-included-gettext --enable-threads=posix --libdir=/usr/lib 
--enable-nls --enable-bootstrap --enable-clocale=gnu 
--enable-libstdcxx-debug --enable-libstdcxx-time=yes 
--with-default-libstdcxx-abi=new --enable-gnu-unique-object 
--disable-libquadmath --disable-libquadmath-support --enable-plugin 
--enable-default-pie --with-system-zlib 
--enable-libphobos-checking=release --with-target-system-zlib=auto 
--enable-objc-gc=auto --enable-multiarch --enable-fix-cortex-a53-843419 
--disable-werror --enable-checking=release --build=aarch64-linux-gnu 
--host=aarch64-linux-gnu --target=aarch64-linux-gnu 
--with-build-config=bootstrap-lto-lean --enable-link-serialization=4
Thread model: posix
Supported LTO compression algorithms: zlib zstd
gcc version 13.2.0 (Debian 13.2.0-6)

---
bod