[Intel-gfx] [PATCH] 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-and-tested-by: Daniel Mack Cc: Signed-off-by: Takashi Iwai --- 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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Fix invalid backpanel values for GEN3 or older chips
Hello! I know this is an old thread, but i am having this problem on my macbook air. Ideas for a quick fix? Thanks, James ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Fix invalid backpanel values for?GEN3 or older chips
On Thu, Apr 12, 2012 at 01:59:07AM +, James wrote: > Hello! I know this is an old thread, but i am having this problem on my > macbook > air. Ideas for a quick fix? Upgrade to kernel 3.2 or newer, that has the patch. -Daniel -- Daniel Vetter Mail: dan...@ffwll.ch Mobile: +41 (0)79 365 57 48 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Fix invalid backpanel values for GEN3 or older chips
On Wed, 16 Nov 2011 18:14:55 +0100, Takashi Iwai wrote: > 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. I'm looking at the mentioned patch and I don't see how that managed to correctly handle bit 0; is this the patch that managed to break this? -- keith.pack...@intel.com pgpNfnYzlHW6a.pgp Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Fix invalid backpanel values for GEN3 or older chips
At Wed, 16 Nov 2011 22:15:41 -0800, Keith Packard wrote: > > On Wed, 16 Nov 2011 18:14:55 +0100, Takashi Iwai wrote: > > > 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. > > I'm looking at the mentioned patch and I don't see how that managed to > correctly handle bit 0; is this the patch that managed to break this? Hrm, then I must have been confused. The gen < 4 check (formerly !IS_I965G()) was firstly introduced in this refactoring. I thought this was done before, but apparently not. Looking more carefully again, actually the IS_PINEVIEW() part (former IS_GND()) was introduced in the commit [078a033f: drm/i915: fix opregion backlight chip detect and range]. Thus the same bug must have been present even in the earlier version, but didn't come up by some reason. So, I'll have to correct the patch commit log. Sorry for that. Now, a question is whether the condition (gen < 4) is really correct. I *guess* the original code in the refactoring was intended to cover it: if (!IS_I965G(dev)) max &= ~1; But this rule wasn't applied to set_backlight(). The bit-0-clear above implies that it's a similar behavior like pineview. And indeed Daniel's machine with 915GM hits this problem. If the above rule should be applied to all places, it'd make more sense to handle both in the same way like pineview. Unfortunately I have no old machines to test this, and the only material to judge was Daniel's case. (Is Harald's machine also 915GM?) If it's only with 915GM, we'll just need to change IS_PINEVEW() to IS_PINEVIEW() || IS_I915GM(). This might be a safer option at this moment unless we check all cases or specs... thanks, Takashi ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Fix invalid backpanel values for GEN3 or older chips
On Thu, 17 Nov 2011 17:33:50 +0100, Takashi Iwai wrote: > If it's only with 915GM, we'll just need to change IS_PINEVEW() to > IS_PINEVIEW() || IS_I915GM(). This might be a safer option at this > moment unless we check all cases or specs... I read through the hardware docs yesterday and figured out what was going on. On all pre-gen4 hardware, the maximum backlight value has the lowest bit (of 16) hard-wired to zero. This means that the possible backlight duty cycle values are limited to 0 .. 0xfffe. On older hardware, this was managed by reporting this true range. This meant that the set_backlight path didn't need any special code; simply setting the values as provided should have worked just fine. On Pineview, this was changed (for reasons unknown) to use only 15 bits for backlight levels. The range of possible values is then 0 .. 0x7fff. In this case, values have to be shifted by one to convert between the advertised range of 0 .. 0x7fff and the hardware range of 0 .. 0xfffe. Exposing the range of 0 .. 0xfffe introduces a potential problem -- write a value of 0x and the hardware gets mightily confused, and probably ends up turning the backlight off. Using the range of 0 .. 0x7fff avoids this issue completely. Using the narrower range does limit the precision of the backlight level setting, but it seems like 32767 possible values is more than sufficient... Here's a patch which just uses the pineview version everywhere (and cleans that up at the same time). From e06789f688dc7ab1331f5f15ad3d60326b5139b4 Mon Sep 17 00:00:00 2001 From: Keith Packard Date: Fri, 18 Nov 2011 11:09:24 -0800 Subject: [PATCH] drm/i915: Treat pre-gen4 backlight duty cycle value consistently For i945 and earlier chips, the backlight frequency value had the low bit (of 16) fixed to zero. The Pineview code path handled this by just exposing the backlight range as 15 bits while other chips had the backlight range limited to 0 .. 0xfffe. This patch makes everyone take the pineview code path, providing 15 bits of backlight duty cycle range which seems more than sufficient to me. Signed-off-by: Keith Packard --- drivers/gpu/drm/i915/intel_panel.c | 16 +--- 1 files changed, 5 insertions(+), 11 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index 21f60b7..04d79fd 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -178,13 +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 { + else max >>= 16; - if (INTEL_INFO(dev)->gen < 4) - max &= ~1; - } if (is_backlight_combination_mode(dev)) max *= 0xff; @@ -203,13 +200,12 @@ 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)) { u8 lbpc; - val &= ~1; pci_read_config_byte(dev->pdev, PCI_LBPC, &lbpc); val *= lbpc; } @@ -246,11 +242,9 @@ static void intel_panel_actually_set_backlight(struct drm_device *dev, u32 level } tmp = I915_READ(BLC_PWM_CTL); - if (IS_PINEVIEW(dev)) { - tmp &= ~(BACKLIGHT_DUTY_CYCLE_MASK - 1); + if (INTEL_INFO(dev)->gen < 4) level <<= 1; - } else - tmp &= ~BACKLIGHT_DUTY_CYCLE_MASK; + tmp &= ~BACKLIGHT_DUTY_CYCLE_MASK; I915_WRITE(BLC_PWM_CTL, tmp | level); } -- 1.7.7.3 -- keith.pack...@intel.com pgphIVM6LQ6Q1.pgp Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Fix invalid backpanel values for GEN3 or older chips
At Fri, 18 Nov 2011 11:25:48 -0800, Keith Packard wrote: > > On Thu, 17 Nov 2011 17:33:50 +0100, Takashi Iwai wrote: > > > If it's only with 915GM, we'll just need to change IS_PINEVEW() to > > IS_PINEVIEW() || IS_I915GM(). This might be a safer option at this > > moment unless we check all cases or specs... > > I read through the hardware docs yesterday and figured out what was > going on. On all pre-gen4 hardware, the maximum backlight value has the > lowest bit (of 16) hard-wired to zero. This means that the possible > backlight duty cycle values are limited to 0 .. 0xfffe. > > On older hardware, this was managed by reporting this true range. This > meant that the set_backlight path didn't need any special code; simply > setting the values as provided should have worked just fine. > > On Pineview, this was changed (for reasons unknown) to use only 15 bits > for backlight levels. The range of possible values is then > 0 .. 0x7fff. In this case, values have to be shifted by one to convert > between the advertised range of 0 .. 0x7fff and the hardware range of > 0 .. 0xfffe. > > Exposing the range of 0 .. 0xfffe introduces a potential problem -- > write a value of 0x and the hardware gets mightily confused, > and probably ends up turning the backlight off. Using the range of > 0 .. 0x7fff avoids this issue completely. > > Using the narrower range does limit the precision of the backlight level > setting, but it seems like 32767 possible values is more than sufficient... > > Here's a patch which just uses the pineview version everywhere (and > cleans that up at the same time). Thanks! It's pretty similar with my patch in the end, so in case needed: Reviewed-by: Takashi Iwai Maybe it'd be better to mention that actually setting bit-0 caused a blank screen on some machines. Takashi > From e06789f688dc7ab1331f5f15ad3d60326b5139b4 Mon Sep 17 00:00:00 2001 > From: Keith Packard > Date: Fri, 18 Nov 2011 11:09:24 -0800 > Subject: [PATCH] drm/i915: Treat pre-gen4 backlight duty cycle value > consistently > > For i945 and earlier chips, the backlight frequency value had the low > bit (of 16) fixed to zero. The Pineview code path handled this by just > exposing the backlight range as 15 bits while other chips had the > backlight range limited to 0 .. 0xfffe. > > This patch makes everyone take the pineview code path, providing 15 > bits of backlight duty cycle range which seems more than sufficient to me. > > Signed-off-by: Keith Packard > --- > drivers/gpu/drm/i915/intel_panel.c | 16 +--- > 1 files changed, 5 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_panel.c > b/drivers/gpu/drm/i915/intel_panel.c > index 21f60b7..04d79fd 100644 > --- a/drivers/gpu/drm/i915/intel_panel.c > +++ b/drivers/gpu/drm/i915/intel_panel.c > @@ -178,13 +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 { > + else > max >>= 16; > - if (INTEL_INFO(dev)->gen < 4) > - max &= ~1; > - } > > if (is_backlight_combination_mode(dev)) > max *= 0xff; > @@ -203,13 +200,12 @@ 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)) { > u8 lbpc; > > - val &= ~1; > pci_read_config_byte(dev->pdev, PCI_LBPC, &lbpc); > val *= lbpc; > } > @@ -246,11 +242,9 @@ static void intel_panel_actually_set_backlight(struct > drm_device *dev, u32 level > } > > tmp = I915_READ(BLC_PWM_CTL); > - if (IS_PINEVIEW(dev)) { > - tmp &= ~(BACKLIGHT_DUTY_CYCLE_MASK - 1); > + if (INTEL_INFO(dev)->gen < 4) > level <<= 1; > - } else > - tmp &= ~BACKLIGHT_DUTY_CYCLE_MASK; > + tmp &= ~BACKLIGHT_DUTY_CYCLE_MASK; > I915_WRITE(BLC_PWM_CTL, tmp | level); > } > > -- > 1.7.7.3 > > > > -- > keith.pack...@intel.com ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Fix invalid backpanel values for GEN3 or older chips
On Sat, 19 Nov 2011 11:05:05 +0100, Takashi Iwai wrote: > Maybe it'd be better to mention that actually setting bit-0 caused a > blank screen on some machines. Was that caused by *just* setting bit zero? Or was it caused by setting the duty cycle to 0x, in which case it would be larger than the maximum value? I'll clean up the commit log message with your answer and then push this out. -- keith.pack...@intel.com pgpHsSu2riBkU.pgp Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Fix invalid backpanel values for GEN3 or older chips
At Sat, 19 Nov 2011 10:34:12 -0800, Keith Packard wrote: > > [1 ] > On Sat, 19 Nov 2011 11:05:05 +0100, Takashi Iwai wrote: > > > Maybe it'd be better to mention that actually setting bit-0 caused a > > blank screen on some machines. > > Was that caused by *just* setting bit zero? Or was it caused by setting > the duty cycle to 0x, in which case it would be larger than the > maximum value? > > I'll clean up the commit log message with your answer and then push this out. According to Daniels' original post: On 11/04/2011 03:36 PM, Daniel Mack wrote: > I'm facing a bug on a Samsung X20 notebook which features an i915 > chipset (output of 'lspci -v' attached). > > The effect is that setting the backlight to odd values causes the value > to be misinterpreted. Harald Hoyer (cc:) had the same thing on a Netbook > (I don't recall which model it was). > > So this will turn the backlight to full brightness: > > # cat /sys/class/backlight/intel_backlight/max_brightness > 29750 > # echo 29750 > /sys/class/backlight/intel_backlight/brightness > > However, writing 29749 will turn the display backlight off, and 29748 > appears to be the next valid lower value. So, writing bit-0 caused a problem, as it seems. Takashi ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Fix invalid backpanel values for GEN3 or older chips
On Sun, 20 Nov 2011 12:22:50 +0100, Takashi Iwai wrote: > So, writing bit-0 caused a problem, as it seems. Thanks. I've noted that in the patch message. -- keith.pack...@intel.com pgpCXCnhRKCYp.pgp Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Fix invalid backpanel values for GEN3 or older chips
On Tue, 22 Nov 2011 17:40:25 +0100, Daniel Mack wrote: > Just curious - is this patch queued somewhere for mainline yet? I'm waiting for airlied to pull drm-fixes-intel before I push more stuff there. -- keith.pack...@intel.com pgpn9lOMl3zqH.pgp Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx