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

[Takashi Iwai <tiwai@suse.de>]

At Thu, 10 Mar 2011 09:45:18 +0100 (CET),
Indan Zupancic wrote:
> 
> 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...

I guess it's for the case GEN < 4.  But, no certain idea.

In my patch, I left it since this is relatively harmless, even if it's
not correct.


Takashi