PWM backlight initial state assumptions, or how pwm_bl killed my (nyan) cat^W backlight support

DISCUSSION Paul Kocialkowski 3 participants 8 messages Jul 4, 2017 – Jul 6, 2017 PWM

Share & tools

Paul Kocialkowski

author

2017-07-04 20:13 UTC #0 lore raw

As I try to maintain support for ARM CrOS (read, ChromeOS/ChromiumOS) devices in upstream Linux on my spare time, I try to test out rc and stable versions as often as time allows. I have been rolling out 4.12 since Monday and noticed that the backlight on my tegra124 nyan big stayed dark for this release.

Not very cool, although I'm not blaming anyone else than myself on this, I should have just tested it and brought the issue up during the rc cycle. Still, let's try to move forward.

After investigating, it appears that the pwm_bl driver is enforcing a policy on heavily relying on the backlight initial state (pwm_backlight_initial_power_state). To make it short, if backlight wasn't detected as already enabled by the bootloader, it's going to refuse to enable it during the whole lifetime of the driver.

This policy isn't exactly new (so I do realize that I'm a bit late to the party), but it went one step further this cycle by adding a check on the PWM state. This broke support for my nyan big, as the pwm driver does not check for the previous state at probe time and reports it as disabled initially.

One could say that the driver has to be fixed to report that state (and I agree it is a desirable thing to do), but I think it is a symptom of a broader issue.

Basically, do we really want pwm_bl to behave this way? What is the rationale behind this decision, other than "because we can"? A strong argument against it is that not all bootloaders have support for turning the backlight on (that is definitely not the case on the omap3 sniper and omap4 kc1 boards with upstream U-Boot, that I introduced to mainline Linux).

Also, we can still expect the gpio/regulator/pwm drivers to be reset at probe time (and I also agree it's not necessarily a good thing, especially as far as backlight is concerned, but that's the reality and dropping backlight support in those cases doesn't seem like an appropriate course of action). This will result in pwm_bl assuming that backlight was not enabled by the bootloader and thus refuse to enable it at all times.

Comments and reactions are welcome, as I'd really like to find a sane way to resolve this problem.

Cheers!

Signature · 5 lines

Paul Kocialkowski, developer of free digital technology and hardware support

Website: https://www.paulk.fr/
Coding blog: https://code.paulk.fr/
Git repositories: https://git.paulk.fr/ https://git.code.paulk.fr/

Daniel Thompson

2017-07-05 10:24 UTC #1 lore raw

Quoted 8 lines · Paul Kocialkowski

As I try to maintain support for ARM CrOS (read, ChromeOS/ChromiumOS) devices in upstream Linux on my spare time, I try to test out rc and stable versions as often as time allows. I have been rolling out 4.12 since Monday and noticed that the backlight on my tegra124 nyan big stayed dark for this release.
Not very cool, although I'm not blaming anyone else than myself on this, I should have just tested it and brought the issue up during the rc cycle. Still, let's try to move forward.

Personally I might be inclined to spread the blame a bit wider ;-).

Did you bisect it down to a specific patch? An SHA-1 would be something of a time saver here!

Daniel.

Quoted 33 lines

