backlight control stopped working

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.

: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
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Hi,

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.

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-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

On my laptop it changed the behaviour from working to non-working, which is a regression whether it was intended or not.

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.

You can drop the "may". I have already told you that it broke my configuration, haven't I?

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?

Now I believe we are moving out on the prairie here. I am concerned about the *kernel*, not desktop environments.

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
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

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.

.. 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?

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
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

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

 drivers/acpi/video.c | 42 ++++++++++++++++++++++++++++++------------
 1 file changed, 30 insertions(+), 12 deletions(-)

index 071c1dfb93f3..9c4afface8e7 100644
@@ -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

@@ -270,11 +270,21 @@ static int acpi_video_get_brightness(struct backlight_device *bd)
 	return 0;
 }
 
+static u32 acpi_brightness_event;
+static struct acpi_video_device *acpi_brightness_device;
+static void acpi_video_set_brighness_delayed(struct work_struct *work)
+{
+	acpi_video_switch_brightness(acpi_brightness_device, acpi_brightness_event);
+}
+
+static DECLARE_DELAYED_WORK(acpi_brightness_work, acpi_video_set_brighness_delayed);
+
 static int acpi_video_set_brightness(struct backlight_device *bd)
 {
 	int request_level = bd->props.brightness + 2;
 	struct acpi_video_device *vd = bl_get_data(bd);
 
+	cancel_delayed_work(&acpi_brightness_work);
 	return acpi_video_device_lcd_set_level(vd,
 				vd->brightness->levels[request_level]);
 }

@@ -1601,6 +1611,19 @@ static void acpi_video_bus_notify(struct acpi_device *device, u32 event)
 	return;
 }
 
+static void brightness_switch_event(struct acpi_video_device *video_device, u32 event)
+{
+	if (!brightness_switch_enabled)
+		return;
+	if (brightness_switch_enabled > 0) {
+		acpi_video_switch_brightness(video_device, event);
+		return;
+	}
+	acpi_brightness_device = video_device;
+	acpi_brightness_event = event;
+	schedule_delayed_work(&acpi_brightness_work, HZ/10);
+}
+
 static void acpi_video_device_notify(acpi_handle handle, u32 event, void *data)
 {
 	struct acpi_video_device *video_device = data;

@@ -1618,28 +1641,23 @@ static void acpi_video_device_notify(acpi_handle handle, u32 event, void *data)
 
 	switch (event) {
 	case ACPI_VIDEO_NOTIFY_CYCLE_BRIGHTNESS:	/* Cycle brightness */
-		if (brightness_switch_enabled)
-			acpi_video_switch_brightness(video_device, event);
+		brightness_switch_event(video_device, event);
 		keycode = KEY_BRIGHTNESS_CYCLE;
 		break;
 	case ACPI_VIDEO_NOTIFY_INC_BRIGHTNESS:	/* Increase brightness */
-		if (brightness_switch_enabled)
-			acpi_video_switch_brightness(video_device, event);
+		brightness_switch_event(video_device, event);
 		keycode = KEY_BRIGHTNESSUP;
 		break;
 	case ACPI_VIDEO_NOTIFY_DEC_BRIGHTNESS:	/* Decrease brightness */
-		if (brightness_switch_enabled)
-			acpi_video_switch_brightness(video_device, event);
+		brightness_switch_event(video_device, event);
 		keycode = KEY_BRIGHTNESSDOWN;
 		break;
 	case ACPI_VIDEO_NOTIFY_ZERO_BRIGHTNESS:	/* zero brightness */
-		if (brightness_switch_enabled)
-			acpi_video_switch_brightness(video_device, event);
+		brightness_switch_event(video_device, event);
 		keycode = KEY_BRIGHTNESS_ZERO;
 		break;
 	case ACPI_VIDEO_NOTIFY_DISPLAY_OFF:	/* display device off */
-		if (brightness_switch_enabled)
-			acpi_video_switch_brightness(video_device, event);
+		brightness_switch_event(video_device, event);
 		keycode = KEY_DISPLAY_OFF;
 		break;
 	default:

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.

That sounds like a good solution to me FWIW.

Bjørn
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Try the patch. It *might* work. I'm not saying it will, but it seemed to at least compile for me.

Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Yes, the patch works as advertised for me. Thanks.

But this will break existing configs:

Ok, so it should probably be tested by people who see the "move by two" problem too.

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

I've queued up a revert already. I'll include it into my next pull request later this week.

Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Hi,

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.

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.

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 ?

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
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

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
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Hi,

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>

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
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html