Hi,
On Tue 04-01-11 17:15:45, Linus Torvalds wrote: [...]
We did have another revert to fix hopefullythe last "blank screen" regression on intel graphics.
It seems that there is still a regression for intel graphic cards backlight. One report is https://bugzilla.kernel.org/show_bug.cgi?id=22672. I can reproduce the problem easily by: xset dpms force standby; sleep 3s; xset dpms force on
backlight doesn't get up (there is really dark picture though which doesn't get brighter by function keys which work normally) after dpms on until I close and open lid.
The problem wasn't present in 2.6.36
$ lspci -vv [...] 00:02.0 VGA compatible controller: Intel Corporation Mobile 945GM/GMS, 943/940GML Express Integrated Graphics Controller (rev 03) (prog-if 00 [VGA controller]) Subsystem: Fujitsu Limited. Device 137a Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx- Status: Cap+ 66MHz- UDF- FastB2B+ ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx- Latency: 0 Interrupt: pin A routed to IRQ 16 Region 0: Memory at f0300000 (32-bit, non-prefetchable) [size=512K] Region 1: I/O ports at 1800 [size=8] Region 2: Memory at e0000000 (32-bit, prefetchable) [size=256M] Region 3: Memory at f0400000 (32-bit, non-prefetchable) [size=256K] Expansion ROM at <unassigned> [disabled] Capabilities: <access denied> Kernel driver in use: i915 [...]
On Thu, Jan 6, 2011 at 2:48 AM, Michal Hocko mhocko@suse.cz wrote:
It seems that there is still a regression for intel graphic cards backlight. One report is https://bugzilla.kernel.org/show_bug.cgi?id=22672. I can reproduce the problem easily by: xset dpms force standby; sleep 3s; xset dpms force on
backlight doesn't get up (there is really dark picture though which doesn't get brighter by function keys which work normally) after dpms on until I close and open lid.
Hmm. That commit no longer reverts cleanly, so it's not trivial to test whether all those things are exactly the same issue. It's been bisected in the bugzilla entry, but it would be good to verify that yes, reverting it really does fix the issue, and your issue is the exact same one.
Chris, any ideas?
Linus
Just for reference, my initial report was: https://lkml.org/lkml/2010/11/23/146
On Thu 06-01-11 08:29:22, Linus Torvalds wrote:
On Thu, Jan 6, 2011 at 2:48 AM, Michal Hocko mhocko@suse.cz wrote:
It seems that there is still a regression for intel graphic cards backlight. One report is https://bugzilla.kernel.org/show_bug.cgi?id=22672. I can reproduce the problem easily by: xset dpms force standby; sleep 3s; xset dpms force on
backlight doesn't get up (there is really dark picture though which doesn't get brighter by function keys which work normally) after dpms on until I close and open lid.
Hmm. That commit no longer reverts cleanly, so it's not trivial to test whether all those things are exactly the same issue. It's been bisected in the bugzilla entry, but it would be good to verify that yes, reverting it really does fix the issue, and your issue is the exact same one.
Chris, any ideas?
Linus
On Thu, 6 Jan 2011 08:29:22 -0800, Linus Torvalds torvalds@linux-foundation.org wrote:
On Thu, Jan 6, 2011 at 2:48 AM, Michal Hocko mhocko@suse.cz wrote:
It seems that there is still a regression for intel graphic cards backlight. One report is https://bugzilla.kernel.org/show_bug.cgi?id=22672. I can reproduce the problem easily by: xset dpms force standby; sleep 3s; xset dpms force on
backlight doesn't get up (there is really dark picture though which doesn't get brighter by function keys which work normally) after dpms on until I close and open lid.
Hmm. That commit no longer reverts cleanly, so it's not trivial to test whether all those things are exactly the same issue. It's been bisected in the bugzilla entry, but it would be good to verify that yes, reverting it really does fix the issue, and your issue is the exact same one.
Chris, any ideas?
My fear is that some machines have a dependency between the backlight and panel power status. The patch in question changed the timing between turning on the panel and adjusting the backlight which would be restore with:
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c index aa23070..0b40b4f 100644 --- a/drivers/gpu/drm/i915/intel_lvds.c +++ b/drivers/gpu/drm/i915/intel_lvds.c @@ -106,6 +106,12 @@ static void intel_lvds_enable(struct intel_lvds *intel_lvds) I915_WRITE(ctl_reg, I915_READ(ctl_reg) | POWER_TARGET_ON); POSTING_READ(lvds_reg);
+ { + u32 reg = HAS_PCH_SPLIT(dev) ? PCH_PP_STATUS : PPS_STATUS; + if (wait_for(I915_READ(reg) & PP_ON, 1000)) + DRM_ERROR("timed out waiting for panel to power up\n"); + } + intel_panel_set_backlight(dev, dev_priv->backlight_level); }
On Thu, Jan 6, 2011 at 18:49, Chris Wilson chris@chris-wilson.co.uk wrote:
My fear is that some machines have a dependency between the backlight and panel power status. The patch in question changed the timing between turning on the panel and adjusting the backlight which would be restore with:
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c index aa23070..0b40b4f 100644 --- a/drivers/gpu/drm/i915/intel_lvds.c +++ b/drivers/gpu/drm/i915/intel_lvds.c @@ -106,6 +106,12 @@ static void intel_lvds_enable(struct intel_lvds *intel_lvds) I915_WRITE(ctl_reg, I915_READ(ctl_reg) | POWER_TARGET_ON); POSTING_READ(lvds_reg);
- {
- u32 reg = HAS_PCH_SPLIT(dev) ? PCH_PP_STATUS : PPS_STATUS;
- if (wait_for(I915_READ(reg) & PP_ON, 1000))
- DRM_ERROR("timed out waiting for panel to power up\n");
- }
intel_panel_set_backlight(dev, dev_priv->backlight_level); }
FWIW it does not compile: CC drivers/gpu/drm/i915/intel_lvds.o drivers/gpu/drm/i915/intel_lvds.c: In function ‘intel_lvds_enable’: drivers/gpu/drm/i915/intel_lvds.c:110: error: ‘PPS_STATUS’ undeclared (first use in this function) drivers/gpu/drm/i915/intel_lvds.c:110: error: (Each undeclared identifier is reported only once drivers/gpu/drm/i915/intel_lvds.c:110: error: for each function it appears in.) make[4]: *** [drivers/gpu/drm/i915/intel_lvds.o] Error 1
On Thu, 6 Jan 2011 21:55:23 +0100, Alex Riesen raa.lkml@gmail.com wrote:
On Thu, Jan 6, 2011 at 18:49, Chris Wilson chris@chris-wilson.co.uk wrote:
My fear is that some machines have a dependency between the backlight and panel power status. The patch in question changed the timing between turning on the panel and adjusting the backlight which would be restore with:
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c index aa23070..0b40b4f 100644 --- a/drivers/gpu/drm/i915/intel_lvds.c +++ b/drivers/gpu/drm/i915/intel_lvds.c @@ -106,6 +106,12 @@ static void intel_lvds_enable(struct intel_lvds *intel_lvds) I915_WRITE(ctl_reg, I915_READ(ctl_reg) | POWER_TARGET_ON); POSTING_READ(lvds_reg);
- {
- u32 reg = HAS_PCH_SPLIT(dev) ? PCH_PP_STATUS : PPS_STATUS;
- if (wait_for(I915_READ(reg) & PP_ON, 1000))
- DRM_ERROR("timed out waiting for panel to power up\n");
- }
intel_panel_set_backlight(dev, dev_priv->backlight_level); }
FWIW it does not compile: CC drivers/gpu/drm/i915/intel_lvds.o drivers/gpu/drm/i915/intel_lvds.c: In function ‘intel_lvds_enable’: drivers/gpu/drm/i915/intel_lvds.c:110: error: ‘PPS_STATUS’ undeclared (first use in this function) drivers/gpu/drm/i915/intel_lvds.c:110: error: (Each undeclared identifier is reported only once drivers/gpu/drm/i915/intel_lvds.c:110: error: for each function it appears in.) make[4]: *** [drivers/gpu/drm/i915/intel_lvds.o] Error 1
Daniel quickly pointed out my typo: s/PPS_STATUS/PP_STATUS/
Apologies, -Chris
On Thu, Jan 6, 2011 at 21:55, Alex Riesen raa.lkml@gmail.com wrote:
On Thu, Jan 6, 2011 at 18:49, Chris Wilson chris@chris-wilson.co.uk wrote:
My fear is that some machines have a dependency between the backlight and panel power status. The patch in question changed the timing between turning on the panel and adjusting the backlight which would be restore with:
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c index aa23070..0b40b4f 100644 --- a/drivers/gpu/drm/i915/intel_lvds.c +++ b/drivers/gpu/drm/i915/intel_lvds.c @@ -106,6 +106,12 @@ static void intel_lvds_enable(struct intel_lvds *intel_lvds) I915_WRITE(ctl_reg, I915_READ(ctl_reg) | POWER_TARGET_ON); POSTING_READ(lvds_reg);
- {
- u32 reg = HAS_PCH_SPLIT(dev) ? PCH_PP_STATUS : PPS_STATUS;
...
FWIW it does not compile: CC drivers/gpu/drm/i915/intel_lvds.o drivers/gpu/drm/i915/intel_lvds.c: In function ‘intel_lvds_enable’: drivers/gpu/drm/i915/intel_lvds.c:110: error: ‘PPS_STATUS’ undeclared
Ah, I see. Should be PP_STATUS. Whatever. It does not help. The backlight stays off.
P.S. Probably unrelated, but I just noticed that the backlight never goes off when closing the lid. Am I supposed to hook up on the corresponding input event and put the panel in standby? It used to work all by itself, I think...
On Thu 06-01-11 22:08:46, Alex Riesen wrote:
On Thu, Jan 6, 2011 at 21:55, Alex Riesen raa.lkml@gmail.com wrote:
On Thu, Jan 6, 2011 at 18:49, Chris Wilson chris@chris-wilson.co.uk wrote:
My fear is that some machines have a dependency between the backlight and panel power status. The patch in question changed the timing between turning on the panel and adjusting the backlight which would be restore with:
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c index aa23070..0b40b4f 100644 --- a/drivers/gpu/drm/i915/intel_lvds.c +++ b/drivers/gpu/drm/i915/intel_lvds.c @@ -106,6 +106,12 @@ static void intel_lvds_enable(struct intel_lvds *intel_lvds) ?? ?? ?? ??I915_WRITE(ctl_reg, I915_READ(ctl_reg) | POWER_TARGET_ON); ?? ?? ?? ??POSTING_READ(lvds_reg);
- ?? ?? ?? {
- ?? ?? ?? ?? ?? ?? ?? u32 reg = HAS_PCH_SPLIT(dev) ? PCH_PP_STATUS : PPS_STATUS;
...
FWIW it does not compile: ??CC ?? ?? ??drivers/gpu/drm/i915/intel_lvds.o drivers/gpu/drm/i915/intel_lvds.c: In function ???intel_lvds_enable???: drivers/gpu/drm/i915/intel_lvds.c:110: error: ???PPS_STATUS??? undeclared
Ah, I see. Should be PP_STATUS. Whatever. It does not help. The backlight stays off.
It didn't help in my case either
Hi,
On Tue 04-01-11 17:15:45, Linus Torvalds wrote: [...]
We did have another revert to fix hopefullythe last "blank screen" regression on intel graphics.
It seems that there is still a regression for intel graphic cards backlight. One report is https://bugzilla.kernel.org/show_bug.cgi?id=22672. I can reproduce the problem easily by: xset dpms force standby; sleep 3s; xset dpms force on
(You are using "vesa" or "fbcon" X11 driver, right? I seen same problem until I switched to "intel" X11 driver).
On Tue 11-01-11 14:33:20, Pavel Machek wrote:
Hi,
On Tue 04-01-11 17:15:45, Linus Torvalds wrote: [...]
We did have another revert to fix hopefullythe last "blank screen" regression on intel graphics.
It seems that there is still a regression for intel graphic cards backlight. One report is https://bugzilla.kernel.org/show_bug.cgi?id=22672. I can reproduce the problem easily by: xset dpms force standby; sleep 3s; xset dpms force on
(You are using "vesa" or "fbcon" X11 driver, right? I seen same problem until I switched to "intel" X11 driver).
I am using intel X11 driver: [...] (II) Loading extension DRI2 (II) LoadModule: "intel" (II) Loading /usr/lib/xorg/modules/drivers/intel_drv.so [...]
So this doesn't look like the case.
On Tue, Jan 11, 2011 at 14:33, Pavel Machek pavel@ucw.cz wrote:
I can reproduce the problem easily by: xset dpms force standby; sleep 3s; xset dpms force on
(You are using "vesa" or "fbcon" X11 driver, right? I seen same problem until I switched to "intel" X11 driver).
No, the "intel".
[Let's CC Indan - author of the bugzilla patches]
On Thu 06-01-11 11:48:16, Michal Hocko wrote:
Hi,
On Tue 04-01-11 17:15:45, Linus Torvalds wrote: [...]
We did have another revert to fix hopefullythe last "blank screen" regression on intel graphics.
It seems that there is still a regression for intel graphic cards backlight. One report is https://bugzilla.kernel.org/show_bug.cgi?id=22672. I can reproduce the problem easily by: xset dpms force standby; sleep 3s; xset dpms force on
backlight doesn't get up (there is really dark picture though which doesn't get brighter by function keys which work normally) after dpms on until I close and open lid.
The problem wasn't present in 2.6.36
I can confirm that this problem is not present if both patches from bko#22672 are applied on top of 2.6.37 kernel. I haven't tried both patches separately, but I can surely try it if it makes any sense.
$ lspci -vv [...] 00:02.0 VGA compatible controller: Intel Corporation Mobile 945GM/GMS, 943/940GML Express Integrated Graphics Controller (rev 03) (prog-if 00 [VGA controller]) Subsystem: Fujitsu Limited. Device 137a Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx- Status: Cap+ 66MHz- UDF- FastB2B+ ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx- Latency: 0 Interrupt: pin A routed to IRQ 16 Region 0: Memory at f0300000 (32-bit, non-prefetchable) [size=512K] Region 1: I/O ports at 1800 [size=8] Region 2: Memory at e0000000 (32-bit, prefetchable) [size=256M] Region 3: Memory at f0400000 (32-bit, non-prefetchable) [size=256K] Expansion ROM at <unassigned> [disabled] Capabilities: <access denied> Kernel driver in use: i915
[...]
On Tue 11-01-11 18:17:44, Michal Hocko wrote:
[Let's CC Indan - author of the bugzilla patches]
On Thu 06-01-11 11:48:16, Michal Hocko wrote:
Hi,
On Tue 04-01-11 17:15:45, Linus Torvalds wrote: [...]
We did have another revert to fix hopefullythe last "blank screen" regression on intel graphics.
It seems that there is still a regression for intel graphic cards backlight. One report is https://bugzilla.kernel.org/show_bug.cgi?id=22672. I can reproduce the problem easily by: xset dpms force standby; sleep 3s; xset dpms force on
backlight doesn't get up (there is really dark picture though which doesn't get brighter by function keys which work normally) after dpms on until I close and open lid.
The problem wasn't present in 2.6.36
I can confirm that this problem is not present if both patches from bko#22672 are applied on top of 2.6.37 kernel. I haven't tried both patches separately, but I can surely try it if it makes any sense.
I have just tried that and they are really both necessary.
On Tue 11-01-11 18:37:41, Michal Hocko wrote:
On Tue 11-01-11 18:17:44, Michal Hocko wrote:
[Let's CC Indan - author of the bugzilla patches]
On Thu 06-01-11 11:48:16, Michal Hocko wrote:
Hi,
On Tue 04-01-11 17:15:45, Linus Torvalds wrote: [...]
We did have another revert to fix hopefullythe last "blank screen" regression on intel graphics.
It seems that there is still a regression for intel graphic cards backlight. One report is https://bugzilla.kernel.org/show_bug.cgi?id=22672. I can reproduce the problem easily by: xset dpms force standby; sleep 3s; xset dpms force on
backlight doesn't get up (there is really dark picture though which doesn't get brighter by function keys which work normally) after dpms on until I close and open lid.
The problem wasn't present in 2.6.36
I can confirm that this problem is not present if both patches from bko#22672 are applied on top of 2.6.37 kernel. I haven't tried both patches separately, but I can surely try it if it makes any sense.
I have just tried that and they are really both necessary.
I must have mixed something. After retesting with the first patch (https://bugzilla.kernel.org/show_bug.cgi?id=22672#c33) the usecase works just fine (backlight is on without need to close&open the lid)
Sorry for confusion
On Tue, 11 Jan 2011 18:19:17 +0100, Michal Hocko mhocko@suse.cz wrote:
[Let's CC Indan - author of the bugzilla patches]
On Thu 06-01-11 11:48:16, Michal Hocko wrote:
Hi,
On Tue 04-01-11 17:15:45, Linus Torvalds wrote: [...]
We did have another revert to fix hopefullythe last "blank screen" regression on intel graphics.
It seems that there is still a regression for intel graphic cards backlight. One report is https://bugzilla.kernel.org/show_bug.cgi?id=22672. I can reproduce the problem easily by: xset dpms force standby; sleep 3s; xset dpms force on
backlight doesn't get up (there is really dark picture though which doesn't get brighter by function keys which work normally) after dpms on until I close and open lid.
The problem wasn't present in 2.6.36
I can confirm that this problem is not present if both patches from bko#22672 are applied on top of 2.6.37 kernel. I haven't tried both patches separately, but I can surely try it if it makes any sense.
The second patch is wrong in that it will prevent changing resolutions using the panel fitter. The first patch looks along the right lines, just misses the possibility that the backlight can be modified by other means.
So hopefully, you just need the first patch. -Chris
On Tue 11-01-11 17:39:42, Chris Wilson wrote:
On Tue, 11 Jan 2011 18:19:17 +0100, Michal Hocko mhocko@suse.cz wrote:
[Let's CC Indan - author of the bugzilla patches]
On Thu 06-01-11 11:48:16, Michal Hocko wrote:
Hi,
On Tue 04-01-11 17:15:45, Linus Torvalds wrote: [...]
We did have another revert to fix hopefullythe last "blank screen" regression on intel graphics.
It seems that there is still a regression for intel graphic cards backlight. One report is https://bugzilla.kernel.org/show_bug.cgi?id=22672. I can reproduce the problem easily by: xset dpms force standby; sleep 3s; xset dpms force on
backlight doesn't get up (there is really dark picture though which doesn't get brighter by function keys which work normally) after dpms on until I close and open lid.
The problem wasn't present in 2.6.36
I can confirm that this problem is not present if both patches from bko#22672 are applied on top of 2.6.37 kernel. I haven't tried both patches separately, but I can surely try it if it makes any sense.
The second patch is wrong in that it will prevent changing resolutions using the panel fitter. The first patch looks along the right lines, just misses the possibility that the backlight can be modified by other means.
So hopefully, you just need the first patch.
I would have bet I have tested the 1st patch but let me double check. The 2nd patch alone doesn't fix the problem.
-Chris
-- Chris Wilson, Intel Open Source Technology Centre
Hello,
On Tue, January 11, 2011 18:39, Chris Wilson wrote:
On Tue, 11 Jan 2011 18:19:17 +0100, Michal Hocko mhocko@suse.cz wrote:
[Let's CC Indan - author of the bugzilla patches]
On Thu 06-01-11 11:48:16, Michal Hocko wrote:
Hi,
On Tue 04-01-11 17:15:45, Linus Torvalds wrote: [...]
We did have another revert to fix hopefullythe last "blank screen" regression on intel graphics.
It seems that there is still a regression for intel graphic cards backlight. One report is https://bugzilla.kernel.org/show_bug.cgi?id=22672. I can reproduce the problem easily by: xset dpms force standby; sleep 3s; xset dpms force on
backlight doesn't get up (there is really dark picture though which doesn't get brighter by function keys which work normally) after dpms on until I close and open lid.
The problem wasn't present in 2.6.36
I can confirm that this problem is not present if both patches from bko#22672 are applied on top of 2.6.37 kernel. I haven't tried both patches separately, but I can surely try it if it makes any sense.
The second patch is wrong in that it will prevent changing resolutions using the panel fitter.
I can confirm that with the second patch changing resolutions doesn't always go right.
$ xrandr Screen 0: minimum 320 x 200, current 1024 x 768, maximum 2048 x 2048 LVDS1 connected 1024x768+0+0 (normal left inverted right x axis y axis) 0mm x 0mm 1024x768 50.0*+ 85.0 75.0 70.1 60.0 832x624 74.6 800x600 85.1 72.2 75.0 60.3 56.2 640x480 85.0 72.8 75.0 59.9 720x400 85.0 640x400 85.1 640x350 85.1 VGA1 disconnected (normal left inverted right x axis y axis)
Going from xrandr -s 1, 2 or 3 back to 0 works, but not from 4, 5 or 6 to 0.
The second patch was really a stab in the dark, I'm happy it was in the right direction at least.
The first patch looks along the right lines, just misses the possibility that the backlight can be modified by other means.
I'm not sure about that. All it does is avoiding setting backlight_level to 0. If it's modified some other way and intel_panel_get_backlight() returns 0, backlight_level is just never set and will stay zero. If there is a way to switch between ways to modify the backlight then I can see how this may not always work, because then backlight_level is stuck on the last non-zero value before it was switched to intel_panel_set_backlight(0) and the different method.
Alex reported a slightly dimmer display after resume, so I wonder what I missed in my patch. Larry's problem was probably fixed with just the first patch, but then I wonder why he reported that it didn't work first. Maybe he meant the setpci thing, or made a mistake.
So hopefully, you just need the first patch. -Chris
Yeah, the second patch is a bit of a desperate attempt because Larry reported that it didn't fix his problem.
About your patch, you still do:
+void intel_panel_setup_backlight(struct drm_device *dev) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + + dev_priv->backlight_level = intel_panel_get_max_backlight(dev); + dev_priv->backlight_enabled = dev_priv->backlight_level != 0; +}
While my patch changes that to:
+ u32 level;
- if (dev_priv->backlight_level == 0) - dev_priv->backlight_level = intel_panel_get_max_backlight(dev); + if (dev_priv->backlight_level == 0) { + level = intel_panel_get_backlight(dev); + if (level == 0) + level = intel_panel_get_max_backlight(dev); + dev_priv->backlight_level = level; + }
Your patch uses intel_panel_get_max_backlight() to check if the backlight is enabled. Is this always correct, or may it cause bugs in the future?
Anyway, my main concern with unconditionally setting the backlight level to the maximum is that any stored brightness level (by the BIOS or whatever) may be lost, and that the screen is set to maximum brightness at each boot. This is certainly the behaviour I've seen with an unpatched kernel. So I propose to do what my patch does and set it to intel_panel_get_backlight(dev) if that returns non-zero. Something like this:
+void intel_panel_setup_backlight(struct drm_device *dev) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + u32 max, level; + + max = intel_panel_get_max_backlight(dev); + if (max) { + dev_priv->backlight_enabled = 1; + level = intel_panel_get_backlight(dev); + if (!level) + level = max; + dev_priv->backlight_level = level; + } +}
While I'm glad this problem is being fixed upstream, it would be nice to get some credit for finding the source of the problem.
Greetings,
Indan
On Wed, 12 Jan 2011 01:35:49 +0100 (CET), "Indan Zupancic" indan@nul.nu wrote:
Yeah, the second patch is a bit of a desperate attempt because Larry reported that it didn't fix his problem.
About your patch, you still do:
+void intel_panel_setup_backlight(struct drm_device *dev) +{
- struct drm_i915_private *dev_priv = dev->dev_private;
- dev_priv->backlight_level = intel_panel_get_max_backlight(dev);
- dev_priv->backlight_enabled = dev_priv->backlight_level != 0;
+}
While my patch changes that to:
- u32 level;
- if (dev_priv->backlight_level == 0)
dev_priv->backlight_level = intel_panel_get_max_backlight(dev);
- if (dev_priv->backlight_level == 0) {
level = intel_panel_get_backlight(dev);if (level == 0)level = intel_panel_get_max_backlight(dev);dev_priv->backlight_level = level;- }
Your patch uses intel_panel_get_max_backlight() to check if the backlight is enabled. Is this always correct, or may it cause bugs in the future?
That was a typo, cut'n'pasting the line from above.
Anyway, my main concern with unconditionally setting the backlight level to the maximum is that any stored brightness level (by the BIOS or whatever) may be lost, and that the screen is set to maximum brightness at each boot. This is certainly the behaviour I've seen with an unpatched kernel. So I propose to do what my patch does and set it to intel_panel_get_backlight(dev) if that returns non-zero. Something like this:
Sure, s/intel_panel_get_max_backlight/intel_panel_get_backlight/ and we get the behaviour we both want - preserving the current backlight unless none is set.
While I'm glad this problem is being fixed upstream, it would be nice to get some credit for finding the source of the problem.
Sorry. You found the bug but I felt your rationale was off. However, I was amiss in not giving you the credit you fully deserved. -Chris
commit 9c1c388a53e5df8819e898106a64e34d0994a5d4 Author: Indan Zupancic indan@nul.nu Date: Wed Jan 12 11:59:19 2011 +0000
drm/i915/panel: The backlight is enabled if the current value is non-zero
... and not if the maximum is non-zero. This fixes the typo introduced in 47356eb6728501452 and preserves the backlight value from boot.
[ickle: My thanks also to Indan Zupancic for diagnosing the original regression and suggesting the appropriate fix.] Signed-off-by: Chris Wilson chris@chris-wilson.co.uk Cc: stable@kernel.org # after 47356eb6728501452
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_pan index e00d200..27c79c7 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -278,6 +278,6 @@ void intel_panel_setup_backlight(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private;
- dev_priv->backlight_level = intel_panel_get_max_backlight(dev); + dev_priv->backlight_level = intel_panel_max_backlight(dev); dev_priv->backlight_enabled = dev_priv->backlight_level != 0; }
On Wed, 12 Jan 2011 12:07:23 +0000, Chris Wilson chris@chris-wilson.co.uk wrote:
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_pan index e00d200..27c79c7 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -278,6 +278,6 @@ void intel_panel_setup_backlight(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private;
dev_priv->backlight_level = intel_panel_get_max_backlight(dev);
dev_priv->backlight_level = intel_panel_max_backlight(dev); dev_priv->backlight_enabled = dev_priv->backlight_level != 0;
dev_priv->backlight_level = intel_panel_get_backlight(dev); -ETIRED
On Wed, January 12, 2011 13:07, Chris Wilson wrote:
Sure, s/intel_panel_get_max_backlight/intel_panel_get_backlight/ and we get the behaviour we both want - preserving the current backlight unless none is set.
Indeed, I hadn't noticed that shortcut. That's a lot nicer than my ifdefery.
While I'm glad this problem is being fixed upstream, it would be nice to get some credit for finding the source of the problem.
Sorry. You found the bug but I felt your rationale was off. However, I was amiss in not giving you the credit you fully deserved.
Thank you very much!
The rationale was that intel_panel_set_backlight(0) was somehow called twice, and that the current code unconditionally stored the old backlight, and thus losing the original brightness level. This is exactly what happened. My fix was to prevent backlight_level from being overwritten by zero. Your fix was more structural and properly fixed backlight enabled/disabled state. In the end both have the same effect and solve the bug. Perhaps I was unclear in the bug description.
Anyhow, it's a pleasure working with you. I'll try to not bother you too much, you got enough on your plate as it is. I'll leave you alone for a while after you looked into my fix for bug 23472, after that all my Intel graphics are pretty much solved. :-)
Take care,
Indan
dri-devel@lists.freedesktop.org
-
Alex Riesen -
Chris Wilson -
Indan Zupancic -
Linus Torvalds -
Michal Hocko -
Pavel Machek