Re: [PATCH 2.6.29] eeepc-laptop: report brightness control events via the input layer

[Corentin Chary-2]

On Sat, Jun 13, 2009 at 7:51 PM, Alan
Jenkins<sourcejedi.lkml@...> wrote:

> On 6/13/09, Darren Salt <linux@...> wrote:
>> I demand that Corentin Chary may or may not have written...
>>
>>> On Sat, Jun 13, 2009 at 11:33 AM, Alan
>>> Jenkins<alan-jenkins@...>
>>> wrote:
>> [snip]
>>>> The firmware still changes the brightness immediately.  It seems
>>>> that when g-p-m gets delayed, it responds _wrongly_. It doesn't realize
>>>> that the firmware already changed the brightness, so it changes the
>>>> brightness again.
>>
>> Should it be changing the brightness at all? I ask because every laptop
>> which
>> I've used will change the brightness without userspace being involved.
>> (Although it's possible that g-p-m might get brightness-change events from
>> some source other than that which is used to report the laptop's own
>> brightness controls...)
>
> Good point.  Google suggests it may be necessary on some other systems though.
>
> <https://lists.ubuntu.com/archives/kubuntu-bugs/2009-February/067008.html>
>
> And I was wrong before when I said g-p-m should watch the generic
> backlight interface.  It doesn't generate uevents, and in one way
> that's good, because uevents are relatively heavyweight.  So the
> brightness up / down "keypress" events are the only generic way that
> g-p-m can use to detect changes :-(.
>
> But I don't think it's important to be able to show a brightness
> pop-up.  So I'm less confident, but I still think this change should
> be reverted.
>
>> [snip]
>>> Version: 2.24.2-2ubuntu8
>>> Ok I can reproduce [the brightness being changed inappropriately from
>>> userspace].
>>
>>> I want to check if we can't fix g-p-m before reverting the patch. If
>>> there is no way to fix it, I'll revert.
>>
>> I don't see that it can ever be reliable in the face of the brightness
>> having
>> already been changed without userspace involvement short of being able to
>> tell it to report only on some/all events from some input devices.
>
> Hmm.  I think I could accept it if it only played up when thrashing
> the SSD.  So I'll try to work out exactly what happens in the other
> case I reported, in case they're not the same problem.  (This other
> problem is where I hold down "brightness down", then tap "brightness
> up" a couple of times; the first couple of taps casue a flash as the
> brightness goes first up and then down).
>
> Alan
>

CCed gnome-power-manager, as it seems to be the only userspace program
concerned.
You may be able to help us here.

You can find the complet discussion here:
http://groups.google.com/group/linux.kernel/browse_thread/thread/a7bef6cffb7c2d6b/c732f616555d5180?#c732f616555d5180


--
Corentin Chary
http://xf.iksaif.net - http://uffs.org
_______________________________________________
gnome-power-manager-list mailing list
gnome-power-manager-list@...
http://mail.gnome.org/mailman/listinfo/gnome-power-manager-list

On 14/06/2009, Corentin Chary <corentin.chary@...> wrote:
> CCed gnome-power-manager, as it seems to be the only userspace program
> concerned.
> You may be able to help us here.
>
> You can find the complet discussion here:
> http://groups.google.com/group/linux.kernel/browse_thread/thread/a7bef6cffb7c2d6b/c732f616555d5180?#c732f616555d5180

Since this is my complaint, I'll try to summarize.


Summary: g-p-m's reaction to brightness events makes the brightness
changes less reliable
Severity: cosmetic; decrease in quality of user experience

Hardware: Asus Eee PC.
Software: linux kernel, gnome-power-manager

Last working version: linux 2.6.29.4
First broken version: linux 2.6.30

Root cause:
1) The eeepc-laptop platform driver added support for brightness
events.  They occur when the brightness up/down keys are pressed, and
are exported as normal keypresses (on a specific input device).  This
is apparently the same thing other kernel drivers do on other systems.

2) g-p-m isn't sure whether the firmware is changing the brightness
when the brightness keys are pressed.  (The EeePc firmware does change
the brightness, like most laptops).  It has a workaround for this
problem, but it isn't completely reliable.  In some cases g-p-m gets
it wrong, and changes the brightness a second time.

You can see the problem by looking at the code, and considering what
happens when more than one input event is buffered at a time:

        /* check to see if the panel has changed */
        gpm_brightness_lcd_get_hw (brightness, ¤t_hw);

        /* the panel has been updated in firmware */
        if (current_hw != brightness->priv->last_set_hw) {
                brightness->priv->last_set_hw = current_hw;
        } else {
                [ increment the brightness ]
        }

