[PATCH v3 0/2] Add support for 'power saving policy' property

[PATCH v3 0/2] Add support for 'power saving policy' property

[Mario Limonciello]

During the Display Next hackfest 2024 one of the topics discussed
was the need for compositor to be able to relay intention to drivers
that color fidelity is preferred over power savings.

To accomplish this a new optional DRM property is being introduced called
"power saving policy".  This property is a bit mask that can be configured
with requests of "Require color accuracy" or "Require low latency"
that can be configured by the compositor.

When a driver advertises support for this property and the compositor
sets it to "Require color accuracy" then the driver will disable any power
saving features that can compromise color fidelity.

In practice the main feature this currently applies to is the
"Adaptive Backlight Modulation" feature within AMD DCN on eDP panels.

When the compositor has marked the property  "Require color accuracy" then
this feature will be disabled and any userspace that tries to turn it on
will get an -EBUSY return code.

Compositors can also request that low latency is critical which in
practice should cause PSR and PSR2 to be disabled.

When the compositor has restored the value back to no requirements then
the previous value that would have been programmed will be restored.

v2->v3:
 * Updates from Leo's comments (see individual patches)

The matching changes for the igt are here:
https://lore.kernel.org/dri-devel/20240522220849.33343-1-mario.limonciello@amd.com/

Mario Limonciello (2):
  drm: Introduce 'power saving policy' drm property
  drm/amd: Add power_saving_policy drm property to eDP connectors

 drivers/gpu/drm/amd/amdgpu/amdgpu_display.c   |  4 ++
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 50 +++++++++++++++++--
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  2 +
 drivers/gpu/drm/drm_connector.c               | 46 +++++++++++++++++
 include/drm/drm_connector.h                   |  2 +
 include/drm/drm_mode_config.h                 |  5 ++
 include/uapi/drm/drm_mode.h                   |  7 +++
 7 files changed, 111 insertions(+), 5 deletions(-)

-- 
2.43.0

[PATCH v3 1/2] drm: Introduce 'power saving policy' drm property

[Mario Limonciello]

The `power saving policy` DRM property is an optional property that
can be added to a connector by a driver.

This property is for compositors to indicate intent of policy of
whether a driver can use power saving features that may compromise
the experience intended by the compositor.

Acked-by: Leo Li <sunpeng.li@amd.com>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v2->v3:
* Add tag
---
 drivers/gpu/drm/drm_connector.c | 46 +++++++++++++++++++++++++++++++++
 include/drm/drm_connector.h     |  2 ++
 include/drm/drm_mode_config.h   |  5 ++++
 include/uapi/drm/drm_mode.h     |  7 +++++
 4 files changed, 60 insertions(+)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 4d2df7f64dc5..088a5874c7a4 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -961,6 +961,11 @@ static const struct drm_prop_enum_list drm_scaling_mode_enum_list[] = {
 	{ DRM_MODE_SCALE_ASPECT, "Full aspect" },
 };
 
+static const struct drm_prop_enum_list drm_power_saving_policy_enum_list[] = {
+	{ __builtin_ffs(DRM_MODE_REQUIRE_COLOR_ACCURACY) - 1, "Require color accuracy" },
+	{ __builtin_ffs(DRM_MODE_REQUIRE_LOW_LATENCY) - 1, "Require low latency" },
+};
+
 static const struct drm_prop_enum_list drm_aspect_ratio_enum_list[] = {
 	{ DRM_MODE_PICTURE_ASPECT_NONE, "Automatic" },
 	{ DRM_MODE_PICTURE_ASPECT_4_3, "4:3" },
@@ -1499,6 +1504,16 @@ static const u32 dp_colorspaces =
  *
  *	Drivers can set up these properties by calling
  *	drm_mode_create_tv_margin_properties().
+ * power saving policy:
+ *	This property is used to set the power saving policy for the connector.
+ *	This property is populated with a bitmask of optional requirements set
+ *	by the drm master for the drm driver to respect:
+ *	- "Require color accuracy": Disable power saving features that will
+ *	  affect color fidelity.
+ *	  For example: Hardware assisted backlight modulation.
+ *	- "Require low latency": Disable power saving features that will
+ *	  affect latency.
+ *	  For example: Panel self refresh (PSR)
  */
 
 int drm_connector_create_standard_properties(struct drm_device *dev)
@@ -1963,6 +1978,37 @@ int drm_mode_create_scaling_mode_property(struct drm_device *dev)
 }
 EXPORT_SYMBOL(drm_mode_create_scaling_mode_property);
 
+/**
+ * drm_mode_create_power_saving_policy_property - create power saving policy property
+ * @dev: DRM device
+ * @supported_policies: bitmask of supported power saving policies
+ *
+ * Called by a driver the first time it's needed, must be attached to desired
+ * connectors.
+ *
+ * Returns: %0
+ */
+int drm_mode_create_power_saving_policy_property(struct drm_device *dev,
+						 uint64_t supported_policies)
+{
+	struct drm_property *power_saving;
+
+	if (dev->mode_config.power_saving_policy)
+		return 0;
+	WARN_ON((supported_policies & DRM_MODE_POWER_SAVING_POLICY_ALL) == 0);
+
+	power_saving =
+		drm_property_create_bitmask(dev, 0, "power saving policy",
+					    drm_power_saving_policy_enum_list,
+					    ARRAY_SIZE(drm_power_saving_policy_enum_list),
+					    supported_policies);
+
+	dev->mode_config.power_saving_policy = power_saving;
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_mode_create_power_saving_policy_property);
+
 /**
  * DOC: Variable refresh properties
  *
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index fe88d7fc6b8f..b0ec2ec48de7 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -2025,6 +2025,8 @@ int drm_mode_create_dp_colorspace_property(struct drm_connector *connector,
 					   u32 supported_colorspaces);
 int drm_mode_create_content_type_property(struct drm_device *dev);
 int drm_mode_create_suggested_offset_properties(struct drm_device *dev);
+int drm_mode_create_power_saving_policy_property(struct drm_device *dev,
+						 uint64_t supported_policies);
 
 int drm_connector_set_path_property(struct drm_connector *connector,
 				    const char *path);
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index 8de3c9a5f61b..eefcbf6c5377 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -969,6 +969,11 @@ struct drm_mode_config {
 	 */
 	struct drm_atomic_state *suspend_state;
 