After investigating, it appears that the pwm_bl driver is enforcing a policy on heavily relying on the backlight initial state (pwm_backlight_initial_power_state). To make it short, if backlight wasn't detected as already enabled by the bootloader, it's going to refuse to enable it during the whole lifetime of the driver.
This policy isn't exactly new (so I do realize that I'm a bit late to the party), but it went one step further this cycle by adding a check on the PWM state. This broke support for my nyan big, as the pwm driver does not check for the previous state at probe time and reports it as disabled initially.
One could say that the driver has to be fixed to report that state (and I agree it is a desirable thing to do), but I think it is a symptom of a broader issue.
Basically, do we really want pwm_bl to behave this way? What is the rationale behind this decision, other than "because we can"? A strong argument against it is that not all bootloaders have support for turning the backlight on (that is definitely not the case on the omap3 sniper and omap4 kc1 boards with upstream U-Boot, that I introduced to mainline Linux).
Also, we can still expect the gpio/regulator/pwm drivers to be reset at probe time (and I also agree it's not necessarily a good thing, especially as far as backlight is concerned, but that's the reality and dropping backlight support in those cases doesn't seem like an appropriate course of action). This will result in pwm_bl assuming that backlight was not enabled by the bootloader and thus refuse to enable it at all times.
Comments and reactions are welcome, as I'd really like to find a sane way to resolve this problem.
Cheers!

Paul Kocialkowski

author

2017-07-05 10:41 UTC #2 lore raw

Hey,

Quoted 8 lines · Daniel Thompson

Quoted 7 lines · Paul Kocialkowski
As I try to maintain support for ARM CrOS (read, ChromeOS/ChromiumOS) devices in
upstream Linux on my spare time, I try to test out rc and stable versions as often as time allows. I have been rolling out 4.12 since Monday and noticed that
the backlight on my tegra124 nyan big stayed dark for this release.

Quoted 8 lines

Quoted 3 lines
Not very cool, although I'm not blaming anyone else than myself on this, I should have just tested it and brought the issue up during the rc cycle. Still, let's try to move forward.
Personally I might be inclined to spread the blame a bit wider ;-).
Did you bisect it down to a specific patch? An SHA-1 would be something of a time saver here!

The offending commit here is d1b81294575098d989be1f2f6bb628091ceaa87b, that added a check on the intial PWM state.

The policy I'm describing was introduced with 3698d7e7d221a5c90d4b55e96d0c8f98a8b4d7df which did not break my use case at this point. It will still disable the driver if the regulator was not enabled already, which I think is not desirable. The policy on the initial GPIO state is also quite disputable.

Quoted 44 lines

Quoted 44 lines
After investigating, it appears that the pwm_bl driver is enforcing a policy on
heavily relying on the backlight initial state (pwm_backlight_initial_power_state). To make it short, if backlight wasn't detected as already enabled by the bootloader, it's going to refuse to enable it
during the whole lifetime of the driver.
This policy isn't exactly new (so I do realize that I'm a bit late to the party), but it went one step further this cycle by adding a check on the PWM state. This broke support for my nyan big, as the pwm driver does not check for
the previous state at probe time and reports it as disabled initially.
One could say that the driver has to be fixed to report that state (and I agree
it is a desirable thing to do), but I think it is a symptom of a broader issue.
Basically, do we really want pwm_bl to behave this way? What is the rationale
behind this decision, other than "because we can"? A strong argument against it
is that not all bootloaders have support for turning the backlight on (that is
definitely not the case on the omap3 sniper and omap4 kc1 boards with upstream
U-Boot, that I introduced to mainline Linux).
Also, we can still expect the gpio/regulator/pwm drivers to be reset at probe
time (and I also agree it's not necessarily a good thing, especially as far as
backlight is concerned, but that's the reality and dropping backlight support in
those cases doesn't seem like an appropriate course of action). This will result
in pwm_bl assuming that backlight was not enabled by the bootloader and thus refuse to enable it at all times.
Comments and reactions are welcome, as I'd really like to find a sane way to resolve this problem.
Cheers!

Signature · 5 lines

Paul Kocialkowski, developer of free digital technology and hardware support

Website: https://www.paulk.fr/
Coding blog: https://code.paulk.fr/
Git repositories: https://git.paulk.fr/ https://git.code.paulk.fr/

Philipp Zabel

2017-07-05 11:07 UTC #3 lore raw

Hi Paul,

Quoted 20 lines · Paul Kocialkowski

Hey,
Quoted 8 lines · Daniel Thompson
Quoted 7 lines · Paul Kocialkowski
As I try to maintain support for ARM CrOS (read, ChromeOS/ChromiumOS) devices in
upstream Linux on my spare time, I try to test out rc and stable versions as often as time allows. I have been rolling out 4.12 since Monday and noticed that
the backlight on my tegra124 nyan big stayed dark for this release.
Quoted 8 lines
Quoted 3 lines
Not very cool, although I'm not blaming anyone else than myself on this, I should have just tested it and brought the issue up during the rc cycle. Still, let's try to move forward.
Personally I might be inclined to spread the blame a bit wider ;-).
Did you bisect it down to a specific patch? An SHA-1 would be something of a time saver here!

Quoted 8 lines

