Re: [Nouveau] [PATCH v5 02/31] drm/i915: Don't register backlight when another backlight should be used (v2)

[Hans de Goede <hdegoede@redhat.com>]

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@intel.com>
>> Signed-off-by: Hans de Goede <hdegoede@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@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