+	/**
+	 * @power_saving_policy: bitmask for power saving policy requests.
+	 */
+	struct drm_property *power_saving_policy;
+
 	const struct drm_mode_config_helper_funcs *helper_private;
 };
 
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index 1ca5c7e418fd..558a306a23a7 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -152,6 +152,13 @@ extern "C" {
 #define DRM_MODE_SCALE_CENTER		2 /* Centered, no scaling */
 #define DRM_MODE_SCALE_ASPECT		3 /* Full screen, preserve aspect */
 
+/* power saving policy options */
+#define DRM_MODE_REQUIRE_COLOR_ACCURACY	BIT(0)	/* Compositor requires color accuracy */
+#define DRM_MODE_REQUIRE_LOW_LATENCY	BIT(1)	/* Compositor requires low latency */
+
+#define DRM_MODE_POWER_SAVING_POLICY_ALL	(DRM_MODE_REQUIRE_COLOR_ACCURACY |\
+						 DRM_MODE_REQUIRE_LOW_LATENCY)
+
 /* Dithering mode options */
 #define DRM_MODE_DITHERING_OFF	0
 #define DRM_MODE_DITHERING_ON	1
-- 
2.43.0

[PATCH v3 2/2] drm/amd: Add power_saving_policy drm property to eDP connectors

[Mario Limonciello]

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>
---
v2->v3:
 * Use `disallow_edp_enter_psr` instead
 * Drop case in dm_update_crtc_state()
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_display.c   |  4 ++
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 50 +++++++++++++++++--
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  2 +
 3 files changed, 51 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 f1d67c6f4b98..5fd7210b2479 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -6436,6 +6436,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;
@@ -6478,6 +6485,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;
@@ -6504,9 +6518,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);
@@ -6530,10 +6547,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;
@@ -7704,6 +7727,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);
@@ -9307,6 +9337,7 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
 	for_each_oldnew_connector_in_state(state, connector, old_con_state, new_con_state, i) {
 		struct dm_connector_state *dm_new_con_state = to_dm_connector_state(new_con_state);
 		struct dm_connector_state *dm_old_con_state = to_dm_connector_state(old_con_state);
+		struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector);
 		struct amdgpu_crtc *acrtc = to_amdgpu_crtc(dm_new_con_state->base.crtc);
 		struct dc_surface_update *dummy_updates;
 		struct dc_stream_update stream_update;
@@ -9360,6 +9391,15 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
 			stream_update.hdr_static_metadata = &hdr_packet;
 		}
 
+		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);
+		}
+
 		status = dc_stream_get_status(dm_new_crtc_state->stream);
 
 		if (WARN_ON(!status))
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;
-- 
2.43.0

Re: [PATCH v3 2/2] drm/amd: Add power_saving_policy drm property to eDP connectors

[Leo Li]

On 2024-06-05 22:04, 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>
Reviewed-by: Leo Li <sunpeng.li@amd.com>

Thanks!

> ---
> v2->v3:
>   * Use `disallow_edp_enter_psr` instead
>   * Drop case in dm_update_crtc_state()
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_display.c   |  4 ++
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 50 +++++++++++++++++--
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  2 +
>   3 files changed, 51 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 f1d67c6f4b98..5fd7210b2479 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -6436,6 +6436,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;
> @@ -6478,6 +6485,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;
> @@ -6504,9 +6518,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);
> @@ -6530,10 +6547,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;
> @@ -7704,6 +7727,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);
> @@ -9307,6 +9337,7 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
>   	for_each_oldnew_connector_in_state(state, connector, old_con_state, new_con_state, i) {
>   		struct dm_connector_state *dm_new_con_state = to_dm_connector_state(new_con_state);
>   		struct dm_connector_state *dm_old_con_state = to_dm_connector_state(old_con_state);
> +		struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector);
>   		struct amdgpu_crtc *acrtc = to_amdgpu_crtc(dm_new_con_state->base.crtc);
>   		struct dc_surface_update *dummy_updates;
>   		struct dc_stream_update stream_update;
> @@ -9360,6 +9391,15 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
>   			stream_update.hdr_static_metadata = &hdr_packet;
>   		}
>   
> +		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);
> +		}
> +
>   		status = dc_stream_get_status(dm_new_crtc_state->stream);
>   
>   		if (WARN_ON(!status))
> 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;

Re: [PATCH v3 2/2] drm/amd: Add power_saving_policy drm property to eDP connectors

[Mario Limonciello]

On 6/18/2024 17:36, Leo Li wrote:
> 
> 
> On 2024-06-05 22:04, 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>
> Reviewed-by: Leo Li <sunpeng.li@amd.com>

Thanks!  I don't have permissions, so can you (or someone else) please 
apply to drm-misc-next for me?

After it's merged I'll rebase and work on the feedback for the new IGT 
tests.

