Date:	Sun, 28 Dec 2014 06:30:16 -0800
From:	Jeremiah Mahler <jmmahler@...il.com>
To:	Hans de Goede <hdegoede@...hat.com>
Cc:	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

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.  Could this have been fixed by updating
acpi_video_backlight_support() function?

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

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

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