[PATCH] fix backlight brightness on intel LVDS panel after reopening lid
At Thu, 10 Mar 2011 11:06:28 +0100 (CET),
Indan Zupancic wrote:
>
> On Thu, March 10, 2011 09:25, Takashi Iwai wrote:
> > At Thu, 10 Mar 2011 08:49:37 +0100,
> > 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
> >> > > 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
> >> >
> >> > ---
> >> >
> >> > 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,
> >> > );
> >> > +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
[PATCH] fix backlight brightness on intel LVDS panel after reopening lid
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 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
> >>
> >> ---
> >>
> >> 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, );
> >> + 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
[PATCH] fix backlight brightness on intel LVDS panel after reopening lid
On Thu, March 10, 2011 09:25, Takashi Iwai wrote:
> At Thu, 10 Mar 2011 08:49:37 +0100,
> 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 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
>> >
>> > ---
>> >
>> > 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, );
>> > + 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:
>>
>>
[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 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
>>
>> ---
>>
>> 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, );
>> +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
[PATCH] fix backlight brightness on intel LVDS panel after reopening lid
At Thu, 10 Mar 2011 08:49:37 +0100,
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 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
> >
> > ---
> >
> > 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, );
> > + 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
[PATCH] fix backlight brightness on intel LVDS panel after reopening lid
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 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
>
> ---
>
> 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, );
> + 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...
thanks,
Takashi
[PATCH] fix backlight brightness on intel LVDS panel after reopening lid
drm/i915: Fix DPMS and suspend interaction for intel_panel.c
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 at resume time.
Nothing guarantees that those calls will be balanced, so having
backlight_enabled makes no sense, as the real state can change
without the panel code noticing. So keep things as stateless as
possible.
Signed-off-by: Indan Zupancic
---
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 456f404..4a3d9ed 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -333,7 +333,6 @@ typedef struct drm_i915_private {
/* LVDS info */
int backlight_level; /* restore backlight to this value */
- bool backlight_enabled;
struct drm_display_mode *panel_fixed_mode;
struct drm_display_mode *lfp_lvds_vbt_mode; /* if any */
struct drm_display_mode *sdvo_lvds_vbt_mode; /* if any */
@@ -1220,10 +1219,6 @@ void i915_debugfs_cleanup(struct drm_minor *minor);
extern int i915_save_state(struct drm_device *dev);
extern int i915_restore_state(struct drm_device *dev);
-/* i915_suspend.c */
-extern int i915_save_state(struct drm_device *dev);
-extern int i915_restore_state(struct drm_device *dev);
-
/* intel_i2c.c */
extern int intel_setup_gmbus(struct drm_device *dev);
extern void intel_teardown_gmbus(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 49fb54f..1b5a32d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5924,8 +5924,6 @@ static void intel_setup_outputs(struct drm_device *dev)
encoder->base.possible_clones =
intel_encoder_clones(dev, encoder->clone_mask);
}
-
- intel_panel_setup_backlight(dev);
}
static void intel_user_framebuffer_destroy(struct drm_framebuffer *fb)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 2c43104..70e8b82 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -257,7 +257,6 @@ extern void intel_pch_panel_fitting(struct drm_device *dev,
extern u32 intel_panel_get_max_backlight(struct drm_device *dev);
extern u32 intel_panel_get_backlight(struct drm_device *dev);
extern void intel_panel_set_backlight(struct drm_device *dev, u32 level);
-extern void intel_panel_setup_backlight(struct drm_device *dev);
extern void intel_panel_enable_backlight(struct drm_device *dev);
extern void intel_panel_disable_backlight(struct drm_device *dev);
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
@@ -217,12 +255,11 @@ void intel_panel_set_backlight(struct drm_device *dev,
u32 level)
void intel_panel_disable_backlight(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev->dev_private;
+ u32 level = intel_panel_get_backlight(dev);
- if (dev_priv->backlight_enabled) {
- dev_priv->backlight_level = intel_panel_get_backlight(dev);
- dev_priv->backlight_enabled = false;
- }
-
+ if (level == 0)
+ return;
+ dev_priv->backlight_level = level;
intel_panel_set_backlight(dev, 0);
}
@@ -230,17 +267,9 @@ void intel_panel_enable_backlight(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev->dev_private;
+ if (intel_panel_get_backlight(dev))
+ return;
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;
-}
-
-void intel_panel_setup_backlight(struct drm_device *dev)
-{
- struct drm_i915_private *dev_priv = dev->dev_private;
-
- dev_priv->backlight_level = intel_panel_get_backlight(dev);
- dev_priv->backlight_enabled = dev_priv->backlight_level != 0;
}
[PATCH] fix backlight brightness on intel LVDS panel after reopening lid
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 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
---
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, );
+ 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;
+ pci_write_config_byte(dev->pdev, PCI_LBPC, lpbc);
+ }
tmp = I915_READ(BLC_PWM_CTL);
if (IS_PINEVIEW(dev)) {
tmp &= ~(BACKLIGHT_DUTY_CYCLE_MASK - 1);
Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid
At Thu, 10 Mar 2011 08:49:37 +0100,
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 in...@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 in...@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...
That is, Chris' original code in that portion was correct:
if (is_backlight_combination_mode(dev)){
u32 max = intel_panel_get_max_backlight(dev);
u8 lpbc;
lpbc = level * 0xfe / max
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 in...@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 in...@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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid
On Thu, March 10, 2011 09:25, Takashi Iwai wrote:
At Thu, 10 Mar 2011 08:49:37 +0100,
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 in...@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 in...@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...
That is, Chris' original code in that portion was correct:
if (is_backlight_combination_mode(dev)){
u32 max = intel_panel_get_max_backlight(dev);
u8 lpbc;
lpbc = level * 0xfe / max + 1;
Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid
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 in...@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 in...@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
Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid
At Thu, 10 Mar 2011 11:06:28 +0100 (CET),
Indan Zupancic wrote:
On Thu, March 10, 2011 09:25, Takashi Iwai wrote:
At Thu, 10 Mar 2011 08:49:37 +0100,
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 in...@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 in...@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
Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid
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 in...@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 in...@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;
+ pci_write_config_byte(dev-pdev, PCI_LBPC, lpbc);
+ }
tmp = I915_READ(BLC_PWM_CTL);
if (IS_PINEVIEW(dev)) {
tmp = ~(BACKLIGHT_DUTY_CYCLE_MASK - 1);
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] fix backlight brightness on intel LVDS panel after reopening lid
On Fri, Mar 4, 2011 at 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? I afraid mine is a different case, because backlight in this system works properly even with Indan's patch reverted. Sorry for the late reply... > On Thu, Mar 3, 2011 at 10:53 PM, Indan Zupancic 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. > > > Linus >
Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid
On Fri, Mar 4, 2011 at 19:47, Linus Torvalds torva...@linux-foundation.org wrote: Alex, can you confirm that the revert of 951f3512dba5 plus the one-liner patch from Takashi that Indan quoted also works for you? I afraid mine is a different case, because backlight in this system works properly even with Indan's patch reverted. Sorry for the late reply... On Thu, Mar 3, 2011 at 10:53 PM, Indan Zupancic in...@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. Linus ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] fix backlight brightness on intel LVDS panel after reopening lid
Indan Zupancic wrote: > What I don't understand is how BIOS makers could know about those bits. They have relationships with Intel since 30 years, ie. they get what they need in one form or other. //Peter
[PATCH] fix backlight brightness on intel LVDS panel after reopening lid
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 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. > For what it's worth, doing the above does prevent the regression for the ASLE case, but for me with gen 2 hardware the brightness isn't quite right after suspend/resume, while it is with my patch applied. So assuming that there are no gen 2 systems with ASLE out there, the best solution may be the remove the combination mode check for gen 2 hardware and leave it for gen >=4. Would be nice if Jesse or Chris could confirm that there are no gen 2 ASLE systems out there though. I'm going camping, I'll send a patch next week. Greetings, Indan
[PATCH] fix backlight brightness on intel LVDS panel after reopening lid
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 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.
[PATCH] fix backlight brightness on intel LVDS panel after reopening lid
Hello, On Wed, February 23, 2011 02:09, Linus Torvalds wrote: > On Tue, Feb 22, 2011 at 2:31 PM, Tino Keitel wrote: >> >> I just tried 2.6.38-rc6 on my ThinkPad X61s without any other DRM >> related patches, and my backlight issue is gone. > > I applied Indan's fix in -rc6 (commit 951f3512dba5), since it had > several testers and seemed to simplify the code nicely too. Sadly, as so often in life, it's not correct. At this point I'm not sure if it's better to revert that patch and add a correct one, or to just fix it up. The end result is the same I suppose. I've also found more documentation, namely: ACPI_IGD_OpRegion_Spec.pdf, which has the ASLE stuff in intel_opregion.c, and VOL_1_graphics_core.pdf, which mentions LBPC (I was looking at 3 before). Apparently the undocumented stuff the old code did was correct. What I don't understand is how BIOS makers could know about those bits. The good side is that that big warning in my patch description is invalid, something else was going on: The BIOS used LBPC to set and restore brightness, while the driver only used BLC_PWM_CTL after my patch. All credits to Intel for making something simple as backlight control as stupid and complex as possible: - It has two registers to control brightness, sometimes one is used, sometimes the other, sometimes both, and it's unknown what the BIOS uses, and it's undefined what registers are restored by the BIOS after reboot/resume. - When using ACPI and ASLE, the kernel requests a brightness change via a standard ACPI method, which in turns lets the BIOS generate an ASLE interrupt, which is handled by the driver. The brightness to set is between 0 and 255, and the driver is supposed to store the current brightness in another register. That register stores the brightness in percentages, which is used by the BIOS to restore brightness. How it does that is undefined, so it can use either register. So the BIOS obviously knows how to change the brightness, and it's still seemed like a good idea to bother the driver with it. The ASLE interface is a mess. All in all, after my patch, systems using ASLE and a BIOS storing the brightness in LBPC stopped working. The reason it works without ASLE is because the brightness is never changed by the driver, the backlight is only enabled or disabled. I'd love to clean up the whole backlight mess, but it's too late in the RC cycle to do that. 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.
Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid
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 in...@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. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid
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 in...@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. For what it's worth, doing the above does prevent the regression for the ASLE case, but for me with gen 2 hardware the brightness isn't quite right after suspend/resume, while it is with my patch applied. So assuming that there are no gen 2 systems with ASLE out there, the best solution may be the remove the combination mode check for gen 2 hardware and leave it for gen =4. Would be nice if Jesse or Chris could confirm that there are no gen 2 ASLE systems out there though. I'm going camping, I'll send a patch next week. Greetings, Indan ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid
Indan Zupancic wrote: What I don't understand is how BIOS makers could know about those bits. They have relationships with Intel since 30 years, ie. they get what they need in one form or other. //Peter ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid
Hello, On Wed, February 23, 2011 02:09, Linus Torvalds wrote: On Tue, Feb 22, 2011 at 2:31 PM, Tino Keitel tino.kei...@tikei.de wrote: I just tried 2.6.38-rc6 on my ThinkPad X61s without any other DRM related patches, and my backlight issue is gone. I applied Indan's fix in -rc6 (commit 951f3512dba5), since it had several testers and seemed to simplify the code nicely too. Sadly, as so often in life, it's not correct. At this point I'm not sure if it's better to revert that patch and add a correct one, or to just fix it up. The end result is the same I suppose. I've also found more documentation, namely: ACPI_IGD_OpRegion_Spec.pdf, which has the ASLE stuff in intel_opregion.c, and VOL_1_graphics_core.pdf, which mentions LBPC (I was looking at 3 before). Apparently the undocumented stuff the old code did was correct. What I don't understand is how BIOS makers could know about those bits. The good side is that that big warning in my patch description is invalid, something else was going on: The BIOS used LBPC to set and restore brightness, while the driver only used BLC_PWM_CTL after my patch. All credits to Intel for making something simple as backlight control as stupid and complex as possible: - It has two registers to control brightness, sometimes one is used, sometimes the other, sometimes both, and it's unknown what the BIOS uses, and it's undefined what registers are restored by the BIOS after reboot/resume. - When using ACPI and ASLE, the kernel requests a brightness change via a standard ACPI method, which in turns lets the BIOS generate an ASLE interrupt, which is handled by the driver. The brightness to set is between 0 and 255, and the driver is supposed to store the current brightness in another register. That register stores the brightness in percentages, which is used by the BIOS to restore brightness. How it does that is undefined, so it can use either register. So the BIOS obviously knows how to change the brightness, and it's still seemed like a good idea to bother the driver with it. The ASLE interface is a mess. All in all, after my patch, systems using ASLE and a BIOS storing the brightness in LBPC stopped working. The reason it works without ASLE is because the brightness is never changed by the driver, the backlight is only enabled or disabled. I'd love to clean up the whole backlight mess, but it's too late in the RC cycle to do that. 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. From f6b8a45b9544072e6ddbb944a4c03a9ec8cbca3a Mon Sep 17 00:00:00 2001 From: Takashi Iwai ti...@suse.de Date: Mon, 21 Feb 2011 14:19:27 +0100 Subject: [PATCH] drm/i915: Fix calculation of backlight value in combined mode The commit a95735569312f2ab0c80425e2cd1e5cb0b4e1870 drm/i915: Refactor panel backlight controls causes a regression for GM45 that is using the combined mode for controlling the backlight brightness. The commit introduced a wrong bit shift for computing the current backlight level. Bugzilla: https://bugzilla.novell.com/show_bug.cgi?id=672946 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=34524 Signed-off-by: Takashi Iwai ti...@suse.de Cc: sta...@kernel.org --- drivers/gpu/drm/i915/intel_panel.c |1 - 1 file changed, 1 deletion(-) --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -176,7 +176,6 @@ val = ~1; pci_read_config_byte(dev-pdev, PCI_LBPC, lbpc); val *= lbpc; - val = 1; } } ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] fix backlight brightness on intel LVDS panel after reopening lid
On Tue, February 22, 2011 22:04, Jesse Barnes wrote: > On Sat, 19 Feb 2011 15:07:49 -0800 > Linus Torvalds wrote: > >> On Sat, Feb 19, 2011 at 4:26 AM, Alex Riesen wrote: >> > On Sat, Feb 19, 2011 at 13:11, Alex Riesen wrote: >> >>> Lastly, could you verify that my patch at >> >>> https://lkml.org/lkml/2011/2/16/447 >> fixes >> >>> it for you too? (Make sure you're at max brightness before rebooting.) >> >> >> >> I'll try it now. >> >> >> > >> > I can confirm that it does fix backlight in my case (Dell XPS 1330, >> > LVDS panel, GM965/GL960). >> > >> > Tested-by: Alex Riesen >> >> Guys, should I apply this, or will I get it through somebody's pull? > > I'm worried that removing combo mode will break some working setups, > but if it's seen a lot of testing and is ok, then I'm fine with it, as > it definitely simplifies things. This all seems new code added in 2.6.37, it wasn't there before. The working setups stopped working when that code was added. The only reason it may work for some gen 2 and gen 3 hardware is because of a random value of the max brightness (bit 16). The buggy code seems to be written in a haste without any testing done. It's so off from the official documentation that I suspect that the windows driver was used as reference, but its code was misinterpreted. I grepped the userspace driver source, and LBPC is defined there for 810, but not used anywhere either. This patch should be added to stable kernel 2.6.37.2, because it messes up the LPBC register, which some laptops store between boots. Quoting https://bugzilla.kernel.org/show_bug.cgi?id=23472#c57 - Checking bit 16 in BLC_PWM_CTL is wrong, it has no special meaning. - If LBPC == 0xff, it should be ignored and it's not in combination mode. (This is for gen 3). - Gen 2 documentation doesn't mention LBPC or combination mode at all. Gen 3 does, but doesn't tell what the register value is or how to use it, it just mentions it. - The calculations are rubbish, resulting in bogus LBPC values, and depending on how lucky you are, it writes different values for the registers after a restore. All this code is new and causes problems, while everything worked before just fine, when the driver didn't do anything special. So it seems a bit like voodoo programming, because nothing the driver did followed the official Intel documentation. Now, there may be real reasons for some of the code, but I propose we add them one at a time when people show up with problems without the weird code added. That way the reason for any weirdness is also documented. Greetings, Indan
[PATCH] fix backlight brightness on intel LVDS panel after reopening lid
On Tue, Feb 22, 2011 at 13:04:40 -0800, Jesse Barnes wrote: > On Sat, 19 Feb 2011 15:07:49 -0800 > Linus Torvalds wrote: > > > On Sat, Feb 19, 2011 at 4:26 AM, Alex Riesen wrote: > > > On Sat, Feb 19, 2011 at 13:11, Alex Riesen wrote: > > >>> Lastly, could you verify that my patch at > > >>> https://lkml.org/lkml/2011/2/16/447 fixes > > >>> it for you too? (Make sure you're at max brightness before rebooting.) > > >> > > >> I'll try it now. > > >> > > > > > > I can confirm that it does fix backlight in my case (Dell XPS 1330, > > > LVDS panel, GM965/GL960). > > > > > > Tested-by: Alex Riesen > > > > Guys, should I apply this, or will I get it through somebody's pull? > > I'm worried that removing combo mode will break some working setups, > but if it's seen a lot of testing and is ok, then I'm fine with it, as > it definitely simplifies things. Hi, I just tried 2.6.38-rc6 on my ThinkPad X61s without any other DRM related patches, and my backlight issue is gone. Regards, Tino
[PATCH] fix backlight brightness on intel LVDS panel after reopening lid
On Tue, Feb 22, 2011 at 2:31 PM, Tino Keitel wrote: > > I just tried 2.6.38-rc6 on my ThinkPad X61s without any other DRM > related patches, and my backlight issue is gone. I applied Indan's fix in -rc6 (commit 951f3512dba5), since it had several testers and seemed to simplify the code nicely too. Linus
[PATCH] fix backlight brightness on intel LVDS panel after reopening lid
On Sat, 19 Feb 2011 15:07:49 -0800 Linus Torvalds wrote: > On Sat, Feb 19, 2011 at 4:26 AM, Alex Riesen wrote: > > On Sat, Feb 19, 2011 at 13:11, Alex Riesen wrote: > >>> Lastly, could you verify that my patch at > >>> https://lkml.org/lkml/2011/2/16/447 fixes > >>> it for you too? (Make sure you're at max brightness before rebooting.) > >> > >> I'll try it now. > >> > > > > I can confirm that it does fix backlight in my case (Dell XPS 1330, > > LVDS panel, GM965/GL960). > > > > Tested-by: Alex Riesen > > Guys, should I apply this, or will I get it through somebody's pull? I'm worried that removing combo mode will break some working setups, but if it's seen a lot of testing and is ok, then I'm fine with it, as it definitely simplifies things. -- Jesse Barnes, Intel Open Source Technology Center
[PATCH] fix backlight brightness on intel LVDS panel after reopening lid
On Sat, Feb 19, 2011 at 4:26 AM, Alex Riesen wrote: > On Sat, Feb 19, 2011 at 13:11, Alex Riesen wrote: >>> Lastly, could you verify that my patch at >>> https://lkml.org/lkml/2011/2/16/447 fixes >>> it for you too? (Make sure you're at max brightness before rebooting.) >> >> I'll try it now. >> > > I can confirm that it does fix backlight in my case (Dell XPS 1330, > LVDS panel, GM965/GL960). > > Tested-by: Alex Riesen Guys, should I apply this, or will I get it through somebody's pull? Linus
[PATCH] fix backlight brightness on intel LVDS panel after reopening lid
On Sat, Feb 19, 2011 at 13:11, Alex Riesen wrote: >> Lastly, could you verify that my patch at >> https://lkml.org/lkml/2011/2/16/447 fixes >> it for you too? (Make sure you're at max brightness before rebooting.) > > I'll try it now. > I can confirm that it does fix backlight in my case (Dell XPS 1330, LVDS panel, GM965/GL960). Tested-by: Alex Riesen
[PATCH] fix backlight brightness on intel LVDS panel after reopening lid
Sorry for this late answer. I only get a very little time for this.
On Fri, Feb 18, 2011 at 05:57, Indan Zupancic wrote:
> On Thu, February 17, 2011 23:13, Tino Keitel wrote:
>> with kernel 2.6.37, the display brightness of my ThinkPad X61s was
>> always reduced after lid open, resume from suspend etc. ?With this
>> patch on top of 2.6.38-rc5, the problem is gone. ?Thanks.
>
> Tino, I think Alex's patch only hides the problem and doesn't properly solve
Could well be. I don't understand what the code is supposed to do.
The patch was created just be looking at diffs.
> the real bug. Can you confirm that this is the bit that fixes it for you?
>
> diff --git a/drivers/gpu/drm/i915/intel_panel.c
> b/drivers/gpu/drm/i915/intel_panel.c
> index c65992d..c4b1ca4 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -267,6 +235,9 @@ void intel_panel_enable_backlight(struct drm_device *dev)
> ?{
> ? ? ? ?struct drm_i915_private *dev_priv = dev->dev_private;
>
> + ? ? ? if (dev_priv->backlight_enabled)
> + ? ? ? ? ? ? ? return;
> +
> ? ? ? ?if (dev_priv->backlight_level == 0)
> ? ? ? ? ? ? ? ?dev_priv->backlight_level = intel_panel_get_max_backlight(dev);
>
> (Alex's patch edited by hand, offsets might be wrong.)
It is not enough, at least for me.
> The other bits either don't change the logic, or should be harmless, or are
> plain wrong, like setting the brightness to maximum at bootup.
I am not absolutely sure, but I don't think this is what happens on this laptop.
> Lastly, could you verify that my patch at https://lkml.org/lkml/2011/2/16/447
> fixes
> it for you too? (Make sure you're at max brightness before rebooting.)
I'll try it now.
Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid
Sorry for this late answer. I only get a very little time for this.
On Fri, Feb 18, 2011 at 05:57, Indan Zupancic in...@nul.nu wrote:
On Thu, February 17, 2011 23:13, Tino Keitel wrote:
with kernel 2.6.37, the display brightness of my ThinkPad X61s was
always reduced after lid open, resume from suspend etc. With this
patch on top of 2.6.38-rc5, the problem is gone. Thanks.
Tino, I think Alex's patch only hides the problem and doesn't properly solve
Could well be. I don't understand what the code is supposed to do.
The patch was created just be looking at diffs.
the real bug. Can you confirm that this is the bit that fixes it for you?
diff --git a/drivers/gpu/drm/i915/intel_panel.c
b/drivers/gpu/drm/i915/intel_panel.c
index c65992d..c4b1ca4 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -267,6 +235,9 @@ void intel_panel_enable_backlight(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev-dev_private;
+ if (dev_priv-backlight_enabled)
+ return;
+
if (dev_priv-backlight_level == 0)
dev_priv-backlight_level = intel_panel_get_max_backlight(dev);
(Alex's patch edited by hand, offsets might be wrong.)
It is not enough, at least for me.
The other bits either don't change the logic, or should be harmless, or are
plain wrong, like setting the brightness to maximum at bootup.
I am not absolutely sure, but I don't think this is what happens on this laptop.
Lastly, could you verify that my patch at https://lkml.org/lkml/2011/2/16/447
fixes
it for you too? (Make sure you're at max brightness before rebooting.)
I'll try it now.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid
On Sat, Feb 19, 2011 at 13:11, Alex Riesen raa.l...@gmail.com wrote: Lastly, could you verify that my patch at https://lkml.org/lkml/2011/2/16/447 fixes it for you too? (Make sure you're at max brightness before rebooting.) I'll try it now. I can confirm that it does fix backlight in my case (Dell XPS 1330, LVDS panel, GM965/GL960). Tested-by: Alex Riesen raa.l...@gmail.com ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid
On Sat, Feb 19, 2011 at 4:26 AM, Alex Riesen raa.l...@gmail.com wrote: On Sat, Feb 19, 2011 at 13:11, Alex Riesen raa.l...@gmail.com wrote: Lastly, could you verify that my patch at https://lkml.org/lkml/2011/2/16/447 fixes it for you too? (Make sure you're at max brightness before rebooting.) I'll try it now. I can confirm that it does fix backlight in my case (Dell XPS 1330, LVDS panel, GM965/GL960). Tested-by: Alex Riesen raa.l...@gmail.com Guys, should I apply this, or will I get it through somebody's pull? Linus ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] fix backlight brightness on intel LVDS panel after reopening lid
On Wed, Feb 16, 2011 at 20:26:58 +0100, Alex Riesen wrote: > Signed-off-by: Alex Riesen > > --- > Linus Torvalds, Wed, Feb 16, 2011 05:16:01 +0100: > > Most of the changes are pretty spread out and small, with drivers/gpu > > (radeon and i915) somewhat standing out from the pack. ... > > The backlight level on this Dell XPS M1330 reduces every time I reopen the > lid, and BIOS does not seem to know anything about that (the keyboard > shortcuts to set backlight brightness cause it to jump to the level next to > the one set before closing the lid). Hi, with kernel 2.6.37, the display brightness of my ThinkPad X61s was always reduced after lid open, resume from suspend etc. With this patch on top of 2.6.38-rc5, the problem is gone. Thanks. Regards, Tino -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] fix backlight brightness on intel LVDS panel after reopening lid
On Thu, February 17, 2011 23:13, Tino Keitel wrote:
> with kernel 2.6.37, the display brightness of my ThinkPad X61s was
> always reduced after lid open, resume from suspend etc. With this
> patch on top of 2.6.38-rc5, the problem is gone. Thanks.
Tino, I think Alex's patch only hides the problem and doesn't properly solve
the real bug. Can you confirm that this is the bit that fixes it for you?
diff --git a/drivers/gpu/drm/i915/intel_panel.c
b/drivers/gpu/drm/i915/intel_panel.c
index c65992d..c4b1ca4 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -267,6 +235,9 @@ void intel_panel_enable_backlight(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev->dev_private;
+ if (dev_priv->backlight_enabled)
+ return;
+
if (dev_priv->backlight_level == 0)
dev_priv->backlight_level = intel_panel_get_max_backlight(dev);
(Alex's patch edited by hand, offsets might be wrong.)
The other bits either don't change the logic, or should be harmless, or are
plain wrong, like setting the brightness to maximum at bootup.
If the above bit "fixes" it then it's because intel_panel_set_backlight() is
called
less often, as that's the buggy function the problem doesn't show up (or is less
clear). Calling intel_panel_set_backlight() with the same value should keep the
brightness the same. Because of the buggy combination code it doesn't always.
Also, try suspending/resuming or "xset dpms force off/on" often in a loop with
both
highest and lowest brightness and check if it works correctly with just Alex's
patch.
Lastly, could you verify that my patch at https://lkml.org/lkml/2011/2/16/447
fixes
it for you too? (Make sure you're at max brightness before rebooting.)
That said, the above bit of Alex's patch should be fine to apply, because it
avoids
unnecessary register fiddling either way.
Greetings,
Indan
[PATCH] fix backlight brightness on intel LVDS panel after reopening lid
On Wed, Feb 16, 2011 at 20:26:58 +0100, Alex Riesen wrote: > Signed-off-by: Alex Riesen > > --- > Linus Torvalds, Wed, Feb 16, 2011 05:16:01 +0100: > > Most of the changes are pretty spread out and small, with drivers/gpu > > (radeon and i915) somewhat standing out from the pack. ... > > The backlight level on this Dell XPS M1330 reduces every time I reopen the > lid, and BIOS does not seem to know anything about that (the keyboard > shortcuts to set backlight brightness cause it to jump to the level next to > the one set before closing the lid). Hi, with kernel 2.6.37, the display brightness of my ThinkPad X61s was always reduced after lid open, resume from suspend etc. With this patch on top of 2.6.38-rc5, the problem is gone. Thanks. Regards, Tino
Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid
On Wed, Feb 16, 2011 at 20:26:58 +0100, Alex Riesen wrote: Signed-off-by: Alex Riesen raa.l...@gmail.com --- Linus Torvalds, Wed, Feb 16, 2011 05:16:01 +0100: Most of the changes are pretty spread out and small, with drivers/gpu (radeon and i915) somewhat standing out from the pack. ... The backlight level on this Dell XPS M1330 reduces every time I reopen the lid, and BIOS does not seem to know anything about that (the keyboard shortcuts to set backlight brightness cause it to jump to the level next to the one set before closing the lid). Hi, with kernel 2.6.37, the display brightness of my ThinkPad X61s was always reduced after lid open, resume from suspend etc. With this patch on top of 2.6.38-rc5, the problem is gone. Thanks. Regards, Tino ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid
On Thu, February 17, 2011 23:13, Tino Keitel wrote:
with kernel 2.6.37, the display brightness of my ThinkPad X61s was
always reduced after lid open, resume from suspend etc. With this
patch on top of 2.6.38-rc5, the problem is gone. Thanks.
Tino, I think Alex's patch only hides the problem and doesn't properly solve
the real bug. Can you confirm that this is the bit that fixes it for you?
diff --git a/drivers/gpu/drm/i915/intel_panel.c
b/drivers/gpu/drm/i915/intel_panel.c
index c65992d..c4b1ca4 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -267,6 +235,9 @@ void intel_panel_enable_backlight(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev-dev_private;
+ if (dev_priv-backlight_enabled)
+ return;
+
if (dev_priv-backlight_level == 0)
dev_priv-backlight_level = intel_panel_get_max_backlight(dev);
(Alex's patch edited by hand, offsets might be wrong.)
The other bits either don't change the logic, or should be harmless, or are
plain wrong, like setting the brightness to maximum at bootup.
If the above bit fixes it then it's because intel_panel_set_backlight() is
called
less often, as that's the buggy function the problem doesn't show up (or is less
clear). Calling intel_panel_set_backlight() with the same value should keep the
brightness the same. Because of the buggy combination code it doesn't always.
Also, try suspending/resuming or xset dpms force off/on often in a loop with
both
highest and lowest brightness and check if it works correctly with just Alex's
patch.
Lastly, could you verify that my patch at https://lkml.org/lkml/2011/2/16/447
fixes
it for you too? (Make sure you're at max brightness before rebooting.)
That said, the above bit of Alex's patch should be fine to apply, because it
avoids
unnecessary register fiddling either way.
Greetings,
Indan
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] fix backlight brightness on intel LVDS panel after reopening lid
On Wed, Feb 16, 2011 at 21:05, Jesse Barnes wrote: > On Wed, 16 Feb 2011 20:59:35 +0100 > Alex Riesen wrote: >> >> I don't think it is related. I don't have problems switching >> the outputs (frankly, didn't try) and I do have problems >> restoring backlight, very similar to what I had earlier in >> 2.6.37. > > Right, but it affects the registration of the backlight and ACPI video > interface as well, so can affect backlight restore on resume. ?In my > case, without the above patch my backlight wouldn't be restored on > resume, so I'd have to manually echo a value > into /sys/class/backlight/ to get my display back. Ok, I tried, but only to find out that my tree already has the patch (it is in mainline since today). So it definitely does not help. Thanks anyway
[PATCH] fix backlight brightness on intel LVDS panel after reopening lid
On Wed, Feb 16, 2011 at 20:54, Jesse Barnes wrote: > On Wed, 16 Feb 2011 20:46:45 +0100 > Alex Riesen wrote: >> > The backlight level on this Dell XPS M1330 reduces every time I reopen >> > the lid, and BIOS does not seem to know anything about that (the >> > keyboard shortcuts to set backlight brightness cause it to jump to the >> > level next to the one set before closing the lid). >> >> It is this bug, I believe: >> >> https://bugzilla.kernel.org/show_bug.cgi?id=25072 >> >> I somehow missed it at first, and only noticed after sending the patch. > > There's also this patch: https://patchwork.kernel.org/patch/499221/. > It got things working on my E6510 again at least. I don't think it is related. I don't have problems switching the outputs (frankly, didn't try) and I do have problems restoring backlight, very similar to what I had earlier in 2.6.37.
[PATCH] fix backlight brightness on intel LVDS panel after reopening lid
On Wed, Feb 16, 2011 at 20:26, Alex Riesen wrote: > Linus Torvalds, Wed, Feb 16, 2011 05:16:01 +0100: >> Most of the changes are pretty spread out and small, with drivers/gpu >> (radeon and i915) somewhat standing out from the pack. ... > > The backlight level on this Dell XPS M1330 reduces every time I reopen > the lid, and BIOS does not seem to know anything about that (the > keyboard shortcuts to set backlight brightness cause it to jump to the > level next to the one set before closing the lid). It is this bug, I believe: https://bugzilla.kernel.org/show_bug.cgi?id=25072 I somehow missed it at first, and only noticed after sending the patch.
[PATCH] fix backlight brightness on intel LVDS panel after reopening lid
Signed-off-by: Alex Riesen
---
Linus Torvalds, Wed, Feb 16, 2011 05:16:01 +0100:
> Most of the changes are pretty spread out and small, with drivers/gpu
> (radeon and i915) somewhat standing out from the pack. ...
The backlight level on this Dell XPS M1330 reduces every time I reopen the
lid, and BIOS does not seem to know anything about that (the keyboard
shortcuts to set backlight brightness cause it to jump to the level next to
the one set before closing the lid).
Maybe i915 code for LVDS panels have lost some parts of the backlight
enable/disable balancing patch by Chris Wilson? Or maybe it just got broken
along the way...
This part of the patch by Chris helped here, but I afraid it might be not
complete or just wrong (for instance, the original patch didn't have to remove
the i915_read_blc_pwm_ctl function).
drivers/gpu/drm/i915/intel_panel.c | 65 ++--
1 files changed, 18 insertions(+), 47 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_panel.c
b/drivers/gpu/drm/i915/intel_panel.c
index c65992d..c4b1ca4 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -125,55 +125,15 @@ static int is_backlight_combination_mode(struct
drm_device *dev)
return 0;
}
-static u32 i915_read_blc_pwm_ctl(struct drm_i915_private *dev_priv)
-{
- u32 val;
-
- /* Restore the CTL value if it lost, e.g. GPU reset */
-
- if (HAS_PCH_SPLIT(dev_priv->dev)) {
- val = I915_READ(BLC_PWM_PCH_CTL2);
- if (dev_priv->saveBLC_PWM_CTL2 == 0) {
- dev_priv->saveBLC_PWM_CTL2 = val;
- } else if (val == 0) {
- I915_WRITE(BLC_PWM_PCH_CTL2,
- dev_priv->saveBLC_PWM_CTL);
- val = dev_priv->saveBLC_PWM_CTL;
- }
- } else {
- val = I915_READ(BLC_PWM_CTL);
- if (dev_priv->saveBLC_PWM_CTL == 0) {
- dev_priv->saveBLC_PWM_CTL = val;
- dev_priv->saveBLC_PWM_CTL2 = I915_READ(BLC_PWM_CTL2);
- } else if (val == 0) {
- I915_WRITE(BLC_PWM_CTL,
- dev_priv->saveBLC_PWM_CTL);
- I915_WRITE(BLC_PWM_CTL2,
- dev_priv->saveBLC_PWM_CTL2);
- val = dev_priv->saveBLC_PWM_CTL;
- }
- }
-
- return val;
-}
-
u32 intel_panel_get_max_backlight(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev->dev_private;
u32 max;
- max = i915_read_blc_pwm_ctl(dev_priv);
- if (max == 0) {
- /* XXX add code here to query mode clock or hardware clock
-* and program max PWM appropriately.
-*/
- printk_once(KERN_WARNING "fixme: max PWM is zero.\n");
- return 1;
- }
-
if (HAS_PCH_SPLIT(dev)) {
- max >>= 16;
+ max = I915_READ(BLC_PWM_PCH_CTL2) >> 16;
} else {
+ max = I915_READ(BLC_PWM_CTL);
if (IS_PINEVIEW(dev)) {
max >>= 17;
} else {
@@ -186,6 +146,14 @@ u32 intel_panel_get_max_backlight(struct drm_device *dev)
max *= 0xff;
}
+ if (max == 0) {
+ /* XXX add code here to query mode clock or hardware clock
+* and program max PWM appropriately.
+*/
+ DRM_ERROR("fixme: max PWM is zero.\n");
+ max = 1;
+ }
+
DRM_DEBUG_DRIVER("max backlight PWM = %d\n", max);
return max;
}
@@ -255,11 +223,11 @@ 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;
- }
+ if (!dev_priv->backlight_enabled)
+ return;
+ dev_priv->backlight_enabled = false;
+ dev_priv->backlight_level = intel_panel_get_backlight(dev);
intel_panel_set_backlight(dev, 0);
}
@@ -267,6 +235,9 @@ void intel_panel_enable_backlight(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev->dev_private;
+ if (dev_priv->backlight_enabled)
+ return;
+
if (dev_priv->backlight_level == 0)
dev_priv->backlight_level = intel_panel_get_max_backlight(dev);
@@ -278,6 +249,6 @@ void intel_panel_setup_backlight(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev->dev_private;
- dev_priv->backlight_level = intel_panel_get_backlight(dev);
+ dev_priv->backlight_level = intel_panel_get_max_backlight(dev);
dev_priv->backlight_enabled = dev_priv->backlight_level != 0;
}
--
1.7.4.27.gf5729
[PATCH] fix backlight brightness on intel LVDS panel after reopening lid
On Wed, 16 Feb 2011 20:59:35 +0100 Alex Riesen wrote: > On Wed, Feb 16, 2011 at 20:54, Jesse Barnes > wrote: > > On Wed, 16 Feb 2011 20:46:45 +0100 > > Alex Riesen wrote: > >> > The backlight level on this Dell XPS M1330 reduces every time I reopen > >> > the lid, and BIOS does not seem to know anything about that (the > >> > keyboard shortcuts to set backlight brightness cause it to jump to the > >> > level next to the one set before closing the lid). > >> > >> It is this bug, I believe: > >> > >> https://bugzilla.kernel.org/show_bug.cgi?id=25072 > >> > >> I somehow missed it at first, and only noticed after sending the patch. > > > > There's also this patch: https://patchwork.kernel.org/patch/499221/. > > It got things working on my E6510 again at least. > > I don't think it is related. I don't have problems switching > the outputs (frankly, didn't try) and I do have problems > restoring backlight, very similar to what I had earlier in > 2.6.37. Right, but it affects the registration of the backlight and ACPI video interface as well, so can affect backlight restore on resume. In my case, without the above patch my backlight wouldn't be restored on resume, so I'd have to manually echo a value into /sys/class/backlight/ to get my display back. -- Jesse Barnes, Intel Open Source Technology Center
[PATCH] fix backlight brightness on intel LVDS panel after reopening lid
On Wed, 16 Feb 2011 20:46:45 +0100 Alex Riesen wrote: > On Wed, Feb 16, 2011 at 20:26, Alex Riesen wrote: > > Linus Torvalds, Wed, Feb 16, 2011 05:16:01 +0100: > >> Most of the changes are pretty spread out and small, with drivers/gpu > >> (radeon and i915) somewhat standing out from the pack. ... > > > > The backlight level on this Dell XPS M1330 reduces every time I reopen > > the lid, and BIOS does not seem to know anything about that (the > > keyboard shortcuts to set backlight brightness cause it to jump to the > > level next to the one set before closing the lid). > > It is this bug, I believe: > > https://bugzilla.kernel.org/show_bug.cgi?id=25072 > > I somehow missed it at first, and only noticed after sending the patch. There's also this patch: https://patchwork.kernel.org/patch/499221/. It got things working on my E6510 again at least. -- Jesse Barnes, Intel Open Source Technology Center
