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@xxxxxxxxx>
>> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxx/
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
