Re: [PATCH] drm/i915: Revive combination mode for backlight control

[Takashi Iwai <tiwai@suse.de>]

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 <indan@nul.nu>
> > Cc: <stable@kernel.org>
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> >  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
> 
>