Re: PWM backlight initial state assumptions, or how pwm_bl killed my (nyan) cat^W backlight support
On 06/07/17 13:41, Paul Kocialkowski wrote: 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. 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. 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.
Re: PWM backlight initial state assumptions, or how pwm_bl killed my (nyan) cat^W backlight support
On 06/07/17 13:41, Paul Kocialkowski wrote: 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. 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. 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.
Re: PWM backlight initial state assumptions, or how pwm_bl killed my (nyan) cat^W backlight support
On Thu, 2017-07-06 at 13:57 +0100, Daniel Thompson wrote: > On 06/07/17 13:41, Paul Kocialkowski wrote: > > > > 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. > > > > > > 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. > > > > 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). 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, -- 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/
Re: PWM backlight initial state assumptions, or how pwm_bl killed my (nyan) cat^W backlight support
On Thu, 2017-07-06 at 13:57 +0100, Daniel Thompson wrote: > On 06/07/17 13:41, Paul Kocialkowski wrote: > > > > 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. > > > > > > 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. > > > > 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). 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, -- 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/
Re: PWM backlight initial state assumptions, or how pwm_bl killed my (nyan) cat^W backlight support
On Wed, 2017-07-05 at 14:47 +0300, Paul Kocialkowski wrote: > Hi, > > On Wed, 2017-07-05 at 13:07 +0200, Philipp Zabel wrote: > > Hi Paul, > > > > On Wed, 2017-07-05 at 13:41 +0300, Paul Kocialkowski wrote: > > > Hey, > > > > > > On Wed, 2017-07-05 at 11:24 +0100, Daniel Thompson wrote: > > > > On 04/07/17 21:13, Paul Kocialkowski wrote: > > > > > 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! > > > > > > 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. > > 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. 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. > > > > > 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 > > > >
Re: PWM backlight initial state assumptions, or how pwm_bl killed my (nyan) cat^W backlight support
On Wed, 2017-07-05 at 14:47 +0300, Paul Kocialkowski wrote: > Hi, > > On Wed, 2017-07-05 at 13:07 +0200, Philipp Zabel wrote: > > Hi Paul, > > > > On Wed, 2017-07-05 at 13:41 +0300, Paul Kocialkowski wrote: > > > Hey, > > > > > > On Wed, 2017-07-05 at 11:24 +0100, Daniel Thompson wrote: > > > > On 04/07/17 21:13, Paul Kocialkowski wrote: > > > > > 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! > > > > > > 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. > > 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. 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. > > > > > 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 > > > >
Re: PWM backlight initial state assumptions, or how pwm_bl killed my (nyan) cat^W backlight support
Hi, On Wed, 2017-07-05 at 13:07 +0200, Philipp Zabel wrote: > Hi Paul, > > On Wed, 2017-07-05 at 13:41 +0300, Paul Kocialkowski wrote: > > Hey, > > > > On Wed, 2017-07-05 at 11:24 +0100, Daniel Thompson wrote: > > > On 04/07/17 21:13, Paul Kocialkowski wrote: > > > > 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! > > > > 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. 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. > > > > 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
Re: PWM backlight initial state assumptions, or how pwm_bl killed my (nyan) cat^W backlight support
Hi, On Wed, 2017-07-05 at 13:07 +0200, Philipp Zabel wrote: > Hi Paul, > > On Wed, 2017-07-05 at 13:41 +0300, Paul Kocialkowski wrote: > > Hey, > > > > On Wed, 2017-07-05 at 11:24 +0100, Daniel Thompson wrote: > > > On 04/07/17 21:13, Paul Kocialkowski wrote: > > > > 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! > > > > 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. 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. > > > > 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
Re: PWM backlight initial state assumptions, or how pwm_bl killed my (nyan) cat^W backlight support
Hi Paul, On Wed, 2017-07-05 at 13:41 +0300, Paul Kocialkowski wrote: > Hey, > > On Wed, 2017-07-05 at 11:24 +0100, Daniel Thompson wrote: > > On 04/07/17 21:13, Paul Kocialkowski wrote: > > > 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! > > 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. > > > 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! regards Philipp
Re: PWM backlight initial state assumptions, or how pwm_bl killed my (nyan) cat^W backlight support
Hi Paul, On Wed, 2017-07-05 at 13:41 +0300, Paul Kocialkowski wrote: > Hey, > > On Wed, 2017-07-05 at 11:24 +0100, Daniel Thompson wrote: > > On 04/07/17 21:13, Paul Kocialkowski wrote: > > > 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! > > 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. > > > 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! regards Philipp
Re: PWM backlight initial state assumptions, or how pwm_bl killed my (nyan) cat^W backlight support
Hey, On Wed, 2017-07-05 at 11:24 +0100, Daniel Thompson wrote: > On 04/07/17 21:13, Paul Kocialkowski wrote: > > 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! 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. > > 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, 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/
Re: PWM backlight initial state assumptions, or how pwm_bl killed my (nyan) cat^W backlight support
Hey, On Wed, 2017-07-05 at 11:24 +0100, Daniel Thompson wrote: > On 04/07/17 21:13, Paul Kocialkowski wrote: > > 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! 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. > > 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, 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/
Re: PWM backlight initial state assumptions, or how pwm_bl killed my (nyan) cat^W backlight support
On 04/07/17 21:13, Paul Kocialkowski wrote: 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. 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!
Re: PWM backlight initial state assumptions, or how pwm_bl killed my (nyan) cat^W backlight support
On 04/07/17 21:13, Paul Kocialkowski wrote: 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. 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!
