From mboxrd@z Thu Jan 1 00:00:00 1970 From: Seth Forshee Subject: Re: [PATCH 3/5] acpi_video: Add workaround for broken Windows 8 backlight implementations Date: Thu, 4 Apr 2013 09:02:44 -0500 Message-ID: <20130404140244.GC22945@thinkpad-t410> References: <20130307193812.GD24233@thinkpad-t410> <1362685180-7768-1-git-send-email-seth.forshee@canonical.com> <1362685180-7768-3-git-send-email-seth.forshee@canonical.com> <515D6784.4010900@gmail.com> <20130404123506.GB22945@thinkpad-t410> <515D8447.7040806@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Received: from youngberry.canonical.com ([91.189.89.112]:43951 "EHLO youngberry.canonical.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1760655Ab3DDOCt (ORCPT ); Thu, 4 Apr 2013 10:02:49 -0400 Content-Disposition: inline In-Reply-To: <515D8447.7040806@gmail.com> Sender: linux-acpi-owner@vger.kernel.org List-Id: linux-acpi@vger.kernel.org To: Aaron Lu Cc: linux-acpi@vger.kernel.org, Matthew Garrett , Len Brown , "Rafael J. Wysocki" , Ben Jencks , joeyli 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 > >>> --- > >>> 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