Support for brightness_in_hardware hal property

[Tomaz Solc-2]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi everyone

I see that g-p-m supported the "brightness_in_hardware" hal property in
2.15, but it was later removed. Respecting this property seems to me
like a good idea and I can't find the reason for removal in the
changelog. Was it intentional?

I've created a patch [1] for 2.22.1 that reintroduces this feature,
since it makes g-p-m work properly with LCD brightness hotkeys on my EeePC.

For some reason the check in the code ("current_hw !=
brightness->priv->last_set_hw") that should detect if hardware changed
the brightness isn't working in all cases on Eee (for example if you
keep the hotkey pressed for some time or when you reach the upper or
lower limit).

[1]
http://bugs.debian.org/cgi-bin/bugreport.cgi?msg=5;filename=respect-brightness-in-hardware.patch;att=1;bug=479089

Best regards
Tomaz Solc
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFIHEMIsAlAlRhL9q8RAg2+AKCjXqv6Y00PQ0AFqxGqmc2+oW+rsQCgpG7V
uYABG8zezHotutOdl6IGwyA=
=yGYF
-----END PGP SIGNATURE-----
_______________________________________________
gnome-power-manager-list mailing list
gnome-power-manager-list@...
http://mail.gnome.org/mailman/listinfo/gnome-power-manager-list

[Richard Hughes]

On Sat, 2008-05-03 at 12:48 +0200, Tomaž Šolc wrote:
> I see that g-p-m supported the "brightness_in_hardware" hal property in
> 2.15, but it was later removed. Respecting this property seems to me
> like a good idea and I can't find the reason for removal in the
> changelog. Was it intentional?

No, I don't think so - although trunk has had quite some refactoring in
that area.

> I've created a patch [1] for 2.22.1 that reintroduces this feature,
> since it makes g-p-m work properly with LCD brightness hotkeys on my EeePC.

Could you diff the patch against trunk please? Thanks.

Richard.


_______________________________________________
gnome-power-manager-list mailing list
gnome-power-manager-list@...
http://mail.gnome.org/mailman/listinfo/gnome-power-manager-list

[Tomaz Solc-2]

Hi

A while ago I sent a patch to this list that added support for
"brightness_in_hardware" hal property to g-p-m 2.22.1.

This is the same patch against the current SVN trunk, however I'm unable
to test it since the current g-p-m fails to start on my system:

(gnome-power-manager:2601): GLib-GObject-WARNING **:
/build/buildd/glib2.0-2.16.3/gobject/gsignal.c:1669: signal
`monitors_changed' is invalid for instance `0x80b40b0'
Segmentation fault

Backtrace is attached below.

Best regards
Tomaz


#0  0xb71f70f3 in strlen () from /lib/i686/cmov/libc.so.6
#1  0x08055de6 in gpm_cell_get_id (cell=0x81059c0) at gpm-cell.c:478
#2  0x08057d0e in gpm_cell_array_collection_changed (cell_array=0x8179140)
     at gpm-cell-array.c:466
#3  0x0805813b in gpm_cell_array_add_hal_udi (cell_array=0x8179140,
     udi=0x818e828 "/org/freedesktop/Hal/devices/acpi_BAT0")
     at gpm-cell-array.c:819
#4  0x0805822e in gpm_cell_array_coldplug (cell_array=0x8179140)
     at gpm-cell-array.c:855
#5  0x080583b1 in gpm_cell_array_set_type (cell_array=0x8179140,
     kind=GPM_CELL_UNIT_KIND_PRIMARY) at gpm-cell-array.c:879
#6  0x080517b3 in gpm_engine_start (engine=0x814b210) at gpm-engine.c:1027
#7  0x08069878 in gpm_manager_init (manager=0x80b6600) at gpm-manager.c:1813
#8  0xb73c6e66 in g_type_create_instance () from
/usr/lib/libgobject-2.0.so.0
#9  0xb73ac1e2 in ?? () from /usr/lib/libgobject-2.0.so.0
#10 0x080d86f0 in ?? ()
#11 0x00000000 in ?? ()

