On Thu, March 10, 2011 08:49, Takashi Iwai wrote:
> At Thu, 10 Mar 2011 06:50:09 +0100 (CET),
> Indan Zupancic wrote:
>>
>> Hello,
>>
>> On Fri, March 4, 2011 19:47, Linus Torvalds wrote:
>> > Alex, can you confirm that the revert of 951f3512dba5 plus the
>> > one-liner patch from Takashi that Indan quoted also works for you?
>> >
>> > Linus
>> >
>> > On Thu, Mar 3, 2011 at 10:53 PM, Indan Zupancic <indan at nul.nu> wrote:
>> >>
>> >> So please revert my patch and apply Takashi Iwai's, which fixes the
>> >> most immediate bug without changing anything else. This should go
>> >> in stable too.
>> >
>>
>> I found another backlight bug:
>>
>> When suspending intel_panel_disable_backlight() is never called,
>> but intel_panel_enable_backlight() is called at resume. With the
>> effect that if the brightness was ever changed after screen
>> blanking, the wrong brightness gets restored.
>>
>> This explains the weird behaviour I've seen. I didn't see it with
>> combination mode, because then the brightness is always the same
>> (zero or the maximum, the BIOS only uses LBPC on my system.) I'll
>> send a patch in a moment.
>>
>> Alternative for reverting the combination mode removal (I can also
>> redo the patch against the revert and Takashi's patch, if that's
>> preferred):
>>
>> --
>>
>> drm/i915: Do handle backlight combination mode specially
>>
>> Add back the combination mode check, but with slightly cleaner code
>> and the weirdness removed: No val >>= 1, but also no val &= ~1. The
>> old code probably confused bit 0 with BLM_LEGACY_MODE, which is bit 16.
>> The other change is clearer calculations: Just check for zero level
>> explicitly instead of avoiding the divide-by-zero.
>>
>> Signed-off-by: Indan Zupancic <indan at 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
