[PATCH] platform/x86: nvidia-wmi-ec-backlight: Add force module parameter

[PATCH] platform/x86: nvidia-wmi-ec-backlight: Add force module parameter

[Hans de Goede]

On models with the Nvidia WMI EC backlight interface, when the laptop is
configured in dynamic mux mode in the BIOS the backlight should always be
controlled by the Nvidia WMI EC backlight interface.

But it turns out that on some laptop models, E.g. Some Lenovo Legion models
/sys/class/backlight/nvidia_wmi_ec_backlight only works when the LCD panel
is muxed to the Nvidia GPU and when muxed to the AMD iGPU amdgpu_bl0
controls the backlight.

This appears to be a firmware bug on these laptops, but prior to 6.1.4
users would have both nvidia_wmi_ec_backlight and amdgpu_bl0 and could
work around this in userspace.

Add a force module parameter which can be used with acpi_backlight=native
to restore the old behavior as a workound (for now):

"acpi_backlight=native nvidia-wmi-ec-backlight.force=1"

Fixes: 8d0ca287fd8c ("platform/x86: nvidia-wmi-ec-backlight: Use acpi_video_get_backlight_type()")
Link: https://bugzilla.kernel.org/show_bug.cgi?id=217026
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/nvidia-wmi-ec-backlight.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/nvidia-wmi-ec-backlight.c b/drivers/platform/x86/nvidia-wmi-ec-backlight.c
index baccdf658538..1b572c90c76e 100644
--- a/drivers/platform/x86/nvidia-wmi-ec-backlight.c
+++ b/drivers/platform/x86/nvidia-wmi-ec-backlight.c
@@ -12,6 +12,10 @@
 #include <linux/wmi.h>
 #include <acpi/video.h>
 
+static bool force;
+module_param(force, bool, 0444);
+MODULE_PARM_DESC(force, "Force loading (disable acpi_backlight=xxx checks");
+
 /**
  * wmi_brightness_notify() - helper function for calling WMI-wrapped ACPI method
  * @w:    Pointer to the struct wmi_device identified by %WMI_BRIGHTNESS_GUID
@@ -91,7 +95,7 @@ static int nvidia_wmi_ec_backlight_probe(struct wmi_device *wdev, const void *ct
 	int ret;
 
 	/* drivers/acpi/video_detect.c also checks that SOURCE == EC */
-	if (acpi_video_get_backlight_type() != acpi_backlight_nvidia_wmi_ec)
+	if (!force && acpi_video_get_backlight_type() != acpi_backlight_nvidia_wmi_ec)
 		return -ENODEV;
 
 	/*
-- 
2.39.1

Re: [PATCH] platform/x86: nvidia-wmi-ec-backlight: Add force module parameter

[Daniel Dadap]

Thanks, Hans:

On Tue, Feb 14, 2023 at 05:34:35PM +0100, Hans de Goede wrote:
> On models with the Nvidia WMI EC backlight interface, when the laptop is
> configured in dynamic mux mode in the BIOS the backlight should always be
> controlled by the Nvidia WMI EC backlight interface.
> 
> But it turns out that on some laptop models, E.g. Some Lenovo Legion models
> /sys/class/backlight/nvidia_wmi_ec_backlight only works when the LCD panel
> is muxed to the Nvidia GPU and when muxed to the AMD iGPU amdgpu_bl0
> controls the backlight.

I'm not certain that this description is accurate. From my understanding so
far, the problem is that the working backlight device changes at runtime,
regardless of the mux state. Recall that we don't actually have support for
dynamic mux switching, yet, so if the system is booted in dynamic mode, it
will remain muxed to the AMD iGPU the whole time.

In theory the EC should in charge of backlight control at all times when
operating in dynamic mux switch mode, regardless of which GPU the mux is
switched to. However, on some of the Lenovo Legion models it appears that
occasionally the backlight control falls to the native interface, instead.
The user who initially reported this class of issue observed that only the
EC backlight interface would work upon a fresh boot, and only the native
AMD iGPU interface would work after a suspend/resume cycle, and it would
continue to work after further suspend/resume cycles. The second reporter
observed that the EC backlight interface was usually active when connected
to external power, and the iGPU native interface was usually active when
running off of internal battery power, however, occasionally this wouldn't
hold true, with no discernible pattern.

This third report sounds a little bit different: it is stated that different
interfaces work to "change the brightness of the screen depending on if I
was using the AMD GPU or NVIDIA GPU to display the current application." It
is unclear to me what it means to use a GPU to display an application: my
interpretation of this is that the system is configured to use PRIME Render
Offload, and the desktop is being primarily driven by the iGPU, while select
applications are explicitly render offloaded to be driven by the dGPU, but
displayed on the iGPU, since the display always remains muxed to the iGPU.

Since we don't fully understand what is going wrong, yet, it may be safer
to leave the mux out of the description of the problem that this patch
restores the ability to work around for, and say something more vague like
"On some Lenovo Legion models, the backlight might be driven by either one
of nvidia_wmi_ec_backlight or amdgpu_bl0 at different times."

> This appears to be a firmware bug on these laptops, but prior to 6.1.4
> users would have both nvidia_wmi_ec_backlight and amdgpu_bl0 and could
> work around this in userspace.
>
> Add a force module parameter which can be used with acpi_backlight=native
> to restore the old behavior as a workound (for now):
> 
> "acpi_backlight=native nvidia-wmi-ec-backlight.force=1"
> 
> Fixes: 8d0ca287fd8c ("platform/x86: nvidia-wmi-ec-backlight: Use acpi_video_get_backlight_type()")
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217026
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/platform/x86/nvidia-wmi-ec-backlight.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/nvidia-wmi-ec-backlight.c b/drivers/platform/x86/nvidia-wmi-ec-backlight.c
> index baccdf658538..1b572c90c76e 100644
> --- a/drivers/platform/x86/nvidia-wmi-ec-backlight.c
> +++ b/drivers/platform/x86/nvidia-wmi-ec-backlight.c
> @@ -12,6 +12,10 @@
>  #include <linux/wmi.h>
>  #include <acpi/video.h>
>  
> +static bool force;
> +module_param(force, bool, 0444);
> +MODULE_PARM_DESC(force, "Force loading (disable acpi_backlight=xxx checks");

This is a bit of a nit, but it took me several times reading the description
to understand that disabling the acpi_backlight check refers specifically to
disabling the acpi_video_get_backlight_type() check within the EC backlight
driver. My initial read was that it is intended to disable the various checks
in drivers/acpi/video_detect.c, which it does not. Something along the lines
of "(ignore acpi_video_get_backlight_type check)" makes the intent a little
bit clearer, I think.

Apart from the commit message and documentation nits, I think that this patch
makes sense:

ACKed-by: Daniel Dadap <ddadap@nvidia.com>

> +
>  /**
>   * wmi_brightness_notify() - helper function for calling WMI-wrapped ACPI method
>   * @w:    Pointer to the struct wmi_device identified by %WMI_BRIGHTNESS_GUID
> @@ -91,7 +95,7 @@ static int nvidia_wmi_ec_backlight_probe(struct wmi_device *wdev, const void *ct
>  	int ret;
>  
>  	/* drivers/acpi/video_detect.c also checks that SOURCE == EC */
> -	if (acpi_video_get_backlight_type() != acpi_backlight_nvidia_wmi_ec)
> +	if (!force && acpi_video_get_backlight_type() != acpi_backlight_nvidia_wmi_ec)
>  		return -ENODEV;
>  
>  	/*
> -- 
> 2.39.1
>