[PATCH] acpi-video: Add use native backlight quirk for the ThinkPad W530

[PATCH] acpi-video: Add use native backlight quirk for the ThinkPad W530

[Hans de Goede]

Like all of the other *30 ThinkPad models, the W530 has a broken acpi-video
backlight control. Note in order for this to actually fix things on the
ThinkPad W530 the commit titled:
"nouveau: Don't check acpi_video_backlight_support() before registering backlight"
is also needed.

https://bugzilla.redhat.com/show_bug.cgi?id=1093171

Cc: stable@vger.kernel.org
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/video.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index 34198b2..6a2099d 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -520,6 +520,14 @@ static struct dmi_system_id video_dmi_table[] __initdata = {
 		},
 	},
 	{
+	 .callback = video_set_use_native_backlight,
+	 .ident = "ThinkPad W530",
+	 .matches = {
+		DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+		DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad W530"),
+		},
+	},
+	{
 	.callback = video_set_use_native_backlight,
 	.ident = "ThinkPad X1 Carbon",
 	.matches = {
-- 
1.9.0

Re: [PATCH] acpi-video: Add use native backlight quirk for the ThinkPad W530

[Aaron Lu]

On 05/12/2014 03:57 PM, Hans de Goede wrote:
> Like all of the other *30 ThinkPad models, the W530 has a broken acpi-video
> backlight control. Note in order for this to actually fix things on the
> ThinkPad W530 the commit titled:
> "nouveau: Don't check acpi_video_backlight_support() before registering backlight"
> is also needed.

Note that the backlight_device_registered(raw) may return false as when
acpi_video_verify_backlight_support is called, the nouveau driver may
not run yet.

Previously, we don't know anything about how laptops with nvidia graphics
card alone control backlight in Win8, so the existing solution doesn't
consider this case. If nvidia graphics system also should favour native
backlight control interface in Win8, the current solution needs
modifications.

Thanks,
Aaron

> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1093171
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/acpi/video.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
> index 34198b2..6a2099d 100644
> --- a/drivers/acpi/video.c
> +++ b/drivers/acpi/video.c
> @@ -520,6 +520,14 @@ static struct dmi_system_id video_dmi_table[] __initdata = {
>  		},
>  	},
>  	{
> +	 .callback = video_set_use_native_backlight,
> +	 .ident = "ThinkPad W530",
> +	 .matches = {
> +		DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> +		DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad W530"),
> +		},
> +	},
> +	{
>  	.callback = video_set_use_native_backlight,
>  	.ident = "ThinkPad X1 Carbon",
>  	.matches = {
> 

Re: [PATCH] acpi-video: Add use native backlight quirk for the ThinkPad W530

[Hans de Goede]

Hi,

On 05/12/2014 10:16 AM, Aaron Lu wrote:
> On 05/12/2014 03:57 PM, Hans de Goede wrote:
>> Like all of the other *30 ThinkPad models, the W530 has a broken acpi-video
>> backlight control. Note in order for this to actually fix things on the
>> ThinkPad W530 the commit titled:
>> "nouveau: Don't check acpi_video_backlight_support() before registering backlight"
>> is also needed.
> 
> Note that the backlight_device_registered(raw) may return false as when
> acpi_video_verify_backlight_support is called, the nouveau driver may
> not run yet.
> 
> Previously, we don't know anything about how laptops with nvidia graphics
> card alone control backlight in Win8, so the existing solution doesn't
> consider this case. If nvidia graphics system also should favour native
> backlight control interface in Win8, the current solution needs
> modifications.

Hmm, how is this dealt with in the case of the intel gfx driver ?

Regards,

Hans



> 
> Thanks,
> Aaron
> 
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=1093171
>>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/acpi/video.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
>> index 34198b2..6a2099d 100644
>> --- a/drivers/acpi/video.c
>> +++ b/drivers/acpi/video.c
>> @@ -520,6 +520,14 @@ static struct dmi_system_id video_dmi_table[] __initdata = {
>>  		},
>>  	},
>>  	{
>> +	 .callback = video_set_use_native_backlight,
>> +	 .ident = "ThinkPad W530",
>> +	 .matches = {
>> +		DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
>> +		DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad W530"),
>> +		},
>> +	},
>> +	{
>>  	.callback = video_set_use_native_backlight,
>>  	.ident = "ThinkPad X1 Carbon",
>>  	.matches = {
>>
> 

Re: [PATCH] acpi-video: Add use native backlight quirk for the ThinkPad W530

[Hans de Goede]

Hi,

On 05/12/2014 01:02 PM, Hans de Goede wrote:
> Hi,
> 
> On 05/12/2014 10:16 AM, Aaron Lu wrote:
>> On 05/12/2014 03:57 PM, Hans de Goede wrote:
>>> Like all of the other *30 ThinkPad models, the W530 has a broken acpi-video
>>> backlight control. Note in order for this to actually fix things on the
>>> ThinkPad W530 the commit titled:
>>> "nouveau: Don't check acpi_video_backlight_support() before registering backlight"
>>> is also needed.
>>
>> Note that the backlight_device_registered(raw) may return false as when
>> acpi_video_verify_backlight_support is called, the nouveau driver may
>> not run yet.
>>
>> Previously, we don't know anything about how laptops with nvidia graphics
>> card alone control backlight in Win8, so the existing solution doesn't
>> consider this case. If nvidia graphics system also should favour native
>> backlight control interface in Win8, the current solution needs
>> modifications.
> 
> Hmm, how is this dealt with in the case of the intel gfx driver ?

Ok, I've figured out now how this is dealt with in the case of the intel gfx
drivers. That looks like something which will likely be hard to do for
nouveau, since it relies on some intel gfx specific ACPI calls being there,
which nouveau does not have. So this would require doing something like
duplicating the nouveau pci-ids or some such, which would be far from ideal.

Still this bug: https://bugzilla.redhat.com/show_bug.cgi?id=1093171

Shows that we need *working* video.use_native_brightness=1 support for
non intel too. Which the current loading order issues caused by the
backlight_device_registered(raw) breaks.

So maybe we should simply drop the backlight_device_registered(raw) check?
I don't think we really need it any win8 supporting laptop will have
intel, nv or ati gfx, and all 3 of them have native backlight support,
which if our experiences so far is any indication we will likely want
to use instead of acpi_video.

Regards,

Hans






> 
> Regards,
> 
> Hans
> 
> 
> 
>>
>> Thanks,
>> Aaron
>>
>>>
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1093171
>>>
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>>  drivers/acpi/video.c | 8 ++++++++
>>>  1 file changed, 8 insertions(+)
>>>
>>> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
>>> index 34198b2..6a2099d 100644
>>> --- a/drivers/acpi/video.c
>>> +++ b/drivers/acpi/video.c
>>> @@ -520,6 +520,14 @@ static struct dmi_system_id video_dmi_table[] __initdata = {
>>>  		},
>>>  	},
>>>  	{
>>> +	 .callback = video_set_use_native_backlight,
>>> +	 .ident = "ThinkPad W530",
>>> +	 .matches = {
>>> +		DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
>>> +		DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad W530"),
>>> +		},
>>> +	},
>>> +	{
>>>  	.callback = video_set_use_native_backlight,
>>>  	.ident = "ThinkPad X1 Carbon",
>>>  	.matches = {
>>>
>>

Re: [PATCH] acpi-video: Add use native backlight quirk for the ThinkPad W530

[Rafael J. Wysocki]

On Wednesday, May 14, 2014 01:09:33 PM Hans de Goede wrote:
> Hi,
> 
> On 05/12/2014 01:02 PM, Hans de Goede wrote:
> > Hi,
> > 
> > On 05/12/2014 10:16 AM, Aaron Lu wrote:
> >> On 05/12/2014 03:57 PM, Hans de Goede wrote:
> >>> Like all of the other *30 ThinkPad models, the W530 has a broken acpi-video
> >>> backlight control. Note in order for this to actually fix things on the
> >>> ThinkPad W530 the commit titled:
> >>> "nouveau: Don't check acpi_video_backlight_support() before registering backlight"
> >>> is also needed.
> >>
> >> Note that the backlight_device_registered(raw) may return false as when
> >> acpi_video_verify_backlight_support is called, the nouveau driver may
> >> not run yet.
> >>
> >> Previously, we don't know anything about how laptops with nvidia graphics
> >> card alone control backlight in Win8, so the existing solution doesn't
> >> consider this case. If nvidia graphics system also should favour native
> >> backlight control interface in Win8, the current solution needs
> >> modifications.
> > 
> > Hmm, how is this dealt with in the case of the intel gfx driver ?
> 
> Ok, I've figured out now how this is dealt with in the case of the intel gfx
> drivers. That looks like something which will likely be hard to do for
> nouveau, since it relies on some intel gfx specific ACPI calls being there,
> which nouveau does not have. So this would require doing something like
> duplicating the nouveau pci-ids or some such, which would be far from ideal.
> 
> Still this bug: https://bugzilla.redhat.com/show_bug.cgi?id=1093171
> 
> Shows that we need *working* video.use_native_brightness=1 support for
> non intel too. Which the current loading order issues caused by the
> backlight_device_registered(raw) breaks.
> 
> So maybe we should simply drop the backlight_device_registered(raw) check?
> I don't think we really need it any win8 supporting laptop will have
> intel, nv or ati gfx, and all 3 of them have native backlight support,
> which if our experiences so far is any indication we will likely want
> to use instead of acpi_video.

We can try that, but if it turns out to break things for someone, we'll need
a different approach.

Thanks!

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

Re: [PATCH] acpi-video: Add use native backlight quirk for the ThinkPad W530

[Hans de Goede]

Hi,

On 05/14/2014 02:07 PM, Rafael J. Wysocki wrote:
> On Wednesday, May 14, 2014 01:09:33 PM Hans de Goede wrote:
>> Hi,
>>
>> On 05/12/2014 01:02 PM, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 05/12/2014 10:16 AM, Aaron Lu wrote:
>>>> On 05/12/2014 03:57 PM, Hans de Goede wrote:
>>>>> Like all of the other *30 ThinkPad models, the W530 has a broken acpi-video
>>>>> backlight control. Note in order for this to actually fix things on the
>>>>> ThinkPad W530 the commit titled:
>>>>> "nouveau: Don't check acpi_video_backlight_support() before registering backlight"
>>>>> is also needed.
>>>>
>>>> Note that the backlight_device_registered(raw) may return false as when
>>>> acpi_video_verify_backlight_support is called, the nouveau driver may
>>>> not run yet.
>>>>
>>>> Previously, we don't know anything about how laptops with nvidia graphics
>>>> card alone control backlight in Win8, so the existing solution doesn't
>>>> consider this case. If nvidia graphics system also should favour native
>>>> backlight control interface in Win8, the current solution needs
>>>> modifications.
>>>
>>> Hmm, how is this dealt with in the case of the intel gfx driver ?
>>
>> Ok, I've figured out now how this is dealt with in the case of the intel gfx
>> drivers. That looks like something which will likely be hard to do for
>> nouveau, since it relies on some intel gfx specific ACPI calls being there,
>> which nouveau does not have. So this would require doing something like
>> duplicating the nouveau pci-ids or some such, which would be far from ideal.
>>
>> Still this bug: https://bugzilla.redhat.com/show_bug.cgi?id=1093171
>>
>> Shows that we need *working* video.use_native_brightness=1 support for
>> non intel too. Which the current loading order issues caused by the
>> backlight_device_registered(raw) breaks.
>>
>> So maybe we should simply drop the backlight_device_registered(raw) check?
>> I don't think we really need it any win8 supporting laptop will have
>> intel, nv or ati gfx, and all 3 of them have native backlight support,
>> which if our experiences so far is any indication we will likely want
>> to use instead of acpi_video.
> 
> We can try that, but if it turns out to break things for someone, we'll need
> a different approach.

Ok, I'll write a patch for this tomorrow. As for trying a different approach,
whatever the different approach may be, I strongly believe that we should get
rid of the various module loading order dependencies we've now, not add more.

I also have a plan now on how to cleanup the code for deciding which
backlight control interface to use, and remove the various ordering issues
we've there. But I've run out of steam for today, so I'll go and write that
up and post it tomorrow.

Regards,

Hans

Re: [PATCH] acpi-video: Add use native backlight quirk for the ThinkPad W530

[Aaron Lu]

On 05/14/2014 07:09 PM, Hans de Goede wrote:
> Hi,
> 
> On 05/12/2014 01:02 PM, Hans de Goede wrote:
>> Hi,
>>
>> On 05/12/2014 10:16 AM, Aaron Lu wrote:
>>> On 05/12/2014 03:57 PM, Hans de Goede wrote:
>>>> Like all of the other *30 ThinkPad models, the W530 has a broken acpi-video
>>>> backlight control. Note in order for this to actually fix things on the
>>>> ThinkPad W530 the commit titled:
>>>> "nouveau: Don't check acpi_video_backlight_support() before registering backlight"
>>>> is also needed.
>>>
>>> Note that the backlight_device_registered(raw) may return false as when
>>> acpi_video_verify_backlight_support is called, the nouveau driver may
>>> not run yet.
>>>
>>> Previously, we don't know anything about how laptops with nvidia graphics
>>> card alone control backlight in Win8, so the existing solution doesn't
>>> consider this case. If nvidia graphics system also should favour native
>>> backlight control interface in Win8, the current solution needs
>>> modifications.
>>
>> Hmm, how is this dealt with in the case of the intel gfx driver ?
> 
> Ok, I've figured out now how this is dealt with in the case of the intel gfx

Reading your other reply, I thought you already figured this out so I
didn't explain, sorry for that.

> drivers. That looks like something which will likely be hard to do for
> nouveau, since it relies on some intel gfx specific ACPI calls being there,
> which nouveau does not have. So this would require doing something like
> duplicating the nouveau pci-ids or some such, which would be far from ideal.
> 
> Still this bug: https://bugzilla.redhat.com/show_bug.cgi?id=1093171
> 
> Shows that we need *working* video.use_native_brightness=1 support for
> non intel too. Which the current loading order issues caused by the
> backlight_device_registered(raw) breaks.
> 
> So maybe we should simply drop the backlight_device_registered(raw) check?

Unfortunately, there are indeed systems that with Intel GFX do not have
a GPU backlight control interface:

commit c675949ec58ca50d5a3ae3c757892f1560f6e896
Author: Jani Nikula <jani.nikula@intel.com>
Date:   Wed Apr 9 11:31:37 2014 +0300

    drm/i915: do not setup backlight if not available according to VBT

And I remembered last time when we push the use_native default to 1
without checking if a raw interface is available, there are people
complaining about no backlight interface is created on his system(and
the only working interface is acpi_video on his Win8 system). So simply
dropping this check doesn't seem like a good idea.

Thanks,
Aaron

> I don't think we really need it any win8 supporting laptop will have
> intel, nv or ati gfx, and all 3 of them have native backlight support,
> which if our experiences so far is any indication we will likely want
> to use instead of acpi_video.
> 
> Regards,
> 
> Hans
> 
> 
> 
> 
> 
> 
>>
>> Regards,
>>
>> Hans
>>
>>
>>
>>>
>>> Thanks,
>>> Aaron
>>>
>>>>
>>>> https://bugzilla.redhat.com/show_bug.cgi?id=1093171
>>>>
>>>> Cc: stable@vger.kernel.org
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>>  drivers/acpi/video.c | 8 ++++++++
>>>>  1 file changed, 8 insertions(+)
>>>>
>>>> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
>>>> index 34198b2..6a2099d 100644
>>>> --- a/drivers/acpi/video.c
>>>> +++ b/drivers/acpi/video.c
>>>> @@ -520,6 +520,14 @@ static struct dmi_system_id video_dmi_table[] __initdata = {
>>>>  		},
>>>>  	},
>>>>  	{
>>>> +	 .callback = video_set_use_native_backlight,
>>>> +	 .ident = "ThinkPad W530",
>>>> +	 .matches = {
>>>> +		DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
>>>> +		DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad W530"),
>>>> +		},
>>>> +	},
>>>> +	{
>>>>  	.callback = video_set_use_native_backlight,
>>>>  	.ident = "ThinkPad X1 Carbon",
>>>>  	.matches = {
>>>>
>>>

Re: [PATCH] acpi-video: Add use native backlight quirk for the ThinkPad W530

[Hans de Goede]

Hi,

On 05/15/2014 03:45 AM, Aaron Lu wrote:
> On 05/14/2014 07:09 PM, Hans de Goede wrote:
>> Hi,
>>
>> On 05/12/2014 01:02 PM, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 05/12/2014 10:16 AM, Aaron Lu wrote:
>>>> On 05/12/2014 03:57 PM, Hans de Goede wrote:
>>>>> Like all of the other *30 ThinkPad models, the W530 has a broken acpi-video
>>>>> backlight control. Note in order for this to actually fix things on the
>>>>> ThinkPad W530 the commit titled:
>>>>> "nouveau: Don't check acpi_video_backlight_support() before registering backlight"
>>>>> is also needed.
>>>>
>>>> Note that the backlight_device_registered(raw) may return false as when
>>>> acpi_video_verify_backlight_support is called, the nouveau driver may
>>>> not run yet.
>>>>
>>>> Previously, we don't know anything about how laptops with nvidia graphics
>>>> card alone control backlight in Win8, so the existing solution doesn't
>>>> consider this case. If nvidia graphics system also should favour native
>>>> backlight control interface in Win8, the current solution needs
>>>> modifications.
>>>
>>> Hmm, how is this dealt with in the case of the intel gfx driver ?
>>
>> Ok, I've figured out now how this is dealt with in the case of the intel gfx
> 
> Reading your other reply, I thought you already figured this out so I
> didn't explain, sorry for that.
> 
>> drivers. That looks like something which will likely be hard to do for
>> nouveau, since it relies on some intel gfx specific ACPI calls being there,
>> which nouveau does not have. So this would require doing something like
>> duplicating the nouveau pci-ids or some such, which would be far from ideal.
>>
>> Still this bug: https://bugzilla.redhat.com/show_bug.cgi?id=1093171
>>
>> Shows that we need *working* video.use_native_brightness=1 support for
>> non intel too. Which the current loading order issues caused by the
>> backlight_device_registered(raw) breaks.
>>
>> So maybe we should simply drop the backlight_device_registered(raw) check?
> 
> Unfortunately, there are indeed systems that with Intel GFX do not have
> a GPU backlight control interface:
> 
> commit c675949ec58ca50d5a3ae3c757892f1560f6e896
> Author: Jani Nikula <jani.nikula@intel.com>
> Date:   Wed Apr 9 11:31:37 2014 +0300
> 
>     drm/i915: do not setup backlight if not available according to VBT
> 
> And I remembered last time when we push the use_native default to 1
> without checking if a raw interface is available, there are people
> complaining about no backlight interface is created on his system(and
> the only working interface is acpi_video on his Win8 system). So simply
> dropping this check doesn't seem like a good idea.

Hmm, ok. So any smart ideas how to deal with the ordering problem we've
here ?

Note this also plays into the proposal I'm about to send to unify and
simplify backlight control selection. Which besides just trying to
clean things up also tries to get rid of various module load ordering
issues.

... <this represent me thinking for half an hour trying to come up with a clever solution>

So I think we really need some clean and generic way to deal with this,
which is not prone to module loading ordering issues, any suggestions?

Regards,

Hans

Re: [PATCH] acpi-video: Add use native backlight quirk for the ThinkPad W530

[Hans de Goede]

Hi,

On 05/15/2014 10:56 AM, Hans de Goede wrote:

<snip>

>>> So maybe we should simply drop the backlight_device_registered(raw) check?
>>
>> Unfortunately, there are indeed systems that with Intel GFX do not have
>> a GPU backlight control interface:
>>
>> commit c675949ec58ca50d5a3ae3c757892f1560f6e896
>> Author: Jani Nikula <jani.nikula@intel.com>
>> Date:   Wed Apr 9 11:31:37 2014 +0300
>>
>>     drm/i915: do not setup backlight if not available according to VBT
>>
>> And I remembered last time when we push the use_native default to 1
>> without checking if a raw interface is available, there are people
>> complaining about no backlight interface is created on his system(and
>> the only working interface is acpi_video on his Win8 system). So simply
>> dropping this check doesn't seem like a good idea.
> 
> Hmm, ok. So any smart ideas how to deal with the ordering problem we've
> here ?
> 
> Note this also plays into the proposal I'm about to send to unify and
> simplify backlight control selection. Which besides just trying to
> clean things up also tries to get rid of various module load ordering
> issues.
> 
> ... <this represent me thinking for half an hour trying to come up with a clever solution>
> 
> So I think we really need some clean and generic way to deal with this,
> which is not prone to module loading ordering issues, any suggestions?

Ok, after yet more thinking about this I think I've what is likely going
to be the best solution for this:

1) Add a callback to the backlight core which allows interested parties
to get notified if a backlight device gets registered / unregistered

2) make acpi/video.c listen to these events and on these events re-check
acpi_video_verify_backlight_support, and if it returns something
different then the current situation (un)register the acpi_video#
backlight devices

This means that we will have ping-ponging of backlight interfaces which
I really wanted to avoid, but given all the interdepencies that
seems unavoidable. This will also simplify my cleanup proposal since
if we accept the ping-ponging all the quirks can stay in the vendor
specific firmware backlight control drivers.

So, good or bad idea ?

Regards,

Hans