toshiba_acpi: fingers off backlight if video.ko is serving this functionality

Subject: [PATCH] toshiba_acpi: fingers off backlight if video.ko is serving this functionality
From: Andrey Borzenkov <[email protected]>

Signed-off-by Andrey Borzenkov <[email protected]>

---

This completes backlight series; toshiba_acpi was missing.

 drivers/acpi/toshiba_acpi.c |   19 ++++++++++---------
 1 files changed, 10 insertions(+), 9 deletions(-)

drivers/acpi/toshiba_acpi.c +10 -9

index a7db0c2..c140709 100644
@@ -780,19 +780,20 @@ static int __init toshiba_acpi_init(void)
 		}
 	}
 
-	toshiba_backlight_device = backlight_device_register("toshiba",
+	if (!acpi_video_backlight_support()) {
+		toshiba_backlight_device = backlight_device_register("toshiba",
 						&toshiba_acpi.p_dev->dev,
 						NULL,
 						&toshiba_backlight_data);
-        if (IS_ERR(toshiba_backlight_device)) {
-		ret = PTR_ERR(toshiba_backlight_device);
-
-		printk(KERN_ERR "Could not register toshiba backlight device\n");
-		toshiba_backlight_device = NULL;
-		toshiba_acpi_exit();
-		return ret;
+		if (IS_ERR(toshiba_backlight_device)) {
+			ret = PTR_ERR(toshiba_backlight_device);
+			printk(KERN_ERR "Could not register toshiba backlight device\n");
+			toshiba_backlight_device = NULL;
+			toshiba_acpi_exit();
+			return ret;
+		}
+		toshiba_backlight_device->props.max_brightness = HCI_LCD_BRIGHTNESS_LEVELS - 1;
 	}
-        toshiba_backlight_device->props.max_brightness = HCI_LCD_BRIGHTNESS_LEVELS - 1;
 
 	/* Register rfkill switch for Bluetooth */
 	if (hci_get_bt_present(&bt_present) == HCI_SUCCESS && bt_present) {

This patch does what it is supposed to do, but it isn't clear if that is a step forward.

I have satellite pro (original centrino about 4 years old)

where toshiba-acpi exports 8 brightness levels,
and acpi exports just 3 (min, 40%, max)

so when i apply this patch, i get fewer brightness levels. I prefer to have all 8 brightness levels.

-Len

Quoted 48 lines · Andrey Borzenkov

Subject: [PATCH] toshiba_acpi: fingers off backlight if video.ko is serving this functionality From: Andrey Borzenkov <[email protected]>

Signed-off-by Andrey Borzenkov <[email protected]>

---

This completes backlight series; toshiba_acpi was missing.

 drivers/acpi/toshiba_acpi.c |   19 ++++++++++---------
 1 files changed, 10 insertions(+), 9 deletions(-)

drivers/acpi/toshiba_acpi.c +10 -9

index a7db0c2..c140709 100644
@@ -780,19 +780,20 @@ static int __init toshiba_acpi_init(void)
 		}
 	}
 
-	toshiba_backlight_device = backlight_device_register("toshiba",
+	if (!acpi_video_backlight_support()) {
+		toshiba_backlight_device = backlight_device_register("toshiba",
 						&toshiba_acpi.p_dev->dev,
 						NULL,
 						&toshiba_backlight_data);
-        if (IS_ERR(toshiba_backlight_device)) {
-		ret = PTR_ERR(toshiba_backlight_device);
-
-		printk(KERN_ERR "Could not register toshiba backlight device\n");
-		toshiba_backlight_device = NULL;
-		toshiba_acpi_exit();
-		return ret;
+		if (IS_ERR(toshiba_backlight_device)) {
+			ret = PTR_ERR(toshiba_backlight_device);
+			printk(KERN_ERR "Could not register toshiba backlight device\n");
+			toshiba_backlight_device = NULL;
+			toshiba_acpi_exit();
+			return ret;
+		}
+		toshiba_backlight_device->props.max_brightness =
 HCI_LCD_BRIGHTNESS_LEVELS - 1; }
-        toshiba_backlight_device->props.max_brightness =
 HCI_LCD_BRIGHTNESS_LEVELS - 1;
 
 	/* Register rfkill switch for Bluetooth */

if (hci_get_bt_present(&bt_present) == HCI_SUCCESS && bt_present) {

Quoted 2 lines · Len Brown

so when i apply this patch, i get fewer brightness levels. I prefer to have all 8 brightness levels.

The patch from Andrey is correct.
Looks like all, the ordinary user needs, and probably the way it works on a plain Vista OS is there: off, battery, full level.
You can still use the boot param for your specific needs:
acpi_display_output=vendor

Quoted 9 lines · Thomas Renninger

Quoted 2 lines · Len Brown

so when i apply this patch, i get fewer brightness levels. I prefer to have all 8 brightness levels.

The patch from Andrey is correct.
Looks like all, the ordinary user needs, and probably the way it works on a plain Vista OS is there: off, battery, full level.
You can still use the boot param for your specific needs:
acpi_display_output=vendor

The Toshiba BIOSes I've looked at all implement the ACPI code on top of the legacy interface. Do you have examples where that's not the case? If not, I don't see the harm in exposing both.

- exposing two knobs for the *same* thing confuses user level tools; you never know which one is used and they compete behind your back

Quoted 2 lines · Andrey Borzenkov

- exposing two knobs for the *same* thing confuses user level tools; you never know which one is used and they compete behind your back

How do they compete?

As I said, the implementations appear to be implemented on top of the same underlying functionality.

The problem
with providing both vendor and ACPI functionality comes when both use different mechanisms for changing the backlight and so can get out of sync with each other. Are there any machines supported by toshiba_acpi where this is the case?

Quoted 6 lines · Matthew Garrett

Quoted 2 lines · Andrey Borzenkov

- exposing two knobs for the *same* thing confuses user level tools; you never know which one is used and they compete behind your back

How do they compete?

In my case user level program (kpowersave) decided to use video.ko for brightness control (or, may be, it used them both). Which gave me 2 levels instead of 8. And there is no way to control it, at least known to me.

Quoted 6 lines · Andrey Borzenkov

Quoted 5 lines · Matthew Garrett

Quoted 2 lines · Andrey Borzenkov

- exposing two knobs for the *same* thing confuses user level tools; you never know which one is used and they compete behind your back

Quoted 5 lines

Quoted 1 line

How do they compete?

In my case user level program (kpowersave) decided to use video.ko for brightness control (or, may be, it used them both). Which gave me 2 levels instead of 8. And there is no way to control it, at least known to me.

Right. But that doesn't mean they're competing, as such. If you set the brightness via toshiba_acpi

to a value that isn't supported via the
acpi driver, what value does the acpi backlight claim to be at?

Quoted 2 lines · Matthew Garrett

Right. But that doesn't mean they're competing, as such. If you set the brightness via toshiba_acpi

I probably have problems with expressing myself as non-native English speaker.

I am not interested in setting values via echoing into sysfs file. I am interested in my desktop brightness control working out of the box. And desktop driver control has no way to select, which of two sysfs files to use. Nor do I understand why I have to create this problem of selecting right driver when I already have possibility to avoid it.

If you think exposing both knobs is non-issue, why are all those patches for other vendor drivers included in the kernel in the first place?

Quoted 13 lines · Andrey Borzenkov

Quoted 2 lines · Matthew Garrett

Right. But that doesn't mean they're competing, as such. If you set the brightness via toshiba_acpi

I probably have problems with expressing myself as non-native English speaker.

I am not interested in setting values via echoing into sysfs file. I am interested in my desktop brightness control working out of the box. And desktop driver control has no way to select, which of two sysfs files to use. Nor do I understand why I have to create this problem of selecting right driver when I already have possibility to avoid it.

Quoted 2 lines

If you think exposing both knobs is non-issue, why are all those patches for other vendor drivers included in the kernel in the first place?

Because in some of those cases, the ACPI and vendor function are implemented in different ways that can then get out of sync with each other. As a result, you can get garbage information. If the values in your two backlight interfaces are always sane, then there's no inherent need to hide one of them.

The kernel exposes the available functionality
and userland then determines the policy used to choose one over the other.

Quoted 3 lines · Matthew Garrett

Quoted 2 lines

If you think exposing both knobs is non-issue, why are all those patches for other vendor drivers included in the kernel in the first place?

Quoted 5 lines

Because in some of those cases, the ACPI and vendor function are implemented in different ways that can then get out of sync with each other. As a result, you can get garbage information. If the values in your two backlight interfaces are always sane, then there's no inherent need to hide one of them.

THE VALUE IN MY TWO BACKLIGHT INTERFACES ARE NOT THE SAME. I said this many times already. What exactly is not clear in this sentence?

Quoted 10 lines · Andrey Borzenkov

Quoted 8 lines · Matthew Garrett

Quoted 2 lines

If you think exposing both knobs is non-issue, why are all those patches for other vendor drivers included in the kernel in the first place?

Because in some of those cases, the ACPI and vendor function are implemented in different ways that can then get out of sync with each other. As a result, you can get garbage information. If the values in your two backlight interfaces are always sane, then there's no inherent need to hide one of them.

Quoted 2 lines

THE VALUE IN MY TWO BACKLIGHT INTERFACES ARE NOT THE SAME. I said this many times already. What exactly is not clear in this sentence?

Where did I say that they were? You never answered the question I asked
- if you set the value in the toshiba specific backlight control to a value that isn't supported via the generic acpi one, what value does the generic acpi one claim to have?

Ok, so they can get unsynchronised. I agree with Thomas that it's desirable to remove one of them in that case. As you and Len have pointed out, the difficulty is in knowing which one to remove.

Quoted 3 lines · Matthew Garrett

Ok, so they can get unsynchronised. I agree with Thomas that it's desirable to remove one of them in that case. As you and Len have pointed out, the difficulty is in knowing which one to remove.

May I point out the obvious, and *very* annoying fact?

It is clear by now that if we want to solve all border conditions nicely, we will need centralized control of backlight interfaces to broker between platform-specific drivers and ACPI generic.

My idea is: separate them in two layers. Have the "backends" (ACPI generic and platform specific drivers) register parameters with a "frontend". The "frontend" exposes a *single* backlight interface. The backends expose NOTHING (or at most a deprecated interface if it won't break things).

Backlight interfaces can have their parameters updated at runtime, so we do just that if the frontend has to switch backends (i.e. due to module loading/unloading). Make sure the backend is informed when it is connected/disconnected from driving the backlight.

Use an intelligent setup to select which backend should be driving the backlight. e.g: the backends provide a priority information and a quality information (priority is low-medium-high. Quality is the number of levels, adjusted up or down if you know there is something special about it that should lower or rise the quality).

If there is only one backend loaded, select that. If there are more than one, select the highest priority one. When the priority of the backends is the same, select the one with the highest quality. When quality and priority are the same, select the generic ACPI.

Priority should always be medium, except when you know for sure that you need to do something special for an specific platform/box.

Any other ideas, comments?

I would not add such complexity for a problem which isn't a real problem.

IMO we should either:
1) Just do nothing and use video.ko even for Toshibas which only provide 3 brightness states.

2) DMI blacklist for Toshiba in general to use toshiba_laptop for brightness switching.

For the brightness level it's something else. A short documentation into the right forum/mailing list and everybody can google the acpi_backlight=vendor/video boot param in a second on his already running machine and is happy.

The question is, are these 3 brightness levels to be considered as broken.

IMO it is something 95% of all toshiba users won't care, the brightness level

I very much expect that newer Toshibas export more brightness levels via video.ko (does someone have a new one and can double check?) and at some time the Toshiba specific functions may even vanish. Therefore I would prefer to not do an exception here and go for 1(see above).

Signed-off-by Thomas Renninger <[email protected]>

---
 drivers/acpi/video.c        |   38 +++------------------
 drivers/acpi/video_detect.c |   74

++++++++++++++++++++++++++++++++++++++---- 2 files changed, 73 insertions(+), 39 deletions(-)

drivers/acpi/video.c +6 -31

index baa4419..239800b 100644
@@ -445,37 +445,6 @@ acpi_video_device_set_state(struct acpi_video_device
 *device, int state) }
 
 static int
-acpi_video_device_lcd_query_levels(struct acpi_video_device *device,
-				   union acpi_object **levels)
-{
-	int status;
-	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
-	union acpi_object *obj;
-
-
-	*levels = NULL;
-
-	status = acpi_evaluate_object(device->dev->handle, "_BCL", NULL,
 &buffer); -	if (!ACPI_SUCCESS(status))
-		return status;
-	obj = (union acpi_object *)buffer.pointer;
-	if (!obj || (obj->type != ACPI_TYPE_PACKAGE)) {
-		printk(KERN_ERR PREFIX "Invalid _BCL data\n");
-		status = -EFAULT;
-		goto err;
-	}
-
-	*levels = obj;
-
-	return 0;
-
-      err:
-	kfree(buffer.pointer);
-
-	return status;
-}
-
-static int
 acpi_video_device_lcd_set_level(struct acpi_video_device *device, int
 level) {
 	int status = AE_OK;

@@ -625,6 +594,10 @@ acpi_video_bus_DOS(struct acpi_video_bus *video, int
 bios_flag, int lcd_flag) return status;
 }
 
+/* from video_detect.c */
+int
+acpi_video_device_lcd_query_levels(acpi_handle handle,
+				   union acpi_object **levels);
 /*
  *  Arg:
  *  	device	: video output device (LCD, CRT, ..)

@@ -643,7 +616,8 @@ acpi_video_init_brightness(struct acpi_video_device
 *device) union acpi_object *o;
 	struct acpi_video_device_brightness *br = NULL;
 
-	if (!ACPI_SUCCESS(acpi_video_device_lcd_query_levels(device, &obj))) {
+	if (!ACPI_SUCCESS(acpi_video_device_lcd_query_levels(device->dev->
+							     handle, &obj))) {
 		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Could not query available "
 						"LCD brightness level\n"));
 		goto out;

drivers/acpi/video_detect.c +53 -3

index f022eb6..29e317c 100644
@@ -42,19 +42,62 @@ ACPI_MODULE_NAME("video");
 
 static long acpi_video_support;
 static bool acpi_video_caps_checked;
+static int brightness_levels;
+
+/* also used in video.c */
+int
+acpi_video_device_lcd_query_levels(acpi_handle handle,
+				   union acpi_object **levels)
+{
+	int status;
+	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+	union acpi_object *obj;
+
+	*levels = NULL;
+
+	status = acpi_evaluate_object(handle, "_BCL", NULL, &buffer);
+	if (!ACPI_SUCCESS(status))
+		return status;
+	obj = (union acpi_object *)buffer.pointer;
+	if (!obj || (obj->type != ACPI_TYPE_PACKAGE)) {
+		printk(KERN_ERR PREFIX "Invalid _BCL data\n");
+		status = -EFAULT;
+		goto err;
+	}
+
+	*levels = obj;
+
+	return 0;
+
+      err:
+	kfree(buffer.pointer);
+
+	return status;
+}
+EXPORT_SYMBOL_GPL(acpi_video_device_lcd_query_levels);
 
 static acpi_status
 acpi_backlight_cap_match(acpi_handle handle, u32 level, void *context,
-			  void **retyurn_value)
+			  void **return_value)
 {
 	long *cap = context;
 	acpi_handle h_dummy;
+	union acpi_object *obj = NULL;
+	int ret;
 
 	if (ACPI_SUCCESS(acpi_get_handle(handle, "_BCM", &h_dummy)) &&
 	    ACPI_SUCCESS(acpi_get_handle(handle, "_BCL", &h_dummy))) {
 		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Found generic backlight "
 				  "support\n"));
-		*cap |= ACPI_VIDEO_BACKLIGHT;
+
+		ret = acpi_video_device_lcd_query_levels(handle, &obj);
+		if (!ret)
+			brightness_levels = obj->package.count;
+		kfree(obj);
+
+		if (brightness_levels > 1)
+			*cap |= ACPI_VIDEO_BACKLIGHT;
+
 		/* We have backlight support, no need to scan further */
 		return AE_CTRL_TERMINATE;
 	}

@@ -178,7 +221,14 @@ long acpi_video_get_capabilities(acpi_handle
 graphics_handle) }
 EXPORT_SYMBOL(acpi_video_get_capabilities);
 
-/* Returns true if video.ko can do backlight switching */
+/*
+ * Returns:
+ * >  1 - found _BCL brightness levels -> generic video support
+ * == 1 - could not get brightness level through _BCL, but generic video
+ *        support enforced, due to dmi or boot param, should not happen
+ * == 0 - no generic video functions found or vendor specific
+ *        support enforced via dmi or boot param.
+ */
 int acpi_video_backlight_support(void)
 {
 	/*

@@ -192,16 +242,26 @@ int acpi_video_backlight_support(void)
 	if (acpi_video_support & ACPI_VIDEO_BACKLIGHT_FORCE_VENDOR)
 		return 0;

drivers/acpi/video_detect.c +4 -2

-	/* Then go the default way */
-	return acpi_video_support & ACPI_VIDEO_BACKLIGHT;
+	if (acpi_video_support & ACPI_VIDEO_BACKLIGHT) {
+ video:
+		if (brightness_levels < 2) {
+			printk(KERN_WARNING "Default Video backlight interface"