[kirin] [aosp 10] [kernel 4.14] Notification LED does not work

[xperia10dev]

Platform: ganges
Device: kirin (i4113)
Kernel version: 4.14.176
Android version: android-10.0.0_r33
Software binaries version: 10.0.7.1_r1_v6_ganges

Previously working on
Android 9

Description
The notification LED does not work

Symptoms
The notification LED does not blink when a notification is received

How to reproduce
Receive a notification from an application

Additional context
The "Blink light" option is enabled in Settings

[oshmoun]

@Haxk20 can you confirm this?

[Haxk20]

Depends on application but yes notification led does not work as it should. @MarijnS95 created few patches that implement functionality we are still missing. Mainly blink. That fixes most of the issues

[stefanhh0]

Btw. same for yoshino/lilac... is it something that can fixed generally for all platforms or is it a device specific problem? In the latter case I would open a separate bug for yoshino...

[Haxk20]

Generic issue

[xperia10dev]

I noticed that for kernel 4.9 the following code is present in https://github.com/sonyxperiadev/kernel/blob/aosp/LE.UM.2.3.2.r1.4/arch/arm64/boot/dts/qcom/sdm630-ganges-common.dtsi:

/* PM660L */
&red_led {
	linux,name = "led:rgb_red";
	qcom,start-idx = <0>;
	qcom,lut-flags = <31>;
	qcom,pause-lo = <500>;
	qcom,pause-hi = <500>;
	qcom,ramp-step-ms = <50>;
	qcom,duty-pcts =
		[00 0E 1C 2A 38 46 54 64];
	qcom,use-blink;
};

&green_led {
	linux,name = "led:rgb_green";
	qcom,start-idx = <0>;
	qcom,lut-flags = <31>;
	qcom,pause-lo = <500>;
	qcom,pause-hi = <500>;
	qcom,ramp-step-ms = <50>;
	qcom,duty-pcts =
		[00 0E 1C 2A 38 46 54 64];
	qcom,use-blink;
};

&blue_led {
	linux,name = "led:rgb_blue";
	qcom,start-idx = <0>;
	qcom,lut-flags = <31>;
	qcom,pause-lo = <500>;
	qcom,pause-hi = <500>;
	qcom,ramp-step-ms = <50>;
	qcom,duty-pcts =
		[00 0E 1C 2A 38 46 54 64];
	qcom,use-blink;
};

This does not seem to be the case for kernel 4.14.

But again, if it is indeed a generic issue I guess the fix is elsewhere? I'm available to test any patches. This is now the last remaining issue on an otherwise pretty solid device with Android 10 / kernel 4.14. Thanks!

[Haxk20]

The driver for the LEDs has changed in 4.14 so that DTS part isn't needed. Also the notification led works. It's just that the driver is missing few functions. But as this isn't a critical issue we are concentrating on fixing other issues on other platforms at the moment.

[MarijnS95]

@Haxk20 The "implemented functionality" is simply disabling blink by writing 0 instead of 1:

MarijnS95/device-sony-common@21658721

There are too many issues with the driver currently, and this is not the right way to solve it. For colors are desynced ~90% of the time resulting in a nice rainbow effect rather than the desired color. Not that it matters though, only WhatsApp is color-aware and shows a green light, all other apps (that I use) simply send white 😒.

I still have some local patches in the works that update this "mainline" led driver to what's used on Stock. The diff is relatively small and it would be nice to get it in to take full advantage of our LEDs. Iirc it includes better fading options/patterns, too.

[Haxk20]

That blink at least fixes low battery led color if nothing else. But yeah thanks for taking time to make it closer to the right shape.

[xperia10dev]

Commit MarijnS95/device-sony-common@2165872 works great for me on kirin: there's now a white blinking notification light for all the apps I tested so far. The yellow and green charging lights also work as expected (as they already did before).
After some further testing, the behaviour with the above patch applied seems to be more inconsistent than I thought at first: sometimes the notification light works perfectly and keeps blinking indefinitely until dismissed, other times it will disappear after only a few seconds, and one time it rapidly showed yellow then blue at each blink (I would assume it was trying to show green but the colors are somehow desynchronized?).

