Received-SPF: pass (google.com: domain of linux-kernel+bounces-67447-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <b117ccc0-6569-4753-9cb9-c304f9e50d30@amd.com>
Date: Thu, 15 Feb 2024 12:54:54 -0500
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: Mario Limonciello <mario.limonciello@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>
From: Harry Wentland <harry.wentland@amd.com>
In-Reply-To: <0b94b25a-9ffa-41a5-b931-ad84e1892d36@amd.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?a2w4ZGZEN0JLK2Zpc3VQejBLWiszWSswd0d3cmFETWVaempRTGtHRjBmdDNU?=
 =?utf-8?B?TzAzMjg3S2tYeUZkWW1ZZUZQMG02djhJT3ZsMXh0VHYvRUxyKzRvY3VwdFNQ?=
 =?utf-8?B?cTRncHFMZ3ZWcXFhVkpSeXhuL0N6b3owa1pOMFN1eU1zQjJFRlFjWjlLck5i?=
 =?utf-8?B?YzYvVXJUSEVEb3ZwM1FKd3BMZ3dvS2hNZkJOcmFlRXd0bmVMdjN1KzhCQzBr?=
 =?utf-8?B?UFAyaGN6Yjd6bnpXVmY3QmhmS3ZMcGhITkhrT1BlejhLMHZXS0ZBVTU0dXFj?=
 =?utf-8?B?RktKRGd4VytLQ0JRZTRXbGdVRG1rVVZabDBCaFJ2Wm1FSXNzK3FxbkJUdk40?=
 =?utf-8?B?eUowQnlhS05maVJhUXMzZWJJTTZzVFBlNzRQZzBoNUNqN2tKT0dXbFF3K2JP?=
 =?utf-8?B?di9wVXZCSjFxT2VHUnZrQzBJd09PbjRvVUxsZ0lSR3Z0MzhHdDJ2M1RvSHZY?=
 =?utf-8?B?R2tnVFIxRnNLT3cwWmQwSmZGcWIvazRzcVpZTTZtY1lUdmZrVWVqRWZrb0RL?=
 =?utf-8?B?NzBZWVJpVmlhaU5ENVJNWGJiYkxUYUJxU3ZDM3dDeVp0TnZNc05NTVE0bmdX?=
 =?utf-8?B?Uk1IK29hejJhSHM5N2lLdXlaWG9GQ2QrZVVLTG9raFE3KzhJdnVzQmhGb0xU?=
 =?utf-8?B?QmJ3VnVRY1o2dkYzaXdoT0V0a0NTeVFUY2YwbVY0Wlk0Mno1ZEprSHNYeEFD?=
 =?utf-8?B?d3ErdUNjaHB5VEtkL2hyY0k2U0JQWnZDRytTbVBzMDBteDI3Mjk4cmYvMjVu?=
 =?utf-8?B?TElqU3lTZTBCWkR5QzlBSWJTZ29mdzBpeTdBSU80TE9kcXZLNFIzUDV6eVNo?=
 =?utf-8?B?dmFJQkxRRzE4UFhxR2xWZG03ZStIVWJKejF5OGh3YjgxOURXUkNtQldRUEti?=
 =?utf-8?B?L2EyUXRjWXVST2piZm5tU1Mya0dic2F4S3p4SUJZeVF0eGt6ZVdyNCtDeXJy?=
 =?utf-8?B?Q2ZZRnRHU3M0cjdaMzBzYkNOMllsZlpPNDViWHlZN2E0bmRrWk5yTUl5Mm1N?=
 =?utf-8?B?THhTN0xINTh1c2l0dTNJMk43aFNuUjlVWnVnSGhDWk1OMmY3MndqNmYwOHN0?=
 =?utf-8?B?cVM1SjlTRFFCRGY2NnlrSTFnQjdhaGFSb1B1UEVhZWhYNHdxeEpnTWRFMkli?=
 =?utf-8?B?MHVpM0FKV1YxdTlBeGtGMmVkOVhVdHNESFhqMEc3WDRmU0ozMEhSQStHOEl3?=
 =?utf-8?B?QWI5UU00Z3NkTjhFK1VEc1VkNlp6R0djY2VnenRMTlJFcUNaMW0xTHl6enNs?=
 =?utf-8?B?Rzh6eFlreGE4RERjUTJ3V2JnMDFkWHpMbzhuc2E5M1B0SHFZelJaZ05QVHF5?=
 =?utf-8?B?b0x3c3ZiYkJCS2IrbTB5Z1Zjd3VLb04vK2xzMjZwL1hKTWxVQXZmQ0JiOGVY?=
 =?utf-8?B?bEVBcFJDSnBrRys2Szg4VjloYThkV1gxdmNidldHV2F1cVRxcWk4TWpMdm1D?=
 =?utf-8?B?NHl3RFhKaXgrUzVjV1lUMSttT0x1bGZwS0tGVjNBWEVtRTdHaW9mMnhCSW51?=
 =?utf-8?B?c0wrV1pGMGt1eWxYTXprL2FqR3dRY2xZdy9lVWRMMmhZRG9weFlPU08xaHc3?=
 =?utf-8?B?RWJNRGVINzlaNTNGWE05a1ZmWVp4VTRDc3BHaXhWWFhIU2xBVjdLNVJqVndx?=
 =?utf-8?B?MEpnY1h2eTIzRWVnelFMY0g1MW4rTWxNb1dPQzRCbEJlZWVqbnJYWldYcVla?=
 =?utf-8?B?aERodUNRTml2aG1EczlWUzdjeWlmcERreVpHVmFHZVEzbEpkbTBmWVRFbnBP?=
 =?utf-8?B?Vys5eWpDWGJZNnZaYkJRTmFaVjA3VU1BendqdjliRmRjdllNQ2RLN1dEVzBa?=
 =?utf-8?B?UFNtbTBWenRNQlluVkkweHlSMlpGOTVYKzkwYXEzaHI3dkU0eFkvbTdDQTVD?=
 =?utf-8?B?dTJWZW5jc1pncElmVktDVXhpTUtOaytWRlk2bC85VEFSVEZpRkJTcmtLT2FE?=
 =?utf-8?B?ZXlNaG5VRVRlUHU2TnM0eWNJcjJYZWptaHhhZ2ZVU2JralZGOEkzQzhXb2Rv?=
 =?utf-8?B?M0JaSFAvYWtCRW5QWjVLVHRzSHBScGlwQ3labHJDMUlCZTBHdFliWVRYTkk2?=
 =?utf-8?B?UXd5b3ZlMzdTTU9mV3RIZE9zQmwwR1V4cEgxc2pLZTVXQkpxVllSYTEwMWlB?=
 =?utf-8?Q?DwkL/cRspbVbPshzA5S2LR8eg?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: ebf1b3dd-963e-4c77-bd54-08dc2e4f39fa
X-MS-Exchange-CrossTenant-AuthSource: CO6PR12MB5427.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Feb 2024 17:54:59.0945
 (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: RUgUzL3c0rCSRvunOLd3imn/xh4Wk/qTVF3pANFOq74fbYuYcoXzYa9Kxey8IUuxyXb8TThyIXcArN1yJXN7Kg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY8PR12MB8411

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'm also not sure a new sysfs is a good candidate for drm-fixes.

Harry

>> Cc: Mario Limonciello <mario.limonciello@amd.com>
>> Signed-off-by: Hamza Mahfooz <hamza.mahfooz@amd.com>
> 
> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> Tested-by: Mario Limonciello <mario.limonciello@amd.com>
> 
>> ---
>> v2: hide ABM_LEVEL_IMMEDIATE_DISABLE in the read case, force an atomic
>>      commit when setting the value, call sysfs_remove_group() in
>>      amdgpu_dm_connector_unregister() and add some documentation.
>> ---
>>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 76 +++++++++++++++++++
>>   1 file changed, 76 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> index 8590c9f1dda6..3c62489d03dc 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -6436,10 +6436,79 @@ int amdgpu_dm_connector_atomic_get_property(struct drm_connector *connector,
>>       return ret;
>>   }
>>   +/**
>> + * DOC: panel power savings
>> + *
>> + * The display manager allows you to set your desired **panel power savings**
>> + * level (between 0-4, with 0 representing off), e.g. using the following::
>> + *
>> + *   # echo 3 > /sys/class/drm/card0-eDP-1/amdgpu/panel_power_savings
>> + *
>> + * Modifying this value can have implications on color accuracy, so tread
>> + * carefully.
>> + */
>> +
>> +static ssize_t panel_power_savings_show(struct device *device,
>> +                    struct device_attribute *attr,
>> +                    char *buf)
>> +{
>> +    struct drm_connector *connector = dev_get_drvdata(device);
>> +    struct drm_device *dev = connector->dev;
>> +    u8 val;
>> +
>> +    drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>> +    val = to_dm_connector_state(connector->state)->abm_level ==
>> +        ABM_LEVEL_IMMEDIATE_DISABLE ? 0 :
>> +        to_dm_connector_state(connector->state)->abm_level;
>> +    drm_modeset_unlock(&dev->mode_config.connection_mutex);
>> +
>> +    return sysfs_emit(buf, "%u\n", val);
>> +}
>> +
>> +static ssize_t panel_power_savings_store(struct device *device,
>> +                     struct device_attribute *attr,
>> +                     const char *buf, size_t count)
>> +{
>> +    struct drm_connector *connector = dev_get_drvdata(device);
>> +    struct drm_device *dev = connector->dev;
>> +    long val;
>> +    int ret;
>> +
>> +    ret = kstrtol(buf, 0, &val);
>> +
>> +    if (ret)
>> +        return ret;
>> +
>> +    if (val < 0 || val > 4)
>> +        return -EINVAL;
>> +
>> +    drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>> +    to_dm_connector_state(connector->state)->abm_level = val ?:
>> +        ABM_LEVEL_IMMEDIATE_DISABLE;
>> +    drm_modeset_unlock(&dev->mode_config.connection_mutex);
>> +
>> +    drm_kms_helper_hotplug_event(dev);
>> +
>> +    return count;
>> +}
>> +
>> +static DEVICE_ATTR_RW(panel_power_savings);
>> +
>> +static struct attribute *amdgpu_attrs[] = {
>> +    &dev_attr_panel_power_savings.attr,
>> +    NULL
>> +};
>> +
>> +static const struct attribute_group amdgpu_group = {
>> +    .name = "amdgpu",
>> +    .attrs = amdgpu_attrs
>> +};
>> +
>>   static void amdgpu_dm_connector_unregister(struct drm_connector *connector)
>>   {
>>       struct amdgpu_dm_connector *amdgpu_dm_connector = to_amdgpu_dm_connector(connector);
>>   +    sysfs_remove_group(&connector->kdev->kobj, &amdgpu_group);
>>       drm_dp_aux_unregister(&amdgpu_dm_connector->dm_dp_aux.aux);
>>   }
>>   @@ -6541,6 +6610,13 @@ amdgpu_dm_connector_late_register(struct drm_connector *connector)
>>           to_amdgpu_dm_connector(connector);
>>       int r;
>>   +    if (connector->connector_type == DRM_MODE_CONNECTOR_eDP) {
>> +        r = sysfs_create_group(&connector->kdev->kobj,
>> +                       &amdgpu_group);
>> +        if (r)
>> +            return r;
>> +    }
>> +
>>       amdgpu_dm_register_backlight_device(amdgpu_dm_connector);
>>         if ((connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort) ||
>