Yes, I actually bisected this just to get
886129a8eebebec260165741fe31421482371006 is the first bad commit
commit 886129a8eebebec260165741fe31421482371006
Author: Hans de Goede <[email protected]>
Date: Tue May 6 14:46:23 2014 +0200
ACPI / video: change acpi-video brightness_switch_enabled default to 0
acpi-video is unique in that it not only generates brightness up/down
keypresses, but also (sometimes) actively changes the brightness itself.
This presents an inconsistent kernel interface to userspace, basically there
are 2 different scenarios, depending on the laptop model:
1) On some laptops a brightness up/down keypress means: show a brightness osd
with the current brightness, iow it is a brightness has changed notification.
2) Where as on (a lot of) other laptops it means a brightness up/down key was
pressed, deal with it.
Most of the desktop environments interpret any press as in scenario 2, and
change the brightness up / down as a response to the key events, causing it
to be changed twice, once by acpi-video and once by the DE.
With the new default for video.use_native_backlight we will be moving even
more laptops over to behaving as in scenario 2. Making the remaining laptops
even more of a weird exception. Also note that it is hard to detect scenario
1 properly in userspace, and AFAIK none of the DE-s deals with it.
Therefor this commit changes the default of brightness_switch_enabled to 0
making its behavior consistent with all the other backlight drivers.
Signed-off-by: Hans de Goede <[email protected]>
Reviewed-by: Aaron Lu <[email protected]>
Signed-off-by: Rafael J. Wysocki <[email protected]>
:040000 040000 5bbac8c4f3e9fd5421daf84289004c32c3217f2a 82c99a358bda6360f845b6063182d3e707ff90f0 M Documentation
:040000 040000 81ed56a41130bbbea980620114ff11e3bb23ee63 a9870ba1d046bde69796060304c78dfbb1d00a1f M drivers
The fact that this seems to be an *intentional* breakage does not help a
lot. Yes, I understand that you believe the choice of default was
incorrect for some reason. You might even be right about that. But
that is still not a valid reason to break existing configurations for
end users! Please do not do that.
Note that NO USER cares about "some laptops" or "other laptops". They
care about their own systems, which either
a) depend on the old default and therefore breaks with your change, or
b) have a user modified setting and therefore are unaffected by your
change
The above commit should be reverted. It causes breakage for end users.
If you think the default was wrong, then please go back in time and fix
it when it was introduced. Thanks.
Bjørn
Hi,
On 07/14/2014 02:59 PM, Bjørn Mork wrote:
> Yes, I actually bisected this just to get
>
> 886129a8eebebec260165741fe31421482371006 is the first bad commit
> commit 886129a8eebebec260165741fe31421482371006
> Author: Hans de Goede <[email protected]>
> Date: Tue May 6 14:46:23 2014 +0200
>
> ACPI / video: change acpi-video brightness_switch_enabled default to 0
>
> acpi-video is unique in that it not only generates brightness up/down
> keypresses, but also (sometimes) actively changes the brightness itself.
>
> This presents an inconsistent kernel interface to userspace, basically there
> are 2 different scenarios, depending on the laptop model:
>
> 1) On some laptops a brightness up/down keypress means: show a brightness osd
> with the current brightness, iow it is a brightness has changed notification.
>
> 2) Where as on (a lot of) other laptops it means a brightness up/down key was
> pressed, deal with it.
>
> Most of the desktop environments interpret any press as in scenario 2, and
> change the brightness up / down as a response to the key events, causing it
> to be changed twice, once by acpi-video and once by the DE.
>
> With the new default for video.use_native_backlight we will be moving even
> more laptops over to behaving as in scenario 2. Making the remaining laptops
> even more of a weird exception. Also note that it is hard to detect scenario
> 1 properly in userspace, and AFAIK none of the DE-s deals with it.
>
> Therefor this commit changes the default of brightness_switch_enabled to 0
> making its behavior consistent with all the other backlight drivers.
>
> Signed-off-by: Hans de Goede <[email protected]>
> Reviewed-by: Aaron Lu <[email protected]>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
>
> :040000 040000 5bbac8c4f3e9fd5421daf84289004c32c3217f2a 82c99a358bda6360f845b6063182d3e707ff90f0 M Documentation
> :040000 040000 81ed56a41130bbbea980620114ff11e3bb23ee63 a9870ba1d046bde69796060304c78dfbb1d00a1f M drivers
>
>
>
> The fact that this seems to be an *intentional* breakage does not help a
> lot. Yes, I understand that you believe the choice of default was
> incorrect for some reason. You might even be right about that. But
> that is still not a valid reason to break existing configurations for
> end users! Please do not do that.
This *not* a regression, it is an intended behavior change which can be
flipped back to its old behavior with a single cmdline argument (add
video.brightness_switch_enabled=1 to the kernel commandline).
Yes this may break existing configurations for some users, most likely
users running some custom setup, who thus should be able to fix things
by adding one more customization to there setup.
OTOH this will also unbreak a lot of existing setups, likely an amount
of setups which is at least one order of magnitude bigger then the
amount it will break.
Most users will be running a desktop environment which will react
to the keypresses (which are always generated in cases where
brightness_switch_enabled does anything) by changing the brightness
a second time. This happens at least in gnome, kde, xfce, unity and
probably in a few smaller desktop environments as configured ootb too.
If you've a backlight control which only has 8 steps taking 2 steps
at a time is a bug issue, see e.g. :
https://bugs.launchpad.net/gnome-settings-daemon/+bug/527157
http://askubuntu.com/questions/173921/why-does-my-thinkpad-brightness-control-skip-steps
TL;DR: This change really is for the better and is here to stay.
> Note that NO USER cares about "some laptops" or "other laptops". They
> care about their own systems, which either
> a) depend on the old default and therefore breaks with your change, or
> b) have a user modified setting and therefore are unaffected by your
> change
This is not how it works, sometimes we *must* look at the bigger picture,
e.g. when the Linux kernel first started advertising to acpi that it
was "Windows 2012" (aka win8), this causes some breakage, still we went
ahead with the change, because in the end we must advertise "Windows 2012"
support to be able to get support for certain features from the firmware.
Regards,
Hans
Hans de Goede <[email protected]> writes:
> Hi,
>
> On 07/14/2014 02:59 PM, Bjørn Mork wrote:
>> Yes, I actually bisected this just to get
>>
>> 886129a8eebebec260165741fe31421482371006 is the first bad commit
>> commit 886129a8eebebec260165741fe31421482371006
>> Author: Hans de Goede <[email protected]>
>> Date: Tue May 6 14:46:23 2014 +0200
>>
>> ACPI / video: change acpi-video brightness_switch_enabled default to 0
>>
>> acpi-video is unique in that it not only generates brightness up/down
>> keypresses, but also (sometimes) actively changes the brightness itself.
>>
>> This presents an inconsistent kernel interface to userspace, basically there
>> are 2 different scenarios, depending on the laptop model:
>>
>> 1) On some laptops a brightness up/down keypress means: show a brightness osd
>> with the current brightness, iow it is a brightness has changed notification.
>>
>> 2) Where as on (a lot of) other laptops it means a brightness up/down key was
>> pressed, deal with it.
>>
>> Most of the desktop environments interpret any press as in scenario 2, and
>> change the brightness up / down as a response to the key events, causing it
>> to be changed twice, once by acpi-video and once by the DE.
>>
>> With the new default for video.use_native_backlight we will be moving even
>> more laptops over to behaving as in scenario 2. Making the remaining laptops
>> even more of a weird exception. Also note that it is hard to detect scenario
>> 1 properly in userspace, and AFAIK none of the DE-s deals with it.
>>
>> Therefor this commit changes the default of brightness_switch_enabled to 0
>> making its behavior consistent with all the other backlight drivers.
>>
>> Signed-off-by: Hans de Goede <[email protected]>
>> Reviewed-by: Aaron Lu <[email protected]>
>> Signed-off-by: Rafael J. Wysocki <[email protected]>
>>
>> :040000 040000 5bbac8c4f3e9fd5421daf84289004c32c3217f2a 82c99a358bda6360f845b6063182d3e707ff90f0 M Documentation
>> :040000 040000 81ed56a41130bbbea980620114ff11e3bb23ee63 a9870ba1d046bde69796060304c78dfbb1d00a1f M drivers
>>
>>
>>
>> The fact that this seems to be an *intentional* breakage does not help a
>> lot. Yes, I understand that you believe the choice of default was
>> incorrect for some reason. You might even be right about that. But
>> that is still not a valid reason to break existing configurations for
>> end users! Please do not do that.
>
> This *not* a regression, it is an intended behavior change
On my laptop it changed the behaviour from working to non-working, which
is a regression whether it was intended or not.
> which can be
> flipped back to its old behavior with a single cmdline argument (add
> video.brightness_switch_enabled=1 to the kernel commandline).
Yes, sure.
But we do not require users to add command line settings to keep
existing behaviour.
For the record, AFAICS, you could achive *your* wanted effect by adding
video.brightness_switch_enabled=0 to the kernel commandline.
> Yes this may break existing configurations for some users, most likely
> users running some custom setup, who thus should be able to fix things
> by adding one more customization to there setup.
You can drop the "may". I have already told you that it broke my
configuration, haven't I?
> OTOH this will also unbreak a lot of existing setups, likely an amount
> of setups which is at least one order of magnitude bigger then the
> amount it will break.
If so, then would adding video.brightness_switch_enabled=0 to the kernel
commandline on those systems. Which would have the nice effect that it
wouldn't break anything.
But whavever. Since when was it OK to intentionally break existing
setups for *any* reason?
> Most users will be running a desktop environment which will react
> to the keypresses (which are always generated in cases where
> brightness_switch_enabled does anything) by changing the brightness
> a second time. This happens at least in gnome, kde, xfce, unity and
> probably in a few smaller desktop environments as configured ootb too.
Now I believe we are moving out on the prairie here. I am concerned
about the *kernel*, not desktop environments.
> If you've a backlight control which only has 8 steps taking 2 steps
> at a time is a bug issue, see e.g. :
>
> https://bugs.launchpad.net/gnome-settings-daemon/+bug/527157
> http://askubuntu.com/questions/173921/why-does-my-thinkpad-brightness-control-skip-steps
>
> TL;DR: This change really is for the better and is here to stay.
>
>> Note that NO USER cares about "some laptops" or "other laptops". They
>> care about their own systems, which either
>> a) depend on the old default and therefore breaks with your change, or
>> b) have a user modified setting and therefore are unaffected by your
>> change
>
> This is not how it works, sometimes we *must* look at the bigger picture,
> e.g. when the Linux kernel first started advertising to acpi that it
> was "Windows 2012" (aka win8), this causes some breakage, still we went
> ahead with the change, because in the end we must advertise "Windows 2012"
> support to be able to get support for certain features from the firmware.
Please explain the bigger picture then. From what I can see, you have
not made a single attempt on explaining why the broken systems cannot be
fixed be fixed by adding video.brightness_switch_enabled=0 to the kernel
commandline.
Bjørn
On Mon, Jul 14, 2014 at 6:14 AM, Hans de Goede <[email protected]> wrote:
>
> This *not* a regression, it is an intended behavior change [..]
That counts as a regression.
If things used to work, and they don't work, it's a regression. If
it's intentional, that just makes it worse.
> Yes this may break existing configurations for some users, most likely
> users running some custom setup, who thus should be able to fix things
> by adding one more customization to there setup.
.. and apparently this whole paragraph is completely bogus. It *does*
break things, and for normal people. That's what the bug report is all
about. So don't waffle about it.
Bjørn, what's your setup? Is this perhaps solvable some other way?
> TL;DR: This change really is for the better and is here to stay.
Wrong. We don't break existing setups, and your attitude needs fixing.
Rafael, please get it reverted, or I will have to revert it. We have
*long* had a rule that we don't break things "in order to improve
things for others", and quite frankly, power management and ACPI in
particular was exactly *why* that rule was introduced, because the
whole "one step back, two steps forward" model does not work.
The problem needs to be solved some other way, and developers need to
f*cking stop with the "we can break peoples setups" mentality./
Hans, seriously. You have the wrong mental model. Fix it.
Linus
On Mon, Jul 14, 2014 at 9:24 AM, Linus Torvalds
<[email protected]> wrote:
>
> Bjørn, what's your setup? Is this perhaps solvable some other way?
For example, I wonder if we could fix the "dual brightness change"
problem automatically by making a new option for
'brightness_switch_enabled'.
Currently, there are two cases:
- enabled: do the actual brightness change _and_ send the input
report keycode for a brightness change
- disabled: just send the keycode, excpecting the desktop software to
handle it.
and maybe we could have a new case (and make *that* the default):
- delayed: send the keycode, and set up a delayed timer (say, one
tenth of a second) to do the actual brightness change. And if a
brightness change from user mode comes in during that delay, we cancel
the kernel-induced pending change.
Something like the very hacky attached patch that is COMPLETELY UNTESTED.
My point being that I think we can get this right *without* some
stupid "user has to specify the behavior of their desktop application
and ACPI implementation" crap. Especially since it's entirely possible
that there are different behaviors for the same machine (ie the user
session may act differently from the login screen, which will act
differently from the text virtual terminal).
I really don't expect my patch to work as-is, it is really meant more
as an illustration of an approach that might work. There may well be
many other complications (ie how does this interact with the whole
"use_native_backlight" thing and user space possibly accessing *other*
backlight controls). But I have the feeling that this should be
solvable without breaking old setups or causing problems on newer
ones.
Linus
On Monday, July 14, 2014 09:24:16 AM Linus Torvalds wrote:
> On Mon, Jul 14, 2014 at 6:14 AM, Hans de Goede <[email protected]> wrote:
> >
> > This *not* a regression, it is an intended behavior change [..]
>
> That counts as a regression.
>
> If things used to work, and they don't work, it's a regression. If
> it's intentional, that just makes it worse.
>
> > Yes this may break existing configurations for some users, most likely
> > users running some custom setup, who thus should be able to fix things
> > by adding one more customization to there setup.
>
> .. and apparently this whole paragraph is completely bogus. It *does*
> break things, and for normal people. That's what the bug report is all
> about. So don't waffle about it.
>
> Bjørn, what's your setup? Is this perhaps solvable some other way?
>
> > TL;DR: This change really is for the better and is here to stay.
>
> Wrong. We don't break existing setups, and your attitude needs fixing.
>
> Rafael, please get it reverted, or I will have to revert it.
I've queued up a revert already. I'll include it into my next pull
request later this week.
Rafael
Linus Torvalds <[email protected]> writes:
> On Mon, Jul 14, 2014 at 9:24 AM, Linus Torvalds
> <[email protected]> wrote:
>>
>> Bjørn, what's your setup? Is this perhaps solvable some other way?
Just to answer that: I don't use any particular desktop environment. I
have acpid running to take care of the most basic power management
stuff. My X session is simply WindowMaker (sic) running directly from
lightdm. No session management or fancy policy daemons. So I don't have
any daemon which would react on the brightness key codes.
Now, it's not that I would mind adding a daemon to handle stuff like
brightness control. I reported this as a bug because I was a bit
surprised by the existing behaviour breaking like that, and I thought
that other users might be as surprised as me. Some maybe even without
the ability to track down the change and the setting that would restore
the old behaviour.
> For example, I wonder if we could fix the "dual brightness change"
> problem automatically by making a new option for
> 'brightness_switch_enabled'.
>
> Currently, there are two cases:
>
> - enabled: do the actual brightness change _and_ send the input
> report keycode for a brightness change
>
> - disabled: just send the keycode, excpecting the desktop software to
> handle it.
>
> and maybe we could have a new case (and make *that* the default):
>
> - delayed: send the keycode, and set up a delayed timer (say, one
> tenth of a second) to do the actual brightness change. And if a
> brightness change from user mode comes in during that delay, we cancel
> the kernel-induced pending change.
That sounds like a good solution to me FWIW.
Bjørn
On Mon, Jul 14, 2014 at 1:45 PM, Bjørn Mork <[email protected]> wrote:
>> brightness change from user mode comes in during that delay, we cancel
>> the kernel-induced pending change.
>
> That sounds like a good solution to me FWIW.
Try the patch. It *might* work. I'm not saying it will, but it seemed
to at least compile for me.
Linus
Linus Torvalds <[email protected]> writes:
> On Mon, Jul 14, 2014 at 1:45 PM, Bjørn Mork <[email protected]> wrote:
>>> brightness change from user mode comes in during that delay, we cancel
>>> the kernel-induced pending change.
>>
>> That sounds like a good solution to me FWIW.
>
> Try the patch. It *might* work. I'm not saying it will, but it seemed
> to at least compile for me.
Yes, the patch works as advertised for me. Thanks.
But this will break existing configs:
> --- a/drivers/acpi/video.c
> +++ b/drivers/acpi/video.c
> @@ -68,8 +68,8 @@ MODULE_AUTHOR("Bruno Ducrot");
> MODULE_DESCRIPTION("ACPI Video Driver");
> MODULE_LICENSE("GPL");
>
> -static bool brightness_switch_enabled;
> -module_param(brightness_switch_enabled, bool, 0644);
> +static int brightness_switch_enabled = -1;
> +module_param(brightness_switch_enabled, int, 0644);
>
> /*
> * By default, we don't allow duplicate ACPI video bus devices
Any setup using e.g "options video brightness_switch_enabled=Y" will
barf on this, won't they?
Bjørn
On Mon, Jul 14, 2014 at 2:46 PM, Bjørn Mork <[email protected]> wrote:
>
> Yes, the patch works as advertised for me. Thanks.
Ok, so it should probably be tested by people who see the "move by
two" problem too.
> But this will break existing configs:
>
> Any setup using e.g "options video brightness_switch_enabled=Y" will
> barf on this, won't they?
Hmm. Probably. That said, that kind of breakage I think we might want
to live with, especially if it actually ends up causing them to test
the new default (which might just work for them).
But regardless, let's make sure that patch (or something _like_ it)
works for people who saw the reverse problem for you. Then the exact
syntax of whatever module parameter (if any) can be a separate
discussion.
Anyway, for 3.16 I think we should just do what we used to do (ie the
revert that Rafael apparently already has queued up), so this isn't
necessarily critical.
Linus
Hi,
On 07/14/2014 06:24 PM, Linus Torvalds wrote:
> On Mon, Jul 14, 2014 at 6:14 AM, Hans de Goede <[email protected]> wrote:
>>
>> This *not* a regression, it is an intended behavior change [..]
>
> That counts as a regression.
>
> If things used to work, and they don't work, it's a regression. If
> it's intentional, that just makes it worse.
The problem is that our previous default behavior turns out to be
wrong for many users.
So as answered later in the thread Bjørn is using a custom setup with
acpid and WindowMaker. So in order to keep his exotic (and very simple
to fix) setup working we are keeping thing broken on what are probably
a 100 thousand Linux installs or more.
>
>> Yes this may break existing configurations for some users, most likely
>> users running some custom setup, who thus should be able to fix things
>> by adding one more customization to there setup.
>
> .. and apparently this whole paragraph is completely bogus. It *does*
> break things, and for normal people.
It breaks things on exotic setups, running a non standard desktop
environment or no desktop environment at all. Note that this is
about laptops most people will be using one of gnome / kde / unity /
xfce on a desktop, and for all of these the old default behavior is
bad.
> That's what the bug report is all about. So don't waffle about it.
>
> Bjørn, what's your setup?
> Is this perhaps solvable some other way?
I see that further down the thread you've send a patch to fix
this by waiting sometime for userspace to react and if it does
not then do the change in the kernel as it used to be.
FWIW I don't think this is a good idea. This is inherently racy, so
we will still sometimes see the backlight taking 2 steps leading to
hard to debug problems.
It has made me think about doing something to keep both setups
like Bjørn's working, and get rid of the 2 steps problem. I think
adding an auto setting for brightness_switch_enabled and making that
the default is a good idea. But instead of using a timeout, I suggest
to make -1 / auto behave the same as 1 / on until userspace sets the
brightness. Once userspace has written to the brightness attribute
we start interpreting auto as 0 / off.
Rafael would you be willing to take a patch doing this ?
>> TL;DR: This change really is for the better and is here to stay.
>
> Wrong. We don't break existing setups, and your attitude needs fixing.
>
> Rafael, please get it reverted, or I will have to revert it. We have
> *long* had a rule that we don't break things "in order to improve
> things for others", and quite frankly, power management and ACPI in
> particular was exactly *why* that rule was introduced, because the
> whole "one step back, two steps forward" model does not work.
>
> The problem needs to be solved some other way, and developers need to
> f*cking stop with the "we can break peoples setups" mentality./
>
> Hans, seriously. You have the wrong mental model. Fix it.
Linus, normally I agree 101% with your no regressions policy. but I'm
also a big fan of the it just works philosophy. The problem here is
that when this switch got introduced a long time ago it got (in hindsight)
the wrong default. In order to make things just work it needs to get
fixed, but maybe we can come up with a solution to both have our cake
and eat it.
Regards,
Hans
Hans de Goede <[email protected]> writes:
> It breaks things on exotic setups,
I find your attitude extremely arrogant. If you want to run a
non-exotic setup then you should definitely not consider Linux for your
desktop system...
I am trying to help you sort out a bug in code you have written, before
it hits a release. I do not really expect to receive a beer for this,
but maybe a little less of the "yes, I know there is a bug and I don't
give a damn" attitude.
Yes, I can of course fix *my* system, making your reported bug count go
from 1 to 0. Does that help you in any way? How does that make your
projected number of affected users change?
Bjørn
Hi,
> On 07/14/2014 06:24 PM, Linus Torvalds wrote:
> Hans, seriously. You have the wrong mental model. Fix it.
> On 07/15/2014 Bjørn Mork wrote:
> I find your attitude extremely arrogant. If you want to run a
> non-exotic setup then you should definitely not consider Linux for your
> desktop system...
After stepping away from the keyboard and thinking about this for
a while I looked in the mirror and I saw someone who liked like
one of those developers who keeps taking away features to streamline
the user experience, and who does not care about your use-case because
it is not mainstream.
I've butted head with those kind of developers several times and
I really don't want to become like one of them. So you two are
right, and I apologize for my behavior.
So now lets go and fix this in way so that we can both have our
cake and eat it :)
On 07/15/2014 09:00 AM, Hans de Goede wrote:
<snip>
> I see that further down the thread you've send a patch to fix
> this by waiting sometime for userspace to react and if it does
> not then do the change in the kernel as it used to be.
>
> FWIW I don't think this is a good idea. This is inherently racy, so
> we will still sometimes see the backlight taking 2 steps leading to
> hard to debug problems.
>
> It has made me think about doing something to keep both setups
> like Bjørn's working, and get rid of the 2 steps problem. I think
> adding an auto setting for brightness_switch_enabled and making that
> the default is a good idea. But instead of using a timeout, I suggest
> to make -1 / auto behave the same as 1 / on until userspace sets the
> brightness. Once userspace has written to the brightness attribute
> we start interpreting auto as 0 / off.
Thinking more about this I can still think of several use cases which
will break with this change. So I believe that Linus' suggestion for
fixing this is probably the best solution and we will just have to
live with the race (if we hit the race we will behave as before and
take 2 steps).
After testing several laptops I've found one which exhibits the
2 step problem. For the record to reproduce this you need a laptop
with non intel gfx, as the xf86-video-intel driver caches the backlight
setting, and thus userspace does not see the kernel made changes
when it applies its own changes, so the backlight still gets stepped
twice, but the second step (done by userspace) just repeats the change
already done by the kernel.
Testing Linus' patch on that laptop unfortunately shows that it
does not fix the problem (while setting brightness_switch_enabled=0
does). I've already tried raising the delay to 250ms to no avail,
so I'm going to need to do some debugging on this. Besides that
the patch should be modified to not use globals as theoretically
there can be more then 1 acpi backlight device. Assuming I can get
the patch working (it should work in theory), I'll send a fixed
and cleaned-up version to Rafael for 3.17 .
Regards,
Hans