Message-ID: <54A019BA.4080305@redhat.com>
Date:	Sun, 28 Dec 2014 15:54:50 +0100
From:	Hans de Goede <hdegoede@...hat.com>
To:	Jeremiah Mahler <jmmahler@...il.com>,
	Ilia Mirkin <imirkin@...m.mit.edu>,
	David Airlie <airlied@...ux.ie>,
	Ben Skeggs <bskeggs@...hat.com>,
	"Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
	"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Darren Hart <dvhart@...radead.org>
Subject: Re: [PATCH] nouveau: fix ambiguous backlight controls

Hi,

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 <jmmahler@...il.com> 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 <jmmahler@...il.com>
>>>>> ---
>>>>>   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.

Correct.

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

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists