[BUG] drm/i915: backlight off after resume

[BUG] drm/i915: backlight off after resume

[Jeremiah Mahler]

Jani, all,

On a Lenovo X1 Carbon if the display is off when suspend is entered
it will be off when it is resumed.  A key must be pressed to restore
normal brightness.

  xset dpms force off
  sleep 1
  sudo systemctl suspend
  (resume)
  (screen off, press any key)

The behavior I am accustomed to is for it to resume with the screen on.
All of my other machines behave this way and the X1 Carbon behaved this
way in the past.

I performed a bisect and found that the following commit introduced the
problem.

  From 6dda730e55f412a6dfb181cae6784822ba463847 Mon Sep 17 00:00:00 2001
  From: Jani Nikula <jani.nikula@intel.com>
  Date: Tue, 24 Jun 2014 18:27:40 +0300
  Subject: [PATCH] drm/i915: respect the VBT minimum backlight brightness
  
  Historically we've exposed the full backlight PWM duty cycle range to
  the userspace, in the name of "mechanism, not policy". However, it turns
  out there are both panels and board designs where there is a minimum
  duty cycle that is required for proper operation. The minimum duty cycle
  is available in the VBT.
  
  The backlight class sysfs interface does not make any promises to the
  userspace about the physical meaning of the range
  0..max_brightness. Specifically there is no guarantee that 0 means off;
  indeed for acpi_backlight 0 usually is not off, but the minimum
  acceptable value.
  
  Respect the minimum backlight, and expose the range acceptable to the
  hardware as 0..max_brightness to the userspace via the backlight class
  device; 0 means the minimum acceptable enabled value. To switch off the
  backlight, the user must disable the encoder.
  
  As a side effect, make the backlight class device max brightness and
  physical PWM modulation frequency (i.e. max duty cycle)
  independent. This allows a follow-up patch to virtualize the max value
  exposed to the userspace.
  
  Signed-off-by: Jani Nikula <jani.nikula@intel.com>
  Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
  [danvet: s/BUG_ON/WARN_ON/]
  Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

-- 
- Jeremiah Mahler

Re: [BUG] drm/i915: backlight off after resume

[Jani Nikula]

On Fri, 09 Jan 2015, Jeremiah Mahler <jmmahler@gmail.com> wrote:
> Jani, all,
>
> On a Lenovo X1 Carbon if the display is off when suspend is entered
> it will be off when it is resumed.  A key must be pressed to restore
> normal brightness.

Please file a bug on [1] and attach dmesg with drm.debug=14 set, from
boot to reproducing the problem.

Thanks for the report.

BR,
Jani.


[1] https://bugs.freedesktop.org/enter_bug.cgi?product=DRI&component=DRM/Intel

>
>   xset dpms force off
>   sleep 1
>   sudo systemctl suspend
>   (resume)
>   (screen off, press any key)
>
> The behavior I am accustomed to is for it to resume with the screen on.
> All of my other machines behave this way and the X1 Carbon behaved this
> way in the past.
>
> I performed a bisect and found that the following commit introduced the
> problem.
>
>   From 6dda730e55f412a6dfb181cae6784822ba463847 Mon Sep 17 00:00:00 2001
>   From: Jani Nikula <jani.nikula@intel.com>
>   Date: Tue, 24 Jun 2014 18:27:40 +0300
>   Subject: [PATCH] drm/i915: respect the VBT minimum backlight brightness
>   
>   Historically we've exposed the full backlight PWM duty cycle range to
>   the userspace, in the name of "mechanism, not policy". However, it turns
>   out there are both panels and board designs where there is a minimum
>   duty cycle that is required for proper operation. The minimum duty cycle
>   is available in the VBT.
>   
>   The backlight class sysfs interface does not make any promises to the
>   userspace about the physical meaning of the range
>   0..max_brightness. Specifically there is no guarantee that 0 means off;
>   indeed for acpi_backlight 0 usually is not off, but the minimum
>   acceptable value.
>   
>   Respect the minimum backlight, and expose the range acceptable to the
>   hardware as 0..max_brightness to the userspace via the backlight class
>   device; 0 means the minimum acceptable enabled value. To switch off the
>   backlight, the user must disable the encoder.
>   
>   As a side effect, make the backlight class device max brightness and
>   physical PWM modulation frequency (i.e. max duty cycle)
>   independent. This allows a follow-up patch to virtualize the max value
>   exposed to the userspace.
>   
>   Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>   Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
>   [danvet: s/BUG_ON/WARN_ON/]
>   Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> -- 
> - Jeremiah Mahler

