Bug 31803

Summary: [GM45] Conflict between ACPI backlight controls and lvds backlight disable/enable
Product: DRI Reporter: Nick Bowler <nbowler>
Component: DRM/IntelAssignee: Daniel Vetter <daniel>
Status: CLOSED FIXED QA Contact:
Severity: normal    
Priority: medium CC: jbarnes
Version: unspecified   
Hardware: Other   
OS: All   
Whiteboard:
i915 platform: i915 features:
Attachments:
Description Flags
Fix backlight combination mode
none
Fix backlight combination mode
none
Stub out backlight enable/disable/setup functions. none

Description Nick Bowler 2010-11-20 19:15:25 UTC
With 2.6.37-rc2+, closing and opening my laptop lid causes the screen to
be dimmed by a few notches when it comes back up.  The process can be
repeated until the backlight is _completely_ off.  Touching the
backlight controls on the keyboard causes the backlight to go back to
normal.  Happens both in X and at the console.  This is a regression
from 2.6.36.

I'm using a ThinkPad T500 laptop with a GM45.  Bisection implicates the
following.  Unfortunately, it doesn't revert cleanly.

e9e331a8abeece1565d383510ed985945132ffe3 is the first bad commit
commit e9e331a8abeece1565d383510ed985945132ffe3
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Mon Sep 13 01:16:10 2010 +0100

    drm/i915/lvds: Ensure panel is unlocked for Ironlake or the panel fitter
    
    Commit 77d07fd9d73ef28689737c0952dbd5d6a5017743 introduced a regression
    where by not waiting for the panel to be turned off, left the panel and
    PLL registers locked across the modeset. Thus the panel remaining blank.
    
    As pointed out by Daniel Vetter, when testing LVDS it helps to open the
    laptop and look at the actual panel you are purporting to test.
    
    A second issue with the patch was that in order to modify the panel
    fitter before gen5, the pipe and the panel must have be completely
    powered down. So we wait.
    
    Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

:040000 040000 292f504943b059e03adbf0ec744a4a77f72dba5a 5d8c29b4581d37de8f188b8f1817847b63ce155d M	drivers

