On Tue, Aug 28, 2012 at 04:39:34PM +0200, Indan Zupancic wrote: > Hello, > > You seem to be making exactly the same mistake I made. > > On Tue, August 28, 2012 08:53, Jani Nikula wrote: > > The combination/legacy mode was first removed in > > > > commit 951f3512dba5bd44cda3e5ee22b4b522e4bb09fb > > Author: Indan Zupancic <indan at nul.nu> > > Date: Thu Feb 17 02:41:49 2011 +0100 > > > > drm/i915: Do not handle backlight combination mode specially > > > > which was subsequently reverted due to regression in > > > > commit ba3820ade317ee36e496b9b40d2ec3987dd4aef0 > > Author: Takashi Iwai <tiwai at suse.de> > > Date: Thu Mar 10 14:02:12 2011 +0100 > > > > drm/i915: Revive combination mode for backlight control > > > > It seems that on some machines the combination mode was necessary because > > it reset the LBPC register value on resume. > > No, it was bad interaction with systems using ASLE to control backlight. > > To quote my old email: > > "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." > > See http://marc.info/?l=dri-devel&m=129980668309522&w=2 Meh, I've totally failed to dig this out. Makes quite a bit more sense and clarifies the confusion revert commit message quite a bit. Thanks for digging this out again, I guess we should include it. > > Since the driver now saves and > > restores the register over suspend, the combination mode no longer needs to > > be treated specially, and the driver doesn't need to modify the LBPC > > register at all. > > That's what I thought when I wrote my original patch. But that only seems > to be true for GEN2 hardware which has no ASLE. It seems GEN4 with ASLE > needs the combination mode check because there the BIOS might use LBPC to > set the brightness at system boot, even though the driver only uses the > PWM registers to set it. (If the BIOS does something similar at resume > time then it's really messed up.) But assuming it only happens at bootup, > you could check for it once at module init and turn it off if it's enabled. > But it seems you can't totally ignore legacy/combination mode. > > Some backlight problems on GEN4 can be solved by not fiddling with the > backlight. The current code sets the backlight to 0 to disable the panel > (last year anyway, maybe the code changed), but on gen >= 4 it can do the > same by clearing bit 31 in BLC_PWM_CLT2. That way the original backlight > value doesn't need to be saved anywhere. A while back I've improved the backlight code to properly switch the pipe that controls the pwm and also to properly toggle the enable bit for gen4+, see the new intel_panel_enable_backlight functions. Would it be correct in your opinion to simply ditch the call to intel_panel_actually_set_backlight for gen4+ unconditionally? Same for intel_panel_disable_backlight obviously? Thanks, Daniel > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=53153 > > CC: Indan Zupancic <indan at nul.nu> > > CC: Takashi Iwai <tiwai at suse.de> > > Signed-off-by: Jani Nikula <jani.nikula at intel.com> > > --- > > drivers/gpu/drm/i915/intel_panel.c | 32 -------------------------------- > > 1 file changed, 32 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c > > index 9546f97..0a7f47a 100644 > > --- a/drivers/gpu/drm/i915/intel_panel.c > > +++ b/drivers/gpu/drm/i915/intel_panel.c > > @@ -115,19 +115,6 @@ 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; > > @@ -181,9 +168,6 @@ u32 intel_panel_get_max_backlight(struct drm_device *dev) > > max >>= 17; > > else > > max >>= 16; > > - > > - if (is_backlight_combination_mode(dev)) > > - max *= 0xff; > > } > > > > DRM_DEBUG_DRIVER("max backlight PWM = %d\n", max); > > @@ -222,13 +206,6 @@ static u32 intel_panel_get_backlight(struct drm_device *dev) > > val = I915_READ(BLC_PWM_CTL) & BACKLIGHT_DUTY_CYCLE_MASK; > > if (INTEL_INFO(dev)->gen < 4) > > val >>= 1; > > - > > - if (is_backlight_combination_mode(dev)) { > > - u8 lbpc; > > - > > - pci_read_config_byte(dev->pdev, PCI_LBPC, &lbpc); > > - val *= lbpc; > > - } > > } > > > > val = intel_panel_compute_brightness(dev, val); > > @@ -254,15 +231,6 @@ static void intel_panel_actually_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 (INTEL_INFO(dev)->gen < 4) > > level <<= 1; > > -- > > 1.7.9.5 > > > > > > Greetings, > > Indan > > -- Daniel Vetter Mail: daniel at ffwll.ch Mobile: +41 (0)79 365 57 48
