diff mbox series

[v2,2/2] drm/amd: Add power_saving_policy drm property to eDP connectors

Message ID 20240522220531.32728-3-mario.limonciello@amd.com (mailing list archive)
State New, archived
Headers show
Series Add support for 'power saving policy' property | expand

Commit Message

Mario Limonciello May 22, 2024, 10:05 p.m. UTC 
When the `power_saving_policy` property is set to bit mask
"Require color accuracy" ABM should be disabled immediately and
any requests by sysfs to update will return an -EBUSY error.

When the `power_saving_policy` property is set to bit mask
"Require low latency" PSR should be disabled.

When the property is restored to an empty bit mask the previous
value of ABM and pSR will be restored.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_display.c   |  4 ++
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 51 +++++++++++++++++--
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  2 +
 3 files changed, 52 insertions(+), 5 deletions(-)

Comments

Leo Li June 4, 2024, 9:52 p.m. UTC | #1 
On 2024-05-22 18:05, Mario Limonciello wrote:
> When the `power_saving_policy` property is set to bit mask
> "Require color accuracy" ABM should be disabled immediately and
> any requests by sysfs to update will return an -EBUSY error.
> 
> When the `power_saving_policy` property is set to bit mask
> "Require low latency" PSR should be disabled.
> 
> When the property is restored to an empty bit mask the previous
> value of ABM and pSR will be restored.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_display.c   |  4 ++
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 51 +++++++++++++++++--
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  2 +
>   3 files changed, 52 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> index 3ecc7ef95172..cfb5220cf182 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> @@ -1350,6 +1350,10 @@ int amdgpu_display_modeset_create_props(struct amdgpu_device *adev)
>   					 "dither",
>   					 amdgpu_dither_enum_list, sz);
>   
> +	if (adev->dc_enabled)
> +		drm_mode_create_power_saving_policy_property(adev_to_drm(adev),
> +							     DRM_MODE_POWER_SAVING_POLICY_ALL);
> +
>   	return 0;
>   }
>   
> 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 01b0a331e4a6..a480e86892de 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -6421,6 +6421,13 @@ 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 == dev->mode_config.power_saving_policy) {
> +		dm_new_state->abm_forbidden = val & DRM_MODE_REQUIRE_COLOR_ACCURACY;
> +		dm_new_state->abm_level = (dm_new_state->abm_forbidden || !amdgpu_dm_abm_level) ?
> +						ABM_LEVEL_IMMEDIATE_DISABLE :
> +						amdgpu_dm_abm_level;
> +		dm_new_state->psr_forbidden = val & DRM_MODE_REQUIRE_LOW_LATENCY;
> +		ret = 0;
>   	}
>   
>   	return ret;
> @@ -6463,6 +6470,13 @@ 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 == dev->mode_config.power_saving_policy) {
> +		*val = 0;
> +		if (dm_state->psr_forbidden)
> +			*val |= DRM_MODE_REQUIRE_LOW_LATENCY;
> +		if (dm_state->abm_forbidden)
> +			*val |= DRM_MODE_REQUIRE_COLOR_ACCURACY;
> +		ret = 0;
>   	}
>   
>   	return ret;
> @@ -6489,9 +6503,12 @@ static ssize_t panel_power_savings_show(struct device *device,
>   	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;
> +	if (to_dm_connector_state(connector->state)->abm_forbidden)
> +		val = 0;
> +	else
> +		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);
> @@ -6515,10 +6532,16 @@ static ssize_t panel_power_savings_store(struct device *device,
>   		return -EINVAL;
>   
>   	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> -	to_dm_connector_state(connector->state)->abm_level = val ?:
> -		ABM_LEVEL_IMMEDIATE_DISABLE;
> +	if (to_dm_connector_state(connector->state)->abm_forbidden)
> +		ret = -EBUSY;
> +	else
> +		to_dm_connector_state(connector->state)->abm_level = val ?:
> +			ABM_LEVEL_IMMEDIATE_DISABLE;
>   	drm_modeset_unlock(&dev->mode_config.connection_mutex);
>   
> +	if (ret)
> +		return ret;
> +
>   	drm_kms_helper_hotplug_event(dev);
>   
>   	return count;
> @@ -7689,6 +7712,13 @@ 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)) {
> +		drm_object_attach_property(&aconnector->base.base,
> +				dm->ddev->mode_config.power_saving_policy, 0);
> +	}
> +
>   	if (connector_type == DRM_MODE_CONNECTOR_HDMIA) {
>   		/* Content Type is currently only implemented for HDMI. */
>   		drm_connector_attach_content_type_property(&aconnector->base);
> @@ -9344,6 +9374,11 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
>   			stream_update.hdr_static_metadata = &hdr_packet;
>   		}
>   
> +		if (dm_old_con_state->psr_forbidden && !dm_new_con_state->psr_forbidden)
> +			amdgpu_dm_psr_disable(dm_new_crtc_state->stream);
> +		else if (!dm_old_con_state->psr_forbidden && dm_new_con_state->psr_forbidden)
> +			amdgpu_dm_psr_enable(dm_new_crtc_state->stream);
> +

