| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <a04bdafb8c7a97d92593e980b01b37fd.squirrel@webmail.greenhost.nl>
Date: Thu, 10 Mar 2011 11:06:28 +0100 (CET)
From: "Indan Zupancic" <indan@....nu>
To: "Takashi Iwai" <tiwai@...e.de>
Cc: "Linus Torvalds" <torvalds@...ux-foundation.org>,
"Alex Riesen" <raa.lkml@...il.com>,
"Jesse Barnes" <jbarnes@...tuousgeek.org>,
"DRI mailing list" <dri-devel@...ts.freedesktop.org>,
"Chris Wilson" <chris@...is-wilson.co.uk>,
"Linux Kernel Mailing List" <linux-kernel@...r.kernel.org>,
"Tino Keitel" <tino.keitel@...ei.de>, stable@...nel.org
Subject: 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 <indan@....nu> wrote:
>> > >>
>> > >> So please revert my patch and apply Takashi Iwai's, which fixes the
>> > >> most immediate bug without changing anything else. This should go
>> > >> in stable too.
>> > >
>> >
>> > I found another backlight bug:
>> >
>> > When suspending intel_panel_disable_backlight() is never called,
>> > but intel_panel_enable_backlight() is called at resume. With the
>> > effect that if the brightness was ever changed after screen
>> > blanking, the wrong brightness gets restored.
>> >
>> > This explains the weird behaviour I've seen. I didn't see it with
>> > combination mode, because then the brightness is always the same
>> > (zero or the maximum, the BIOS only uses LBPC on my system.) I'll
>> > send a patch in a moment.
>> >
>> > Alternative for reverting the combination mode removal (I can also
>> > redo the patch against the revert and Takashi's patch, if that's
>> > preferred):
>> >
>> > --
>> >
>> > drm/i915: Do handle backlight combination mode specially
>> >
>> > Add back the combination mode check, but with slightly cleaner code
>> > and the weirdness removed: No val >>= 1, but also no val &= ~1. The
>> > old code probably confused bit 0 with BLM_LEGACY_MODE, which is bit 16.
>> > The other change is clearer calculations: Just check for zero level
>> > explicitly instead of avoiding the divide-by-zero.
>> >
>> > Signed-off-by: Indan Zupancic <indan@....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;
> level /= lpbc;
> pci_write_config_byte(dev->pdev, PCI_LBPC, lpbc);
> }
>
> This will fit within the right range.
> Though, changing like below will give a bit better calculation,
> closer to the real level.
>
> lpbc = level * 0xfe / max + 1;
> level = (level + lpbc / 2) / lpbc;
Indeed, though I don't think it makes much difference in practise.
All in all it seems best to just revert my patch and apply your fix.
Any "improvements" I may have are either buggy or can be added later.
Care to make a new patch with the above improvement added? You can
add my acked-by, for what it's worth.
At this point I don't even dare removing that "obviously" bogus
val &= ~1; I bet it's an undocumented bit having some obscure
secret function on not well tested systems.
Greetings,
Indan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists