Re: Strange effect with i915 backlight controller

Re: Strange effect with i915 backlight controller

[Daniel Mack]

[-- Attachment #1: Type: text/plain, Size: 1113 bytes --]

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


[-- Attachment #2: lspci-v-x20.txt --]
[-- Type: text/plain, Size: 7996 bytes --]

00:00.0 Host bridge: Intel Corporation Mobile 915GM/PM/GMS/910GML Express Processor to DRAM Controller (rev 03)
	Subsystem: Samsung Electronics Co Ltd Device c01a
	Flags: bus master, fast devsel, latency 0
	Capabilities: [e0] Vendor Specific Information: Len=09 <?>
	Kernel driver in use: agpgart-intel

00:02.0 VGA compatible controller: Intel Corporation Mobile 915GM/GMS/910GML Express Graphics Controller (rev 03) (prog-if 00 [VGA controller])
	Subsystem: Samsung Electronics Co Ltd Device c01a
	Flags: bus master, fast devsel, latency 0, IRQ 16
	Memory at b0080000 (32-bit, non-prefetchable) [size=512K]
	I/O ports at 1800 [size=8]
	Memory at c0000000 (32-bit, prefetchable) [size=256M]
	Memory at b0000000 (32-bit, non-prefetchable) [size=256K]
	Expansion ROM at <unassigned> [disabled]
	Capabilities: [d0] Power Management version 2
	Kernel driver in use: i915
	Kernel modules: intelfb, i915

00:02.1 Display controller: Intel Corporation Mobile 915GM/GMS/910GML Express Graphics Controller (rev 03)
	Subsystem: Samsung Electronics Co Ltd Device c01a
	Flags: fast devsel
	Memory at 42000000 (32-bit, non-prefetchable) [disabled] [size=512K]
	Capabilities: [d0] Power Management version 2

00:1c.0 PCI bridge: Intel Corporation 82801FB/FBM/FR/FW/FRW (ICH6 Family) PCI Express Port 1 (rev 03) (prog-if 00 [Normal decode])
	Flags: bus master, fast devsel, latency 0
	Bus: primary=00, secondary=01, subordinate=01, sec-latency=0
	I/O behind bridge: 00003000-00003fff
	Memory behind bridge: b4000000-b7ffffff
	Prefetchable memory behind bridge: 00000000d0000000-00000000d3ffffff
	Capabilities: [40] Express Root Port (Slot+), MSI 00
	Capabilities: [80] MSI: Enable+ Count=1/1 Maskable- 64bit-
	Capabilities: [90] Subsystem: Samsung Electronics Co Ltd Device c01a
	Capabilities: [a0] Power Management version 2
	Capabilities: [100] Virtual Channel
	Capabilities: [180] Root Complex Link
	Kernel driver in use: pcieport
	Kernel modules: shpchp

00:1d.0 USB Controller: Intel Corporation 82801FB/FBM/FR/FW/FRW (ICH6 Family) USB UHCI #1 (rev 03) (prog-if 00 [UHCI])
	Subsystem: Samsung Electronics Co Ltd Device c01a
	Flags: bus master, medium devsel, latency 0, IRQ 23
	I/O ports at 1820 [size=32]
	Kernel driver in use: uhci_hcd

00:1d.1 USB Controller: Intel Corporation 82801FB/FBM/FR/FW/FRW (ICH6 Family) USB UHCI #2 (rev 03) (prog-if 00 [UHCI])
	Subsystem: Samsung Electronics Co Ltd Device c01a
	Flags: bus master, medium devsel, latency 0, IRQ 19
	I/O ports at 1840 [size=32]
	Kernel driver in use: uhci_hcd

00:1d.2 USB Controller: Intel Corporation 82801FB/FBM/FR/FW/FRW (ICH6 Family) USB UHCI #3 (rev 03) (prog-if 00 [UHCI])
	Subsystem: Samsung Electronics Co Ltd Device c01a
	Flags: bus master, medium devsel, latency 0, IRQ 18
	I/O ports at 1860 [size=32]
	Kernel driver in use: uhci_hcd

00:1d.3 USB Controller: Intel Corporation 82801FB/FBM/FR/FW/FRW (ICH6 Family) USB UHCI #4 (rev 03) (prog-if 00 [UHCI])
	Subsystem: Samsung Electronics Co Ltd Device c01a
	Flags: bus master, medium devsel, latency 0, IRQ 16
	I/O ports at 1880 [size=32]
	Kernel driver in use: uhci_hcd

00:1d.7 USB Controller: Intel Corporation 82801FB/FBM/FR/FW/FRW (ICH6 Family) USB2 EHCI Controller (rev 03) (prog-if 20 [EHCI])
	Subsystem: Samsung Electronics Co Ltd Device c01a
	Flags: bus master, medium devsel, latency 0, IRQ 23
	Memory at b0040000 (32-bit, non-prefetchable) [size=1K]
	Capabilities: [50] Power Management version 2
	Capabilities: [58] Debug port: BAR=1 offset=00a0
	Kernel driver in use: ehci_hcd

00:1e.0 PCI bridge: Intel Corporation 82801 Mobile PCI Bridge (rev d3) (prog-if 01 [Subtractive decode])
	Flags: bus master, fast devsel, latency 0
	Bus: primary=00, secondary=02, subordinate=06, sec-latency=32
	I/O behind bridge: 00004000-00004fff
	Memory behind bridge: b8000000-b80fffff
	Prefetchable memory behind bridge: 0000000044000000-0000000049ffffff
	Capabilities: [50] Subsystem: Gammagraphx, Inc. (or missing ID) Device 0000

00:1e.2 Multimedia audio controller: Intel Corporation 82801FB/FBM/FR/FW/FRW (ICH6 Family) AC'97 Audio Controller (rev 03)
	Subsystem: Samsung Electronics Co Ltd Device c01a
	Flags: bus master, medium devsel, latency 0, IRQ 17
	I/O ports at 1c00 [size=256]
	I/O ports at 18c0 [size=64]
	Memory at b0040800 (32-bit, non-prefetchable) [size=512]
	Memory at b0040400 (32-bit, non-prefetchable) [size=256]
	Capabilities: [50] Power Management version 2
	Kernel driver in use: snd_intel8x0
	Kernel modules: snd-intel8x0

00:1e.3 Modem: Intel Corporation 82801FB/FBM/FR/FW/FRW (ICH6 Family) AC'97 Modem Controller (rev 03) (prog-if 00 [Generic])
	Subsystem: Samsung Electronics Co Ltd Device 2115
	Flags: medium devsel, IRQ 20
	I/O ports at 2400 [size=256]
	I/O ports at 2000 [size=128]
	Capabilities: [50] Power Management version 2
	Kernel modules: snd-intel8x0m

00:1f.0 ISA bridge: Intel Corporation 82801FBM (ICH6M) LPC Interface Bridge (rev 03)
	Subsystem: Samsung Electronics Co Ltd Device c01a
	Flags: bus master, medium devsel, latency 0
	Kernel modules: iTCO_wdt, intel-rng

00:1f.1 IDE interface: Intel Corporation 82801FB/FBM/FR/FW/FRW (ICH6 Family) IDE Controller (rev 03) (prog-if 8a [Master SecP PriP])
	Subsystem: Samsung Electronics Co Ltd Device c01a
	Flags: bus master, medium devsel, latency 0, IRQ 18
	I/O ports at 01f0 [size=8]
	I/O ports at 03f4 [size=1]
	I/O ports at 0170 [size=8]
	I/O ports at 0374 [size=1]
	I/O ports at 1810 [size=16]
	Kernel driver in use: ata_piix

00:1f.3 SMBus: Intel Corporation 82801FB/FBM/FR/FW/FRW (ICH6 Family) SMBus Controller (rev 03)
	Subsystem: Samsung Electronics Co Ltd Device c01a
	Flags: medium devsel, IRQ 5
	I/O ports at 18a0 [size=32]
	Kernel modules: i2c-i801

02:05.0 Ethernet controller: Broadcom Corporation BCM4401-B0 100Base-TX (rev 02)
	Subsystem: Samsung Electronics Co Ltd Device c01a
	Flags: bus master, fast devsel, latency 32, IRQ 22
	Memory at b8000000 (32-bit, non-prefetchable) [size=8K]
	[virtual] Expansion ROM at 44000000 [disabled] [size=64K]
	Capabilities: [40] Power Management version 2
	Kernel driver in use: b44
	Kernel modules: b44

02:07.0 Network controller: Intel Corporation PRO/Wireless 2200BG [Calexico2] Network Connection (rev 05)
	Subsystem: Intel Corporation Samsung P35 integrated WLAN
	Flags: bus master, medium devsel, latency 32, IRQ 20
	Memory at b8002000 (32-bit, non-prefetchable) [size=4K]
	Capabilities: [dc] Power Management version 2
	Kernel driver in use: ipw2200
	Kernel modules: ipw2200

02:09.0 CardBus bridge: Ricoh Co Ltd RL5c476 II (rev b3)
	Subsystem: Samsung Electronics Co Ltd Device c01a
	Flags: bus master, medium devsel, latency 168, IRQ 16
	Memory at 000da000 (32-bit, non-prefetchable) [size=4K]
	Bus: primary=02, secondary=03, subordinate=06, sec-latency=176
	Memory window 0: 50000000-53fff000 (prefetchable)
	Memory window 1: 4c000000-4ffff000
	I/O window 0: 00004400-000044ff
	I/O window 1: 00004000-000040ff
	16-bit legacy interface ports at 0001
	Kernel driver in use: yenta_cardbus
	Kernel modules: yenta_socket

02:09.1 FireWire (IEEE 1394): Ricoh Co Ltd R5C552 IEEE 1394 Controller (rev 08) (prog-if 10 [OHCI])
	Subsystem: Samsung Electronics Co Ltd Device c01a
	Flags: bus master, medium devsel, latency 64, IRQ 17
	Memory at b8003000 (32-bit, non-prefetchable) [size=2K]
	Capabilities: [dc] Power Management version 2
	Kernel driver in use: firewire_ohci
	Kernel modules: firewire-ohci

02:09.2 SD Host controller: Ricoh Co Ltd R5C822 SD/SDIO/MMC/MS/MSPro Host Adapter (rev 17)
	Subsystem: Samsung Electronics Co Ltd Device c01a
	Flags: bus master, medium devsel, latency 32, IRQ 18
	Memory at b8003800 (32-bit, non-prefetchable) [size=256]
	Capabilities: [80] Power Management version 2
	Kernel driver in use: sdhci-pci
	Kernel modules: sdhci-pci

02:09.3 System peripheral: Ricoh Co Ltd R5C592 Memory Stick Bus Host Adapter (rev 08)
	Subsystem: Samsung Electronics Co Ltd Device c01a
	Flags: medium devsel, IRQ 255
	Memory at b8003c00 (32-bit, non-prefetchable) [size=256]
	Capabilities: [80] Power Management version 2

Re: Strange effect with i915 backlight controller

[Daniel Mack]

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
> 

Re: Strange effect with i915 backlight controller

[Takashi Iwai]

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);
 }
 

Re: Strange effect with i915 backlight controller

[Daniel Mack]

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

Re: Strange effect with i915 backlight controller

[Takashi Iwai]

[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
-- 
1.7.7

Re: Strange effect with i915 backlight controller

[Daniel Mack]

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

Re: Strange effect with i915 backlight controller

[Takashi Iwai]

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

Re: Strange effect with i915 backlight controller

[Daniel Mack]

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;
>  		} 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

Re: Strange effect with i915 backlight controller

[Takashi Iwai]

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