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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Wed, 29 May 2024 11:26:58 -0500
From: Mario Limonciello <mario.limonciello@....com>
To: Ville Syrjälä <ville.syrjala@...ux.intel.com>
Cc: dri-devel@...ts.freedesktop.org, amd-gfx@...ts.freedesktop.org,
 Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
 David Airlie <airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>,
 linux-kernel@...r.kernel.org, Chris Bainbridge <chris.bainbridge@...il.com>,
 hughsient@...il.com
Subject: Re: [PATCH v2] drm/client: Detect when ACPI lid is closed during
 initialization


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