> 
> Thanks!
> 
>> ---
>> v2->v3:
>>   * Use `disallow_edp_enter_psr` instead
>>   * Drop case in dm_update_crtc_state()
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_display.c   |  4 ++
>>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 50 +++++++++++++++++--
>>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  2 +
>>   3 files changed, 51 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 f1d67c6f4b98..5fd7210b2479 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -6436,6 +6436,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;
>> @@ -6478,6 +6485,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;
>> @@ -6504,9 +6518,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);
>> @@ -6530,10 +6547,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;
>> @@ -7704,6 +7727,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);
>> @@ -9307,6 +9337,7 @@ static void amdgpu_dm_atomic_commit_tail(struct 
>> drm_atomic_state *state)
>>       for_each_oldnew_connector_in_state(state, connector, 
>> old_con_state, new_con_state, i) {
>>           struct dm_connector_state *dm_new_con_state = 
>> to_dm_connector_state(new_con_state);
>>           struct dm_connector_state *dm_old_con_state = 
>> to_dm_connector_state(old_con_state);
>> +        struct amdgpu_dm_connector *aconnector = 
>> to_amdgpu_dm_connector(connector);
>>           struct amdgpu_crtc *acrtc = 
>> to_amdgpu_crtc(dm_new_con_state->base.crtc);
>>           struct dc_surface_update *dummy_updates;
>>           struct dc_stream_update stream_update;
>> @@ -9360,6 +9391,15 @@ static void amdgpu_dm_atomic_commit_tail(struct 
>> drm_atomic_state *state)
>>               stream_update.hdr_static_metadata = &hdr_packet;
>>           }
>> +        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);
>> +        }
>> +
>>           status = dc_stream_get_status(dm_new_crtc_state->stream);
>>           if (WARN_ON(!status))
>> 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;

Re: [PATCH v3 2/2] drm/amd: Add power_saving_policy drm property to eDP connectors

[Xaver Hugl]

Am Mi., 19. Juni 2024 um 06:08 Uhr schrieb Mario Limonciello
<mario.limonciello@amd.com>
> Thanks!  I don't have permissions, so can you (or someone else) please
> apply to drm-misc-next for me?
>
> After it's merged I'll rebase and work on the feedback for the new IGT
> tests.

Merging can only happen once a real world userspace application has
implemented support for it. I'll try to do that sometime next week in
KWin

Re: [PATCH v3 2/2] drm/amd: Add power_saving_policy drm property to eDP connectors

[Xaver Hugl]

Am Do., 20. Juni 2024 um 22:22 Uhr schrieb Xaver Hugl <xaver.hugl@gmail.com>:
> Merging can only happen once a real world userspace application has
> implemented support for it. I'll try to do that sometime next week in
> KWin

Here's the promised implementation:
https://invent.kde.org/plasma/kwin/-/merge_requests/6028

In testing with the patches on top of kernel 6.9.6, setting the
property to `Require color accuracy` makes the sysfs file correctly
report "Device or resource busy" when trying to change the power
saving level, but setting the property to zero doesn't really work.
Once KWin sets the property to zero, changing the power saving level
"works" but the screen blanks for a moment (might just be a single
frame) and reading from the file returns zero again, with the visuals
and backlight level unchanged as well.

Re: [PATCH v3 2/2] drm/amd: Add power_saving_policy drm property to eDP connectors

[Mario Limonciello]

On 7/1/2024 13:47, Xaver Hugl wrote:
> Am Do., 20. Juni 2024 um 22:22 Uhr schrieb Xaver Hugl <xaver.hugl@gmail.com>:
>> Merging can only happen once a real world userspace application has
>> implemented support for it. I'll try to do that sometime next week in
>> KWin
> 
> Here's the promised implementation:
> https://invent.kde.org/plasma/kwin/-/merge_requests/6028

Thanks!

> 
> In testing with the patches on top of kernel 6.9.6, setting the
> property to `Require color accuracy` makes the sysfs file correctly
> report "Device or resource busy" when trying to change the power
> saving level, but setting the property to zero doesn't really work.
> Once KWin sets the property to zero, changing the power saving level
> "works" but the screen blanks for a moment (might just be a single
> frame) and reading from the file returns zero again, with the visuals
> and backlight level unchanged as well.

Hmm I'm a bit surprised the IGT tests I did didn't catch this.

Are you working on a system with two GPUs by chance (like a Framework 
16)?  If so; can you try the "other GPU"?

As it seems your PR to span 3 projects and I've never built KDE before 
can you spit out some artifacts somewhere that I can have a play with to 
reproduce your result and find the kernel issue?  Arch pkgs would be 
preferable for me, but some RPMs or DEBs are fine too.

Re: [PATCH v3 2/2] drm/amd: Add power_saving_policy drm property to eDP connectors

[Xaver Hugl]

Am Mo., 1. Juli 2024 um 21:02 Uhr schrieb Mario Limonciello
<mario.limonciello@amd.com>:
> Hmm I'm a bit surprised the IGT tests I did didn't catch this.
>
> Are you working on a system with two GPUs by chance (like a Framework
> 16)?  If so; can you try the "other GPU"?

No, I tested on a Framework 13.

> As it seems your PR to span 3 projects and I've never built KDE before
> can you spit out some artifacts somewhere that I can have a play with to
> reproduce your result and find the kernel issue?  Arch pkgs would be
> preferable for me, but some RPMs or DEBs are fine too.

Here you go: https://nx44777.your-storageshare.de/s/2j4Jy5anDwwzCtF
and https://nx44777.your-storageshare.de/s/2rxJ4Tp2L8gdc8Y
You can set the drm property to "Require color accuracy" with
"kscreen-doctor output.1.allowColorPowerSaving.disallow" and to zero
again with "kscreen-doctor output.1.allowColorPowerSaving.allow"

