[lkml]   [2014]   [Dec]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH] nouveau: fix ambiguous backlight controls

On 28-12-14 15:30, Jeremiah Mahler wrote:
> Hans,
> On Sat, Dec 27, 2014 at 02:39:42PM +0100, Hans de Goede wrote:
>> Hi,
>> On 27-12-14 00:51, Jeremiah Mahler wrote:
>>> Ilia,
>>> On Fri, Dec 26, 2014 at 04:39:08PM -0500, Ilia Mirkin wrote:
>>>> On Fri, Dec 26, 2014 at 4:26 PM, Jeremiah Mahler <> wrote:
>>>>> If a display supports backlight control using the nouveau driver, and
>>>>> also supports standard ACPI backlight control, there will be two sets of
>>>>> controls.
>>>>> /sys/class/backlight/acpi_video0
>>>>> /sys/class/backlight/nv_backlight
>>>>> This creates ambiguity because these controls can be out of sync with
>>>>> each other. One could be at 100% while the other is at 0% and the
>>>>> actual display brightness depends on which one was used last. This also
>>>>> creates anomalies in Powertop which will show two values for brightness
>>>>> with potentially different values.
>>>>> Fix this ambiguity by having the nouveau driver only enable its
>>>>> backlight controls if the standard ACPI controls are not present.
>>>>> Signed-off-by: Jeremiah Mahler <>
>>>>> ---
>>>>> drivers/gpu/drm/nouveau/nouveau_backlight.c | 5 +++++
>>>>> 1 file changed, 5 insertions(+)
>>>>> diff --git a/drivers/gpu/drm/nouveau/nouveau_backlight.c b/drivers/gpu/drm/nouveau/nouveau_backlight.c
>>>>> index e566c5b..3a52bd4 100644
>>>>> --- a/drivers/gpu/drm/nouveau/nouveau_backlight.c
>>>>> +++ b/drivers/gpu/drm/nouveau/nouveau_backlight.c
>>>>> @@ -221,6 +221,11 @@ nouveau_backlight_init(struct drm_device *dev)
>>>>> struct nvif_device *device = &drm->device;
>>>>> struct drm_connector *connector;
>>>>> + if (acpi_video_backlight_support()) {
>>>> None of the other drivers have this. Is nouveau somehow different
>>>> than, say, radeon in this respect?
>>>> Unfortunately the backlight situation is pretty fubar'd... sometimes
>>>> the acpi controls don't work, sometimes the card controls don't work,
>>>> sometimes they both work but in different ways (and then everyone's
>>>> favourite -- neither works, and there's some third platform thing).
>>>> I'm pretty sure this code existed before, but got removed. See commit
>>>> bee564430feec1175ee64bcfd4913cacc519f817 and the previous commit
>>>> 5bead799d3f8 before that. The ping-pong is probably not the right way
>>>> to go.
>>> I was not aware of that change. But you are right, it took out what I
>>> was trying to put back in.
>>> Thanks for the helpful information. I will have to rethink this fix.
>> So first of all NAK to the original fix, but I think that much was
>> already clear.
>> Let me explain how this currently works, most laptops have up to 3
>> backlight control interfaces (all talking to the same single backlight):
>> acpi_video: a standardized acpi interface for backlight control, broken on most
>> win8 ready laptops.
>> vendor: e.g. asus_wmi, dell_laptop, etc. typically not much better on
>> win8 ready laptops.
>> native: e.g. intel_backlight, nv_backlight, usually your best bet on win8
>> laptops, but not so much on older models.
>> Before windows8 only 2 of these 3 get registered / exported to userspace,
>> either you've:
>> acpi_video + native:
>> or:
>> vendor + native:
>> Since most vendor drivers contain:
>> if (acpi_video_backlight_support())
>> return 0;
>> And userspace backlight control code knows the prefer the firmware interfaces
>> over the native one and to simply ignore the native interface, unless there
>> is no firmware interface, so having 2 interfaces present in sysfs is not
>> really a problem as userspace knows how to deal with this.
>> So along came Windows 8, breaking most acpi_video implementations. This got
>> fixed by a new module parameter to the acpi_video driver called use_native_backlight,
>> which now a days defaults to 1. When this parameter is true *and* the BIOS is
>> a win8 ready bios, then acpi_video will not register a backlight interface itself,
>> and acpi_video_backlight_support() will still return 1, causing the vendor interfaces
>> to not register. Leaving only the native interface.
> So acpi_video_backlight_support() will return true for win8 even when ACPI
> isn't actually supported.

Well it is supported (the bios has an acpi_video backlight interface), but
drivers/acpi/video.c choses not to register the interface. Note that video-detect.c
knows nothing about that. As said this is not pretty.

> Could this have been fixed by updating
> acpi_video_backlight_support() function?

In retrospect it would probably have been best to do something akin to adding
a acpi_video_get_backlight_type function when the video.use_native_backlight
kernel option was first introduced.

>> Your proposed patch will break things on win8 laptops using nv_backlight, since in the
>> use_native_backlight case it will cause nv_backlight to not register resulting in
>> not having any backlight interface at all.
>> I will happily admit that the combination of acpi_backlight=[video|vendor]
>> + video.use_native_backlight=[0|1] which has evolved over time is not the prettiest
>> solution. IMHO if you want to clean things up, and ensure only one interface gets
>> registered at a time, the solution would be to change acpi_backlight to also take
>> a native option, so that on the kernel commandline we end up with only:
>> acpi_backlight=[video|vendor|native] and move the use_native_backlight handling
>> from drivers/acpi/video.c to drivers/acpi/video_detect.c .
> It is not ideal but I can see why it was done. It is much better to have
> at least one working interface even if this means there are two in some
> cases.
> My complaint of two which can be out of sync with each other is a non-issue
> in most cases.
>> Code wise this would mean replacing acpi_video_backlight_support() with a function
>> called acpi_video_get_backlight_type which returns an enum which can be:
>> acpi_video_backlight_acpi_video,
>> acpi_video_backlight_vendor,
>> acpi_video_backlight_native,
>> And fix all callers to use that.
> So all the detection would be done in drivers/acpi/video_detect.c and
> then acpi_video_backlight_type() could be called to determine the type.


>> But, things are not that easy because there also is acpi_video_dmi_promote_vendor()
>> which is used by vendor drivers (mostly found under drivers/platform/x86) to tell
>> video_detect.c that the vendor driver knows that on this particular model laptop
>> it is better to use the vendor driver then acpi_video, and in some cases
>> this is used in combination with not actually registering the vendor backlight interface
>> to get the same end result as the new "native" option would give, but then on laptops
>> which need this despite not being win8 ready (and thus not automatically defaulting
>> to native).
>> So you would need to replace this to with a acpi_video_set_dmi_backlight_type, which
>> should change the return of acpi_video_get_backlight_type, but only if not overridden
>> from the kernel commandline, as the commandline takes presedence over the dmi
>> quirks which are tracked in the various vendor drivers.
> Ugh, this is a messy situation.

This is sort of this way by design, so that we can keep the quirks in per vendor
drivers, rather then having one gargantuan quirk table in video-detect.c

>> A cleanup to all this would certainly be welcome, but as outlined above it is not
>> trivial. I do not have time to actively work on this myself, but I will happily
>> review any patches you come up with for this.
>> Regards,
>> Hans
> Thanks for the very detailed description :)

You're welcome.



 \ /
  Last update: 2014-12-28 16:01    [W:0.042 / U:0.192 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site