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

On Thu, March 10, 2011 08:49, Takashi Iwai wrote:

At Thu, 10 Mar 2011 06:50:09 +0100 (CET), Indan Zupancic wrote:

...

Hello,

On Fri, March 4, 2011 19:47, Linus Torvalds wrote:

...

Alex, can you confirm that the revert of 951f3512dba5 plus the one-liner patch from Takashi that Indan quoted also works for you?

          Linus

On Thu, Mar 3, 2011 at 10:53 PM, Indan Zupancic indan@nul.nu wrote:

...

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.

I found another backlight bug:

When suspending intel_panel_disable_backlight() is never called, but intel_panel_enable_backlight() is called at resume. With the effect that if the brightness was ever changed after screen blanking, the wrong brightness gets restored.

This explains the weird behaviour I've seen. I didn't see it with combination mode, because then the brightness is always the same (zero or the maximum, the BIOS only uses LBPC on my system.) I'll send a patch in a moment.

Alternative for reverting the combination mode removal (I can also redo the patch against the revert and Takashi's patch, if that's preferred):

--

drm/i915: Do handle backlight combination mode specially

Add back the combination mode check, but with slightly cleaner code and the weirdness removed: No val >>= 1, but also no val &= ~1. The old code probably confused bit 0 with BLM_LEGACY_MODE, which is bit 16. The other change is clearer calculations: Just check for zero level explicitly instead of avoiding the divide-by-zero.

Signed-off-by: Indan Zupancic indan@nul.nu

---

diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index d860abe..b05631a 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -30,6 +30,10 @@

#include "intel_drv.h"

+#define PCI_LBPC 0xf4 /* legacy/combination backlight modes */ +#define BLM_COMBINATION_MODE (1 << 30) +#define BLM_LEGACY_MODE (1 << 16)

void intel_fixed_panel_mode(struct drm_display_mode *fixed_mode, struct drm_display_mode *adjusted_mode) @@ -110,6 +114,22 @@ done: dev_priv->pch_pf_size = (width << 16) | height; }

+/*

• • What about gen 3? If there are no gen 3 systems with ASLE,
• • then it doesn't matter, as we don't need to change the
• • brightness. But then the gen 2 check can be removed too.
• */

+static int is_backlight_combination_mode(struct drm_device *dev) +{

• struct drm_i915_private *dev_priv = dev->dev_private;
• if (INTEL_INFO(dev)->gen >= 4)

• return I915_READ(BLC_PWM_CTL2) & BLM_COMBINATION_MODE;
  

• if (IS_GEN2(dev))

• return I915_READ(BLC_PWM_CTL) & BLM_LEGACY_MODE;
  

• return 0;

+}

static u32 i915_read_blc_pwm_ctl(struct drm_i915_private *dev_priv) { u32 val; @@ -163,9 +183,12 @@ u32 intel_panel_get_max_backlight(struct drm_device *dev) max >>= 17; } else { max >>= 16;

• 	/* Ignore BLM_LEGACY_MODE bit */
  if (INTEL_INFO(dev)->gen < 4)
  	max &= ~1;
  

  }

• if (is_backlight_combination_mode(dev))
  

• 	max *= 0xff;
  

  }

  DRM_DEBUG_DRIVER("max backlight PWM = %d\n", max);

@@ -183,6 +206,12 @@ u32 intel_panel_get_backlight(struct drm_device *dev) val = I915_READ(BLC_PWM_CTL) & BACKLIGHT_DUTY_CYCLE_MASK; if (IS_PINEVIEW(dev)) val >>= 1;

• if (is_backlight_combination_mode(dev)){
  

• 	u8 lbpc;
  

• 	pci_read_config_byte(dev->pdev, PCI_LBPC, &lbpc);
  

• 	val *= lbpc;
  

• }
  

  }

  DRM_DEBUG_DRIVER("get backlight PWM = %d\n", val);

@@ -205,6 +234,15 @@ void intel_panel_set_backlight(struct drm_device *dev, u32 level)

if (HAS_PCH_SPLIT(dev)) return intel_pch_panel_set_backlight(dev, level);

• if (level && is_backlight_combination_mode(dev)){

• u32 max = intel_panel_get_max_backlight(dev);
  

• u8 lpbc;
  

• lpbc = level * 0xff / max;
  

• level /= lpbc;
  

Hmm, I don't think this calculation is correct. This would result in level of opregion over its limit. For example, assume the level max = 100, so total max = 25500. Passing level=150 here will be:

lbpc = 150 * 0xff / 25500 = 1.5 = 1 level = 150 / 1 = 150, which is over limit.

More worse, lbpc can be zero when level is below 100 in the case above...

Yes, you're right. It seems that any simplification I try to do creates a new bug.

Do you have any bright idea why the old code did val &= ~1; too? It seems obvious it's related to val >>= 1, but...

Thanks,

Indan