Also tested sonyxperiadev/device-sony-common@84249bb separately: in this case some apps turn on a "solid" notification light without doing any blinking, while other apps turn the light on for a second or so and then it turns off.

This might all be some misbehaviour coming from the particular apps I'm using, but I hope the information can be useful anyway.

Thanks!

[MarijnS95]

other times it will disappear after only a few seconds, and one time it rapidly showed yellow then blue at each blink (I would assume it was trying to show green but the colors are somehow desynchronized?).

Yup, that is exactly the behaviour described above. My PR links another patch, would you mind to try that? I doubt it solves anything, but at this point we never know.

EDIT: Note that I have just forcepushed that patch because it was broken: it writes brightness after setting delay, which disables that again.

Also tested sonyxperiadev/device-sony-common@84249bb separately: in this case some apps turn on a "solid" notification light without doing any blinking, while other apps turn the light on for a second or so and then it turns off.

That's... completely unexpected. Can you monitor this for a while? All that was changed in that patch was remove a redundant 0 > blink write, which I doubt has any effect. If it doesn't change, please uncomment writeInt(base_path + "breath", 1); and change the 1 to a 0 (effectively making the patch identical), then try again.

[xperia10dev]

Surprisingly, the 0 > breath write does not seem to be redundant, this can be easily tested via adb shell:
# cd /sys/class/led/green
# echo 255 > brightness; echo 1000 > delay_on; echo 1000 > delay_off
gives a solid green light, while
# echo 255 > brightness; echo 1000 > delay_on; echo 1000 > delay_off; echo 0 > breath
gives a blinking (but maybe sometimes unstable?) green light.

Also, writing something different from 0 to delay_on and delay_off seems to automatically set brightness to 255.

So in fact the easiest thing to do in the Light::blinkLed method would be just something equivalent to:
# echo 1000 > delay_on; echo 1000 > delay_off
which in my device gives a correct blinking light every time (admittedly losing some nuances in the color mixing, because the brightness will be set to 255 instead of the original values in LightState.color).

This is all of course just the result of trial and error and I have no idea how the underlying driver works, so take it with a grain of salt.

[MarijnS95]

@xperia10dev Looks like it's heavily platform-dependent then. Did you explicitly set breath to 0 before trying those commands?

I can get a nice blinking (rainbow, desynced...) light on Akatsuki with:

tee /sys/class/leds/{blue,green,red}/delay* <<< 200                                                                       
tee /sys/class/leds/{blue,green,red}/brightness <<< 200 

One peculiarity: the glob is expanded right-to-left. Meaning this doesn't work:

tee /sys/class/leds/{blue,green,red}/{brightness,delay*} <<< 200 

Yet this does:

tee /sys/class/leds/{blue,green,red}/{delay*,brightness} <<< 200

It's a total mindboggling mess which I rather fix up in the driver than address in the lights HAL. I'll introduce the write to 0 again to satisfy Ganges.

[Haxk20]

Correct solution is to fix the kernel driver which we sadly do not have time at all to do. The blink fix is just temporary.

[MarijnS95]

"Temporary": I use this patch for well over half a year now.

The problem isn't that we don't have any time at all to fix the kernel driver. Rather, we put a hack in liblights and no-one cares to fix it in kernel anymore 🤷

[Haxk20]

I use it too. But as we also agreed it is just so that we can later on fix the kernel driver thats causing all this mess.

But you have to agree that our time is limited after all. If it wasnt we would find the time to fix the kernel driver a long time ago.

[xperia10dev]

Did you explicitly set breath to 0 before trying those commands?

In fact I have ended up cherry-picking sonyxperiadev/device-sony-common@84249bb and then removing all calls in Light.cpp to writeInt(${COLOR}_LED_BREATH_FILE, X), either with 0 or 1 as the written value.

It seems to be the only way to get a consistent blink behaviour in kirin without rainbows or notifications that disappear occasionally. Colors also look good in the apps that support them. This solution is good enough for me at the moment.

