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

["Indan Zupancic" <indan@nul.nu>]

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

> 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.

>
> 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