Re: Strange effect with i915 backlight controller

[Takashi Iwai <tiwai@suse.de>]

[Added Chris to Cc]

At Sun, 13 Nov 2011 17:24:09 +0100,
Daniel Mack wrote:
> 
> Hi Takashi,
> 
> On 11/10/2011 04:39 PM, Takashi Iwai wrote:
> > At Thu, 10 Nov 2011 16:11:29 +0100,
> > Daniel Mack wrote:
> >>
> >> On 11/08/2011 01:57 AM, Daniel Mack wrote:
> >>> Didn't get any response yet, hence copying LKML for a broader audience.
> >>
> >> Nobody, really?
> >>
> >> This is a rather annoying regression, as touching the brightness keys
> >> appearantly switches off the whole machine. I'm sure this is trivial to
> >> fix, I just don't have the insight of this driver and the chipset.
> > 
> > I vaguely remember that the bit 0 is invalid on some old chips.
> > Maybe 915GM is one of them, as it's gen3?  If so, the patch like below
> > may work.
> 
> Thank you for looking into this.
> 
> > ---
> > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> > index 499d4c0..be952d1 100644
> > --- a/drivers/gpu/drm/i915/intel_panel.c
> > +++ b/drivers/gpu/drm/i915/intel_panel.c
> > @@ -249,8 +249,11 @@ static void intel_panel_actually_set_backlight(struct drm_device *dev, u32 level
> >  	if (IS_PINEVIEW(dev)) {
> >  		tmp &= ~(BACKLIGHT_DUTY_CYCLE_MASK - 1);
> >  		level <<= 1;
> > -	} else
> > +	} else {
> >  		tmp &= ~BACKLIGHT_DUTY_CYCLE_MASK;
> > +		if (INTEL_INFO(dev)->gen < 4)
> > +			tmp &= ~1;
> > +	}
> >  	I915_WRITE(BLC_PWM_CTL, tmp | level);
> >  }
> >  
> 
> This seems to be the right intention, but the value you want to modify
> under this condition is 'level', not 'tmp'.

Ah, of course.  Sorry for that.

> With this amendment of your
> patch, things work perfectly fine here.

OK, then perhaps a better fix is to change the check to be equivalent
with pineview, as you mentioned in the original post.  The handling of
bit 0 for old chips was lost during the refactoring of backlight code
since 2.6.37.

Does the patch below work for you?

The only concern by this fix is that it changes the max value.  If
apps expect some certain (e.g. recorded) value, it may screw up.  But
I don't expect this would happen with sane apps.


thanks,

Takashi

===
From: Takashi Iwai <tiwai@suse.de>
Subject: drm/i915: Fix invalid backpanel values for GEN3 or older chips

While refactoring of backlight control code in commit [a95735569:
drm/i915: Refactor panel backlight controls], the handling of the bit
0 of duty-cycle was gone except for pineview.  This resulted in invalid
register values for old chips like 915GM.  When the bit 0 is set, the
backlight is turned off suddenly.

This patch changes the bit-0 check by replacing with the condition of
gen < 4 (pineview is included in this condition, too).

Reported-by: Daniel Mack <zonque@gmail.com>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/gpu/drm/i915/intel_panel.c |    8 +++-----
 1 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index 499d4c0..737d00f 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -178,12 +178,10 @@ u32 intel_panel_get_max_backlight(struct drm_device *dev)
 	if (HAS_PCH_SPLIT(dev)) {
 		max >>= 16;
 	} else {
-		if (IS_PINEVIEW(dev)) {
+		if (INTEL_INFO(dev)->gen < 4) {
 			max >>= 17;
 		} else {
 			max >>= 16;
-			if (INTEL_INFO(dev)->gen < 4)
-				max &= ~1;
 		}
 
 		if (is_backlight_combination_mode(dev))
@@ -203,7 +201,7 @@ u32 intel_panel_get_backlight(struct drm_device *dev)
 		val = I915_READ(BLC_PWM_CPU_CTL) & BACKLIGHT_DUTY_CYCLE_MASK;
 	} else {
 		val = I915_READ(BLC_PWM_CTL) & BACKLIGHT_DUTY_CYCLE_MASK;
-		if (IS_PINEVIEW(dev))
+		if (INTEL_INFO(dev)->gen < 4)
 			val >>= 1;
 
 		if (is_backlight_combination_mode(dev)) {
@@ -246,7 +244,7 @@ static void intel_panel_actually_set_backlight(struct drm_device *dev, u32 level
 	}
 
 	tmp = I915_READ(BLC_PWM_CTL);
-	if (IS_PINEVIEW(dev)) {
+	if (INTEL_INFO(dev)->gen < 4) {
 		tmp &= ~(BACKLIGHT_DUTY_CYCLE_MASK - 1);
 		level <<= 1;
 	} else
-- 
1.7.7