-- 
Jani Nikula, Intel Open Source Technology Center

[PATCH] drm/i915: fix inconsistent brightness after resume

[Jeremiah Mahler]

Commit 6dda730e55f4 introduced a bug which resulted in inconsistent
brightness levels on different machines. If a suspended was entered
with the screen off some machines would resume with the screen
at minimum brightness and others at maximum brightness.

The following commands can be used to produce this behavior.

  xset dpms force off
  sleep 1
  sudo systemctl suspend
  (resume ...)

The root cause of this problem is a comparison which checks to see if
the backlight level is zero when the panel is enabled.  If it is zero,
it is set to the maximum level.  Unfortunately, not all machines have a
minimum level of zero. On those machines the level is left at the
minimum instead of begin set to the maximum.

Fix the bug by updating the comparison to check for the minimum
backlight level instead of zero.

Fixes: 6dda730e55f4 ("respect the VBT minimum backlight brightness")
Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>
---
 drivers/gpu/drm/i915/intel_panel.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index 4d63839..4ef4d66 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -962,7 +962,7 @@ void intel_panel_enable_backlight(struct intel_connector *connector)
 
 	WARN_ON(panel->backlight.max == 0);
 
-	if (panel->backlight.level == 0) {
+	if (panel->backlight.level == panel->backlight.min) {
 		panel->backlight.level = panel->backlight.max;
 		if (panel->backlight.device)
 			panel->backlight.device->props.brightness =
-- 
2.1.4

Re: [PATCH] drm/i915: fix inconsistent brightness after resume

[Jani Nikula]

On Sat, 10 Jan 2015, Jeremiah Mahler <jmmahler@gmail.com> wrote:
> Commit 6dda730e55f4 introduced a bug which resulted in inconsistent
> brightness levels on different machines. If a suspended was entered
> with the screen off some machines would resume with the screen
> at minimum brightness and others at maximum brightness.
>
> The following commands can be used to produce this behavior.
>
>   xset dpms force off
>   sleep 1
>   sudo systemctl suspend
>   (resume ...)
>
> The root cause of this problem is a comparison which checks to see if
> the backlight level is zero when the panel is enabled.  If it is zero,
> it is set to the maximum level.  Unfortunately, not all machines have a
> minimum level of zero. On those machines the level is left at the
> minimum instead of begin set to the maximum.

Good catch!

I think part of the problem is that the userspace sets brightness to
minimum before suspend, but apparently does not restore it after
resume. The dmesg would confirm this. But I guess it doesn't matter,
since we're pretty much stuck with having to do this anyway.

> Fix the bug by updating the comparison to check for the minimum
> backlight level instead of zero.
>
> Fixes: 6dda730e55f4 ("respect the VBT minimum backlight brightness")
> Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>
> ---
>  drivers/gpu/drm/i915/intel_panel.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index 4d63839..4ef4d66 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -962,7 +962,7 @@ void intel_panel_enable_backlight(struct intel_connector *connector)
>  
>  	WARN_ON(panel->backlight.max == 0);
>  
> -	if (panel->backlight.level == 0) {
> +	if (panel->backlight.level == panel->backlight.min) {

Perhaps <= instead of == would be safest?


BR,
Jani.

>  		panel->backlight.level = panel->backlight.max;
>  		if (panel->backlight.device)
>  			panel->backlight.device->props.brightness =
> -- 
> 2.1.4
>

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH] drm/i915: fix inconsistent brightness after resume

[Jeremiah Mahler]

Jani,

On Mon, Jan 12, 2015 at 12:31:09PM +0200, Jani Nikula wrote:
> On Sat, 10 Jan 2015, Jeremiah Mahler <jmmahler@gmail.com> wrote:
[...]
> 
> I think part of the problem is that the userspace sets brightness to
> minimum before suspend, but apparently does not restore it after
> resume. The dmesg would confirm this. But I guess it doesn't matter,
> since we're pretty much stuck with having to do this anyway.
> 

I did notice it doing this.  There were several calls to *_update_status
as it was entering suspend which set it to the minimum.

I am not familiar with the intricate details of this system but it seems
like there must be a way to fix this.  If the backlight can be powered
off and back on with the correct level it seems like it should be
possible when a suspend/resume is involved.

[...]
> > -	if (panel->backlight.level == 0) {
> > +	if (panel->backlight.level == panel->backlight.min) {
> 
> Perhaps <= instead of == would be safest?
> 
We could do that too in case that corner case ever arises.

[...]

I will fix it up in v2.

-- 
- Jeremiah Mahler

[PATCH v2] drm/i915: fix inconsistent brightness after resume

[Jeremiah Mahler]

Commit 6dda730e55f4 introduced a bug which resulted in inconsistent
brightness levels on different machines. If a suspended was entered
with the screen off some machines would resume with the screen
at minimum brightness and others at maximum brightness.

The following commands can be used to produce this behavior.

  xset dpms force off
  sleep 1
  sudo systemctl suspend
  (resume ...)

The root cause of this problem is a comparison which checks to see if
the backlight level is zero when the panel is enabled.  If it is zero,
it is set to the maximum level.  Unfortunately, not all machines have a
minimum level of zero. On those machines the level is left at the
minimum instead of begin set to the maximum.

Fix the bug by updating the comparison to check for the minimum
backlight level instead of zero.  Also, expand the comparison for
the possible case when the level is less than the minimum.

Fixes: 6dda730e55f4 ("respect the VBT minimum backlight brightness")
Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>
---

Notes:
    Changes in v2:
    
      - Expand the comparision for the possible case when
        the level is less than the minimum.

 drivers/gpu/drm/i915/intel_panel.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index 4d63839..dfb783a 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -962,7 +962,7 @@ void intel_panel_enable_backlight(struct intel_connector *connector)
 
 	WARN_ON(panel->backlight.max == 0);
 
-	if (panel->backlight.level == 0) {
+	if (panel->backlight.level <= panel->backlight.min) {
 		panel->backlight.level = panel->backlight.max;
 		if (panel->backlight.device)
 			panel->backlight.device->props.brightness =
-- 
2.1.4

Re: [PATCH v2] drm/i915: fix inconsistent brightness after resume

[Jani Nikula]

On Mon, 12 Jan 2015, Jeremiah Mahler <jmmahler@gmail.com> wrote:
> Commit 6dda730e55f4 introduced a bug which resulted in inconsistent
> brightness levels on different machines. If a suspended was entered
> with the screen off some machines would resume with the screen
> at minimum brightness and others at maximum brightness.
>
> The following commands can be used to produce this behavior.
>
>   xset dpms force off
>   sleep 1
>   sudo systemctl suspend
>   (resume ...)
>
> The root cause of this problem is a comparison which checks to see if
> the backlight level is zero when the panel is enabled.  If it is zero,
> it is set to the maximum level.  Unfortunately, not all machines have a
> minimum level of zero. On those machines the level is left at the
> minimum instead of begin set to the maximum.
>
> Fix the bug by updating the comparison to check for the minimum
> backlight level instead of zero.  Also, expand the comparison for
> the possible case when the level is less than the minimum.
>
> Fixes: 6dda730e55f4 ("respect the VBT minimum backlight brightness")
> Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>

Pushed to drm-intel-fixes, thanks for the patch.

BR,
Jani.


> ---
>
> Notes:
>     Changes in v2:
>     
>       - Expand the comparision for the possible case when
>         the level is less than the minimum.
>
>  drivers/gpu/drm/i915/intel_panel.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index 4d63839..dfb783a 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -962,7 +962,7 @@ void intel_panel_enable_backlight(struct intel_connector *connector)
>  
>  	WARN_ON(panel->backlight.max == 0);
>  
> -	if (panel->backlight.level == 0) {
> +	if (panel->backlight.level <= panel->backlight.min) {
>  		panel->backlight.level = panel->backlight.max;
>  		if (panel->backlight.device)
>  			panel->backlight.device->props.brightness =
> -- 
> 2.1.4
>

-- 
Jani Nikula, Intel Open Source Technology Center