Skip to content
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

Merged
merged 9 commits into from
Dec 21, 2024

Conversation

amazingfate
Copy link
Contributor

Description

This pr has done:

  • Merge patches from rockchip-rk3588 6.12 to rockchip64 6.12.
  • Merge kernel config from rockchip-rk3588 edge to rockchip64 edge. I first rewrite the kernel config of the two families, then merge the defconfig generated, then rewrite the kernel config based on the merged defconfig
  • disable h264 decoder of vpu121 hantro g1 decoder because gsteamer can't deal with two h264 decoder(hantro g1 and rkvdec2)
  • bump rockchip64-current to 6.12
  • remove current and edge branch from rockchip-rk3588, now the kernel branch is defined in config/sources/families/include/rockchip64_common.inc.
  • rockchip64 has a default CMA size 128M, which is not enough for rkvdec2 driver. I renamed bootenv file rk356x.txt to rk35xx.txt and set it for rk356x and rk3588, so they can use 256M CMA size for rkvdec2 driver.

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.

  • ./compile.sh kernel BOARD=rock-3a BRANCH=edge BUILD_MINIMAL=no DEB_COMPRESS=xz KERNEL_CONFIGURE=no KERNEL_GIT=shallow
  • ./compile.sh kernel BOARD=rock-5b BRANCH=edge BUILD_MINIMAL=no DEB_COMPRESS=xz KERNEL_CONFIGURE=no KERNEL_GIT=shallow
  • The above two build commands will generate same deb packages. Tested booting on rock5b.

Checklist:

Please delete options that are not relevant.

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

@github-actions github-actions bot added size/large PR with 250 lines or more Needs review Seeking for review Hardware Hardware related like kernel, U-Boot, ... Patches Patches related to kernel, U-Boot, ... labels Dec 11, 2024
@paolosabatino
Copy link
Contributor

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.

@paolosabatino
Copy link
Contributor

paolosabatino commented Dec 11, 2024

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

@amazingfate
Copy link
Contributor Author

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.

@amazingfate
Copy link
Contributor Author

rockchip-rk3588 is a huge kernel:

$ du -sh /lib/modules/6.12.4-edge-rockchip*
224M    /lib/modules/6.12.4-edge-rockchip64
212M    /lib/modules/6.12.4-edge-rockchip-rk3588

It should be based on arch/arm64/configs/defconfig when introduced.

@amazingfate
Copy link
Contributor Author

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/
222M /lib/modules/6.12.4-edge-rockchip64/
$ ls -lh /boot/vmlinuz-6.12.4-edge-rockchip64
-rw-r--r-- 1 root root 31M 12月 12 15:58 /boot/vmlinuz-6.12.4-edge-rockchip64

@paolosabatino
Copy link
Contributor

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

@amazingfate
Copy link
Contributor Author

I have removed acpi and hisi configs in the latest commit.

@paolosabatino
Copy link
Contributor

paolosabatino commented Dec 12, 2024

With this kernel config I get back a 218M kernel image package: rockchip64-kernel-iteration1.config.txt

I look for something else to remove...

@amazingfate
Copy link
Contributor Author

some others looks like have been moved outside the kernel into modules (rk808 and friends)

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.

With this kernel config I get back a 218M kernel image package: rockchip64-kernel-iteration1.config.txt

Take a quick look at the deletions:

  • joystick drivers are deleted. rk3588 has the power to do nintendo switch emulation, so the drivers are useful
  • khadas mcu is disabled, which was introduced by 5e4393d
  • hdmirx driver of rk3588 is deleted: b9d7bd9
  • DRM_LOAD_EDID_FIRMWARE is disabled, I think this is useful if user want to set a specific resolution of monitor by loading a firmware file

@amazingfate
Copy link
Contributor Author

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

$ du -sh /boot/vmlinuz-6.12.4-edge-rockchip64  /lib/modules/6.12.4-edge-rockchip64/
29M	/boot/vmlinuz-6.12.4-edge-rockchip64
174M	/lib/modules/6.12.4-edge-rockchip64/

Tested with rock5b(rk3588) and orangepi3b(rk3566).

@amazingfate amazingfate force-pushed the rockchip64-rk3588 branch 2 times, most recently from 0771709 to e239677 Compare December 13, 2024 06:19
@paolosabatino
Copy link
Contributor