The first event will be ignored as expected.  But on the second event,
g-p-m will re-apply the brightness change.  It doesn't notice that the
brightness jumped _several_ steps before it processed the first event.

Symptoms:
1) When thrashing the EeePC's cheap sold-state drive, the system
becomes much slower to respond.  If you tap the brightness up key
three times, you can see the brightness jump more than 3 steps.  The
first 3 steps are immediate, then g-p-m appears to "catch up", and
erroneously re-apply the brightness changes.

This is a neat way to demonstrate the problem (see upthread for exact
reproduction steps), but we don't really care too much about it.
Laggy systems are laggy and strange.  I don't find this surprising and
I think most users will accept it, even if we could do better.

2) Switching quickly between holding "brightness down" and "brightness
up" can cause a flicker/flash of brightness.  This flash goes away
when g-p-m is killed (or on older kernels).

More specifically:
i) Set the screen to maximum brightness
ii) Hold down "brightness down"
iii) When brightness is at minimum, immediately release it and hold
down "brightness up" (or quickly tap it multiple times).

What probably happens is that g-p-m "falls behind" during step ii).
During step ii), all this does is cause the brightness to change a
little bit faster.  However, it means that in step iii), it's still
trying to decrease the brightness when the firmware starts to increase
the brightness.  So during step iii) I think you see the firmware
increase the brightness, then g-p-m decrease the brightness, and then
g-p-m catches up and the brightness increases again.

This crosses my annoyance threshold.  That's partly because it's a
regression, and because I've spent a lot of time tracking down
brightness-change related regressions which ultimately crashed the
entire system.  But I do think it's an annoyance in it's own right.
It's definitely a realistic scenario.  One will often move these small
laptops around and adjust the brightness to suit different light
conditions.

Thanks
Alan
_______________________________________________
gnome-power-manager-list mailing list
gnome-power-manager-list@...
http://mail.gnome.org/mailman/listinfo/gnome-power-manager-list

[Alan Jenkins-4]

On 6/15/09, Alan Jenkins <alan-jenkins@...> wrote:
> On 14/06/2009, Corentin Chary <corentin.chary@...> wrote:
>> CCed gnome-power-manager, as it seems to be the only userspace program
>> concerned.

Warning pour les autres: subscription-only mailing list.

Posts on the g-p-m list will be subject to moderation and delays.

>> You may be able to help us here.
>>
>> You can find the complet discussion here:
>> http://groups.google.com/group/linux.kernel/browse_thread/thread/a7bef6cffb7c2d6b/c732f616555d5180?#c732f616555d5180
>
> Since this is my complaint, I'll try to summarize.
>
>
> Summary: g-p-m's reaction to brightness events makes the brightness
> changes less reliable
> Severity: cosmetic; decrease in quality of user experience
>
> Hardware: Asus Eee PC.
> Software: linux kernel, gnome-power-manager
>
> Last working version: linux 2.6.29.4
> First broken version: linux 2.6.30
>
> Root cause:
> 1) The eeepc-laptop platform driver added support for brightness
> events.  They occur when the brightness up/down keys are pressed, and
> are exported as normal keypresses (on a specific input device).  This
> is apparently the same thing other kernel drivers do on other systems.
>
> 2) g-p-m isn't sure whether the firmware is changing the brightness
> when the brightness keys are pressed.  (The EeePc firmware does change
> the brightness, like most laptops).  It has a workaround for this
> problem, but it isn't completely reliable.  In some cases g-p-m gets
> it wrong, and changes the brightness a second time.
>
> You can see the problem by looking at the code, and considering what
> happens when more than one input event is buffered at a time:
>
> /* check to see if the panel has changed */
> gpm_brightness_lcd_get_hw (brightness, ¤t_hw);
>
> /* the panel has been updated in firmware */
> if (current_hw != brightness->priv->last_set_hw) {
> brightness->priv->last_set_hw = current_hw;
> } else {
> [ increment the brightness ]
> }
>
> The first event will be ignored as expected.  But on the second event,
> g-p-m will re-apply the brightness change.  It doesn't notice that the
> brightness jumped _several_ steps before it processed the first event.
>
> Symptoms:
> 1) When thrashing the EeePC's cheap sold-state drive, the system
> becomes much slower to respond.  If you tap the brightness up key
> three times, you can see the brightness jump more than 3 steps.  The
> first 3 steps are immediate, then g-p-m appears to "catch up", and
> erroneously re-apply the brightness changes.
>
> This is a neat way to demonstrate the problem (see upthread for exact
> reproduction steps), but we don't really care too much about it.
> Laggy systems are laggy and strange.  I don't find this surprising and
> I think most users will accept it, even if we could do better.
>
> 2) Switching quickly between holding "brightness down" and "brightness
> up" can cause a flicker/flash of brightness.  This flash goes away
> when g-p-m is killed (or on older kernels).
>
> More specifically:
> i) Set the screen to maximum brightness
> ii) Hold down "brightness down"
> iii) When brightness is at minimum, immediately release it and hold
> down "brightness up" (or quickly tap it multiple times).
>
> What probably happens is that g-p-m "falls behind" during step ii).
> During step ii), all this does is cause the brightness to change a
> little bit faster.  However, it means that in step iii), it's still
> trying to decrease the brightness when the firmware starts to increase
> the brightness.  So during step iii) I think you see the firmware
> increase the brightness, then g-p-m decrease the brightness, and then
> g-p-m catches up and the brightness increases again.
>
> This crosses my annoyance threshold.  That's partly because it's a
> regression, and because I've spent a lot of time tracking down
> brightness-change related regressions which ultimately crashed the
> entire system.  But I do think it's an annoyance in it's own right.
> It's definitely a realistic scenario.  One will often move these small
> laptops around and adjust the brightness to suit different light
> conditions.
>
> Thanks
> Alan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@...
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