The offending commit here is d1b81294575098d989be1f2f6bb628091ceaa87b, that added a check on the intial PWM state.
The policy I'm describing was introduced with 3698d7e7d221a5c90d4b55e96d0c8f98a8b4d7df which did not break my use case at this point. It will still disable the driver if the regulator was not enabled already, which I think is not desirable. The policy on the initial GPIO state is also quite disputable.

Some panels have a documented powerup sequence, which usually ends with the backlight being enabled to avoid showing garbage before the panel is initialized completely.
The reason for 3698d7e7d221 was a device with the display disabled out of the bootloader, where the backlight is controlled by the simple-panel driver. Enabling the backlight from the backlight driver before the panel driver requests the backlight to be enabled (before the panel is powered) would result in a white flash during boot.

I tried to be careful to only let the backlight driver set the initial state to disabled if a few conditions are met: the GPIO is already configured as output and disabled, and the backlight device tree node has a phandle pointing to it, so we can expect there to be some controlling instance that will enable it when appropriate.

I wonder why in your case there is a phandle link to the backlight node but nothing actually enables the backlight during boot. I would expect that to be handled by the panel driver.

Quoted 40 lines

Quoted 40 lines
Quoted 40 lines
After investigating, it appears that the pwm_bl driver is enforcing a policy on
heavily relying on the backlight initial state (pwm_backlight_initial_power_state). To make it short, if backlight wasn't detected as already enabled by the bootloader, it's going to refuse to enable it
during the whole lifetime of the driver.
This policy isn't exactly new (so I do realize that I'm a bit late to the party), but it went one step further this cycle by adding a check on the PWM state. This broke support for my nyan big, as the pwm driver does not check for
the previous state at probe time and reports it as disabled initially.
One could say that the driver has to be fixed to report that state (and I agree
it is a desirable thing to do), but I think it is a symptom of a broader issue.
Basically, do we really want pwm_bl to behave this way? What is the rationale
behind this decision, other than "because we can"? A strong argument against it
is that not all bootloaders have support for turning the backlight on (that is
definitely not the case on the omap3 sniper and omap4 kc1 boards with upstream
U-Boot, that I introduced to mainline Linux).
Also, we can still expect the gpio/regulator/pwm drivers to be reset at probe
time (and I also agree it's not necessarily a good thing, especially as far as
backlight is concerned, but that's the reality and dropping backlight support in
those cases doesn't seem like an appropriate course of action). This will result
in pwm_bl assuming that backlight was not enabled by the bootloader and thus refuse to enable it at all times.

Quoted 4 lines

Quoted 4 lines
Quoted 4 lines
Comments and reactions are welcome, as I'd really like to find a sane way to resolve this problem.
Cheers!

regards
Philipp

Paul Kocialkowski

author

2017-07-05 11:47 UTC #4 lore raw

Hi,

Quoted 52 lines · Philipp Zabel

Hi Paul,
Quoted 26 lines · Paul Kocialkowski
Hey,
Quoted 15 lines · Daniel Thompson
Quoted 9 lines · Paul Kocialkowski
As I try to maintain support for ARM CrOS (read, ChromeOS/ChromiumOS) devices in
upstream Linux on my spare time, I try to test out rc and stable versions as
often as time allows. I have been rolling out 4.12 since Monday and noticed
that
the backlight on my tegra124 nyan big stayed dark for this release.
Quoted 4 lines
Not very cool, although I'm not blaming anyone else than myself on this, I should have just tested it and brought the issue up during the rc cycle.
Still, let's try to move forward.
Quoted 4 lines
Personally I might be inclined to spread the blame a bit wider ;-).
Did you bisect it down to a specific patch? An SHA-1 would be something of a time saver here!
The offending commit here is d1b81294575098d989be1f2f6bb628091ceaa87b, that added a check on the intial PWM state.
Quoted 7 lines
The policy I'm describing was introduced with 3698d7e7d221a5c90d4b55e96d0c8f98a8b4d7df which did not break my use case at this
point. It will still disable the driver if the regulator was not enabled already, which I think is not desirable. The policy on the initial GPIO state is
also quite disputable.
Some panels have a documented powerup sequence, which usually ends with the backlight being enabled to avoid showing garbage before the panel is initialized completely.
The reason for 3698d7e7d221 was a device with the display disabled out of the bootloader, where the backlight is controlled by the simple-panel driver. Enabling the backlight from the backlight driver before the panel driver requests the backlight to be enabled (before the panel is powered) would result in a white flash during boot.
I tried to be careful to only let the backlight driver set the initial state to disabled if a few conditions are met: the GPIO is already configured as output and disabled, and the backlight device tree node has a phandle pointing to it, so we can expect there to be some controlling instance that will enable it when appropriate.

