[RFC PATCH 3/5] ACPI video: support _BCL packages that don't export brightness levels when machine is on AC/Battery

[RFC PATCH 3/5] ACPI video: support _BCL packages that don't export brightness levels when machine is on AC/Battery

[Zhang Rui]

Subject: support _BCL packages that don't export brightness levels when machine is on AC/Battery
From: Zhang Rui <rui.zhang@intel.com>

Many buggy BIOSes don't export the brightness levels when machine
is on AC/Battery in the _BCL method.

Reformat the _BCL package for these laptops:
now the elements in device->brightness->levels[] are like:
levels[0]: brightness level when on AC power.
levels[1]: brightness level when on Battery power.
levels[2]: supported brightness level 1.
levels[3]: supported brightness level 2.
...
levels[n]: supported brightness level n-1.
levels[n + 1]: supported brightness level n.
So if there are n supported brightness levels on this laptop,
we will have n+2 entries in device->brightnes->levels[].

level[0] and level[1] are invalid on the laptops that don't
export the brightness levels on AC/Battery.
Fortunately, we never use these two values at all, even for the
valid ones.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/acpi/video.c |   37 +++++++++++++++++++++++++++++++------
 1 file changed, 31 insertions(+), 6 deletions(-)

Index: linux-2.6/drivers/acpi/video.c
===================================================================
--- linux-2.6.orig/drivers/acpi/video.c
+++ linux-2.6/drivers/acpi/video.c
@@ -168,10 +168,15 @@ struct acpi_video_device_cap {
 	u8 _DSS:1;		/*Device state set */
 };
 
