| Message ID | 1233545621.3715.42.camel@localhost.localdomain (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
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 -- 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
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 -- 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
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 > -- 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
> 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 -- 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
-- 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 -- 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
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 > -- 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
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.
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.
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;