_______________________________________________
gnome-power-manager-list mailing list
gnome-power-manager-list@...
http://mail.gnome.org/mailman/listinfo/gnome-power-manager-list

[Richard Hughes-6]

On Mon, Jun 15, 2009 at 9:09 AM, Alan
Jenkins<alan-jenkins@...> wrote:
> So during step iii) I think you see the firmware
> increase the brightness, then g-p-m decrease the brightness, and then
> g-p-m catches up and the brightness increases again.

Sorry for not responding sooner, been moving house.

I don't think the kernel driver should modify the brightness itself,
as that's applying policy. Is this is left to userspace then we can
put on policy such as "don't allow brightness to be set below 30%" or
"automatically set the brightness using a ambient light sensor, and
use the brightness keys to set ambient thresholds".

Doing this policy in the driver is the wrong thing to do IMO.

Richard.
_______________________________________________
gnome-power-manager-list mailing list
gnome-power-manager-list@...
http://mail.gnome.org/mailman/listinfo/gnome-power-manager-list

Richard Hughes wrote:

> On Mon, Jun 15, 2009 at 9:09 AM, Alan
> Jenkins<alan-jenkins@...> wrote:
>  
>> So during step iii) I think you see the firmware
>> increase the brightness, then g-p-m decrease the brightness, and then
>> g-p-m catches up and the brightness increases again.
>>    
>
> Sorry for not responding sooner, been moving house.
>
> I don't think the kernel driver should modify the brightness itself,
> as that's applying policy. Is this is left to userspace then we can
> put on policy such as "don't allow brightness to be set below 30%" or
> "automatically set the brightness using a ambient light sensor, and
> use the brightness keys to set ambient thresholds".
>
> Doing this policy in the driver is the wrong thing to do IMO.
>
> Richard.
>  

The driver doesn't.  As far as we know, none of them do.  Firmware does.

Thanks
Alan
_______________________________________________
gnome-power-manager-list mailing list
gnome-power-manager-list@...
http://mail.gnome.org/mailman/listinfo/gnome-power-manager-list

[Richard Hughes-6]

On Tue, Jun 16, 2009 at 9:34 AM, Alan
Jenkins<alan-jenkins@...> wrote:
> The driver doesn't.  As far as we know, none of them do.  Firmware does.

In that case there's a "laptop_panel.brightness_in_hardware" hal quirk
that turns off all the fancy stuff, and makes the GUI DTRT without
guessing.

Richard.
_______________________________________________
gnome-power-manager-list mailing list
gnome-power-manager-list@...
http://mail.gnome.org/mailman/listinfo/gnome-power-manager-list

[Corentin Chary-2]

On Tue, Jun 16, 2009 at 10:47 AM, Richard Hughes<hughsient@...> wrote:

> On Tue, Jun 16, 2009 at 9:34 AM, Alan
> Jenkins<alan-jenkins@...> wrote:
>> The driver doesn't.  As far as we know, none of them do.  Firmware does.
>
> In that case there's a "laptop_panel.brightness_in_hardware" hal quirk
> that turns off all the fancy stuff, and makes the GUI DTRT without
> guessing.
>
> Richard.
>