+struct acpi_video_brightness_flags {
+	u8 _BCL_no_ac_battery_levels:1;	/* no AC/Battery levels in _BCL */
+};
+
 struct acpi_video_device_brightness {
 	int curr;
 	int count;
 	int *levels;
+	struct acpi_video_brightness_flags flags;
 };
 
 struct acpi_video_device {
@@ -682,7 +687,7 @@ static int
 acpi_video_init_brightness(struct acpi_video_device *device)
 {
 	union acpi_object *obj = NULL;
-	int i, max_level = 0, count = 0;
+	int i, max_level = 0, count = 0, level_ac_battery = 0;
 	union acpi_object *o;
 	struct acpi_video_device_brightness *br = NULL;
 
@@ -701,7 +706,7 @@ acpi_video_init_brightness(struct acpi_v
 		goto out;
 	}
 
-	br->levels = kmalloc(obj->package.count * sizeof *(br->levels),
+	br->levels = kmalloc((obj->package.count + 2) * sizeof *(br->levels),
 				GFP_KERNEL);
 	if (!br->levels)
 		goto out_free;
@@ -719,16 +724,36 @@ acpi_video_init_brightness(struct acpi_v
 		count++;
 	}
 
+	/*
+	 * some buggy BIOS don't export the levels
+	 * when machine is on AC/Battery in _BCL package.
+	 * In this case, the first two elements in _BCL packages
+	 * are also supported brightness levels that OS should take care of.
+	 */
+	for (i = 2; i < count; i++)
+		if (br->levels[i] == br->levels[0] ||
+		    br->levels[i] == br->levels[1])
+			level_ac_battery++;
+
+	if (level_ac_battery > 2) {
+		ACPI_ERROR((AE_INFO, "Two many duplicates in _BCL package\n"));
+		goto out_free_levels;
+	} else if (level_ac_battery < 2) {
+		level_ac_battery = 2 - level_ac_battery;
+		br->flags._BCL_no_ac_battery_levels = 1;
+		for (i = (count - 1 + level_ac_battery); i >= 2; i--)
+			br->levels[i] = br->levels[i - level_ac_battery];
+		count += level_ac_battery;
+	}
+
 	/* don't sort the first two brightness levels */
 	sort(&br->levels[2], count - 2, sizeof(br->levels[2]),
 		acpi_video_cmp_level, NULL);
 
-	if (count < 2)
-		goto out_free_levels;
-
 	br->count = count;
 	device->brightness = br;
-	ACPI_DEBUG_PRINT((ACPI_DB_INFO, "found %d brightness levels\n", count));
+	ACPI_DEBUG_PRINT((ACPI_DB_INFO,
+			  "found %d brightness levels\n", count - 2));
 	kfree(obj);
 	return max_level;
 

Re: [RFC PATCH 3/5] ACPI video: support _BCL packages that don't export brightness levels when machine is on AC/Battery

[Matthew Garrett]

On Tue, Mar 10, 2009 at 04:03:29PM +0800, Zhang Rui wrote:

> +	if (level_ac_battery > 2) {
> +		ACPI_ERROR((AE_INFO, "Two many duplicates in _BCL package\n"));
> +		goto out_free_levels;

Exiting here seems a little excessive? I'd just go with a warning and 
carry on. In future maybe we could strip duplicates. Also, s/two/too/.

>  	/* don't sort the first two brightness levels */
>  	sort(&br->levels[2], count - 2, sizeof(br->levels[2]),
>  		acpi_video_cmp_level, NULL);

I think the comment here needs to be clarified - it sounds like you'll 
ignore the first two in the package, even if they're actual values 
rather than the ac and battery ones.

Otherwise:

Acked-by: Matthew Garrett <mjg@redhat.com>

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [RFC PATCH 3/5] ACPI video: support _BCL packages that don't export brightness levels when machine is on AC/Battery

[Zhang Rui]

On Wed, 2009-03-11 at 01:06 +0800, Matthew Garrett wrote:
> On Tue, Mar 10, 2009 at 04:03:29PM +0800, Zhang Rui wrote:
> 
> > +	if (level_ac_battery > 2) {
> > +		ACPI_ERROR((AE_INFO, "Two many duplicates in _BCL package\n"));
> > +		goto out_free_levels;
> 
> Exiting here seems a little excessive? I'd just go with a warning and 
> carry on. In future maybe we could strip duplicates. Also, s/two/too/.
> 
Agree.

> >  	/* don't sort the first two brightness levels */
> >  	sort(&br->levels[2], count - 2, sizeof(br->levels[2]),
> >  		acpi_video_cmp_level, NULL);
> 
> I think the comment here needs to be clarified - it sounds like you'll 
> ignore the first two in the package, even if they're actual values 
> rather than the ac and battery ones.
> 
Agree.

Refreshed patch attached.

Subject: support _BCL packages that don't export brightness levels when machine is on AC/Battery
From: Zhang Rui <rui.zhang@intel.com>

Many buggy BIOSes don't export the brightness levels when machine
is on AC/Battery in the _BCL method.

Reformat the _BCL package for these laptops:
now the elements in device->brightness->levels[] are like:
levels[0]: brightness level when on AC power.
levels[1]: brightness level when on Battery power.
levels[2]: supported brightness level 1.
levels[3]: supported brightness level 2.
...
levels[n]: supported brightness level n-1.
levels[n + 1]: supported brightness level n.
So if there are n supported brightness levels on this laptop,
we will have n+2 entries in device->brightnes->levels[].

level[0] and level[1] are invalid on the laptops that don't
export the brightness levels on AC/Battery.
Fortunately, we never use these two values at all, even for the
valid ones.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
Acked-by: Matthew Garrett <mjg@redhat.com>
---
 drivers/acpi/video.c |   38 +++++++++++++++++++++++++++++++-------
 1 file changed, 31 insertions(+), 7 deletions(-)

Index: linux-2.6/drivers/acpi/video.c
===================================================================
--- linux-2.6.orig/drivers/acpi/video.c
+++ linux-2.6/drivers/acpi/video.c
@@ -168,10 +168,15 @@ struct acpi_video_device_cap {
 	u8 _DSS:1;		/*Device state set */
 };
 
+struct acpi_video_brightness_flags {
+	u8 _BCL_no_ac_battery_levels:1;	/* no AC/Battery levels in _BCL */
+};
+
 struct acpi_video_device_brightness {
 	int curr;
 	int count;
 	int *levels;
+	struct acpi_video_brightness_flags flags;
 };
 
 struct acpi_video_device {
@@ -682,7 +687,7 @@ static int
 acpi_video_init_brightness(struct acpi_video_device *device)
 {
 	union acpi_object *obj = NULL;
-	int i, max_level = 0, count = 0;
+	int i, max_level = 0, count = 0, level_ac_battery = 0;
 	union acpi_object *o;
 	struct acpi_video_device_brightness *br = NULL;
 
@@ -701,7 +706,7 @@ acpi_video_init_brightness(struct acpi_v
 		goto out;
 	}
 
-	br->levels = kmalloc(obj->package.count * sizeof *(br->levels),
+	br->levels = kmalloc((obj->package.count + 2) * sizeof *(br->levels),
 				GFP_KERNEL);
 	if (!br->levels)
 		goto out_free;
@@ -719,16 +724,35 @@ acpi_video_init_brightness(struct acpi_v
 		count++;
 	}
 
-	/* don't sort the first two brightness levels */
+	/*
+	 * some buggy BIOS don't export the levels
+	 * when machine is on AC/Battery in _BCL package.
+	 * In this case, the first two elements in _BCL packages
+	 * are also supported brightness levels that OS should take care of.
+	 */
+	for (i = 2; i < count; i++)
+		if (br->levels[i] == br->levels[0] ||
+		    br->levels[i] == br->levels[1])
+			level_ac_battery++;
+
+	if (level_ac_battery > 2) {
+		ACPI_ERROR((AE_INFO, "Two many duplicates in _BCL package\n"));
+	} else if (level_ac_battery < 2) {
+		level_ac_battery = 2 - level_ac_battery;
+		br->flags._BCL_no_ac_battery_levels = 1;
+		for (i = (count - 1 + level_ac_battery); i >= 2; i--)
+			br->levels[i] = br->levels[i - level_ac_battery];
+		count += level_ac_battery;
+	}
+
+	/* sort all the supported brightness levels */
 	sort(&br->levels[2], count - 2, sizeof(br->levels[2]),
 		acpi_video_cmp_level, NULL);
 
-	if (count < 2)
-		goto out_free_levels;
-
 	br->count = count;
 	device->brightness = br;
-	ACPI_DEBUG_PRINT((ACPI_DB_INFO, "found %d brightness levels\n", count));
+	ACPI_DEBUG_PRINT((ACPI_DB_INFO,
+			  "found %d brightness levels\n", count - 2));
 	kfree(obj);
 	return max_level;
 

Re: [RFC PATCH 3/5] ACPI video: support _BCL packages that don't export brightness levels when machine is on AC/Battery

[Matthew Garrett]

On Wed, Mar 11, 2009 at 09:09:26AM +0800, Zhang Rui wrote:

> +		ACPI_ERROR((AE_INFO, "Two many duplicates in _BCL package\n"));
                                      ^^^
Too, not Two :)

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [RFC PATCH 3/5] ACPI video: support _BCL packages that don't export brightness levels when machine is on AC/Battery

[Thomas Renninger]

On Tuesday 10 March 2009 09:03:29 Zhang Rui wrote:

> +	/*
> +	 * some buggy BIOS don't export the levels
> +	 * when machine is on AC/Battery in _BCL package.
> +	 * In this case, the first two elements in _BCL packages
> +	 * are also supported brightness levels that OS should take care of.
> +	 */
> +	for (i = 2; i < count; i++)
> +		if (br->levels[i] == br->levels[0] ||
> +		    br->levels[i] == br->levels[1])
> +			level_ac_battery++;
Hmm, I wonder whether this is what Len sees on one of his machine.
Do you remember when I added the patches to distinguish native vs
acpi brightness switching?
IIRC you missed some brightness levels with ACPI?
Wasn't this 6 vs 8, ACPI vs native?
Rui's patches should fix this then.
IIRC it was a panasonic.
Hmm both, panasonic and toshiba drivers seem to register for the
backlight interface unconditionally and miss the check whether the
ACPI video driver also might register for that one.
If above is true, I can add the check for them again.

    Thomas