GNOME Bugzilla – Bug 744278
power: On raw backlight types, clamp the value to a minumum
Last modified: 2017-05-04 12:42:47 UTC
As advised by kernel developers.
Created attachment 296521 [details] [review] power: Refactor the backlight helper a bit We'll need to read values before setting a new one in a coming patch so factor those out.
Created attachment 296522 [details] [review] power: Return the backlight type along with the file path We'll need to apply an heuristic according to the backlight type, so return it along with the path.
Created attachment 296523 [details] [review] power: On raw backlight types, clamp the value to a minumum On raw backlight types, it's likely that setting it to zero will completely turn the backlight off which isn't very useful. Instead, let's always clamp the value to a minimum in that case. For now this is 1% of the max backlight value but we might tweak it as needed.
Review of attachment 296521 [details] [review]: ::: plugins/power/gsd-backlight-helper.c @@ +50,3 @@ gboolean ret = TRUE; + filename_file = g_build_filename (filename, "brightness", NULL); That's filename_path. @@ +84,3 @@ + gint value; + + if (g_file_get_contents (filename, &contents, NULL, error)) { No need for the braces here.
Review of attachment 296522 [details] [review]: Sure.
Review of attachment 296523 [details] [review]: It should be equivalent to "one step", so it shouldn't be based on a percentage, but on the max brightness. (max brightness of 7 means a max of 7 steps, and that would mean the minimum is 15%). I believe this also doesn't clamp the UI, showing it being stuck at just above 0 when reducing the brightness.
(In reply to Bastien Nocera from comment #6) > It should be equivalent to "one step", so it shouldn't be based on a > percentage, but on the max brightness. (max brightness of 7 means a max of 7 > steps, and that would mean the minimum is 15%). I don't agree it should be one step because for the UI this is actually the 0 value, otherwise we would be needlessly removing on step from the available range. > I believe this also doesn't clamp the UI, showing it being stuck at just > above 0 when reducing the brightness. Right, it doesn't clamp the UI but I think that's fine because from the UI POV this should actually be 0 i.e. the minimum value. We're just hiding the fact that 0 for the UI isn't really 0 for the kernel so as to not turn off the backlight. Also, AFAIK for this type of interface, i.e. interfaces for which we don't want to go all the way to 0, we usually have a big range of values. On this laptop I have a range of 0 up to 851. This means that if max_brightness is actually < 100 then we'll clamp to 0, i.e. we won't clamp at all, which I think is fine since in that case 0 is probably fine. I mean this is just an heuristic, if we get bug reports we can add more special casing.
(In reply to Rui Matos from comment #7) > (In reply to Bastien Nocera from comment #6) > > It should be equivalent to "one step", so it shouldn't be based on a > > percentage, but on the max brightness. (max brightness of 7 means a max of 7 > > steps, and that would mean the minimum is 15%). > > I don't agree it should be one step because for the UI this is actually the > 0 value, otherwise we would be needlessly removing on step from the > available range. And we'll start receiving bug reports that say that brightness can't go all the way down. When the brightness is at the minimum reachable, it should be at 0 in the UI. It's going to make the behaviour of the brightness slider in gnome-shell and gnome-control-center really obnoxious otherwise. > > I believe this also doesn't clamp the UI, showing it being stuck at just > > above 0 when reducing the brightness. > > Right, it doesn't clamp the UI but I think that's fine because from the UI > POV this should actually be 0 i.e. the minimum value. We're just hiding the > fact that 0 for the UI isn't really 0 for the kernel so as to not turn off > the backlight. > > Also, AFAIK for this type of interface, i.e. interfaces for which we don't > want to go all the way to 0, we usually have a big range of values. On this > laptop I have a range of 0 up to 851. This means that if max_brightness is > actually < 100 then we'll clamp to 0, i.e. we won't clamp at all, which I > think is fine since in that case 0 is probably fine. I mean this is just an > heuristic, if we get bug reports we can add more special casing. Does "1" keep the backlight on, and "0" turns off the screen?
(In reply to Bastien Nocera from comment #8) > When the brightness is at the minimum reachable, it should be > at 0 in the UI. Yes! That's what I've been saying too and it's how it works with this patch. > It's going to make the behaviour of the brightness slider in > gnome-shell and gnome-control-center really obnoxious otherwise. They work perfectly fine. When you turn them all over to 0, the brightness goes to a "minimum value". It just happens that behind the scenes that value isn't actually 0, but for the UI it is 0. > Does "1" keep the backlight on, and "0" turns off the screen? On this laptop, yes. Although IMO, "1" is too low already and that's why I chose 1% which is actually 8 for this laptop.
Created attachment 296948 [details] [review] power: Refactor the backlight helper a bit -- Changed the variable name and removed the braces
Review of attachment 296948 [details] [review]: Sure.
(In reply to Rui Matos from comment #9) > (In reply to Bastien Nocera from comment #8) > > When the brightness is at the minimum reachable, it should be > > at 0 in the UI. > > Yes! That's what I've been saying too and it's how it works with this patch. OK. > > It's going to make the behaviour of the brightness slider in > > gnome-shell and gnome-control-center really obnoxious otherwise. > > They work perfectly fine. When you turn them all over to 0, the brightness > goes to a "minimum value". It just happens that behind the scenes that value > isn't actually 0, but for the UI it is 0. > > > Does "1" keep the backlight on, and "0" turns off the screen? > > On this laptop, yes. Although IMO, "1" is too low already and that's why I > chose 1% which is actually 8 for this laptop. If the backlight is still on, I'd really prefer if it was clamped at the actual "1" value, rather than "8" (1%) which might have a different effect on other laptops.
Created attachment 296955 [details] [review] power: On raw backlight types, clamp the value to a minumum On raw backlight types, it's likely that setting it to zero will completely turn the backlight off which isn't very useful. Instead, let's always clamp the value to a minimum in that case. For now we just clamp if the backlight has at least 100 levels but we might tweak it as needed.
Review of attachment 296955 [details] [review]: Looks good.
Attachment 296522 [details] pushed as 82bc476 - power: Return the backlight type along with the file path Attachment 296948 [details] pushed as c5e9138 - power: Refactor the backlight helper a bit Attachment 296955 [details] pushed as bb5c1ae - power: On raw backlight types, clamp the value to a minumum
*** Bug 747791 has been marked as a duplicate of this bug. ***
*** Bug 719949 has been marked as a duplicate of this bug. ***
*** Bug 766291 has been marked as a duplicate of this bug. ***
Apologies I'm not very technical and I've just only started to explore debian. What is the process with fixing this bug, how long can I expect it to be fixed? (I don't want to have a black screen at minimum brightness). I see that it says RESOLVED FIXED, does that mean we're just waiting for release? Please advise as I am a newbie...
(In reply to philipw from comment #19) > Apologies I'm not very technical and I've just only started to explore > debian. What is the process with fixing this bug, how long can I expect it > to be fixed? It's fixed already. > (I don't want to have a black screen at minimum brightness). > > I see that it says RESOLVED FIXED, does that mean we're just waiting for > release? But Debian hasn't picked it up. File a bug against Debian to request them to pick up this fix.
I can still reduplicate this issue on Dell Precision 5720 AIO. It only has 9 levels in /sys/class/backlight/intel_backlight/max_brightness because the wrong settings in VBIOS. How can we fix this problem?
(In reply to Shih-Yuan Lee (FourDollars) from comment #21) > I can still reduplicate this issue on Dell Precision 5720 AIO. It only has 9 > levels in /sys/class/backlight/intel_backlight/max_brightness because the > wrong settings in VBIOS. How can we fix this problem? Please follow this up in the new bug you filed. This patch has already been in 2 releases, digging it up again won't help.