[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
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 = >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
[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
> >>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 = >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
>
[PATCH] nouveau: fix ambiguous backlight controls
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 = >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.
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 .
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.
But, things are not that easy because there also is
acpi_video_dmi_promote_vendor()
which is used by
[PATCH] nouveau: fix ambiguous backlight controls
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 = >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.
> + dev_info(dev->dev, "Standard ACPI backlight control
> supported, disabling local control.\n");
> + return 0;
> + }
> +
> list_for_each_entry(connector, >mode_config.connector_list,
> head) {
> if (connector->connector_type != DRM_MODE_CONNECTOR_LVDS &&
> connector->connector_type != DRM_MODE_CONNECTOR_eDP)
> --
> 2.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
[PATCH] nouveau: fix ambiguous backlight controls
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 = >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.
[...]
--
- Jeremiah Mahler
[PATCH] nouveau: fix ambiguous backlight controls
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 = >device;
struct drm_connector *connector;
+ if (acpi_video_backlight_support()) {
+ dev_info(dev->dev, "Standard ACPI backlight control supported,
disabling local control.\n");
+ return 0;
+ }
+
list_for_each_entry(connector, >mode_config.connector_list, head) {
if (connector->connector_type != DRM_MODE_CONNECTOR_LVDS &&
connector->connector_type != DRM_MODE_CONNECTOR_eDP)
--
2.1.4
