All of lore.kernel.org
 help / color / mirror / Atom feed
From: Corentin Chary <corentin.chary@gmail.com>
To: Alan Jenkins <alan-jenkins@tuffmail.co.uk>
Cc: Darren Salt <linux@youmustbejoking.demon.co.uk>,
	linux-kernel@vger.kernel.org,
	acpi4asus-user@lists.sourceforge.net,
	Matthew Garrett <mjg59@srcf.ucam.org>
Subject: Re: [PATCH 2.6.29] eeepc-laptop: report brightness control events via  the input layer
Date: Sat, 13 Jun 2009 12:06:18 +0200	[thread overview]
Message-ID: <71cd59b00906130306w319c0fc2i376bb03323845b80@mail.gmail.com> (raw)
In-Reply-To: <4A337251.7010705@tuffmail.co.uk>

On Sat, Jun 13, 2009 at 11:33 AM, Alan
Jenkins<alan-jenkins@tuffmail.co.uk> wrote:
> Corentin Chary wrote:
>>
>> On Mon, Jun 8, 2009 at 5:24 PM, Alan
>> Jenkins<sourcejedi.lkml@googlemail.com> wrote:
>>
>>>
>>> On 4/4/09, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
>>>
>>>>
>>>> On Fri, Apr 03, 2009 at 06:57:50PM +0100, Darren Salt wrote:
>>>>
>>>>>
>>>>> This maps the brightness control events to one of two keys, either
>>>>> KEY_BRIGHTNESSDOWN or KEY_BRIGHTNESSUP, as needed.
>>>>>
>>>>> Some mapping has to be done due to the fact that the BIOS reports them
>>>>> as
>>>>> <base value> + <current brightness index>; the selection is done
>>>>> according
>>>>> to
>>>>> the sign of the change in brightness (if this is 0, no keypress is
>>>>> reported).
>>>>>
>>>>> (Ref.
>>>>>
>>>>> http://lists.alioth.debian.org/pipermail/debian-eeepc-devel/2009-April/002001.html)
>>>>>
>>>>> Signed-off-by: Darren Salt <linux@youmustbejoking.demon.co.uk>
>>>>>
>>>>
>>>> The reason I didn't do this is that the Eee changes the input brightness
>>>> in hardware, which means reporting it via the input layer as well can
>>>> cause a single keypress to raise the brightness by two steps - one in
>>>> hardware and one triggered by userland's response to the key press. I'd
>>>> be a little bit wary of this causing problems.
>>>>
>>>> On the other hand, the default behaviour of the acpi video driver is to
>>>> change the brightness itself and then also to send the even to
>>>> userspace, so I guess if it was going to break things it probably would
>>>> have done already...
>>>>
>>>
>>> Actually, I think userspace has learnt to hack around it but it
>>> doesn't work perfectly.  I would like to request that this change be
>>> reverted, or otherwise improved.
>>>
>>> Before this patch (2.6.29.4), gnome-power-manager doesn't interfere
>>> with the brightness keys, and they work smoothly.
>>>
>>> After this patch (2.6.30-rc7), g-p-m produces a "nice" popup in the
>>> middle of my tiny netbook screen.  Unfortunately it can't be disabled,
>>> but that's not your fault :-).  The brightness controls generally work
>>> ok.  It doesn't jump two steps in response to one brightness keypress.
>>>  But:
>>>
>>> 1) If I'm thrashing the SSD.  I get jerky after-effects, where g-p-m
>>> seems to take too long to "catch up" with the brightness change.
>>>
>>
>> There is the same "lag" problem with sound :/
>>
>
> Yeah :/.  At it's worst, it isn't a pure "lag".  It's a bit harder to
> explain.  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.
>
> That's why I think this is a bad "feature".  User-space is working around
> it, but the workaround isn't reliable.  I think the proper solution is that
> if userspace wants to respond to firmware-initiated brightness changes, it
> should listen for uevents on the brightness class device.
>
> You can see the problem most clearly by pressing the brightness keys in
> quick succession e.g. 3 times in a row, and see 3+3 brightness changes.  I
> reproduced this with
>
> 1) 2 writers:
>   F=1; while true do dd if=/dev/zero of=$F bs=1M count=1 conv=sync; done &
>   F=2; while true do dd if=/dev/zero of=$F bs=1M count=1 conv=sync; done &
>
> 2) 1 reader:
>   dd if=/dev/sda of=/dev/null
>
> 3) Drop caches before pressing the brightness keys
>   echo 1 > /sys/proc/vm/drop_caches
>
>
>>> 2) If I go all the way down from full (holding down the "brightness
>>> down" key), and then back up a few steps.  I get a noticable flash
>>> where the brightness looks to go up two steps, then down one.  It's
>>> probably most noticable here because the step change between the
>>> lowest and the second lowest brightness is much more visible than any
>>> of the other steps.
>>>
>>>
>>
>> I tried to install gnome-power-manager to test that, but there is no
>> "popup".
>> What do I have to install to test that ? entire gnome desktop :/ ?
>>
>> Thanks
>>
>
> That's weird.  I'm running it from KDE here (g-p-m package version 2.22.1-4
> on debian stable).  I'm pretty sure the only other GNOME app I have
> installed is Pidgin.  I would know if I had much more installed, because I'm
> often running short of disk space :-).
>
> Maybe you have a newer version, that doesn't try to do such unreliable
> things?
>
> Thanks for your time
> Alan
>
Hum .. running g-p-m as root, I can get the popup, strange
Version: 2.24.2-2ubuntu8
Ok I can reproduce that.

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.

-- 
Corentin Chary
http://xf.iksaif.net - http://uffs.org

  reply	other threads:[~2009-06-13 10:06 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2009-04-03 17:57 [PATCH 2.6.29] eeepc-laptop: report brightness control events via the input layer Darren Salt
2009-04-04  4:18 ` Matthew Garrett
2009-04-04  8:33   ` Corentin Chary
2009-04-04 12:20     ` Darren Salt
2009-04-04 12:35       ` Corentin Chary
2009-04-04 22:10         ` Darren Salt
2009-04-05  8:22           ` Corentin Chary
2009-06-08 15:24   ` Alan Jenkins
2009-06-13  8:55     ` Corentin Chary
2009-06-13  9:33       ` Alan Jenkins
2009-06-13 10:06         ` Corentin Chary [this message]
2009-06-13 12:55           ` Darren Salt
2009-06-13 17:51             ` Alan Jenkins
2009-06-14 19:26               ` Corentin Chary
2009-06-15  8:09                 ` Alan Jenkins
2009-06-15  8:12                   ` Alan Jenkins
2009-06-16  8:33                   ` [gpm] " Richard Hughes
2009-06-16  8:34                     ` Alan Jenkins
2009-06-16  8:47                       ` Richard Hughes
2009-06-16  9:44                         ` Corentin Chary
2009-06-16 10:04                           ` Richard Hughes
2009-06-18 13:33                             ` Alan Jenkins
2009-06-18 22:44                               ` Corentin Chary

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=71cd59b00906130306w319c0fc2i376bb03323845b80@mail.gmail.com \
    --to=corentin.chary@gmail.com \
    --cc=acpi4asus-user@lists.sourceforge.net \
    --cc=alan-jenkins@tuffmail.co.uk \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@youmustbejoking.demon.co.uk \
    --cc=mjg59@srcf.ucam.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.