Re: [PATCH v5 02/31] drm/i915: Don't register backlight when another backlight should be used (v2)
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. 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).
Re: [PATCH v5 02/31] drm/i915: Don't register backlight when another backlight should be used (v2)
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.
Re: [PATCH v5 02/31] drm/i915: Don't register backlight when another backlight should be used (v2)
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.
Re: [PATCH v5 02/31] drm/i915: Don't register backlight when another backlight should be used (v2)
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.
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. 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. So, it feels like there's two choices:
1) Make a default policy decision, but then allow that decision to be
altered later on (eg, when a vendor-specific platform driver has been
loaded) - you've said this poses additional complexities.
2) Move the knowledge of whether or not there's a vendor interface into
the core code. Basically take every platform driver that exposes a
vendor interface, and move the detection code into the core.
I think any other approach is going to result in machines that
previously worked no longer working (and you can't just make the
vendor/native split dependent on the Coreboot DMI BIOS string, because
there are some Coreboot platforms that implement the vendor interface
for compatibility, and you also can't ask all Coreboot users to update
their firmware to fix things)
Re: [PATCH v5 02/31] drm/i915: Don't register backlight when another backlight should be used (v2)
On Tue, Oct 25, 2022 at 10:25:33PM +0200, Hans de Goede wrote: > Having the native driver come and then go and be replaced > with the vendor driver would also be quite inconvenient > for these planned changes. I understand that it would be inconvenient, but you've broken existing working setups. > Can you perhaps explain a bit in what way your laptop > is weird ? It's a Chinese replacement motherboard for a Thinkpad X201, running my own port of Coreboot. Its DMI strings look like an actual Thinkpad in order to ensure that thinkpad_acpi can bind for hotkey suport, so it's hard to quirk. It'll actually be fixed by your proposed patch to fall back to native rather than vendor, but that patch will break any older machines that offer a vendor interface and don't have the native control hooked up (pretty sure at least the Thinkpad X40 falls into that category).
Re: [PATCH v5 02/31] drm/i915: Don't register backlight when another backlight should be used (v2)
On Tue, Oct 25, 2022 at 08:50:54PM +0200, Hans de Goede wrote: > That is a valid point, but keep in mind that this is only used on ACPI > platforms and then only on devices with a builtin LCD panel and then > only by GPU drivers which actually call acpi_video_get_backlight_type(), > so e.g. not by all the ARM specific display drivers. > > So I believe that Chromebooks quite likely are the only devices with > this issue. My laptop is, uh, weird, but it falls into this category. > > 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. > > Getting rid of the whole ping-ponging of which backlight drivers > get loaded during boot was actually one of the goals of the rework > which landed in 6.1 this actually allowed us to remove some quirks > because some hw/firmware did not like us changing our mind and > switching backlight interfaces after first poking another one. > So we definitely don't want to go back to the ping-pong thing. Defaulting to native but then having a vendor driver be able to disable native drivers seems easiest? It shouldn't be a regression over the previous state of affairs since both drivers were being loaded already.
Re: [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.