Quoted 3 lines

I wonder why in your case there is a phandle link to the backlight node but nothing actually enables the backlight during boot. I would expect that to be handled by the panel driver.

I had completely missed the fact that the panel driver is supposed to request backlight enable! With that in mind, this policy no longer concerns "the whole lifetime of the driver" but only the timeframe between backlight probe and panel probe. Thanks for clarifying this. I suppose I agree with this policy now.

So remains the question: why does the panel not enable backlight on my device?

I will investigate this later today. Hopefully, the probe defer logic should prevent a case where the panel is probed (to the end) before the backlight driver.

Quoted 60 lines

Quoted 51 lines
Quoted 51 lines
Quoted 51 lines
After investigating, it appears that the pwm_bl driver is enforcing a policy
on
heavily relying on the backlight initial state (pwm_backlight_initial_power_state). To make it short, if backlight wasn't
detected as already enabled by the bootloader, it's going to refuse to enable it
during the whole lifetime of the driver.
This policy isn't exactly new (so I do realize that I'm a bit late to the
party), but it went one step further this cycle by adding a check on the PWM
state. This broke support for my nyan big, as the pwm driver does not check
for
the previous state at probe time and reports it as disabled initially.
One could say that the driver has to be fixed to report that state (and I
agree
it is a desirable thing to do), but I think it is a symptom of a broader issue.
Basically, do we really want pwm_bl to behave this way? What is the rationale
behind this decision, other than "because we can"? A strong argument against
it
is that not all bootloaders have support for turning the backlight on (that
is
definitely not the case on the omap3 sniper and omap4 kc1 boards with upstream
U-Boot, that I introduced to mainline Linux).
Also, we can still expect the gpio/regulator/pwm drivers to be reset at probe
time (and I also agree it's not necessarily a good thing, especially as far
as
backlight is concerned, but that's the reality and dropping backlight support in
those cases doesn't seem like an appropriate course of action). This will
result
in pwm_bl assuming that backlight was not enabled by the bootloader and thus
refuse to enable it at all times.
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Comments and reactions are welcome, as I'd really like to find a sane way to
resolve this problem.
Cheers!
regards
Philipp

Signature · 5 lines

Paul Kocialkowski, developer of free digital technology and hardware support

Website: https://www.paulk.fr/
Coding blog: https://code.paulk.fr/
Git repositories: https://git.paulk.fr/ https://git.code.paulk.fr/

Paul Kocialkowski

author

2017-07-06 12:41 UTC #5 lore raw

Quoted 68 lines · Paul Kocialkowski

Hi,
Quoted 55 lines · Philipp Zabel
Hi Paul,
Quoted 28 lines · Paul Kocialkowski
Hey,
Quoted 16 lines · Daniel Thompson
Quoted 9 lines · Paul Kocialkowski
As I try to maintain support for ARM CrOS (read, ChromeOS/ChromiumOS) devices in
upstream Linux on my spare time, I try to test out rc and stable versions as
often as time allows. I have been rolling out 4.12 since Monday and noticed
that
the backlight on my tegra124 nyan big stayed dark for this release.
Quoted 5 lines
Not very cool, although I'm not blaming anyone else than myself on this,
I should have just tested it and brought the issue up during the rc cycle.
Still, let's try to move forward.
Quoted 4 lines
Personally I might be inclined to spread the blame a bit wider ;-).
Did you bisect it down to a specific patch? An SHA-1 would be something of a time saver here!
The offending commit here is d1b81294575098d989be1f2f6bb628091ceaa87b, that
added a check on the intial PWM state.
Quoted 8 lines
The policy I'm describing was introduced with 3698d7e7d221a5c90d4b55e96d0c8f98a8b4d7df which did not break my use case at
this
point. It will still disable the driver if the regulator was not enabled already, which I think is not desirable. The policy on the initial GPIO state is
also quite disputable.
Some panels have a documented powerup sequence, which usually ends with the backlight being enabled to avoid showing garbage before the panel is initialized completely.
The reason for 3698d7e7d221 was a device with the display disabled out of the bootloader, where the backlight is controlled by the simple-panel driver. Enabling the backlight from the backlight driver before the panel driver requests the backlight to be enabled (before the panel is powered) would result in a white flash during boot.
I tried to be careful to only let the backlight driver set the initial state to disabled if a few conditions are met: the GPIO is already configured as output and disabled, and the backlight device tree node has a phandle pointing to it, so we can expect there to be some controlling instance that will enable it when appropriate.
Quoted 3 lines
I wonder why in your case there is a phandle link to the backlight node but nothing actually enables the backlight during boot. I would expect that to be handled by the panel driver.
I had completely missed the fact that the panel driver is supposed to request backlight enable! With that in mind, this policy no longer concerns "the whole lifetime of the driver" but only the timeframe between backlight probe and panel
probe. Thanks for clarifying this. I suppose I agree with this policy now.

