Bug 31803

How's the backlight faring with Keith's fixes to both -intel and drm.ko?

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).

Still present in latest git (-rc8 with the subsequent i915 pull).

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.

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.

(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).

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.

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.

Ho hum. Could it just be as simple as that the "get >>= 1" is completely wrong?

Created attachment 41959 [details] [review]
Fix backlight combination mode

Created attachment 41960 [details] [review]
Fix backlight combination mode

Works better if we don't overflow the u8.

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.

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!

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;
 }

(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.

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

(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.

Daniel is working on some patches to fix the backlight controller and is looking for victims^Wtesters.

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.

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.

(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.

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 :(