Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid

[Indan Zupancic]

Hello,

On Wed, February 23, 2011 02:09, Linus Torvalds wrote:
> On Tue, Feb 22, 2011 at 2:31 PM, Tino Keitel <tino.keitel@xxxxxxxx> wrote:
>>
>> I just tried 2.6.38-rc6 on my ThinkPad X61s without any other DRM
>> related patches, and my backlight issue is gone.
>
> I applied Indan's fix in -rc6 (commit 951f3512dba5), since it had
> several testers and seemed to simplify the code nicely too.

Sadly, as so often in life, it's not correct. At this point I'm not sure
if it's better to revert that patch and add a correct one, or to just
fix it up. The end result is the same I suppose. I've also found more
documentation, namely: ACPI_IGD_OpRegion_Spec.pdf, which has the ASLE
stuff in intel_opregion.c, and VOL_1_graphics_core.pdf, which mentions
LBPC (I was looking at 3 before). Apparently the undocumented stuff
the old code did was correct. What I don't understand is how BIOS
makers could know about those bits.

The good side is that that big warning in my patch description is
invalid, something else was going on: The BIOS used LBPC to set and
restore brightness, while the driver only used BLC_PWM_CTL after my
patch.

All credits to Intel for making something simple as backlight control
as stupid and complex as possible:

- It has two registers to control brightness, sometimes one is used,
sometimes the other, sometimes both, and it's unknown what the BIOS
uses, and it's undefined what registers are restored by the BIOS after
reboot/resume.

- When using ACPI and ASLE, the kernel requests a brightness change
via a standard ACPI method, which in turns lets the BIOS generate an
ASLE interrupt, which is handled by the driver. The brightness to set
is between 0 and 255, and the driver is supposed to store the current
brightness in another register. That register stores the brightness in
percentages, which is used by the BIOS to restore brightness. How it
does that is undefined, so it can use either register. So the BIOS
obviously knows how to change the brightness, and it's still seemed
like a good idea to bother the driver with it. The ASLE interface is
a mess.

All in all, after my patch, systems using ASLE and a BIOS storing
the brightness in LBPC stopped working. The reason it works without
ASLE is because the brightness is never changed by the driver, the
backlight is only enabled or disabled.

I'd love to clean up the whole backlight mess, but it's too late in
the RC cycle to do that.

So please revert my patch and apply Takashi Iwai's, which fixes the
most immediate bug without changing anything else. This should go
in stable too.

>From f6b8a45b9544072e6ddbb944a4c03a9ec8cbca3a Mon Sep 17 00:00:00 2001 From: Takashi
Iwai <tiwai@xxxxxxx>
Date: Mon, 21 Feb 2011 14:19:27 +0100
Subject: [PATCH] drm/i915: Fix calculation of backlight value in combined mode

The commit a95735569312f2ab0c80425e2cd1e5cb0b4e1870
    drm/i915: Refactor panel backlight controls
causes a regression for GM45 that is using the combined mode for
controlling the backlight brightness.  The commit introduced a wrong bit shift for
computing the current backlight level.

Bugzilla: https://bugzilla.novell.com/show_bug.cgi?id=672946
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=34524

Signed-off-by: Takashi Iwai <tiwai@xxxxxxx>
Cc: <stable@xxxxxxxxxx>
---
 drivers/gpu/drm/i915/intel_panel.c |    1 -
 1 file changed, 1 deletion(-)

--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -176,7 +176,6 @@
 			val &= ~1;
 			pci_read_config_byte(dev->pdev, PCI_LBPC, &lbpc);
 			val *= lbpc;
-			val >>= 1;
 		}
 	}


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/