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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, May 29, 2024 at 09:45:55AM -0500, Mario Limonciello wrote:
> On 5/29/2024 09:14, Ville Syrjälä wrote:
> > On Tue, May 28, 2024 at 04:03:19PM -0500, Mario Limonciello wrote:
> >> If the lid on a laptop is closed when eDP connectors are populated
> >> then it remains enabled when the initial framebuffer configuration
> >> is built.
> >>
> >> When creating the initial framebuffer configuration detect the ACPI
> >> lid status and if it's closed disable any eDP connectors.
> >>
> >> Reported-by: Chris Bainbridge <chris.bainbridge@gmail.com>
> >> Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/3349
> >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> >> ---
> >> Cc: hughsient@gmail.com
> >> v1->v2:
> >>   * Match LVDS as well
> >> ---
> >>   drivers/gpu/drm/drm_client_modeset.c | 30 ++++++++++++++++++++++++++++
> >>   1 file changed, 30 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c
> >> index 31af5cf37a09..0b0411086e76 100644
> >> --- a/drivers/gpu/drm/drm_client_modeset.c
> >> +++ b/drivers/gpu/drm/drm_client_modeset.c
> >> @@ -8,6 +8,7 @@
> >>    */
> >>   
> >>   #include "drm/drm_modeset_lock.h"
> >> +#include <acpi/button.h>
> >>   #include <linux/module.h>
> >>   #include <linux/mutex.h>
> >>   #include <linux/slab.h>
> >> @@ -257,6 +258,34 @@ static void drm_client_connectors_enabled(struct drm_connector **connectors,
> >>   		enabled[i] = drm_connector_enabled(connectors[i], false);
> >>   }
> >>   
> >> +static void drm_client_match_edp_lid(struct drm_device *dev,
> >> +				     struct drm_connector **connectors,
> >> +				     unsigned int connector_count,
> >> +				     bool *enabled)
> >> +{
> >> +	int i;
> >> +
> >> +	for (i = 0; i < connector_count; i++) {
> >> +		struct drm_connector *connector = connectors[i];
> >> +
> >> +		switch (connector->connector_type) {
> >> +		case DRM_MODE_CONNECTOR_LVDS:
> >> +		case DRM_MODE_CONNECTOR_eDP:
> >> +			if (!enabled[i])
> >> +				continue;
> >> +			break;
> >> +		default:
> >> +			continue;
> >> +		}
> >> +
> >> +		if (!acpi_lid_open()) {
> >> +			drm_dbg_kms(dev, "[CONNECTOR:%d:%s] lid is closed, disabling\n",
> >> +				    connector->base.id, connector->name);
> >> +			enabled[i] = false;
> >> +		}
> >> +	}
> >> +}
> > 
> > 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.

> 
> 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.

> 
> 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.

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...


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.

-- 
Ville Syrjälä
Intel