Ok, so we just need to patch this file :
http://cgit.freedesktop.org/hal-info/tree/fdi/information/10freedesktop/10-laptop-panel-hardware.fdi
?
If found it /usr/share/hal/fdi/information/10freedesktop/

But I'm not sure how to do it to make it works on all Eeepc.

--
Corentin Chary
http://xf.iksaif.net - http://uffs.org
_______________________________________________
gnome-power-manager-list mailing list
gnome-power-manager-list@...
http://mail.gnome.org/mailman/listinfo/gnome-power-manager-list

[Richard Hughes-6]

On Tue, Jun 16, 2009 at 10:44 AM, Corentin
Chary<corentin.chary@...> wrote:
> Ok, so we just need to patch this file :
> http://cgit.freedesktop.org/hal-info/tree/fdi/information/10freedesktop/10-laptop-panel-hardware.fdi

Yes

> But I'm not sure how to do it to make it works on all Eeepc.

Well, really we want to match "classes of eeepc", so matching with
prefix might be a good idea.

Richard.
_______________________________________________
gnome-power-manager-list mailing list
gnome-power-manager-list@...
http://mail.gnome.org/mailman/listinfo/gnome-power-manager-list

[Alan Jenkins-4]

On 6/16/09, Richard Hughes <hughsient@...> wrote:

> On Tue, Jun 16, 2009 at 10:44 AM, Corentin
> Chary<corentin.chary@...> wrote:
>> Ok, so we just need to patch this file :
>> http://cgit.freedesktop.org/hal-info/tree/fdi/information/10freedesktop/10-laptop-panel-hardware.fdi
>
> Yes
>
>> But I'm not sure how to do it to make it works on all Eeepc.
>
> Well, really we want to match "classes of eeepc", so matching with
> prefix might be a good idea.
>
> Richard.

Yes please!  I see the 701 is (just added?) on the list, but this
applies to any hardware which is driven by the eeepc-laptop driver.
There is no "brightness up key" or "brightness down key" notification
recognised by the driver, only "brightness level is now equal to X".

Btw, I looked at other drivers and fujistsu-laptop is implemented in
the same way.

I guess the problem is the DMI manufacturer and product name don't
mention "EeePC".  At least on my model, the serial number starts with
"EeePC-", so we _could_ try to use that.

But I hope there's a nicer solution.  Corentin, do you think it would
be correct to apply this quirk to _all_ Asus laptops?

At the moment asus-laptop doesn't generate brightness events.  The
deprecated asus-acpi driver generates brightness events (via the old
procfs interface).   But the BR_UP/BR_DOWN events carry an absolute
brightness value; it strongly suggests that the firmware modifies the
brightness.  asus-acpi uses the brightness value to update a generic
backlight device - so we already rely on the value being correct,
otherwise g-p-m's heuristic won't work.

Thanks
Alan
_______________________________________________
gnome-power-manager-list mailing list
gnome-power-manager-list@...
http://mail.gnome.org/mailman/listinfo/gnome-power-manager-list

[Corentin Chary-2]

On Thu, Jun 18, 2009 at 3:33 PM, Alan
Jenkins<sourcejedi.lkml@...> wrote:

> On 6/16/09, Richard Hughes <hughsient@...> wrote:
>> On Tue, Jun 16, 2009 at 10:44 AM, Corentin
>> Chary<corentin.chary@...> wrote:
>>> Ok, so we just need to patch this file :
>>> http://cgit.freedesktop.org/hal-info/tree/fdi/information/10freedesktop/10-laptop-panel-hardware.fdi
>>
>> Yes
>>
>>> But I'm not sure how to do it to make it works on all Eeepc.
>>
>> Well, really we want to match "classes of eeepc", so matching with
>> prefix might be a good idea.
>>
>> Richard.
>
> Yes please!  I see the 701 is (just added?) on the list, but this

The 701 was already here when I checked.

> But I hope there's a nicer solution.  Corentin, do you think it would
> be correct to apply this quirk to _all_ Asus laptops?

I can confirm that all the asus laptop I know change the brightness in the
firmware. So, in my opinion, we can safely apply this quirk to all Asus laptops.


--
Corentin Chary
http://xf.iksaif.net - http://uffs.org
_______________________________________________
gnome-power-manager-list mailing list
gnome-power-manager-list@...
http://mail.gnome.org/mailman/listinfo/gnome-power-manager-list