Quoted 5 lines

So remains the question: why does the panel not enable backlight on my device?
I will investigate this later today. Hopefully, the probe defer logic should prevent a case where the panel is probed (to the end) before the backlight driver.

This is actually due to the tegra drm driver not probing. It is happening on my setup (with a somewhat custom, out-of-tree config) on both jetson-tk1 and nyan_big, but also on kernelci, running both tegra_defconfig and multi_v7_defconfig.

Since this is no longer a concern related to the pwm backlight driver, I will start a new thread with the relevant addresses in CC.

Quoted 66 lines

Quoted 66 lines
Quoted 57 lines
Quoted 57 lines
Quoted 57 lines
After investigating, it appears that the pwm_bl driver is enforcing a policy
on
heavily relying on the backlight initial state (pwm_backlight_initial_power_state). To make it short, if backlight wasn't
detected as already enabled by the bootloader, it's going to refuse to enable it
during the whole lifetime of the driver.
This policy isn't exactly new (so I do realize that I'm a bit late to the
party), but it went one step further this cycle by adding a check on the
PWM
state. This broke support for my nyan big, as the pwm driver does not check
for
the previous state at probe time and reports it as disabled initially.
One could say that the driver has to be fixed to report that state (and
I
agree
it is a desirable thing to do), but I think it is a symptom of a broader
issue.
Basically, do we really want pwm_bl to behave this way? What is the rationale
behind this decision, other than "because we can"? A strong argument against
it
is that not all bootloaders have support for turning the backlight on (that
is
definitely not the case on the omap3 sniper and omap4 kc1 boards with upstream
U-Boot, that I introduced to mainline Linux).
Also, we can still expect the gpio/regulator/pwm drivers to be reset at
probe
time (and I also agree it's not necessarily a good thing, especially as
far
as
backlight is concerned, but that's the reality and dropping backlight support in
those cases doesn't seem like an appropriate course of action). This will
result
in pwm_bl assuming that backlight was not enabled by the bootloader and
thus
refuse to enable it at all times.
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Comments and reactions are welcome, as I'd really like to find a sane way to
resolve this problem.
Cheers!
regards
Philipp

Signature · 5 lines

Paul Kocialkowski, developer of free digital technology and hardware support

Website: https://www.paulk.fr/
Coding blog: https://code.paulk.fr/
Git repositories: https://git.paulk.fr/ https://git.code.paulk.fr/

Daniel Thompson

2017-07-06 12:57 UTC #6 lore raw

Quoted 31 lines · Paul Kocialkowski

Quoted 25 lines
Quoted 15 lines
Some panels have a documented powerup sequence, which usually ends with the backlight being enabled to avoid showing garbage before the panel is initialized completely.
The reason for 3698d7e7d221 was a device with the display disabled out of the bootloader, where the backlight is controlled by the simple-panel driver. Enabling the backlight from the backlight driver before the panel driver requests the backlight to be enabled (before the panel is powered) would result in a white flash during boot.
I tried to be careful to only let the backlight driver set the initial state to disabled if a few conditions are met: the GPIO is already configured as output and disabled, and the backlight device tree node has a phandle pointing to it, so we can expect there to be some controlling instance that will enable it when appropriate.
Quoted 3 lines
I wonder why in your case there is a phandle link to the backlight node but nothing actually enables the backlight during boot. I would expect that to be handled by the panel driver.
I had completely missed the fact that the panel driver is supposed to request backlight enable! With that in mind, this policy no longer concerns "the whole lifetime of the driver" but only the timeframe between backlight probe and panel
probe. Thanks for clarifying this. I suppose I agree with this policy now.
Quoted 5 lines
So remains the question: why does the panel not enable backlight on my device?
I will investigate this later today. Hopefully, the probe defer logic should prevent a case where the panel is probed (to the end) before the backlight driver.

Quoted 7 lines

This is actually due to the tegra drm driver not probing. It is happening on my setup (with a somewhat custom, out-of-tree config) on both jetson-tk1 and nyan_big, but also on kernelci, running both tegra_defconfig and multi_v7_defconfig.
Since this is no longer a concern related to the pwm backlight driver, I will start a new thread with the relevant addresses in CC.

Thanks for the update. Much appreciated!

BTW, I got the impression from your first mail that you are chasing a regression between v4.11 and v4.12. If that's the case you should know that the backlight changes we have been discussing landed in v4.11 (so if you do have a regression between these two releases then mentioning backlight in the other thread may just be confusing).

Daniel.

Paul Kocialkowski

author

2017-07-06 13:02 UTC #7 lore raw

Quoted 63 lines · Daniel Thompson

Quoted 56 lines · Paul Kocialkowski
Quoted 40 lines
Quoted 24 lines
Some panels have a documented powerup sequence, which usually ends with
the backlight being enabled to avoid showing garbage before the panel is
initialized completely.
The reason for 3698d7e7d221 was a device with the display disabled out
of the bootloader, where the backlight is controlled by the simple-panel
driver. Enabling the backlight from the backlight driver before the
panel driver requests the backlight to be enabled (before the panel is
powered) would result in a white flash during boot.
I tried to be careful to only let the backlight driver set the initial
state to disabled if a few conditions are met: the GPIO is already
configured as output and disabled, and the backlight device tree node
has a phandle pointing to it, so we can expect there to be some controlling instance that will enable it when appropriate.
Quoted 5 lines
I wonder why in your case there is a phandle link to the backlight node
but nothing actually enables the backlight during boot. I would expect
that to be handled by the panel driver.
I had completely missed the fact that the panel driver is supposed to request
backlight enable! With that in mind, this policy no longer concerns "the whole
lifetime of the driver" but only the timeframe between backlight probe and
panel
probe. Thanks for clarifying this. I suppose I agree with this policy now.
Quoted 8 lines
So remains the question: why does the panel not enable backlight on my device?
I will investigate this later today. Hopefully, the probe defer logic should
prevent a case where the panel is probed (to the end) before the backlight
driver.
This is actually due to the tegra drm driver not probing. It is happening on my
setup (with a somewhat custom, out-of-tree config) on both jetson- tk1 and
nyan_big, but also on kernelci, running both tegra_defconfig and multi_v7_defconfig.
Quoted 3 lines
Since this is no longer a concern related to the pwm backlight driver, I will
start a new thread with the relevant addresses in CC.
Thanks for the update. Much appreciated!

Quoted 8 lines

BTW, I got the impression from your first mail that you are chasing a regression between v4.11 and v4.12. If that's the case you should know
that the backlight changes we have been discussing landed in v4.11 (so
if you do have a regression between these two releases then mentioning
backlight in the other thread may just be confusing).

That makes sense. The commit I had pointed out as offending was actually not, this was just me hastily jumping to conclusions. I had just skipped the fact that the display driver was supposed to re-enable backlight.

So this is definitely not backlight-related and I'm not going to mention this in the follow-up discussion about the drm issue.

Cheers,

Signature · 7 lines

Paul Kocialkowski,

developer of free digital technology and hardware support

Website: https://www.paulk.fr/
Coding blog: https://code.paulk.fr/
Git repositories: https://git.paulk.fr/ https://git.code.paulk.fr/