After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 710380 - Fn Keys Wont Step up and down brightness
Fn Keys Wont Step up and down brightness
Status: RESOLVED FIXED
Product: gnome-settings-daemon
Classification: Core
Component: power
3.10.x
Other Linux
: Normal major
: ---
Assigned To: gnome-settings-daemon-maint
gnome-settings-daemon-maint
Depends on:
Blocks:
 
 
Reported: 2013-10-17 12:11 UTC by Asad Mehmood
Modified: 2013-10-24 20:24 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Retry settings brightness (2.00 KB, patch)
2013-10-17 12:18 UTC, Asad Mehmood
rejected Details | Review
mutter patch (1.27 KB, patch)
2013-10-17 16:22 UTC, Asad Mehmood
none Details | Review
gnome-desktop patch (2.64 KB, patch)
2013-10-17 16:23 UTC, Asad Mehmood
none Details | Review
gnome-settings-daemon patch (1.73 KB, patch)
2013-10-17 16:23 UTC, Asad Mehmood
none Details | Review
mutter patch (1.27 KB, patch)
2013-10-17 17:07 UTC, Asad Mehmood
accepted-commit_now Details | Review
gnome-desktop patch (2.63 KB, patch)
2013-10-17 17:07 UTC, Asad Mehmood
committed Details | Review
gnome-settings-daemon patch (1.68 KB, patch)
2013-10-17 17:08 UTC, Asad Mehmood
accepted-commit_now Details | Review
Gnome-Settings_Daemon patch (1.73 KB, patch)
2013-10-17 17:15 UTC, Asad Mehmood
needs-work Details | Review
mutter patch (1.38 KB, patch)
2013-10-19 23:57 UTC, Asad Mehmood
reviewed Details | Review
gnome-settings-daemon patch (1.73 KB, patch)
2013-10-19 23:58 UTC, Asad Mehmood
committed Details | Review
mutter patch (1.38 KB, patch)
2013-10-20 12:11 UTC, Asad Mehmood
committed Details | Review

Description Asad Mehmood 2013-10-17 12:11:27 UTC
On my system:

Samsung RV511 laptop

The FN Keys (FN+UP and FN+DOWN) for brightness will show the OSD but not change the brightness. The slider in the power panel will change the power.
Comment 1 Asad Mehmood 2013-10-17 12:18:12 UTC
Created attachment 257510 [details] [review]
Retry settings brightness

This keeps trying to increase or decrease the brightness until it actually succeeds
Comment 2 Bastien Nocera 2013-10-17 12:47:05 UTC
Review of attachment 257510 [details] [review]:

That's absolutely out of the question. XRandR should queue, and cache the values to be used.
The driver for your graphics card needs fixing.
Comment 3 Asad Mehmood 2013-10-17 16:22:41 UTC
Created attachment 257557 [details] [review]
mutter patch
Comment 4 Asad Mehmood 2013-10-17 16:23:00 UTC
Created attachment 257558 [details] [review]
gnome-desktop patch
Comment 5 Asad Mehmood 2013-10-17 16:23:20 UTC
Created attachment 257559 [details] [review]
gnome-settings-daemon patch
Comment 6 Giovanni Campagna 2013-10-17 16:35:41 UTC
Uhm, the patches look good, but I'd expose the minimum backlight step in percentage rather than the number of steps (which is in some unspecified HW-dependent unit, while everything else is %)
Comment 7 Asad Mehmood 2013-10-17 17:07:34 UTC
Created attachment 257564 [details] [review]
mutter patch

Expose minimum step as a percentage. We dont need to add 1 ... 0-4 is 5 different levels but they would be 0,25,50,75,100
Comment 8 Asad Mehmood 2013-10-17 17:07:58 UTC
Created attachment 257565 [details] [review]
gnome-desktop patch
Comment 9 Asad Mehmood 2013-10-17 17:08:43 UTC
Created attachment 257566 [details] [review]
gnome-settings-daemon patch