Index: gpm-brightness-hal.c
===================================================================
--- gpm-brightness-hal.c (revision 2822)
+++ gpm-brightness-hal.c (working copy)
@@ -58,6 +58,10 @@
  gchar *udi;
  gboolean hw_changed;
  DbusProxy *gproxy;
+
+ /* true if hardware automatically sets brightness in response to
+ * key press events */
+ gboolean does_own_updates;
 };
 
 enum {
@@ -315,7 +319,8 @@
  gpm_brightness_hal_get_hw (brightness, ¤t_hw);
 
  /* the panel has been updated in firmware */
- if (current_hw != brightness->priv->last_set_hw) {
+ if (current_hw != brightness->priv->last_set_hw ||
+            brightness->priv->does_own_updates) {
  brightness->priv->last_set_hw = current_hw;
  } else {
  /* macbook pro has a bazzillion brightness levels, be a bit clever */
@@ -357,7 +362,8 @@
  gpm_brightness_hal_get_hw (brightness, ¤t_hw);
 
  /* the panel has been updated in firmware */
- if (current_hw != brightness->priv->last_set_hw) {
+ if (current_hw != brightness->priv->last_set_hw ||
+              brightness->priv->does_own_updates) {
  gpm_brightness_hal_get_hw (brightness, &brightness->priv->last_set_hw);
  } else {
  /* macbook pro has a bazzillion brightness levels, be a bit clever */
@@ -438,6 +444,7 @@
  gchar **names;
  HalGManager *manager;
  HalGDevice *device;
+ gboolean res;
 
  brightness->priv = GPM_BRIGHTNESS_HAL_GET_PRIVATE (brightness);
  brightness->priv->gproxy = NULL;
@@ -466,6 +473,24 @@
  if (brightness->priv->levels == 0 || brightness->priv->levels > 256) {
  gpm_warning ("Laptop panel levels are invalid!");
  }
+
+ /* Check if hardware handles brightness changes automatically */
+ res = hal_gdevice_get_bool (device,
+    "laptop_panel.brightness_in_hardware",
+            &brightness->priv->does_own_updates, NULL);
+
+ if (!res) {
+ brightness->priv->does_own_updates = FALSE;
+ gpm_debug ("laptop_panel.brightness_in_hardware not found. "
+   "Assuming false");
+ } else {
+ if (brightness->priv->does_own_updates) {
+ gpm_debug ("laptop_panel.brightness_in_hardware: True");
+ } else {
+ gpm_debug ("laptop_panel.brightness_in_hardware: False");
+ }
+ }
+
  g_object_unref (device);
 
  /* get a managed proxy */

_______________________________________________
gnome-power-manager-list mailing list
gnome-power-manager-list@...
http://mail.gnome.org/mailman/listinfo/gnome-power-manager-list

[Richard Hughes]

On Sun, 2008-06-22 at 13:54 +0200, Tomaž Šolc wrote:
> This is the same patch against the current SVN trunk, however I'm unable
> to test it since the current g-p-m fails to start on my system:

Applied, thanks.

> (gnome-power-manager:2601): GLib-GObject-WARNING **:
> /build/buildd/glib2.0-2.16.3/gobject/gsignal.c:1669: signal
> `monitors_changed' is invalid for instance `0x80b40b0'

That's pretty harmless, you just need a newer GTK to support the
monitors changed stuff.

> Segmentation fault

Less cool. I've applied:

2008-06-23  Richard Hughes  <richard@...>

        * src/gpm-cell.c: (gpm_cell_get_id):
        Don't segfault in gpm_cell_get_id() if we don't have a model or
        serial number.

Can you retry svn head please? Thanks.

Richard.


_______________________________________________
gnome-power-manager-list mailing list
gnome-power-manager-list@...
http://mail.gnome.org/mailman/listinfo/gnome-power-manager-list

[Tomaz Solc-2]

Hi

> Can you retry svn head please? Thanks.

Thanks, that fixed the segfault. OTOH there are a couple of problems
with my patch now - the new g-p-m prefers to use xrandr to set the
brightness, which means that my changes no longer have any effect on the
Eee.

If I force the use of HAL in gpm_brightness_init(), the brightness
buttons work properly, but the feedback widget doesn't appear on the
screen.

I'm not sure what is the correct way to solve this problem. If xrandr is
now the preferred way, what is the best way to tell it that brightness
buttons work in hardware?

Best regards
Tomaz
_______________________________________________
gnome-power-manager-list mailing list
gnome-power-manager-list@...
http://mail.gnome.org/mailman/listinfo/gnome-power-manager-list

[Richard Hughes]

On Mon, 2008-06-23 at 14:01 +0200, Tomaž Šolc wrote:
> Hi
>
> > Can you retry svn head please? Thanks.
>
> Thanks, that fixed the segfault.

Cool, thanks.

> OTOH there are a couple of problems
> with my patch now - the new g-p-m prefers to use xrandr to set the
> brightness, which means that my changes no longer have any effect on the
> Eee.

I should be a lot faster to set with XRANDR - is that what you found
also?

> If I force the use of HAL in gpm_brightness_init(), the brightness
> buttons work properly, but the feedback widget doesn't appear on the
> screen.

Right, I guess the gpm_brightness_xrandr_X commands are not setting
hw_changed properly.

> I'm not sure what is the correct way to solve this problem. If xrandr is
> now the preferred way, what is the best way to tell it that brightness
> buttons work in hardware?

Well, there are two ways -- preferable would be to keep track of the
last brightness and then compare the current against the last, or we
could use the hal attribute in the xrandr code. I'm not sure which is
the best to do.

Richard.


_______________________________________________
gnome-power-manager-list mailing list
gnome-power-manager-list@...
http://mail.gnome.org/mailman/listinfo/gnome-power-manager-list

[Tomaz Solc-2]

Hi

> I should be a lot faster to set with XRANDR - is that what you found
> also?

I haven't noticed any difference in speed.

>> If I force the use of HAL in gpm_brightness_init(), the brightness
>> buttons work properly, but the feedback widget doesn't appear on the
>> screen.
>
> Right, I guess the gpm_brightness_xrandr_X commands are not setting
> hw_changed properly.

I thought widget is displayed when the "brightness-changed" signal is
emited (like in gpm_brightness_xrandr_may_have_changed() ).

I don't see any function that would emit this signal in
gpm_brightness_hal_*.

>> I'm not sure what is the correct way to solve this problem. If xrandr is
>> now the preferred way, what is the best way to tell it that brightness
>> buttons work in hardware?
>
> Well, there are two ways -- preferable would be to keep track of the
> last brightness and then compare the current against the last, or we
> could use the hal attribute in the xrandr code. I'm not sure which is
> the best to do.

Hm. It looks like gpm_brightness_xrandr_output_get_internal() doesn't
detect that the hardware has changed the brightness.

If I disable gpm_brightness_xrandr_output_set_internal(), then
output_get always returns the same value, regardless of the actual state
of the backlight.

So even if you use hal attribute, you can't get the real brightness
level through xrandr.

There is an added complication that on Eee you can't know which
brightness button was pressed (up or down - on mine both buttons behave
as BrightnessDown). I heard that some other Asus laptops also behave
like this.

Best regards
Tomaz
_______________________________________________
gnome-power-manager-list mailing list
gnome-power-manager-list@...
http://mail.gnome.org/mailman/listinfo/gnome-power-manager-list

[Richard Hughes]

On Tue, 2008-06-24 at 18:05 +0200, Tomaž Šolc wrote:
> > Right, I guess the gpm_brightness_xrandr_X commands are not setting
> > hw_changed properly.
>
> I thought widget is displayed when the "brightness-changed" signal is
> emited (like in gpm_brightness_xrandr_may_have_changed() ).
>
> I don't see any function that would emit this signal in
> gpm_brightness_hal_*.

Right, in the xrandr code it's just set to this, so it's never fired
either:

        if (FALSE) {
                gpm_brightness_xrandr_may_have_changed (brightness);
        }

I'm waiting for xrandr to send a "brightness changed" signal, but this
means implementing some code in the X server and I'm waiting for someone
else to fix it :-)

We probably need to hook this up in the HAL code, but HAL doesn't seem
to emit this either for me. This is all a mess -- I guess from 2.25 I'll
only support the xrandr way of doing things and then we'll just have to
fix the drivers.

> Hm. It looks like gpm_brightness_xrandr_output_get_internal() doesn't
> detect that the hardware has changed the brightness.
>
> If I disable gpm_brightness_xrandr_output_set_internal(), then
> output_get always returns the same value, regardless of the actual state
> of the backlight.
>
> So even if you use hal attribute, you can't get the real brightness
> level through xrandr.

Right, so this sounds like a X bug. When we do a query brightness, we
should always check the hardware and not use a cached value. Can you
open a bug against the X driver you are using please?

> There is an added complication that on Eee you can't know which
> brightness button was pressed (up or down - on mine both buttons behave
> as BrightnessDown). I heard that some other Asus laptops also behave
> like this.

Sure, but if we can read the backlight after we get the button press,
then it's trivial to reverse the logic if it's the "other way".

Thanks,

Richard.


_______________________________________________
gnome-power-manager-list mailing list
gnome-power-manager-list@...
http://mail.gnome.org/mailman/listinfo/gnome-power-manager-list

[Tomaz Solc-2]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi

>> Hm. It looks like gpm_brightness_xrandr_output_get_internal() doesn't
>> detect that the hardware has changed the brightness.
>>
>> If I disable gpm_brightness_xrandr_output_set_internal(), then
>> output_get always returns the same value, regardless of the actual state
>> of the backlight.
>>
>> So even if you use hal attribute, you can't get the real brightness
>> level through xrandr.
>
> Right, so this sounds like a X bug. When we do a query brightness, we
> should always check the hardware and not use a cached value. Can you
> open a bug against the X driver you are using please?

I've created a short program that tries to demonstrate this problem. It
uses the same way of getting brightness level from xrandr as g-p-m.

On my Eee, it will output the same value regardless of the actual
backlight setting.

Can someone please check if this works properly on some other hardware?
(i.e. if the value printed changes when you change the brightness level)
I can then file a bug for the Intel X.org driver.

Thanks
Tomaž
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFIjO2asAlAlRhL9q8RAjUDAKCU/JwwOoimpTfHZh9FCttlfjwcYACfcMdN
+jKe/CBaSR4mbjKYSuFHXD8=
=oquy
-----END PGP SIGNATURE-----

#include <stdio.h>

#include <X11/Xlib.h>
#include <X11/Xatom.h>
#include <X11/Xlib.h>
#include <X11/extensions/Xrandr.h>

int main() {
        Display *dpy;
        dpy = XOpenDisplay(":0.0");
        if(!dpy) {
                printf("Display\n");
                return 1;
        }

        Screen *scr;
        scr = XDefaultScreenOfDisplay(dpy);
        if(!scr) {
                printf("Screen\n");
                return 1;
        }

        Window root = RootWindowOfScreen(scr);

        Atom backlight = XInternAtom(dpy, "BACKLIGHT", True);
        if(backlight == None) {
                printf("Backlight Atom\n");
        }

        XRRScreenResources *resource;
        resource = XRRGetScreenResources(dpy, root);
        if(!resource || resource->noutput < 1) {
                printf("Resource\n");
                return 1;
        }

        printf("%d outputs\n", resource->noutput);

        int i;
        for(i = 0; i < resource->noutput; i++) {
                printf("%d\n", i);
                RROutput output = resource->outputs[i];

                unsigned long nitems;
                unsigned long bytes_after;
                unsigned char *prop;
                Atom actual_type;
                int actual_format;

                if(XRRGetOutputProperty(dpy, output, backlight,
                                         0, 4, False, False, None,
                                         &actual_type, &actual_format,
                                         &nitems, &bytes_after, &prop) != Success) {
                        printf("XRRGetOutputProperty\n");
                }

                if (actual_type == XA_INTEGER && nitems == 1 && actual_format == 32) {
                        printf("Brightness setting: %d\n", *((int *) prop));
                }
        }

        return 0;
}

_______________________________________________
gnome-power-manager-list mailing list
gnome-power-manager-list@...
http://mail.gnome.org/mailman/listinfo/gnome-power-manager-list