Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751601Ab1CJIpb (ORCPT ); Thu, 10 Mar 2011 03:45:31 -0500 Received: from smarthost1.greenhost.nl ([195.190.28.78]:37165 "EHLO smarthost1.greenhost.nl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750769Ab1CJIp3 (ORCPT ); Thu, 10 Mar 2011 03:45:29 -0500 Message-ID: <5c286d66bf790d7fbdd69c9861edc91d.squirrel@webmail.greenhost.nl> In-Reply-To: References: <20110216192658.GA7225@blimp.localdomain> <20110217221329.GA3332@x61.home> <3f792aaf90cf0b3d49be21baa2682d5d.squirrel@webmail.greenhost.nl> <20110222130440.21a27714@jbarnes-desktop> <20110222223120.GA3567@x61.home> <3e6f092bd0aa54fd6b9eb524f6c87ecf.squirrel@webmail.greenhost.nl> <1a06e2711df021a802d609ad1a75db17.squirrel@webmail.greenhost.nl> Date: Thu, 10 Mar 2011 09:45:18 +0100 (CET) Subject: Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid From: "Indan Zupancic" To: "Takashi Iwai" Cc: "Linus Torvalds" , "Alex Riesen" , "Jesse Barnes" , "DRI mailing list" , "Chris Wilson" , "Linux Kernel Mailing List" , "Tino Keitel" , stable@kernel.org User-Agent: SquirrelMail/1.4.17 MIME-Version: 1.0 Content-Type: text/plain;charset=UTF-8 Content-Transfer-Encoding: 8bit X-Priority: 3 (Normal) Importance: Normal X-Spam-Score: 0.1 X-Scan-Signature: 373e2adaaf179be0df59b6b9982b76fe Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5036 Lines: 151 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, &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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/