@amazingfate thanks for understanding

@rpardini
Copy link
Member

If I am not mistaken, currently we force the kernel/Kbuild itself to not compress the modules, and let update-initramfs compress the whole of the initrd

Currently the modules for rockchip64 are already kept in rootfs as .xz

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 .ko's.


By removing plymouth (apt -y purge plymouth) things look a bit better:

# 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.

@rpardini
Copy link
Member

... was probably the reason I disabled ko.xz's in the first place

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 sid?) https://salsa.debian.org/kernel-team/initramfs-tools/-/commit/81fd41f72dd894b2cec259c932d09ee3eba0fc03 -- that would leave .ko.xz's uncompressed if the cpio itself is not compressed (which isn't our case, but we could abuse it).

@paolosabatino
Copy link
Contributor

paolosabatino commented Dec 15, 2024

That was my point, they aren't, really.

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?

@paolosabatino
Copy link
Contributor

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"

@igorpecovnik
Copy link
Member

igorpecovnik commented Dec 18, 2024

and the PR can be marked as "Ready to merge"

@rpardini @amazingfate needs to decide that. I didn't pay enough attention on this.

@amazingfate
Copy link
Contributor Author

I will vote merge for the following reasons:

  • kernel 6.12 is used by both rockchip-rk3588 and rockchip64 for a while, so bumping 6.12 to current should be fine
  • Changes to kernel configs added from rockchip-rk3588 are not huge and easy to track
  • rk3308, rk3328, rk3399, rk356x and rk3588 are all confirmed working
  • There is already a pr bumping edge of rk3588 to 6.13: rockchip-rk3588-edge: update to v6.13-rc1 #7560. It can continue after this merge.

@efectn
Copy link
Member

efectn commented Dec 19, 2024

I will test some rk3588 boards today but since some boards were tested already, i think the PR is ready to be merged

@paolosabatino paolosabatino added the Ready to merge Reviewed, tested and ready for merge label Dec 19, 2024
@brentr
Copy link
Collaborator

brentr commented Dec 19, 2024

Two points:

  1. The latest Rock PI-S and S0 builds override the default CMA size to set it at 16MB
    (the new default of 256MB would be 50% of the available DRAM)

  2. Would it not make sense to maintain two flavors of the rockchip64 kernel?
    One for boards with video output, the other for boards that lack any video out.
    After all, plymouth is useless in an IOT device, as is panfrost and its ilk.

@igorpecovnik
Copy link
Member

Would it not make sense to maintain two flavors of the rockchip64 kernel?

No, but we can manipulate CMA via kernel command line, right?

@brentr
Copy link
Collaborator

brentr commented Dec 19, 2024

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.
Could one also omit plymouth via that, or a similar, mechanism?

@brentr
Copy link
Collaborator

brentr commented Dec 20, 2024

I expect that this breaks audio on the RockPI-S.
I will verify this evening.
See my posts in:
#7519

@brentr
Copy link
Collaborator

brentr commented Dec 20, 2024

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:

  1. Restore the vendor's analog mixer driver by reinstating the patches that were recently disabled.

  2. Leave the "current" Rockchip64 kernel at 6.6 until we have an Armbian patch against the mainline mixer for Rock PI-S.

  3. Break Rock PI-S audio until we have an Armbian patch against the mainline mixer that fixes it. That could take a looong time.

The previous vendor mixer works reliably on Armbian kernels from 5.10 to 6.9
(even if the labels on its mixer controls are laughably cryptic :-)
I could merge it back into the current kernel this weekend, on top of this PR.
When we get a mainline kernel mixer that handled the RockPI-S, we'd disable the vendor's (again :-)

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:

Why does the rk3308_codec in kernel 6.12 not support ACODEC_VERSION_B?

Due to no documentation and lack of hardware for me to test it.
Moreover I removed several features from the driver I sent mainline, as
those were not present on the hardware I have and keeping them would
have been a large effort and also open to possible bugs as I could not
test them.

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.
Their reasoning was, understandably, that their driver was better than nothing.

In fairness, he did go on to say:

... it should be easy to add support for version B. I'm glad to help if you
have patches to send or any questions.

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.

@amazingfate
Copy link
Contributor Author