Re: [PATCH v3 2/2] drm/amd: Add power_saving_policy drm property to eDP connectors

[Mario Limonciello]

On 7/1/2024 17:34, Xaver Hugl wrote:
> Am Mo., 1. Juli 2024 um 21:02 Uhr schrieb Mario Limonciello
> <mario.limonciello@amd.com>:
>> Hmm I'm a bit surprised the IGT tests I did didn't catch this.
>>
>> Are you working on a system with two GPUs by chance (like a Framework
>> 16)?  If so; can you try the "other GPU"?
> 
> No, I tested on a Framework 13.
> 
>> As it seems your PR to span 3 projects and I've never built KDE before
>> can you spit out some artifacts somewhere that I can have a play with to
>> reproduce your result and find the kernel issue?  Arch pkgs would be
>> preferable for me, but some RPMs or DEBs are fine too.
> 
> Here you go: https://nx44777.your-storageshare.de/s/2j4Jy5anDwwzCtF
> and https://nx44777.your-storageshare.de/s/2rxJ4Tp2L8gdc8Y
> You can set the drm property to "Require color accuracy" with
> "kscreen-doctor output.1.allowColorPowerSaving.disallow" and to zero
> again with "kscreen-doctor output.1.allowColorPowerSaving.allow"

Thanks, I snagged the artifacts.  Of course when I tried to reproduce I 
hit a new issue where my series is causing problems with DSC on my panel.

X crashes and consequently SDDM doesn't come up so I don't even get to 
the point I can toggle the knob.

Mostly posting trace below in case root cause jumps out at anyone what 
actually went wrong.  It reproduces with amdgpu.abmlevel=0 on both 6.9.7 
and 6.10-rc6, but drop my patches and it's gone.