git bisect start 'drivers/gpu'
# bad: [0adcbd7033be103482fd965496c8be5dd8bcd95e] Revert "ipv6: addrconf: don't remove address state on ifdown if the address is being kept"
git bisect bad 0adcbd7033be103482fd965496c8be5dd8bcd95e
# good: [f6f94e2ab1b33f0082ac22d71f66385a60d8157f] Linux 2.6.36
git bisect good f6f94e2ab1b33f0082ac22d71f66385a60d8157f
# bad: [a424d761a00c0233cb7734a8cd572ecd6d0362aa] drm/nouveau/kms: Avoid a hang entering KDB with VT accel on.
git bisect bad a424d761a00c0233cb7734a8cd572ecd6d0362aa
# bad: [e65d9305f528d4f354378690135ee8d1c60ec0a5] drm/i915: Initialize intel_crtc->active
git bisect bad e65d9305f528d4f354378690135ee8d1c60ec0a5
# good: [106dadacbeeea92f61a2c32f3651ee31c1b34e31] drm/i915/overlay: Workaround i830 overlay activation bug.
git bisect good 106dadacbeeea92f61a2c32f3651ee31c1b34e31
# good: [f875c15a4fbf37534dda30771d8bde8604fbbf09] drm/i915: Use the direct mapping of pipe->crtc
git bisect good f875c15a4fbf37534dda30771d8bde8604fbbf09
# good: [ea056c14a269be393468fe3734f6c2319eb23a3f] drm/i915: enable thermal reporting for IPS
git bisect good ea056c14a269be393468fe3734f6c2319eb23a3f
# good: [ec5da01e23eec303dd313aa62b8ed4712c488437] drm/i915: Use msleep instead of mdelay during wait_vblank_off
git bisect good ec5da01e23eec303dd313aa62b8ed4712c488437
# bad: [8aadf70bd72c8f15994e68503af8f6722cd5c813] drm/i915/lvds: Remove incorrect mode locking
git bisect bad 8aadf70bd72c8f15994e68503af8f6722cd5c813
# good: [6edc3242e35f03990e362e7c115e722717f0f7a7] drm/i915/bios: Prevent NULL dereference after allocation failure
git bisect good 6edc3242e35f03990e362e7c115e722717f0f7a7
# bad: [e9e331a8abeece1565d383510ed985945132ffe3] drm/i915/lvds: Ensure panel is unlocked for Ironlake or the panel fitter
git bisect bad e9e331a8abeece1565d383510ed985945132ffe3
Comment 1 Chris Wilson 2010-12-07 03:33:41 UTC
How's the backlight faring with Keith's fixes to both -intel and drm.ko?
Comment 2 Nick Bowler 2010-12-07 06:11:46 UTC
The issue persists with 2.6.37-rc5 (which includes Keith's patches) and lastest git xf86-video-intel (although note that it also occurs before X is even started).
Comment 3 Nick Bowler 2011-01-03 11:23:04 UTC
Still present in latest git (-rc8 with the subsequent i915 pull).
Comment 4 Chris Wilson 2011-01-11 15:20:51 UTC
I believe this is the fix you are looking for:

commit 47356eb67285014527a5ab87543ba1fae3d1e10a
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Tue Jan 11 17:06:04 2011 +0000

    drm/i915/panel: Only record the backlight level when it is enabled
    
    By tracking the current status of the backlight we can prevent recording
    the value of the current backlight when we have disabled it. And so
    prevent restoring it to 'off' after an unbalanced sequence of
    intel_lvds_disable/enable.
    
    Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=22672
    Tested-by: Alex Riesen <raa.lkml@gmail.com>
    Tested-by: Larry Finger <Larry.Finger@lwfinger.net>
    Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
    Cc: stable@kernel.org

pushed to -fixes and pull request sent.
Comment 5 Nick Bowler 2011-01-11 17:56:26 UTC
Unfortunately, that commit doesn't seem to have changed anything.  I tested
both 2.6.37 with that commit cherry-picked, and also Linus' master with your
drm-intel-fixes branch merged in: in either case, the backlight is still
dimmed when the lid is opened.

Something I didn't really notice until recently is that there is a delay before
the backlight gets set to the wrong value: when the lid is first opened, the
backlight comes on at the (presumably) correct brightness; after about half a
second or so, it is set to the dimmer value.
Comment 6 Nick Bowler 2011-01-11 18:19:03 UTC
(In reply to comment #5)
> Unfortunately, that commit doesn't seem to have changed anything.

I spoke a little too soon: it did change _something_.  With this commit,
the backlight cannot be dimmed repeatedly by opening and closing the lid
multiple times as described on the original report -- if I close and open the
lid a second time, the backlight will remain at the previous (dim) level.  If
I touch one of the brightness keys, everything is reset and closing/opening the
lid will again dim the backlight (once).
Comment 7 Chris Wilson 2011-01-12 03:46:30 UTC
Nick, do you mind instrumenting intel_panel_enable_backlight() and intel_panel_disable_backlight() to check when they are being called and recording the backlight level before turning off? What it sounds like is there is a conflict between the value we are reading and the value ACPI is using.
Comment 8 Nick Bowler 2011-01-12 16:58:02 UTC
So this seems interesting: with Linus' master, I added printks to
intel_panel_{enable,disable}_backlight and booted with drm.debug=0x2 to see
what's going on.  Here's the redacted and annotated summary of what gets
printed.

Early in boot (before fbcon is started), I can see the printk I added in
disable:

  [drm:intel_panel_get_backlight], get backlight PWM = 1204110
  intel_panel_disable_backlight: level was 1204110

(so the level 1204110 got stored in the private structure.  There is another
call to disable_backlight which skips saving the value due to the earlier
patch).

Later, after fbcon is initialized, we see the backlight being set to the proper
level (via ACPI?) -- it remains at this level until the lid is closed.

  Console: switching to colour frame buffer device 210x65
  fb0: inteldrmfb frame buffer device
  drm: registered panic notifier
  [drm:intel_opregion_asle_intr], non asle set request??
  [drm:intel_panel_get_max_backlight], max backlight PWM = 2408475
  [drm:intel_panel_set_backlight], set backlight PWM = 2408475
  [drm:intel_panel_get_max_backlight], max backlight PWM = 2408475
  [drm:intel_opregion_asle_intr], non asle set request??
  [drm:intel_opregion_asle_intr], non asle set request??
  [drm:intel_panel_get_max_backlight], max backlight PWM = 2408475

After boot, when the lid is closed and opened, the following is printed (note
that disable_backlight is *not* called):

  intel_panel_enable_backlight: level set to 1204110
  [drm:intel_panel_set_backlight], set backlight PWM = 1204110
  [drm:intel_panel_get_max_backlight], max backlight PWM = 2408475
  [drm:intel_opregion_asle_intr], non asle set request??

So enable_backlight restored the value saved at early boot, which is presumably
the dim value that I'm seeing.  (If I use the keyboard controls to drop the
backlight as far as it'll go, the effect is that the screen gets brighter when
the lid is closed and opened).  When I press the backlight up key, I see
requests to set the backlight to the maximum value:

  [drm:intel_opregion_asle_intr], non asle set request??
  [drm:intel_panel_get_max_backlight], max backlight PWM = 2408475
  [drm:intel_panel_set_backlight], set backlight PWM = 2408475
  [drm:intel_panel_get_max_backlight], max backlight PWM = 2408475
  [drm:intel_opregion_asle_intr], non asle set request??

Finally, I decided to look at what gets printed when I do 'xset dpms force
standby' -- this case was uninteresting to me before because it doesn't cause
any problems with the backlight.  But with the debugging info, it looks really
interesting.  First, on standby (while the screen is still blank):

  [drm:intel_panel_get_backlight], get backlight PWM = 1204110
  intel_panel_disable_backlight: level was 1204110
  [drm:intel_panel_set_backlight], set backlight PWM = 0
  [drm:intel_panel_get_max_backlight], max backlight PWM = 2408475
  [drm:intel_opregion_asle_intr], non asle set request??
  [drm:intel_opregion_asle_intr], non asle set request??
  [drm:intel_panel_get_max_backlight], max backlight PWM = 2408475
  [drm:intel_panel_set_backlight], set backlight PWM = 217235
  [drm:intel_panel_get_max_backlight], max backlight PWM = 2408475
  [drm:intel_opregion_asle_intr], non asle set request??
  intel_panel_disable_backlight: level was 4294967295
  [drm:intel_panel_set_backlight], set backlight PWM = 0
  [drm:intel_panel_get_max_backlight], max backlight PWM = 2408475
  [drm:intel_opregion_asle_intr], non asle set request??
  [drm:intel_opregion_asle_intr], non asle set request??
  [drm:intel_panel_get_max_backlight], max backlight PWM = 2408475
  [drm:intel_panel_set_backlight], set backlight PWM = 217235
  [drm:intel_panel_get_max_backlight], max backlight PWM = 2408475
  [drm:intel_opregion_asle_intr], non asle set request??

It seems that 1204110 got saved, despite the backlight being set to 2408475.
(the value 4294967295 was the one I used if nothing got saved due to the
double call to disable_backlight).  Finally, when the srceen is brought out of
standby, we se the following:

  intel_panel_enable_backlight: level set to 1204110
  [drm:intel_panel_set_backlight], set backlight PWM = 1204110
  [drm:intel_panel_get_max_backlight], max backlight PWM = 2408475
  [drm:intel_opregion_asle_intr], non asle set request??
  [drm:intel_opregion_asle_intr], non asle set request??
  [drm:intel_panel_get_max_backlight], max backlight PWM = 2408475
  [drm:intel_panel_set_backlight], set backlight PWM = 2408475
  [drm:intel_panel_get_max_backlight], max backlight PWM = 2408475
  [drm:intel_opregion_asle_intr], non asle set request??

So it seems that in the DPMS case, the backlight was restored to the wrong
level after all, but then something (ACPI?) sets it to the right one.

Let me know if you need any more info.
Comment 9 Chris Wilson 2011-01-12 17:21:55 UTC
Ho hum. Could it just be as simple as that the "get >>= 1" is completely wrong?
Comment 10 Chris Wilson 2011-01-13 04:55:11 UTC
Created attachment 41959 [details] [review]
Fix backlight combination mode
Comment 11 Chris Wilson 2011-01-13 05:25:33 UTC
Created attachment 41960 [details] [review]
Fix backlight combination mode

Works better if we don't overflow the u8.
Comment 12 Nick Bowler 2011-01-13 06:35:31 UTC
I won't be able to test anything until this evening, but I tried the first patch earlier -- with it, the system restores the backlight to full brightness every time (probably because disable_backlight does not seem to be called when I close the lid).  So if I lower the backlight using the keyboard controls, the issue still occurs (but in reverse).

I decided to try out 2.6.36 again, as well, and I noticed something else with this kernel (and I think all others before it) which may be related:  When the panel is opened, first the backlight is on (at the correct level), then the screen goes completely black, then the screen turns on again (at the correct backlight level).  The time where the screen goes black in 2.6.36 is the same as where 2.6.37 sets the backlight to the wrong level -- the difference is that 2.6.36 seems to correct the issue after a moment but 2.6.37 does not.
Comment 13 Nick Bowler 2011-01-13 16:24:26 UTC
Created attachment 41989 [details] [review]
Stub out backlight enable/disable/setup functions.

(In reply to comment #11)
> Created an attachment (id=41960) [details]
> Fix backlight combination mode
> 
> Works better if we don't overflow the u8.

The second patch completely breaks the keyboard backlight controls; with it
applied, I can only set the backlight to either maximum brightness or
completely off.  Specifically, every setting except the brightest results in
the backlight turning off (e.g., if I press backlight-down 8 times, I have to
press backlight-up 8 times for the panel to turn back on).

The first patch also caused some issues with the keyboard controls, but none
so severe as the second.

Anyway, I decided to try an experiment with the attached patch.  I'm probably
not understanding all the issues, but my backlight seems to work perfectly with
the enable/disable/setup functions stubbed out!
Comment 14 Chris Wilson 2011-01-14 02:07:53 UTC
Yeah, on your machine we have two drivers fighting over control of the backlight. Admittedly one of those is broken...

One last experiment:
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_pan
index 71ba046..c8bcc4c 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -282,6 +282,9 @@ void intel_panel_setup_backlight(struct drm_device *dev)
 {
        struct drm_i915_private *dev_priv = dev->dev_private;
 
+       I915_WRITE(BLC_PWM_CTL2,
+                  I915_READ(BLC_PWM_CTL2) &~ BLM_COMBINATION_MODE)
+
        dev_priv->backlight_level = intel_panel_get_backlight(dev);
        dev_priv->backlight_enabled = dev_priv->backlight_level != 0;
 }
Comment 15 Nick Bowler 2011-01-14 16:41:53 UTC
(In reply to comment #14)
> Yeah, on your machine we have two drivers fighting over control of the
> backlight. Admittedly one of those is broken...
> 
> One last experiment:

The effects of this patch are the same as described in comment #12 (first
paragraph).  That is: the backlight seems to be restored correctly but the
current level is not saved when I close the lid (it is saved on dpms off,
however).  Furthermore, the keyboard controls still work correctly with this
one.
Comment 16 Chris Wilson 2011-02-24 15:22:39 UTC
commit 951f3512dba5bd44cda3e5ee22b4b522e4bb09fb
Author: Indan Zupancic <indan@nul.nu>
Date:   Thu Feb 17 02:41:49 2011 +0100

    drm/i915: Do not handle backlight combination mode specially
Comment 17 Nick Bowler 2011-02-24 16:33:25 UTC
(In reply to comment #16)
> commit 951f3512dba5bd44cda3e5ee22b4b522e4bb09fb
> Author: Indan Zupancic <indan@nul.nu>
> Date:   Thu Feb 17 02:41:49 2011 +0100
> 
>     drm/i915: Do not handle backlight combination mode specially

Unfortunately, this hasn't fixed the issue completely: the effects are the same
as described in comment #15.

To clarify: the backlight is restored to whatever level it was at during the
last DPMS off (or bootup, whichever is more recent).

So if I boot up with the backlight at full brightness, then set the backlight
as dim as it'll go, then close/open the lid, the backlight will come back at
full brightness again.  As before, once I touch the backlight controls, it will
suddenly become fully dim.

Similarly, if I set the backlight fully dim, then dpms off/on, then set the
backlight fully bright, then close/open the lid, the backlight will come back
filly dim.  Once I touch the backlight controls in this case, it will suddenly
be fully bright again.

For the record, my patch which deletes the backlight-setting code still works.
Comment 18 Chris Wilson 2012-05-11 07:17:50 UTC
Daniel is working on some patches to fix the backlight controller and is looking for victims^Wtesters.
Comment 19 Daniel Vetter 2012-06-05 07:09:13 UTC
Ok, I've created these patches, available at http://cgit.freedesktop.org/~danvet/drm/log/?h=backlight-confusion

Git link is git://people.freedesktop.org/~danvet/drm backlight-confusion

Please test them, thanks.
Comment 20 Daniel Vetter 2012-08-22 14:20:15 UTC
Reporter seems to have disappeared, but 3.6 has all the backlight changes. Presumably the bug is now fixed, please reopen if this is still an issue.
Comment 21 Nick Bowler 2012-08-23 01:23:43 UTC
(In reply to comment #20)
> Reporter seems to have disappeared, but 3.6 has all the backlight changes.
> Presumably the bug is now fixed, please reopen if this is still an issue.

The regression as originally reported has actually been fixed for some time
now (i.e., the behaviour is the same as it was with Linux 2.6.36).  So yes,
this bug can be closed as fixed.

As it was with Linux 2.6.36, the screen turns black for a very short time about
1 second after opening the lid.  It is clearly possible to work better than
this, since there was no such flicker (and the backlight worked fine) way back
when I tested the patch in comment #13.  That patch no longer works.

Anyway, that flicker is a separate issue not related to the original report,
and is not a regression.  I may file another bug about it if it still happens
with 3.6.
Comment 22 Daniel Vetter 2012-08-23 08:14:44 UTC
That flickering when opening the lid is most likely a "feature" added by Linus Torvalds in:

commit c9354c85c1c7bac788ce57d3c17f2016c1c45b1d
Author: Linus Torvalds <torvalds@linux-foundation.org>
Date:   Mon Nov 2 09:29:55 2009 -0800

    i915: fix intel graphics suspend breakage due to resume/lid event confusion

That commit adds a superflous (sometimes) modeset at lid open. We're looking into it, unfortunately this area is a regression minefield, so don't expect anything anytime soon :(

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.