[Intel-gfx] [PATCH 2/4] drm/i915: remove combination mode for backlight control, again
Daniel Vetter
daniel at ffwll.ch
Tue Aug 28 16:55:57 CEST 2012
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
More information about the Intel-gfx
mailing list