Received-SPF: pass (google.com: domain of linux-kernel+bounces-67481-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <a1d2749b-8db5-46d1-bf60-7820902cfc8f@amd.com>
Date: Thu, 15 Feb 2024 12:15:08 -0600
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2] drm/amd/display: add panel_power_savings sysfs entry
 to eDP connectors
Content-Language: en-US
To: Harry Wentland <harry.wentland@amd.com>,
 Hamza Mahfooz <hamza.mahfooz@amd.com>, amd-gfx@lists.freedesktop.org
Cc: Leo Li <sunpeng.li@amd.com>, Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>,
 Alex Deucher <alexander.deucher@amd.com>,
 =?UTF-8?Q?Christian_K=C3=B6nig?= <christian.koenig@amd.com>,
 "Pan, Xinhui" <Xinhui.Pan@amd.com>, David Airlie <airlied@gmail.com>,
 Daniel Vetter <daniel@ffwll.ch>, Alex Hung <alex.hung@amd.com>,
 Srinivasan Shanmugam <srinivasan.shanmugam@amd.com>,
 Wayne Lin <wayne.lin@amd.com>, dri-devel@lists.freedesktop.org,
 linux-kernel@vger.kernel.org
References: <20240202152837.7388-1-hamza.mahfooz@amd.com>
 <0b94b25a-9ffa-41a5-b931-ad84e1892d36@amd.com>
 <b117ccc0-6569-4753-9cb9-c304f9e50d30@amd.com>
