[PATCH]: ACPI: Skip the first two elements in the _BCL package

[PATCH]: ACPI: Skip the first two elements in the _BCL package

[yakui_zhao]

Subject: ACPI: Skip the first two elements in the _BCL package
From: Zhao Yakui <yakui.zhao@intel.com> 
   According to the Spec the first two elements in the _BCL package won't be
regarded as the available brightness level. The first is the brightness when 
full power is connected to the box(It means that the AC adapter is plugged).
The second is the brightness level when the box is on battery.
    If the first two elements are still used while finding the next brightness
level, it will fall back to the lowest level when keeping on pressing 
hotkey. (On some boxes the brightness will be changed twice when hotkey is
pressed once. One is in the ACPI video driver. The other is changed by sys I/F.
In the ACPI video driver the first two elements will be used while changing
the brightness. But the first two elements is skipped while using sys I/F.
In such case there exists the inconsistency).
    So he first two elements had better be skipped while showing the available
brightness or finding the next brightness level.
     
http://bugzilla.kernel.org/show_bug.cgi?id=12450

Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
cc: Zhang Rui <rui.zhang@intel.com>
---
 drivers/acpi/video.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Index: linux-2.6/drivers/acpi/video.c
===================================================================
--- linux-2.6.orig/drivers/acpi/video.c	2009-01-23 14:47:25.000000000 +0800
+++ linux-2.6/drivers/acpi/video.c	2009-02-02 11:11:55.000000000 +0800
@@ -1020,7 +1020,7 @@
 	}
 
 	seq_printf(seq, "levels: ");
-	for (i = 0; i < dev->brightness->count; i++)
+	for (i = 2; i < dev->brightness->count; i++)
 		seq_printf(seq, " %d", dev->brightness->levels[i]);
 	seq_printf(seq, "\ncurrent: %d\n", dev->brightness->curr);
 
@@ -1059,7 +1059,7 @@
 		return -EFAULT;
 
 	/* validate through the list of available levels */
