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

Hi Matthew,

On 10/27/22 11:11, Matthew Garrett wrote:
> On Thu, Oct 27, 2022 at 10:51:45AM +0200, Hans de Goede wrote:
> 
>> In their backlight register paths and this has been present since
>> circa 2015.
>>
>> So both before and after my 6.1 refactor vendor is only preferred
>> on devices which don't implement the ACPI video bus control method.
> 
> Sorry, yes, that's the case I meant.
> 
>> Just because a vendor interface is present does not mean that it will
>> work. Unfortunately for none of the 3 main native/acpi_video/vendor
>> backlight control methods the control method being present also
guarantees
>> that it will work. Which completely sucks, but it is the reality we
>> have to deal with.
> 
> But traditionally that's been logic that we've encoded into the
vendor
> drivers, which can take other factors into account when determining 
> whether the exposed interface works. You've now discarded that 
> knowledge.
As I already mentioned in my previous email, the vendor drivers have
been using something like:

	if (acpi_video_get_backlight_type() != acpi_backlight_vendor)
		return;

In their backlight register paths *since 2015* and even before
then most of them called some acpi_video helper function to determine
if ACPI video backlight control was available and skipped registering
their backlight device if that returned true. And calling that
acpi_video helper is as smart as they traditionally were. That +
DMI quirks and we still have all those quirks.

I was very careful with the refactoring landing in 6.1 to *not*
change any of this.

The *only* behavior which actually is new in 6.1 is the native GPU
drivers now doing the equivalent of:

	if (acpi_video_get_backlight_type() != acpi_backlight_native)
		return;

In their backlight register paths (i), which is causing the native
backlight to disappear on your custom laptop setup and on Chromebooks
(with the Chromebooks case being already solved I hope.).

I am fully aware that this may turn out to also impact other laptops.
I'm keeping out an eye for this and I have specifically reached-out
to the coreboot community asking them to test 6.1 .
> The only way you can maintain the degree of functionality 
> that 6.0 had is to move that determination into core code, or 
> alternatively support dynamic reattachment of backlight interfaces based 
> on vendor drivers loading later. An alternative would be to just revert 
> all of this, and instead only use this logic for the output property 
> interface (which would still result in different outcomes, but only for 
> userland that's choosing to use the new interface, so that's a
different
> problem).
Yes I am considering dropping the "acpi_video_get_backlight_type()
!acpi_backlight_native" check from at least the i915 driver if we get more
bug reports and then indeed only do the equivalent of that check in
the code for the new output property.

I agree this is a possible solution if this turns out to break more
systems and there is no other easy/clean way to fix those. But I would
greatly prefer to keep this change and stop the IMHO bad kernel behavior
of "registering multiple backlight-devices for a single panel and then
let userspace sort it out".

Regards,

Hans


i) Before this, the kernel was relying on userspace preferring acpi_video
or vendor backlight devices over native if both are present and the
native backlight devices were registered unconditionally.

On Thu, Oct 27, 2022 at 11:39:38AM +0200, Hans de Goede wrote:
> The *only* behavior which actually is new in 6.1 is the native GPU
> drivers now doing the equivalent of:
> 
> 	if (acpi_video_get_backlight_type() != acpi_backlight_native)
> 		return;
> 
> In their backlight register paths (i), which is causing the native
> backlight to disappear on your custom laptop setup and on Chromebooks
> (with the Chromebooks case being already solved I hope.).
It's causing the backlight control to vanish on any machine that isn't 
((acpi_video || vendor interface) || !acpi). Most machines that fall 
into that are either weird or Chromebooks or old, but there are machines 
that fall into that.

(I wrote https://mjg59.livejournal.com/127103.html over 12 years ago, so 
please do assume that I'm familiar with the complexities here :) )
> I agree this is a possible solution if this turns out to break more
> systems and there is no other easy/clean way to fix those. But I would
> greatly prefer to keep this change and stop the IMHO bad kernel behavior
> of "registering multiple backlight-devices for a single panel and then
> let userspace sort it out".
If we're not able to make a correct policy decision in the kernel then 
punting it to userland seems like the right thing to do? The kernel 
absolutely *should* make the right decision where it has enough 
information to do so, but in this case the code that's making that 
decision doesn't have the full set of information.