diff mbox series

[2/2] Revert "drm/amd/display: add panel_power_savings sysfs entry to eDP connectors"

Message ID 20240806184214.224672-2-sebastian.wick@redhat.com (mailing list archive)
State New, archived
Headers show
Series [1/2] Revert "drm/amd/display: Don't register panel_power_savings on OLED panels" | expand

Commit Message

Sebastian Wick Aug. 6, 2024, 6:42 p.m. UTC 
From: Sebastian Wick <sebastian@sebastianwick.net>

This reverts commit 63d0b87213a0ba241b3fcfba3fe7b0aed0cd1cc5.

The panel_power_savings sysfs entry can be used to change the displayed
colorimetry which breaks color managed setups.

The "do not break userspace" rule which was violated here is enough
reason to revert this commit.

The bigger problem is that this feature is part of the display chain
which is supposed to be controlled by KMS. This sysfs entry bypasses the
DRM master process and splits control to two independent processes which
do not know about each other. This is what caused the broken user space.
It also causes modesets which can be extremely confusing for the DRM
master process, causing unexpected timings.

We should in general not allow anything other than KMS to control the
display path. If we make an exception to this rule, this must be first
discussed on dri-devel with all the stakeholders approving the
exception.

This has not happened which is the second reason to revert this commit.

Signed-off-by: Sebastian Wick <sebastian.wick@redhat.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 80 -------------------
 1 file changed, 80 deletions(-)

Comments

Mario Limonciello Aug. 7, 2024, 5:13 a.m. UTC | #1 
On 8/6/24 13:42, Sebastian Wick wrote:
> From: Sebastian Wick <sebastian@sebastianwick.net>
> 
> This reverts commit 63d0b87213a0ba241b3fcfba3fe7b0aed0cd1cc5.
> 
> The panel_power_savings sysfs entry can be used to change the displayed
> colorimetry which breaks color managed setups.
> 
> The "do not break userspace" rule which was violated here is enough
> reason to revert this commit.
> 
> The bigger problem is that this feature is part of the display chain
> which is supposed to be controlled by KMS. This sysfs entry bypasses the
> DRM master process and splits control to two independent processes which
> do not know about each other. This is what caused the broken user space.
> It also causes modesets which can be extremely confusing for the DRM
> master process, causing unexpected timings.
> 
> We should in general not allow anything other than KMS to control the
> display path. If we make an exception to this rule, this must be first
> discussed on dri-devel with all the stakeholders approving the
> exception.
> 
> This has not happened which is the second reason to revert this commit.
> 
> Signed-off-by: Sebastian Wick <sebastian.wick@redhat.com>

For anyone who hasn't seen it, there has been a bunch of discussions 
that have transpired on this topic and what to do about it on [1] as 
well as some other linked places on that bug.

Also FWIW there was a discussion on the merits of the sysfs file on 
dri-devel during the initial patch submission [2].

If this revert ends up going through, please also revert 
0887054d14ae23061e28e28747cdea7e40be9224 in the same series so the 
feature can "at least" be accessed by the compositor and changed at 
runtime like the sysfs file had allowed.

[1] https://gitlab.freedesktop.org/upower/power-profiles-daemon/-/issues/159
[2] 
https://lore.kernel.org/dri-devel/20240202152837.7388-1-hamza.mahfooz@amd.com/

> ---
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 80 -------------------
>   1 file changed, 80 deletions(-)
> 
> diff --git ./drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c ../drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 4d4c75173fc3..16c9051d9ccf 100644
> --- ./drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ ../drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -6772,82 +6772,10 @@ 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);
>   
> -	if (connector->connector_type == DRM_MODE_CONNECTOR_eDP &&
> -	    amdgpu_dm_abm_level < 0)
> -		sysfs_remove_group(&connector->kdev->kobj, &amdgpu_group);
> -
>   	drm_dp_aux_unregister(&amdgpu_dm_connector->dm_dp_aux.aux);
>   }
>   
> @@ -6952,14 +6880,6 @@ amdgpu_dm_connector_late_register(struct drm_connector *connector)
>   		to_amdgpu_dm_connector(connector);
>   	int r;
>   
> -	if (connector->connector_type == DRM_MODE_CONNECTOR_eDP &&
> -	    amdgpu_dm_abm_level < 0) {
> -		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) ||
Leo Li Aug. 7, 2024, 1:52 p.m. UTC | #2 
On 2024-08-07 01:13, Mario Limonciello wrote:
> On 8/6/24 13:42, Sebastian Wick wrote:
>> From: Sebastian Wick <sebastian@sebastianwick.net>
>>
>> This reverts commit 63d0b87213a0ba241b3fcfba3fe7b0aed0cd1cc5.
>>
>> The panel_power_savings sysfs entry can be used to change the displayed
>> colorimetry which breaks color managed setups.
>>
>> The "do not break userspace" rule which was violated here is enough
>> reason to revert this commit.

