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

Hi,

On 10/26/22 01:40, Matthew Garrett wrote:
> 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.
Ok, so this is a local customization to what is already a custom BIOS
for a custom motherboard. There is a lot of custom in that sentence and
TBH at some point things might become too custom for them to be expected
to work OOTB.

Note that you can always just override the choses made by the heuristisc/
quirks on the kernel commandline by adding:

acpi_backlight=native   (I think you want this one?)

or if you want the old thinkpad_acpi module vendor/EC interface:

acpi_backlight=vendor

Asking you to pass this on the commandline does not seem like a huge
stretch given the large amount of hw/firmware customization you have done ?
> 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.
I'm afraid things are not that simple. I assume that with
"if ACPI backlight control is expected to work" you mean don't
use ACPI backlight control when (acpi_osi_is_win8() && native_available)
evaluates to true because it is known to be broken on some of
those systems because Windows 8 stopped using it ?

Unfortunately something similar applies to vendor interfaces,
When Windows XP started using (and mandating for certification
IIRC) ACPI backlight control, vendors still kept their own
vendor specific EC/smbios/ACPI/WMI backlight interfaces around for
a long long time, except they were often no longer tested.

So basically we have 3 major backlight control methods:

1. native GPU backlight control, which sometimes does not work
on older laptops because the backlight is connected to the EC
rather then the GPU there, yet often still enabled in the
video-bios-tables so the GPU drivers will still try to use it.

2. ACPI -> known to be always present on recent Windows laptops
because mandated by the hardware certification requirements
(even on Windows 8+), but regularly broken on Windows 8+ because
their backlight control was moved from the core-os to the GPU
drivers and those typically use the native method.

3. Vendor specific EC/smbios/ACPI/WMI interfaces which work
on older laptops, but are often present on newer laptops
despite them no longer working and to get working backlight
control either 1. or 2. should be used.

So basically non of the 3 main backlight control methods can
be trusted even if they are present. Which is why need to have
a combination of heuristics + quirks.

And I have been working on moving all this into a central
place in drivers/acpi/video_detect.c because having
the heuristics + quirks spread out all over the place does
not help.
> 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.
I don't understand why you keep talking about the old vendor interfaces,
at least for the chromebook part of this thread the issue is that
the i915 driver no longer registers the intel_backlight device which
is a native device type, which is caused by the patch this email
thread is about (and old vendor interfaces do not come into play
at all here). So AFAICT this is a native vs acpi backlight control
issue ?

I really want to resolve your bug, but I still lack a lot of info,
like what backlight interface you were actually using in 6.0 ?

Can you please provide the following info for your laptop:

1. Output of "ls /sys/class/backlight" with 6.0  (working setup)
2. Output of "ls /sys/class/backlight" with 6.1  (non-working setup)
3. dmidecode output, so that I can check if this quirk:

        {
         .callback = video_detect_force_video,
         /* ThinkPad X201s */
         .matches = {
                DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
                DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad X201s"),
                },
        },

will trigger.

4. An acpidump. Although you already said that you have removed the
ACPI video bus bits, so I guess I can just assume that
the ACPI_VIDEO_BACKLIGHT flag won't get set.

Regards,

Hans


p.s.

This thread has made me wonder if the 6.1 changes don't cause
regressions on other laptops flashed with a CoreOS BIOS, I will
start a mail-thread asking for testing on the CoreOS mailinglist.

On Wed, Oct 26, 2022 at 11:59:28AM +0200, Hans de Goede wrote:
> Ok, so this is a local customization to what is already a custom BIOS
> for a custom motherboard. There is a lot of custom in that sentence and
> TBH at some point things might become too custom for them to be expected
> to work OOTB.
But it *did* work OOTB before. You broke it. I accept that I'm a 
ludicrously weird corner case here, but there are going to be other 
systems that are also affected by this.
> I'm afraid things are not that simple. I assume that with
> "if ACPI backlight control is expected to work" you mean
don't
> use ACPI backlight control when (acpi_osi_is_win8() &&
native_available)
> evaluates to true because it is known to be broken on some of
> those systems because Windows 8 stopped using it ?
Correct.
> Unfortunately something similar applies to vendor interfaces,
> When Windows XP started using (and mandating for certification
> IIRC) ACPI backlight control, vendors still kept their own
> vendor specific EC/smbios/ACPI/WMI backlight interfaces around for
> a long long time, except they were often no longer tested.
The current situation (both before your patchset and with its current 
implementation) is that vendor is preferred to native, so if the vendor 
interface is present then we're already using it.
> > 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.
> 
> I don't understand why you keep talking about the old vendor
interfaces,
> at least for the chromebook part of this thread the issue is that
> the i915 driver no longer registers the intel_backlight device which
> is a native device type, which is caused by the patch this email
> thread is about (and old vendor interfaces do not come into play
> at all here). So AFAICT this is a native vs acpi backlight control
> issue ?
I'm referring to your proposed patch that changed the default from 
backlight_vendor to backlight_native, which would fix my machine and 
Chromebooks but break anything that relies on the vendor interfaces.
> I really want to resolve your bug, but I still lack a lot of info,
> like what backlight interface you were actually using in 6.0 ?
Native.
>         {
>          .callback = video_detect_force_video,
>          /* ThinkPad X201s */
>          .matches = {
>                 DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
>                 DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad X201s"),
>                 },
>         },
> 
> will trigger.
In this case you'd break anyone else running the system who isn't using 
the hacked EC and different ACPI tables - obviously there's ways round 
this, but realistically since I'm (as far as I know) the only person in 
this situation it makes more sense for me to add a kernel parameter than 
carry around an exceedingly niche DMI quirk. I'm fine with that. But the 
point I'm trying to make is that the machines *are* telling you whether 
they'd prefer vendor or native, and you're not taking that into account 
in the video_detect code.