| Message ID | 1362685180-7768-3-git-send-email-seth.forshee@canonical.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
On 03/08/2013 03:39 AM, Seth Forshee wrote: > Windows 8 requires that all backlights report 101 brightness levels. > When Lenovo updated the firmware for some machines for Windows 8 they > met this requirement my making _BCL return a larger set of values for > Windows 8 than for other OSes. However, only the values in the smaller > set actually change the brightness at all. The rest of the values are > silently discarded. > > As a workaround, change acpi_video to set all intermediate backlight > levels when setting the brightness. This isn't perfect, but it will mean > that most brightness changes done by common userspace utilities will hit > at least one valid brightness value. > > [1] http://msdn.microsoft.com/en-us/library/windows/hardware/jj128256.aspx > > Signed-off-by: Seth Forshee <seth.forshee@canonical.com> > --- > drivers/acpi/video.c | 51 ++++++++++++++++++++++++++++++++++++++++---------- > 1 file changed, 41 insertions(+), 10 deletions(-) > > diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c > index edfcd74..b83fbbd 100644 > --- a/drivers/acpi/video.c > +++ b/drivers/acpi/video.c > @@ -352,25 +352,56 @@ acpi_video_device_lcd_query_levels(struct acpi_video_device *device, > static int > acpi_video_device_lcd_set_state(struct acpi_video_device *device, int state) > { > - int level = device->brightness->levels[state]; > union acpi_object arg0 = { ACPI_TYPE_INTEGER }; > struct acpi_object_list args = { 1, &arg0 }; > + int curr_state, offset; > acpi_status status; > + int result = 0; > > - arg0.integer.value = level; > + curr_state = device->brightness->curr_state; > > - status = acpi_evaluate_object(device->dev->handle, "_BCM", > - &args, NULL); > - if (ACPI_FAILURE(status)) { > - ACPI_ERROR((AE_INFO, "Evaluating _BCM failed")); > - return -EIO; > + /* > + * Some Lenovo firmware has a broken backlight implementation > + * for Windows 8 where _BCL returns 101 backlight levels but > + * only 16 or so levels actually change the brightness at all. > + * As a workaround for these machines we set every intermediate > + * value between the old and new brightness levels whenever the > + * system has made the Windows 8 OSI call, hoping that at least > + * one of them will cause a change in brightness. > + */ > + if (acpi_osi_windows_version() == ACPI_OSI_WIN_8) { What do you think of testing br->count > 100 instead of OSI version? It looks like only win8 systems will try to claim so many brightness levels. Thanks, Aaron > + if (state == curr_state) > + offset = 0; > + else > + offset = state > curr_state ? 1 : -1; > + } else { > + offset = state - curr_state; > } > > - device->brightness->curr_state = state; > + do { > + curr_state += offset; > + arg0.integer.value = device->brightness->levels[curr_state]; > + > + status = acpi_evaluate_object(device->dev->handle, "_BCM", > + &args, NULL); > + if (ACPI_FAILURE(status)) { > + ACPI_ERROR((AE_INFO, "Evaluating _BCM failed")); > + > + /* > + * Change curr_state back to that of last > + * successful _BCM call > + */ > + curr_state -= offset; > + result = -EIO; > + break; > + } > + } while (curr_state != state); > + > + device->brightness->curr_state = curr_state; > if (device->backlight) > - device->backlight->props.brightness = state - 2; > + device->backlight->props.brightness = curr_state - 2; > > - return 0; > + return result; > } > > static int > -- 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 Thu, Apr 04, 2013 at 07:44:04PM +0800, Aaron Lu wrote: > On 03/08/2013 03:39 AM, Seth Forshee wrote: > > Windows 8 requires that all backlights report 101 brightness levels. > > When Lenovo updated the firmware for some machines for Windows 8 they > > met this requirement my making _BCL return a larger set of values for > > Windows 8 than for other OSes. However, only the values in the smaller > > set actually change the brightness at all. The rest of the values are > > silently discarded. > > > > As a workaround, change acpi_video to set all intermediate backlight > > levels when setting the brightness. This isn't perfect, but it will mean > > that most brightness changes done by common userspace utilities will hit > > at least one valid brightness value. > > > > [1] http://msdn.microsoft.com/en-us/library/windows/hardware/jj128256.aspx > > > > Signed-off-by: Seth Forshee <seth.forshee@canonical.com> > > --- > > drivers/acpi/video.c | 51 ++++++++++++++++++++++++++++++++++++++++---------- > > 1 file changed, 41 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c > > index edfcd74..b83fbbd 100644 > > --- a/drivers/acpi/video.c > > +++ b/drivers/acpi/video.c > > @@ -352,25 +352,56 @@ acpi_video_device_lcd_query_levels(struct acpi_video_device *device, > > static int > > acpi_video_device_lcd_set_state(struct acpi_video_device *device, int state) > > { > > - int level = device->brightness->levels[state]; > > union acpi_object arg0 = { ACPI_TYPE_INTEGER }; > > struct acpi_object_list args = { 1, &arg0 }; > > + int curr_state, offset; > > acpi_status status; > > + int result = 0; > > > > - arg0.integer.value = level; > > + curr_state = device->brightness->curr_state; > > > > - status = acpi_evaluate_object(device->dev->handle, "_BCM", > > - &args, NULL); > > - if (ACPI_FAILURE(status)) { > > - ACPI_ERROR((AE_INFO, "Evaluating _BCM failed")); > > - return -EIO; > > + /* > > + * Some Lenovo firmware has a broken backlight implementation > > + * for Windows 8 where _BCL returns 101 backlight levels but > > + * only 16 or so levels actually change the brightness at all. > > + * As a workaround for these machines we set every intermediate > > + * value between the old and new brightness levels whenever the > > + * system has made the Windows 8 OSI call, hoping that at least > > + * one of them will cause a change in brightness. > > + */ > > + if (acpi_osi_windows_version() == ACPI_OSI_WIN_8) { > > What do you think of testing br->count > 100 instead of OSI version? It > looks like only win8 systems will try to claim so many brightness levels. I agree that it would be roughly the same set of machines today. But if we assume Microsoft will keep the same requirement in the future then it begins to expand beyond Windows 8. If anything I'd prefer reducing the number of machines we apply this workaround to. Like say limiting it to Lenovo Win8 machines, if we can reasonably assume that Lenovo will be the only vendor with this ridiculous implementation. Seth -- 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 04/04/2013 08:35 PM, Seth Forshee wrote: > On Thu, Apr 04, 2013 at 07:44:04PM +0800, Aaron Lu wrote: >> On 03/08/2013 03:39 AM, Seth Forshee wrote: >>> Windows 8 requires that all backlights report 101 brightness levels. >>> When Lenovo updated the firmware for some machines for Windows 8 they >>> met this requirement my making _BCL return a larger set of values for >>> Windows 8 than for other OSes. However, only the values in the smaller >>> set actually change the brightness at all. The rest of the values are >>> silently discarded. >>> >>> As a workaround, change acpi_video to set all intermediate backlight >>> levels when setting the brightness. This isn't perfect, but it will mean >>> that most brightness changes done by common userspace utilities will hit >>> at least one valid brightness value. >>> >>> [1] http://msdn.microsoft.com/en-us/library/windows/hardware/jj128256.aspx >>> >>> Signed-off-by: Seth Forshee <seth.forshee@canonical.com> >>> --- >>> drivers/acpi/video.c | 51 ++++++++++++++++++++++++++++++++++++++++---------- >>> 1 file changed, 41 insertions(+), 10 deletions(-) >>> >>> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c >>> index edfcd74..b83fbbd 100644 >>> --- a/drivers/acpi/video.c >>> +++ b/drivers/acpi/video.c >>> @@ -352,25 +352,56 @@ acpi_video_device_lcd_query_levels(struct acpi_video_device *device, >>> static int >>> acpi_video_device_lcd_set_state(struct acpi_video_device *device, int state) >>> { >>> - int level = device->brightness->levels[state]; >>> union acpi_object arg0 = { ACPI_TYPE_INTEGER }; >>> struct acpi_object_list args = { 1, &arg0 }; >>> + int curr_state, offset; >>> acpi_status status; >>> + int result = 0; >>> >>> - arg0.integer.value = level; >>> + curr_state = device->brightness->curr_state; >>> >>> - status = acpi_evaluate_object(device->dev->handle, "_BCM", >>> - &args, NULL); >>> - if (ACPI_FAILURE(status)) { >>> - ACPI_ERROR((AE_INFO, "Evaluating _BCM failed")); >>> - return -EIO; >>> + /* >>> + * Some Lenovo firmware has a broken backlight implementation >>> + * for Windows 8 where _BCL returns 101 backlight levels but >>> + * only 16 or so levels actually change the brightness at all. >>> + * As a workaround for these machines we set every intermediate >>> + * value between the old and new brightness levels whenever the >>> + * system has made the Windows 8 OSI call, hoping that at least >>> + * one of them will cause a change in brightness. >>> + */ >>> + if (acpi_osi_windows_version() == ACPI_OSI_WIN_8) { >> >> What do you think of testing br->count > 100 instead of OSI version? It >> looks like only win8 systems will try to claim so many brightness levels. > > I agree that it would be roughly the same set of machines today. But if > we assume Microsoft will keep the same requirement in the future then it > begins to expand beyond Windows 8. Right, and the br->count > 100 test should also work, so it seems to be a better condition check than OSI version. > > If anything I'd prefer reducing the number of machines we apply this > workaround to. Like say limiting it to Lenovo Win8 machines, if we can > reasonably assume that Lenovo will be the only vendor with this > ridiculous implementation. This is probably not the case. I saw a Dell system also claims to have 100 levels in win8 mode: https://bugzilla.kernel.org/show_bug.cgi?id=55071 Thanks, Aaron -- 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 Thu, Apr 04, 2013 at 09:46:47PM +0800, Aaron Lu wrote: > On 04/04/2013 08:35 PM, Seth Forshee wrote: > > On Thu, Apr 04, 2013 at 07:44:04PM +0800, Aaron Lu wrote: > >> On 03/08/2013 03:39 AM, Seth Forshee wrote: > >>> Windows 8 requires that all backlights report 101 brightness levels. > >>> When Lenovo updated the firmware for some machines for Windows 8 they > >>> met this requirement my making _BCL return a larger set of values for > >>> Windows 8 than for other OSes. However, only the values in the smaller > >>> set actually change the brightness at all. The rest of the values are > >>> silently discarded. > >>> > >>> As a workaround, change acpi_video to set all intermediate backlight > >>> levels when setting the brightness. This isn't perfect, but it will mean > >>> that most brightness changes done by common userspace utilities will hit > >>> at least one valid brightness value. > >>> > >>> [1] http://msdn.microsoft.com/en-us/library/windows/hardware/jj128256.aspx > >>> > >>> Signed-off-by: Seth Forshee <seth.forshee@canonical.com> > >>> --- > >>> drivers/acpi/video.c | 51 ++++++++++++++++++++++++++++++++++++++++---------- > >>> 1 file changed, 41 insertions(+), 10 deletions(-) > >>> > >>> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c > >>> index edfcd74..b83fbbd 100644 > >>> --- a/drivers/acpi/video.c > >>> +++ b/drivers/acpi/video.c > >>> @@ -352,25 +352,56 @@ acpi_video_device_lcd_query_levels(struct acpi_video_device *device, > >>> static int > >>> acpi_video_device_lcd_set_state(struct acpi_video_device *device, int state) > >>> { > >>> - int level = device->brightness->levels[state]; > >>> union acpi_object arg0 = { ACPI_TYPE_INTEGER }; > >>> struct acpi_object_list args = { 1, &arg0 }; > >>> + int curr_state, offset; > >>> acpi_status status; > >>> + int result = 0; > >>> > >>> - arg0.integer.value = level; > >>> + curr_state = device->brightness->curr_state; > >>> > >>> - status = acpi_evaluate_object(device->dev->handle, "_BCM", > >>> - &args, NULL); > >>> - if (ACPI_FAILURE(status)) { > >>> - ACPI_ERROR((AE_INFO, "Evaluating _BCM failed")); > >>> - return -EIO; > >>> + /* > >>> + * Some Lenovo firmware has a broken backlight implementation > >>> + * for Windows 8 where _BCL returns 101 backlight levels but > >>> + * only 16 or so levels actually change the brightness at all. > >>> + * As a workaround for these machines we set every intermediate > >>> + * value between the old and new brightness levels whenever the > >>> + * system has made the Windows 8 OSI call, hoping that at least > >>> + * one of them will cause a change in brightness. > >>> + */ > >>> + if (acpi_osi_windows_version() == ACPI_OSI_WIN_8) { > >> > >> What do you think of testing br->count > 100 instead of OSI version? It > >> looks like only win8 systems will try to claim so many brightness levels. > > > > I agree that it would be roughly the same set of machines today. But if > > we assume Microsoft will keep the same requirement in the future then it > > begins to expand beyond Windows 8. > > Right, and the br->count > 100 test should also work, so it seems to be > a better condition check than OSI version. My take on this is that for Lenovo this is an issue of transitioning to Windows 8. As far as I can tell the affected machines were all sold with Windows 7 previously and updated for Windows 8, and the implementation we're seeing looks like it's just a lazy way to meet the 101 brightness levels requirement. If that's true then extending it past Windows 8 doesn't make sense. Unfortunately I haven't been able to get Lenovo to comment on it, so right now it's just a guess. > > If anything I'd prefer reducing the number of machines we apply this > > workaround to. Like say limiting it to Lenovo Win8 machines, if we can > > reasonably assume that Lenovo will be the only vendor with this > > ridiculous implementation. > > This is probably not the case. I saw a Dell system also claims to have > 100 levels in win8 mode: > https://bugzilla.kernel.org/show_bug.cgi?id=55071 For that bug it looks like even writing the !Win8 values doesn't change the brightness, so this workaround isn't going to help anyway. Seth -- 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 04/04/2013 10:02 PM, Seth Forshee wrote: > On Thu, Apr 04, 2013 at 09:46:47PM +0800, Aaron Lu wrote: >> On 04/04/2013 08:35 PM, Seth Forshee wrote: >>> On Thu, Apr 04, 2013 at 07:44:04PM +0800, Aaron Lu wrote: >>>> On 03/08/2013 03:39 AM, Seth Forshee wrote: >>>>> Windows 8 requires that all backlights report 101 brightness levels. >>>>> When Lenovo updated the firmware for some machines for Windows 8 they >>>>> met this requirement my making _BCL return a larger set of values for >>>>> Windows 8 than for other OSes. However, only the values in the smaller >>>>> set actually change the brightness at all. The rest of the values are >>>>> silently discarded. >>>>> >>>>> As a workaround, change acpi_video to set all intermediate backlight >>>>> levels when setting the brightness. This isn't perfect, but it will mean >>>>> that most brightness changes done by common userspace utilities will hit >>>>> at least one valid brightness value. >>>>> >>>>> [1] http://msdn.microsoft.com/en-us/library/windows/hardware/jj128256.aspx >>>>> >>>>> Signed-off-by: Seth Forshee <seth.forshee@canonical.com> >>>>> --- >>>>> drivers/acpi/video.c | 51 ++++++++++++++++++++++++++++++++++++++++---------- >>>>> 1 file changed, 41 insertions(+), 10 deletions(-) >>>>> >>>>> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c >>>>> index edfcd74..b83fbbd 100644 >>>>> --- a/drivers/acpi/video.c >>>>> +++ b/drivers/acpi/video.c >>>>> @@ -352,25 +352,56 @@ acpi_video_device_lcd_query_levels(struct acpi_video_device *device, >>>>> static int >>>>> acpi_video_device_lcd_set_state(struct acpi_video_device *device, int state) >>>>> { >>>>> - int level = device->brightness->levels[state]; >>>>> union acpi_object arg0 = { ACPI_TYPE_INTEGER }; >>>>> struct acpi_object_list args = { 1, &arg0 }; >>>>> + int curr_state, offset; >>>>> acpi_status status; >>>>> + int result = 0; >>>>> >>>>> - arg0.integer.value = level; >>>>> + curr_state = device->brightness->curr_state; >>>>> >>>>> - status = acpi_evaluate_object(device->dev->handle, "_BCM", >>>>> - &args, NULL); >>>>> - if (ACPI_FAILURE(status)) { >>>>> - ACPI_ERROR((AE_INFO, "Evaluating _BCM failed")); >>>>> - return -EIO; >>>>> + /* >>>>> + * Some Lenovo firmware has a broken backlight implementation >>>>> + * for Windows 8 where _BCL returns 101 backlight levels but >>>>> + * only 16 or so levels actually change the brightness at all. >>>>> + * As a workaround for these machines we set every intermediate >>>>> + * value between the old and new brightness levels whenever the >>>>> + * system has made the Windows 8 OSI call, hoping that at least >>>>> + * one of them will cause a change in brightness. >>>>> + */ >>>>> + if (acpi_osi_windows_version() == ACPI_OSI_WIN_8) { >>>> >>>> What do you think of testing br->count > 100 instead of OSI version? It >>>> looks like only win8 systems will try to claim so many brightness levels. >>> >>> I agree that it would be roughly the same set of machines today. But if >>> we assume Microsoft will keep the same requirement in the future then it >>> begins to expand beyond Windows 8. >> >> Right, and the br->count > 100 test should also work, so it seems to be >> a better condition check than OSI version. > > My take on this is that for Lenovo this is an issue of transitioning to > Windows 8. As far as I can tell the affected machines were all sold with > Windows 7 previously and updated for Windows 8, and the implementation > we're seeing looks like it's just a lazy way to meet the 101 brightness > levels requirement. If that's true then extending it past Windows 8 > doesn't make sense. Unfortunately I haven't been able to get Lenovo to > comment on it, so right now it's just a guess. Then perhaps use a DMI table for them? > >>> If anything I'd prefer reducing the number of machines we apply this >>> workaround to. Like say limiting it to Lenovo Win8 machines, if we can >>> reasonably assume that Lenovo will be the only vendor with this >>> ridiculous implementation. >> >> This is probably not the case. I saw a Dell system also claims to have >> 100 levels in win8 mode: >> https://bugzilla.kernel.org/show_bug.cgi?id=55071 > > For that bug it looks like even writing the !Win8 values doesn't change > the brightness, so this workaround isn't going to help anyway. Oh, I thought you mean 101 levels, but obviously you mean the ridiculous implementations. -Aaron -- 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
diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c index edfcd74..b83fbbd 100644 --- a/drivers/acpi/video.c +++ b/drivers/acpi/video.c @@ -352,25 +352,56 @@ acpi_video_device_lcd_query_levels(struct acpi_video_device *device, static int acpi_video_device_lcd_set_state(struct acpi_video_device *device, int state) { - int level = device->brightness->levels[state]; union acpi_object arg0 = { ACPI_TYPE_INTEGER }; struct acpi_object_list args = { 1, &arg0 }; + int curr_state, offset; acpi_status status; + int result = 0; - arg0.integer.value = level; + curr_state = device->brightness->curr_state; - status = acpi_evaluate_object(device->dev->handle, "_BCM", - &args, NULL); - if (ACPI_FAILURE(status)) { - ACPI_ERROR((AE_INFO, "Evaluating _BCM failed")); - return -EIO; + /* + * Some Lenovo firmware has a broken backlight implementation + * for Windows 8 where _BCL returns 101 backlight levels but + * only 16 or so levels actually change the brightness at all. + * As a workaround for these machines we set every intermediate + * value between the old and new brightness levels whenever the + * system has made the Windows 8 OSI call, hoping that at least + * one of them will cause a change in brightness. + */ + if (acpi_osi_windows_version() == ACPI_OSI_WIN_8) { + if (state == curr_state) + offset = 0; + else + offset = state > curr_state ? 1 : -1; + } else { + offset = state - curr_state; } - device->brightness->curr_state = state; + do { + curr_state += offset; + arg0.integer.value = device->brightness->levels[curr_state]; + + status = acpi_evaluate_object(device->dev->handle, "_BCM", + &args, NULL); + if (ACPI_FAILURE(status)) { + ACPI_ERROR((AE_INFO, "Evaluating _BCM failed")); + + /* + * Change curr_state back to that of last + * successful _BCM call + */ + curr_state -= offset; + result = -EIO; + break; + } + } while (curr_state != state); + + device->brightness->curr_state = curr_state; if (device->backlight) - device->backlight->props.brightness = state - 2; + device->backlight->props.brightness = curr_state - 2; - return 0; + return result; } static int
Windows 8 requires that all backlights report 101 brightness levels. When Lenovo updated the firmware for some machines for Windows 8 they met this requirement my making _BCL return a larger set of values for Windows 8 than for other OSes. However, only the values in the smaller set actually change the brightness at all. The rest of the values are silently discarded. As a workaround, change acpi_video to set all intermediate backlight levels when setting the brightness. This isn't perfect, but it will mean that most brightness changes done by common userspace utilities will hit at least one valid brightness value. [1] http://msdn.microsoft.com/en-us/library/windows/hardware/jj128256.aspx Signed-off-by: Seth Forshee <seth.forshee@canonical.com> --- drivers/acpi/video.c | 51 ++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 41 insertions(+), 10 deletions(-)