Hi Sebastian,

The default pane_power_savings sysfs value is 0, which is ABM disabled, so it
wouldn't break colorimetry *by default*. It would break colorimetry only if set
to a non-zero value by the user, or something in userspace.

That said, this sysfs opens a door to "break" colorimetry. But if we make it
such that it can only be set in a user-aware way, then the user can decide
whether they want color accuracy, or power. I don't think that's anything new.
User already decide between setting max backlight for better color, or lower
backlight for better battery. Setting max cpu freq, or capping it. Either can be
seen as breaking.

So I think the issue is really in ppd (power profiles daemon), which afaik is
the only user of this sysfs. It sets a non-zero value by default without the
user being aware.

>>
>> The bigger problem is that this feature is part of the display chain
>> which is supposed to be controlled by KMS. This sysfs entry bypasses the
>> DRM master process and splits control to two independent processes which
>> do not know about each other. This is what caused the broken user space.
>> It also causes modesets which can be extremely confusing for the DRM
>> master process, causing unexpected timings.
>>
>> We should in general not allow anything other than KMS to control the
>> display path. If we make an exception to this rule, this must be first
>> discussed on dri-devel with all the stakeholders approving the
>> exception.
>>
>> This has not happened which is the second reason to revert this commit.

I also agree that ABM/backlight related things that affect colorimetry should be
under KMS control. However, from the way things are going, getting there will
take a while, and an interim solution is desired.

We (mostly Mario) proposed a KMS connector property to limit control to KMS as a
way to improve on the sysfs, but that needs more work with compositor folks.
After that, each compositor will need to pipe it through to users. So I think
having something like ppd provide generic support is beneficial in the interim.
It just needs to be 100% opt in.

If we decide to revert this, I think it's worth noting that ABM will be out of
reach for users for a very long while.

On the modeset thing, it's not clear to me why ABM needs a modeset, and I wonder
if it's really necessary. I'll go and find out.

Thanks,
Leo

>>
>> Signed-off-by: Sebastian Wick <sebastian.wick@redhat.com>
> 
> For anyone who hasn't seen it, there has been a bunch of discussions that have 
> transpired on this topic and what to do about it on [1] as well as some other 
> linked places on that bug.
> 
> Also FWIW there was a discussion on the merits of the sysfs file on dri-devel 
> during the initial patch submission [2].
> 
> If this revert ends up going through, please also revert 
> 0887054d14ae23061e28e28747cdea7e40be9224 in the same series so the feature can 
> "at least" be accessed by the compositor and changed at runtime like the sysfs 
> file had allowed.
> 
> [1] https://gitlab.freedesktop.org/upower/power-profiles-daemon/-/issues/159
> [2] https://lore.kernel.org/dri-devel/20240202152837.7388-1-hamza.mahfooz@amd.com/
> 
>> ---
>>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 80 -------------------
>>   1 file changed, 80 deletions(-)
>>
>> diff --git ./drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
>> ../drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> index 4d4c75173fc3..16c9051d9ccf 100644
>> --- ./drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ ../drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -6772,82 +6772,10 @@ 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);
>> -    if (connector->connector_type == DRM_MODE_CONNECTOR_eDP &&
>> -        amdgpu_dm_abm_level < 0)
>> -        sysfs_remove_group(&connector->kdev->kobj, &amdgpu_group);
>> -
>>       drm_dp_aux_unregister(&amdgpu_dm_connector->dm_dp_aux.aux);
>>   }
>> @@ -6952,14 +6880,6 @@ amdgpu_dm_connector_late_register(struct drm_connector 
>> *connector)
>>           to_amdgpu_dm_connector(connector);
>>       int r;
>> -    if (connector->connector_type == DRM_MODE_CONNECTOR_eDP &&
>> -        amdgpu_dm_abm_level < 0) {
>> -        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) ||
>
diff mbox series

Patch

diff --git ./drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c ../drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 4d4c75173fc3..16c9051d9ccf 100644
--- ./drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ ../drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -6772,82 +6772,10 @@  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);
 
-	if (connector->connector_type == DRM_MODE_CONNECTOR_eDP &&
-	    amdgpu_dm_abm_level < 0)
-		sysfs_remove_group(&connector->kdev->kobj, &amdgpu_group);
-
 	drm_dp_aux_unregister(&amdgpu_dm_connector->dm_dp_aux.aux);
 }
 
@@ -6952,14 +6880,6 @@  amdgpu_dm_connector_late_register(struct drm_connector *connector)
 		to_amdgpu_dm_connector(connector);
 	int r;
 
-	if (connector->connector_type == DRM_MODE_CONNECTOR_eDP &&
-	    amdgpu_dm_abm_level < 0) {
-		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) ||