Hi,
I'm facing a bug on a Samsung X20 notebook which features an i915 chipset (output of 'lspci -v' attached).
The effect is that setting the backlight to odd values causes the value to be misinterpreted. Harald Hoyer (cc:) had the same thing on a Netbook (I don't recall which model it was).
So this will turn the backlight to full brightness:
# cat /sys/class/backlight/intel_backlight/max_brightness 29750 # echo 29750 > /sys/class/backlight/intel_backlight/brightness
However, writing 29749 will turn the display backlight off, and 29748 appears to be the next valid lower value.
It seems like the IS_PINEVIEW() branch in drivers/gpu/drm/i915/intel_panel.c: intel_panel_actually_set_backlight() could do the right thing, but this code is written for an entirely different model, right?
I can reproduce this on 3.0 and 3.1 vanilla as well as with the current mainline git.
Let me know if there is any patch that I can test.
Thanks, Daniel
Didn't get any response yet, hence copying LKML for a broader audience.
On 11/04/2011 03:36 PM, Daniel Mack wrote:
I'm facing a bug on a Samsung X20 notebook which features an i915 chipset (output of 'lspci -v' attached).
The effect is that setting the backlight to odd values causes the value to be misinterpreted. Harald Hoyer (cc:) had the same thing on a Netbook (I don't recall which model it was).
So this will turn the backlight to full brightness:
# cat /sys/class/backlight/intel_backlight/max_brightness 29750 # echo 29750 > /sys/class/backlight/intel_backlight/brightness
However, writing 29749 will turn the display backlight off, and 29748 appears to be the next valid lower value.
It seems like the IS_PINEVIEW() branch in drivers/gpu/drm/i915/intel_panel.c:intel_panel_actually_set_backlight() could do the right thing, but this code is written for an entirely different model, right?
I can reproduce this on 3.0 and 3.1 vanilla as well as with the current mainline git.
Let me know if there is any patch that I can test.
Thanks, Daniel
On 11/08/2011 01:57 AM, Daniel Mack wrote:
Didn't get any response yet, hence copying LKML for a broader audience.
Nobody, really?
This is a rather annoying regression, as touching the brightness keys appearantly switches off the whole machine. I'm sure this is trivial to fix, I just don't have the insight of this driver and the chipset.
Any pointer greatly appreciated, and I can test patches.
Thanks, Daniel
On 11/04/2011 03:36 PM, Daniel Mack wrote:
I'm facing a bug on a Samsung X20 notebook which features an i915 chipset (output of 'lspci -v' attached).
The effect is that setting the backlight to odd values causes the value to be misinterpreted. Harald Hoyer (cc:) had the same thing on a Netbook (I don't recall which model it was).
So this will turn the backlight to full brightness:
# cat /sys/class/backlight/intel_backlight/max_brightness 29750 # echo 29750 > /sys/class/backlight/intel_backlight/brightness
However, writing 29749 will turn the display backlight off, and 29748 appears to be the next valid lower value.
It seems like the IS_PINEVIEW() branch in drivers/gpu/drm/i915/intel_panel.c:intel_panel_actually_set_backlight() could do the right thing, but this code is written for an entirely different model, right?
I can reproduce this on 3.0 and 3.1 vanilla as well as with the current mainline git.
Let me know if there is any patch that I can test.
Thanks, Daniel
At Thu, 10 Nov 2011 16:11:29 +0100, Daniel Mack wrote:
On 11/08/2011 01:57 AM, Daniel Mack wrote:
Didn't get any response yet, hence copying LKML for a broader audience.
Nobody, really?
This is a rather annoying regression, as touching the brightness keys appearantly switches off the whole machine. I'm sure this is trivial to fix, I just don't have the insight of this driver and the chipset.
I vaguely remember that the bit 0 is invalid on some old chips. Maybe 915GM is one of them, as it's gen3? If so, the patch like below may work.
Takashi
--- diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index 499d4c0..be952d1 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -249,8 +249,11 @@ static void intel_panel_actually_set_backlight(struct drm_device *dev, u32 level if (IS_PINEVIEW(dev)) { tmp &= ~(BACKLIGHT_DUTY_CYCLE_MASK - 1); level <<= 1; - } else + } else { tmp &= ~BACKLIGHT_DUTY_CYCLE_MASK; + if (INTEL_INFO(dev)->gen < 4) + tmp &= ~1; + } I915_WRITE(BLC_PWM_CTL, tmp | level); }
Hi Takashi,
On 11/10/2011 04:39 PM, Takashi Iwai wrote:
At Thu, 10 Nov 2011 16:11:29 +0100, Daniel Mack wrote:
On 11/08/2011 01:57 AM, Daniel Mack wrote:
Didn't get any response yet, hence copying LKML for a broader audience.
Nobody, really?
This is a rather annoying regression, as touching the brightness keys appearantly switches off the whole machine. I'm sure this is trivial to fix, I just don't have the insight of this driver and the chipset.
I vaguely remember that the bit 0 is invalid on some old chips. Maybe 915GM is one of them, as it's gen3? If so, the patch like below may work.
Thank you for looking into this.
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index 499d4c0..be952d1 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -249,8 +249,11 @@ static void intel_panel_actually_set_backlight(struct drm_device *dev, u32 level if (IS_PINEVIEW(dev)) { tmp &= ~(BACKLIGHT_DUTY_CYCLE_MASK - 1); level <<= 1;
- } else
- } else { tmp &= ~BACKLIGHT_DUTY_CYCLE_MASK;
if (INTEL_INFO(dev)->gen < 4)tmp &= ~1;- } I915_WRITE(BLC_PWM_CTL, tmp | level);
}
This seems to be the right intention, but the value you want to modify under this condition is 'level', not 'tmp'. With this amendment of your patch, things work perfectly fine here.
You can add my
Reported-and-tested-by: Daniel Mack zonque@gmail.com
and please Cc: stable so we get this to the users asap.
Thanks!
Daniel
[Added Chris to Cc]
At Sun, 13 Nov 2011 17:24:09 +0100, Daniel Mack wrote:
Hi Takashi,
On 11/10/2011 04:39 PM, Takashi Iwai wrote:
At Thu, 10 Nov 2011 16:11:29 +0100, Daniel Mack wrote:
On 11/08/2011 01:57 AM, Daniel Mack wrote:
Didn't get any response yet, hence copying LKML for a broader audience.
Nobody, really?
This is a rather annoying regression, as touching the brightness keys appearantly switches off the whole machine. I'm sure this is trivial to fix, I just don't have the insight of this driver and the chipset.
I vaguely remember that the bit 0 is invalid on some old chips. Maybe 915GM is one of them, as it's gen3? If so, the patch like below may work.
Thank you for looking into this.
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index 499d4c0..be952d1 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -249,8 +249,11 @@ static void intel_panel_actually_set_backlight(struct drm_device *dev, u32 level if (IS_PINEVIEW(dev)) { tmp &= ~(BACKLIGHT_DUTY_CYCLE_MASK - 1); level <<= 1;
- } else
- } else { tmp &= ~BACKLIGHT_DUTY_CYCLE_MASK;
if (INTEL_INFO(dev)->gen < 4)tmp &= ~1;- } I915_WRITE(BLC_PWM_CTL, tmp | level);
}
This seems to be the right intention, but the value you want to modify under this condition is 'level', not 'tmp'.
Ah, of course. Sorry for that.
With this amendment of your patch, things work perfectly fine here.
OK, then perhaps a better fix is to change the check to be equivalent with pineview, as you mentioned in the original post. The handling of bit 0 for old chips was lost during the refactoring of backlight code since 2.6.37.
Does the patch below work for you?
The only concern by this fix is that it changes the max value. If apps expect some certain (e.g. recorded) value, it may screw up. But I don't expect this would happen with sane apps.
thanks,
Takashi
=== From: Takashi Iwai tiwai@suse.de Subject: drm/i915: Fix invalid backpanel values for GEN3 or older chips
While refactoring of backlight control code in commit [a95735569: drm/i915: Refactor panel backlight controls], the handling of the bit 0 of duty-cycle was gone except for pineview. This resulted in invalid register values for old chips like 915GM. When the bit 0 is set, the backlight is turned off suddenly.
This patch changes the bit-0 check by replacing with the condition of gen < 4 (pineview is included in this condition, too).
Reported-by: Daniel Mack zonque@gmail.com Signed-off-by: Takashi Iwai tiwai@suse.de --- drivers/gpu/drm/i915/intel_panel.c | 8 +++----- 1 files changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index 499d4c0..737d00f 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -178,12 +178,10 @@ u32 intel_panel_get_max_backlight(struct drm_device *dev) if (HAS_PCH_SPLIT(dev)) { max >>= 16; } else { - if (IS_PINEVIEW(dev)) { + if (INTEL_INFO(dev)->gen < 4) { max >>= 17; } else { max >>= 16; - if (INTEL_INFO(dev)->gen < 4) - max &= ~1; }
if (is_backlight_combination_mode(dev)) @@ -203,7 +201,7 @@ u32 intel_panel_get_backlight(struct drm_device *dev) val = I915_READ(BLC_PWM_CPU_CTL) & BACKLIGHT_DUTY_CYCLE_MASK; } else { val = I915_READ(BLC_PWM_CTL) & BACKLIGHT_DUTY_CYCLE_MASK; - if (IS_PINEVIEW(dev)) + if (INTEL_INFO(dev)->gen < 4) val >>= 1;
if (is_backlight_combination_mode(dev)) { @@ -246,7 +244,7 @@ static void intel_panel_actually_set_backlight(struct drm_device *dev, u32 level }
tmp = I915_READ(BLC_PWM_CTL); - if (IS_PINEVIEW(dev)) { + if (INTEL_INFO(dev)->gen < 4) { tmp &= ~(BACKLIGHT_DUTY_CYCLE_MASK - 1); level <<= 1; } else
On 11/14/2011 11:39 AM, Takashi Iwai wrote:
[Added Chris to Cc]
At Sun, 13 Nov 2011 17:24:09 +0100, Daniel Mack wrote:
Hi Takashi,
On 11/10/2011 04:39 PM, Takashi Iwai wrote:
At Thu, 10 Nov 2011 16:11:29 +0100, Daniel Mack wrote:
On 11/08/2011 01:57 AM, Daniel Mack wrote:
Didn't get any response yet, hence copying LKML for a broader audience.
Nobody, really?
This is a rather annoying regression, as touching the brightness keys appearantly switches off the whole machine. I'm sure this is trivial to fix, I just don't have the insight of this driver and the chipset.
I vaguely remember that the bit 0 is invalid on some old chips. Maybe 915GM is one of them, as it's gen3? If so, the patch like below may work.
Thank you for looking into this.
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index 499d4c0..be952d1 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -249,8 +249,11 @@ static void intel_panel_actually_set_backlight(struct drm_device *dev, u32 level if (IS_PINEVIEW(dev)) { tmp &= ~(BACKLIGHT_DUTY_CYCLE_MASK - 1); level <<= 1;
- } else
- } else { tmp &= ~BACKLIGHT_DUTY_CYCLE_MASK;
if (INTEL_INFO(dev)->gen < 4)tmp &= ~1;- } I915_WRITE(BLC_PWM_CTL, tmp | level);
}
This seems to be the right intention, but the value you want to modify under this condition is 'level', not 'tmp'.
Ah, of course. Sorry for that.
With this amendment of your patch, things work perfectly fine here.
OK, then perhaps a better fix is to change the check to be equivalent with pineview, as you mentioned in the original post. The handling of bit 0 for old chips was lost during the refactoring of backlight code since 2.6.37.
Does the patch below work for you?
Will test, but I only have occasional access to the machine, so this will have to wait for some days.
The only concern by this fix is that it changes the max value. If apps expect some certain (e.g. recorded) value, it may screw up. But I don't expect this would happen with sane apps.
I don't think so either.
Thanks, Daniel
At Mon, 14 Nov 2011 13:03:46 +0100, Daniel Mack wrote:
On 11/14/2011 11:39 AM, Takashi Iwai wrote:
[Added Chris to Cc]
At Sun, 13 Nov 2011 17:24:09 +0100, Daniel Mack wrote:
Hi Takashi,
On 11/10/2011 04:39 PM, Takashi Iwai wrote:
At Thu, 10 Nov 2011 16:11:29 +0100, Daniel Mack wrote:
On 11/08/2011 01:57 AM, Daniel Mack wrote:
Didn't get any response yet, hence copying LKML for a broader audience.
Nobody, really?
This is a rather annoying regression, as touching the brightness keys appearantly switches off the whole machine. I'm sure this is trivial to fix, I just don't have the insight of this driver and the chipset.
I vaguely remember that the bit 0 is invalid on some old chips. Maybe 915GM is one of them, as it's gen3? If so, the patch like below may work.
Thank you for looking into this.
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index 499d4c0..be952d1 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -249,8 +249,11 @@ static void intel_panel_actually_set_backlight(struct drm_device *dev, u32 level if (IS_PINEVIEW(dev)) { tmp &= ~(BACKLIGHT_DUTY_CYCLE_MASK - 1); level <<= 1;
- } else
- } else { tmp &= ~BACKLIGHT_DUTY_CYCLE_MASK;
if (INTEL_INFO(dev)->gen < 4)tmp &= ~1;- } I915_WRITE(BLC_PWM_CTL, tmp | level);
}
This seems to be the right intention, but the value you want to modify under this condition is 'level', not 'tmp'.
Ah, of course. Sorry for that.
With this amendment of your patch, things work perfectly fine here.
OK, then perhaps a better fix is to change the check to be equivalent with pineview, as you mentioned in the original post. The handling of bit 0 for old chips was lost during the refactoring of backlight code since 2.6.37.
Does the patch below work for you?
Will test, but I only have occasional access to the machine, so this will have to wait for some days.
It's an old bug over a year, so no need to hurry.
Takashi
On 11/14/2011 11:39 AM, Takashi Iwai wrote:
OK, then perhaps a better fix is to change the check to be equivalent with pineview, as you mentioned in the original post. The handling of bit 0 for old chips was lost during the refactoring of backlight code since 2.6.37.
Does the patch below work for you?
The only concern by this fix is that it changes the max value. If apps expect some certain (e.g. recorded) value, it may screw up. But I don't expect this would happen with sane apps.
Works perfectly - let's ship it :)
Thanks again, Daniel
=== From: Takashi Iwai tiwai@suse.de Subject: drm/i915: Fix invalid backpanel values for GEN3 or older chips
While refactoring of backlight control code in commit [a95735569: drm/i915: Refactor panel backlight controls], the handling of the bit 0 of duty-cycle was gone except for pineview. This resulted in invalid register values for old chips like 915GM. When the bit 0 is set, the backlight is turned off suddenly.
This patch changes the bit-0 check by replacing with the condition of gen < 4 (pineview is included in this condition, too).
Reported-by: Daniel Mack zonque@gmail.com Signed-off-by: Takashi Iwai tiwai@suse.de
drivers/gpu/drm/i915/intel_panel.c | 8 +++----- 1 files changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index 499d4c0..737d00f 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -178,12 +178,10 @@ u32 intel_panel_get_max_backlight(struct drm_device *dev) if (HAS_PCH_SPLIT(dev)) { max >>= 16; } else {
if (IS_PINEVIEW(dev)) {
if (INTEL_INFO(dev)->gen < 4) { max >>= 17;
if (INTEL_INFO(dev)->gen < 4)max &= ~1;}
if (is_backlight_combination_mode(dev))
@@ -203,7 +201,7 @@ u32 intel_panel_get_backlight(struct drm_device *dev) val = I915_READ(BLC_PWM_CPU_CTL) & BACKLIGHT_DUTY_CYCLE_MASK; } else { val = I915_READ(BLC_PWM_CTL) & BACKLIGHT_DUTY_CYCLE_MASK;
if (IS_PINEVIEW(dev))
if (INTEL_INFO(dev)->gen < 4) val >>= 1;if (is_backlight_combination_mode(dev)) {
@@ -246,7 +244,7 @@ static void intel_panel_actually_set_backlight(struct drm_device *dev, u32 level }
tmp = I915_READ(BLC_PWM_CTL);
- if (IS_PINEVIEW(dev)) {
- if (INTEL_INFO(dev)->gen < 4) { tmp &= ~(BACKLIGHT_DUTY_CYCLE_MASK - 1); level <<= 1; } else
At Wed, 16 Nov 2011 13:58:57 +0100, Daniel Mack wrote:
On 11/14/2011 11:39 AM, Takashi Iwai wrote:
OK, then perhaps a better fix is to change the check to be equivalent with pineview, as you mentioned in the original post. The handling of bit 0 for old chips was lost during the refactoring of backlight code since 2.6.37.
Does the patch below work for you?
The only concern by this fix is that it changes the max value. If apps expect some certain (e.g. recorded) value, it may screw up. But I don't expect this would happen with sane apps.
Works perfectly - let's ship it :)
OK, now the patch was resent to intel-gfx with proper tags.
thanks,
Takashi
dri-devel@lists.freedesktop.org
-
Daniel Mack -
Takashi Iwai