Hopefully I wont have to recompile mutter, gsd and gnome-desktop again for a while.
Comment 10 Giovanni Campagna 2013-10-17 17:10:40 UTC
Review of attachment 257564 [details] [review]:

Looks good.
Comment 11 Giovanni Campagna 2013-10-17 17:11:07 UTC
Review of attachment 257565 [details] [review]:

Yes
Comment 12 Giovanni Campagna 2013-10-17 17:12:05 UTC
Review of attachment 257566 [details] [review]:

Just a style nit.

::: plugins/power/gpm-common.c
@@ +1276,3 @@
                 if (now < 0)
                        return percentage_value;
+                step = MAX (gnome_rr_output_get_min_backlight_step(output), BRIGHTNESS_STEP_AMOUNT (max - min + 1));

Space before (
Comment 13 Asad Mehmood 2013-10-17 17:15:55 UTC
Created attachment 257570 [details] [review]
Gnome-Settings_Daemon patch

Style fixes
Comment 14 Conley Moorhous 2013-10-18 20:34:33 UTC
I have the same issue with the RV520. Thanks for fixing this Asad!
Comment 15 Bastien Nocera 2013-10-19 22:08:14 UTC
Review of attachment 257570 [details] [review]:

::: plugins/power/gpm-common.c
@@ +1276,3 @@
                 if (now < 0)
                        return percentage_value;
+                step = MAX (gnome_rr_output_get_min_backlight_step(output), BRIGHTNESS_STEP_AMOUNT (max - min + 1));

Space before the bracket.

@@ +1335,3 @@
                 if (now < 0)
                        return percentage_value;
+                step = MAX (gnome_rr_output_get_min_backlight_step(output), BRIGHTNESS_STEP_AMOUNT (max - min + 1));

Ditto.
Comment 16 Asad Mehmood 2013-10-19 23:57:00 UTC
Created attachment 257732 [details] [review]
mutter patch

Add some error handling ... so it doesn't divide by zero ... oops
Comment 17 Asad Mehmood 2013-10-19 23:58:02 UTC
Created attachment 257733 [details] [review]
gnome-settings-daemon patch

Some more style fixes ... How wrong can I make 2 lines of code
Comment 18 Bastien Nocera 2013-10-20 06:56:21 UTC
Review of attachment 257733 [details] [review]:

Looks good :)
Comment 19 Bastien Nocera 2013-10-20 06:57:52 UTC
Review of attachment 257732 [details] [review]:

::: src/core/monitor.c
@@ +808,3 @@
                              g_variant_new_int32 (output->backlight));
+      g_variant_builder_add (&properties, "{sv}", "min-backlight-step",
+                             g_variant_new_int32 ( (output->backlight_max - output->backlight_min) ?

it should read:
g_variant_new_int32 ((output->backlight_max - output->backlight_min) ?
there's no need for the second space, it's a space after function names, before the bracket.
Comment 20 Asad Mehmood 2013-10-20 12:11:37 UTC
Created attachment 257747 [details] [review]
mutter patch
Comment 21 Bastien Nocera 2013-10-21 21:30:53 UTC
And all committed, many thanks for the patches and iterating over them.
Comment 22 Adam Williamson 2013-10-23 21:47:53 UTC
FWIW, I've had this problem on my laptop for a few weeks, just managed to find this bug report - many thanks for the fixes, folks. hadess, can this fix go to 3.10 ? I'm in the middle of doing scratch builds to check that the patches apply and the fix works, but it'd be a pain to keep doing my own side builds each time any of those three is bumped...thanks!
Comment 23 Adam Williamson 2013-10-23 22:37:50 UTC
Confirmed the fix, with all three patched components rebuilt, on my 2010 Vaio Z (VPCZ1). This is a moderately popular laptop (it has its own mailing list!), so backporting the fix would be appreciated by a few folks, I think.