Re: [PATCH 3/5] acpi_video: Add workaround for broken Windows 8 backlight implementations

[Seth Forshee <seth.forshee@canonical.com>]

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