-	for (i = 0; i < dev->brightness->count; i++)
+	for (i = 2; i < dev->brightness->count; i++)
 		if (level == dev->brightness->levels[i]) {
 			if (ACPI_SUCCESS
 			    (acpi_video_device_lcd_set_level(dev, level)))
@@ -1712,7 +1712,7 @@
 	max = max_below = 0;
 	min = min_above = 255;
 	/* Find closest level to level_current */
-	for (i = 0; i < device->brightness->count; i++) {
+	for (i = 2; i < device->brightness->count; i++) {
 		l = device->brightness->levels[i];
 		if (abs(l - level_current) < abs(delta)) {
 			delta = l - level_current;
@@ -1722,7 +1722,7 @@
 	}
 	/* Ajust level_current to closest available level */
 	level_current += delta;
-	for (i = 0; i < device->brightness->count; i++) {
+	for (i = 2; i < device->brightness->count; i++) {
 		l = device->brightness->levels[i];
 		if (l < min)
 			min = l;

Re: [PATCH]: ACPI: Skip the first two elements in the _BCL package

[Len Brown]

applied.

btw. in the future...
it would be helpful to my tools if you do not include two copies of 
the patch in your e-mail, since the 1st one applies and the 2nd one fails,
causing the whole message to fail to apply...

thanks,
--
Len Brown, Intel Open Source Technology Center

Re: [PATCH]: ACPI: Skip the first two elements in the _BCL package

[Thomas Renninger]

On Monday 02 February 2009 04:33:41 yakui_zhao wrote:
> Subject: ACPI: Skip the first two elements in the _BCL package
> From: Zhao Yakui <yakui.zhao@intel.com>
>    According to the Spec the first two elements in the _BCL package won't
> be regarded as the available brightness level. The first is the brightness
> when full power is connected to the box(It means that the AC adapter is
> plugged). The second is the brightness level when the box is on battery.
>     If the first two elements are still used while finding the next
> brightness level, it will fall back to the lowest level when keeping on
> pressing hotkey. (On some boxes the brightness will be changed twice when
> hotkey is pressed once. One is in the ACPI video driver. The other is
> changed by sys I/F. In the ACPI video driver the first two elements will be
> used while changing the brightness. But the first two elements is skipped
> while using sys I/F. In such case there exists the inconsistency).
>     So he first two elements had better be skipped while showing the
> available brightness or finding the next brightness level.
I remember that Rui pointed me to a brightness level list, which included
a value in AC/battery values which was not in the rest of the list.
I expect simply ignoring battery/AC values is not right.
I posted a patch a while ago which did:
  - Go through the brightness values and extract from all (including
    AC and battery) unique values
  - Sort them
  - Create a with the data a new list.
Not sure whether this would have worked, but something is still
missing. Currently we do not use battery/AC values, but what if we want
do that, e.g. exporting them to userspace?
Then trying to set them will fail.
We either need to consider AC/battery levels or we need to
take the closest level if it's not included in the list. I
expect the first is correct, otherwise it would be stupid from
the vendors to provide an AC/battery level which cannot be set.

I expect we still miss a little piece of Windows compatibility here ...

   Thomas


> http://bugzilla.kernel.org/show_bug.cgi?id=12450
>
> Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
> cc: Zhang Rui <rui.zhang@intel.com>
> ---
>  drivers/acpi/video.c |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> Index: linux-2.6/drivers/acpi/video.c
> ===================================================================
> --- linux-2.6.orig/drivers/acpi/video.c	2009-01-23 14:47:25.000000000 +0800
> +++ linux-2.6/drivers/acpi/video.c	2009-02-02 11:11:55.000000000 +0800
> @@ -1020,7 +1020,7 @@
>  	}
>
>  	seq_printf(seq, "levels: ");
> -	for (i = 0; i < dev->brightness->count; i++)
> +	for (i = 2; i < dev->brightness->count; i++)
>  		seq_printf(seq, " %d", dev->brightness->levels[i]);
>  	seq_printf(seq, "\ncurrent: %d\n", dev->brightness->curr);
>
> @@ -1059,7 +1059,7 @@
>  		return -EFAULT;
>
>  	/* validate through the list of available levels */
> -	for (i = 0; i < dev->brightness->count; i++)
> +	for (i = 2; i < dev->brightness->count; i++)
>  		if (level == dev->brightness->levels[i]) {
>  			if (ACPI_SUCCESS
>  			    (acpi_video_device_lcd_set_level(dev, level)))
> @@ -1712,7 +1712,7 @@
>  	max = max_below = 0;
>  	min = min_above = 255;
>  	/* Find closest level to level_current */
> -	for (i = 0; i < device->brightness->count; i++) {
> +	for (i = 2; i < device->brightness->count; i++) {
>  		l = device->brightness->levels[i];
>  		if (abs(l - level_current) < abs(delta)) {
>  			delta = l - level_current;
> @@ -1722,7 +1722,7 @@
>  	}
>  	/* Ajust level_current to closest available level */
>  	level_current += delta;
> -	for (i = 0; i < device->brightness->count; i++) {
> +	for (i = 2; i < device->brightness->count; i++) {
>  		l = device->brightness->levels[i];
>  		if (l < min)
>  			min = l;
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH]: ACPI: Skip the first two elements in the _BCL package

[yakui_zhao]

On Tue, 2009-02-03 at 21:56 +0800, Thomas Renninger wrote:
> On Monday 02 February 2009 04:33:41 yakui_zhao wrote:
> > Subject: ACPI: Skip the first two elements in the _BCL package
> > From: Zhao Yakui <yakui.zhao@intel.com>
> >    According to the Spec the first two elements in the _BCL package won't
> > be regarded as the available brightness level. The first is the brightness
> > when full power is connected to the box(It means that the AC adapter is
> > plugged). The second is the brightness level when the box is on battery.
> >     If the first two elements are still used while finding the next
> > brightness level, it will fall back to the lowest level when keeping on
> > pressing hotkey. (On some boxes the brightness will be changed twice when
> > hotkey is pressed once. One is in the ACPI video driver. The other is
> > changed by sys I/F. In the ACPI video driver the first two elements will be
> > used while changing the brightness. But the first two elements is skipped
> > while using sys I/F. In such case there exists the inconsistency).
> >     So he first two elements had better be skipped while showing the
> > available brightness or finding the next brightness level.
> I remember that Rui pointed me to a brightness level list, which included
> a value in AC/battery values which was not in the rest of the list.
> I expect simply ignoring battery/AC values is not right.
Understand what you said. On some boxes the AC/battery level is not
included by the rest brightness level. 
>From the spec the rest are treated as the list of levels OSPM can cycle
through when user toggles(via keystroke) the brightness level of the
display. 
    If so, the first two elements should be excluded in the available
brightness levels for hotkey.
    And the first two elements are only used for that the power is
switched between AC and battery.  
    Right?
> I posted a patch a while ago which did:
>   - Go through the brightness values and extract from all (including
>     AC and battery) unique values
>   - Sort them
>   - Create a with the data a new list.
> Not sure whether this would have worked, but something is still
> missing. Currently we do not use battery/AC values, but what if we want
> do that, e.g. exporting them to userspace?
     In current driver the BIOS flag of the _DOD input argument is zero,
which means that the BIOS will automatically control the brightness of
level when the power is switched between AC and DC. 
     
     If we expect to change the brightness when switching power between
AC and DC, it is necessary to export them to user space. 
     Can we still use the brightness sys I/F to change the brightness
for AC/DC? The current sys I/F only exports the available brightness
level that can be changed via hotkey. Maybe we should use another I/F to
change the brightness for AC/DC.
    
         
> Then trying to set them will fail.
> We either need to consider AC/battery levels or we need to
> take the closest level if it's not included in the list. I
> expect the first is correct, otherwise it would be stupid from
> the vendors to provide an AC/battery level which cannot be set.
> 
> I expect we still miss a little piece of Windows compatibility here ...
> 
>    Thomas
> 
> 
> > http://bugzilla.kernel.org/show_bug.cgi?id=12450
> >
> > Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
> > cc: Zhang Rui <rui.zhang@intel.com>
> > ---
> >  drivers/acpi/video.c |    8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > Index: linux-2.6/drivers/acpi/video.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/acpi/video.c	2009-01-23 14:47:25.000000000 +0800
> > +++ linux-2.6/drivers/acpi/video.c	2009-02-02 11:11:55.000000000 +0800
> > @@ -1020,7 +1020,7 @@
> >  	}
> >
> >  	seq_printf(seq, "levels: ");
> > -	for (i = 0; i < dev->brightness->count; i++)
> > +	for (i = 2; i < dev->brightness->count; i++)
> >  		seq_printf(seq, " %d", dev->brightness->levels[i]);
> >  	seq_printf(seq, "\ncurrent: %d\n", dev->brightness->curr);
> >
> > @@ -1059,7 +1059,7 @@
> >  		return -EFAULT;
> >
> >  	/* validate through the list of available levels */
> > -	for (i = 0; i < dev->brightness->count; i++)
> > +	for (i = 2; i < dev->brightness->count; i++)
> >  		if (level == dev->brightness->levels[i]) {
> >  			if (ACPI_SUCCESS
> >  			    (acpi_video_device_lcd_set_level(dev, level)))
> > @@ -1712,7 +1712,7 @@
> >  	max = max_below = 0;
> >  	min = min_above = 255;
> >  	/* Find closest level to level_current */
> > -	for (i = 0; i < device->brightness->count; i++) {
> > +	for (i = 2; i < device->brightness->count; i++) {
> >  		l = device->brightness->levels[i];
> >  		if (abs(l - level_current) < abs(delta)) {
> >  			delta = l - level_current;
> > @@ -1722,7 +1722,7 @@
> >  	}
> >  	/* Ajust level_current to closest available level */
> >  	level_current += delta;
> > -	for (i = 0; i < device->brightness->count; i++) {
> > +	for (i = 2; i < device->brightness->count; i++) {
> >  		l = device->brightness->levels[i];
> >  		if (l < min)
> >  			min = l;
> >
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH]: ACPI: Skip the first two elements in the _BCL package

[Len Brown]

--
Len Brown, Intel Open Source Technology Center

On Wed, 4 Feb 2009, yakui_zhao wrote:

> On Tue, 2009-02-03 at 21:56 +0800, Thomas Renninger wrote:
> > On Monday 02 February 2009 04:33:41 yakui_zhao wrote:
> > > Subject: ACPI: Skip the first two elements in the _BCL package
> > > From: Zhao Yakui <yakui.zhao@intel.com>
> > >    According to the Spec the first two elements in the _BCL package won't
> > > be regarded as the available brightness level. The first is the brightness
> > > when full power is connected to the box(It means that the AC adapter is
> > > plugged). The second is the brightness level when the box is on battery.
> > >     If the first two elements are still used while finding the next
> > > brightness level, it will fall back to the lowest level when keeping on
> > > pressing hotkey. (On some boxes the brightness will be changed twice when
> > > hotkey is pressed once. One is in the ACPI video driver. The other is
> > > changed by sys I/F. In the ACPI video driver the first two elements will be
> > > used while changing the brightness. But the first two elements is skipped
> > > while using sys I/F. In such case there exists the inconsistency).
> > >     So he first two elements had better be skipped while showing the
> > > available brightness or finding the next brightness level.
> > I remember that Rui pointed me to a brightness level list, which included
> > a value in AC/battery values which was not in the rest of the list.
> > I expect simply ignoring battery/AC values is not right.

> Understand what you said. On some boxes the AC/battery level is not
> included by the rest brightness level. 
> From the spec the rest are treated as the list of levels OSPM can cycle
> through when user toggles(via keystroke) the brightness level of the
> display. 
>     If so, the first two elements should be excluded in the available
> brightness levels for hotkey.

maybe, maybe not.

Thomas,
Do we have an example?

I suspect that users will prever to be able to get to the ac/dc
brightness via hotkeys rather than having to generate an ac/dc
event to get to them...

>     And the first two elements are only used for that the power is
> switched between AC and battery.  
>     Right?
> > I posted a patch a while ago which did:
> >   - Go through the brightness values and extract from all (including
> >     AC and battery) unique values
> >   - Sort them
> >   - Create a with the data a new list.
> > Not sure whether this would have worked, but something is still
> > missing. Currently we do not use battery/AC values, but what if we want
> > do that, e.g. exporting them to userspace?

>      In current driver the BIOS flag of the _DOD input argument is zero,

you mean _DOS bit 2?

> which means that the BIOS will automatically control the brightness of
> level when the power is switched between AC and DC. 
>      
>      If we expect to change the brightness when switching power between
> AC and DC, it is necessary to export them to user space. 

per my previous e-mail, i would recommend that if we export them
to user-space, that we allow user-space to override them
and have the kernel simply consume them.  That way things
would work normall even if user-space is ignorant.

>      Can we still use the brightness sys I/F to change the brightness
> for AC/DC? The current sys I/F only exports the available brightness
> level that can be changed via hotkey. Maybe we should use another I/F to
> change the brightness for AC/DC.

yes, i think we'd need to extend the I/F.

But the real question is if we want to keep using _DOS=0
or if we want to try _DOS=4

If we stick with _DOS=0, then the user-space I/F thing is moot.
Howver, adding (potentially) missing ac/dc values to the toggle
list applies in both cases.

thanks,
-Len

Re: [PATCH]: ACPI: Skip the first two elements in the _BCL package

[yakui_zhao]

On Wed, 2009-02-04 at 12:25 +0800, Len Brown wrote:
> 
> --
> Len Brown, Intel Open Source Technology Center
> 
> On Wed, 4 Feb 2009, yakui_zhao wrote:
> 
> > On Tue, 2009-02-03 at 21:56 +0800, Thomas Renninger wrote:
> > > On Monday 02 February 2009 04:33:41 yakui_zhao wrote:
> > > > Subject: ACPI: Skip the first two elements in the _BCL package
> > > > From: Zhao Yakui <yakui.zhao@intel.com>
> > > >    According to the Spec the first two elements in the _BCL package won't
> > > > be regarded as the available brightness level. The first is the brightness
> > > > when full power is connected to the box(It means that the AC adapter is
> > > > plugged). The second is the brightness level when the box is on battery.
> > > >     If the first two elements are still used while finding the next
> > > > brightness level, it will fall back to the lowest level when keeping on
> > > > pressing hotkey. (On some boxes the brightness will be changed twice when
> > > > hotkey is pressed once. One is in the ACPI video driver. The other is
> > > > changed by sys I/F. In the ACPI video driver the first two elements will be
> > > > used while changing the brightness. But the first two elements is skipped
> > > > while using sys I/F. In such case there exists the inconsistency).
> > > >     So he first two elements had better be skipped while showing the
> > > > available brightness or finding the next brightness level.
> > > I remember that Rui pointed me to a brightness level list, which included
> > > a value in AC/battery values which was not in the rest of the list.
> > > I expect simply ignoring battery/AC values is not right.
> 
> > Understand what you said. On some boxes the AC/battery level is not
> > included by the rest brightness level. 
> > From the spec the rest are treated as the list of levels OSPM can cycle
> > through when user toggles(via keystroke) the brightness level of the
> > display. 
> >     If so, the first two elements should be excluded in the available
> > brightness levels for hotkey.
> 
> maybe, maybe not.
> 
> Thomas,
> Do we have an example?
Name (PBCL, Package (0x07)
                        {
                            0x50,
                            0x32,
                            0x14,
                            0x28,
                            0x3C,
                            0x50,
                            0x64
                        })
Method (_BCL, 0, NotSerialized)
 {
       Return (PBCL)
}
> the above info is found in the bug 11166
Of course there exist the same issue on the bug 12037/12450.

> 
> I suspect that users will prever to be able to get to the ac/dc
> brightness via hotkeys rather than having to generate an ac/dc
> event to get to them...
> 
> >     And the first two elements are only used for that the power is
> > switched between AC and battery.  
> >     Right?
> > > I posted a patch a while ago which did:
> > >   - Go through the brightness values and extract from all (including
> > >     AC and battery) unique values
> > >   - Sort them
> > >   - Create a with the data a new list.
> > > Not sure whether this would have worked, but something is still
> > > missing. Currently we do not use battery/AC values, but what if we want
> > > do that, e.g. exporting them to userspace?
> 
> >      In current driver the BIOS flag of the _DOD input argument is zero,
> 
> you mean _DOS bit 2?
Yes. 
> 
> > which means that the BIOS will automatically control the brightness of
> > level when the power is switched between AC and DC. 
> >      
> >      If we expect to change the brightness when switching power between
> > AC and DC, it is necessary to export them to user space. 
> 
> per my previous e-mail, i would recommend that if we export them
> to user-space, that we allow user-space to override them
> and have the kernel simply consume them.  That way things
> would work normall even if user-space is ignorant.
> 
> >      Can we still use the brightness sys I/F to change the brightness
> > for AC/DC? The current sys I/F only exports the available brightness
> > level that can be changed via hotkey. Maybe we should use another I/F to
> > change the brightness for AC/DC.
> 

> yes, i think we'd need to extend the I/F.
> 
It will be harmless that the brightness is changed even when the
_DOS=0(Of course it will be redundant). 
> But the real question is if we want to keep using _DOS=0
> or if we want to try _DOS=4
> If we stick with _DOS=0, then the user-space I/F thing is moot.
> Howver, adding (potentially) missing ac/dc values to the toggle
> list applies in both cases.

> 
> thanks,
> -Len
> 

Re: [PATCH]: ACPI: Skip the first two elements in the _BCL package

[Matthew Garrett]

On Tue, Feb 03, 2009 at 11:25:45PM -0500, Len Brown wrote:

> per my previous e-mail, i would recommend that if we export them
> to user-space, that we allow user-space to override them
> and have the kernel simply consume them.  That way things
> would work normall even if user-space is ignorant.

I'm not sure how this would be useful. The kernel doesn't perform the 
ac/dc brightness change - that's handled by either the firmware or 
userspace (depending on _DOS bit 2). We can't provide new values to the 
firmware.

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

Re: [PATCH]: ACPI: Skip the first two elements in the _BCL package

[Len Brown]

> I remember that Rui pointed me to a brightness level list, which included
> a value in AC/battery values which was not in the rest of the list.

if that is true, then I like your idea better.
we should make sure that the ac and dc values appear on the list.

> I expect simply ignoring battery/AC values is not right.
> I posted a patch a while ago which did:
>   - Go through the brightness values and extract from all (including
>     AC and battery) unique values
>   - Sort them
>   - Create a with the data a new list.
> Not sure whether this would have worked, but something is still
> missing. Currently we do not use battery/AC values, but what if we want
> do that, e.g. exporting them to userspace?

I should think that it would be prudent to expose the
ac and dc defaults to user-space -- allowing user-space
to over-ride them, and then on ac events doing the
brightness switch in the kernel.  That way it will work
even w/o teaching user-space new tricks.

thanks,
-Len

> > http://bugzilla.kernel.org/show_bug.cgi?id=12450

Re: [PATCH]: ACPI: Skip the first two elements in the _BCL package

[Matthew Garrett]

On Tue, Feb 03, 2009 at 11:09:26PM -0500, Len Brown wrote:

> I should think that it would be prudent to expose the
> ac and dc defaults to user-space -- allowing user-space
> to over-ride them, and then on ac events doing the
> brightness switch in the kernel.  That way it will work
> even w/o teaching user-space new tricks.

Woah - it's bad enough that we've got hotkey policy in kernel for ACPI 
(and nothing else). Please don't add more policy to the core code - we 
leave that up to userspace for a reason.

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