| Message ID | 20230214163435.7065-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | platform/x86: nvidia-wmi-ec-backlight: Add force module parameter | expand |
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 >
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; /*
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(-)