From: Mario Limonciello <mario.limonciello@amd.com>
In-Reply-To: <b117ccc0-6569-4753-9cb9-c304f9e50d30@amd.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?cHRnKzVNdytoWTRPSksyaE4yZFR0V3pic0ppeXQ0ZXpqdnhqaDNsR041WStW?=
 =?utf-8?B?dVN6dXVJbWx2NFRJandKa21KRGVEZDJlN2V6QmV1aFpybmI3akZaeTBJdDFL?=
 =?utf-8?B?dTNuc2VLQ0xyV3VwSFY3U21OdW5SZXp1NWJ1bzhaTm5KV21pK3dJelBSZGdW?=
 =?utf-8?B?YXRqWW9lbm54ZkZ6VUZqMzBHZjM0QXRFc1ZFRTNzSTNHS1lvQWdBd2RlMGo1?=
 =?utf-8?B?ZFQzTm1xN1kxZkdBejZWVzdzOTV1VzVIUm45eVdkcitNN25GOEZ6NFoxR1d6?=
 =?utf-8?B?QTFvdDROc0l3SEk4Njg5TEtWUkdPb3RzdkpnbWh2UWlIazg4SjlLQ1ZseUhu?=
 =?utf-8?B?UmdzeEZuMUh2ZXNRNTlLbllFWUVMZVpFTE05T3R2UEVPK1J1eG9hWnp3S1Ba?=
 =?utf-8?B?SDUxTGR4eFA5YVQ1MzRlUmtVYVBxampTVCtLWjRTQ2JXQUpBVGFmcVYwV1NK?=
 =?utf-8?B?RzZNdHR4T3B0RlR4dkdpaTZiTWZHaWR6TFlyYmRUU0JMZmNpUVRZc2ZidTZJ?=
 =?utf-8?B?ZjVJamhCL3lYSHB4N3FoRnZLbUl5b3dtR1owYS83akxkMEpVdjNKRXdIN3Rk?=
 =?utf-8?B?U1VpMkkxV0h1YjJBRDdjbUtJa2ZOL0pCT1ZQSTFUQ0IyaVF0ZGxIbUtLSTI5?=
 =?utf-8?B?aDFOM1JEZXJsZjFEemtKL01vSG9ZTUVQY1lWdk02UWNIdzM4SmdHazBpbllP?=
 =?utf-8?B?aTFBQXpiaFVBZ1Bhcklia3ozZWh2QThRV2cwQUg3VUNqOHI3MWJkbm5FTW8y?=
 =?utf-8?B?cHdBNkZhWjRRS1hRSmpIcVh4SlIwYnNjR09yL1ZEM01hSFpFWk9JRWpLN1NB?=
 =?utf-8?B?dzczOWFoR25oUTA5NFRMRE1UcE9Ec0cvUnBWeDNGallBc3VyM1NteHNQM0Vl?=
 =?utf-8?B?K081V0JZZ1UzcElaQllrTWl1OTZpTHlxQS9HZVp0OHpUZS9DSjZ4bmREVDVK?=
 =?utf-8?B?U0FRbDVqM25wcnBsU2V0bDByVm45SHlhbmFxU3o5L3A0QTJXZ1c4Z2UzdjRx?=
 =?utf-8?B?Q2RqVmY2Y0xudm1CcFdoU29vdUtmTE9iMjlCN2VuRDhNejljdkpjVjFBTWdi?=
 =?utf-8?B?V3ZYUGRUM2gxRFBTZGpnczhGZk9KcXozcEkyQk9vamFGR0VUSVk2c1NTMFNv?=
 =?utf-8?B?QkFucWxGNWZ2SjhXd0FFUWsxRmhpTmRkcm96WW5aTDZ2ZmF0YzdaT0VPbGtx?=
 =?utf-8?B?cUVkSE1hR0xCejI0V2grNURSeUphOU5KQXRmSzhRZ2VCNHhuMnYvY1NZbUJr?=
 =?utf-8?B?Mmt3eWh0OUtYRDQ2cStqc1V4QWN5VGdwVFBobkttaUlyMzU4Vk9mSmpqdU9v?=
 =?utf-8?B?WTNzL2Q5Ujc1VHpBUHdJSUtqbjBsMVprY0lyMTFVbldhQkhDaWtWejVWUUh0?=
 =?utf-8?B?V3BoU0dzcHMxYjZZWjF0NUgwbXhxdnBUN1IrYlBVT01Uc0VGTmhIeWFnQ01k?=
 =?utf-8?B?WFo5MnE1dTIrek1INWdKcUxFWEUzZjB0akpTVUNmL3A5TlJ6TlErM25EZFAr?=
 =?utf-8?B?ZGppM3UxVW55NVM1ZjNsSTI3L21ndW1QaXdQQThYUE5IZkczUTBLSGVZWkJB?=
 =?utf-8?B?Rk0reXNPdEVmc3RISlNiTWcrM3pqMGdGaDkzYXhhNUowOFFDUlVnUkVvZUpj?=
 =?utf-8?B?Q3JsN0Y2a0RvcE0xUXM0T2hDNXNHMnNLZ25remZlS3hxeDRENGo5MDl4SFBr?=
 =?utf-8?B?R2FOK3VPYmJwbEtzZ2ZTTDJBQUVGYUR6cWxUOXAydUxGVlJXWU4reDFacVdw?=
 =?utf-8?B?QUxxQVYwOEJ6bEFjMXJsVCtrUU1Ec0NBVmMrRDN4ZTNlRS90cUtOQVd0TW1S?=
 =?utf-8?B?YnJlWktqTU9IUXY3VERQeWxDd20zS1owNGIzbW9BSThEaHpkR282U2plOVNq?=
 =?utf-8?B?QXRlRmFPNXZraHR1L3dHanduK3NNYWRtZ0tUOXllUm05VTg1bytuSm80UXlJ?=
 =?utf-8?B?RllWSmtGb1lrRXNzM3ZBN25keDZnSVpPbFRhdXN3RVBjUERDNFhVQ0FmNGcz?=
 =?utf-8?B?bGNZUzZGZER5WVR4UUhLN0IyZUluRk9EMDdnOFcxTTg4cFo2U09JOVEwejM3?=
 =?utf-8?B?VEpISnR6UjgxMzloVkxNRUtiWjlqa1hDZnE0cFRoL3FIejVlRXBadXdUQ1FK?=
 =?utf-8?Q?UeWl+eoXcW4VZvq0id60nt5lD?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 7ff70b53-29b4-48c7-8409-08dc2e520c44
X-MS-Exchange-CrossTenant-AuthSource: MN0PR12MB6101.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Feb 2024 18:15:10.9545
 (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: Nc7BQZtu7K1wcQNyqW1zmz8UNQt+PRsRN+Ywxo2PWcMWb3J8nVAbR+UPwOvftoxLWtuF78RUaji5DekoQqlbRw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY5PR12MB4196

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.