Thanks for the patch!

Unfortunately, enabling/disabling PSR today isn't as straightforward as a call
to amdgpu_dm_psr_enable/disable. The conditions for enabling PSR is quite
convoluted and need some untangling...

For now, I think the easiest way to pipe this property through is to hijack the
`amdgpu_dm_connector->disallow_edp_enter_psr` flag. IIRC, it is currently hooked
up to a debugfs that force disables PSR for testing purposes. Eventually, we can
probably deprecate the debugfs and use this property instead.

We could change the above 'if/else if' to something like the following:

```
struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector);
<snip>

aconnector->disallow_edp_enter_psr = dm_new_con_state->psr_forbidden

/* immediately disable PSR if disallowed */
if (aconnector->disallow_edp_enter_psr) {
	mutex_lock(&dm->dc_lock);
	amdgpu_dm_psr_disable(dm_new_crtc_state->stream);
	mutex_unlock(&dm->dc_lock);
}
```

The moment disallow_edp_enter_psr flips to 0, the next fast update should
re-enable PSR. There isn't any guarantee of an immediate re-enable, but I think
that should be fine.


>   		status = dc_stream_get_status(dm_new_crtc_state->stream);
>   
>   		if (WARN_ON(!status))
> @@ -10019,6 +10054,12 @@ static int dm_update_crtc_state(struct amdgpu_display_manager *dm,
>   		update_stream_scaling_settings(
>   			&new_crtc_state->mode, dm_new_conn_state, dm_new_crtc_state->stream);
>   
> +
> +	if (dm_old_conn_state->psr_forbidden && !dm_new_conn_state->psr_forbidden)
> +		amdgpu_dm_psr_disable(dm_new_crtc_state->stream);
> +	else if (!dm_old_conn_state->psr_forbidden && dm_new_conn_state->psr_forbidden)
> +		amdgpu_dm_psr_enable(dm_new_crtc_state->stream);
> +

dm_update_crtc_state() can be called as part of atomic check, so we should not 
do any hw programming here.

There aren't any reasons I can think of to reject allow/disallowing PSR either, 
so this part shouldn't be necessary and can be dropped.

Thanks,
Leo

>   	/* ABM settings */
>   	dm_new_crtc_state->abm_level = dm_new_conn_state->abm_level;
>   
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> index 09519b7abf67..b246e82f5b0d 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> @@ -873,6 +873,8 @@ struct dm_connector_state {
>   	bool underscan_enable;
>   	bool freesync_capable;
>   	bool update_hdcp;
> +	bool abm_forbidden;
> +	bool psr_forbidden;
>   	uint8_t abm_level;
>   	int vcpi_slots;
>   	uint64_t pbn;
diff mbox series

Patch

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
index 3ecc7ef95172..cfb5220cf182 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
@@ -1350,6 +1350,10 @@  int amdgpu_display_modeset_create_props(struct amdgpu_device *adev)
 					 "dither",
 					 amdgpu_dither_enum_list, sz);
 
+	if (adev->dc_enabled)
+		drm_mode_create_power_saving_policy_property(adev_to_drm(adev),
+							     DRM_MODE_POWER_SAVING_POLICY_ALL);
+
 	return 0;
 }
 
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 01b0a331e4a6..a480e86892de 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -6421,6 +6421,13 @@  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 == dev->mode_config.power_saving_policy) {
+		dm_new_state->abm_forbidden = val & DRM_MODE_REQUIRE_COLOR_ACCURACY;
+		dm_new_state->abm_level = (dm_new_state->abm_forbidden || !amdgpu_dm_abm_level) ?
+						ABM_LEVEL_IMMEDIATE_DISABLE :
+						amdgpu_dm_abm_level;
+		dm_new_state->psr_forbidden = val & DRM_MODE_REQUIRE_LOW_LATENCY;
+		ret = 0;
 	}
 
 	return ret;
