Received-SPF: pass (google.com: domain of linux-kernel+bounces-194439-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <1f8ccefa-204f-4477-814f-4a1dfbbe6741@amd.com>
Date: Wed, 29 May 2024 11:26:58 -0500
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2] drm/client: Detect when ACPI lid is closed during
 initialization
To: =?UTF-8?B?VmlsbGUgU3lyasOkbMOk?= <ville.syrjala@linux.intel.com>
Cc: 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
References: <20240528210319.1242-1-mario.limonciello@amd.com>
 <Zlc4V1goFvU2antl@intel.com> <197d195f-9206-41dd-8ff1-f4bb4988fb9b@amd.com>
 <ZldMKZ1MzSDXOheJ@intel.com>
Content-Language: en-US
From: Mario Limonciello <mario.limonciello@amd.com>
In-Reply-To: <ZldMKZ1MzSDXOheJ@intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?dHROUjRzMCtmMTdKMjhhS2pQcVFXd3I0UXZVWHJJbzlXYlBqemhXbVpxaWlh?=
 =?utf-8?B?dXE0RmNURzhZQ2lnNXN3WXZvcnJiNjZWcUt4V3pxYmZCMHFFYTVTYWRjQTVS?=
 =?utf-8?B?L2MvUmtwQ0prWTd3ODJRTnpPWkhaU045UkZsNXM4Yy9BSW1wRXdQek9YaU5N?=
 =?utf-8?B?MDdEaUhHbVBHRHpvdjJRbEcxYUc3Ylh6eEhlRTI0OWlkYjFGWmErcEhsNDdM?=
 =?utf-8?B?dzFQSWNQVGF5YjI0eEswY29oSVhGMHhuSVRWZW5lMEEvWkN3bEMwT1FaRi9V?=
 =?utf-8?B?bjcwelZQc1pzYk45bVhsbjVNWHVrd2Jnc1VlRzNQYTRYdGp2eVo1WENFN1kw?=
 =?utf-8?B?b3pybXZ1bUM3Z1RjZXJiOXJmOFVvVjRYWlNCQjQ5Y04zQno2YWlzdmVmajBD?=
 =?utf-8?B?QVJpZkhYTys1ZkJuL1BJTENVdjFoaHlmbmZRNTYvbFhObDN2Q0prWGVCQ2Mr?=
 =?utf-8?B?UXhQcGx4RFZyNjN1NnJVd3RiTVZhdUEzT1RXZUtUelNTV3FMWURVTStBVnZv?=
 =?utf-8?B?cHpHQlQzNzhuS09HTWgxT28xMUNmcUlkUjNzWitPVEd0cm9ZYWhzMjR6dXpW?=
 =?utf-8?B?UjVOOE52MlA3bzJpd1dZN0tMWko1QVJJeHhZQ000bUZuR0dQc2VPZ2E0VjRD?=
 =?utf-8?B?NTFVRTV3TkZZZEhJQ21NcHJXUjUwd0dWNEZRMytyTXFmaWk1ZVFFQ1N2Qmxr?=
 =?utf-8?B?RXZkMFlqd1lUeWJtZ2htZmtMZ1hxd2hvanpJY21pUFM2cVlEb0NpdXZaMTg5?=
 =?utf-8?B?RU5SVzk4cWNwdkF2c3RoR2xQREdrNDBWclZLQUZCb2tNM0hoS0FvRHJtN3NJ?=
 =?utf-8?B?Z3BXNTRkTXZ5R3FUeTE3N3QrM0daNVNYVmVRSjJScmVZU0dUY1RXNlY3OFFQ?=
 =?utf-8?B?cE43WktUMWFzOTM3WmpWdVplQWtQbExaMGdBVHBsTHNmTnpMMXhXblpZdTl2?=
 =?utf-8?B?MHVwVnAvMWE0R09ON01PQnJOMmRJVkwvS0hOK3JoWDNJNUlPK2RJQUpYejZt?=
 =?utf-8?B?NE9OTVF5SXdsVUk2SGlkUFljcFNwQlhYTXRwei9VOVFtRC80U05VVFhVT3Yz?=
 =?utf-8?B?clJxOEVtMW5SZGZ0NklxdExFVFJ1U09qMkJXUXJ1aEt6MDJqWUZNcWpwcm5T?=
 =?utf-8?B?S3VpdkVQSnVvb2k0d2N4QkE2ejhoNitGUU1LdEsvK1pUSWtDeVBhZHZoenlk?=
 =?utf-8?B?ZEd5dkZOeGxsV1Z3anhYa2pCRGpjdHRGaWhJMDI1L3JOc1VPNlhRcEFVTWpH?=
 =?utf-8?B?NTc2eUZHN3diOFpzM0VZb1JqUTR5dDBESEI2TlZEeXcrLzE4d1I3QUUxbG1t?=
 =?utf-8?B?c2dyZndxd2hWNTVwa2hCV3FTSE5rNzA1eHdzcUJEVUVYRXJCcHBnTXdXdTcz?=
 =?utf-8?B?cGRkVHRYZURRcS9JQ3ZRa0p3dmJ4Uzk5SCtLdy80THZCTkw2cFJQeFVFcVVK?=
 =?utf-8?B?dFh0YThOdDU4SnNPYkZlWVVKaW1jUVN4NlEyTXRoN0o2WFprRDRnOWF2V0pS?=
 =?utf-8?B?WHRmMGdobzVwMW1ENEx4WEhDNFFQR1drNUhkbjF6RlliRnRQVnJDR1hHbG5E?=
 =?utf-8?B?MUE5cklYSkRLNVc4bUdKWTRmNWw2dlhxLzJnVG9TNUI5MDFlQmZJKzFHY05j?=
 =?utf-8?B?a21uL2pWeUN3WlcxZlpRWFlQdCtscTUyQ2dFY3ZZaStRRTBLbnUwUkp4a3gv?=
 =?utf-8?B?ZS94c2NwOGdKNkFSeDhLa095b2ozZ0lkNzUwUVcvZk91UTMxdExxR0ZVZVJQ?=
 =?utf-8?B?SURxeGJxaTNvTU1nZEttRDRIWWhJYWRuVE9nKzF5aVJxeHFNL2hxaCtKQk1J?=
 =?utf-8?B?dXhvc2lpNWo1dysyR0xSMkE4VWx0MThpdXV0c01VS0ROUW8vKzJyMWplQzRG?=
 =?utf-8?B?SmtDTVZFN0RqaEZPdGNwT3lpTjlCUC9HVlpSaVc0SXA2S0JJUXJRQzQ1USsr?=
 =?utf-8?B?emJCNjNRdldUdlBmMHQ3bEtjTG1wVmlrdEpxVkc4TUQ1cm5OUnEweEpmcEdZ?=
 =?utf-8?B?THpGZjlhc1pETXlya09hNnhtekozNlZwOHBnVzRwc0NPb0NVa0x6R3NXdjMy?=
 =?utf-8?B?czJQOXozZ2g5eWg2dHgwcGdOTUtxYXRrQ2IzTzltUVcyemIwS0tqeFFVZ0dZ?=
 =?utf-8?Q?99OqamNGHiw2r64m0G3m0Y6iE?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: dab408f2-08ea-4119-bcb0-08dc7ffc2a98
X-MS-Exchange-CrossTenant-AuthSource: MN0PR12MB6101.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 May 2024 16:27:00.4543
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: jR8o9Z/YcgQjAp0exN2trRsG3eXaRT/RLRJ3xOx6a9FHyr/7OUwKmjmjaKN1ss8wK6DpNYhamXszOXNL63XdLg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR12MB8311


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