I will however leave this bug open since it seems to affect all devices and not only kirin. Feel free to close it if/when you think it's no longer relevant. Many thanks again for your help!

[MarijnS95]

Awesome, orbital strike on all those writes incoming!

I'll test it on my devices and update the PR. Breath is breaking the led no matter whether it's explicitly set on or off, what a useless feature...

[MarijnS95]

Disabling the writes make the LED completely unreliable on Akatsuki. It always works fine from the shell, but most of the time results in a solid ON when set through the HAL.

Goes to show that my previous point is more valid than ever:

There are too many issues with the driver currently, and this is not the right way to solve it.

I have closed the PR. It makes no sense to continue like this, it's a waste of time. Let's update the driver to the last mainline revision and see if our registers are correct in the kernel.

[simonmicro]

Hmm, spoiler: The registers are not right too!

I merged the mainline led-qti driver into my kernel (sodp, merged leds-qti..., pwm-qti..., [SOME_OTHER_HEADER_FOR_TWO_STRUCTS]) and tried to cat lut_led, which causes a kernel panic. It seems the lut (or was it lpg) is not read / set correctly. Otherwise it shows the exact same behaviour as the included driver.

EDIT: Why do I think the registers are wrong? Well, as far as I read the driver: It reads the registers content directly, therefore when they are right they should work. As the driver of the stock rom works fine...

[MartinX3]

Still happening with the XZ2 Akari and a june (2021-06-10) build.

[MarijnS95]

To give a little update on the kernel discussions we had looooong ago, at least in mainline the LEDs framework is well capable to take care of all these things, including color handling, patterns, and synchronization. Breathing in an full-RGB color is still not possible (directly from the PMIC, it is when leaving the APPS up), but is possible in the basic colors (where R, G and B are a simple binary).

It would however be quite a lot of work to backport that all, and I don't fully remember where our last kernel+userspace discussions and experiments ended up, maybe @simonmicro does?

Still happening with the XZ2 Akari and a june (2021-06-10) build.

So in that sense, don't consider this fixed soon if at all, unless hearing a large fanfare here on sonyxperiadev.

[simonmicro]

It would however be quite a lot of work to backport that all, and I don't fully remember where our last kernel+userspace discussions and experiments ended up, maybe @simonmicro does?

As far as I remember, the kernel needed support to properly write the rgb (using the lpg) patterns for every pwm channel (into their luts) - I think you @MarijnS95 had prepared an experimental patch, but it was not ready for prime time & you were worried about breaking older devices. I think I had tried an older version of it and it wasn't working... Sadly the used communication software between us dropped any older conversation data and I have therefore no link or further references anymore.

[MarijnS95]

Going through branches, let's see if I stitch together where we left off:

https://github.com/MarijnS95/kernel/commits/platforms-fix-lpg-sync Afaik this can be PR'd and merged as-is, and should fix the discoball effect (where colors are out of sync). I don't know if I have forgotten some platforms, if this needs to be applied to 4.19, nor if it required any driver changes (does not seem to be the case).

Then, we have a ton of driver fixes in https://github.com/MarijnS95/kernel/commits/sdam-pause, https://github.com/MarijnS95/kernel/commits/breath-pattern-scaling, and what I have currently flashed to all my devices, https://github.com/MarijnS95/kernel/commits/5c40a3d1cc23. Again, this needs a thorough re-check after being quite involved in the review of the LPG driver that is supposed to land in the mainline kernel: https://lore.kernel.org/linux-leds/[email protected]/t/#u.

Finally, some userspace fixups are probably in place. I seem to only have MarijnS95/device-sony-common@662917e to use the new pattern scaling according to last written brightness.

[MarijnS95]

To clarify, I probably won't have any time to look further into this in the not-so-near future. And with our mainline efforts progressing really nicely (a lot of things are so much easier to implement, most drivers are actually thought out and implemented with multiple platforms and extendability in mind) it becomes incredibly difficult to justify any time spent on downstream. Sorry.

[MartinX3]

If manpower gets redirected from downstream to mainline, it's perfect in my opinion.
I can't wait to run mainline on tama somewhere in the future. 🎉

[jerpelea]

Discontinued Android version