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 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