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@xxxxxxxxx> > > >> Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/3349 > > >> Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx> > > >> --- > > >> Cc: hughsient@xxxxxxxxx > > >> 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
