Re: [PATCH v2] drm/client: Detect when ACPI lid is closed during initialization

From: Mario Limonciello
Date: Wed May 29 2024 - 12:27:13 EST




If you don't hook into some lid notify event how is one supposed to get
the display back to life after opening the lid?

I guess in my mind it's a tangential to the "initial modeset". The DRM
master can issue a modeset to enable the combination as desired.

This code is run whenever there's a hotplug/etc. Not sure why you're
only thinking about the initial modeset.

Got it; so in that case adding a notification chain for lid events to run it again should do the trick.



When I tested I did confirm that with mutter such an event is received
and it does the modeset to enable the eDP when lid is opened.

This code isn't relevant when you have a userspace drm master
calling the shots.

Right.



Let me ask this - what happens if no DRM master running and you hotplug
a DP cable? Does a "new" clone configuration get done?

Yes, this code reprobes the displays and comes up with a new
config to suit the new situation.

Got it; in this case you're right we should have some notification chain. Do you think it should be in the initial patch or a follow up?


The other potential issue here is whether acpi_lid_open() is actually
trustworthy. Some kms drivers have/had some lid handling in their own
code, and I'm pretty sure those have often needed quirks/modparams
to actually do sensible things on certain machines.

FWIW I ripped out all the lid crap from i915 long ago since it was
half backed, mostly broken, and ugly, and I'm not looking to add it
back there. But I do think handling that in drm_client does seem
somewhat sane, as that should more or less match what userspace
clients would do. Just a question of how bad the quirk situation
will get...


If the lid reporting is wrong it's not just drm_client that would falter. There are other parts of the kernel that rely upon acpi_lid_open() being accurate and IMO it would be best to put any quirks to the effect in drivers/acpi/button.c.

If it can't be relied upon then it's best to just report -EINVAL or -ENODEV.


Also a direct acpi_lid_open() call seems a bit iffy. But I guess if
someone needs this to work on non-ACPI system they get to figure out
how to abstract it better. acpi_lid_open() does seem to return != 0
when ACPI is not supported, so at least it would err on the side
of enabling everything.


Yeah acpi_lid_open() seemed fine to me specifically because non ACPI hardcodes to open.