Leave the "current" Rockchip64 kernel at 6.6 until we have an Armbian patch against the mainline mixer for Rock PI-S.

This is impossible because 6.6 doesn't support rk3588.

The previous vendor mixer works reliably on Armbian kernels from 5.10 to 6.9
(even if the labels on its mixer controls are laughably cryptic :-)
I could merge it back into the current kernel this weekend, on top of this PR.

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.

@brentr
Copy link
Collaborator

brentr commented Dec 20, 2024

The board config for RockPI-S could redefine the "current" kernel to 6.6.
Not recommending this, but it would be possible.

@paolosabatino
Copy link
Contributor

paolosabatino commented Dec 20, 2024

@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 edge to current. End users won't be affected in the short term though, since the change should only go to beta.armbian.com APT users, not the regular ones, if you're concerned about that.

I was already investigating the analog codec differences among A, B and C versions. A and C are very similar, B requires some special handling mostly due to some line voltage sequence when playback starts and stops, plus other things. Can't promise anything, but perhaps in the holidays I will give some love to it (or not, the amount of love I have is limited).

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.

@brentr
Copy link
Collaborator

brentr commented Dec 20, 2024

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".

@brentr
Copy link
Collaborator

brentr commented Dec 20, 2024

@paolosabatino
Sorry, I didn't see your last post while I was writing mine.
We're on the same page here.
I'm willing to work with you to test this over the holidays.
If we don't get a something working based on the mainline, we will revert to the vendor acodec driver.

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?
I think you could do that PR faster than I, but I've got the hardware here and enough "love" to spare for that task.

@paolosabatino
Copy link
Contributor

@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.

@brentr
Copy link
Collaborator

brentr commented Dec 20, 2024

Sounds like a plan.

@amazingfate
We're good to go.
Please merge this PR as you see fit.

Copy link
Collaborator

@brentr brentr left a 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.

@brentr
Copy link
Collaborator

brentr commented Dec 20, 2024

@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:

  1. The "Headphone" volume control has no effect
  2. DAC HPMIX toggle often causes audio to clip badly
    With DAC HPMIX at "0" headphone output is too low.
  3. I've asked the author why the MICxy HPF Cutoff controls appear on the Playback side of the mixer and to explain the names of the controls.

@amazingfate amazingfate merged commit bff12a6 into armbian:main Dec 21, 2024
@rpardini
Copy link
Member

Excellent work here. Kudos!

For those who have old images (using the rockchip-rk3588/edge kernel), there's the apt incantation to change kernel family -- this will in one step remove the old and install the new:

apt install linux-image-edge-rockchip64 linux-dtb-edge-rockchip64 linux-image-edge-rockchip-rk3588- linux-dtb-edge-rockchip-rk3588-

@igorpecovnik
Copy link
Member

Dunno if this is known:

  [🐳|🔨]   Building initial module for 6.12.8-current-rockchip64
  [🐳|🔨]   Error! Bad return status for module build on kernel: 6.12.8-current-rockchip64 (aarch64)
  [🐳|🔨]   Consult /var/lib/dkms/v4l2loopback/0.12.7/build/make.log for more information.
  [🐳|🔨]   dpkg: error processing package v4l2loopback-dkms (--configure):
  [🐳|🔨]    installed v4l2loopback-dkms package post-installation script subprocess returned error exit status 10
  [🐳|🔨]   Setting up gstreamer1.0-tools (1.22.0-2+deb12u1) ...
  [🐳|🔨]   Setting up libv4l2rds0:arm64 (1.22.1-5+b2) ...
  [🐳|🔨]   Setting up v4l-utils (1.22.1-5+b2) ...
  [🐳|🔨]   Setting up v4l2loopback-utils (0.12.7-2) ...
  [🐳|🔨]   Processing triggers for man-db (2.11.2-2) ...
  [🐳|🔨]   Processing triggers for libc-bin (2.36-9+deb12u9) ...
  [🐳|🔨]   Errors were encountered while processing:
  [🐳|🔨]    v4l2loopback-dkms

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Hardware Hardware related like kernel, U-Boot, ... Needs review Seeking for review Patches Patches related to kernel, U-Boot, ... Ready to merge Reviewed, tested and ready for merge size/large PR with 250 lines or more
Development

Successfully merging this pull request may close these issues.

6 participants