linux-kernel - Re: [PATCH v2] drm/amd/display: add panel_power

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]

Message-ID: <a1d2749b-8db5-46d1-bf60-7820902cfc8f@amd.com>
Date: Thu, 15 Feb 2024 12:15:08 -0600
From: Mario Limonciello <mario.limonciello@....com>
To: Harry Wentland <harry.wentland@....com>,
 Hamza Mahfooz <hamza.mahfooz@....com>, amd-gfx@...ts.freedesktop.org
Cc: Leo Li <sunpeng.li@....com>, Rodrigo Siqueira <Rodrigo.Siqueira@....com>,
 Alex Deucher <alexander.deucher@....com>,
 Christian König <christian.koenig@....com>,
 "Pan, Xinhui" <Xinhui.Pan@....com>, David Airlie <airlied@...il.com>,
 Daniel Vetter <daniel@...ll.ch>, Alex Hung <alex.hung@....com>,
 Srinivasan Shanmugam <srinivasan.shanmugam@....com>,
 Wayne Lin <wayne.lin@....com>, dri-devel@...ts.freedesktop.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] drm/amd/display: add panel_power_savings sysfs entry
 to eDP connectors

On 2/15/2024 11:54, Harry Wentland wrote:
> On 2024-02-02 11:20, Mario Limonciello wrote:
>> On 2/2/2024 09:28, Hamza Mahfooz wrote:
>>> We want programs besides the compositor to be able to enable or disable
>>> panel power saving features. However, since they are currently only
>>> configurable through DRM properties, that isn't possible. So, to remedy
>>> that issue introduce a new "panel_power_savings" sysfs attribute.
>>>
> 
> I've been trying to avoid looking at this too closely, partly because
> I want ABM enablement by default, with control for users. But the
> more I think about this the more uncomfortable I get. The key for my
> discomfort is that we're going around the back of DRM master to set
> a DRM property. This is apt to create lots of weird behaviors,
> especially if compositors also decide to implement support for the
> abm_level property and then potentially fight PPD, or other users
> of this sysfs.

I feel the problem is that specifically both DRM master and sysfs can 
simultaneously fight over the value for ABM.

This isn't a totally new problem because previously the user and DRM
master could fight over this (with the user setting it on command line
and DRM master overriding that).  That was fixed in a follow up patch
though in that the DRM property isn't attached if a user sets the value
on the command line.

I feel the solution to these concerns is that we should make a knob that 
controls whether the DRM property is created or the sysfs file is 
created but not let them happen simultaneously.

We already have amdgpu.abmlevel=-1 is the only time sysfs is created. 
I'd say we should drop the drm property in that case and add a case for 
amdgpu.abmlevel=-2 which will only make the drm property and not the 
sysfs value.  With that done it turns into:

-2  : DRM master is in control
-1  : sysfs is in control.  Software like PPD will tune it as needed.
0-4 : User is in control.