Received-SPF: pass (google.com: domain of linux-kernel+bounces-194542-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Wed, 29 May 2024 20:55:32 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
To: Ville =?utf-8?B?U3lyasOkbMOk?= <ville.syrjala@linux.intel.com>
Cc: Mario Limonciello <mario.limonciello@amd.com>, 
	dri-devel@lists.freedesktop.org, amd-gfx@lists.freedesktop.org, 
	Maarten Lankhorst <maarten.lankhorst@linux.intel.com>, Maxime Ripard <mripard@kernel.org>, 
	Thomas Zimmermann <tzimmermann@suse.de>, David Airlie <airlied@gmail.com>, 
	Daniel Vetter <daniel@ffwll.ch>, linux-kernel@vger.kernel.org, 
	Chris Bainbridge <chris.bainbridge@gmail.com>, hughsient@gmail.com
Subject: Re: [PATCH v2] drm/client: Detect when ACPI lid is closed during
 initialization
Message-ID: <g34f3sdk22grheq2vaaonkl543dtk7nb5sffqgmkl5ywtj5skk@p5ht5ug33q4z>
References: <20240528210319.1242-1-mario.limonciello@amd.com>
 <Zlc4V1goFvU2antl@intel.com>
 <197d195f-9206-41dd-8ff1-f4bb4988fb9b@amd.com>
 <ZldMKZ1MzSDXOheJ@intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <ZldMKZ1MzSDXOheJ@intel.com>

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@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:

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