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@xxxxxx> > > Cc: <stable@xxxxxxxxxx> > > Signed-off-by: Takashi Iwai <tiwai@xxxxxxx> > > --- > > 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 > > _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