[drm] DSC precompute is not needed.
------------[ cut here ]------------
WARNING: CPU: 7 PID: 321 at 
drivers/gpu/drm/amd/amdgpu/../display/dc/dsc/dcn20/dcn20_dsc.c:272 
dsc2_disable+0xec/0x170 [amdgpu]
Modules linked in: amdgpu(+) amdxcp i2c_algo_bit drm_ttm_helper ttm 
drm_exec gpu_sched drm_suballoc_helper drm_buddy drm_display_helper 
drm_kms_helper drm drm_panel_orientation_quirks nvme serio_raw nvme_core 
video
CPU: 7 PID: 321 Comm: (udev-worker) Not tainted 
6.9.7-00002-gde664ea69218 #241
Hardware name: Framework Laptop 13 (AMD Ryzen 7040Series)/FRANMDCP05, 
BIOS 03.05 03/29/2024
RIP: 0010:dsc2_disable+0xec/0x170 [amdgpu]
Code: 4c 24 0c 44 8b 43 10 48 8b 40 10 48 8b 30 48 85 f6 74 04 48 8b 76 
08 48 c7 c1 98 71 49 c1 ba 08 00 00 00 31 ff e8 84 53 7d ff <0f> 0b 48 
8b 53 20 48 8b 43 28 45 31 c9 48 8b 7b 08 0f b6 8a b4 00
RSP: 0018:ffffb96f80a46e80 EFLAGS: 00010246
RAX: 0000000000000000 RBX: ffff95a144c19000 RCX: ffffffffc1497198
RDX: 0000000000000008 RSI: ffff95a141d8b0c8 RDI: 0000000000000000
RBP: ffff95a1552002d8 R08: 0000000000000000 R09: 0000000000000000
R10: 000000000000014b R11: 0000000000000000 R12: ffff95a14f41d680
R13: 0000000000000001 R14: ffff95a151338000 R15: ffff95a151338000
FS:  00007f3e17557880(0000) GS:ffff95a4ae780000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 000055ab019b1288 CR3: 000000010bb2c000 CR4: 0000000000f50ef0
PKRU: 55555554
Call Trace:
  <TASK>
  ? dsc2_disable+0xec/0x170 [amdgpu]
  ? __warn.cold+0x8e/0xe8
  ? dsc2_disable+0xec/0x170 [amdgpu]
  ? report_bug+0xef/0x1d0
  ? handle_bug+0x3c/0x80
  ? exc_invalid_op+0x13/0x60
  ? asm_exc_invalid_op+0x16/0x20
  ? dsc2_disable+0xec/0x170 [amdgpu]
  link_set_dsc_on_stream+0x3f2/0x470 [amdgpu]
  ? srso_alias_return_thunk+0x5/0xfbef5
  ? dm_helpers_dp_write_dsc_enable+0x286/0x720 [amdgpu]
  ? srso_alias_return_thunk+0x5/0xfbef5
  ? disable_link+0x1e1/0x210 [amdgpu]
  link_set_dsc_enable+0x7f/0x90 [amdgpu]
  link_set_dpms_off+0x1ad/0x990 [amdgpu]
  ? srso_alias_return_thunk+0x5/0xfbef5
  ? srso_alias_return_thunk+0x5/0xfbef5
  ? dmub_srv_get_fw_boot_status+0x43/0x60 [amdgpu]
  ? srso_alias_return_thunk+0x5/0xfbef5
  ? srso_alias_return_thunk+0x5/0xfbef5
  ? dm_read_reg_func+0x57/0xc0 [amdgpu]
  ? srso_alias_return_thunk+0x5/0xfbef5
  dc_commit_state_no_check+0x878/0x1990 [amdgpu]
  dc_commit_streams+0x29d/0x580 [amdgpu]
  ? srso_alias_return_thunk+0x5/0xfbef5
  amdgpu_dm_atomic_commit_tail+0x686/0x44c0 [amdgpu]
  ? srso_alias_return_thunk+0x5/0xfbef5
  ? dcn314_validate_bandwidth+0x181/0x3f0 [amdgpu]
  ? srso_alias_return_thunk+0x5/0xfbef5
  ? srso_alias_return_thunk+0x5/0xfbef5
  ? dma_resv_get_fences+0xb7/0x310
  ? srso_alias_return_thunk+0x5/0xfbef5
  ? srso_alias_return_thunk+0x5/0xfbef5
  ? amdgpu_dm_plane_helper_prepare_fb+0x1a0/0x2b0 [amdgpu]
  commit_tail+0xbf/0x170 [drm_kms_helper]
  drm_atomic_helper_commit+0x116/0x140 [drm_kms_helper]
  drm_atomic_commit+0x99/0xd0 [drm]
  ? __pfx___drm_printfn_info+0x10/0x10 [drm]
  drm_client_modeset_commit_atomic+0x1e2/0x220 [drm]
  drm_client_modeset_commit_locked+0x56/0x160 [drm]
  ? srso_alias_return_thunk+0x5/0xfbef5
  drm_client_modeset_commit+0x21/0x40 [drm]
  __drm_fb_helper_restore_fbdev_mode_unlocked+0xae/0xf0 [drm_kms_helper]
  drm_fb_helper_set_par+0x2c/0x40 [drm_kms_helper]
  fbcon_init+0x2d6/0x670
  visual_init+0xea/0x180
  do_bind_con_driver.isra.0+0x241/0x390
  ? fbcon_startup+0x1fe/0x2e0
  do_take_over_console+0x177/0x1f0
  ? kmalloc_trace+0x246/0x2f0
  do_fbcon_takeover+0x72/0x130
  fbcon_fb_registered+0x33/0x80
  register_framebuffer+0x192/0x2b0
  __drm_fb_helper_initial_config_and_unlock+0x389/0x450 [drm_kms_helper]
  ? srso_alias_return_thunk+0x5/0xfbef5
  ? amdgpu_driver_open_kms+0xcd/0x260 [amdgpu]
  drm_fbdev_generic_client_hotplug+0x67/0xa0 [drm_kms_helper]
  ? srso_alias_return_thunk+0x5/0xfbef5
  drm_client_register+0x72/0xb0 [drm]
  amdgpu_pci_probe+0x441/0x4c0 [amdgpu]
  ? srso_alias_return_thunk+0x5/0xfbef5
  local_pci_probe+0x3e/0x80
  pci_device_probe+0xbd/0x2a0
  really_probe+0xeb/0x350
  ? pm_runtime_barrier+0x50/0x90
  __driver_probe_device+0x87/0x130
  driver_probe_device+0x1f/0xc0
  __driver_attach+0xcb/0x1f0
  ? __pfx___driver_attach+0x10/0x10
  bus_for_each_dev+0x77/0xd0
  bus_add_driver+0x10e/0x200
  driver_register+0x6e/0xc0
  ? __pfx_amdgpu_init+0x10/0x10 [amdgpu]
  do_one_initcall+0x3f/0x2f0
  ? do_init_module+0x22/0x230
  do_init_module+0x60/0x230
  init_module_from_file+0x86/0xc0
  idempotent_init_module+0x10b/0x2a0
  __x64_sys_finit_module+0x5a/0xb0
  do_syscall_64+0x80/0x180
  ? srso_alias_return_thunk+0x5/0xfbef5
  ? vfs_fstatat+0x90/0xb0
  ? srso_alias_return_thunk+0x5/0xfbef5
  ? __do_sys_newfstatat+0x25/0x60
  ? do_syscall_64+0x8d/0x180
  ? srso_alias_return_thunk+0x5/0xfbef5
  ? vfs_read+0x1f9/0x330
  ? srso_alias_return_thunk+0x5/0xfbef5
  ? syscall_exit_to_user_mode+0x71/0x210
  ? srso_alias_return_thunk+0x5/0xfbef5
  ? do_syscall_64+0x8d/0x180
  ? srso_alias_return_thunk+0x5/0xfbef5
  ? syscall_exit_to_user_mode+0x71/0x210
  ? srso_alias_return_thunk+0x5/0xfbef5
  ? do_syscall_64+0x8d/0x180
  ? srso_alias_return_thunk+0x5/0xfbef5
  ? do_sys_openat2+0x82/0xc0
  ? do_syscall_64+0x8d/0x180
  ? srso_alias_return_thunk+0x5/0xfbef5
  ? syscall_exit_to_user_mode+0x71/0x210
  ? srso_alias_return_thunk+0x5/0xfbef5
  ? do_syscall_64+0x8d/0x180
  ? srso_alias_return_thunk+0x5/0xfbef5
  entry_SYSCALL_64_after_hwframe+0x76/0x7e
RIP: 0033:0x7f3e17db6f9d
Code: ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 48 89 f8 48 89 
f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 
f0 ff ff 73 01 c3 48 8b 0d 5b 6d 0c 00 f7 d8 64 89 01 48
RSP: 002b:00007ffd4f93b6c8 EFLAGS: 00000246 ORIG_RAX: 0000000000000139
RAX: ffffffffffffffda RBX: 000055ab019a43d0 RCX: 00007f3e17db6f9d
RDX: 0000000000000000 RSI: 000055ab019a85b0 RDI: 0000000000000021
RBP: 000055ab019a85b0 R08: 0000000000000040 R09: 000055ab0199158f
R10: fffffffffffffec0 R11: 0000000000000246 R12: 0000000000020000
R13: 000055ab019a6300 R14: 000055ab019aae70 R15: 000055ab019aaa90
  </TASK>
---[ end trace 0000000000000000 ]---

