Re: [BISECTED 3.16-rc REGREGRESSION] backlight control stopped working

[Hans de Goede]

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 <hdegoede@xxxxxxxxxx>
> 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 <hdegoede@xxxxxxxxxx>
>     Reviewed-by: Aaron Lu <aaron.lu@xxxxxxxxx>
>     Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> 
> :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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/