Rationalise ACPI backlight implementation

PATCH Applied Matthew Garrett 6 participants 16 messages Dec 26, 2007 – Feb 6, 2008 ACPI

Share & tools

Matthew Garrett

author

2007-12-26 02:03 UTC #0 lore raw

PATCH 1/1 Rationalise ACPI backlight implementation

The sysfs backlight class provides no mechanism for querying the acceptable brightness for a backlight. The ACPI spec states that values are only valid if they are reported as available by the firmware. Since we can't provide that information to userspace, instead collapse the range to the number of actual values that can be set.

Signed-off-by Matthew Garrett <[email protected]>

---

drivers/acpi/video.c +13 -7

index 521645e..12b2adb 100644
@@ -296,18 +296,26 @@ static int acpi_video_device_set_state(struct acpi_video_device *device, int sta
 static int acpi_video_get_brightness(struct backlight_device *bd)
 {
 	unsigned long cur_level;
+	int i;
 	struct acpi_video_device *vd =
 		(struct acpi_video_device *)bl_get_data(bd);
 	acpi_video_device_lcd_get_level_current(vd, &cur_level);
-	return (int) cur_level;
+	for (i=2; i<vd->brightness->count; i++) {
+		if (vd->brightness->levels[i] == cur_level)
+			/* The first two entries are special - see page 575
+			   of the ACPI spec 3.0 */
+			return i-2;
+	}
+	return 0;
 }
 
 static int acpi_video_set_brightness(struct backlight_device *bd)
 {
-	int request_level = bd->props.brightness;
+	int request_level = bd->props.brightness+2;
 	struct acpi_video_device *vd =
 		(struct acpi_video_device *)bl_get_data(bd);
-	acpi_video_device_lcd_set_level(vd, request_level);
+	acpi_video_device_lcd_set_level(vd, 
+					vd->brightness->levels[request_level]);
 	return 0;
 }

@@ -656,7 +664,6 @@ static void acpi_video_device_find_cap(struct acpi_video_device *device)
 	kfree(obj);
 
 	if (device->cap._BCL && device->cap._BCM && device->cap._BQC && max_level > 0){
-		unsigned long tmp;
 		static int count = 0;
 		char *name;
 		name = kzalloc(MAX_NAME_LEN, GFP_KERNEL);

@@ -664,11 +671,10 @@ static void acpi_video_device_find_cap(struct acpi_video_device *device)
 			return;
 
 		sprintf(name, "acpi_video%d", count++);
-		acpi_video_device_lcd_get_level_current(device, &tmp);
 		device->backlight = backlight_device_register(name,
 			NULL, device, &acpi_backlight_ops);
-		device->backlight->props.max_brightness = max_level;
-		device->backlight->props.brightness = (int)tmp;
+		device->backlight->props.max_brightness = device->brightness->count-3;
+		device->backlight->props.brightness = acpi_video_get_brightness(device->backlight);
 		backlight_update_status(device->backlight);
 
 		kfree(name);

Signature · 1 line

Matthew Garrett | [email protected]

Matthew Garrett

author

2008-01-14 01:51 UTC #1 lore raw

Len, I've had no feedback on this - the backlight maintainer thinks it's the right way to go, so I'd like to get it queued for .25 at least.

Signature · 1 line

Matthew Garrett | [email protected]

Zhang Rui

2008-01-22 08:28 UTC #2 lore raw

In reply to Matthew Garrett

Quoted 6 lines · Matthew Garrett

The sysfs backlight class provides no mechanism for querying the acceptable brightness for a backlight. The ACPI spec states that values are only valid if they are reported as available by the firmware. Since we can't provide that information to userspace, instead collapse the range to the number of actual values that can be set.

There is another patch which round the bd->props.brightness to the nearest but larger acceptable brightness level.
Please refer to http://bugzilla.kernel.org/show_bug.cgi?id=9277

I have no obvious objection on either of these two proposals. But one thing to mention is that
both of these two patches is written on the assumption that the brightness levels listed in _BCL method are in ascending order, while this is not stated in the ACPI spec.
Is this a problem?

Thanks,
Rui

Quoted 61 lines

Signed-off-by Matthew Garrett <[email protected]>

---

drivers/acpi/video.c +13 -7

index 521645e..12b2adb 100644
@@ -296,18 +296,26 @@ static int acpi_video_device_set_state(struct acpi_video_device *device, int sta
 static int acpi_video_get_brightness(struct backlight_device *bd)
 {
 	unsigned long cur_level;
+	int i;
 	struct acpi_video_device *vd =
 		(struct acpi_video_device *)bl_get_data(bd);
 	acpi_video_device_lcd_get_level_current(vd, &cur_level);
-	return (int) cur_level;
+	for (i=2; i<vd->brightness->count; i++) {
+		if (vd->brightness->levels[i] == cur_level)
+			/* The first two entries are special - see page 575
+			   of the ACPI spec 3.0 */
+			return i-2;
+	}
+	return 0;
 }
 
 static int acpi_video_set_brightness(struct backlight_device *bd)
 {
-	int request_level = bd->props.brightness;
+	int request_level = bd->props.brightness+2;
 	struct acpi_video_device *vd =
 		(struct acpi_video_device *)bl_get_data(bd);
-	acpi_video_device_lcd_set_level(vd, request_level);
+	acpi_video_device_lcd_set_level(vd, 
+					vd->brightness->levels[request_level]);
 	return 0;
 }

@@ -656,7 +664,6 @@ static void acpi_video_device_find_cap(struct acpi_video_device *device)
 	kfree(obj);
 
 	if (device->cap._BCL && device->cap._BCM && device->cap._BQC && max_level > 0){
-		unsigned long tmp;
 		static int count = 0;
 		char *name;
 		name = kzalloc(MAX_NAME_LEN, GFP_KERNEL);

@@ -664,11 +671,10 @@ static void acpi_video_device_find_cap(struct acpi_video_device *device)
 			return;
 
 		sprintf(name, "acpi_video%d", count++);
-		acpi_video_device_lcd_get_level_current(device, &tmp);
 		device->backlight = backlight_device_register(name,
 			NULL, device, &acpi_backlight_ops);
-		device->backlight->props.max_brightness = max_level;
-		device->backlight->props.brightness = (int)tmp;
+		device->backlight->props.max_brightness = device->brightness->count-3;
+		device->backlight->props.brightness = acpi_video_get_brightness(device->backlight);
 		backlight_update_status(device->backlight);
 
 		kfree(name);

Matthew Garrett

author

2008-01-22 11:46 UTC #3 lore raw

Quoted 6 lines · Zhang Rui

I have no obvious objection on either of these two proposals. But one thing to mention is that
both of these two patches is written on the assumption that the brightness levels listed in _BCL method are in ascending order, while this is not stated in the ACPI spec.
Is this a problem?

The driver already makes that assumption, and it's implicit in the spec.

Signature · 1 line

Matthew Garrett | [email protected]

Henrique de Moraes Holschuh

2008-01-22 12:39 UTC #4 lore raw

Quoted 9 lines · Matthew Garrett

Quoted 6 lines · Zhang Rui
I have no obvious objection on either of these two proposals. But one thing to mention is that
both of these two patches is written on the assumption that the brightness levels listed in _BCL method are in ascending order, while this is not stated in the ACPI spec.
Is this a problem?
The driver already makes that assumption, and it's implicit in the spec.

Actually, no, it is not implicit. The spec clearly states that the brightness values will be cycled through the ones given by that list. It is implicit that brightness up actions will cycle through the list in order, and brightness down will cycle through the list in reverse order.

If a vendor where to decide to place the most used brightness values in the beginning of the list, and the rest of it later, he would be allowed to do so. Nobody was weird enough to do it yet though, AFAIK.

Signature · 4 lines

  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Len Brown

reviewer

2008-01-24 21:45 UTC #5 lore raw

In reply to Matthew Garrett

Quoted 7 lines · Matthew Garrett

The sysfs backlight class provides no mechanism for querying the acceptable brightness for a backlight. The ACPI spec states that values are only valid if they are reported as available by the firmware. Since we can't provide that information to userspace, instead collapse the range to the number of actual values that can be set.
Signed-off-by Matthew Garrett <[email protected]>

I wish we did this in the first place.
But doing it now is an API change -- since with the old way 100 always meant 100% brightness, yes?

so my concern is that if we change what "10" means, somebody like akpm with an existing script gets grumpy.

-Len

Quoted 59 lines

---

drivers/acpi/video.c +13 -7

index 521645e..12b2adb 100644
@@ -296,18 +296,26 @@ static int acpi_video_device_set_state(struct acpi_video_device *device, int sta
 static int acpi_video_get_brightness(struct backlight_device *bd)
 {
 	unsigned long cur_level;
+	int i;
 	struct acpi_video_device *vd =
 		(struct acpi_video_device *)bl_get_data(bd);
 	acpi_video_device_lcd_get_level_current(vd, &cur_level);
-	return (int) cur_level;
+	for (i=2; i<vd->brightness->count; i++) {
+		if (vd->brightness->levels[i] == cur_level)
+			/* The first two entries are special - see page 575
+			   of the ACPI spec 3.0 */
+			return i-2;
+	}
+	return 0;
 }
 
 static int acpi_video_set_brightness(struct backlight_device *bd)
 {
-	int request_level = bd->props.brightness;
+	int request_level = bd->props.brightness+2;
 	struct acpi_video_device *vd =
 		(struct acpi_video_device *)bl_get_data(bd);
-	acpi_video_device_lcd_set_level(vd, request_level);
+	acpi_video_device_lcd_set_level(vd, 
+					vd->brightness->levels[request_level]);
 	return 0;
 }

@@ -656,7 +664,6 @@ static void acpi_video_device_find_cap(struct acpi_video_device *device)
 	kfree(obj);
 
 	if (device->cap._BCL && device->cap._BCM && device->cap._BQC && max_level > 0){
-		unsigned long tmp;
 		static int count = 0;
 		char *name;
 		name = kzalloc(MAX_NAME_LEN, GFP_KERNEL);

@@ -664,11 +671,10 @@ static void acpi_video_device_find_cap(struct acpi_video_device *device)
 			return;
 
 		sprintf(name, "acpi_video%d", count++);
-		acpi_video_device_lcd_get_level_current(device, &tmp);
 		device->backlight = backlight_device_register(name,
 			NULL, device, &acpi_backlight_ops);
-		device->backlight->props.max_brightness = max_level;
-		device->backlight->props.brightness = (int)tmp;
+		device->backlight->props.max_brightness = device->brightness->count-3;
+		device->backlight->props.brightness = acpi_video_get_brightness(device->backlight);
 		backlight_update_status(device->backlight);
 
 		kfree(name);

Matthew Garrett

author

2008-01-27 02:06 UTC #6 lore raw

Quoted 7 lines · Len Brown

Quoted 6 lines · Matthew Garrett
The sysfs backlight class provides no mechanism for querying the acceptable brightness for a backlight. The ACPI spec states that values are only valid if they are reported as available by the firmware. Since we can't provide that information to userspace, instead collapse the range to the number of actual values that can be set.

Quoted 5 lines

Quoted 1 line
Signed-off-by Matthew Garrett <[email protected]>
I wish we did this in the first place.
But doing it now is an API change -- since with the old way 100 always meant 100% brightness, yes?

Yes.

Quoted 2 lines

so my concern is that if we change what "10" means, somebody like akpm with an existing script gets grumpy.

I don't buy that argument. Assuming that the values will always result in an identical brightness is making incorrect assumptions about the API. For example, consider the case where a bug means that only half the hardware's available brightness levels are exposed. Fixing that bug would change the meaning of the numbers, but it's not an API change in any real way. Anyone using the backlight class should check the maximum_brightness field before deciding what range of values to use.

Signature · 1 line

Matthew Garrett | [email protected]

Andrew Morton

2008-01-27 06:01 UTC #7 lore raw

In reply to Len Brown

Quoted 8 lines · On Thu, 24 Jan 2008 16:44:48 -0500 Len Brown <[email protected]> wrote:

Quoted 6 lines · Matthew Garrett
The sysfs backlight class provides no mechanism for querying the acceptable brightness for a backlight. The ACPI spec states that values are only valid if they are reported as available by the firmware. Since we can't provide that information to userspace, instead collapse the range to the number of actual values that can be set.

Quoted 8 lines

Quoted 1 line
Signed-off-by Matthew Garrett <[email protected]>
I wish we did this in the first place.
But doing it now is an API change -- since with the old way 100 always meant 100% brightness, yes?
so my concern is that if we change what "10" means, somebody like akpm with an existing script gets grumpy.

It takes more than that to make me grumpy. I've been very grumpy lately.

- Create a new /sys node with a new name which has the new semantics.

- Deprecate the old /sys entry by emitting an angry printk when someone uses it.

- Wait 12 months

- Kill the old one.

Matthew Garrett

author

2008-01-28 01:25 UTC #8 lore raw

Quoted 1 line · Andrew Morton

- Create a new /sys node with a new name which has the new semantics.

The semantics are the same as they always have been - values between 0 and max_brightness are valid values. If you've made assumptions about what max_brightness is, then that's a bug in the userspace application rather than a change in the semantics of the interface.

Signature · 1 line

Matthew Garrett | [email protected]

Andrew Morton

2008-01-28 05:10 UTC #9 lore raw

Quoted 8 lines · On Mon, 28 Jan 2008 01:25:50 +0000 Matthew Garrett <[email protected]> wrote:

Quoted 1 line · Andrew Morton
- Create a new /sys node with a new name which has the new semantics.
The semantics are the same as they always have been - values between 0 and max_brightness are valid values. If you've made assumptions about what max_brightness is, then that's a bug in the userspace application rather than a change in the semantics of the interface.

WTH? My (utterly comedic chase-crap-around-the-tree) brightness script was:

(
0 sh -c "echo $1 > /proc/acpi/sony/brightness"
0 sh -c "echo $1 > /proc/acpi/sony/brightness_default"
0 sh -c "echo $1 > /sys/class/backlight/sony/brightness" 0 sh -c "echo $1 > /sys/class/backlight/thinkpad_screen/brightness" ) 2>/dev/null

And yes, I had an rc.local command which assumed that 7 (later 8) is max brightness.

You cannot seriously tell me that if we are to change this range from 0-8 up to 0-100 then this is not a backwards-incompatible change in semantics.

My /sys/class/backlight/ directory is presently empty. Rather than trying to find out why, I think I'll just collapse in laughter.

Guys, sort it out, please.

Matthew Garrett

author

2008-01-28 05:28 UTC #10 lore raw

Quoted 3 lines · Andrew Morton

You cannot seriously tell me that if we are to change this range from 0-8 up to 0-100 then this is not a backwards-incompatible change in semantics.

We're talking about changing 0-100 to 0-something sane, because the current driver is quite clearly broken. And yes, I'm perfectly happy to say that - the reason that max_brightness is exported is to allow userspace to determine what range of values is acceptable, and if userspace ignores that then things are going to break for it at some point.

Signature · 1 line

Matthew Garrett | [email protected]

Henrique de Moraes Holschuh

2008-01-28 15:51 UTC #11 lore raw

In reply to Andrew Morton

Quoted 5 lines · Andrew Morton

0 sh -c "echo $1 > /sys/class/backlight/thinkpad_screen/brightness" ) 2>/dev/null
And yes, I had an rc.local command which assumed that 7 (later 8) is max brightness.

You have to use 15 (or 16? I forget) on T61, X61, R61, X60(I think)... for
maximum brightness :-p And the interface to get maximum brightness has
always been to cat max_brightness > brightness, AFAIK.

Quoted 2 lines

My /sys/class/backlight/ directory is presently empty. Rather than trying to find out why, I think I'll just collapse in laughter.

Well, I can tell you why it is empty on the thinkpad-acpi side. It defaults to brightness_enable=0 on Lenovo 16-brightness level boxes, where the right thing to do is to control brightness using the X.org drivers or through the ACPI brightness driver.

But I have no idea why it is empty re. the ACPI video driver.

Quoted 1 line

Guys, sort it out, please.

I am reading this ML with eyes fully open. When a sensible (read: any) consensus is found, I will follow it.

Signature · 4 lines

  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Len Brown

reviewer

2008-02-02 03:46 UTC #12 lore raw

In reply to Andrew Morton

Quoted 24 lines · Andrew Morton

Quoted 8 lines · On Mon, 28 Jan 2008 01:25:50 +0000 Matthew Garrett <[email protected]> wrote:
Quoted 1 line · Andrew Morton
- Create a new /sys node with a new name which has the new semantics.
The semantics are the same as they always have been - values between 0 and max_brightness are valid values. If you've made assumptions about what max_brightness is, then that's a bug in the userspace application rather than a change in the semantics of the interface.
WTH? My (utterly comedic chase-crap-around-the-tree) brightness script was:
(
0 sh -c "echo $1 > /proc/acpi/sony/brightness"
0 sh -c "echo $1 > /proc/acpi/sony/brightness_default"
0 sh -c "echo $1 > /sys/class/backlight/sony/brightness" 0 sh -c "echo $1 > /sys/class/backlight/thinkpad_screen/brightness" ) 2>/dev/null
And yes, I had an rc.local command which assumed that 7 (later 8) is max brightness.

Quoted 8 lines

You cannot seriously tell me that if we are to change this range from 0-8 up to 0-100 then this is not a backwards-incompatible change in semantics.
My /sys/class/backlight/ directory is presently empty. Rather than trying to find out why, I think I'll just collapse in laughter.
Guys, sort it out, please.

I think that Matthew got it right.

The generic API is unchanged, brightness goes from 0 to max_brighness.

What he did was repair systems that use the acpi video driver (which none of akpm's examples above do) such that the generic API works for that case the same as it does with all other video drivers.

Andrew,
You might check if CONFIG_ACPI_VIDEO=m is set and you can load the "video" module. While the sony may be non-standard and not load, your thinkpad may work.

thanks,
-Len

Henrique de Moraes Holschuh

2008-02-02 11:30 UTC #13 lore raw

Quoted 2 lines · Len Brown

You might check if CONFIG_ACPI_VIDEO=m is set and you can load the "video" module. While the sony may be non-standard and not load, your thinkpad may work.

It will work except under new X.org, which disables BIOS backlight functionality in order to do it directly, at least on Intel GPUs (and probably also on ATI GPUs. nVidia is no-man's-land).

And I have been told that X.org can do 256 backlight levels on Intel hardware *including* switching the backlight off, instead of just the 16
levels Lenovo chose to export through ACPI. Looks like someone should
add a kernel framebuffer device for those Intels :)

We really need to solve the userspace mess, though.

Signature · 4 lines

  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Romano Giannetti

2008-02-06 10:09 UTC #14 lore raw

Quoted 3 lines · Henrique de Moraes Holschuh

Quoted 2 lines · Len Brown
You might check if CONFIG_ACPI_VIDEO=m is set and you can load the "video" module. While the sony may be non-standard and not load, your thinkpad may work.

[...]

Quoted 3 lines

We really need to solve the userspace mess, though.

I do not know if this is relevant... but for example, with a i915 hw here (toshiba laptop):

#cat max_brightness
100

#echo 50 > brightness ; cat actual_brightness
0
#echo 60 > brightness ; cat actual_brightness
0
#echo 70 > brightness ; cat actual_brightness
70
#echo 80 > brightness ; cat actual_brightness
0
#echo 90 > brightness ; cat actual_brightness
0
#echo 100 > brightness ; cat actual_brightness
100

#uname -a
Linux rukbat 2.6.24 #13 SMP PREEMPT Fri Feb 1 13:12:23 CET 2008 i686 GNU/Linux

which is at least strange (and probably broken). actual_brightness is real, when it says 0 backlight is off. You can imagine what happens using the backlight up/down control... flashing lights!

Romano

Signature · 12 lines

Sorry for the disclaimer --- ¡I cannot stop it!



--
La presente comunicación tiene carácter confidencial y es para el exclusivo uso del destinatario indicado en la misma. Si Ud. no es el destinatario indicado, le informamos que cualquier forma de distribución, reproducción o uso de esta comunicación y/o de la información contenida en la misma están estrictamente prohibidos por la ley. Si Ud. ha recibido esta comunicación por error, por favor, notifíquelo inmediatamente al remitente contestando a este mensaje y proceda a continuación a destruirlo. Gracias por su colaboración.

This communication contains confidential information. It is for the exclusive use of the intended addressee. If you are not the intended addressee, please note that any form of distribution, copying or use of this communication or the information in it is strictly prohibited by law. If you have received this communication in error, please immediately notify the sender by reply e-mail and destroy this message. Thank you for your cooperation. 
-
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

Len Brown

reviewer Applied

2008-02-02 03:44 UTC #15 lore raw

In reply to Matthew Garrett

Applied.

thanks,
-Len

Quoted 67 lines · Matthew Garrett

Signed-off-by Matthew Garrett <[email protected]>

---

drivers/acpi/video.c +13 -7

index 521645e..12b2adb 100644
@@ -296,18 +296,26 @@ static int acpi_video_device_set_state(struct acpi_video_device *device, int sta
 static int acpi_video_get_brightness(struct backlight_device *bd)
 {
 	unsigned long cur_level;
+	int i;
 	struct acpi_video_device *vd =
 		(struct acpi_video_device *)bl_get_data(bd);
 	acpi_video_device_lcd_get_level_current(vd, &cur_level);
-	return (int) cur_level;
+	for (i=2; i<vd->brightness->count; i++) {
+		if (vd->brightness->levels[i] == cur_level)
+			/* The first two entries are special - see page 575
+			   of the ACPI spec 3.0 */
+			return i-2;
+	}
+	return 0;
 }
 
 static int acpi_video_set_brightness(struct backlight_device *bd)
 {
-	int request_level = bd->props.brightness;
+	int request_level = bd->props.brightness+2;
 	struct acpi_video_device *vd =
 		(struct acpi_video_device *)bl_get_data(bd);
-	acpi_video_device_lcd_set_level(vd, request_level);
+	acpi_video_device_lcd_set_level(vd, 
+					vd->brightness->levels[request_level]);
 	return 0;
 }

@@ -656,7 +664,6 @@ static void acpi_video_device_find_cap(struct acpi_video_device *device)
 	kfree(obj);
 
 	if (device->cap._BCL && device->cap._BCM && device->cap._BQC && max_level > 0){
-		unsigned long tmp;
 		static int count = 0;
 		char *name;
 		name = kzalloc(MAX_NAME_LEN, GFP_KERNEL);

@@ -664,11 +671,10 @@ static void acpi_video_device_find_cap(struct acpi_video_device *device)
 			return;
 
 		sprintf(name, "acpi_video%d", count++);
-		acpi_video_device_lcd_get_level_current(device, &tmp);
 		device->backlight = backlight_device_register(name,
 			NULL, device, &acpi_backlight_ops);
-		device->backlight->props.max_brightness = max_level;
-		device->backlight->props.brightness = (int)tmp;
+		device->backlight->props.max_brightness = device->brightness->count-3;
+		device->backlight->props.brightness = acpi_video_get_brightness(device->backlight);
 		backlight_update_status(device->backlight);
 
 		kfree(name);