The commit 47356eb67285014527a5ab87543ba1fae3d1e10a introduced a mechanism to record the backlight level only at disabling time, but it also introduced a regression. Since intel_lvds_enable() may be called without disabling (e.g. intel_lvds_commit() calls it unconditionally), the backlight gets back to the last recorded value. For example, this happens when you dim the backlight, close the lid and open the lid, then the backlight suddenly goes to the brightest.
This patch fixes the bug by recording the backlight level always when changed but only when dev_priv->backlight_enabled is set. In this way, the bogus value for disabling backlight can be skipped.
Signed-off-by: Takashi Iwai tiwai@suse.de ---
Feel free to add Cc to stable kernel, as this is a regression fix. I've tested only a few machines here so more tests would be appreciated, because the backlight issue is SOOO sensitive :)
drivers/gpu/drm/i915/intel_panel.c | 11 +++++------ 1 files changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index a9e0c7b..269d526 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -233,6 +233,9 @@ void intel_panel_set_backlight(struct drm_device *dev, u32 level)
DRM_DEBUG_DRIVER("set backlight PWM = %d\n", level);
+ if (dev_priv->backlight_enabled) + dev_priv->backlight_level = level; + if (HAS_PCH_SPLIT(dev)) return intel_pch_panel_set_backlight(dev, level);
@@ -258,11 +261,7 @@ void intel_panel_disable_backlight(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private;
- if (dev_priv->backlight_enabled) { - dev_priv->backlight_level = intel_panel_get_backlight(dev); - dev_priv->backlight_enabled = false; - } - + dev_priv->backlight_enabled = false; intel_panel_set_backlight(dev, 0); }
@@ -273,8 +272,8 @@ void intel_panel_enable_backlight(struct drm_device *dev) if (dev_priv->backlight_level == 0) dev_priv->backlight_level = intel_panel_get_max_backlight(dev);
- intel_panel_set_backlight(dev, dev_priv->backlight_level); dev_priv->backlight_enabled = true; + intel_panel_set_backlight(dev, dev_priv->backlight_level); }
static void intel_panel_init_backlight(struct drm_device *dev)
On Thu, 13 Oct 2011 10:57:35 +0200, Takashi Iwai tiwai@suse.de wrote:
This patch fixes the bug by recording the backlight level always when changed but only when dev_priv->backlight_enabled is set. In this way, the bogus value for disabling backlight can be skipped.
I think this is better than what we have, but I'm not sure it's quite what we want -- the backlight level will only be remembered when it is enabled, so requested changes that happen while the backlight is off will have no effect. And, requested changes while the panel is disabled will still go through to the hardware, which can cause problems with panel power sequencing.
I think intel_panel_set_backlight should always record the level passed in, but that intel_panel_disable_backlight and intel_panel_enable_backlight should use a lower-level function, shared with intel_panel_set_backlight that doesn't record the value:
intel_panel_actually_set_backlight(dev, level) { <internals of current intel_panel_set_backlight> }
intel_panel_set_backlight(dev, level) { dev_priv->backlight_level = level; if (dev_priv->backlight_enabled) intel_panel_actually_set_backlight(dev, level); }
intel_panel_enable_backlight(dev) { dev_priv->backlight_enabled = true; intel_panel_actually_set_backlight(dev, dev_priv->backlight_level); }
intel_panel_disable_backlight(dev) { dev_priv->backlight_enabled = false; intel_panel_actually_set_backlight(dev, 0); }
At Thu, 13 Oct 2011 09:40:29 -0700, Keith Packard wrote:
On Thu, 13 Oct 2011 10:57:35 +0200, Takashi Iwai tiwai@suse.de wrote:
This patch fixes the bug by recording the backlight level always when changed but only when dev_priv->backlight_enabled is set. In this way, the bogus value for disabling backlight can be skipped.
I think this is better than what we have, but I'm not sure it's quite what we want -- the backlight level will only be remembered when it is enabled, so requested changes that happen while the backlight is off will have no effect. And, requested changes while the panel is disabled will still go through to the hardware, which can cause problems with panel power sequencing.
I think intel_panel_set_backlight should always record the level passed in, but that intel_panel_disable_backlight and intel_panel_enable_backlight should use a lower-level function, shared with intel_panel_set_backlight that doesn't record the value:
intel_panel_actually_set_backlight(dev, level) { <internals of current intel_panel_set_backlight> }
intel_panel_set_backlight(dev, level) { dev_priv->backlight_level = level; if (dev_priv->backlight_enabled) intel_panel_actually_set_backlight(dev, level); }
intel_panel_enable_backlight(dev) { dev_priv->backlight_enabled = true; intel_panel_actually_set_backlight(dev, dev_priv->backlight_level); }
intel_panel_disable_backlight(dev) { dev_priv->backlight_enabled = false; intel_panel_actually_set_backlight(dev, 0); }
Yes, this looks more understandable, indeed. Would you patch it by yourself or should I refresh the patch? In either way, I'll test tomorrow, as I'm already at home without a test machine.
thanks,
Takashi
On Thu, 13 Oct 2011 20:05:49 +0200, Takashi Iwai tiwai@suse.de wrote:
Yes, this looks more understandable, indeed. Would you patch it by yourself or should I refresh the patch? In either way, I'll test tomorrow, as I'm already at home without a test machine.
I don't have time before Monday to look at this further.
At Thu, 13 Oct 2011 12:28:07 -0700, Keith Packard wrote:
On Thu, 13 Oct 2011 20:05:49 +0200, Takashi Iwai tiwai@suse.de wrote:
Yes, this looks more understandable, indeed. Would you patch it by yourself or should I refresh the patch? In either way, I'll test tomorrow, as I'm already at home without a test machine.
I don't have time before Monday to look at this further.
OK, I'll refresh and test the patch tomorrow.
Takashi
dri-devel@lists.freedesktop.org