Re: [PATCH 2/4] drm/i915: remove combination mode for backlight control, again

[Daniel Vetter <daniel@ffwll.ch>]

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@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@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@nul.nu>
> > CC: Takashi Iwai <tiwai@suse.de>
> > Signed-off-by: Jani Nikula <jani.nikula@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@ffwll.ch
Mobile: +41 (0)79 365 57 48