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@xxxxxxxxx> 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@xxxxxxxxx>
---
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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
