Re: Linux 2.6.37

Re: Linux 2.6.37

[Michal Hocko]

Hi,

On Tue 04-01-11 17:15:45, Linus Torvalds wrote:
[...]
> We did have another revert to fix hopefullythe last "blank screen"
> regression on intel graphics.

It seems that there is still a regression for intel graphic cards
backlight. One report is https://bugzilla.kernel.org/show_bug.cgi?id=22672.
I can reproduce the problem easily by:
xset dpms force standby; sleep 3s; xset dpms force on

backlight doesn't get up (there is really dark picture though which
doesn't get brighter by function keys which work normally) after dpms on
until I close and open lid. 

The problem wasn't present in 2.6.36

$ lspci -vv
[...]
00:02.0 VGA compatible controller: Intel Corporation Mobile 945GM/GMS, 943/940GML Express Integrated Graphics Controller (rev 03) (prog-if 00 [VGA controller])
        Subsystem: Fujitsu Limited. Device 137a
        Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
        Status: Cap+ 66MHz- UDF- FastB2B+ ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
        Latency: 0
        Interrupt: pin A routed to IRQ 16
        Region 0: Memory at f0300000 (32-bit, non-prefetchable) [size=512K]
        Region 1: I/O ports at 1800 [size=8]
        Region 2: Memory at e0000000 (32-bit, prefetchable) [size=256M]
        Region 3: Memory at f0400000 (32-bit, non-prefetchable) [size=256K]
        Expansion ROM at <unassigned> [disabled]
        Capabilities: <access denied>
        Kernel driver in use: i915
[...]
-- 
Michal Hocko
L3 team 
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Re: Linux 2.6.37

[Linus Torvalds]

On Thu, Jan 6, 2011 at 2:48 AM, Michal Hocko <mhocko@suse.cz> wrote:
>
> It seems that there is still a regression for intel graphic cards
> backlight. One report is https://bugzilla.kernel.org/show_bug.cgi?id=22672.
> I can reproduce the problem easily by:
> xset dpms force standby; sleep 3s; xset dpms force on
>
> backlight doesn't get up (there is really dark picture though which
> doesn't get brighter by function keys which work normally) after dpms on
> until I close and open lid.

Hmm. That commit no longer reverts cleanly, so it's not trivial to
test whether all those things are exactly the same issue. It's been
bisected in the bugzilla entry, but it would be good to verify that
yes, reverting it really does fix the issue, and your issue is the
exact same one.

Chris, any ideas?

                       Linus

Re: Linux 2.6.37

[Michal Hocko]

Just for reference, my initial report was:
https://lkml.org/lkml/2010/11/23/146

On Thu 06-01-11 08:29:22, Linus Torvalds wrote:
> On Thu, Jan 6, 2011 at 2:48 AM, Michal Hocko <mhocko@suse.cz> wrote:
> >
> > It seems that there is still a regression for intel graphic cards
> > backlight. One report is https://bugzilla.kernel.org/show_bug.cgi?id=22672.
> > I can reproduce the problem easily by:
> > xset dpms force standby; sleep 3s; xset dpms force on
> >
> > backlight doesn't get up (there is really dark picture though which
> > doesn't get brighter by function keys which work normally) after dpms on
> > until I close and open lid.
> 
> Hmm. That commit no longer reverts cleanly, so it's not trivial to
> test whether all those things are exactly the same issue. It's been
> bisected in the bugzilla entry, but it would be good to verify that
> yes, reverting it really does fix the issue, and your issue is the
> exact same one.
> 
> Chris, any ideas?
> 
>                        Linus

-- 
Michal Hocko
L3 team 
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Re: Linux 2.6.37

[Chris Wilson]

On Thu, 6 Jan 2011 08:29:22 -0800, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> On Thu, Jan 6, 2011 at 2:48 AM, Michal Hocko <mhocko@suse.cz> wrote:
> >
> > It seems that there is still a regression for intel graphic cards
> > backlight. One report is https://bugzilla.kernel.org/show_bug.cgi?id=22672.
> > I can reproduce the problem easily by:
> > xset dpms force standby; sleep 3s; xset dpms force on
> >
> > backlight doesn't get up (there is really dark picture though which
> > doesn't get brighter by function keys which work normally) after dpms on
> > until I close and open lid.
> 
> Hmm. That commit no longer reverts cleanly, so it's not trivial to
> test whether all those things are exactly the same issue. It's been
> bisected in the bugzilla entry, but it would be good to verify that
> yes, reverting it really does fix the issue, and your issue is the
> exact same one.
> 
> Chris, any ideas?

