Hans de Goede
2022-Oct-25 23:27 UTC
[Nouveau] [PATCH v5 02/31] drm/i915: Don't register backlight when another backlight should be used (v2)
Hi, On 10/25/22 22:40, Matthew Garrett wrote:> On Tue, Oct 25, 2022 at 10:25:33PM +0200, Hans de Goede wrote: > >> Having the native driver come and then go and be replaced >> with the vendor driver would also be quite inconvenient >> for these planned changes. > > I understand that it would be inconvenient, but you've broken existing > working setups.I fully acknowledge that I have broken existing working setups and I definitely want to see this fixed before say 6.1-rc6! I'm not convinced (at all) that any solutions which re-introduce acpi_video_get_backlight_type() return-s value changing half way the boot, with some backlight interface getting registered and then unregistered again later because it turns out to be the wrong one is a good fix here. The whole goal of the refactor was to leave these sorts of shenanigans behind us.>> Can you perhaps explain a bit in what way your laptop >> is weird ? > > It's a Chinese replacement motherboard for a Thinkpad X201, running my > own port of Coreboot. Its DMI strings look like an actual Thinkpad in > order to ensure that thinkpad_acpi can bind for hotkey suport, so it's > hard to quirk. It'll actually be fixed by your proposed patch to fall > back to native rather than vendor, but that patch will break any older > machines that offer a vendor interface and don't have the native control > hooked up (pretty sure at least the Thinkpad X40 falls into that > category).So looking at: https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/heads/master/src/mainboard/51nb/x210/acpi/graphics.asl this code should actually set the ACPI_VIDEO_BACKLIGHT flag: drivers/acpi/scan.c: static acpi_status acpi_backlight_cap_match(acpi_handle handle, u32 level, void *context, void **return_value) { long *cap = context; if (acpi_has_method(handle, "_BCM") && acpi_has_method(handle, "_BCL")) { acpi_handle_debug(handle, "Found generic backlight support\n"); *cap |= ACPI_VIDEO_BACKLIGHT; /* We have backlight support, no need to scan further */ return AE_CTRL_TERMINATE; } return 0; } What does seem to be missing compared to a "normal" DSDT is a call to _OSI("Windows 2012") so I would expect this code in acpi_video_get_backlight_type(): /* On systems with ACPI video use either native or ACPI video. */ if (video_caps & ACPI_VIDEO_BACKLIGHT) { /* * Windows 8 and newer no longer use the ACPI video interface, * so it often does not work. If the ACPI tables are written * for win8 and native brightness ctl is available, use that. * * The native check deliberately is inside the if acpi-video * block on older devices without acpi-video support native * is usually not the best choice. */ if (acpi_osi_is_win8() && native_available) return acpi_backlight_native; else return acpi_backlight_video; } To enter the "return acpi_backlight_video" path since acpi_osi_is_win8() will return false. And then the ACPI backlight methods from: https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/heads/master/src/mainboard/51nb/x210/acpi/graphics.asl should get called when changing the backlight brightness, so assuming that those methods work then things should work fine. What does "ls /sys/class/backlight" output on the X210 / NB51 board with a 6.0 kernel? And what does it output with the 6.1-rc? kernels? IOW which backlight device / control method is being selected and which one do you want / which one(s) do actually work? I have been thinking about maybe doing something with a dmi_get_bios_year() check (see below), but that will cause native to get prefered over vendor on old ThinkPads with coreboot (and thus a new enough year in DMI_BIOS_DATE), which will likely break backlight control there (if i915 offers backlight control on those that is). Also I wonder if it would be possible to set DMI_BIOS_VENDOR to "Coreboot" so that we can use that? Note that thinkpad_acpi does not care about the DMI_BIOS_VENDOR value, at least not on models which start their DMI_PRODUCT_VERSION with either "ThinkPad" or "Lenovo". ### Looking more at this I notice that coreboot has a drivers_intel_gma_displays_ssdt_generate() which seems to at least always generate ACPI video bus ASL including backlight control bits. So the only reason why the current heurstics are not returning native is the acpi_osi_is_win8() check. So maybe that beeds to become: if ((acpi_osi_is_win8() || dmi_get_bios_year() >= 2018) && native_available) return acpi_backlight_native; else return acpi_backlight_video; Although I think that will result in the same behavior as my patch below, and then my patch below would be cleaner... ### Also note that there actually already is a DMI quirk for the X201s, forcing ACPI video backlight control there. This is not strictly necessary, but when we first started using native by default on (back then) newer laptops some users of script everything yourself window-managers like i3 complained that they were relying on the in kernel handling of brightness key presses, which only works when using the acpi backlight control method... If that quirk matches your device then fixing the acpi_video_get_backlight_type() heuristics is not going to help. In that case we might decide to just drop the quirk though, since it was never really necessary in the first place; or change it to native, which may also help the X210 case? Regards, Hans>From 31fa1f5e60b32a5e239023a2f0f5a6d457175e5a Mon Sep 17 00:00:00 2001From: Hans de Goede <hdegoede at redhat.com> Date: Tue, 25 Oct 2022 20:38:56 +0200 Subject: [PATCH] ACPI: video: Fix acpi_video_get_backlight_type() on coreboot laptops On laptops flashed with Coreboot the ACPI tables will often not have ACPI Video Bus backlight control, which was causing acpi_video_get_backlight_type() to return vendor even though GPU native backlight control is available and should be used. Rework acpi_video_get_backlight_type() so as to not rely on the presence of ACPI Video Bus backlight control to decide if native backlight control should be used. Note this may break things on old laptops where the vendor interface should actually be used, in case these have been flashed with Coreboot causing their BIOS-date year to match the check. Signed-off-by: Hans de Goede <hdegoede at redhat.com> --- drivers/acpi/video_detect.c | 31 +++++++++++-------------------- 1 file changed, 11 insertions(+), 20 deletions(-) diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c index 9cd8797d12bb..2fe0fd22a7ac 100644 --- a/drivers/acpi/video_detect.c +++ b/drivers/acpi/video_detect.c @@ -718,30 +718,21 @@ static enum acpi_backlight_type __acpi_video_get_backlight_type(bool native) if (apple_gmux_present()) return acpi_backlight_apple_gmux; - /* On systems with ACPI video use either native or ACPI video. */ - if (video_caps & ACPI_VIDEO_BACKLIGHT) { - /* - * Windows 8 and newer no longer use the ACPI video interface, - * so it often does not work. If the ACPI tables are written - * for win8 and native brightness ctl is available, use that. - * - * The native check deliberately is inside the if acpi-video - * block on older devices without acpi-video support native - * is usually not the best choice. - */ - if (acpi_osi_is_win8() && native_available) - return acpi_backlight_native; - else - return acpi_backlight_video; - } - /* - * Chromebooks that don't have backlight handle in ACPI table - * are supposed to use native backlight if it's available. + * On older systems the backlight was typically connected to the EC + * rather then to the GPU, so GPU native control may not work there. + * Prefer native on devices designed for Windows 8+, Chromebooks and + * laptops with a BIOS from 2018 or later (for misc. Coreboot models). */ - if (google_cros_ec_present() && native_available) + if (native_available && (acpi_osi_is_win8() || + google_cros_ec_present() || + dmi_get_bios_year() >= 2018)) return acpi_backlight_native; + /* Use the ACPI video interface if available */ + if (video_caps & ACPI_VIDEO_BACKLIGHT) + return acpi_backlight_video; + /* No ACPI video (old hw), use vendor specific fw methods. */ return acpi_backlight_vendor; } -- 2.37.3
Matthew Garrett
2022-Oct-25 23:40 UTC
[Nouveau] [PATCH v5 02/31] drm/i915: Don't register backlight when another backlight should be used (v2)
On Wed, Oct 26, 2022 at 01:27:25AM +0200, Hans de Goede wrote:> this code should actually set the ACPI_VIDEO_BACKLIGHT flag: > drivers/acpi/scan.c: > > static acpi_status > acpi_backlight_cap_match(acpi_handle handle, u32 level, void *context, > void **return_value) > { > long *cap = context; > > if (acpi_has_method(handle, "_BCM") && > acpi_has_method(handle, "_BCL")) { > acpi_handle_debug(handle, "Found generic backlight support\n"); > *cap |= ACPI_VIDEO_BACKLIGHT; > /* We have backlight support, no need to scan further */ > return AE_CTRL_TERMINATE; > } > return 0; > }Ah, yeah, my local tree no longer matches the upstream behaviour because I've hacked the EC firmware to remove the backlight trigger because it had an extremely poor brightness curve and also automatically changed it on AC events - as a result I removed the backlight code from the DSDT and just fell back to the native control. Like I said I'm a long way from the normal setup, but this did previously work. The "right" logic here seems pretty simple: if ACPI backlight control is expected to work, use it. If it isn't, but there's a vendor interface, use it. If there's no vendor interface, use the native interface. The problem you're dealing with is that the knowledge of whether or not there's a vendor interface isn't something the core kernel code knows about. What you're proposing here is effectively for us to expose additional information about whether or not there's a vendor interface in the system firmware, but since we're talking in some cases about hardware that's almost 20 years old, we're not realistically going to get those old machines fixed. So, it feels like there's two choices: 1) Make a default policy decision, but then allow that decision to be altered later on (eg, when a vendor-specific platform driver has been loaded) - you've said this poses additional complexities. 2) Move the knowledge of whether or not there's a vendor interface into the core code. Basically take every platform driver that exposes a vendor interface, and move the detection code into the core. I think any other approach is going to result in machines that previously worked no longer working (and you can't just make the vendor/native split dependent on the Coreboot DMI BIOS string, because there are some Coreboot platforms that implement the vendor interface for compatibility, and you also can't ask all Coreboot users to update their firmware to fix things)