[PATCH] drm/amd: Drop abm_level property

[PATCH] drm/amd: Drop abm_level property

[Mario Limonciello]

This vendor specific property has never been used by userspace
software and conflicts with the panel_power_savings sysfs file.
That is a compositor and user could fight over the same data.

Fixes: f97e4303da16 ("drm/amd/display: add panel_power_savings sysfs entry to eDP connectors")
Suggested-by: Harry Wentland <Harry.Wentland@amd.com>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
--
Cc: Hamza Mahfooz <Hamza.Mahfooz@amd.com>
Cc: Sun peng (Leo) Li <Sunpeng.Li@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_display.c       |  8 --------
 drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h          |  2 --
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 14 --------------
 3 files changed, 24 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
index b8fbe97efe1d..3ecc7ef95172 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
@@ -1350,14 +1350,6 @@ int amdgpu_display_modeset_create_props(struct amdgpu_device *adev)
 					 "dither",
 					 amdgpu_dither_enum_list, sz);
 
-	if (adev->dc_enabled) {
-		adev->mode_info.abm_level_property =
-			drm_property_create_range(adev_to_drm(adev), 0,
-						  "abm level", 0, 4);
-		if (!adev->mode_info.abm_level_property)
-			return -ENOMEM;
-	}
-
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
index 2e4911050cc5..1fe21a70ddd0 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
@@ -324,8 +324,6 @@ struct amdgpu_mode_info {
 	struct drm_property *audio_property;
 	/* FMT dithering */
 	struct drm_property *dither_property;
-	/* Adaptive Backlight Modulation (power feature) */
-	struct drm_property *abm_level_property;
 	/* hardcoded DFP edid from BIOS */
 	struct edid *bios_hardcoded_edid;
 	int bios_hardcoded_edid_size;
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 b9ac3d2f8029..e3b32ffba85a 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -6388,9 +6388,6 @@ int amdgpu_dm_connector_atomic_set_property(struct drm_connector *connector,
 	} else if (property == adev->mode_info.underscan_property) {
 		dm_new_state->underscan_enable = val;
 		ret = 0;
-	} else if (property == adev->mode_info.abm_level_property) {
-		dm_new_state->abm_level = val ?: ABM_LEVEL_IMMEDIATE_DISABLE;
-		ret = 0;
 	}
 
 	return ret;
@@ -6433,10 +6430,6 @@ int amdgpu_dm_connector_atomic_get_property(struct drm_connector *connector,
 	} else if (property == adev->mode_info.underscan_property) {
 		*val = dm_state->underscan_enable;
 		ret = 0;
-	} else if (property == adev->mode_info.abm_level_property) {
-		*val = (dm_state->abm_level != ABM_LEVEL_IMMEDIATE_DISABLE) ?
-			dm_state->abm_level : 0;
-		ret = 0;
 	}
 
 	return ret;
@@ -7652,13 +7645,6 @@ void amdgpu_dm_connector_init_helper(struct amdgpu_display_manager *dm,
 	aconnector->base.state->max_bpc = 16;
 	aconnector->base.state->max_requested_bpc = aconnector->base.state->max_bpc;
 
-	if (connector_type == DRM_MODE_CONNECTOR_eDP &&
-	    (dc_is_dmcu_initialized(adev->dm.dc) ||
-	     adev->dm.dc->ctx->dmub_srv) && amdgpu_dm_abm_level < 0) {
-		drm_object_attach_property(&aconnector->base.base,
-				adev->mode_info.abm_level_property, 0);
-	}
-
 	if (connector_type == DRM_MODE_CONNECTOR_HDMIA) {
 		/* Content Type is currently only implemented for HDMI. */
 		drm_connector_attach_content_type_property(&aconnector->base);
-- 
2.34.1

Re: [PATCH] drm/amd: Drop abm_level property

[Harry Wentland]

On 2024-02-16 10:33, Mario Limonciello wrote:
> This vendor specific property has never been used by userspace
> software and conflicts with the panel_power_savings sysfs file.
> That is a compositor and user could fight over the same data.
> 
> Fixes: f97e4303da16 ("drm/amd/display: add panel_power_savings sysfs entry to eDP connectors")
> Suggested-by: Harry Wentland <Harry.Wentland@amd.com>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> --
> Cc: Hamza Mahfooz <Hamza.Mahfooz@amd.com>
> Cc: Sun peng (Leo) Li <Sunpeng.Li@amd.com>

Reviewed-by: Harry Wentland <harry.wentland@amd.com>

Harry

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_display.c       |  8 --------
>  drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h          |  2 --
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 14 --------------
>  3 files changed, 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> index b8fbe97efe1d..3ecc7ef95172 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> @@ -1350,14 +1350,6 @@ int amdgpu_display_modeset_create_props(struct amdgpu_device *adev)
>  					 "dither",
>  					 amdgpu_dither_enum_list, sz);
>  
> -	if (adev->dc_enabled) {
> -		adev->mode_info.abm_level_property =
> -			drm_property_create_range(adev_to_drm(adev), 0,
> -						  "abm level", 0, 4);
> -		if (!adev->mode_info.abm_level_property)
> -			return -ENOMEM;
> -	}
> -
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> index 2e4911050cc5..1fe21a70ddd0 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> @@ -324,8 +324,6 @@ struct amdgpu_mode_info {
>  	struct drm_property *audio_property;
>  	/* FMT dithering */
>  	struct drm_property *dither_property;
> -	/* Adaptive Backlight Modulation (power feature) */
> -	struct drm_property *abm_level_property;
>  	/* hardcoded DFP edid from BIOS */
>  	struct edid *bios_hardcoded_edid;
>  	int bios_hardcoded_edid_size;
> 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 b9ac3d2f8029..e3b32ffba85a 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -6388,9 +6388,6 @@ int amdgpu_dm_connector_atomic_set_property(struct drm_connector *connector,
>  	} else if (property == adev->mode_info.underscan_property) {
>  		dm_new_state->underscan_enable = val;
>  		ret = 0;
> -	} else if (property == adev->mode_info.abm_level_property) {
> -		dm_new_state->abm_level = val ?: ABM_LEVEL_IMMEDIATE_DISABLE;
> -		ret = 0;
>  	}
>  
>  	return ret;
> @@ -6433,10 +6430,6 @@ int amdgpu_dm_connector_atomic_get_property(struct drm_connector *connector,
>  	} else if (property == adev->mode_info.underscan_property) {
>  		*val = dm_state->underscan_enable;
>  		ret = 0;
> -	} else if (property == adev->mode_info.abm_level_property) {
> -		*val = (dm_state->abm_level != ABM_LEVEL_IMMEDIATE_DISABLE) ?
> -			dm_state->abm_level : 0;
> -		ret = 0;
>  	}
>  
>  	return ret;
> @@ -7652,13 +7645,6 @@ void amdgpu_dm_connector_init_helper(struct amdgpu_display_manager *dm,
>  	aconnector->base.state->max_bpc = 16;
>  	aconnector->base.state->max_requested_bpc = aconnector->base.state->max_bpc;
>  
> -	if (connector_type == DRM_MODE_CONNECTOR_eDP &&
> -	    (dc_is_dmcu_initialized(adev->dm.dc) ||
> -	     adev->dm.dc->ctx->dmub_srv) && amdgpu_dm_abm_level < 0) {
> -		drm_object_attach_property(&aconnector->base.base,
> -				adev->mode_info.abm_level_property, 0);
> -	}
> -
>  	if (connector_type == DRM_MODE_CONNECTOR_HDMIA) {
>  		/* Content Type is currently only implemented for HDMI. */
>  		drm_connector_attach_content_type_property(&aconnector->base);

Re: [PATCH] drm/amd: Drop abm_level property

[Christian König]

Am 16.02.24 um 16:33 schrieb Mario Limonciello:
> This vendor specific property has never been used by userspace
> software and conflicts with the panel_power_savings sysfs file.
> That is a compositor and user could fight over the same data.
>
> Fixes: f97e4303da16 ("drm/amd/display: add panel_power_savings sysfs entry to eDP connectors")
> Suggested-by: Harry Wentland <Harry.Wentland@amd.com>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>

Acked-by: Christian König <christian.koenig@amd.com>

> --
> Cc: Hamza Mahfooz <Hamza.Mahfooz@amd.com>
> Cc: Sun peng (Leo) Li <Sunpeng.Li@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_display.c       |  8 --------
>   drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h          |  2 --
>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 14 --------------
>   3 files changed, 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> index b8fbe97efe1d..3ecc7ef95172 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> @@ -1350,14 +1350,6 @@ int amdgpu_display_modeset_create_props(struct amdgpu_device *adev)
>   					 "dither",
>   					 amdgpu_dither_enum_list, sz);
>   
> -	if (adev->dc_enabled) {
> -		adev->mode_info.abm_level_property =
> -			drm_property_create_range(adev_to_drm(adev), 0,
> -						  "abm level", 0, 4);
> -		if (!adev->mode_info.abm_level_property)
> -			return -ENOMEM;
> -	}
> -
>   	return 0;
>   }
>   
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> index 2e4911050cc5..1fe21a70ddd0 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> @@ -324,8 +324,6 @@ struct amdgpu_mode_info {
>   	struct drm_property *audio_property;
>   	/* FMT dithering */
>   	struct drm_property *dither_property;
> -	/* Adaptive Backlight Modulation (power feature) */
> -	struct drm_property *abm_level_property;
>   	/* hardcoded DFP edid from BIOS */
>   	struct edid *bios_hardcoded_edid;
>   	int bios_hardcoded_edid_size;
> 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 b9ac3d2f8029..e3b32ffba85a 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -6388,9 +6388,6 @@ int amdgpu_dm_connector_atomic_set_property(struct drm_connector *connector,
>   	} else if (property == adev->mode_info.underscan_property) {
>   		dm_new_state->underscan_enable = val;
>   		ret = 0;
> -	} else if (property == adev->mode_info.abm_level_property) {
> -		dm_new_state->abm_level = val ?: ABM_LEVEL_IMMEDIATE_DISABLE;
> -		ret = 0;
>   	}
>   
>   	return ret;
> @@ -6433,10 +6430,6 @@ int amdgpu_dm_connector_atomic_get_property(struct drm_connector *connector,
>   	} else if (property == adev->mode_info.underscan_property) {
>   		*val = dm_state->underscan_enable;
>   		ret = 0;
> -	} else if (property == adev->mode_info.abm_level_property) {
> -		*val = (dm_state->abm_level != ABM_LEVEL_IMMEDIATE_DISABLE) ?
> -			dm_state->abm_level : 0;
> -		ret = 0;
>   	}
>   
>   	return ret;
> @@ -7652,13 +7645,6 @@ void amdgpu_dm_connector_init_helper(struct amdgpu_display_manager *dm,
>   	aconnector->base.state->max_bpc = 16;
>   	aconnector->base.state->max_requested_bpc = aconnector->base.state->max_bpc;
>   
> -	if (connector_type == DRM_MODE_CONNECTOR_eDP &&
> -	    (dc_is_dmcu_initialized(adev->dm.dc) ||
> -	     adev->dm.dc->ctx->dmub_srv) && amdgpu_dm_abm_level < 0) {
> -		drm_object_attach_property(&aconnector->base.base,
> -				adev->mode_info.abm_level_property, 0);
> -	}
> -
>   	if (connector_type == DRM_MODE_CONNECTOR_HDMIA) {
>   		/* Content Type is currently only implemented for HDMI. */
>   		drm_connector_attach_content_type_property(&aconnector->base);

Re: [PATCH] drm/amd: Drop abm_level property

[Xaver Hugl]

Like already mentioned in the power profiles daemon repository, I don't think 
this makes sense. This is a display setting, which compositors have interest 
in controlling, for example to:
- disable it in a bright environment, because afaiu it reduces the maximum 
screen brightness
- disable it when it shows color critical content
- disable it while profiling the display
- enable it when it shows content that's definitely not color critical (based 
on the content-type property of Wayland surfaces)
- enable it as a first step before properly dimming the screen on idle

If the primary concern here is that this hasn't been used by compositors and 
potential power savings aren't being realized, that could be solved by 
providing documentation about what the feature does in the kernel, and by 
sending a mail to wayland-devel describing why it should be used.

If the goal is to implement it in power profiles daemon and not get conflicts, I 
think disabling the property by default and instead enable it + disable the 
sysfs file when a CAP for it is set would make more sense than making the 
listed features impossible.

Re: [PATCH] drm/amd: Drop abm_level property

[Mario Limonciello]

On 3/6/2024 11:08, Xaver Hugl wrote:
> Like already mentioned in the power profiles daemon repository, I don't think
> this makes sense. This is a display setting, which compositors have interest
> in controlling, for example to:
> - disable it in a bright environment, because afaiu it reduces the maximum
> screen brightness
> - disable it when it shows color critical content
> - disable it while profiling the display
> - enable it when it shows content that's definitely not color critical (based
> on the content-type property of Wayland surfaces)
> - enable it as a first step before properly dimming the screen on idle
> 

This specific topic is on the agenda to discuss at 2024 Display Next 
Hackfest.

> If the primary concern here is that this hasn't been used by compositors and
> potential power savings aren't being realized, that could be solved by
> providing documentation about what the feature does in the kernel, and by
> sending a mail to wayland-devel describing why it should be used.
> 
> If the goal is to implement it in power profiles daemon and not get conflicts, I
> think disabling the property by default and instead enable it + disable the
> sysfs file when a CAP for it is set would make more sense than making the
> listed features impossible.
> 
> 

So the idea being if the compositor isn't using it we let 
power-profiles-daemon (or any other software) take control via sysfs and 
if the compositor does want to control it then it then it writes a DRM 
cap and we destroy the sysfs file?

Re: [PATCH] drm/amd: Drop abm_level property

[Xaver Hugl]

Am Mi., 6. März 2024 um 18:19 Uhr schrieb Mario Limonciello
<mario.limonciello@amd.com>:
> So the idea being if the compositor isn't using it we let
> power-profiles-daemon (or any other software) take control via sysfs and
> if the compositor does want to control it then it then it writes a DRM
> cap and we destroy the sysfs file?

Yes. That way still only one party controls it at a given time, and we
can get both good default behavior for display servers that don't care
(like Xorg or compositors without color management support), and
compositors that want to put in the effort can do more specific things
with it.

Re: [PATCH] drm/amd: Drop abm_level property

[Mario Limonciello]

On 3/6/2024 12:00, Xaver Hugl wrote:
> Am Mi., 6. März 2024 um 18:19 Uhr schrieb Mario Limonciello
> <mario.limonciello@amd.com>:
>> So the idea being if the compositor isn't using it we let
>> power-profiles-daemon (or any other software) take control via sysfs and
>> if the compositor does want to control it then it then it writes a DRM
>> cap and we destroy the sysfs file?
> 
> Yes. That way still only one party controls it at a given time, and we
> can get both good default behavior for display servers that don't care
> (like Xorg or compositors without color management support), and
> compositors that want to put in the effort can do more specific things
> with it.

I think that's a very good solution.

Harry, Hamza, what do you guys think?

Re: [PATCH] drm/amd: Drop abm_level property

[Harry Wentland]

On 2024-03-06 13:02, Mario Limonciello wrote:
> On 3/6/2024 12:00, Xaver Hugl wrote:
>> Am Mi., 6. März 2024 um 18:19 Uhr schrieb Mario Limonciello
>> <mario.limonciello@amd.com>:
>>> So the idea being if the compositor isn't using it we let
>>> power-profiles-daemon (or any other software) take control via sysfs and
>>> if the compositor does want to control it then it then it writes a DRM
>>> cap and we destroy the sysfs file?
>>
>> Yes. That way still only one party controls it at a given time, and we
>> can get both good default behavior for display servers that don't care
>> (like Xorg or compositors without color management support), and
>> compositors that want to put in the effort can do more specific things
>> with it.
> 
> I think that's a very good solution.
> 
> Harry, Hamza, what do you guys think?

In theory I like it. But how will this look in practice? Is PPD or compositor
on the scene first? Would it be possible to yank the sysfs away from PPD?

DRM client caps are set by the client when the client interacts with DRM.
At driver creation there is no client. How will the driver set things up?

A user might switch between DRM clients (login manager, to desktop compositor,
maybe to another VT with a different compositor). I know everything but the
login manager to desktop compositor hand-off is today considered exotic, but
what if someone starts building a use-case for it? I've done a bunch of gamescope
or IGT work in a different VT while I've had Plasma running on its default
VT.

If someone can sketch this out, with answers to all the questions above and
any other questions you can come up (be creative), I'd be happy to review.
Alternatively we can discuss this at the hackfest and maybe arrive at a
solution.

Harry

Re: [PATCH] drm/amd: Drop abm_level property

[Mario Limonciello]

On 3/6/2024 12:59, Harry Wentland wrote:
> 
> 
> On 2024-03-06 13:02, Mario Limonciello wrote:
>> On 3/6/2024 12:00, Xaver Hugl wrote:
>>> Am Mi., 6. März 2024 um 18:19 Uhr schrieb Mario Limonciello
>>> <mario.limonciello@amd.com>:
>>>> So the idea being if the compositor isn't using it we let
>>>> power-profiles-daemon (or any other software) take control via sysfs and
>>>> if the compositor does want to control it then it then it writes a DRM
>>>> cap and we destroy the sysfs file?
>>>
>>> Yes. That way still only one party controls it at a given time, and we
>>> can get both good default behavior for display servers that don't care
>>> (like Xorg or compositors without color management support), and
>>> compositors that want to put in the effort can do more specific things
>>> with it.
>>
>> I think that's a very good solution.
>>
>> Harry, Hamza, what do you guys think?
> 
> In theory I like it. But how will this look in practice? Is PPD or compositor
> on the scene first? Would it be possible to yank the sysfs away from PPD?
> 

I double checked the existing PPD code to see how well this case maps out.

* If the compositor shows up first then PPD just wouldn't find amdgpu 
support when it probed.
* If PPD goes first and then the compositor I expect right now that PPD 
wouldn't explode.  All the cases that would update it check that the 
file exists before trying to write it.

I think the only risk is TOCTOU for that check relative to when it would 
write the file, but the worst case is you have an error in the journal 
log that it tried to write the file but it's not present (there is 
already error handling everywhere for this).

What we can do to smooth it out even more is export a CHANGE uevent from 
amdgpu when this happens.  PPD can react to the change event and totally 
stop the amdgpu plugin.

> DRM client caps are set by the client when the client interacts with DRM.
> At driver creation there is no client. How will the driver set things up?
> 

You jog my mind for another idea.

1) We could reintroduce abm_level property.
2) If client connects with the cap set, we make any reads or writes to 
the sysfs return -EBUSY and we make amdgpu issue a CHANGE uevent.

* Unchanged PPD will likely log a message to the journal that writes 
fail, but won't explode.
* A changed PPD could react to the uevent and look for -EBUSY to not 
bother even logging a message to the journal.

> A user might switch between DRM clients (login manager, to desktop compositor,
> maybe to another VT with a different compositor). I know everything but the
> login manager to desktop compositor hand-off is today considered exotic, but
> what if someone starts building a use-case for it? I've done a bunch of gamescope
> or IGT work in a different VT while I've had Plasma running on its default
> VT.
> 

If going off the cap of the client to decide whether or not to report 
-EBUSY to sysfs I think it handles the handoff cleanly.

> If someone can sketch this out, with answers to all the questions above and
> any other questions you can come up (be creative), I'd be happy to review.
> Alternatively we can discuss this at the hackfest and maybe arrive at a
> solution.
> 
Yes; I think we should use this thread as a basis for that discussion.