[PATCH] drm/i915/panel: Alwyas record the backlight level again (but cleverly)

[PATCH] drm/i915/panel: Alwyas record the backlight level again (but cleverly)

[Takashi Iwai]

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

Re: [PATCH] drm/i915/panel: Alwyas record the backlight level again (but cleverly)

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 1570 bytes --]

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

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 827 bytes --]

Re: [PATCH] drm/i915/panel: Alwyas record the backlight level again (but cleverly)

[Takashi Iwai]

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

Re: [PATCH] drm/i915/panel: Alwyas record the backlight level again (but cleverly)

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 368 bytes --]

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.

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 827 bytes --]

Re: [PATCH] drm/i915/panel: Alwyas record the backlight level again (but cleverly)

[Takashi Iwai]

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