My fear is that some machines have a dependency between the backlight
and panel power status. The patch in question changed the timing between
turning on the panel and adjusting the backlight which would be restore
with:

diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index aa23070..0b40b4f 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -106,6 +106,12 @@ static void intel_lvds_enable(struct intel_lvds *intel_lvds)
 	I915_WRITE(ctl_reg, I915_READ(ctl_reg) | POWER_TARGET_ON);
 	POSTING_READ(lvds_reg);
 
+	{
+		u32 reg = HAS_PCH_SPLIT(dev) ? PCH_PP_STATUS : PPS_STATUS;
+		if (wait_for(I915_READ(reg) & PP_ON, 1000))
+			DRM_ERROR("timed out waiting for panel to power up\n");
+	}
+
 	intel_panel_set_backlight(dev, dev_priv->backlight_level);
 }

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: Linux 2.6.37

[Alex Riesen]

On Thu, Jan 6, 2011 at 18:49, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>
> My fear is that some machines have a dependency between the backlight
> and panel power status. The patch in question changed the timing between
> turning on the panel and adjusting the backlight which would be restore
> with:
>
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index aa23070..0b40b4f 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -106,6 +106,12 @@ static void intel_lvds_enable(struct intel_lvds *intel_lvds)
>        I915_WRITE(ctl_reg, I915_READ(ctl_reg) | POWER_TARGET_ON);
>        POSTING_READ(lvds_reg);
>
> +       {
> +               u32 reg = HAS_PCH_SPLIT(dev) ? PCH_PP_STATUS : PPS_STATUS;
> +               if (wait_for(I915_READ(reg) & PP_ON, 1000))
> +                       DRM_ERROR("timed out waiting for panel to power up\n");
> +       }
> +
>        intel_panel_set_backlight(dev, dev_priv->backlight_level);
>  }

FWIW it does not compile:
  CC      drivers/gpu/drm/i915/intel_lvds.o
drivers/gpu/drm/i915/intel_lvds.c: In function ‘intel_lvds_enable’:
drivers/gpu/drm/i915/intel_lvds.c:110: error: ‘PPS_STATUS’ undeclared
(first use in this function)
drivers/gpu/drm/i915/intel_lvds.c:110: error: (Each undeclared
identifier is reported only once
drivers/gpu/drm/i915/intel_lvds.c:110: error: for each function it appears in.)
make[4]: *** [drivers/gpu/drm/i915/intel_lvds.o] Error 1

Re: Linux 2.6.37

[Chris Wilson]

On Thu, 6 Jan 2011 21:55:23 +0100, Alex Riesen <raa.lkml@gmail.com> wrote:
> On Thu, Jan 6, 2011 at 18:49, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> >
> > My fear is that some machines have a dependency between the backlight
> > and panel power status. The patch in question changed the timing between
> > turning on the panel and adjusting the backlight which would be restore
> > with:
> >
> > diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> > index aa23070..0b40b4f 100644
> > --- a/drivers/gpu/drm/i915/intel_lvds.c
> > +++ b/drivers/gpu/drm/i915/intel_lvds.c
> > @@ -106,6 +106,12 @@ static void intel_lvds_enable(struct intel_lvds *intel_lvds)
> >        I915_WRITE(ctl_reg, I915_READ(ctl_reg) | POWER_TARGET_ON);
> >        POSTING_READ(lvds_reg);
> >
> > +       {
> > +               u32 reg = HAS_PCH_SPLIT(dev) ? PCH_PP_STATUS : PPS_STATUS;
> > +               if (wait_for(I915_READ(reg) & PP_ON, 1000))
> > +                       DRM_ERROR("timed out waiting for panel to power up\n");
> > +       }
> > +
> >        intel_panel_set_backlight(dev, dev_priv->backlight_level);
> >  }
> 
> FWIW it does not compile:
>   CC      drivers/gpu/drm/i915/intel_lvds.o
> drivers/gpu/drm/i915/intel_lvds.c: In function ‘intel_lvds_enable’:
> drivers/gpu/drm/i915/intel_lvds.c:110: error: ‘PPS_STATUS’ undeclared
> (first use in this function)
> drivers/gpu/drm/i915/intel_lvds.c:110: error: (Each undeclared
> identifier is reported only once
> drivers/gpu/drm/i915/intel_lvds.c:110: error: for each function it appears in.)
> make[4]: *** [drivers/gpu/drm/i915/intel_lvds.o] Error 1