Re: [PATCH v3 2/2] drm/amd: Add power_saving_policy drm property to eDP connectors

[Jani Nikula]

On Mon, 01 Jul 2024, Xaver Hugl <xaver.hugl@gmail.com> wrote:
> Am Do., 20. Juni 2024 um 22:22 Uhr schrieb Xaver Hugl <xaver.hugl@gmail.com>:
>> Merging can only happen once a real world userspace application has
>> implemented support for it. I'll try to do that sometime next week in
>> KWin
>
> Here's the promised implementation:
> https://invent.kde.org/plasma/kwin/-/merge_requests/6028

The requirement is that the userspace patches must be reviewed and ready
for merging into a suitable and canonical upstream project.

Are they?


BR,
Jani.


>
> In testing with the patches on top of kernel 6.9.6, setting the
> property to `Require color accuracy` makes the sysfs file correctly
> report "Device or resource busy" when trying to change the power
> saving level, but setting the property to zero doesn't really work.
> Once KWin sets the property to zero, changing the power saving level
> "works" but the screen blanks for a moment (might just be a single
> frame) and reading from the file returns zero again, with the visuals
> and backlight level unchanged as well.

-- 
Jani Nikula, Intel

Re: [PATCH v3 2/2] drm/amd: Add power_saving_policy drm property to eDP connectors

[Thomas Zimmermann]

Hi

Am 01.08.24 um 14:33 schrieb Jani Nikula:
> On Mon, 01 Jul 2024, Xaver Hugl <xaver.hugl@gmail.com> wrote:
>> Am Do., 20. Juni 2024 um 22:22 Uhr schrieb Xaver Hugl <xaver.hugl@gmail.com>:
>>> Merging can only happen once a real world userspace application has
>>> implemented support for it. I'll try to do that sometime next week in
>>> KWin
>> Here's the promised implementation:
>> https://invent.kde.org/plasma/kwin/-/merge_requests/6028
> The requirement is that the userspace patches must be reviewed and ready
> for merging into a suitable and canonical upstream project.
>
> Are they?

I already saw this series in today's PR for drm-misc-next. :/

Best regards
Thomas

>
>
> BR,
> Jani.
>
>
>> In testing with the patches on top of kernel 6.9.6, setting the
>> property to `Require color accuracy` makes the sysfs file correctly
>> report "Device or resource busy" when trying to change the power
>> saving level, but setting the property to zero doesn't really work.
>> Once KWin sets the property to zero, changing the power saving level
>> "works" but the screen blanks for a moment (might just be a single
>> frame) and reading from the file returns zero again, with the visuals
>> and backlight level unchanged as well.

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Re: [PATCH v3 2/2] drm/amd: Add power_saving_policy drm property to eDP connectors

[Jani Nikula]

On Thu, 01 Aug 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Hi
>
> Am 01.08.24 um 14:33 schrieb Jani Nikula:
>> On Mon, 01 Jul 2024, Xaver Hugl <xaver.hugl@gmail.com> wrote:
>>> Am Do., 20. Juni 2024 um 22:22 Uhr schrieb Xaver Hugl <xaver.hugl@gmail.com>:
>>>> Merging can only happen once a real world userspace application has
>>>> implemented support for it. I'll try to do that sometime next week in
>>>> KWin
>>> Here's the promised implementation:
>>> https://invent.kde.org/plasma/kwin/-/merge_requests/6028
>> The requirement is that the userspace patches must be reviewed and ready
>> for merging into a suitable and canonical upstream project.
>>
>> Are they?
>
> I already saw this series in today's PR for drm-misc-next. :/

Exactly what triggered the question!

BR,
Jani.


>
> Best regards
> Thomas
>
>>
>>
>> BR,
>> Jani.
>>
>>
>>> In testing with the patches on top of kernel 6.9.6, setting the
>>> property to `Require color accuracy` makes the sysfs file correctly
>>> report "Device or resource busy" when trying to change the power
>>> saving level, but setting the property to zero doesn't really work.
>>> Once KWin sets the property to zero, changing the power saving level
>>> "works" but the screen blanks for a moment (might just be a single
>>> frame) and reading from the file returns zero again, with the visuals
>>> and backlight level unchanged as well.

-- 
Jani Nikula, Intel

Re: [PATCH v3 2/2] drm/amd: Add power_saving_policy drm property to eDP connectors

[Xaver Hugl]

Am Do., 1. Aug. 2024 um 14:34 Uhr schrieb Jani Nikula
<jani.nikula@linux.intel.com>:
>
> On Mon, 01 Jul 2024, Xaver Hugl <xaver.hugl@gmail.com> wrote:
> > Am Do., 20. Juni 2024 um 22:22 Uhr schrieb Xaver Hugl <xaver.hugl@gmail.com>:
> >> Merging can only happen once a real world userspace application has
> >> implemented support for it. I'll try to do that sometime next week in
> >> KWin
> >
> > Here's the promised implementation:
> > https://invent.kde.org/plasma/kwin/-/merge_requests/6028
>
> The requirement is that the userspace patches must be reviewed and ready
> for merging into a suitable and canonical upstream project.
>
> Are they?

I've talked about the property with other KWin developers before, but
there's indeed no official review for the MR yet.
As some new discussions about alternative approaches have started as
well, maybe it should be reverted until we're more certain about how
to proceed?

> BR,
> Jani.
>
>
> >
> > In testing with the patches on top of kernel 6.9.6, setting the
> > property to `Require color accuracy` makes the sysfs file correctly
> > report "Device or resource busy" when trying to change the power
> > saving level, but setting the property to zero doesn't really work.
> > Once KWin sets the property to zero, changing the power saving level
> > "works" but the screen blanks for a moment (might just be a single
> > frame) and reading from the file returns zero again, with the visuals
> > and backlight level unchanged as well.
>
> --
> Jani Nikula, Intel

