GNOME Bugzilla – Bug 733044
monitor-manager-xrandr: Normalize the minimum brightness level.
Last modified: 2014-09-18 10:23:28 UTC
According to RandR X protocol version 1.4.0, the minimum brightness can't be 0. 0 is used to turn off the backlight. The minimum brightness can only be 1. http://cgit.freedesktop.org/xorg/proto/randrproto/tree/randrproto.txt
Created attachment 280467 [details] [review] The patch for monitor-manager-xrandr.
(In reply to comment #0) > According to RandR X protocol version 1.4.0, the minimum brightness can't be 0. > 0 is used to turn off the backlight. The minimum brightness can only be 1. > > http://cgit.freedesktop.org/xorg/proto/randrproto/tree/randrproto.txt That's not exactly what it says: " This property controls the brightness on laptop panels and equivalent displays with a backlight controller. The driver specific maximum value MUST turn the backlight to full brightness, 1 SHOULD turn the backlight to minimum brightness, 0 SHOULD turn the backlight off." And I'd rather not fudge the values in intermediate layers which is just prone to make it more confusing when debugging things in the future. What are you trying to fix here?
When I pressed brightness function key on some laptops to turn the brightness level to the lowest. It turns off the backlight. This is not the behavior I want so I did some investigation of gnome-settings-daemon. I thought that was the problem of gsd-backlight-helper in gnome-settings-daemon. But it is not. It is because gnome-settings-daemon will invoke the gnome-rr of gnome-desktop. Now some part of gnome-rr in gnome-desktop is moved to monitor-manager-xrandr of mutter now. I also heard that the following linux kernel will switch to use the video driver to control the brightness of the laptop backlight by default. And I did some investigation for Intel video driver and I found it does follow the RandR X protocol that 0 is to turn the backlight off. So I made this patch, and it does fix my problem. According to the original source code, I think it is not intent to turn off the backlight. It is used for the minimum brightness level instead of turning off the backlight.
http://msdn.microsoft.com/en-us/library/windows/hardware/ff569755(v=vs.85).aspx "Brightness levels are represented as single-byte values in the range from zero to 100 where zero is off and 100 is the maximum brightness that a laptop computer supports. Every laptop computer must report a maximum brightness level of 100; however, a laptop computer is not required to support a level of zero. The only requirement for values from zero to 100 is that larger values must represent higher brightness levels."
Created attachment 282238 [details] [review] Adjust the minimum brightness level when the maximum >= 100. I made another patch. This patch should be better than previous one, and it should not affect the computers shipped before Windows 8 came out.
Created attachment 282239 [details] [review] Adjust the minimum brightness level when the maximum >= 100. Adjust the patch title.
Some information updates: Recent Linux kernel 3.16 is using /sys/class/backlight/intel_backlight directly to control the brightness on Intel UMA laptops of Haswell and Broadwell by default. The value of /sys/class/backlight/intel_backlight/max_brightness is 937. If we set 0 into /sys/class/backlight/intel_backlight/brightness, it will turn off the backlight. I think this is not the behavior that the most users want. We got lots of feedbacks from laptop computers ODMs for this issue. I will provide another revised patch for reviewing.
Created attachment 282623 [details] [review] monitor-manager-xrandr: Normalized the minimum brightness level.
Created attachment 282636 [details] [review] Normalize the minimum brightness level. This patch is aimed to normalize the minimum brightness level for 20 brightness steps used in gnome-settings-daemon and bring a better and consistent user experience for the brigtness control in gnome-settings-daemon and gnome-control-center. For example, most Intel Haswell/Broadwell UMA laptops have the same 937 value as their maximum brightness level on Linux kernel 3.16 by default. By this patch, the minimum brightness level will be changed to 17 because 937 / 20 == 46 and 937 % 46 == 17, and 17 is a reasonable and visible brightness level as the minimum brightness level on these laptops. If we set 0 to /sys/class/backlight/intel_backlight/brightness, it makes the screen off. If we set 1 to /sys/class/backlight/intel_backlight/brightness, the screen becomes hard to see the content. So 17 is a better brightness level. This patch should not affect existing ACPI brightness control especially for those laptops having the maximum brightness level under 20. BTW, this also fixes some wrong variable type, wrong calculation, and avoid the screen off when using the minimum brightness level under some condition.
No. That's not the right way to fix this. The first step is to fix it at the kernel level, so that the kernel documentation actually says what happens at level 0. Until there's a resolution there, any user-space changes should wait. See this file for the under specification: https://www.kernel.org/doc/Documentation/ABI/stable/sysfs-class-backlight
IIRC, mutter works with X RandR protocol and it has a description as the following: "Backlight" aka RR_PROPERTY_BACKLIGHT Type: INTEGER Format: 32 Num. items: 1 Flags: - Range/List: 0-x (driver specific) This property controls the brightness on laptop panels and equivalent displays with a backlight controller. The driver specific maximum value MUST turn the backlight to full brightness, 1 SHOULD turn the backlight to minimum brightness, 0 SHOULD turn the backlight off. http://cgit.freedesktop.org/xorg/proto/randrproto/tree/randrproto.txt My patch is aimed to X RandR protocol but not the kernel brightness system interface. BTW, gnome-settings-daemon does have a fallback to access the kernel brightness system interface directly, but it should not be included in this scope. Why do we need to fix it at the kernel level especially when there is already a such document for X RandR protocol?
SHOULD != MUST, and the XRandR interface is just a small shim on top of the Linux kernel interface. That means that if we fixed it the way your patch does, we'd get reports that some people have one less level of backlight than they should, and the fallback support in gnome-settings-daemon would still suffer from the same bug (which is used right now by every device except Intel video cards).
What if I make a similar patch for gnome-settings-daemon? The behavior of gnome-settings-daemon and gnome-control-center will become consistent. Is it reasonable way to fix the issue?
(In reply to comment #13) > What if I make a similar patch for gnome-settings-daemon? > The behavior of gnome-settings-daemon and gnome-control-center will become > consistent. > Is it reasonable way to fix the issue? No, because you would still have the problem of not knowing whether 0 turns off the backlight, or is the lowest level of backlight for a particular machine.
How about making a gsetting key to let the user decide backlight 0 is off or not?
(In reply to comment #15) > How about making a gsetting key to let the user decide backlight 0 is off or > not? That would break when you have 2 outputs that support backlight setting. And it's a stupid way of supporting quirky hardware.
OK, I see. I will go to trace the source code of Linux kernel to see what is the proper way to fix the problem. Thanks for your feedback. :)
After some investigation, there is no definition about if brightness level 0 is to turn off the display in Linux kernel, and there is no way to know if brightness level 0 is to turn off the display from any Linux kernel system interface. However the behavior is designed by hardware vendors. So far the solution I can figure out is to provide a gsetting key and let users configure their own the minimum brightness level. It could be 0 by default, but the user can change it to any other value less than or equal to the maximum brightness level. If the value is bigger than the maximum, it becomes invalid and fallback to 0. Is it a reasonable solution?
(In reply to comment #18) > After some investigation, there is no definition about if brightness level 0 is > to turn off the display in Linux kernel, Which is why I asked for you to contact kernel developers so that there *is* a definition. > and there is no way to know if > brightness level 0 is to turn off the display from any Linux kernel system > interface. > However the behavior is designed by hardware vendors. Then quirking is needed in the kernel or in udev to tag such devices. > So far the solution I can figure out is to provide a gsetting key and let users > configure their own the minimum brightness level. No. Never. > It could be 0 by default, but the user can change it to any other value less > than or equal to the maximum brightness level. > If the value is bigger than the maximum, it becomes invalid and fallback to 0. > Is it a reasonable solution? Absolutely not.
I found this issue is fixed in Linux kernel. http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=6dda730e55f412a6dfb181cae6784822ba463847.
(In reply to comment #20) > I found this issue is fixed in Linux kernel. > http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=6dda730e55f412a6dfb181cae6784822ba463847. That's one driver fixed, I guess... There's still no policy as to what drivers are supposed to do, or patches for all the other drivers that "do it wrong".