RE: [PATCH] nvidia-wmi-ec-backlight: Add workarounds for confused firmware

RE: [PATCH] nvidia-wmi-ec-backlight: Add workarounds for confused firmware

[Alexandru Dinu]

Hi,

> I'll send out a v2 shortly: Alex, can you
please retest when I do to make sure there aren't any regressions? None
of these suggestions affect the core flow of how either of the
workarounds work, so I'm not expecting any that wouldn't also reproduce
on my EC backlight system that doesn't have either of these problems,
but I can send you the updated version off-list first if you prefer.

It's ok either way. You can send me an updated version off-list.

> Alex, just FYI this was something that came to an AMD bug tracker and wanted you to be aware there are W/A going into nvidia-wmi-ec-backlight for some firmware problems with the mux.
IIRC that was the original suspicion too on the bug reports.

Yes, thanks -- I followed this issue first:
https://gitlab.freedesktop.org/drm/amd/-/issues/1671.

> However I think it's still worth at least noting near the quirk in a comment
what firmware version it was identified.  If later there is confirmation that
a particular firmware version had fixed it the quirk can be adjusted to be
dropped.

That's a good tip. The laptop I tested this on (Lenovo Legion S7
15ACH6) originally shipped with:

UEFI: LENOVO v: HACN27WW date: 08/02/2021

There is an update to version HACN31WW -- see
https://support.lenovo.com/ro/en/downloads/ds550201-bios-update-for-windows-10-64-bit-legion-s7-15ach6
I updated, however, the issue was not addressed, which seems to be
expected given the rather short /changelog:
HACN31WW
BIOS Notification    :
1. Fixed
 1) N/A.
2. Add
  1) Add BOE0A1C support with Cookie and DR Key
3. Modified
  1) Modify MinShortTerm & MinLongTerm PowerLimit value
EC Notification      :
1. Fixed
  1) None.
2. Add
   1) None.
3. Modified
  1)None.

> If you end up introducing a module parameter to try to activate these quirks
it might be viable to ask the folks in those issues to try the v2 of
your patch too
when you're ready with the module parameter.

I posted a link to this mailing list to
https://gitlab.freedesktop.org/drm/amd/-/issues/1671, so people can be
aware and try to test.

Regards,
Alex

RE: [PATCH] nvidia-wmi-ec-backlight: Add workarounds for confused firmware

[Limonciello, Mario]

[Public]



> -----Original Message-----
> From: Daniel Dadap <ddadap@nvidia.com>
> Sent: Wednesday, March 16, 2022 14:24
> To: Limonciello, Mario <Mario.Limonciello@amd.com>; Barnabás Pőcze
> <pobrn@protonmail.com>
> Cc: platform-driver-x86@vger.kernel.org; Alexandru Dinu
> <alex.dinu07@gmail.com>; Hans de Goede <hdegoede@redhat.com>;
> markgross@kernel.org; Deucher, Alexander
> <Alexander.Deucher@amd.com>
> Subject: Re: [PATCH] nvidia-wmi-ec-backlight: Add workarounds for
> confused firmware
> 
> On 3/16/22 13:25, Limonciello, Mario wrote:
> > [Public]
> >
> >
> > I guess when we see backlight issues on these A+N designs the checks
> should be:
> > 1) Are they supposed to be using the nvidia-wmi-ec-backlight driver?
> > 2) Is their kernel new enough to have it?
> > 3) Do they have the config enabled?
> >
> > Do you have a script or could you perhaps include some documentation we
> can
> > point people to check "1" so we don't always have to go tear apart ACPI
> tables
> > and make guesses?
> >
> > I guess it's something like grab _WDG and then parse it to see if there is an
> entry.
> 
> 
> Probably the most foolproof way would be to check for the GUID
> 603E9613-EF25-4338-A3D0-C46177516DB7 in /sys/bus/wmi/devices. (2)
> should
> be true for vanilla 5.16 and later, and many recent pre-5.16 distro
> kernels with HWE backports.

Perfect, thanks!

Re: [PATCH] nvidia-wmi-ec-backlight: Add workarounds for confused firmware

[Daniel Dadap]

On 3/16/22 13:25, Limonciello, Mario wrote:
> [Public]
>
>
> I guess when we see backlight issues on these A+N designs the checks should be:
> 1) Are they supposed to be using the nvidia-wmi-ec-backlight driver?
> 2) Is their kernel new enough to have it?
> 3) Do they have the config enabled?
>
> Do you have a script or could you perhaps include some documentation we can
> point people to check "1" so we don't always have to go tear apart ACPI tables
> and make guesses?
>
> I guess it's something like grab _WDG and then parse it to see if there is an entry.


Probably the most foolproof way would be to check for the GUID 
603E9613-EF25-4338-A3D0-C46177516DB7 in /sys/bus/wmi/devices. (2) should 
be true for vanilla 5.16 and later, and many recent pre-5.16 distro 
kernels with HWE backports.

RE: [PATCH] nvidia-wmi-ec-backlight: Add workarounds for confused firmware

[Limonciello, Mario]

[Public]

> >
> > IIRC this is the bug you want linked in the commit message:
> >
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitla
> b.freedesktop.org%2Fdrm%2Famd%2F-
> %2Fissues%2F1671&data=04%7C01%7CMario.Limonciello%40amd.com
> %7C5559a4f23f46426add1808da0773b4ac%7C3dd8961fe4884e608e11a82d994
> e183d%7C0%7C0%7C637830490785879396%7CUnknown%7CTWFpbGZsb3d8
> eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3
> D%7C3000&sdata=P%2FBcLeN9rnjGam4kh68ZQUBAPIDM4G%2Bk1ukb5
> k%2BRFVg%3D&reserved=0
> 
> 
> Ah, thanks. Most of the people on this bug seem like their problem was
> that they didn't have the nvidia-wmi-ec-backlight driver, which also
> didn't exist at the time the bug was filed. There is one person with a
> newer comment reporting behavior that sounds like what this patch works
> around, and it is the same person who initially reported the issue to me. :)
> 
> 

Thanks for looking at those.

> > But these two look possible to be the same root cause:
> >
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitla
> b.freedesktop.org%2Fdrm%2Famd%2F-
> %2Fissues%2F1791&data=04%7C01%7CMario.Limonciello%40amd.com
> %7C5559a4f23f46426add1808da0773b4ac%7C3dd8961fe4884e608e11a82d994
> e183d%7C0%7C0%7C637830490785879396%7CUnknown%7CTWFpbGZsb3d8
> eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3
> D%7C3000&sdata=Bv3lJJOG7BZxlvizh0L4gmHgakzjlJkl7TqGh9HTho4%3D
> &reserved=0
> 
> 
> This one sounds like it might be a different issue, since it was
> apparently working at some point with a kernel that didn't have the EC
> backlight driver, and then not working on a newer kernel that also
> didn't have the EC backlight driver. That is, of course, assuming
> vanilla kernels: it is certainly possible that the EC backlight driver
> was backported.
> 
> >
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitla
> b.freedesktop.org%2Fdrm%2Famd%2F-
> %2Fissues%2F1794&data=04%7C01%7CMario.Limonciello%40amd.com
> %7C5559a4f23f46426add1808da0773b4ac%7C3dd8961fe4884e608e11a82d994
> e183d%7C0%7C0%7C637830490785879396%7CUnknown%7CTWFpbGZsb3d8
> eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3
> D%7C3000&sdata=JfUhLPRIMVLypLXoAxKhpSw7WIN4M%2BS4Y48MQ
> %2BzXdbk%3D&reserved=0
> 
> 
> This sounds like it could possibly be a simple case of not having the EC
> backlight driver. Notably, the backlight device exposed by the amdgpu
> driver never works, in contrast to the system these workarounds are
> targeting, where the amdgpu driver's backlight device initially works,
> but then stops working after the first suspend/resume cycle (and the EC
> backlight driver doesn't work initially, but then starts working after
> suspend/resume).

I guess when we see backlight issues on these A+N designs the checks should be:
1) Are they supposed to be using the nvidia-wmi-ec-backlight driver?
2) Is their kernel new enough to have it?
3) Do they have the config enabled?

Do you have a script or could you perhaps include some documentation we can
point people to check "1" so we don't always have to go tear apart ACPI tables
and make guesses?

I guess it's something like grab _WDG and then parse it to see if there is an entry.

> 
> 
> >
> > If you end up introducing a module parameter to try to activate these
> quirks
> > it might be viable to ask the folks in those issues to try the v2 of your patch
> too
> > when you're ready with the module parameter.
> >
> 
> v1 already has the quirks plumbed up to module parameters (those module
> parameters just don't have corresponding sysfs entries). In any case, I
> only see one report between those bugs that sounds like the issue these
> WARs are meant to address, and since it's from the same reporter, it
> sounds like we won't need to be adding any additional quirks table
> entries right away.
> 
> 
> >>
> >>> Comments inline as well.
> >>>
> >>>> -----Original Message-----
> >>>> From: Daniel Dadap <ddadap@nvidia.com>
> >>>> Sent: Wednesday, March 16, 2022 10:11
> >>>> To: Barnabás Pőcze <pobrn@protonmail.com>
> >>>> Cc: platform-driver-x86@vger.kernel.org; Alexandru Dinu
> >>>> <alex.dinu07@gmail.com>; Hans de Goede <hdegoede@redhat.com>;
> >>>> markgross@kernel.org
> >>>> Subject: Re: [PATCH] nvidia-wmi-ec-backlight: Add workarounds for
> >>>> confused firmware
> >>>>
> >> [ ... ]
> >>
> >>
> >>>> On 3/15/22 9:50 PM, Barnabás Pőcze wrote:
> >>>>>    [ ... ]
> >>>>> Lastly, is it expected that these bugs will be properly fixed?
> >>>> Possibly, but I wouldn't hold out hope for it for an issue at this scale
> >>>> on an already shipping system.
> >>> This question I'm assuming was aimed at narrowing the quirk to only
> >>> match certain FW versions or so.  If there is no certainty of when/if it
> >>> will be fixed I agree with current direction.
> >>> However I think it's still worth at least noting near the quirk in a
> comment
> >>> what firmware version it was identified.  If later there is confirmation
> that
> >>> a particular firmware version had fixed it the quirk can be adjusted to be
> >>> dropped.
> >>>
> >> Thanks, Mario. Sure, I'll make sure the firmware version this was first
> >> observed in is noted.
> >>
> >>

Re: [PATCH] nvidia-wmi-ec-backlight: Add workarounds for confused firmware

[Daniel Dadap]

On 3/16/22 12:21 PM, Limonciello, Mario wrote:
> [Public]
>
>> On 3/16/22 10:29 AM, Limonciello, Mario wrote:
>>> [Public]
>>>
>>> + Alex D
>>>
>>> Alex, just FYI this was something that came to an AMD bug tracker and
>> wanted you to be aware there are W/A going into nvidia-wmi-ec-backlight
>> for some firmware problems with the mux.
>>> IIRC that was the original suspicion too on the bug reports.
>>
>> Is this on a public or private bug tracker? If this was observed on
>> systems other than the one already added to these quirks, could you
>> share the details of the systems so they can be added as well? (Or I
>> suppose you may want to test to see if these WARs are effective on the
>> affected systems as well; we can always expand the quirks table later.)
> We (AMD folks) don't have the affected systems, we were just trying to help
> users and things pointed at this driver, which seems to have yielded a good
> investigation and conclusion!
>
> IIRC this is the bug you want linked in the commit message:
> https://gitlab.freedesktop.org/drm/amd/-/issues/1671


Ah, thanks. Most of the people on this bug seem like their problem was 
that they didn't have the nvidia-wmi-ec-backlight driver, which also 
didn't exist at the time the bug was filed. There is one person with a 
newer comment reporting behavior that sounds like what this patch works 
around, and it is the same person who initially reported the issue to me. :)


> But these two look possible to be the same root cause:
> https://gitlab.freedesktop.org/drm/amd/-/issues/1791


This one sounds like it might be a different issue, since it was 
apparently working at some point with a kernel that didn't have the EC 
backlight driver, and then not working on a newer kernel that also 
didn't have the EC backlight driver. That is, of course, assuming 
vanilla kernels: it is certainly possible that the EC backlight driver 
was backported.

> https://gitlab.freedesktop.org/drm/amd/-/issues/1794


This sounds like it could possibly be a simple case of not having the EC 
backlight driver. Notably, the backlight device exposed by the amdgpu 
driver never works, in contrast to the system these workarounds are 
targeting, where the amdgpu driver's backlight device initially works, 
but then stops working after the first suspend/resume cycle (and the EC 
backlight driver doesn't work initially, but then starts working after 
suspend/resume).


>
> If you end up introducing a module parameter to try to activate these quirks
> it might be viable to ask the folks in those issues to try the v2 of your patch too
> when you're ready with the module parameter.
>

