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    [W:0.032 / U:1.388 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site