Re: [PATCH v3 2/2] drm/amd: Add power_saving_policy drm property to eDP connectors

[Sebastian Wick]

I'm very unhappy about how this has played out.

We have a new sysfs property that controls a feature of the display
path that has been set to a default(!) which changes the color
behavior! This broke color management for everyone who is on a device
which supports this feature.

What should have been done is the following:

* add the KMS property and have it default to "sysfs cannot override this"
* add the sysfs property after the KMS property has been introduced so
it stays disabled by default
* add support for the new property in compositors and let them enable
this feature only when they allow colors to randomly get broken

Every other path results in broken colors at least temporarily and is
breaking user space! The sysfs property *must* be reverted because it
breaks user space. The KMS property *must* be reverted because it
didn't meet the merging criteria.

Another note: The only way to make sure that this isn't breaking user
space is if user space tells the kernel that this is okay. This means
that the sysfs property can only be used if the compositor grows
support for the new KMS property and at that point, why do we have the
sysfs property?

On Fri, Aug 2, 2024 at 2:49 PM Xaver Hugl <xaver.hugl@gmail.com> wrote:
>
> Am Do., 1. Aug. 2024 um 14:34 Uhr schrieb Jani Nikula
> <jani.nikula@linux.intel.com>:
> >
> > On Mon, 01 Jul 2024, Xaver Hugl <xaver.hugl@gmail.com> wrote:
> > > Am Do., 20. Juni 2024 um 22:22 Uhr schrieb Xaver Hugl <xaver.hugl@gmail.com>:
> > >> Merging can only happen once a real world userspace application has
> > >> implemented support for it. I'll try to do that sometime next week in
> > >> KWin
> > >
> > > Here's the promised implementation:
> > > https://invent.kde.org/plasma/kwin/-/merge_requests/6028
> >
> > The requirement is that the userspace patches must be reviewed and ready
> > for merging into a suitable and canonical upstream project.
> >
> > Are they?
>
> I've talked about the property with other KWin developers before, but
> there's indeed no official review for the MR yet.
> As some new discussions about alternative approaches have started as
> well, maybe it should be reverted until we're more certain about how
> to proceed?
>
> > BR,
> > Jani.
> >
> >
> > >
> > > In testing with the patches on top of kernel 6.9.6, setting the
> > > property to `Require color accuracy` makes the sysfs file correctly
> > > report "Device or resource busy" when trying to change the power
> > > saving level, but setting the property to zero doesn't really work.
> > > Once KWin sets the property to zero, changing the power saving level
> > > "works" but the screen blanks for a moment (might just be a single
> > > frame) and reading from the file returns zero again, with the visuals
> > > and backlight level unchanged as well.
> >
> > --
> > Jani Nikula, Intel
>

Re: [PATCH v3 2/2] drm/amd: Add power_saving_policy drm property to eDP connectors

[Harry Wentland]

On 2024-08-02 09:28, Sebastian Wick wrote:
> I'm very unhappy about how this has played out.
> 
> We have a new sysfs property that controls a feature of the display
> path that has been set to a default(!) which changes the color
> behavior! This broke color management for everyone who is on a device
> which supports this feature.
> 

Has this been a problem that people have noticed or complained about?
Are there bug reports?

AFAIK the default is "off" and PPD will enable ABM if in power or
balanced mode and on battery.

> What should have been done is the following:
> 
> * add the KMS property and have it default to "sysfs cannot override this"
> * add the sysfs property after the KMS property has been introduced so
> it stays disabled by default
> * add support for the new property in compositors and let them enable
> this feature only when they allow colors to randomly get broken
> 
> Every other path results in broken colors at least temporarily and is
> breaking user space! The sysfs property *must* be reverted because it
> breaks user space. The KMS property *must* be reverted because it
> didn't meet the merging criteria.
> 

I agree we should revert the KMS property until we have a userspace
implementation that is reviewed and ready to be merged. It was merged
based on a misunderstanding and shouldn't have been merged.

I don't think we should revert the sysfs property. The power savings to
end-users can be significant. I would like to see compositors take
control if it via the KMS property.

> Another note: The only way to make sure that this isn't breaking user
> space is if user space tells the kernel that this is okay. This means
> that the sysfs property can only be used if the compositor grows
> support for the new KMS property and at that point, why do we have the
> sysfs property?
> 

