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

[Leo Li <sunpeng.li@amd.com>]

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;