-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
merge rockchip64 and rockchip-rk3588 #7566
Conversation
Nicely done! I will make a round of tests on my boards ASAP, although I would have split the rk3588 family merging and the current kernel bump to 6.12 in two different times just to give a bit more time for people to test 6.12 before becoming the current kernel. Anyway I speak from outside and did not partecipate directly in the work, so I guess you already made the right considerations and will give some feedback soon. |
Uhm... perhaps in another pull request some kernel config has to be revised to remove some unnecessary modules or things. The rockchip64 kernel was already huge, but now the image package is almost 30% bigger (215M vs 275M) edit: I can confirm the increase in size is mostly due to the modules. The kernel increased from 31M to 33M, but the modules went from 169M to 226M, and they are already compressed with xz |
I also find some unnecessary kernel configs when merging the two family, such as qcom, allwinner socs. I will try to disable them and see if the kernel size is reduced. |
rockchip-rk3588 is a huge kernel:
It should be based on |
After removing ACPI related configs, kernel image is 31M. But module size is still huge. Rockchip-rk3588 is a huge kernel. $ du -sh /lib/modules/6.12.4-edge-rockchip64/ |
This is a list of the modules that are present in the merged 6.12.4-current kernel which were not present in the previous 6.12.2-edge kernel: added-modules.log Some of them definitely should not be there (acpi, parallel ata, hisilicon drivers, gpu tests, ...), some others looks like have been moved outside the kernel into modules (rk808 and friends). I can give a round of cleaning up if you agree. In the meantime, I tested the big fat kernel on a rockpi-s (rk3308) and it boots ok |
I have removed acpi and hisi configs in the latest commit. |
With this kernel config I get back a 218M kernel image package: rockchip64-kernel-iteration1.config.txt I look for something else to remove... |
I have followed a rule when merging configs of two families: if some config is built as built-in and in another family it is built as module, change it to module. And I think it's better to prefer module if there is no issue caused.
Take a quick look at the deletions:
|
I have added necessary drivers for rk3588 and change some drivers to module based on rockchip64-kernel-iteration1.config.txt: rockchip64-kernel-iteration1.config_amazingfate.txt
Tested with rock5b(rk3588) and orangepi3b(rk3566). |
0771709
to
e239677
Compare
@amazingfate thanks for understanding |
That was my point, they aren't, really. According to https://github.com/armbian/build/blob/main/lib/functions/compilation/armbian-kernel.sh#L20-L35 we shouldn't have compressed modules in kernel builds, for all Armbian kernels, as those config changes are ran unconditionally and override what's in the .config files. I checked the contents of an initrd on a live board (It's a gzipped cpio archive). I had a 6.13 rockchip-rk3588 experiment from before this PR, but it should be similar elsewhere. It also had plymouth enabled: # 36M initrd.img-6.13.0-rc1-edge-rockchip-rk3588
# 109M initrd.img-6.13.0-rc1-edge-rockchip-rk3588.ungzipped
# Extracting the ungzipped cpio yields:
# 70M extracted/usr/lib/modules/6.13.0-rc1-edge-rockchip-rk3588 The modules dir here has 68Mb of uncompressed By removing plymouth ( # 27M /boot/initrd.img-6.13.0-rc1-edge-rockchip-rk3588
# 84M /boot/initrd.img-6.13.0-rc1-edge-rockchip-rk3588.ungzipped
# Inside cpio:
# 60M ./usr/lib/modules/6.13.0-rc1-edge-rockchip-rk3588 If I then compress (with xz) every .ko of those 60Mb, I get a 17Mb result, which is not bad. The compression is very slow (and together with the double-compression of the initrd was probably the reason I disabled ko.xz's in the first place -- to speed up kernel builds) and decompression is not free either, but it might make some 256mb boards more comfortable. |
Nah. It was this: https://salsa.debian.org/kernel-team/initramfs-tools/-/commit/d8c586458115beff0e46b4bf95997a871dde3294 -- upstream Debian mkinitramfs will forcibly and non-configurably decompress all .ko.xz's, thus making the whole compression effort useless in the end. To make it more confusing, meanwhile this has landed (I guess in |
Ah, sorry, I was aware of the opposite and totally missed the change 😔 my confusion came from the large deb packages produced by my local builds, I guess there's a flag to enable deb compression which I'm not using at last. Anyway yes, I see the thing is a bit convoluted and yet in motion; but removing plymouth would harm the armbian boot screen, wouldn't it? |
Any news on this or idea on how to proceed? IMHO the size issues are now solved if the package size is as it was before or so and the PR can be marked as "Ready to merge". Possibile optimizations (plymouth, kernel config slim down and compression, initrd optimizations, etc...) probably are better to handle and discuss in a separate PR, this looks already quite "heavy" |
@rpardini @amazingfate needs to decide that. I didn't pay enough attention on this. |
I will vote merge for the following reasons:
|
I will test some rk3588 boards today but since some boards were tested already, i think the PR is ready to be merged |
Two points:
|
No, but we can manipulate CMA via kernel command line, right? |
CMA is overridden via the kernel command line. That's straightforward. Can any suggest a way to remove the substantial useless code for supporting video without creating an alternative "IOT" variant of the kernel? Board config specifies a list of kernel modules to omit. @rpardini I expected that would omit those modules from initrd as well, but I never verified this. |
I expect that this breaks audio on the RockPI-S. |
Rock PI-S audio is unusable after application of this PR because the mainline analog mixer code that we adopted recently does not support the variant of the RK3308 SOC used by the Rock PI-S. If one turns up all the gains, one can hear clicks, but audio is still unintelligible. For some reason, it is decidedly worse than with the 12.1 kernel, which sometimes managed to produce intelligible audio. Options:
The previous vendor mixer works reliably on Armbian kernels from 5.10 to 6.9 Here's the response I got from Luca Ceresoli luca.ceresoli@bootlin.com when I emailed asking why they don't support the codec in the RockPI-S:
The authors released it knowing that it does not handle a very common chip variant (used on a $30 boards!) because they "lacked hardware" to test. In fairness, he did go on to say:
But, given the lack of documentation, those patches would necessarily involve merging code derived from the vendor driver to support the "version B" SOC into the mainline version. That's a fine goal, but I don't see why we shouldn't have working audio in the meantime. |
This is impossible because 6.6 doesn't support rk3588.
Audio codec driver for rk3308 is changed to mainline since 6.10: 1f7289d. Changing it back to vendor driver or leaving some boards broken are both fine to me. I would prefer a patch based on mainline driver. But it's up to you guys with the device. |
The board config for RockPI-S could redefine the "current" kernel to 6.6. |
@brentr hello! It is not clear to me how the rk3588 patches break the rk3308b (the "b" is important) analog codec in mainline, since rk3308b analog codec was already broken in 6.12. But I agree that this PR moves the breakage from I was already investigating the analog codec differences among In the meantime this PR could be merged and a new PR with a patch could be added to revert the rk3308 analog code to the vendor code, I can surely do that in no time if you agree. |
I just learned that the only board tested by the mainline RK3308 mixer developers was a customer product, unavailable for purchase. Sigh... I'll approve this PR if we can agree to reinstate the vendor analog mixer. This gets us improved RK3588 support without breaking other boards. The mainline RK3308 acodec has never worked. It slipped into 6.10 without testing. End users soon noticed the failure and reported it. https://forum.armbian.com/topic/45295-audio-no-longer-works-after-updating-to-armbian-2482/ In response, @paolosabatino and I fixed the vendor acodec the in "current" 6.6 kernel, but the mainline acodec remained in edge. Now, predictably, we want "edge" to became "current". |
@paolosabatino Agreed? I'd really like to prevent having users seeing audio working and breaking on alternating releases (as has been the case for the past 3!) Can we avoid that if we apply this PR now? Or would it be better to wait until the new year? Maybe it would be best to simply patch in the old, working acodec now so you can work on the new acodec with less pressure? |
@brentr no need to worry about users seeing things break, as said regular end-users won't notice that until next armbian release is out. Beta users are beta users, so they should expect breakage from time to time. Anyway, I already prepared a patch to revert to vendor code for that codec; I just need to fix the device tree to address the changes. Will publish the full working patch in a separate PR I hope later today, there is no need to keep @amazingfate on hold for this. In the meantime, during holidays, if I will work on the mainline codec driver I'll let you know, so we can do some shared tests. |
Sounds like a plan. @amazingfate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only issue I see from here is the fact that moving edge to 6.12 will force RockPI-S to use the (currently broken) mainline acodec. But, @paolosabatino and I have plan to either fix that codec or revert it to back to the vendor codec.
@paolosabatino Note that 6.12.6 is in fact no better or worse than 6.12.1 with regards to the RK3308 acodec. It still sort of, sometimes works -- the total failure I observed last evening was the result of my forgetting to enable the headphone amp on the rockpi-s's POE hat. Summary of mainline mixer control breakage:
|
Excellent work here. Kudos! For those who have old images (using the
|
Dunno if this is known:
|
Description
This pr has done:
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Please also note any relevant details for your test configuration.
Checklist:
Please delete options that are not relevant.