@@ -6463,6 +6470,13 @@  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 == dev->mode_config.power_saving_policy) {
+		*val = 0;
+		if (dm_state->psr_forbidden)
+			*val |= DRM_MODE_REQUIRE_LOW_LATENCY;
+		if (dm_state->abm_forbidden)
+			*val |= DRM_MODE_REQUIRE_COLOR_ACCURACY;
+		ret = 0;
 	}
 
 	return ret;
@@ -6489,9 +6503,12 @@  static ssize_t panel_power_savings_show(struct device *device,
 	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;
+	if (to_dm_connector_state(connector->state)->abm_forbidden)
+		val = 0;
+	else
+		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);
@@ -6515,10 +6532,16 @@  static ssize_t panel_power_savings_store(struct device *device,
 		return -EINVAL;
 
 	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
-	to_dm_connector_state(connector->state)->abm_level = val ?:
-		ABM_LEVEL_IMMEDIATE_DISABLE;
+	if (to_dm_connector_state(connector->state)->abm_forbidden)
+		ret = -EBUSY;
+	else
+		to_dm_connector_state(connector->state)->abm_level = val ?:
+			ABM_LEVEL_IMMEDIATE_DISABLE;
 	drm_modeset_unlock(&dev->mode_config.connection_mutex);
 
+	if (ret)
+		return ret;
+
 	drm_kms_helper_hotplug_event(dev);
 
 	return count;
@@ -7689,6 +7712,13 @@  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)) {
+		drm_object_attach_property(&aconnector->base.base,
+				dm->ddev->mode_config.power_saving_policy, 0);
+	}
+
 	if (connector_type == DRM_MODE_CONNECTOR_HDMIA) {
 		/* Content Type is currently only implemented for HDMI. */
 		drm_connector_attach_content_type_property(&aconnector->base);
@@ -9344,6 +9374,11 @@  static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
 			stream_update.hdr_static_metadata = &hdr_packet;
 		}
 
+		if (dm_old_con_state->psr_forbidden && !dm_new_con_state->psr_forbidden)
+			amdgpu_dm_psr_disable(dm_new_crtc_state->stream);
+		else if (!dm_old_con_state->psr_forbidden && dm_new_con_state->psr_forbidden)
+			amdgpu_dm_psr_enable(dm_new_crtc_state->stream);
+
 		status = dc_stream_get_status(dm_new_crtc_state->stream);
 
 		if (WARN_ON(!status))
@@ -10019,6 +10054,12 @@  static int dm_update_crtc_state(struct amdgpu_display_manager *dm,
 		update_stream_scaling_settings(
 			&new_crtc_state->mode, dm_new_conn_state, dm_new_crtc_state->stream);
 
+
+	if (dm_old_conn_state->psr_forbidden && !dm_new_conn_state->psr_forbidden)
+		amdgpu_dm_psr_disable(dm_new_crtc_state->stream);
+	else if (!dm_old_conn_state->psr_forbidden && dm_new_conn_state->psr_forbidden)
+		amdgpu_dm_psr_enable(dm_new_crtc_state->stream);
+
 	/* ABM settings */
 	dm_new_crtc_state->abm_level = dm_new_conn_state->abm_level;
 
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
index 09519b7abf67..b246e82f5b0d 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -873,6 +873,8 @@  struct dm_connector_state {
 	bool underscan_enable;
 	bool freesync_capable;
 	bool update_hdcp;
+	bool abm_forbidden;
+	bool psr_forbidden;
 	uint8_t abm_level;
 	int vcpi_slots;
 	uint64_t pbn;