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

[Dmitry Baryshkov]

On Wed, May 29, 2024 at 06:39:21PM +0300, Ville Syrjälä wrote:
> 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 at gmail.com>
> > >> Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/3349
> > >> Signed-off-by: Mario Limonciello <mario.limonciello at amd.com>
> > >> ---
> > >> Cc: hughsient at 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:

What about DPI or DSI panels? I think they should also be handled in a
similar way. Not sure regarding the SPI.

> > >> +			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;
> > >> +		}
> > >> +	}
> > >> +}
> > > 

[trimmed]

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

Looking at drivers/acpi/button.c, the button driver handles some of the
quirks when posting the data to the input subsystem.

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

Thanks. I was going to comment, but you got it first. I think a proper
implementation should check for SW_LID input device instead of simply
using acpi_lid_open(). This will handle the issue for other,
non-ACPI-based laptops.

-- 
With best wishes
Dmitry