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.
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 <[email protected]>
Cc: <[email protected]>
Signed-off-by: Takashi Iwai <[email protected]>
---
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)
#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)
+#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 */
+
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;
}
+
+ 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);
--
1.7.4.1
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.
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 <[email protected]>
> Cc: <[email protected]>
> Signed-off-by: Takashi Iwai <[email protected]>
> ---
> 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
On Thu, Mar 10, 2011 at 5:23 PM, Indan Zupancic <[email protected]> wrote:
>
> 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.
Btw, can you repost that one as a new email (and cc keithp too)? I
think it got hidden in the other thread by different subject line.
Jesse, did you take a look at that one? It was in the thread with a
subject like "[PATCH] fix backlight brightness on intel LVDS panel
after reopening lid".
Search for
When suspending intel_panel_disable_backlight() is never called,
but intel_panel_enable_backlight() is called at resume. With the
effect that if the brightness was ever changed after screen
blanking, the wrong brightness gets restored at resume time.
in your mailbox.
Linus
On Thu, March 10, 2011 20:36, Keith Packard wrote:
> On Thu, 10 Mar 2011 14:02:12 +0100, Takashi Iwai <[email protected]> wrote:
>> + val &= ~1;
>
> The reason for this is that some ancient platforms wire this bit to
> be "go to max backlight", or at least that's why this was in the 2D
> driver from which this code was derived.
If that is the case, then shouldn't it be at the end, after val *= lbpc?
I know it doesn't make much difference, as multiplying with an even number
always gives an even result, but it would make the intention clearer and
give a more precise result.
What about gen 3? Does it support combination mode too?
If you can confirm that there are no gen 2 systems with ASLE support,
we can remove the gen 2 check and only touch the PWM register, as the
ASLE code is the only one that changes the brightness. The panel code
only saves and restores it, and LBPC is saved and restored by the
BIOS already. Then those weird gen 2 quirks can be removed.
(Something for 2.6.39 perhaps.)
It's sad that something as simple as backlight control needs so much
code.
Greetings,
Indan
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 <[email protected]>
> > Cc: <[email protected]>
> > Signed-off-by: Takashi Iwai <[email protected]>
> > ---
> > 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
>
>
On Fri, March 11, 2011 08:26, Takashi Iwai wrote:
> 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?
Well, my main concern was the slightly incorrect commit message,
but oh well, too late for that now. I'll probably send a patch
after 2.6.38 is released.
Greetings,
Indan