Dmitry Osipenko
2022-Sep-25 23:39 UTC
[Nouveau] [PATCH v5 02/31] drm/i915: Don't register backlight when another backlight should be used (v2)
25.08.2022 17:36, Hans de Goede ?????:> Before this commit when we want userspace to use the acpi_video backlight > device we register both the GPU's native backlight device and acpi_video's > firmware acpi_video# backlight device. This relies on userspace preferring > firmware type backlight devices over native ones. > > Registering 2 backlight devices for a single display really is > undesirable, don't register the GPU's native backlight device when > another backlight device should be used. > > Changes in v2: > - Use drm_info(drm_dev, ...) for log messages > > Reviewed-by: Jani Nikula <jani.nikula at intel.com> > Signed-off-by: Hans de Goede <hdegoede at redhat.com> > --- > drivers/gpu/drm/i915/display/intel_backlight.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_backlight.c b/drivers/gpu/drm/i915/display/intel_backlight.c > index 681ebcda97ad..03c7966f68d6 100644 > --- a/drivers/gpu/drm/i915/display/intel_backlight.c > +++ b/drivers/gpu/drm/i915/display/intel_backlight.c > @@ -8,6 +8,8 @@ > #include <linux/pwm.h> > #include <linux/string_helpers.h> > > +#include <acpi/video.h> > + > #include "intel_backlight.h" > #include "intel_backlight_regs.h" > #include "intel_connector.h" > @@ -952,6 +954,11 @@ int intel_backlight_device_register(struct intel_connector *connector) > > WARN_ON(panel->backlight.max == 0); > > + if (!acpi_video_backlight_use_native()) { > + drm_info(&i915->drm, "Skipping intel_backlight registration\n"); > + return 0; > + } > + > memset(&props, 0, sizeof(props)); > props.type = BACKLIGHT_RAW; >This breaks backlight on Acer Chromebook Spin 713 because backlight isn't registered anymore. Any ideas how to fix it?
Hans de Goede
2022-Sep-27 11:04 UTC
[Nouveau] [PATCH v5 02/31] drm/i915: Don't register backlight when another backlight should be used (v2)
Hi Dmitry, On 9/26/22 01:39, Dmitry Osipenko wrote:> 25.08.2022 17:36, Hans de Goede ?????: >> Before this commit when we want userspace to use the acpi_video backlight >> device we register both the GPU's native backlight device and acpi_video's >> firmware acpi_video# backlight device. This relies on userspace preferring >> firmware type backlight devices over native ones. >> >> Registering 2 backlight devices for a single display really is >> undesirable, don't register the GPU's native backlight device when >> another backlight device should be used. >> >> Changes in v2: >> - Use drm_info(drm_dev, ...) for log messages >> >> Reviewed-by: Jani Nikula <jani.nikula at intel.com> >> Signed-off-by: Hans de Goede <hdegoede at redhat.com> >> --- >> drivers/gpu/drm/i915/display/intel_backlight.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_backlight.c b/drivers/gpu/drm/i915/display/intel_backlight.c >> index 681ebcda97ad..03c7966f68d6 100644 >> --- a/drivers/gpu/drm/i915/display/intel_backlight.c >> +++ b/drivers/gpu/drm/i915/display/intel_backlight.c >> @@ -8,6 +8,8 @@ >> #include <linux/pwm.h> >> #include <linux/string_helpers.h> >> >> +#include <acpi/video.h> >> + >> #include "intel_backlight.h" >> #include "intel_backlight_regs.h" >> #include "intel_connector.h" >> @@ -952,6 +954,11 @@ int intel_backlight_device_register(struct intel_connector *connector) >> >> WARN_ON(panel->backlight.max == 0); >> >> + if (!acpi_video_backlight_use_native()) { >> + drm_info(&i915->drm, "Skipping intel_backlight registration\n"); >> + return 0; >> + } >> + >> memset(&props, 0, sizeof(props)); >> props.type = BACKLIGHT_RAW; >> > > This breaks backlight on Acer Chromebook Spin 713 because backlight > isn't registered anymore. Any ideas how to fix it?Thank you for reporting this. Let me start with some background info on this change: As you may have noticed sometimes on laptops there are multiple backlights registered under /sys/class/backlight and we just let userspace figure out which one to use, which is quite bad. This patch is part of a series fixing this, this is also preparation for adding a new display brightness control API where the brightness is a property on the drm_connector object for the panel/display, which of course requires the kernel to know which backlight control method to use. If you are want to know more about the new userspace API see: https://lore.kernel.org/dri-devel/b61d3eeb-6213-afac-2e70-7b9791c86d2e at redhat.com/ What this series does is on x86/ACPI platforms make all the possible /sys/class/backlight providers call: acpi_video_get_backlight_type() (acpi_video_backlight_use_native() is a special wrapper) and only if that returns their type then have them register their backlight device. So to fix this we need to make acpi_video_get_backlight_type() return native on the Acer Chromebook Spin 713. The heuristics used in acpi_video_get_backlight_type() is explained by comments in the function: /* * The below heuristics / detection steps are in order of descending * presedence. The commandline takes presedence over anything else. */ /* DMI quirks override any autodetection. */ /* Special cases such as nvidia_wmi_ec and apple gmux. */ None of these apply here, so we end up in the core of this function: /* 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; } /* No ACPI video (old hw), use vendor specific fw methods. */ return acpi_backlight_vendor; The acpi_video_backlight_use_native() wrappers causes native_available to be true, so one or both of these 2 conditions fail: 1. if (video_caps & ACPI_VIDEO_BACKLIGHT) 2. if (acpi_osi_is_win8()) I assume that 2. will actually likely fail on quite a few chromebooks. So to fix this you could do something like this: diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c index 0d9064a9804c..660ea46fbee8 100644 --- a/drivers/acpi/video_detect.c +++ b/drivers/acpi/video_detect.c @@ -75,6 +75,12 @@ find_video(acpi_handle handle, u32 lvl, void *context, void **rv) return AE_OK; } +static bool is_chromebook(void) +{ + // FIXME return true when running under ChromeOS (coreboot) firmware + return false; +} + /* This depends on ACPI_WMI which is X86 only */ #ifdef CONFIG_X86 static bool nvidia_wmi_ec_supported(void) @@ -724,7 +730,7 @@ static enum acpi_backlight_type __acpi_video_get_backlight_type(bool native) * block on older devices without acpi-video support native * is usually not the best choice. */ - if (acpi_osi_is_win8() && native_available) + if (native_available && (acpi_osi_is_win8() || is_chromebook())) return acpi_backlight_native; else return acpi_backlight_video; The ACPI video bus is a pretty standard thing (and part of the ACPI standard), still I would not be surprised if it is missing from the ACPI tables on some Chromebooks, so a slightly bigger hammer approach would be: diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c index 0d9064a9804c..ff950be472a7 100644 --- a/drivers/acpi/video_detect.c +++ b/drivers/acpi/video_detect.c @@ -75,6 +75,12 @@ find_video(acpi_handle handle, u32 lvl, void *context, void **rv) return AE_OK; } +static bool is_chromeos_firmware(void) +{ + // FIXME return true when running under ChromeOS (coreboot) firmware + return false; +} + /* This depends on ACPI_WMI which is X86 only */ #ifdef CONFIG_X86 static bool nvidia_wmi_ec_supported(void) @@ -713,6 +719,10 @@ static enum acpi_backlight_type __acpi_video_get_backlight_type(bool native) if (apple_gmux_present()) return acpi_backlight_apple_gmux; + /* On Chromebooks always use native if available */ + if (is_chromeos_firmware() && native_available) + return acpi_backlight_native; + /* On systems with ACPI video use either native or ACPI video. */ if (video_caps & ACPI_VIDEO_BACKLIGHT) { /* I assume you are more familiar with Chromebooks ACPI tables (or at least are better capable to sample a couple of them) so I will leave which approach to take is best up to you. Regards, Hans
Matthew Garrett
2022-Oct-24 20:30 UTC
[Nouveau] [PATCH v5 02/31] drm/i915: Don't register backlight when another backlight should be used (v2)
On Tue, Sep 27, 2022 at 01:04:52PM +0200, Hans de Goede wrote:> So to fix this we need to make acpi_video_get_backlight_type() > return native on the Acer Chromebook Spin 713.Isn't the issue broader than that? Unless the platform is Windows 8 or later, we'll *always* (outside of some corner cases) return acpi_backlight_vendor if there's no ACPI video interface. This is broken for any platform that implements ACPI but relies on native video control, which is going to include a range of Coreboot platforms, not just Chromebooks. I think for this to work correctly you need to have the infrastructure be aware of whether or not a vendor interface exists, which means having to handle cleanup if a vendor-specific module gets loaded later.