v1 already has the quirks plumbed up to module parameters (those module 
parameters just don't have corresponding sysfs entries). In any case, I 
only see one report between those bugs that sounds like the issue these 
WARs are meant to address, and since it's from the same reporter, it 
sounds like we won't need to be adding any additional quirks table 
entries right away.


>>
>>> Comments inline as well.
>>>
>>>> -----Original Message-----
>>>> From: Daniel Dadap <ddadap@nvidia.com>
>>>> Sent: Wednesday, March 16, 2022 10:11
>>>> To: Barnabás Pőcze <pobrn@protonmail.com>
>>>> Cc: platform-driver-x86@vger.kernel.org; Alexandru Dinu
>>>> <alex.dinu07@gmail.com>; Hans de Goede <hdegoede@redhat.com>;
>>>> markgross@kernel.org
>>>> Subject: Re: [PATCH] nvidia-wmi-ec-backlight: Add workarounds for
>>>> confused firmware
>>>>
>> [ ... ]
>>
>>
>>>> On 3/15/22 9:50 PM, Barnabás Pőcze wrote:
>>>>>    [ ... ]
>>>>> Lastly, is it expected that these bugs will be properly fixed?
>>>> Possibly, but I wouldn't hold out hope for it for an issue at this scale
>>>> on an already shipping system.
>>> This question I'm assuming was aimed at narrowing the quirk to only
>>> match certain FW versions or so.  If there is no certainty of when/if it
>>> will be fixed I agree with current direction.
>>> However I think it's still worth at least noting near the quirk in a comment
>>> what firmware version it was identified.  If later there is confirmation that
>>> a particular firmware version had fixed it the quirk can be adjusted to be
>>> dropped.
>>>
>> Thanks, Mario. Sure, I'll make sure the firmware version this was first
>> observed in is noted.
>>
>>

Re: [PATCH] nvidia-wmi-ec-backlight: Add workarounds for confused firmware

[Daniel Dadap]

On 3/16/22 11:09 AM, Hans de Goede wrote:
> Hi,
>
> On 3/16/22 02:25, Daniel Dadap wrote:
>> Some notebook systems with EC-driven backlight control appear to have a
>> firmware bug which causes the system to use GPU-driven backlight control
>> upon a fresh boot, but then switches to EC-driven backlight control
>> after completing a suspend/resume cycle. All the while, the firmware
>> reports that the backlight is under EC control, regardless of what is
>> actually controlling the backlight brightness.
>>
>> This leads to the following behavior:
>>
>> * nvidia-wmi-ec-backlight gets probed on a fresh boot, due to the
>>    WMI-wrapped ACPI method erroneously reporting EC control.
>> * nvidia-wmi-ec-backlight does not work until after a suspend/resume
>>    cycle, due to the backlight control actually being GPU-driven.
>> * GPU drivers also register their own backlight handlers: in the case
>>    of the notebook system where this behavior has been observed, both
>>    amdgpu and the NVIDIA proprietary driver register backlight handlers.
>> * The GPU which has backlight control upon a fresh boot (amdgpu in the
>>    case observed so far) can successfully control the backlight through
>>    its backlight driver's sysfs interface, but stops working after the
>>    first suspend/resume cycle.
>> * nvidia-wmi-ec-backlight is unable to control the backlight upon a
>>    fresh boot, but begins to work after the first suspend/resume cycle.
>> * The GPU which does not have backlight control (NVIDIA in this case)
>>    is not able to control the backlight at any point while the system
>>    is in operation. On similar hybrid systems with an EC-controlled
>>    backlight, and AMD/NVIDIA iGPU/dGPU, the NVIDIA proprietary driver
>>    does not register its backlight handler. It has not been determined
>>    whether the non-functional handler registered by the NVIDIA driver
>>    is due to another firmware bug, or a bug in the NVIDIA driver.
>>
>> Since nvidia-wmi-ec-backlight registers as a BACKLIGHT_FIRMWARE type
>> device, it takes precedence over the BACKLIGHT_RAW devices registered
>> by the GPU drivers. This in turn leads to backlight control appearing
>> to be non-functional until after completing a suspend/resume cycle.
>> However, it is still possible to control the backlight through direct
>> interaction with the working GPU driver's backlight sysfs interface.
>>
>> These systems also appear to have a second firmware bug which resets
>> the EC's brightness level to 100% on resume, but leaves the state in
>> the kernel at the pre-suspend level. This causes attempts to save
>> and restore the backlight level across the suspend/resume cycle to
>> fail, due to the level appearing not to change even though it did.
>>
>> In order to work around these issue, add quirk tables to detect
>> systems that are known to show these behaviors. So far, there is
>> only one known system that requires these workarounds, and both
>> issues are present on that system, but the quirks are tracked in
>> separate tables to make it easier to add them to other systems which
>> may exhibit one of the bugs, but not the other. The original systems
>> that this driver was tested on during development do not exhibit
>> either of these quirks.
>>
>> If a system with the "GPU driver has backlight control" quirk is
>> detected, nvidia-wmi-ec-backlight will grab a reference to the working
>> (when freshly booted) GPU backlight handler and relays any backlight
>> brightness level change requests directed at the EC to also be applied
>> to the GPU backlight interface. This leads to redundant updates
>> directed at the GPU backlight driver after a suspend/resume cycle, but
>> it does allow the EC backlight control to work when the system is
>> freshly booted.
>>
>> If a system with the "backlight level reset to full on resume" quirk
>> is detected, nvidia-wmi-ec-backlight will register a PM notifier to
>> reset the backlight to the previous level upon resume.
>>
>> These workarounds are also plumbed through to kernel module parameters,
>> to make it easier for users who suspect they may be affected by one or
>> both of these bugs to test whether these workarounds are effective on
>> their systems as well.
>>
>> Signed-off-by: Daniel Dadap <ddadap@nvidia.com>
>> Tested-by: Alexandru Dinu <alex.dinu07@gmail.com>
>> ---
>>   .../platform/x86/nvidia-wmi-ec-backlight.c    | 181 +++++++++++++++++-
>>   1 file changed, 179 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/platform/x86/nvidia-wmi-ec-backlight.c b/drivers/platform/x86/nvidia-wmi-ec-backlight.c
>> index 61e37194df70..ccb3b506c12c 100644
>> --- a/drivers/platform/x86/nvidia-wmi-ec-backlight.c
>> +++ b/drivers/platform/x86/nvidia-wmi-ec-backlight.c
>> @@ -3,8 +3,11 @@
>>    * Copyright (c) 2020, NVIDIA CORPORATION.  All rights reserved.
>>    */
>>   
>> +#define pr_fmt(f) "%s: " f "\n", KBUILD_MODNAME
>> +
>>   #include <linux/acpi.h>
>>   #include <linux/backlight.h>
>> +#include <linux/dmi.h>
>>   #include <linux/mod_devicetable.h>
>>   #include <linux/module.h>
>>   #include <linux/types.h>
>> @@ -75,6 +78,69 @@ struct wmi_brightness_args {
>>   	u32 ignored[3];
>>   };
>>   
>> +/**
>> + * struct nvidia_wmi_ec_backlight_priv - driver private data
>> + * @bl_dev:       the associated backlight device
>> + * @proxy_target: backlight device which receives relayed brightness changes
>> + * @notifier:     notifier block for resume callback
>> + */
>> +struct nvidia_wmi_ec_backlight_priv {
>> +	struct backlight_device *bl_dev;
>> +	struct backlight_device *proxy_target;
>> +	struct notifier_block nb;
>> +};
>> +
>> +static char *backlight_proxy_target;
>> +module_param(backlight_proxy_target, charp, 0);
>> +MODULE_PARM_DESC(backlight_proxy_target, "Relay brightness change requests to the named backlight driver, on systems which erroneously report EC backlight control.");
>> +
>> +static int max_reprobe_attempts = 128;
>> +module_param(max_reprobe_attempts, int, 0);
>> +MODULE_PARM_DESC(max_reprobe_attempts, "Limit of reprobe attempts when relaying brightness change requests.");
>> +
>> +static bool restore_level_on_resume;
>> +module_param(restore_level_on_resume, bool, 0);
>> +MODULE_PARM_DESC(restore_level_on_resume, "Restore the backlight level when resuming from suspend, on systems which reset the EC's backlight level on resume.");
>> +
>> +static int assign_relay_quirk(const struct dmi_system_id *id)
>> +{
>> +	backlight_proxy_target = id->driver_data;
>> +	return true;
>> +}
>> +
>> +#define PROXY_QUIRK_ENTRY(vendor, product, quirk_data) { \
>> +	.callback = assign_relay_quirk,                  \
>> +	.matches = {                                     \
>> +		DMI_MATCH(DMI_SYS_VENDOR, vendor),       \
>> +		DMI_MATCH(DMI_PRODUCT_VERSION, product)  \
>> +	},                                               \
>> +	.driver_data = quirk_data                        \
>> +}
>> +
>> +static const struct dmi_system_id proxy_quirk_table[] = {
>> +	PROXY_QUIRK_ENTRY("LENOVO", "Legion S7 15ACH6", "amdgpu_bl1"),
>> +	{ }
>> +};
>> +
>> +static int assign_restore_quirk(const struct dmi_system_id *id)
>> +{
>> +	restore_level_on_resume = true;
>> +	return true;
>> +}
>> +
>> +#define RESTORE_QUIRK_ENTRY(vendor, product) {           \
>> +	.callback = assign_restore_quirk,                \
>> +	.matches = {                                     \
>> +		DMI_MATCH(DMI_SYS_VENDOR, vendor),       \
>> +		DMI_MATCH(DMI_PRODUCT_VERSION, product), \
>> +	}                                                \
>> +}
>> +
>> +static const struct dmi_system_id restore_quirk_table[] = {
>> +	RESTORE_QUIRK_ENTRY("LENOVO", "Legion S7 15ACH6"),
>> +	{ }
>> +};
>> +
>>   /**
>>    * wmi_brightness_notify() - helper function for calling WMI-wrapped ACPI method
>>    * @w:    Pointer to the struct wmi_device identified by %WMI_BRIGHTNESS_GUID
> Note not a full review, just something which I noticed on a quick scan,
> please use only 1 dmi_system_id table and make driver_data a bit field.
>
> Here is some example code for that copied from another recent review:
>
> So you would get something like this:
>
> #define SERIO_QUIRK_RESET		BIT(0)
> #define SERIO_QUIRK_NOMUX		BIT(1)
> #define SERIO_QUIRK_NOPNP		BIT(2)
> #define SERIO_QUIRK_NOLOOP		BIT(3)
> #define SERIO_QUIRK_NOSELFTEST		BIT(4)
> // etc.
>
> static const struct dmi_system_id i8042_dmi_quirk_table[] __initconst = {
>          {
>                  /* Entroware Proteus */
>                  .matches = {
>                          DMI_MATCH(DMI_SYS_VENDOR, "Entroware"),
>                          DMI_MATCH(DMI_PRODUCT_NAME, "Proteus"),
>                          DMI_MATCH(DMI_PRODUCT_VERSION, "EL07R4"),
>                  },
> 		.driver_data = (void *)(SERIO_QUIRK_RESET | SERIO_QUIRK_NOMUX)
>          },
> 	{}
> };


Thanks, yes: merging the tables would be pretty straightforward. I 
actually thought I might do a unified quirks table when we noticed the 
second quirk, but then thought it was kind of gross to cast a bit field 
to a pointer and then then back. I didn't think to check for prior art 
to see that in fact, this is exactly what other drivers do. I also was 
slightly worried about running out of bits if there are enough unique 
GPU backlight device names among other affected systems, but the 
likelihood of that happening seems remote enough that it isn't really 
worth considering.


