[PATCH] acpi-video: Add disable_native_backlight quirk for Dell XPS15 L521X

[PATCH] acpi-video: Add disable_native_backlight quirk for Dell XPS15 L521X

[Hans de Goede]

The L521X variant of the Dell XPS15 has integrated nvidia graphics, and
backlight control does not work properly when using the native interfaces.

BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1163574
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/video.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index c72e79d2c5..032db45 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -522,6 +522,16 @@ static struct dmi_system_id video_dmi_table[] __initdata = {
 		DMI_MATCH(DMI_PRODUCT_NAME, "370R4E/370R4V/370R5E/3570RE/370R5V"),
 		},
 	},
+
+	{
+	 /* https://bugzilla.redhat.com/show_bug.cgi?id=1163574 */
+	 .callback = video_disable_native_backlight,
+	 .ident = "Dell XPS15 L521X",
+	 .matches = {
+		DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+		DMI_MATCH(DMI_PRODUCT_NAME, "XPS L521X"),
+		},
+	},
 	{}
 };
 
-- 
2.1.0

Re: [PATCH] acpi-video: Add disable_native_backlight quirk for Dell XPS15 L521X

[Rafael J. Wysocki]

On Monday, January 05, 2015 08:57:04 AM Hans de Goede wrote:
> The L521X variant of the Dell XPS15 has integrated nvidia graphics, and
> backlight control does not work properly when using the native interfaces.
> 
> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1163574
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Applied, thanks!

> ---
>  drivers/acpi/video.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
> index c72e79d2c5..032db45 100644
> --- a/drivers/acpi/video.c
> +++ b/drivers/acpi/video.c
> @@ -522,6 +522,16 @@ static struct dmi_system_id video_dmi_table[] __initdata = {
>  		DMI_MATCH(DMI_PRODUCT_NAME, "370R4E/370R4V/370R5E/3570RE/370R5V"),
>  		},
>  	},
> +
> +	{
> +	 /* https://bugzilla.redhat.com/show_bug.cgi?id=1163574 */
> +	 .callback = video_disable_native_backlight,
> +	 .ident = "Dell XPS15 L521X",
> +	 .matches = {
> +		DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +		DMI_MATCH(DMI_PRODUCT_NAME, "XPS L521X"),
> +		},
> +	},
>  	{}
>  };
>  
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH] acpi-video: Add disable_native_backlight quirk for Dell XPS15 L521X

[Matthew Garrett]

On Mon, Jan 05, 2015 at 08:57:04AM +0100, Hans de Goede wrote:
> The L521X variant of the Dell XPS15 has integrated nvidia graphics, and
> backlight control does not work properly when using the native interfaces.

I'm not convinced that the assessment of this bug is correct. Is the 
system using the nvidia graphics device? If so, why is userspace writing 
to the intel backlight device?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] acpi-video: Add disable_native_backlight quirk for Dell XPS15 L521X

[Hans de Goede]

Hi,

On 11-01-15 07:16, Matthew Garrett wrote:
> On Mon, Jan 05, 2015 at 08:57:04AM +0100, Hans de Goede wrote:
>> The L521X variant of the Dell XPS15 has integrated nvidia graphics, and
>> backlight control does not work properly when using the native interfaces.
>
> I'm not convinced that the assessment of this bug is correct. Is the
> system using the nvidia graphics device?

I do not know.

> If so, why is userspace writing to the intel backlight device?

Userspace is writing to the intel_backlight device because it is the
only one present under /sys/class/backlight, as we disable acpi-video
on win8 ready laptops by default now.

What this commit does is disable the disabling of acpi-video, and
after that userspace will see an acpi_video0 backlight under
/sys/class/backlight, use that and then things work, this has been
confirmed by the bug reporter.

The commit message may not be 100% accurate, although I believe this is
related to the laptop having nvidia graphics, but the code / fix is correct
regardless.

Regards,

Hans

Re: [PATCH] acpi-video: Add disable_native_backlight quirk for Dell XPS15 L521X

[Matthew Garrett]

On Sun, Jan 11, 2015 at 11:37:52AM +0100, Hans de Goede wrote:

> Userspace is writing to the intel_backlight device because it is the
> only one present under /sys/class/backlight, as we disable acpi-video
> on win8 ready laptops by default now.

Why is nouveau not providing a backlight device?

> The commit message may not be 100% accurate, although I believe this is
> related to the laptop having nvidia graphics, but the code / fix is correct
> regardless.

I don't think we have enough evidence to know whether this fix is 
correct.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] acpi-video: Add disable_native_backlight quirk for Dell XPS15 L521X

[Hans de Goede]

Hi,

On 12-01-15 03:44, Matthew Garrett wrote:
> On Sun, Jan 11, 2015 at 11:37:52AM +0100, Hans de Goede wrote:
>
>> Userspace is writing to the intel_backlight device because it is the
>> only one present under /sys/class/backlight, as we disable acpi-video
>> on win8 ready laptops by default now.
>
> Why is nouveau not providing a backlight device?

I do not know.

>> The commit message may not be 100% accurate, although I believe this is
>> related to the laptop having nvidia graphics, but the code / fix is correct
>> regardless.
>
> I don't think we have enough evidence to know whether this fix is
> correct.

This fix fixes a regression introduced from kernel 3.15 to 3.16, when we
switched video.use_native_brightness's default from 0 to 1, all this patch
does is restore the previous behaviour on this specific model as the new
behaviour is broken.

So this is in essence a model targeted "revert" fixing a regression and as
such most certainly is correct, given the clear no regressions policy the
kernel has.

Might it be good to dive deeper as to why the intel_backlight interface
does not work, yes certainly.

Regards,

Hans

Re: [PATCH] acpi-video: Add disable_native_backlight quirk for Dell XPS15 L521X

[Matthew Garrett]

On Mon, Jan 12, 2015 at 10:12:57AM +0100, Hans de Goede wrote:
> Hi,
> 
> On 12-01-15 03:44, Matthew Garrett wrote:
> >On Sun, Jan 11, 2015 at 11:37:52AM +0100, Hans de Goede wrote:
> >
> >>Userspace is writing to the intel_backlight device because it is the
> >>only one present under /sys/class/backlight, as we disable acpi-video
> >>on win8 ready laptops by default now.
> >
> >Why is nouveau not providing a backlight device?
> 
> I do not know.

We should figure that out. One possibility here is that it's correct for 
Intel to be disabling the ACPI device, but not correct for nouveau to 
be. We should possibly extend the interface to only unregister devices 
that correspond to the PCI device provided by the raw interface.

> >I don't think we have enough evidence to know whether this fix is
> >correct.
> 
> This fix fixes a regression introduced from kernel 3.15 to 3.16, when we
> switched video.use_native_brightness's default from 0 to 1, all this patch
> does is restore the previous behaviour on this specific model as the new
> behaviour is broken.

But it does so in a way that discourages us from ever fixing the actual 
problem, and leaves an unknown number of similar systems broken.

> So this is in essence a model targeted "revert" fixing a regression and as
> such most certainly is correct, given the clear no regressions policy the
> kernel has.

If we want to avoid regressions and a patch has broken a system without 
us having a good idea as to why, we should revert the entire patch in 
order to unbreak other systems.

-- 
Matthew Garrett | mjg59@srcf.ucam.org