Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753899Ab1CKH0x (ORCPT ); Fri, 11 Mar 2011 02:26:53 -0500 Received: from cantor2.suse.de ([195.135.220.15]:52934 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753732Ab1CKH0u (ORCPT ); Fri, 11 Mar 2011 02:26:50 -0500 Date: Fri, 11 Mar 2011 08:26:48 +0100 Message-ID: From: Takashi Iwai To: "Indan Zupancic" Cc: "Linus Torvalds" , "Alex Riesen" , "Jesse Barnes" , "DRI mailing list" , "Chris Wilson" , "Linux Kernel Mailing List" , "Tino Keitel" , stable@kernel.org Subject: Re: [PATCH] drm/i915: Revive combination mode for backlight control In-Reply-To: <334e13a8513c5863dc9a5d278831777f.squirrel@webmail.greenhost.nl> 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> <334e13a8513c5863dc9a5d278831777f.squirrel@webmail.greenhost.nl> User-Agent: Wanderlust/2.15.6 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.7 Emacs/23.2 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5897 Lines: 181 At Fri, 11 Mar 2011 02:23:08 +0100 (CET), Indan Zupancic wrote: > > Hi, > > Some nitpicks below. I know you're just restoring the original code, > but if we improve it we can as well do it now. Well, Linus already merged my patch quickly. So, we can improve the readability as an additional patch, but I think it's likely a 2.6.39 material. All you comments below look reasonable. Could you send a new patch? thanks, Takashi > On Thu, March 10, 2011 14:02, Takashi Iwai wrote: > > This reverts commit 951f3512dba5bd44cda3e5ee22b4b522e4bb09fb > > > > drm/i915: Do not handle backlight combination mode specially > > > > since this commit introduced other regressions due to untouched LBPC > > register, e.g. the backlight dimmed after resume. > > The regression was that, if ALSE was used, the maximum brightness > would be the brightness at boot, because LBPC isn't touched and > the BIOS restores it at boot. So the sympom would be less brightness > levels or lower maximum brightness. > > Systems with no ASLE support work fine because there the code is only > used to save and restore the PWM register. LBPC is saved and restored > by the BIOS. > > The backlight dimming after resume/blanking was the original bug > caused by the bogus val <<=1 shift. > > > In addition to the revert, this patch includes a fix for the original > > issue (weird backlight levels) by removing the wrong bit shift for > > computing the current backlight level. > > Also, including typo fixes (lpbc -> lbpc). > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=34524 > > Acked-by: Indan Zupancic > > Cc: > > Signed-off-by: Takashi Iwai > > --- > > drivers/gpu/drm/i915/i915_reg.h | 10 ++++++++++ > > drivers/gpu/drm/i915/intel_panel.c | 36 ++++++++++++++++++++++++++++++++++++ > > 2 files changed, 46 insertions(+), 0 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > index 3e6f486..2abe240 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -1553,7 +1553,17 @@ > > > > /* Backlight control */ > > #define BLC_PWM_CTL 0x61254 > > +#define BACKLIGHT_MODULATION_FREQ_SHIFT (17) > > This one isn't used anywhere. > > > #define BLC_PWM_CTL2 0x61250 /* 965+ only */ > > +#define BLM_COMBINATION_MODE (1 << 30) > > +/* > > + * This is the most significant 15 bits of the number of backlight cycles in a > > + * complete cycle of the modulated backlight control. > > + * > > + * The actual value is this field multiplied by two. > > + */ > > +#define BACKLIGHT_MODULATION_FREQ_MASK (0x7fff << 17) > > This one isn't used and the comment is misleading, I think. > > > +#define BLM_LEGACY_MODE (1 << 16) > > /* > > * This is the number of cycles out of the backlight modulation cycle for which > > * the backlight is on. > > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c > > index d860abe..f8f86e5 100644 > > --- a/drivers/gpu/drm/i915/intel_panel.c > > +++ b/drivers/gpu/drm/i915/intel_panel.c > > @@ -30,6 +30,8 @@ > > > > #include "intel_drv.h" > > > > +#define PCI_LBPC 0xf4 /* legacy/combination backlight modes */ > > + > > I'd either put all the defines in i915_reg.h, or have all combination > mode specific defines here. Though I guess LBPC is an odd one. > > > void > > intel_fixed_panel_mode(struct drm_display_mode *fixed_mode, > > struct drm_display_mode *adjusted_mode) > > @@ -110,6 +112,19 @@ done: > > dev_priv->pch_pf_size = (width << 16) | height; > > } > > > > +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; > > @@ -166,6 +181,9 @@ u32 intel_panel_get_max_backlight(struct drm_device *dev) > > if (INTEL_INFO(dev)->gen < 4) > > max &= ~1; > > I'd add a comment that this is to clear the BLM_LEGACY_MODE bit. > > > } > > + > > + if (is_backlight_combination_mode(dev)) > > + max *= 0xff; > > } > > > > DRM_DEBUG_DRIVER("max backlight PWM = %d\n", max); > > @@ -183,6 +201,14 @@ 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; > > + > > + val &= ~1; > > + pci_read_config_byte(dev->pdev, PCI_LBPC, &lbpc); > > + val *= lbpc; > > + } > > } > > > > DRM_DEBUG_DRIVER("get backlight PWM = %d\n", val); > > @@ -205,6 +231,16 @@ 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 (is_backlight_combination_mode(dev)){ > > + u32 max = intel_panel_get_max_backlight(dev); > > + u8 lbpc; > > + > > + lbpc = level * 0xfe / max + 1; > > + level /= lbpc; > > + pci_write_config_byte(dev->pdev, PCI_LBPC, lbpc); > > + } > > + > > tmp = I915_READ(BLC_PWM_CTL); > > if (IS_PINEVIEW(dev)) { > > tmp &= ~(BACKLIGHT_DUTY_CYCLE_MASK - 1); > > After this patch, combined with my new patch > > "drm/i915: Fix DPMS and suspend interaction for intel_panel.c" > > all known backlight problems should be fixed. > > Greetings, > > 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/