> I picked the Entroware EL07R4 as example here because it needs both the reset and nomux quirks.
>
> And then when checking the quirks do:
>
> #ifdef CONFIG_X86
> 	const struct dmi_system_id *dmi_id;
> 	long quirks = 0;
>
> 	dmi_id = dmi_first_match(i8042_dmi_quirk_table);
> 	if (dmi_id)
> 		quirks = (long)dmi_id->driver_data;
>
> 	if (i8042_reset == I8042_RESET_DEFAULT) {
> 		if (quirks & SERIO_QUIRK_RESET)
> 			i8042_reset = I8042_RESET_ALWAYS;
> 		if (quirks & SERIO_QUIRK_NOSELFTEST)
> 			i8042_reset = I8042_RESET_NEVER;
> 	}
>
>
> This will already shrink the driver a bit by not having 2 dmi_system_id structs
> for the single laptop model and this will also help to avoid getting even
> more dmi_system_id tables if further quirks are necessary in the future,
> basically I want to avoid ending up with something like the somewhat messy
> code which is being cleaned-up here:
>
> https://lore.kernel.org/linux-input/20220308170523.783284-2-wse@tuxedocomputers.com/
>
> Regards,
>
> Hans
>
>
>
>
>
>
>
>> @@ -119,9 +185,30 @@ static int wmi_brightness_notify(struct wmi_device *w, enum wmi_brightness_metho
>>   	return 0;
>>   }
>>   
>> +static int scale_backlight_level(struct backlight_device *a,
>> +				 struct backlight_device *b)
>> +{
>> +	/* because floating point math in the kernel is annoying */
>> +	const int scaling_factor = 65536;
>> +	int level = a->props.brightness;
>> +	int relative_level = level * scaling_factor / a->props.max_brightness;
>> +
>> +	return relative_level * b->props.max_brightness / scaling_factor;
>> +}
>> +
>>   static int nvidia_wmi_ec_backlight_update_status(struct backlight_device *bd)
>>   {
>>   	struct wmi_device *wdev = bl_get_data(bd);
>> +	struct nvidia_wmi_ec_backlight_priv *priv = dev_get_drvdata(&wdev->dev);
>> +	struct backlight_device *proxy_target = priv->proxy_target;
>> +
>> +	if (proxy_target) {
>> +		int level = scale_backlight_level(bd, proxy_target);
>> +
>> +		if (backlight_device_set_brightness(proxy_target, level))
>> +			pr_warn("Failed to relay backlight update to \"%s\"",
>> +				backlight_proxy_target);
>> +	}
>>   
>>   	return wmi_brightness_notify(wdev, WMI_BRIGHTNESS_METHOD_LEVEL,
>>   	                             WMI_BRIGHTNESS_MODE_SET,
>> @@ -147,13 +234,65 @@ static const struct backlight_ops nvidia_wmi_ec_backlight_ops = {
>>   	.get_brightness = nvidia_wmi_ec_backlight_get_brightness,
>>   };
>>   
>> +static int nvidia_wmi_ec_backlight_pm_notifier(struct notifier_block *nb, unsigned long event, void *d)
>> +{
>> +
>> +	/*
>> +	 * On some systems, the EC backlight level gets reset to 100% when
>> +	 * resuming from suspend, but the backlight device state still reflects
>> +	 * the pre-suspend value. Refresh the existing state to sync the EC's
>> +	 * state back up with the kernel's.
>> +	 */
>> +	if (event == PM_POST_SUSPEND) {
>> +		struct nvidia_wmi_ec_backlight_priv *p;
>> +
>> +		p = container_of(nb, struct nvidia_wmi_ec_backlight_priv, nb);
>> +		return backlight_update_status(p->bl_dev);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>   static int nvidia_wmi_ec_backlight_probe(struct wmi_device *wdev, const void *ctx)
>>   {
>> +	struct backlight_device *bdev, *target = NULL;
>> +	struct nvidia_wmi_ec_backlight_priv *priv;
>>   	struct backlight_properties props = {};
>> -	struct backlight_device *bdev;
>>   	u32 source;
>>   	int ret;
>>   
>> +	/*
>> +	 * Check quirks tables to see if this system needs any of the firmware
>> +	 * bug workarounds.
>> +	 */
>> +
>> +	/* User-set quirks from the module parameters take precedence */
>> +	if (!backlight_proxy_target)
>> +		dmi_check_system(proxy_quirk_table);
>> +
>> +	dmi_check_system(restore_quirk_table);
>> +
>> +	if (backlight_proxy_target && backlight_proxy_target[0]) {
>> +		static int num_reprobe_attempts;
>> +
>> +		target = backlight_device_get_by_name(backlight_proxy_target);
>> +
>> +		if (!target) {
>> +			/*
>> +			 * The target backlight device might not be ready;
>> +			 * try again and disable backlight proxying if it
>> +			 * fails too many times.
>> +			 */
>> +			if (num_reprobe_attempts < max_reprobe_attempts) {
>> +				num_reprobe_attempts++;
>> +				return -EPROBE_DEFER;
>> +			}
>> +
>> +			pr_warn("Unable to acquire %s after %d attempts. Disabling backlight proxy.",
>> +				backlight_proxy_target, max_reprobe_attempts);
>> +		}
>> +	}
>> +
>>   	ret = wmi_brightness_notify(wdev, WMI_BRIGHTNESS_METHOD_SOURCE,
>>   	                           WMI_BRIGHTNESS_MODE_GET, &source);
>>   	if (ret)
>> @@ -188,7 +327,44 @@ static int nvidia_wmi_ec_backlight_probe(struct wmi_device *wdev, const void *ct
>>   					      &wdev->dev, wdev,
>>   					      &nvidia_wmi_ec_backlight_ops,
>>   					      &props);
>> -	return PTR_ERR_OR_ZERO(bdev);
>> +
>> +	if (IS_ERR(bdev))
>> +		return PTR_ERR(bdev);
>> +
>> +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
>> +	priv->bl_dev = bdev;
>> +
>> +	dev_set_drvdata(&wdev->dev, priv);
>> +
>> +	if (target) {
>> +		int level = scale_backlight_level(target, bdev);
>> +
>> +		if (backlight_device_set_brightness(bdev, level))
>> +			pr_warn("Unable to import initial brightness level from %s.",
>> +				backlight_proxy_target);
>> +		priv->proxy_target = target;
>> +	}
>> +
>> +	if (restore_level_on_resume) {
>> +		priv->nb.notifier_call = nvidia_wmi_ec_backlight_pm_notifier;
>> +		register_pm_notifier(&priv->nb);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void nvidia_wmi_ec_backlight_remove(struct wmi_device *wdev)
>> +{
>> +	struct nvidia_wmi_ec_backlight_priv *priv = dev_get_drvdata(&wdev->dev);
>> +	struct backlight_device *proxy_target = priv->proxy_target;
>> +
>> +	if (proxy_target)
>> +		put_device(&proxy_target->dev);
>> +
>> +	if (priv->nb.notifier_call)
>> +		unregister_pm_notifier(&priv->nb);
>> +
>> +	kfree(priv);
>>   }
>>   
>>   #define WMI_BRIGHTNESS_GUID "603E9613-EF25-4338-A3D0-C46177516DB7"
>> @@ -204,6 +380,7 @@ static struct wmi_driver nvidia_wmi_ec_backlight_driver = {
>>   		.name = "nvidia-wmi-ec-backlight",
>>   	},
>>   	.probe = nvidia_wmi_ec_backlight_probe,
>> +	.remove = nvidia_wmi_ec_backlight_remove,
>>   	.id_table = nvidia_wmi_ec_backlight_id_table,
>>   };
>>   module_wmi_driver(nvidia_wmi_ec_backlight_driver);

RE: [PATCH] nvidia-wmi-ec-backlight: Add workarounds for confused firmware

[Limonciello, Mario]

[Public]

> On 3/16/22 10:29 AM, Limonciello, Mario wrote:
> > [Public]
> >
> > + Alex D
> >
> > Alex, just FYI this was something that came to an AMD bug tracker and
> wanted you to be aware there are W/A going into nvidia-wmi-ec-backlight
> for some firmware problems with the mux.
> > IIRC that was the original suspicion too on the bug reports.
> 
> 
> Is this on a public or private bug tracker? If this was observed on
> systems other than the one already added to these quirks, could you
> share the details of the systems so they can be added as well? (Or I
> suppose you may want to test to see if these WARs are effective on the
> affected systems as well; we can always expand the quirks table later.)

We (AMD folks) don't have the affected systems, we were just trying to help
users and things pointed at this driver, which seems to have yielded a good
investigation and conclusion!

IIRC this is the bug you want linked in the commit message:
https://gitlab.freedesktop.org/drm/amd/-/issues/1671

But these two look possible to be the same root cause:
https://gitlab.freedesktop.org/drm/amd/-/issues/1791
https://gitlab.freedesktop.org/drm/amd/-/issues/1794

If you end up introducing a module parameter to try to activate these quirks
it might be viable to ask the folks in those issues to try the v2 of your patch too
when you're ready with the module parameter.

> 
> 
> > Comments inline as well.
> >
> >> -----Original Message-----
> >> From: Daniel Dadap <ddadap@nvidia.com>
> >> Sent: Wednesday, March 16, 2022 10:11
> >> To: Barnabás Pőcze <pobrn@protonmail.com>
> >> Cc: platform-driver-x86@vger.kernel.org; Alexandru Dinu
> >> <alex.dinu07@gmail.com>; Hans de Goede <hdegoede@redhat.com>;
> >> markgross@kernel.org
> >> Subject: Re: [PATCH] nvidia-wmi-ec-backlight: Add workarounds for
> >> confused firmware
> >>
> 
> [ ... ]
> 
> 
> >>
> >> On 3/15/22 9:50 PM, Barnabás Pőcze wrote:
> >>>   [ ... ]
> >>> Lastly, is it expected that these bugs will be properly fixed?
> >>
> >> Possibly, but I wouldn't hold out hope for it for an issue at this scale
> >> on an already shipping system.
> > This question I'm assuming was aimed at narrowing the quirk to only
> > match certain FW versions or so.  If there is no certainty of when/if it
> > will be fixed I agree with current direction.
> > However I think it's still worth at least noting near the quirk in a comment
> > what firmware version it was identified.  If later there is confirmation that
> > a particular firmware version had fixed it the quirk can be adjusted to be
> > dropped.
> >
> 
> Thanks, Mario. Sure, I'll make sure the firmware version this was first
> observed in is noted.
> 
> 

Re: [PATCH] nvidia-wmi-ec-backlight: Add workarounds for confused firmware

[Daniel Dadap]

On 3/16/22 10:29 AM, Limonciello, Mario wrote:
> [Public]
>
> + Alex D
>
> Alex, just FYI this was something that came to an AMD bug tracker and wanted you to be aware there are W/A going into nvidia-wmi-ec-backlight for some firmware problems with the mux.
> IIRC that was the original suspicion too on the bug reports.


Is this on a public or private bug tracker? If this was observed on 
systems other than the one already added to these quirks, could you 
share the details of the systems so they can be added as well? (Or I 
suppose you may want to test to see if these WARs are effective on the 
affected systems as well; we can always expand the quirks table later.)


> Comments inline as well.
>
>> -----Original Message-----
>> From: Daniel Dadap <ddadap@nvidia.com>
>> Sent: Wednesday, March 16, 2022 10:11
>> To: Barnabás Pőcze <pobrn@protonmail.com>
>> Cc: platform-driver-x86@vger.kernel.org; Alexandru Dinu
>> <alex.dinu07@gmail.com>; Hans de Goede <hdegoede@redhat.com>;
>> markgross@kernel.org
>> Subject: Re: [PATCH] nvidia-wmi-ec-backlight: Add workarounds for
>> confused firmware
>>

[ ... ]


>>
>> On 3/15/22 9:50 PM, Barnabás Pőcze wrote:
>>>   [ ... ]
>>> Lastly, is it expected that these bugs will be properly fixed?
>>
>> Possibly, but I wouldn't hold out hope for it for an issue at this scale
>> on an already shipping system.
> This question I'm assuming was aimed at narrowing the quirk to only
> match certain FW versions or so.  If there is no certainty of when/if it
> will be fixed I agree with current direction.
> However I think it's still worth at least noting near the quirk in a comment
> what firmware version it was identified.  If later there is confirmation that
> a particular firmware version had fixed it the quirk can be adjusted to be
> dropped.
>

Thanks, Mario. Sure, I'll make sure the firmware version this was first 
observed in is noted.

Re: [PATCH] nvidia-wmi-ec-backlight: Add workarounds for confused firmware

[Hans de Goede]

Hi,

On 3/16/22 02:25, Daniel Dadap wrote:
> Some notebook systems with EC-driven backlight control appear to have a
> firmware bug which causes the system to use GPU-driven backlight control
> upon a fresh boot, but then switches to EC-driven backlight control
> after completing a suspend/resume cycle. All the while, the firmware
> reports that the backlight is under EC control, regardless of what is
> actually controlling the backlight brightness.
> 
> This leads to the following behavior:
> 
> * nvidia-wmi-ec-backlight gets probed on a fresh boot, due to the
>   WMI-wrapped ACPI method erroneously reporting EC control.
> * nvidia-wmi-ec-backlight does not work until after a suspend/resume
>   cycle, due to the backlight control actually being GPU-driven.
> * GPU drivers also register their own backlight handlers: in the case
>   of the notebook system where this behavior has been observed, both
>   amdgpu and the NVIDIA proprietary driver register backlight handlers.
> * The GPU which has backlight control upon a fresh boot (amdgpu in the
>   case observed so far) can successfully control the backlight through
>   its backlight driver's sysfs interface, but stops working after the
>   first suspend/resume cycle.
> * nvidia-wmi-ec-backlight is unable to control the backlight upon a
>   fresh boot, but begins to work after the first suspend/resume cycle.
> * The GPU which does not have backlight control (NVIDIA in this case)
>   is not able to control the backlight at any point while the system
>   is in operation. On similar hybrid systems with an EC-controlled
>   backlight, and AMD/NVIDIA iGPU/dGPU, the NVIDIA proprietary driver
>   does not register its backlight handler. It has not been determined
>   whether the non-functional handler registered by the NVIDIA driver
>   is due to another firmware bug, or a bug in the NVIDIA driver.
> 
> Since nvidia-wmi-ec-backlight registers as a BACKLIGHT_FIRMWARE type
> device, it takes precedence over the BACKLIGHT_RAW devices registered
> by the GPU drivers. This in turn leads to backlight control appearing
> to be non-functional until after completing a suspend/resume cycle.
> However, it is still possible to control the backlight through direct
> interaction with the working GPU driver's backlight sysfs interface.
> 
> These systems also appear to have a second firmware bug which resets
> the EC's brightness level to 100% on resume, but leaves the state in
> the kernel at the pre-suspend level. This causes attempts to save
> and restore the backlight level across the suspend/resume cycle to
> fail, due to the level appearing not to change even though it did.
> 
> In order to work around these issue, add quirk tables to detect
> systems that are known to show these behaviors. So far, there is
> only one known system that requires these workarounds, and both
> issues are present on that system, but the quirks are tracked in
> separate tables to make it easier to add them to other systems which
> may exhibit one of the bugs, but not the other. The original systems
> that this driver was tested on during development do not exhibit
> either of these quirks.
> 
> If a system with the "GPU driver has backlight control" quirk is
> detected, nvidia-wmi-ec-backlight will grab a reference to the working
> (when freshly booted) GPU backlight handler and relays any backlight
> brightness level change requests directed at the EC to also be applied
> to the GPU backlight interface. This leads to redundant updates
> directed at the GPU backlight driver after a suspend/resume cycle, but
> it does allow the EC backlight control to work when the system is
> freshly booted.
> 
> If a system with the "backlight level reset to full on resume" quirk
> is detected, nvidia-wmi-ec-backlight will register a PM notifier to
> reset the backlight to the previous level upon resume.
> 
> These workarounds are also plumbed through to kernel module parameters,
> to make it easier for users who suspect they may be affected by one or
> both of these bugs to test whether these workarounds are effective on
> their systems as well.
> 
> Signed-off-by: Daniel Dadap <ddadap@nvidia.com>
> Tested-by: Alexandru Dinu <alex.dinu07@gmail.com>
> ---
>  .../platform/x86/nvidia-wmi-ec-backlight.c    | 181 +++++++++++++++++-
>  1 file changed, 179 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/nvidia-wmi-ec-backlight.c b/drivers/platform/x86/nvidia-wmi-ec-backlight.c
> index 61e37194df70..ccb3b506c12c 100644
> --- a/drivers/platform/x86/nvidia-wmi-ec-backlight.c
> +++ b/drivers/platform/x86/nvidia-wmi-ec-backlight.c
> @@ -3,8 +3,11 @@
>   * Copyright (c) 2020, NVIDIA CORPORATION.  All rights reserved.
>   */
>  
> +#define pr_fmt(f) "%s: " f "\n", KBUILD_MODNAME
> +
>  #include <linux/acpi.h>
>  #include <linux/backlight.h>
> +#include <linux/dmi.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/module.h>
>  #include <linux/types.h>
> @@ -75,6 +78,69 @@ struct wmi_brightness_args {
>  	u32 ignored[3];
>  };
>  
> +/**
> + * struct nvidia_wmi_ec_backlight_priv - driver private data
> + * @bl_dev:       the associated backlight device
> + * @proxy_target: backlight device which receives relayed brightness changes
> + * @notifier:     notifier block for resume callback
> + */
> +struct nvidia_wmi_ec_backlight_priv {
> +	struct backlight_device *bl_dev;
> +	struct backlight_device *proxy_target;
> +	struct notifier_block nb;
> +};
> +
> +static char *backlight_proxy_target;
> +module_param(backlight_proxy_target, charp, 0);
> +MODULE_PARM_DESC(backlight_proxy_target, "Relay brightness change requests to the named backlight driver, on systems which erroneously report EC backlight control.");
> +
> +static int max_reprobe_attempts = 128;
> +module_param(max_reprobe_attempts, int, 0);
> +MODULE_PARM_DESC(max_reprobe_attempts, "Limit of reprobe attempts when relaying brightness change requests.");
> +
> +static bool restore_level_on_resume;
> +module_param(restore_level_on_resume, bool, 0);
> +MODULE_PARM_DESC(restore_level_on_resume, "Restore the backlight level when resuming from suspend, on systems which reset the EC's backlight level on resume.");
> +
> +static int assign_relay_quirk(const struct dmi_system_id *id)
> +{
> +	backlight_proxy_target = id->driver_data;
> +	return true;
> +}
> +
> +#define PROXY_QUIRK_ENTRY(vendor, product, quirk_data) { \
> +	.callback = assign_relay_quirk,                  \
> +	.matches = {                                     \
> +		DMI_MATCH(DMI_SYS_VENDOR, vendor),       \
> +		DMI_MATCH(DMI_PRODUCT_VERSION, product)  \
> +	},                                               \
> +	.driver_data = quirk_data                        \
> +}
> +
> +static const struct dmi_system_id proxy_quirk_table[] = {
> +	PROXY_QUIRK_ENTRY("LENOVO", "Legion S7 15ACH6", "amdgpu_bl1"),
> +	{ }
> +};
> +
> +static int assign_restore_quirk(const struct dmi_system_id *id)
> +{
> +	restore_level_on_resume = true;
> +	return true;
> +}
> +
> +#define RESTORE_QUIRK_ENTRY(vendor, product) {           \
> +	.callback = assign_restore_quirk,                \
> +	.matches = {                                     \
> +		DMI_MATCH(DMI_SYS_VENDOR, vendor),       \
> +		DMI_MATCH(DMI_PRODUCT_VERSION, product), \
> +	}                                                \
> +}
> +
> +static const struct dmi_system_id restore_quirk_table[] = {
> +	RESTORE_QUIRK_ENTRY("LENOVO", "Legion S7 15ACH6"),
> +	{ }
> +};
> +
>  /**
>   * wmi_brightness_notify() - helper function for calling WMI-wrapped ACPI method
>   * @w:    Pointer to the struct wmi_device identified by %WMI_BRIGHTNESS_GUID

Note not a full review, just something which I noticed on a quick scan,
please use only 1 dmi_system_id table and make driver_data a bit field.

Here is some example code for that copied from another recent review:

So you would get something like this:

#define SERIO_QUIRK_RESET		BIT(0)
#define SERIO_QUIRK_NOMUX		BIT(1)
#define SERIO_QUIRK_NOPNP		BIT(2)
#define SERIO_QUIRK_NOLOOP		BIT(3)
#define SERIO_QUIRK_NOSELFTEST		BIT(4)
// etc.

static const struct dmi_system_id i8042_dmi_quirk_table[] __initconst = {
        {
                /* Entroware Proteus */
                .matches = {
                        DMI_MATCH(DMI_SYS_VENDOR, "Entroware"),
                        DMI_MATCH(DMI_PRODUCT_NAME, "Proteus"),
                        DMI_MATCH(DMI_PRODUCT_VERSION, "EL07R4"),
                },
		.driver_data = (void *)(SERIO_QUIRK_RESET | SERIO_QUIRK_NOMUX)
        },
	{}
};

I picked the Entroware EL07R4 as example here because it needs both the reset and nomux quirks.

And then when checking the quirks do:

#ifdef CONFIG_X86
	const struct dmi_system_id *dmi_id;
	long quirks = 0;

	dmi_id = dmi_first_match(i8042_dmi_quirk_table);
	if (dmi_id)
		quirks = (long)dmi_id->driver_data;

	if (i8042_reset == I8042_RESET_DEFAULT) {
		if (quirks & SERIO_QUIRK_RESET)
			i8042_reset = I8042_RESET_ALWAYS;
		if (quirks & SERIO_QUIRK_NOSELFTEST)
			i8042_reset = I8042_RESET_NEVER;
	}


This will already shrink the driver a bit by not having 2 dmi_system_id structs
for the single laptop model and this will also help to avoid getting even
more dmi_system_id tables if further quirks are necessary in the future,
basically I want to avoid ending up with something like the somewhat messy
code which is being cleaned-up here:

https://lore.kernel.org/linux-input/20220308170523.783284-2-wse@tuxedocomputers.com/

Regards,

Hans







> @@ -119,9 +185,30 @@ static int wmi_brightness_notify(struct wmi_device *w, enum wmi_brightness_metho
>  	return 0;
>  }
>  
> +static int scale_backlight_level(struct backlight_device *a,
> +				 struct backlight_device *b)
> +{
> +	/* because floating point math in the kernel is annoying */
> +	const int scaling_factor = 65536;
> +	int level = a->props.brightness;
> +	int relative_level = level * scaling_factor / a->props.max_brightness;
> +
> +	return relative_level * b->props.max_brightness / scaling_factor;
> +}
> +
>  static int nvidia_wmi_ec_backlight_update_status(struct backlight_device *bd)
>  {
>  	struct wmi_device *wdev = bl_get_data(bd);
> +	struct nvidia_wmi_ec_backlight_priv *priv = dev_get_drvdata(&wdev->dev);
> +	struct backlight_device *proxy_target = priv->proxy_target;
> +
> +	if (proxy_target) {
> +		int level = scale_backlight_level(bd, proxy_target);
> +
> +		if (backlight_device_set_brightness(proxy_target, level))
> +			pr_warn("Failed to relay backlight update to \"%s\"",
> +				backlight_proxy_target);
> +	}
>  
>  	return wmi_brightness_notify(wdev, WMI_BRIGHTNESS_METHOD_LEVEL,
>  	                             WMI_BRIGHTNESS_MODE_SET,
> @@ -147,13 +234,65 @@ static const struct backlight_ops nvidia_wmi_ec_backlight_ops = {
>  	.get_brightness = nvidia_wmi_ec_backlight_get_brightness,
>  };
>  
> +static int nvidia_wmi_ec_backlight_pm_notifier(struct notifier_block *nb, unsigned long event, void *d)
> +{
> +
> +	/*
> +	 * On some systems, the EC backlight level gets reset to 100% when
> +	 * resuming from suspend, but the backlight device state still reflects
> +	 * the pre-suspend value. Refresh the existing state to sync the EC's
> +	 * state back up with the kernel's.
> +	 */
> +	if (event == PM_POST_SUSPEND) {
> +		struct nvidia_wmi_ec_backlight_priv *p;
> +
> +		p = container_of(nb, struct nvidia_wmi_ec_backlight_priv, nb);
> +		return backlight_update_status(p->bl_dev);
> +	}
> +
> +	return 0;
> +}
> +
>  static int nvidia_wmi_ec_backlight_probe(struct wmi_device *wdev, const void *ctx)
>  {
> +	struct backlight_device *bdev, *target = NULL;
> +	struct nvidia_wmi_ec_backlight_priv *priv;
>  	struct backlight_properties props = {};
> -	struct backlight_device *bdev;
>  	u32 source;
>  	int ret;
>  
> +	/*
> +	 * Check quirks tables to see if this system needs any of the firmware
> +	 * bug workarounds.
> +	 */
> +
> +	/* User-set quirks from the module parameters take precedence */
> +	if (!backlight_proxy_target)
> +		dmi_check_system(proxy_quirk_table);
> +
> +	dmi_check_system(restore_quirk_table);
> +
> +	if (backlight_proxy_target && backlight_proxy_target[0]) {
> +		static int num_reprobe_attempts;
> +
> +		target = backlight_device_get_by_name(backlight_proxy_target);
> +
> +		if (!target) {
> +			/*
> +			 * The target backlight device might not be ready;
> +			 * try again and disable backlight proxying if it
> +			 * fails too many times.
> +			 */
> +			if (num_reprobe_attempts < max_reprobe_attempts) {
> +				num_reprobe_attempts++;
> +				return -EPROBE_DEFER;
> +			}
> +
> +			pr_warn("Unable to acquire %s after %d attempts. Disabling backlight proxy.",
> +				backlight_proxy_target, max_reprobe_attempts);
> +		}
> +	}
> +
>  	ret = wmi_brightness_notify(wdev, WMI_BRIGHTNESS_METHOD_SOURCE,
>  	                           WMI_BRIGHTNESS_MODE_GET, &source);
>  	if (ret)
> @@ -188,7 +327,44 @@ static int nvidia_wmi_ec_backlight_probe(struct wmi_device *wdev, const void *ct
>  					      &wdev->dev, wdev,
>  					      &nvidia_wmi_ec_backlight_ops,
>  					      &props);
> -	return PTR_ERR_OR_ZERO(bdev);
> +
> +	if (IS_ERR(bdev))
> +		return PTR_ERR(bdev);
> +
> +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> +	priv->bl_dev = bdev;
> +
> +	dev_set_drvdata(&wdev->dev, priv);
> +
> +	if (target) {
> +		int level = scale_backlight_level(target, bdev);
> +
> +		if (backlight_device_set_brightness(bdev, level))
> +			pr_warn("Unable to import initial brightness level from %s.",
> +				backlight_proxy_target);
> +		priv->proxy_target = target;
> +	}
> +
> +	if (restore_level_on_resume) {
> +		priv->nb.notifier_call = nvidia_wmi_ec_backlight_pm_notifier;
> +		register_pm_notifier(&priv->nb);
> +	}
> +
> +	return 0;
> +}
> +
> +static void nvidia_wmi_ec_backlight_remove(struct wmi_device *wdev)
> +{
> +	struct nvidia_wmi_ec_backlight_priv *priv = dev_get_drvdata(&wdev->dev);
> +	struct backlight_device *proxy_target = priv->proxy_target;
> +
> +	if (proxy_target)
> +		put_device(&proxy_target->dev);
> +
> +	if (priv->nb.notifier_call)
> +		unregister_pm_notifier(&priv->nb);
> +
> +	kfree(priv);
>  }
>  
>  #define WMI_BRIGHTNESS_GUID "603E9613-EF25-4338-A3D0-C46177516DB7"
> @@ -204,6 +380,7 @@ static struct wmi_driver nvidia_wmi_ec_backlight_driver = {
>  		.name = "nvidia-wmi-ec-backlight",
>  	},
>  	.probe = nvidia_wmi_ec_backlight_probe,
> +	.remove = nvidia_wmi_ec_backlight_remove,
>  	.id_table = nvidia_wmi_ec_backlight_id_table,
>  };
>  module_wmi_driver(nvidia_wmi_ec_backlight_driver);

RE: [PATCH] nvidia-wmi-ec-backlight: Add workarounds for confused firmware

[Limonciello, Mario]

[Public]

+ Alex D

Alex, just FYI this was something that came to an AMD bug tracker and wanted you to be aware there are W/A going into nvidia-wmi-ec-backlight for some firmware problems with the mux.
IIRC that was the original suspicion too on the bug reports.

Comments inline as well.

> -----Original Message-----
> From: Daniel Dadap <ddadap@nvidia.com>
> Sent: Wednesday, March 16, 2022 10:11
> To: Barnabás Pőcze <pobrn@protonmail.com>
> Cc: platform-driver-x86@vger.kernel.org; Alexandru Dinu
> <alex.dinu07@gmail.com>; Hans de Goede <hdegoede@redhat.com>;
> markgross@kernel.org
> Subject: Re: [PATCH] nvidia-wmi-ec-backlight: Add workarounds for
> confused firmware
> 
> Thanks for the feedback. I'll send out a v2 shortly: Alex, can you
> please retest when I do to make sure there aren't any regressions? None
> of these suggestions affect the core flow of how either of the
> workarounds work, so I'm not expecting any that wouldn't also reproduce
> on my EC backlight system that doesn't have either of these problems,
> but I can send you the updated version off-list first if you prefer.
> 
> Detailed replies below:
> 
> On 3/15/22 9:50 PM, Barnabás Pőcze wrote:
> > Hi
> >
> >
> > The platform-driver-x86 maintainers should've probably been
> > CCd. You may or may not know, but the `scripts/get_maintainers.pl`
> > script can be used to determine the appropriate recipients.
> 
> 
> Indeed. I've copied the pdx86 maintainers on this message and will for
> future correspondence regarding this patch.
> 
> 
> > 2022. március 16., szerda 2:25 keltezéssel, Daniel Dadap írta:
> >> Some notebook systems with EC-driven backlight control appear to have a
> >> firmware bug which causes the system to use GPU-driven backlight
> control
> >> upon a fresh boot, but then switches to EC-driven backlight control
> >> after completing a suspend/resume cycle. All the while, the firmware
> >> reports that the backlight is under EC control, regardless of what is
> >> actually controlling the backlight brightness.
> >>
> >> This leads to the following behavior:
> >>
> >> * nvidia-wmi-ec-backlight gets probed on a fresh boot, due to the
> >>    WMI-wrapped ACPI method erroneously reporting EC control.
> >> * nvidia-wmi-ec-backlight does not work until after a suspend/resume
> >>    cycle, due to the backlight control actually being GPU-driven.
> >> * GPU drivers also register their own backlight handlers: in the case
> >>    of the notebook system where this behavior has been observed, both
> >>    amdgpu and the NVIDIA proprietary driver register backlight handlers.
> >> * The GPU which has backlight control upon a fresh boot (amdgpu in the
> >>    case observed so far) can successfully control the backlight through
> >>    its backlight driver's sysfs interface, but stops working after the
> >>    first suspend/resume cycle.
> >> * nvidia-wmi-ec-backlight is unable to control the backlight upon a
> >>    fresh boot, but begins to work after the first suspend/resume cycle.
> >> * The GPU which does not have backlight control (NVIDIA in this case)
> >>    is not able to control the backlight at any point while the system
> >>    is in operation. On similar hybrid systems with an EC-controlled
> >>    backlight, and AMD/NVIDIA iGPU/dGPU, the NVIDIA proprietary driver
> >>    does not register its backlight handler. It has not been determined
> >>    whether the non-functional handler registered by the NVIDIA driver
> >>    is due to another firmware bug, or a bug in the NVIDIA driver.
> >>
> >> Since nvidia-wmi-ec-backlight registers as a BACKLIGHT_FIRMWARE type
> >> device, it takes precedence over the BACKLIGHT_RAW devices registered
> >> by the GPU drivers. This in turn leads to backlight control appearing
> >> to be non-functional until after completing a suspend/resume cycle.
> >> However, it is still possible to control the backlight through direct
> >> interaction with the working GPU driver's backlight sysfs interface.
> >>
> >> These systems also appear to have a second firmware bug which resets
> >> the EC's brightness level to 100% on resume, but leaves the state in
> >> the kernel at the pre-suspend level. This causes attempts to save
> >> and restore the backlight level across the suspend/resume cycle to
> >> fail, due to the level appearing not to change even though it did.
> >>
> >> In order to work around these issue, add quirk tables to detect
> >> systems that are known to show these behaviors. So far, there is
> >> only one known system that requires these workarounds, and both
> >> issues are present on that system, but the quirks are tracked in
> >> separate tables to make it easier to add them to other systems which
> >> may exhibit one of the bugs, but not the other. The original systems
> >> that this driver was tested on during development do not exhibit
> >> either of these quirks.
> >>
> >> If a system with the "GPU driver has backlight control" quirk is
> >> detected, nvidia-wmi-ec-backlight will grab a reference to the working
> >> (when freshly booted) GPU backlight handler and relays any backlight
> >> brightness level change requests directed at the EC to also be applied
> >> to the GPU backlight interface. This leads to redundant updates
> >> directed at the GPU backlight driver after a suspend/resume cycle, but
> >> it does allow the EC backlight control to work when the system is
> >> freshly booted.
> >>
> >> If a system with the "backlight level reset to full on resume" quirk
> >> is detected, nvidia-wmi-ec-backlight will register a PM notifier to
> >> reset the backlight to the previous level upon resume.
> >>
> >> These workarounds are also plumbed through to kernel module
> parameters,
> >> to make it easier for users who suspect they may be affected by one or
> >> both of these bugs to test whether these workarounds are effective on
> >> their systems as well.
> >>
> >> Signed-off-by: Daniel Dadap <ddadap@nvidia.com>
> >> Tested-by: Alexandru Dinu <alex.dinu07@gmail.com>
> >> ---
> >>   .../platform/x86/nvidia-wmi-ec-backlight.c    | 181
> +++++++++++++++++-
> >>   1 file changed, 179 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/platform/x86/nvidia-wmi-ec-backlight.c
> b/drivers/platform/x86/nvidia-wmi-ec-backlight.c
> >> index 61e37194df70..ccb3b506c12c 100644
> >> --- a/drivers/platform/x86/nvidia-wmi-ec-backlight.c
> >> +++ b/drivers/platform/x86/nvidia-wmi-ec-backlight.c
> >> @@ -3,8 +3,11 @@
> >>    * Copyright (c) 2020, NVIDIA CORPORATION.  All rights reserved.
> >>    */
> >>
> >> +#define pr_fmt(f) "%s: " f "\n", KBUILD_MODNAME
> > `KBUILD_MODNAME` is a string literal, so you can do e.g.
> >
> >    #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> >
> >
> >> +
> >>   #include <linux/acpi.h>
> >>   #include <linux/backlight.h>
> >> +#include <linux/dmi.h>
> >>   #include <linux/mod_devicetable.h>
> >>   #include <linux/module.h>
> >>   #include <linux/types.h>
> >> @@ -75,6 +78,69 @@ struct wmi_brightness_args {
> >>   	u32 ignored[3];
> >>   };
> >>
> >> +/**
> >> + * struct nvidia_wmi_ec_backlight_priv - driver private data
> >> + * @bl_dev:       the associated backlight device
> >> + * @proxy_target: backlight device which receives relayed brightness
> changes
> >> + * @notifier:     notifier block for resume callback
> >> + */
> >> +struct nvidia_wmi_ec_backlight_priv {
> >> +	struct backlight_device *bl_dev;
> >> +	struct backlight_device *proxy_target;
> >> +	struct notifier_block nb;
> >> +};
> >> +
> >> +static char *backlight_proxy_target;
> >> +module_param(backlight_proxy_target, charp, 0);
> > It seems these module parameters are neither readable nor writable,
> > is that intentional?
> 
> 
> It was intentional that they not be writable, because I didn't want to
> have to plumb everything through to handle changing the values after
> probe. However, you are right that it could still be useful to set up
> the sysfs entries to allow reading the values, as this could be useful
> information for someone who wants to check if either of these quirks are
> enabled.
> 
> 
> >
> >> +MODULE_PARM_DESC(backlight_proxy_target, "Relay brightness
> change requests to the named backlight driver, on systems which
> erroneously report EC backlight control.");
> >> +
> >> +static int max_reprobe_attempts = 128;
> > Can you elaborate how this number was arrived at?
> >
> 
> It's just a medium-small round number. I didn't want probe to return
> -EPROBE_DEFER forever if e.g. somebody specified a wrong device name or
> if the target device name changes and the entry in the quirks table goes
> out of date. On the system I tested this on, the amdgpu_bl1 device was
> accessible on the 14th probe attempt. If there's some better value to
> plug in here, or if it's actually considered more correct to just never
> succeed at probe if the workaround is enabled but the target device can
> be found, I'd be happy to change it.
> 
> 
> >> +module_param(max_reprobe_attempts, int, 0);
> >> +MODULE_PARM_DESC(max_reprobe_attempts, "Limit of reprobe
> attempts when relaying brightness change requests.");
> >> +
> >> +static bool restore_level_on_resume;
> >> +module_param(restore_level_on_resume, bool, 0);
> >> +MODULE_PARM_DESC(restore_level_on_resume, "Restore the
> backlight level when resuming from suspend, on systems which reset the
> EC's backlight level on resume.");
> >> +
> >> +static int assign_relay_quirk(const struct dmi_system_id *id)
> >> +{
> >> +	backlight_proxy_target = id->driver_data;
> >> +	return true;
> >> +}
> >> +
> >> +#define PROXY_QUIRK_ENTRY(vendor, product, quirk_data) { \
> >> +	.callback = assign_relay_quirk,                  \
> >> +	.matches = {                                     \
> >> +		DMI_MATCH(DMI_SYS_VENDOR, vendor),       \
> >> +		DMI_MATCH(DMI_PRODUCT_VERSION, product)  \
> >> +	},                                               \
> >> +	.driver_data = quirk_data                        \
> >> +}
> >> +
> >> +static const struct dmi_system_id proxy_quirk_table[] = {
> >> +	PROXY_QUIRK_ENTRY("LENOVO", "Legion S7 15ACH6",
> "amdgpu_bl1"),
> >> +	{ }
> >> +};
> >> +
> >> +static int assign_restore_quirk(const struct dmi_system_id *id)
> >> +{
> >> +	restore_level_on_resume = true;
> >> +	return true;
> >> +}
> >> +
> >> +#define RESTORE_QUIRK_ENTRY(vendor, product) {           \
> >> +	.callback = assign_restore_quirk,                \
> >> +	.matches = {                                     \
> >> +		DMI_MATCH(DMI_SYS_VENDOR, vendor),       \
> >> +		DMI_MATCH(DMI_PRODUCT_VERSION, product), \
> >> +	}                                                \
> >> +}
> >> +
> >> +static const struct dmi_system_id restore_quirk_table[] = {
> >> +	RESTORE_QUIRK_ENTRY("LENOVO", "Legion S7 15ACH6"),
> >> +	{ }
> >> +};
> >> +
> >>   /**
> >>    * wmi_brightness_notify() - helper function for calling WMI-wrapped
> ACPI method
> >>    * @w:    Pointer to the struct wmi_device identified by
> %WMI_BRIGHTNESS_GUID
> >> @@ -119,9 +185,30 @@ static int wmi_brightness_notify(struct
> wmi_device *w, enum wmi_brightness_metho
> >>   	return 0;
> >>   }
> >>
> >> +static int scale_backlight_level(struct backlight_device *a,
> >> +				 struct backlight_device *b)
> >> +{
> >> +	/* because floating point math in the kernel is annoying */
> >> +	const int scaling_factor = 65536;
> >> +	int level = a->props.brightness;
> >> +	int relative_level = level * scaling_factor / a->props.max_brightness;
> >> +
> >> +	return relative_level * b->props.max_brightness / scaling_factor;
> >> +}
> > Maybe
> >
> >    fixp_linear_interpolate(0, 0, a->props.max_brightness, b-
> >props.max_brightness, a->props.brightness);
> >
> > ? (from `linux/fixp-arith.h`)
> 
> 
> Yes, this is exactly what I want; thank you.
> 
> 
> >
> >> +
> >>   static int nvidia_wmi_ec_backlight_update_status(struct
> backlight_device *bd)
> >>   {
> >>   	struct wmi_device *wdev = bl_get_data(bd);
> >> +	struct nvidia_wmi_ec_backlight_priv *priv =
> dev_get_drvdata(&wdev->dev);
> >> +	struct backlight_device *proxy_target = priv->proxy_target;
> >> +
> >> +	if (proxy_target) {
> >> +		int level = scale_backlight_level(bd, proxy_target);
> >> +
> >> +		if (backlight_device_set_brightness(proxy_target, level))
> >> +			pr_warn("Failed to relay backlight update to \"%s\"",
> >> +				backlight_proxy_target);
> >> +	}
> >>
> >>   	return wmi_brightness_notify(wdev,
> WMI_BRIGHTNESS_METHOD_LEVEL,
> >>   	                             WMI_BRIGHTNESS_MODE_SET,
> >> @@ -147,13 +234,65 @@ static const struct backlight_ops
> nvidia_wmi_ec_backlight_ops = {
> >>   	.get_brightness = nvidia_wmi_ec_backlight_get_brightness,
> >>   };
> >>
> >> +static int nvidia_wmi_ec_backlight_pm_notifier(struct notifier_block
> *nb, unsigned long event, void *d)
> >> +{
> >> +
> >> +	/*
> >> +	 * On some systems, the EC backlight level gets reset to 100% when
> >> +	 * resuming from suspend, but the backlight device state still reflects
> >> +	 * the pre-suspend value. Refresh the existing state to sync the EC's
> >> +	 * state back up with the kernel's.
> >> +	 */
> >> +	if (event == PM_POST_SUSPEND) {
> >> +		struct nvidia_wmi_ec_backlight_priv *p;
> >> +
> >> +		p = container_of(nb, struct nvidia_wmi_ec_backlight_priv,
> nb);
> >> +		return backlight_update_status(p->bl_dev);
> > `backlight_update_status()` returns a negative errno while the notifier
> chain
> > expects something else. It would probably be better to return
> `NOTIFY_DONE`
> > in all cases. Currently a suitable error from `backlight_update_status()` will
> > stop the notifier chain.
> 
> 
> Thanks for catching that: I should have paid more attention to the
> notifier callback signature.
> 
> 
> >
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >>   static int nvidia_wmi_ec_backlight_probe(struct wmi_device *wdev,
> const void *ctx)
> >>   {
> >> +	struct backlight_device *bdev, *target = NULL;
> >> +	struct nvidia_wmi_ec_backlight_priv *priv;
> >>   	struct backlight_properties props = {};
> >> -	struct backlight_device *bdev;
> >>   	u32 source;
> >>   	int ret;
> >>
> >> +	/*
> >> +	 * Check quirks tables to see if this system needs any of the firmware
> >> +	 * bug workarounds.
> >> +	 */
> >> +
> >> +	/* User-set quirks from the module parameters take precedence */
> >> +	if (!backlight_proxy_target)
> >> +		dmi_check_system(proxy_quirk_table);
> >> +
> >> +	dmi_check_system(restore_quirk_table);
> >> +
> >> +	if (backlight_proxy_target && backlight_proxy_target[0]) {
> >> +		static int num_reprobe_attempts;
> >> +
> >> +		target =
> backlight_device_get_by_name(backlight_proxy_target);
> >> +
> >> +		if (!target) {
> >> +			/*
> >> +			 * The target backlight device might not be ready;
> >> +			 * try again and disable backlight proxying if it
> >> +			 * fails too many times.
> >> +			 */
> >> +			if (num_reprobe_attempts <
> max_reprobe_attempts) {
> >> +				num_reprobe_attempts++;
> >> +				return -EPROBE_DEFER;
> >> +			}
> >> +
> >> +			pr_warn("Unable to acquire %s after %d attempts.
> Disabling backlight proxy.",
> >> +				backlight_proxy_target,
> max_reprobe_attempts);
> >> +		}
> >> +	}
> > I think `target` is not put in case of error. You probably need to add
> something like:
> >
> >    if (target) {
> >      ret = devm_add_action_or_reset(&wdev->dev, put_device_wrapper,
> target);
> >      if (ret < 0)
> >        return ret;
> >    }
> >
> >
> >> +
> >>   	ret = wmi_brightness_notify(wdev,
> WMI_BRIGHTNESS_METHOD_SOURCE,
> >>   	                           WMI_BRIGHTNESS_MODE_GET, &source);
> >>   	if (ret)
> >> @@ -188,7 +327,44 @@ static int nvidia_wmi_ec_backlight_probe(struct
> wmi_device *wdev, const void *ct
> >>   					      &wdev->dev, wdev,
> >>   					      &nvidia_wmi_ec_backlight_ops,
> >>   					      &props);
> >> -	return PTR_ERR_OR_ZERO(bdev);
> >> +
> >> +	if (IS_ERR(bdev))
> >> +		return PTR_ERR(bdev);
> >> +
> >> +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> > `devm_kzalloc()` would probably be better and you should check if `!priv`.
> >
> >
> >> +	priv->bl_dev = bdev;
> >> +
> >> +	dev_set_drvdata(&wdev->dev, priv);
> >> +
> >> +	if (target) {
> >> +		int level = scale_backlight_level(target, bdev);
> >> +
> >> +		if (backlight_device_set_brightness(bdev, level))
> >> +			pr_warn("Unable to import initial brightness level
> from %s.",
> >> +				backlight_proxy_target);
> >> +		priv->proxy_target = target;
> >> +	}
> >> +
> >> +	if (restore_level_on_resume) {
> >> +		priv->nb.notifier_call =
> nvidia_wmi_ec_backlight_pm_notifier;
> >> +		register_pm_notifier(&priv->nb);
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static void nvidia_wmi_ec_backlight_remove(struct wmi_device *wdev)
> >> +{
> >> +	struct nvidia_wmi_ec_backlight_priv *priv =
> dev_get_drvdata(&wdev->dev);
> >> +	struct backlight_device *proxy_target = priv->proxy_target;
> >> +
> >> +	if (proxy_target)
> >> +		put_device(&proxy_target->dev);
> > If you switch to `devm_add_action_or_reset()`, this will not be needed.
> >
> >
> >> +
> >> +	if (priv->nb.notifier_call)
> >> +		unregister_pm_notifier(&priv->nb);
> >> +
> >> +	kfree(priv);
> > If you switch to `devm_kzalloc()`, this won't be needed.
> 
> 
> Thank you, the devm_*() variants are indeed useful.
> 
> 
> >
> >>   }
> >>
> >>   #define WMI_BRIGHTNESS_GUID "603E9613-EF25-4338-A3D0-
> C46177516DB7"
> >> @@ -204,6 +380,7 @@ static struct wmi_driver
> nvidia_wmi_ec_backlight_driver = {
> >>   		.name = "nvidia-wmi-ec-backlight",
> >>   	},
> >>   	.probe = nvidia_wmi_ec_backlight_probe,
> >> +	.remove = nvidia_wmi_ec_backlight_remove,
> >>   	.id_table = nvidia_wmi_ec_backlight_id_table,
> >>   };
> >>   module_wmi_driver(nvidia_wmi_ec_backlight_driver);
> >> --
> >> 2.27.0
> >>
> > Lastly, is it expected that these bugs will be properly fixed?
> 
> 
> Possibly, but I wouldn't hold out hope for it for an issue at this scale
> on an already shipping system.

This question I'm assuming was aimed at narrowing the quirk to only
match certain FW versions or so.  If there is no certainty of when/if it
will be fixed I agree with current direction.
However I think it's still worth at least noting near the quirk in a comment
what firmware version it was identified.  If later there is confirmation that
a particular firmware version had fixed it the quirk can be adjusted to be
dropped.

> 
> 
> >
> > Regards,
> > Barnabás Pőcze

Re: [PATCH] nvidia-wmi-ec-backlight: Add workarounds for confused firmware

[Daniel Dadap]

Thanks for the feedback. I'll send out a v2 shortly: Alex, can you 
please retest when I do to make sure there aren't any regressions? None 
of these suggestions affect the core flow of how either of the 
workarounds work, so I'm not expecting any that wouldn't also reproduce 
on my EC backlight system that doesn't have either of these problems, 
but I can send you the updated version off-list first if you prefer.

Detailed replies below:

On 3/15/22 9:50 PM, Barnabás Pőcze wrote:
> Hi
>
>
> The platform-driver-x86 maintainers should've probably been
> CCd. You may or may not know, but the `scripts/get_maintainers.pl`
> script can be used to determine the appropriate recipients.


Indeed. I've copied the pdx86 maintainers on this message and will for 
future correspondence regarding this patch.


> 2022. március 16., szerda 2:25 keltezéssel, Daniel Dadap írta:
>> Some notebook systems with EC-driven backlight control appear to have a
>> firmware bug which causes the system to use GPU-driven backlight control
>> upon a fresh boot, but then switches to EC-driven backlight control
>> after completing a suspend/resume cycle. All the while, the firmware
>> reports that the backlight is under EC control, regardless of what is
>> actually controlling the backlight brightness.
>>
>> This leads to the following behavior:
>>
>> * nvidia-wmi-ec-backlight gets probed on a fresh boot, due to the
>>    WMI-wrapped ACPI method erroneously reporting EC control.
>> * nvidia-wmi-ec-backlight does not work until after a suspend/resume
>>    cycle, due to the backlight control actually being GPU-driven.
>> * GPU drivers also register their own backlight handlers: in the case
>>    of the notebook system where this behavior has been observed, both
>>    amdgpu and the NVIDIA proprietary driver register backlight handlers.
>> * The GPU which has backlight control upon a fresh boot (amdgpu in the
>>    case observed so far) can successfully control the backlight through
>>    its backlight driver's sysfs interface, but stops working after the
>>    first suspend/resume cycle.
>> * nvidia-wmi-ec-backlight is unable to control the backlight upon a
>>    fresh boot, but begins to work after the first suspend/resume cycle.
>> * The GPU which does not have backlight control (NVIDIA in this case)
>>    is not able to control the backlight at any point while the system
>>    is in operation. On similar hybrid systems with an EC-controlled
>>    backlight, and AMD/NVIDIA iGPU/dGPU, the NVIDIA proprietary driver
>>    does not register its backlight handler. It has not been determined
>>    whether the non-functional handler registered by the NVIDIA driver
>>    is due to another firmware bug, or a bug in the NVIDIA driver.
>>
>> Since nvidia-wmi-ec-backlight registers as a BACKLIGHT_FIRMWARE type
>> device, it takes precedence over the BACKLIGHT_RAW devices registered
>> by the GPU drivers. This in turn leads to backlight control appearing
>> to be non-functional until after completing a suspend/resume cycle.
>> However, it is still possible to control the backlight through direct
>> interaction with the working GPU driver's backlight sysfs interface.
>>
>> These systems also appear to have a second firmware bug which resets
>> the EC's brightness level to 100% on resume, but leaves the state in
>> the kernel at the pre-suspend level. This causes attempts to save
>> and restore the backlight level across the suspend/resume cycle to
>> fail, due to the level appearing not to change even though it did.
>>
>> In order to work around these issue, add quirk tables to detect
>> systems that are known to show these behaviors. So far, there is
>> only one known system that requires these workarounds, and both
>> issues are present on that system, but the quirks are tracked in
>> separate tables to make it easier to add them to other systems which
>> may exhibit one of the bugs, but not the other. The original systems
>> that this driver was tested on during development do not exhibit
>> either of these quirks.
>>
>> If a system with the "GPU driver has backlight control" quirk is
>> detected, nvidia-wmi-ec-backlight will grab a reference to the working
>> (when freshly booted) GPU backlight handler and relays any backlight
>> brightness level change requests directed at the EC to also be applied
>> to the GPU backlight interface. This leads to redundant updates
>> directed at the GPU backlight driver after a suspend/resume cycle, but
>> it does allow the EC backlight control to work when the system is
>> freshly booted.
>>
>> If a system with the "backlight level reset to full on resume" quirk
>> is detected, nvidia-wmi-ec-backlight will register a PM notifier to
>> reset the backlight to the previous level upon resume.
>>
>> These workarounds are also plumbed through to kernel module parameters,
>> to make it easier for users who suspect they may be affected by one or
>> both of these bugs to test whether these workarounds are effective on
>> their systems as well.
>>
>> Signed-off-by: Daniel Dadap <ddadap@nvidia.com>
>> Tested-by: Alexandru Dinu <alex.dinu07@gmail.com>
>> ---
>>   .../platform/x86/nvidia-wmi-ec-backlight.c    | 181 +++++++++++++++++-
>>   1 file changed, 179 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/platform/x86/nvidia-wmi-ec-backlight.c b/drivers/platform/x86/nvidia-wmi-ec-backlight.c
>> index 61e37194df70..ccb3b506c12c 100644
>> --- a/drivers/platform/x86/nvidia-wmi-ec-backlight.c
>> +++ b/drivers/platform/x86/nvidia-wmi-ec-backlight.c
>> @@ -3,8 +3,11 @@
>>    * Copyright (c) 2020, NVIDIA CORPORATION.  All rights reserved.
>>    */
>>
>> +#define pr_fmt(f) "%s: " f "\n", KBUILD_MODNAME
> `KBUILD_MODNAME` is a string literal, so you can do e.g.
>
>    #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>
>
>> +
>>   #include <linux/acpi.h>
>>   #include <linux/backlight.h>
>> +#include <linux/dmi.h>
>>   #include <linux/mod_devicetable.h>
>>   #include <linux/module.h>
>>   #include <linux/types.h>
>> @@ -75,6 +78,69 @@ struct wmi_brightness_args {
>>   	u32 ignored[3];
>>   };
>>
>> +/**
>> + * struct nvidia_wmi_ec_backlight_priv - driver private data
>> + * @bl_dev:       the associated backlight device
>> + * @proxy_target: backlight device which receives relayed brightness changes
>> + * @notifier:     notifier block for resume callback
>> + */
>> +struct nvidia_wmi_ec_backlight_priv {
>> +	struct backlight_device *bl_dev;
>> +	struct backlight_device *proxy_target;
>> +	struct notifier_block nb;
>> +};
>> +
>> +static char *backlight_proxy_target;
>> +module_param(backlight_proxy_target, charp, 0);
> It seems these module parameters are neither readable nor writable,
> is that intentional?


It was intentional that they not be writable, because I didn't want to 
have to plumb everything through to handle changing the values after 
probe. However, you are right that it could still be useful to set up 
the sysfs entries to allow reading the values, as this could be useful 
information for someone who wants to check if either of these quirks are 
enabled.


>
>> +MODULE_PARM_DESC(backlight_proxy_target, "Relay brightness change requests to the named backlight driver, on systems which erroneously report EC backlight control.");
>> +
>> +static int max_reprobe_attempts = 128;
> Can you elaborate how this number was arrived at?
>

It's just a medium-small round number. I didn't want probe to return 
-EPROBE_DEFER forever if e.g. somebody specified a wrong device name or 
if the target device name changes and the entry in the quirks table goes 
out of date. On the system I tested this on, the amdgpu_bl1 device was 
accessible on the 14th probe attempt. If there's some better value to 
plug in here, or if it's actually considered more correct to just never 
succeed at probe if the workaround is enabled but the target device can 
be found, I'd be happy to change it.


>> +module_param(max_reprobe_attempts, int, 0);
>> +MODULE_PARM_DESC(max_reprobe_attempts, "Limit of reprobe attempts when relaying brightness change requests.");
>> +
>> +static bool restore_level_on_resume;
>> +module_param(restore_level_on_resume, bool, 0);
>> +MODULE_PARM_DESC(restore_level_on_resume, "Restore the backlight level when resuming from suspend, on systems which reset the EC's backlight level on resume.");
>> +
>> +static int assign_relay_quirk(const struct dmi_system_id *id)
>> +{
>> +	backlight_proxy_target = id->driver_data;
>> +	return true;
>> +}
>> +
>> +#define PROXY_QUIRK_ENTRY(vendor, product, quirk_data) { \
>> +	.callback = assign_relay_quirk,                  \
>> +	.matches = {                                     \
>> +		DMI_MATCH(DMI_SYS_VENDOR, vendor),       \
>> +		DMI_MATCH(DMI_PRODUCT_VERSION, product)  \
>> +	},                                               \
>> +	.driver_data = quirk_data                        \
>> +}
>> +
>> +static const struct dmi_system_id proxy_quirk_table[] = {
>> +	PROXY_QUIRK_ENTRY("LENOVO", "Legion S7 15ACH6", "amdgpu_bl1"),
>> +	{ }
>> +};
>> +
>> +static int assign_restore_quirk(const struct dmi_system_id *id)
>> +{
>> +	restore_level_on_resume = true;
>> +	return true;
>> +}
>> +
>> +#define RESTORE_QUIRK_ENTRY(vendor, product) {           \
>> +	.callback = assign_restore_quirk,                \
>> +	.matches = {                                     \
>> +		DMI_MATCH(DMI_SYS_VENDOR, vendor),       \
>> +		DMI_MATCH(DMI_PRODUCT_VERSION, product), \
>> +	}                                                \
>> +}
>> +
>> +static const struct dmi_system_id restore_quirk_table[] = {
>> +	RESTORE_QUIRK_ENTRY("LENOVO", "Legion S7 15ACH6"),
>> +	{ }
>> +};
>> +
>>   /**
>>    * wmi_brightness_notify() - helper function for calling WMI-wrapped ACPI method
>>    * @w:    Pointer to the struct wmi_device identified by %WMI_BRIGHTNESS_GUID
>> @@ -119,9 +185,30 @@ static int wmi_brightness_notify(struct wmi_device *w, enum wmi_brightness_metho
>>   	return 0;
>>   }
>>
>> +static int scale_backlight_level(struct backlight_device *a,
>> +				 struct backlight_device *b)
>> +{
>> +	/* because floating point math in the kernel is annoying */
>> +	const int scaling_factor = 65536;
>> +	int level = a->props.brightness;
>> +	int relative_level = level * scaling_factor / a->props.max_brightness;
>> +
>> +	return relative_level * b->props.max_brightness / scaling_factor;
>> +}
> Maybe
>
>    fixp_linear_interpolate(0, 0, a->props.max_brightness, b->props.max_brightness, a->props.brightness);
>
> ? (from `linux/fixp-arith.h`)


Yes, this is exactly what I want; thank you.


>
>> +
>>   static int nvidia_wmi_ec_backlight_update_status(struct backlight_device *bd)
>>   {
>>   	struct wmi_device *wdev = bl_get_data(bd);
>> +	struct nvidia_wmi_ec_backlight_priv *priv = dev_get_drvdata(&wdev->dev);
>> +	struct backlight_device *proxy_target = priv->proxy_target;
>> +
>> +	if (proxy_target) {
>> +		int level = scale_backlight_level(bd, proxy_target);
>> +
>> +		if (backlight_device_set_brightness(proxy_target, level))
>> +			pr_warn("Failed to relay backlight update to \"%s\"",
>> +				backlight_proxy_target);
>> +	}
>>
>>   	return wmi_brightness_notify(wdev, WMI_BRIGHTNESS_METHOD_LEVEL,
>>   	                             WMI_BRIGHTNESS_MODE_SET,
>> @@ -147,13 +234,65 @@ static const struct backlight_ops nvidia_wmi_ec_backlight_ops = {
>>   	.get_brightness = nvidia_wmi_ec_backlight_get_brightness,
>>   };
>>
>> +static int nvidia_wmi_ec_backlight_pm_notifier(struct notifier_block *nb, unsigned long event, void *d)
>> +{
>> +
>> +	/*
>> +	 * On some systems, the EC backlight level gets reset to 100% when
>> +	 * resuming from suspend, but the backlight device state still reflects
>> +	 * the pre-suspend value. Refresh the existing state to sync the EC's
>> +	 * state back up with the kernel's.
>> +	 */
>> +	if (event == PM_POST_SUSPEND) {
>> +		struct nvidia_wmi_ec_backlight_priv *p;
>> +
>> +		p = container_of(nb, struct nvidia_wmi_ec_backlight_priv, nb);
>> +		return backlight_update_status(p->bl_dev);
> `backlight_update_status()` returns a negative errno while the notifier chain
> expects something else. It would probably be better to return `NOTIFY_DONE`
> in all cases. Currently a suitable error from `backlight_update_status()` will
> stop the notifier chain.


Thanks for catching that: I should have paid more attention to the 
notifier callback signature.


>
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>   static int nvidia_wmi_ec_backlight_probe(struct wmi_device *wdev, const void *ctx)
>>   {
>> +	struct backlight_device *bdev, *target = NULL;
>> +	struct nvidia_wmi_ec_backlight_priv *priv;
>>   	struct backlight_properties props = {};
>> -	struct backlight_device *bdev;
>>   	u32 source;
>>   	int ret;
>>
>> +	/*
>> +	 * Check quirks tables to see if this system needs any of the firmware
>> +	 * bug workarounds.
>> +	 */
>> +
>> +	/* User-set quirks from the module parameters take precedence */
>> +	if (!backlight_proxy_target)
>> +		dmi_check_system(proxy_quirk_table);
>> +
>> +	dmi_check_system(restore_quirk_table);
>> +
>> +	if (backlight_proxy_target && backlight_proxy_target[0]) {
>> +		static int num_reprobe_attempts;
>> +
>> +		target = backlight_device_get_by_name(backlight_proxy_target);
>> +
>> +		if (!target) {
>> +			/*
>> +			 * The target backlight device might not be ready;
>> +			 * try again and disable backlight proxying if it
>> +			 * fails too many times.
>> +			 */
>> +			if (num_reprobe_attempts < max_reprobe_attempts) {
>> +				num_reprobe_attempts++;
>> +				return -EPROBE_DEFER;
>> +			}
>> +
>> +			pr_warn("Unable to acquire %s after %d attempts. Disabling backlight proxy.",
>> +				backlight_proxy_target, max_reprobe_attempts);
>> +		}
>> +	}
> I think `target` is not put in case of error. You probably need to add something like:
>
>    if (target) {
>      ret = devm_add_action_or_reset(&wdev->dev, put_device_wrapper, target);
>      if (ret < 0)
>        return ret;
>    }
>
>
>> +
>>   	ret = wmi_brightness_notify(wdev, WMI_BRIGHTNESS_METHOD_SOURCE,
>>   	                           WMI_BRIGHTNESS_MODE_GET, &source);
>>   	if (ret)
>> @@ -188,7 +327,44 @@ static int nvidia_wmi_ec_backlight_probe(struct wmi_device *wdev, const void *ct
>>   					      &wdev->dev, wdev,
>>   					      &nvidia_wmi_ec_backlight_ops,
>>   					      &props);
>> -	return PTR_ERR_OR_ZERO(bdev);
>> +
>> +	if (IS_ERR(bdev))
>> +		return PTR_ERR(bdev);
>> +
>> +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> `devm_kzalloc()` would probably be better and you should check if `!priv`.
>
>
>> +	priv->bl_dev = bdev;
>> +
>> +	dev_set_drvdata(&wdev->dev, priv);
>> +
>> +	if (target) {
>> +		int level = scale_backlight_level(target, bdev);
>> +
>> +		if (backlight_device_set_brightness(bdev, level))
>> +			pr_warn("Unable to import initial brightness level from %s.",
>> +				backlight_proxy_target);
>> +		priv->proxy_target = target;
>> +	}
>> +
>> +	if (restore_level_on_resume) {
>> +		priv->nb.notifier_call = nvidia_wmi_ec_backlight_pm_notifier;
>> +		register_pm_notifier(&priv->nb);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void nvidia_wmi_ec_backlight_remove(struct wmi_device *wdev)
>> +{
>> +	struct nvidia_wmi_ec_backlight_priv *priv = dev_get_drvdata(&wdev->dev);
>> +	struct backlight_device *proxy_target = priv->proxy_target;
>> +
>> +	if (proxy_target)
>> +		put_device(&proxy_target->dev);
> If you switch to `devm_add_action_or_reset()`, this will not be needed.
>
>
>> +
>> +	if (priv->nb.notifier_call)
>> +		unregister_pm_notifier(&priv->nb);
>> +
>> +	kfree(priv);
> If you switch to `devm_kzalloc()`, this won't be needed.


Thank you, the devm_*() variants are indeed useful.


>
>>   }
>>
>>   #define WMI_BRIGHTNESS_GUID "603E9613-EF25-4338-A3D0-C46177516DB7"
>> @@ -204,6 +380,7 @@ static struct wmi_driver nvidia_wmi_ec_backlight_driver = {
>>   		.name = "nvidia-wmi-ec-backlight",
>>   	},
>>   	.probe = nvidia_wmi_ec_backlight_probe,
>> +	.remove = nvidia_wmi_ec_backlight_remove,
>>   	.id_table = nvidia_wmi_ec_backlight_id_table,
>>   };
>>   module_wmi_driver(nvidia_wmi_ec_backlight_driver);
>> --
>> 2.27.0
>>
> Lastly, is it expected that these bugs will be properly fixed?


Possibly, but I wouldn't hold out hope for it for an issue at this scale 
on an already shipping system.


>
> Regards,
> Barnabás Pőcze

Re: [PATCH] nvidia-wmi-ec-backlight: Add workarounds for confused firmware

[Barnabás Pőcze]

Hi


The platform-driver-x86 maintainers should've probably been
CCd. You may or may not know, but the `scripts/get_maintainers.pl`
script can be used to determine the appropriate recipients.

2022. március 16., szerda 2:25 keltezéssel, Daniel Dadap írta:
> Some notebook systems with EC-driven backlight control appear to have a
> firmware bug which causes the system to use GPU-driven backlight control
> upon a fresh boot, but then switches to EC-driven backlight control
> after completing a suspend/resume cycle. All the while, the firmware
> reports that the backlight is under EC control, regardless of what is
> actually controlling the backlight brightness.
>
> This leads to the following behavior:
>
> * nvidia-wmi-ec-backlight gets probed on a fresh boot, due to the
>   WMI-wrapped ACPI method erroneously reporting EC control.
> * nvidia-wmi-ec-backlight does not work until after a suspend/resume
>   cycle, due to the backlight control actually being GPU-driven.
> * GPU drivers also register their own backlight handlers: in the case
>   of the notebook system where this behavior has been observed, both
>   amdgpu and the NVIDIA proprietary driver register backlight handlers.
> * The GPU which has backlight control upon a fresh boot (amdgpu in the
>   case observed so far) can successfully control the backlight through
>   its backlight driver's sysfs interface, but stops working after the
>   first suspend/resume cycle.
> * nvidia-wmi-ec-backlight is unable to control the backlight upon a
>   fresh boot, but begins to work after the first suspend/resume cycle.
> * The GPU which does not have backlight control (NVIDIA in this case)
>   is not able to control the backlight at any point while the system
>   is in operation. On similar hybrid systems with an EC-controlled
>   backlight, and AMD/NVIDIA iGPU/dGPU, the NVIDIA proprietary driver
>   does not register its backlight handler. It has not been determined
>   whether the non-functional handler registered by the NVIDIA driver
>   is due to another firmware bug, or a bug in the NVIDIA driver.
>
> Since nvidia-wmi-ec-backlight registers as a BACKLIGHT_FIRMWARE type
> device, it takes precedence over the BACKLIGHT_RAW devices registered
> by the GPU drivers. This in turn leads to backlight control appearing
> to be non-functional until after completing a suspend/resume cycle.
> However, it is still possible to control the backlight through direct
> interaction with the working GPU driver's backlight sysfs interface.
>
> These systems also appear to have a second firmware bug which resets
> the EC's brightness level to 100% on resume, but leaves the state in
> the kernel at the pre-suspend level. This causes attempts to save
> and restore the backlight level across the suspend/resume cycle to
> fail, due to the level appearing not to change even though it did.
>
> In order to work around these issue, add quirk tables to detect
> systems that are known to show these behaviors. So far, there is
> only one known system that requires these workarounds, and both
> issues are present on that system, but the quirks are tracked in
> separate tables to make it easier to add them to other systems which
> may exhibit one of the bugs, but not the other. The original systems
> that this driver was tested on during development do not exhibit
> either of these quirks.
>
> If a system with the "GPU driver has backlight control" quirk is
> detected, nvidia-wmi-ec-backlight will grab a reference to the working
> (when freshly booted) GPU backlight handler and relays any backlight
> brightness level change requests directed at the EC to also be applied
> to the GPU backlight interface. This leads to redundant updates
> directed at the GPU backlight driver after a suspend/resume cycle, but
> it does allow the EC backlight control to work when the system is
> freshly booted.
>
> If a system with the "backlight level reset to full on resume" quirk
> is detected, nvidia-wmi-ec-backlight will register a PM notifier to
> reset the backlight to the previous level upon resume.
>
> These workarounds are also plumbed through to kernel module parameters,
> to make it easier for users who suspect they may be affected by one or
> both of these bugs to test whether these workarounds are effective on
> their systems as well.
>
> Signed-off-by: Daniel Dadap <ddadap@nvidia.com>
> Tested-by: Alexandru Dinu <alex.dinu07@gmail.com>
> ---
>  .../platform/x86/nvidia-wmi-ec-backlight.c    | 181 +++++++++++++++++-
>  1 file changed, 179 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/platform/x86/nvidia-wmi-ec-backlight.c b/drivers/platform/x86/nvidia-wmi-ec-backlight.c
> index 61e37194df70..ccb3b506c12c 100644
> --- a/drivers/platform/x86/nvidia-wmi-ec-backlight.c
> +++ b/drivers/platform/x86/nvidia-wmi-ec-backlight.c
> @@ -3,8 +3,11 @@
>   * Copyright (c) 2020, NVIDIA CORPORATION.  All rights reserved.
>   */
>
> +#define pr_fmt(f) "%s: " f "\n", KBUILD_MODNAME

`KBUILD_MODNAME` is a string literal, so you can do e.g.

  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt


> +
>  #include <linux/acpi.h>
>  #include <linux/backlight.h>
> +#include <linux/dmi.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/module.h>
>  #include <linux/types.h>
> @@ -75,6 +78,69 @@ struct wmi_brightness_args {
>  	u32 ignored[3];
>  };
>
> +/**
> + * struct nvidia_wmi_ec_backlight_priv - driver private data
> + * @bl_dev:       the associated backlight device
> + * @proxy_target: backlight device which receives relayed brightness changes
> + * @notifier:     notifier block for resume callback
> + */
> +struct nvidia_wmi_ec_backlight_priv {
> +	struct backlight_device *bl_dev;
> +	struct backlight_device *proxy_target;
> +	struct notifier_block nb;
> +};
> +
> +static char *backlight_proxy_target;
> +module_param(backlight_proxy_target, charp, 0);

It seems these module parameters are neither readable nor writable,
is that intentional?


> +MODULE_PARM_DESC(backlight_proxy_target, "Relay brightness change requests to the named backlight driver, on systems which erroneously report EC backlight control.");
> +
> +static int max_reprobe_attempts = 128;

Can you elaborate how this number was arrived at?


> +module_param(max_reprobe_attempts, int, 0);
> +MODULE_PARM_DESC(max_reprobe_attempts, "Limit of reprobe attempts when relaying brightness change requests.");
> +
> +static bool restore_level_on_resume;
> +module_param(restore_level_on_resume, bool, 0);
> +MODULE_PARM_DESC(restore_level_on_resume, "Restore the backlight level when resuming from suspend, on systems which reset the EC's backlight level on resume.");
> +
> +static int assign_relay_quirk(const struct dmi_system_id *id)
> +{
> +	backlight_proxy_target = id->driver_data;
> +	return true;
> +}
> +
> +#define PROXY_QUIRK_ENTRY(vendor, product, quirk_data) { \
> +	.callback = assign_relay_quirk,                  \
> +	.matches = {                                     \
> +		DMI_MATCH(DMI_SYS_VENDOR, vendor),       \
> +		DMI_MATCH(DMI_PRODUCT_VERSION, product)  \
> +	},                                               \
> +	.driver_data = quirk_data                        \
> +}
> +
> +static const struct dmi_system_id proxy_quirk_table[] = {
> +	PROXY_QUIRK_ENTRY("LENOVO", "Legion S7 15ACH6", "amdgpu_bl1"),
> +	{ }
> +};
> +
> +static int assign_restore_quirk(const struct dmi_system_id *id)
> +{
> +	restore_level_on_resume = true;
> +	return true;
> +}
> +
> +#define RESTORE_QUIRK_ENTRY(vendor, product) {           \
> +	.callback = assign_restore_quirk,                \
> +	.matches = {                                     \
> +		DMI_MATCH(DMI_SYS_VENDOR, vendor),       \
> +		DMI_MATCH(DMI_PRODUCT_VERSION, product), \
> +	}                                                \
> +}
> +
> +static const struct dmi_system_id restore_quirk_table[] = {
> +	RESTORE_QUIRK_ENTRY("LENOVO", "Legion S7 15ACH6"),
> +	{ }
> +};
> +
>  /**
>   * wmi_brightness_notify() - helper function for calling WMI-wrapped ACPI method
>   * @w:    Pointer to the struct wmi_device identified by %WMI_BRIGHTNESS_GUID
> @@ -119,9 +185,30 @@ static int wmi_brightness_notify(struct wmi_device *w, enum wmi_brightness_metho
>  	return 0;
>  }
>
> +static int scale_backlight_level(struct backlight_device *a,
> +				 struct backlight_device *b)
> +{
> +	/* because floating point math in the kernel is annoying */
> +	const int scaling_factor = 65536;
> +	int level = a->props.brightness;
> +	int relative_level = level * scaling_factor / a->props.max_brightness;
> +
> +	return relative_level * b->props.max_brightness / scaling_factor;
> +}

Maybe

  fixp_linear_interpolate(0, 0, a->props.max_brightness, b->props.max_brightness, a->props.brightness);

? (from `linux/fixp-arith.h`)


> +
>  static int nvidia_wmi_ec_backlight_update_status(struct backlight_device *bd)
>  {
>  	struct wmi_device *wdev = bl_get_data(bd);
> +	struct nvidia_wmi_ec_backlight_priv *priv = dev_get_drvdata(&wdev->dev);
> +	struct backlight_device *proxy_target = priv->proxy_target;
> +
> +	if (proxy_target) {
> +		int level = scale_backlight_level(bd, proxy_target);
> +
> +		if (backlight_device_set_brightness(proxy_target, level))
> +			pr_warn("Failed to relay backlight update to \"%s\"",
> +				backlight_proxy_target);
> +	}
>
>  	return wmi_brightness_notify(wdev, WMI_BRIGHTNESS_METHOD_LEVEL,
>  	                             WMI_BRIGHTNESS_MODE_SET,
> @@ -147,13 +234,65 @@ static const struct backlight_ops nvidia_wmi_ec_backlight_ops = {
>  	.get_brightness = nvidia_wmi_ec_backlight_get_brightness,
>  };
>
> +static int nvidia_wmi_ec_backlight_pm_notifier(struct notifier_block *nb, unsigned long event, void *d)
> +{
> +
> +	/*
> +	 * On some systems, the EC backlight level gets reset to 100% when
> +	 * resuming from suspend, but the backlight device state still reflects
> +	 * the pre-suspend value. Refresh the existing state to sync the EC's
> +	 * state back up with the kernel's.
> +	 */
> +	if (event == PM_POST_SUSPEND) {
> +		struct nvidia_wmi_ec_backlight_priv *p;
> +
> +		p = container_of(nb, struct nvidia_wmi_ec_backlight_priv, nb);
> +		return backlight_update_status(p->bl_dev);

`backlight_update_status()` returns a negative errno while the notifier chain
expects something else. It would probably be better to return `NOTIFY_DONE`
in all cases. Currently a suitable error from `backlight_update_status()` will
stop the notifier chain.


> +	}
> +
> +	return 0;
> +}
> +
>  static int nvidia_wmi_ec_backlight_probe(struct wmi_device *wdev, const void *ctx)
>  {
> +	struct backlight_device *bdev, *target = NULL;
> +	struct nvidia_wmi_ec_backlight_priv *priv;
>  	struct backlight_properties props = {};
> -	struct backlight_device *bdev;
>  	u32 source;
>  	int ret;
>
> +	/*
> +	 * Check quirks tables to see if this system needs any of the firmware
> +	 * bug workarounds.
> +	 */
> +
> +	/* User-set quirks from the module parameters take precedence */
> +	if (!backlight_proxy_target)
> +		dmi_check_system(proxy_quirk_table);
> +
> +	dmi_check_system(restore_quirk_table);
> +
> +	if (backlight_proxy_target && backlight_proxy_target[0]) {
> +		static int num_reprobe_attempts;
> +
> +		target = backlight_device_get_by_name(backlight_proxy_target);
> +
> +		if (!target) {
> +			/*
> +			 * The target backlight device might not be ready;
> +			 * try again and disable backlight proxying if it
> +			 * fails too many times.
> +			 */
> +			if (num_reprobe_attempts < max_reprobe_attempts) {
> +				num_reprobe_attempts++;
> +				return -EPROBE_DEFER;
> +			}
> +
> +			pr_warn("Unable to acquire %s after %d attempts. Disabling backlight proxy.",
> +				backlight_proxy_target, max_reprobe_attempts);
> +		}
> +	}

I think `target` is not put in case of error. You probably need to add something like:

  if (target) {
    ret = devm_add_action_or_reset(&wdev->dev, put_device_wrapper, target);
    if (ret < 0)
      return ret;
  }


> +
>  	ret = wmi_brightness_notify(wdev, WMI_BRIGHTNESS_METHOD_SOURCE,
>  	                           WMI_BRIGHTNESS_MODE_GET, &source);
>  	if (ret)
> @@ -188,7 +327,44 @@ static int nvidia_wmi_ec_backlight_probe(struct wmi_device *wdev, const void *ct
>  					      &wdev->dev, wdev,
>  					      &nvidia_wmi_ec_backlight_ops,
>  					      &props);
> -	return PTR_ERR_OR_ZERO(bdev);
> +
> +	if (IS_ERR(bdev))
> +		return PTR_ERR(bdev);
> +
> +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);

`devm_kzalloc()` would probably be better and you should check if `!priv`.


> +	priv->bl_dev = bdev;
> +
> +	dev_set_drvdata(&wdev->dev, priv);
> +
> +	if (target) {
> +		int level = scale_backlight_level(target, bdev);
> +
> +		if (backlight_device_set_brightness(bdev, level))
> +			pr_warn("Unable to import initial brightness level from %s.",
> +				backlight_proxy_target);
> +		priv->proxy_target = target;
> +	}
> +
> +	if (restore_level_on_resume) {
> +		priv->nb.notifier_call = nvidia_wmi_ec_backlight_pm_notifier;
> +		register_pm_notifier(&priv->nb);
> +	}
> +
> +	return 0;
> +}
> +
> +static void nvidia_wmi_ec_backlight_remove(struct wmi_device *wdev)
> +{
> +	struct nvidia_wmi_ec_backlight_priv *priv = dev_get_drvdata(&wdev->dev);
> +	struct backlight_device *proxy_target = priv->proxy_target;
> +
> +	if (proxy_target)
> +		put_device(&proxy_target->dev);

If you switch to `devm_add_action_or_reset()`, this will not be needed.


> +
> +	if (priv->nb.notifier_call)
> +		unregister_pm_notifier(&priv->nb);
> +
> +	kfree(priv);

If you switch to `devm_kzalloc()`, this won't be needed.


>  }
>
>  #define WMI_BRIGHTNESS_GUID "603E9613-EF25-4338-A3D0-C46177516DB7"
> @@ -204,6 +380,7 @@ static struct wmi_driver nvidia_wmi_ec_backlight_driver = {
>  		.name = "nvidia-wmi-ec-backlight",
>  	},
>  	.probe = nvidia_wmi_ec_backlight_probe,
> +	.remove = nvidia_wmi_ec_backlight_remove,
>  	.id_table = nvidia_wmi_ec_backlight_id_table,
>  };
>  module_wmi_driver(nvidia_wmi_ec_backlight_driver);
> --
> 2.27.0
>

Lastly, is it expected that these bugs will be properly fixed?


Regards,
Barnabás Pőcze

[PATCH] nvidia-wmi-ec-backlight: Add workarounds for confused firmware

[Daniel Dadap]

Some notebook systems with EC-driven backlight control appear to have a
firmware bug which causes the system to use GPU-driven backlight control
upon a fresh boot, but then switches to EC-driven backlight control
after completing a suspend/resume cycle. All the while, the firmware
reports that the backlight is under EC control, regardless of what is
actually controlling the backlight brightness.

This leads to the following behavior:

* nvidia-wmi-ec-backlight gets probed on a fresh boot, due to the
  WMI-wrapped ACPI method erroneously reporting EC control.
* nvidia-wmi-ec-backlight does not work until after a suspend/resume
  cycle, due to the backlight control actually being GPU-driven.
* GPU drivers also register their own backlight handlers: in the case
  of the notebook system where this behavior has been observed, both
  amdgpu and the NVIDIA proprietary driver register backlight handlers.
* The GPU which has backlight control upon a fresh boot (amdgpu in the
  case observed so far) can successfully control the backlight through
  its backlight driver's sysfs interface, but stops working after the
  first suspend/resume cycle.
* nvidia-wmi-ec-backlight is unable to control the backlight upon a
  fresh boot, but begins to work after the first suspend/resume cycle.
* The GPU which does not have backlight control (NVIDIA in this case)
  is not able to control the backlight at any point while the system
  is in operation. On similar hybrid systems with an EC-controlled
  backlight, and AMD/NVIDIA iGPU/dGPU, the NVIDIA proprietary driver
  does not register its backlight handler. It has not been determined
  whether the non-functional handler registered by the NVIDIA driver
  is due to another firmware bug, or a bug in the NVIDIA driver.

Since nvidia-wmi-ec-backlight registers as a BACKLIGHT_FIRMWARE type
device, it takes precedence over the BACKLIGHT_RAW devices registered
by the GPU drivers. This in turn leads to backlight control appearing
to be non-functional until after completing a suspend/resume cycle.
However, it is still possible to control the backlight through direct
interaction with the working GPU driver's backlight sysfs interface.

These systems also appear to have a second firmware bug which resets
the EC's brightness level to 100% on resume, but leaves the state in
the kernel at the pre-suspend level. This causes attempts to save
and restore the backlight level across the suspend/resume cycle to
fail, due to the level appearing not to change even though it did.

In order to work around these issue, add quirk tables to detect
systems that are known to show these behaviors. So far, there is
only one known system that requires these workarounds, and both
issues are present on that system, but the quirks are tracked in
separate tables to make it easier to add them to other systems which
may exhibit one of the bugs, but not the other. The original systems
that this driver was tested on during development do not exhibit
either of these quirks.

If a system with the "GPU driver has backlight control" quirk is
detected, nvidia-wmi-ec-backlight will grab a reference to the working
(when freshly booted) GPU backlight handler and relays any backlight
brightness level change requests directed at the EC to also be applied
to the GPU backlight interface. This leads to redundant updates
directed at the GPU backlight driver after a suspend/resume cycle, but
it does allow the EC backlight control to work when the system is
freshly booted.

If a system with the "backlight level reset to full on resume" quirk
is detected, nvidia-wmi-ec-backlight will register a PM notifier to
reset the backlight to the previous level upon resume.

These workarounds are also plumbed through to kernel module parameters,
to make it easier for users who suspect they may be affected by one or
both of these bugs to test whether these workarounds are effective on
their systems as well.

Signed-off-by: Daniel Dadap <ddadap@nvidia.com>
Tested-by: Alexandru Dinu <alex.dinu07@gmail.com>
---
 .../platform/x86/nvidia-wmi-ec-backlight.c    | 181 +++++++++++++++++-
 1 file changed, 179 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/nvidia-wmi-ec-backlight.c b/drivers/platform/x86/nvidia-wmi-ec-backlight.c
index 61e37194df70..ccb3b506c12c 100644
--- a/drivers/platform/x86/nvidia-wmi-ec-backlight.c
+++ b/drivers/platform/x86/nvidia-wmi-ec-backlight.c
@@ -3,8 +3,11 @@
  * Copyright (c) 2020, NVIDIA CORPORATION.  All rights reserved.
  */
 
+#define pr_fmt(f) "%s: " f "\n", KBUILD_MODNAME
+
 #include <linux/acpi.h>
 #include <linux/backlight.h>
+#include <linux/dmi.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/types.h>
@@ -75,6 +78,69 @@ struct wmi_brightness_args {
 	u32 ignored[3];
 };
 
+/**
+ * struct nvidia_wmi_ec_backlight_priv - driver private data
+ * @bl_dev:       the associated backlight device
+ * @proxy_target: backlight device which receives relayed brightness changes
+ * @notifier:     notifier block for resume callback
+ */
+struct nvidia_wmi_ec_backlight_priv {
+	struct backlight_device *bl_dev;
+	struct backlight_device *proxy_target;
+	struct notifier_block nb;
+};
+
+static char *backlight_proxy_target;
+module_param(backlight_proxy_target, charp, 0);
+MODULE_PARM_DESC(backlight_proxy_target, "Relay brightness change requests to the named backlight driver, on systems which erroneously report EC backlight control.");
+
+static int max_reprobe_attempts = 128;
+module_param(max_reprobe_attempts, int, 0);
+MODULE_PARM_DESC(max_reprobe_attempts, "Limit of reprobe attempts when relaying brightness change requests.");
+
+static bool restore_level_on_resume;
+module_param(restore_level_on_resume, bool, 0);
+MODULE_PARM_DESC(restore_level_on_resume, "Restore the backlight level when resuming from suspend, on systems which reset the EC's backlight level on resume.");
+
+static int assign_relay_quirk(const struct dmi_system_id *id)
+{
+	backlight_proxy_target = id->driver_data;
+	return true;
+}
+
+#define PROXY_QUIRK_ENTRY(vendor, product, quirk_data) { \
+	.callback = assign_relay_quirk,                  \
+	.matches = {                                     \
+		DMI_MATCH(DMI_SYS_VENDOR, vendor),       \
+		DMI_MATCH(DMI_PRODUCT_VERSION, product)  \
+	},                                               \
+	.driver_data = quirk_data                        \
+}
+
+static const struct dmi_system_id proxy_quirk_table[] = {
+	PROXY_QUIRK_ENTRY("LENOVO", "Legion S7 15ACH6", "amdgpu_bl1"),
+	{ }
+};
+
+static int assign_restore_quirk(const struct dmi_system_id *id)
+{
+	restore_level_on_resume = true;
+	return true;
+}
+
+#define RESTORE_QUIRK_ENTRY(vendor, product) {           \
+	.callback = assign_restore_quirk,                \
+	.matches = {                                     \
+		DMI_MATCH(DMI_SYS_VENDOR, vendor),       \
+		DMI_MATCH(DMI_PRODUCT_VERSION, product), \
+	}                                                \
+}
+
+static const struct dmi_system_id restore_quirk_table[] = {
+	RESTORE_QUIRK_ENTRY("LENOVO", "Legion S7 15ACH6"),
+	{ }
+};
+
 /**
  * wmi_brightness_notify() - helper function for calling WMI-wrapped ACPI method
  * @w:    Pointer to the struct wmi_device identified by %WMI_BRIGHTNESS_GUID
@@ -119,9 +185,30 @@ static int wmi_brightness_notify(struct wmi_device *w, enum wmi_brightness_metho
 	return 0;
 }
 
+static int scale_backlight_level(struct backlight_device *a,
+				 struct backlight_device *b)
+{
+	/* because floating point math in the kernel is annoying */
+	const int scaling_factor = 65536;
+	int level = a->props.brightness;
+	int relative_level = level * scaling_factor / a->props.max_brightness;
+
+	return relative_level * b->props.max_brightness / scaling_factor;
+}
+
 static int nvidia_wmi_ec_backlight_update_status(struct backlight_device *bd)
 {
 	struct wmi_device *wdev = bl_get_data(bd);
+	struct nvidia_wmi_ec_backlight_priv *priv = dev_get_drvdata(&wdev->dev);
+	struct backlight_device *proxy_target = priv->proxy_target;
+
+	if (proxy_target) {
+		int level = scale_backlight_level(bd, proxy_target);
+
+		if (backlight_device_set_brightness(proxy_target, level))
+			pr_warn("Failed to relay backlight update to \"%s\"",
+				backlight_proxy_target);
+	}
 
 	return wmi_brightness_notify(wdev, WMI_BRIGHTNESS_METHOD_LEVEL,
 	                             WMI_BRIGHTNESS_MODE_SET,
@@ -147,13 +234,65 @@ static const struct backlight_ops nvidia_wmi_ec_backlight_ops = {
 	.get_brightness = nvidia_wmi_ec_backlight_get_brightness,
 };
 
+static int nvidia_wmi_ec_backlight_pm_notifier(struct notifier_block *nb, unsigned long event, void *d)
+{
+
+	/*
+	 * On some systems, the EC backlight level gets reset to 100% when
+	 * resuming from suspend, but the backlight device state still reflects
+	 * the pre-suspend value. Refresh the existing state to sync the EC's
+	 * state back up with the kernel's.
+	 */
+	if (event == PM_POST_SUSPEND) {
+		struct nvidia_wmi_ec_backlight_priv *p;
+
+		p = container_of(nb, struct nvidia_wmi_ec_backlight_priv, nb);
+		return backlight_update_status(p->bl_dev);
+	}
+
+	return 0;
+}
+
 static int nvidia_wmi_ec_backlight_probe(struct wmi_device *wdev, const void *ctx)
 {
+	struct backlight_device *bdev, *target = NULL;
+	struct nvidia_wmi_ec_backlight_priv *priv;
 	struct backlight_properties props = {};
-	struct backlight_device *bdev;
 	u32 source;
 	int ret;
 
+	/*
+	 * Check quirks tables to see if this system needs any of the firmware
+	 * bug workarounds.
+	 */
+
+	/* User-set quirks from the module parameters take precedence */
+	if (!backlight_proxy_target)
+		dmi_check_system(proxy_quirk_table);
+
+	dmi_check_system(restore_quirk_table);
+
+	if (backlight_proxy_target && backlight_proxy_target[0]) {
+		static int num_reprobe_attempts;
+
+		target = backlight_device_get_by_name(backlight_proxy_target);
+
+		if (!target) {
+			/*
+			 * The target backlight device might not be ready;
+			 * try again and disable backlight proxying if it
+			 * fails too many times.
+			 */
+			if (num_reprobe_attempts < max_reprobe_attempts) {
+				num_reprobe_attempts++;
+				return -EPROBE_DEFER;
+			}
+
+			pr_warn("Unable to acquire %s after %d attempts. Disabling backlight proxy.",
+				backlight_proxy_target, max_reprobe_attempts);
+		}
+	}
+
 	ret = wmi_brightness_notify(wdev, WMI_BRIGHTNESS_METHOD_SOURCE,
 	                           WMI_BRIGHTNESS_MODE_GET, &source);
 	if (ret)
@@ -188,7 +327,44 @@ static int nvidia_wmi_ec_backlight_probe(struct wmi_device *wdev, const void *ct
 					      &wdev->dev, wdev,
 					      &nvidia_wmi_ec_backlight_ops,
 					      &props);
-	return PTR_ERR_OR_ZERO(bdev);
+
+	if (IS_ERR(bdev))
+		return PTR_ERR(bdev);
+
+	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+	priv->bl_dev = bdev;
+
+	dev_set_drvdata(&wdev->dev, priv);
+
+	if (target) {
+		int level = scale_backlight_level(target, bdev);
+
+		if (backlight_device_set_brightness(bdev, level))
+			pr_warn("Unable to import initial brightness level from %s.",
+				backlight_proxy_target);
+		priv->proxy_target = target;
+	}
+
+	if (restore_level_on_resume) {
+		priv->nb.notifier_call = nvidia_wmi_ec_backlight_pm_notifier;
+		register_pm_notifier(&priv->nb);
+	}
+
+	return 0;
+}
+
+static void nvidia_wmi_ec_backlight_remove(struct wmi_device *wdev)
+{
+	struct nvidia_wmi_ec_backlight_priv *priv = dev_get_drvdata(&wdev->dev);
+	struct backlight_device *proxy_target = priv->proxy_target;
+
+	if (proxy_target)
+		put_device(&proxy_target->dev);
+
+	if (priv->nb.notifier_call)
+		unregister_pm_notifier(&priv->nb);
+
+	kfree(priv);
 }
 
 #define WMI_BRIGHTNESS_GUID "603E9613-EF25-4338-A3D0-C46177516DB7"
@@ -204,6 +380,7 @@ static struct wmi_driver nvidia_wmi_ec_backlight_driver = {
 		.name = "nvidia-wmi-ec-backlight",
 	},
 	.probe = nvidia_wmi_ec_backlight_probe,
+	.remove = nvidia_wmi_ec_backlight_remove,
 	.id_table = nvidia_wmi_ec_backlight_id_table,
 };
 module_wmi_driver(nvidia_wmi_ec_backlight_driver);
-- 
2.27.0