lkml.org 
[lkml]   [2011]   [Mar]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] drm/i915: Revive combination mode for backlight control
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
>
>


\
 
 \ /
  Last update: 2011-03-11 08:29    [from the cache]
©2003-2011 Jasper Spaans