Daniel quickly pointed out my typo: s/PPS_STATUS/PP_STATUS/

Apologies,
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: Linux 2.6.37

[Alex Riesen]

On Thu, Jan 6, 2011 at 21:55, Alex Riesen <raa.lkml@gmail.com> wrote:
> On Thu, Jan 6, 2011 at 18:49, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>>
>> My fear is that some machines have a dependency between the backlight
>> and panel power status. The patch in question changed the timing between
>> turning on the panel and adjusting the backlight which would be restore
>> with:
>>
>> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
>> index aa23070..0b40b4f 100644
>> --- a/drivers/gpu/drm/i915/intel_lvds.c
>> +++ b/drivers/gpu/drm/i915/intel_lvds.c
>> @@ -106,6 +106,12 @@ static void intel_lvds_enable(struct intel_lvds *intel_lvds)
>>        I915_WRITE(ctl_reg, I915_READ(ctl_reg) | POWER_TARGET_ON);
>>        POSTING_READ(lvds_reg);
>>
>> +       {
>> +               u32 reg = HAS_PCH_SPLIT(dev) ? PCH_PP_STATUS : PPS_STATUS;
...
> FWIW it does not compile:
>  CC      drivers/gpu/drm/i915/intel_lvds.o
> drivers/gpu/drm/i915/intel_lvds.c: In function ‘intel_lvds_enable’:
> drivers/gpu/drm/i915/intel_lvds.c:110: error: ‘PPS_STATUS’ undeclared

Ah, I see. Should be PP_STATUS. Whatever. It does not help. The backlight
stays off.

P.S. Probably unrelated, but I just noticed that the backlight never goes
off when closing the lid. Am I supposed to hook up on the corresponding
input event and put the panel in standby? It used to work all by itself,
I think...

Re: Linux 2.6.37

[Michal Hocko]

On Thu 06-01-11 22:08:46, Alex Riesen wrote:
> On Thu, Jan 6, 2011 at 21:55, Alex Riesen <raa.lkml@gmail.com> wrote:
> > On Thu, Jan 6, 2011 at 18:49, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> >>
> >> My fear is that some machines have a dependency between the backlight
> >> and panel power status. The patch in question changed the timing between
> >> turning on the panel and adjusting the backlight which would be restore
> >> with:
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> >> index aa23070..0b40b4f 100644
> >> --- a/drivers/gpu/drm/i915/intel_lvds.c
> >> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> >> @@ -106,6 +106,12 @@ static void intel_lvds_enable(struct intel_lvds *intel_lvds)
> >> ?? ?? ?? ??I915_WRITE(ctl_reg, I915_READ(ctl_reg) | POWER_TARGET_ON);
> >> ?? ?? ?? ??POSTING_READ(lvds_reg);
> >>
> >> + ?? ?? ?? {
> >> + ?? ?? ?? ?? ?? ?? ?? u32 reg = HAS_PCH_SPLIT(dev) ? PCH_PP_STATUS : PPS_STATUS;
> ...
> > FWIW it does not compile:
> > ??CC ?? ?? ??drivers/gpu/drm/i915/intel_lvds.o
> > drivers/gpu/drm/i915/intel_lvds.c: In function ???intel_lvds_enable???:
> > drivers/gpu/drm/i915/intel_lvds.c:110: error: ???PPS_STATUS??? undeclared
> 
> Ah, I see. Should be PP_STATUS. Whatever. It does not help. The backlight
> stays off.

It didn't help in my case either

-- 
Michal Hocko
L3 team 
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Re: Linux 2.6.37

[Pavel Machek]

> Hi,
> 
> On Tue 04-01-11 17:15:45, Linus Torvalds wrote:
> [...]
> > We did have another revert to fix hopefullythe last "blank screen"
> > regression on intel graphics.
> 
> It seems that there is still a regression for intel graphic cards
> backlight. One report is https://bugzilla.kernel.org/show_bug.cgi?id=22672.
> I can reproduce the problem easily by:
> xset dpms force standby; sleep 3s; xset dpms force on

(You are using "vesa" or "fbcon" X11 driver, right? I seen same problem
until I switched to "intel" X11 driver).

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: Linux 2.6.37

[Michal Hocko]

On Tue 11-01-11 14:33:20, Pavel Machek wrote:
> > Hi,
> > 
> > On Tue 04-01-11 17:15:45, Linus Torvalds wrote:
> > [...]
> > > We did have another revert to fix hopefullythe last "blank screen"
> > > regression on intel graphics.
> > 
> > It seems that there is still a regression for intel graphic cards
> > backlight. One report is https://bugzilla.kernel.org/show_bug.cgi?id=22672.
> > I can reproduce the problem easily by:
> > xset dpms force standby; sleep 3s; xset dpms force on
> 
> (You are using "vesa" or "fbcon" X11 driver, right? I seen same problem
> until I switched to "intel" X11 driver).

I am using intel X11 driver:
[...]
(II) Loading extension DRI2
(II) LoadModule: "intel"
(II) Loading /usr/lib/xorg/modules/drivers/intel_drv.so
[...]

So this doesn't look like the case.

-- 
Michal Hocko
L3 team 
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Re: Linux 2.6.37

[Alex Riesen]

On Tue, Jan 11, 2011 at 14:33, Pavel Machek <pavel@ucw.cz> wrote:
>> I can reproduce the problem easily by:
>> xset dpms force standby; sleep 3s; xset dpms force on
>
> (You are using "vesa" or "fbcon" X11 driver, right? I seen same problem
> until I switched to "intel" X11 driver).

No, the "intel".

Re: Linux 2.6.37

[Michal Hocko]

[Let's CC Indan - author of the bugzilla patches]

On Thu 06-01-11 11:48:16, Michal Hocko wrote:
> Hi,
> 
> On Tue 04-01-11 17:15:45, Linus Torvalds wrote:
> [...]
> > We did have another revert to fix hopefullythe last "blank screen"
> > regression on intel graphics.
> 
> It seems that there is still a regression for intel graphic cards
> backlight. One report is https://bugzilla.kernel.org/show_bug.cgi?id=22672.
> I can reproduce the problem easily by:
> xset dpms force standby; sleep 3s; xset dpms force on
> 
> backlight doesn't get up (there is really dark picture though which
> doesn't get brighter by function keys which work normally) after dpms on
> until I close and open lid. 
> 
> The problem wasn't present in 2.6.36

I can confirm that this problem is not present if both patches from
bko#22672 are applied on top of 2.6.37 kernel.
I haven't tried both patches separately, but I can surely try it if it
makes any sense.

> 
> $ lspci -vv
> [...]
> 00:02.0 VGA compatible controller: Intel Corporation Mobile 945GM/GMS, 943/940GML Express Integrated Graphics Controller (rev 03) (prog-if 00 [VGA controller])
>         Subsystem: Fujitsu Limited. Device 137a
>         Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
>         Status: Cap+ 66MHz- UDF- FastB2B+ ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
>         Latency: 0
>         Interrupt: pin A routed to IRQ 16
>         Region 0: Memory at f0300000 (32-bit, non-prefetchable) [size=512K]
>         Region 1: I/O ports at 1800 [size=8]
>         Region 2: Memory at e0000000 (32-bit, prefetchable) [size=256M]
>         Region 3: Memory at f0400000 (32-bit, non-prefetchable) [size=256K]
>         Expansion ROM at <unassigned> [disabled]
>         Capabilities: <access denied>
>         Kernel driver in use: i915
[...]

-- 
Michal Hocko
L3 team 
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Re: Linux 2.6.37

[Michal Hocko]

On Tue 11-01-11 18:17:44, Michal Hocko wrote:
> [Let's CC Indan - author of the bugzilla patches]
> 
> On Thu 06-01-11 11:48:16, Michal Hocko wrote:
> > Hi,
> > 
> > On Tue 04-01-11 17:15:45, Linus Torvalds wrote:
> > [...]
> > > We did have another revert to fix hopefullythe last "blank screen"
> > > regression on intel graphics.
> > 
> > It seems that there is still a regression for intel graphic cards
> > backlight. One report is https://bugzilla.kernel.org/show_bug.cgi?id=22672.
> > I can reproduce the problem easily by:
> > xset dpms force standby; sleep 3s; xset dpms force on
> > 
> > backlight doesn't get up (there is really dark picture though which
> > doesn't get brighter by function keys which work normally) after dpms on
> > until I close and open lid. 
> > 
> > The problem wasn't present in 2.6.36
> 
> I can confirm that this problem is not present if both patches from
> bko#22672 are applied on top of 2.6.37 kernel.
> I haven't tried both patches separately, but I can surely try it if it
> makes any sense.

I have just tried that and they are really both necessary.


-- 
Michal Hocko
L3 team 
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Re: Linux 2.6.37

[Michal Hocko]

On Tue 11-01-11 18:37:41, Michal Hocko wrote:
> On Tue 11-01-11 18:17:44, Michal Hocko wrote:
> > [Let's CC Indan - author of the bugzilla patches]
> > 
> > On Thu 06-01-11 11:48:16, Michal Hocko wrote:
> > > Hi,
> > > 
> > > On Tue 04-01-11 17:15:45, Linus Torvalds wrote:
> > > [...]
> > > > We did have another revert to fix hopefullythe last "blank screen"
> > > > regression on intel graphics.
> > > 
> > > It seems that there is still a regression for intel graphic cards
> > > backlight. One report is https://bugzilla.kernel.org/show_bug.cgi?id=22672.
> > > I can reproduce the problem easily by:
> > > xset dpms force standby; sleep 3s; xset dpms force on
> > > 
> > > backlight doesn't get up (there is really dark picture though which
> > > doesn't get brighter by function keys which work normally) after dpms on
> > > until I close and open lid. 
> > > 
> > > The problem wasn't present in 2.6.36
> > 
> > I can confirm that this problem is not present if both patches from
> > bko#22672 are applied on top of 2.6.37 kernel.
> > I haven't tried both patches separately, but I can surely try it if it
> > makes any sense.
> 
> I have just tried that and they are really both necessary.

I must have mixed something. After retesting with the first patch
(https://bugzilla.kernel.org/show_bug.cgi?id=22672#c33) the usecase
works just fine (backlight is on without need to close&open the lid)

Sorry for confusion

-- 
Michal Hocko
L3 team 
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Re: Linux 2.6.37

[Chris Wilson]

On Tue, 11 Jan 2011 18:19:17 +0100, Michal Hocko <mhocko@suse.cz> wrote:
> [Let's CC Indan - author of the bugzilla patches]
> 
> On Thu 06-01-11 11:48:16, Michal Hocko wrote:
> > Hi,
> > 
> > On Tue 04-01-11 17:15:45, Linus Torvalds wrote:
> > [...]
> > > We did have another revert to fix hopefullythe last "blank screen"
> > > regression on intel graphics.
> > 
> > It seems that there is still a regression for intel graphic cards
> > backlight. One report is https://bugzilla.kernel.org/show_bug.cgi?id=22672.
> > I can reproduce the problem easily by:
> > xset dpms force standby; sleep 3s; xset dpms force on
> > 
> > backlight doesn't get up (there is really dark picture though which
> > doesn't get brighter by function keys which work normally) after dpms on
> > until I close and open lid. 
> > 
> > The problem wasn't present in 2.6.36
> 
> I can confirm that this problem is not present if both patches from
> bko#22672 are applied on top of 2.6.37 kernel.
> I haven't tried both patches separately, but I can surely try it if it
> makes any sense.

The second patch is wrong in that it will prevent changing resolutions
using the panel fitter. The first patch looks along the right lines, just
misses the possibility that the backlight can be modified by other means.

So hopefully, you just need the first patch.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: Linux 2.6.37

[Michal Hocko]

On Tue 11-01-11 17:39:42, Chris Wilson wrote:
> On Tue, 11 Jan 2011 18:19:17 +0100, Michal Hocko <mhocko@suse.cz> wrote:
> > [Let's CC Indan - author of the bugzilla patches]
> > 
> > On Thu 06-01-11 11:48:16, Michal Hocko wrote:
> > > Hi,
> > > 
> > > On Tue 04-01-11 17:15:45, Linus Torvalds wrote:
> > > [...]
> > > > We did have another revert to fix hopefullythe last "blank screen"
> > > > regression on intel graphics.
> > > 
> > > It seems that there is still a regression for intel graphic cards
> > > backlight. One report is https://bugzilla.kernel.org/show_bug.cgi?id=22672.
> > > I can reproduce the problem easily by:
> > > xset dpms force standby; sleep 3s; xset dpms force on
> > > 
> > > backlight doesn't get up (there is really dark picture though which
> > > doesn't get brighter by function keys which work normally) after dpms on
> > > until I close and open lid. 
> > > 
> > > The problem wasn't present in 2.6.36
> > 
> > I can confirm that this problem is not present if both patches from
> > bko#22672 are applied on top of 2.6.37 kernel.
> > I haven't tried both patches separately, but I can surely try it if it
> > makes any sense.
> 
> The second patch is wrong in that it will prevent changing resolutions
> using the panel fitter. The first patch looks along the right lines, just
> misses the possibility that the backlight can be modified by other means.
> 
> So hopefully, you just need the first patch.

I would have bet I have tested the 1st patch but let me double check.
The 2nd patch alone doesn't fix the problem.

> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

-- 
Michal Hocko
L3 team 
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Re: Linux 2.6.37

[Indan Zupancic]

Hello,

On Tue, January 11, 2011 18:39, Chris Wilson wrote:
> On Tue, 11 Jan 2011 18:19:17 +0100, Michal Hocko <mhocko@suse.cz> wrote:
>> [Let's CC Indan - author of the bugzilla patches]
>>
>> On Thu 06-01-11 11:48:16, Michal Hocko wrote:
>> > Hi,
>> >
>> > On Tue 04-01-11 17:15:45, Linus Torvalds wrote:
>> > [...]
>> > > We did have another revert to fix hopefullythe last "blank screen"
>> > > regression on intel graphics.
>> >
>> > It seems that there is still a regression for intel graphic cards
>> > backlight. One report is https://bugzilla.kernel.org/show_bug.cgi?id=22672.
>> > I can reproduce the problem easily by:
>> > xset dpms force standby; sleep 3s; xset dpms force on
>> >
>> > backlight doesn't get up (there is really dark picture though which
>> > doesn't get brighter by function keys which work normally) after dpms on
>> > until I close and open lid.
>> >
>> > The problem wasn't present in 2.6.36
>>
>> I can confirm that this problem is not present if both patches from
>> bko#22672 are applied on top of 2.6.37 kernel.
>> I haven't tried both patches separately, but I can surely try it if it
>> makes any sense.
>
> The second patch is wrong in that it will prevent changing resolutions
> using the panel fitter.

I can confirm that with the second patch changing resolutions doesn't always go right.

$ xrandr
Screen 0: minimum 320 x 200, current 1024 x 768, maximum 2048 x 2048
LVDS1 connected 1024x768+0+0 (normal left inverted right x axis y axis) 0mm x 0mm
   1024x768       50.0*+   85.0     75.0     70.1     60.0
   832x624        74.6
   800x600        85.1     72.2     75.0     60.3     56.2
   640x480        85.0     72.8     75.0     59.9
   720x400        85.0
   640x400        85.1
   640x350        85.1
VGA1 disconnected (normal left inverted right x axis y axis)

Going from xrandr -s 1, 2 or 3 back to 0 works, but not from 4, 5 or 6 to 0.

The second patch was really a stab in the dark, I'm happy it was in the right
direction at least.

> The first patch looks along the right lines, just
> misses the possibility that the backlight can be modified by other means.

I'm not sure about that. All it does is avoiding setting backlight_level to
0. If it's modified some other way and intel_panel_get_backlight() returns 0,
backlight_level is just never set and will stay zero. If there is a way to
switch between ways to modify the backlight then I can see how this may not
always work, because then backlight_level is stuck on the last non-zero value
before it was switched to intel_panel_set_backlight(0) and the different method.

Alex reported a slightly dimmer display after resume, so I wonder what I missed
in my patch. Larry's problem was probably fixed with just the first patch, but
then I wonder why he reported that it didn't work first. Maybe he meant the
setpci thing, or made a mistake.

> So hopefully, you just need the first patch.
> -Chris

Yeah, the second patch is a bit of a desperate attempt because Larry reported that
it didn't fix his problem.

About your patch, you still do:

+void intel_panel_setup_backlight(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	dev_priv->backlight_level = intel_panel_get_max_backlight(dev);
+	dev_priv->backlight_enabled = dev_priv->backlight_level != 0;
+}

While my patch changes that to:

+	u32 level;

-	if (dev_priv->backlight_level == 0)
-		dev_priv->backlight_level = intel_panel_get_max_backlight(dev);
+	if (dev_priv->backlight_level == 0) {
+		level = intel_panel_get_backlight(dev);
+		if (level == 0)
+			level = intel_panel_get_max_backlight(dev);
+		dev_priv->backlight_level = level;
+	}

Your patch uses intel_panel_get_max_backlight() to check if the backlight is
enabled. Is this always correct, or may it cause bugs in the future?

Anyway, my main concern with unconditionally setting the backlight level to
the maximum is that any stored brightness level (by the BIOS or whatever) may
be lost, and that the screen is set to maximum brightness at each boot. This
is certainly the behaviour I've seen with an unpatched kernel. So I propose to
do what my patch does and set it to intel_panel_get_backlight(dev) if that
returns non-zero. Something like this:

+void intel_panel_setup_backlight(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	u32 max, level;
+
+	max = intel_panel_get_max_backlight(dev);
+	if (max) {
+		dev_priv->backlight_enabled = 1;
+		level = intel_panel_get_backlight(dev);
+		if (!level)
+			level = max;
+		dev_priv->backlight_level = level;
+	}
+}

While I'm glad this problem is being fixed upstream, it would be nice to get
some credit for finding the source of the problem.

Greetings,

Indan

Re: Linux 2.6.37

[Chris Wilson]

On Wed, 12 Jan 2011 01:35:49 +0100 (CET), "Indan Zupancic" <indan@nul.nu> wrote:
> Yeah, the second patch is a bit of a desperate attempt because Larry reported that
> it didn't fix his problem.
> 
> About your patch, you still do:
> 
> +void intel_panel_setup_backlight(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	dev_priv->backlight_level = intel_panel_get_max_backlight(dev);
> +	dev_priv->backlight_enabled = dev_priv->backlight_level != 0;
> +}
> 
> While my patch changes that to:
> 
> +	u32 level;
> 
> -	if (dev_priv->backlight_level == 0)
> -		dev_priv->backlight_level = intel_panel_get_max_backlight(dev);
> +	if (dev_priv->backlight_level == 0) {
> +		level = intel_panel_get_backlight(dev);
> +		if (level == 0)
> +			level = intel_panel_get_max_backlight(dev);
> +		dev_priv->backlight_level = level;
> +	}
> 
> Your patch uses intel_panel_get_max_backlight() to check if the backlight is
> enabled. Is this always correct, or may it cause bugs in the future?

That was a typo, cut'n'pasting the line from above.

> Anyway, my main concern with unconditionally setting the backlight level to
> the maximum is that any stored brightness level (by the BIOS or whatever) may
> be lost, and that the screen is set to maximum brightness at each boot. This
> is certainly the behaviour I've seen with an unpatched kernel. So I propose to
> do what my patch does and set it to intel_panel_get_backlight(dev) if that
> returns non-zero. Something like this:

Sure, s/intel_panel_get_max_backlight/intel_panel_get_backlight/ and we
get the behaviour we both want - preserving the current backlight unless
none is set.

> While I'm glad this problem is being fixed upstream, it would be nice to get
> some credit for finding the source of the problem.

Sorry. You found the bug but I felt your rationale was off. However, I was
amiss in not giving you the credit you fully deserved.
-Chris

commit 9c1c388a53e5df8819e898106a64e34d0994a5d4
Author: Indan Zupancic <indan@nul.nu>
Date:   Wed Jan 12 11:59:19 2011 +0000

    drm/i915/panel: The backlight is enabled if the current value is non-zero
    
    ... and not if the maximum is non-zero. This fixes the typo introduced
    in 47356eb6728501452 and preserves the backlight value from boot.
    
    [ickle: My thanks also to Indan Zupancic for diagnosing the original
            regression and suggesting the appropriate fix.]
    Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
    Cc: stable@kernel.org # after 47356eb6728501452

diff --git a/drivers/gpu/drm/i915/intel_panel.c
b/drivers/gpu/drm/i915/intel_pan
index e00d200..27c79c7 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -278,6 +278,6 @@ void intel_panel_setup_backlight(struct drm_device
*dev)
 {
        struct drm_i915_private *dev_priv = dev->dev_private;
 
-       dev_priv->backlight_level = intel_panel_get_max_backlight(dev);
+       dev_priv->backlight_level = intel_panel_max_backlight(dev);
        dev_priv->backlight_enabled = dev_priv->backlight_level != 0;
 }

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: Linux 2.6.37

[Chris Wilson]

On Wed, 12 Jan 2011 12:07:23 +0000, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> diff --git a/drivers/gpu/drm/i915/intel_panel.c
> b/drivers/gpu/drm/i915/intel_pan
> index e00d200..27c79c7 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -278,6 +278,6 @@ void intel_panel_setup_backlight(struct drm_device
> *dev)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
>  
> -       dev_priv->backlight_level = intel_panel_get_max_backlight(dev);
> +       dev_priv->backlight_level = intel_panel_max_backlight(dev);
>         dev_priv->backlight_enabled = dev_priv->backlight_level != 0;

          dev_priv->backlight_level = intel_panel_get_backlight(dev);
-ETIRED

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: Linux 2.6.37

[Indan Zupancic]

On Wed, January 12, 2011 13:07, Chris Wilson wrote:
>
> Sure, s/intel_panel_get_max_backlight/intel_panel_get_backlight/ and we
> get the behaviour we both want - preserving the current backlight unless
> none is set.

Indeed, I hadn't noticed that shortcut. That's a lot nicer than my ifdefery.

>
>> While I'm glad this problem is being fixed upstream, it would be nice to get
>> some credit for finding the source of the problem.
>
> Sorry. You found the bug but I felt your rationale was off. However, I was
> amiss in not giving you the credit you fully deserved.

Thank you very much!

The rationale was that intel_panel_set_backlight(0) was somehow called twice,
and that the current code unconditionally stored the old backlight, and thus
losing the original brightness level. This is exactly what happened. My fix
was to prevent backlight_level from being overwritten by zero. Your fix was
more structural and properly fixed backlight enabled/disabled state. In the
end both have the same effect and solve the bug. Perhaps I was unclear in
the bug description.

Anyhow, it's a pleasure working with you. I'll try to not bother you too much,
you got enough on your plate as it is. I'll leave you alone for a while after
you looked into my fix for bug 23472, after that all my Intel graphics are
pretty much solved. :-)

Take care,

Indan

Re: Linux 2.6.37

[Zephaniah E. Loss-Cutler-Hull]

On Thu, 06 Jan 2011 11:50:02 +0100, Michal Hocko wrote:
> Hi,
> 
> On Tue 04-01-11 17:15:45, Linus Torvalds wrote:
> [...]
> > We did have another revert to fix hopefullythe last "blank screen"
> > regression on intel graphics.
> 
> It seems that there is still a regression for intel graphic cards
> backlight. One report is
> https://bugzilla.kernel.org/show_bug.cgi?id=22672.
> I can reproduce the problem easily by:
> xset dpms force standby; sleep 3s; xset dpms force on
> 
> backlight doesn't get up (there is really dark picture though which
> doesn't get brighter by function keys which work normally) after dpms on
> until I close and open lid.
> 
> The problem wasn't present in 2.6.36

And I have a different regression.

On 2.6.36.2 everything is fine, on 2.6.37 my laptop's display is more or
less blank, backlight on, with a white pattern slowly moving across the
screen like it was a (bad) moving spot light effect.

dmesg output and .configs at
https://bugs.freedesktop.org/show_bug.cgi?id=32903

Zephaniah E. Loss-Cutler-Hull.