We have a good handful of widely used compositors. We have one PPD
with a replacement for it in the works. A sysfs allows all users
to get the power benefits even if compositors don't explicitly
enable support for power saving features in KMS. The goal of PPD
and co. is power savings while that is not always a primary goal
for all compositors (even though compositors play a large role in
a system's power usage).

Harry

> On Fri, Aug 2, 2024 at 2:49 PM Xaver Hugl <xaver.hugl@gmail.com> wrote:
>>
>> Am Do., 1. Aug. 2024 um 14:34 Uhr schrieb Jani Nikula
>> <jani.nikula@linux.intel.com>:
>>>
>>> On Mon, 01 Jul 2024, Xaver Hugl <xaver.hugl@gmail.com> wrote:
>>>> Am Do., 20. Juni 2024 um 22:22 Uhr schrieb Xaver Hugl <xaver.hugl@gmail.com>:
>>>>> Merging can only happen once a real world userspace application has
>>>>> implemented support for it. I'll try to do that sometime next week in
>>>>> KWin
>>>>
>>>> Here's the promised implementation:
>>>> https://invent.kde.org/plasma/kwin/-/merge_requests/6028
>>>
>>> The requirement is that the userspace patches must be reviewed and ready
>>> for merging into a suitable and canonical upstream project.
>>>
>>> Are they?
>>
>> I've talked about the property with other KWin developers before, but
>> there's indeed no official review for the MR yet.
>> As some new discussions about alternative approaches have started as
>> well, maybe it should be reverted until we're more certain about how
>> to proceed?
>>
>>> BR,
>>> Jani.
>>>
>>>
>>>>
>>>> In testing with the patches on top of kernel 6.9.6, setting the
>>>> property to `Require color accuracy` makes the sysfs file correctly
>>>> report "Device or resource busy" when trying to change the power
>>>> saving level, but setting the property to zero doesn't really work.
>>>> Once KWin sets the property to zero, changing the power saving level
>>>> "works" but the screen blanks for a moment (might just be a single
>>>> frame) and reading from the file returns zero again, with the visuals
>>>> and backlight level unchanged as well.
>>>
>>> --
>>> Jani Nikula, Intel
>>
> 

Re: [PATCH v3 2/2] drm/amd: Add power_saving_policy drm property to eDP connectors

[Sebastian Wick]

On Fri, Aug 2, 2024 at 4:37 PM Harry Wentland <harry.wentland@amd.com> wrote:
>
> On 2024-08-02 09:28, Sebastian Wick wrote:
> > I'm very unhappy about how this has played out.
> >
> > We have a new sysfs property that controls a feature of the display
> > path that has been set to a default(!) which changes the color
> > behavior! This broke color management for everyone who is on a device
> > which supports this feature.
> >
>
> Has this been a problem that people have noticed or complained about?
> Are there bug reports?

Yes. Even worse, it's not obvious to people that something is broken,
where it is broken and why it is broken.

https://gitlab.gnome.org/GNOME/mutter/-/issues/3589

>
> AFAIK the default is "off" and PPD will enable ABM if in power or
> balanced mode and on battery.
>
> > What should have been done is the following:
> >
> > * add the KMS property and have it default to "sysfs cannot override this"
> > * add the sysfs property after the KMS property has been introduced so
> > it stays disabled by default
> > * add support for the new property in compositors and let them enable
> > this feature only when they allow colors to randomly get broken
> >
> > Every other path results in broken colors at least temporarily and is
> > breaking user space! The sysfs property *must* be reverted because it
> > breaks user space. The KMS property *must* be reverted because it
> > didn't meet the merging criteria.
> >
>
> I agree we should revert the KMS property until we have a userspace
> implementation that is reviewed and ready to be merged. It was merged
> based on a misunderstanding and shouldn't have been merged.
>
> I don't think we should revert the sysfs property. The power savings to
> end-users can be significant. I would like to see compositors take
> control if it via the KMS property.

If this was just a KMS property then I wouldn't have anything to
complain about. The problem here is the sysfs / PPD thing.

> > Another note: The only way to make sure that this isn't breaking user
> > space is if user space tells the kernel that this is okay. This means
> > that the sysfs property can only be used if the compositor grows
> > support for the new KMS property and at that point, why do we have the
> > sysfs property?
> >
>
> We have a good handful of widely used compositors. We have one PPD
> with a replacement for it in the works. A sysfs allows all users
> to get the power benefits even if compositors don't explicitly
> enable support for power saving features in KMS. The goal of PPD
> and co. is power savings while that is not always a primary goal
> for all compositors (even though compositors play a large role in
> a system's power usage).

The problem is that if the compositor didn't explicitly opt-in, then
it can break something (and it actually did). I appreciate the intent
(enabling it as broadly as possible) but the only way I can see how
you can enable feature at all is by somehow doing it per-compositor.

Given all of that, there shouldn't be a sysfs property (you should
revert this!) but it should be yet another KMS property.

> Harry
>
> > On Fri, Aug 2, 2024 at 2:49 PM Xaver Hugl <xaver.hugl@gmail.com> wrote:
> >>
> >> Am Do., 1. Aug. 2024 um 14:34 Uhr schrieb Jani Nikula
> >> <jani.nikula@linux.intel.com>:
> >>>
> >>> On Mon, 01 Jul 2024, Xaver Hugl <xaver.hugl@gmail.com> wrote:
> >>>> Am Do., 20. Juni 2024 um 22:22 Uhr schrieb Xaver Hugl <xaver.hugl@gmail.com>:
> >>>>> Merging can only happen once a real world userspace application has
> >>>>> implemented support for it. I'll try to do that sometime next week in
> >>>>> KWin
> >>>>
> >>>> Here's the promised implementation:
> >>>> https://invent.kde.org/plasma/kwin/-/merge_requests/6028
> >>>
> >>> The requirement is that the userspace patches must be reviewed and ready
> >>> for merging into a suitable and canonical upstream project.
> >>>
> >>> Are they?
> >>
> >> I've talked about the property with other KWin developers before, but
> >> there's indeed no official review for the MR yet.
> >> As some new discussions about alternative approaches have started as
> >> well, maybe it should be reverted until we're more certain about how
> >> to proceed?
> >>
> >>> BR,
> >>> Jani.
> >>>
> >>>
> >>>>
> >>>> In testing with the patches on top of kernel 6.9.6, setting the
> >>>> property to `Require color accuracy` makes the sysfs file correctly
> >>>> report "Device or resource busy" when trying to change the power
> >>>> saving level, but setting the property to zero doesn't really work.
> >>>> Once KWin sets the property to zero, changing the power saving level
> >>>> "works" but the screen blanks for a moment (might just be a single
> >>>> frame) and reading from the file returns zero again, with the visuals
> >>>> and backlight level unchanged as well.
> >>>
> >>> --
> >>> Jani Nikula, Intel
> >>
> >
>