[PATCH] drm/amd/display: change the panel power savings level without a modeset

[PATCH] drm/amd/display: change the panel power savings level without a modeset

[Hamza Mahfooz]

We don't actually need to request that the compositor does a full
modeset to modify the panel power savings level, we can instead
just make a request to DMUB, to set the new level dynamically.

Cc: Harry Wentland <harry.wentland@amd.com>
Cc: Leo Li <sunpeng.li@amd.com>
Cc: Mario Limonciello <mario.limonciello@amd.com>
Cc: Sebastian Wick <sebastian@sebastianwick.net>
Signed-off-by: Hamza Mahfooz <hamza.mahfooz@amd.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 17 +++++++-
 drivers/gpu/drm/amd/display/dc/core/dc.c      | 39 +++++++++++--------
 drivers/gpu/drm/amd/display/dc/dc.h           |  2 +
 3 files changed, 41 insertions(+), 17 deletions(-)

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 dd8353283bda..00a8a5959aa9 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -6819,9 +6819,14 @@ static ssize_t panel_power_savings_store(struct device *device,
 					 const char *buf, size_t count)
 {
 	struct drm_connector *connector = dev_get_drvdata(device);
+	struct amdgpu_dm_connector *aconn = to_amdgpu_dm_connector(connector);
 	struct drm_device *dev = connector->dev;
+	struct amdgpu_device *adev = drm_to_adev(dev);
+	struct dc *dc = adev->dm.dc;
+	struct pipe_ctx *pipe_ctx;
 	long val;
 	int ret;
+	int i;
 
 	ret = kstrtol(buf, 0, &val);
 
@@ -6836,7 +6841,17 @@ static ssize_t panel_power_savings_store(struct device *device,
 		ABM_LEVEL_IMMEDIATE_DISABLE;
 	drm_modeset_unlock(&dev->mode_config.connection_mutex);
 
-	drm_kms_helper_hotplug_event(dev);
+	mutex_lock(&adev->dm.dc_lock);
+	for (i = 0; i < dc->res_pool->pipe_count; i++) {
+		pipe_ctx = &dc->current_state->res_ctx.pipe_ctx[i];
+
+		if (pipe_ctx->stream &&
+		    pipe_ctx->stream->link == aconn->dc_link) {
+			dc_set_abm_level(dc, pipe_ctx, val);
+			break;
+		}
+	}
+	mutex_unlock(&adev->dm.dc_lock);
 
 	return count;
 }
diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c b/drivers/gpu/drm/amd/display/dc/core/dc.c
index 3ba2acfdae2a..60081cd978b7 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc.c
@@ -3319,6 +3319,23 @@ static bool update_planes_and_stream_state(struct dc *dc,
 
 }
 
+void dc_set_abm_level(struct dc *dc, struct pipe_ctx *pipe_ctx, int level)
+{
+	struct timing_generator *tg = pipe_ctx->stream_res.tg;
+	struct abm *abm = pipe_ctx->stream_res.abm;
+
+	if (!abm)
+		return;
+
+	if (tg->funcs->is_blanked && !tg->funcs->is_blanked(tg))
+		tg->funcs->wait_for_state(tg, CRTC_STATE_VBLANK);
+
+	if (level == ABM_LEVEL_IMMEDIATE_DISABLE)
+		dc->hwss.set_abm_immediate_disable(pipe_ctx);
+	else
+		abm->funcs->set_abm_level(abm, level);
+}
+
 static void commit_planes_do_stream_update(struct dc *dc,
 		struct dc_stream_state *stream,
 		struct dc_stream_update *stream_update,
@@ -3447,22 +3464,12 @@ static void commit_planes_do_stream_update(struct dc *dc,
 				dc->link_srv->set_dpms_on(dc->current_state, pipe_ctx);
 			}
 
-			if (stream_update->abm_level && pipe_ctx->stream_res.abm) {
-				bool should_program_abm = true;
-
-				// if otg funcs defined check if blanked before programming
-				if (pipe_ctx->stream_res.tg->funcs->is_blanked)
-					if (pipe_ctx->stream_res.tg->funcs->is_blanked(pipe_ctx->stream_res.tg))
-						should_program_abm = false;
-
-				if (should_program_abm) {
-					if (*stream_update->abm_level == ABM_LEVEL_IMMEDIATE_DISABLE) {
-						dc->hwss.set_abm_immediate_disable(pipe_ctx);
-					} else {
-						pipe_ctx->stream_res.abm->funcs->set_abm_level(
-							pipe_ctx->stream_res.abm, stream->abm_level);
-					}
-				}
+			if (stream_update->abm_level) {
+				dc_set_abm_level(dc, pipe_ctx,
+						 *stream_update->abm_level ==
+						 ABM_LEVEL_IMMEDIATE_DISABLE ?
+						 ABM_LEVEL_IMMEDIATE_DISABLE :
+						 stream->abm_level);
 			}
 		}
 	}
diff --git a/drivers/gpu/drm/amd/display/dc/dc.h b/drivers/gpu/drm/amd/display/dc/dc.h
index 7873daf72608..134ef00d9668 100644
--- a/drivers/gpu/drm/amd/display/dc/dc.h
+++ b/drivers/gpu/drm/amd/display/dc/dc.h
@@ -2494,6 +2494,8 @@ void dc_z10_save_init(struct dc *dc);
 bool dc_is_dmub_outbox_supported(struct dc *dc);
 bool dc_enable_dmub_notifications(struct dc *dc);
 
+void dc_set_abm_level(struct dc *dc, struct pipe_ctx *pipe_ctx, int level);
+
 bool dc_abm_save_restore(
 		struct dc *dc,
 		struct dc_stream_state *stream,
-- 
2.45.2

Re: [PATCH] drm/amd/display: change the panel power savings level without a modeset

[Mario Limonciello]

On 8/9/24 15:42, Hamza Mahfooz wrote:
> We don't actually need to request that the compositor does a full
> modeset to modify the panel power savings level, we can instead
> just make a request to DMUB, to set the new level dynamically.
> 
> Cc: Harry Wentland <harry.wentland@amd.com>
> Cc: Leo Li <sunpeng.li@amd.com>
> Cc: Mario Limonciello <mario.limonciello@amd.com>
> Cc: Sebastian Wick <sebastian@sebastianwick.net>
> Signed-off-by: Hamza Mahfooz <hamza.mahfooz@amd.com>
> ---

Thanks, this will solve the side effects that users of GNOME shell were 
seeing when the attribute was modified.

I tested it on an applicable laptop running 6.11-rc4 and it works 
correctly.  I have one nit below, but feel free to ignore it if you 
don't agree.

Here's some tags:

Tested-by: Mario Limonciello <mario.limonciello@amd.com>
Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
Closes: https://gitlab.gnome.org/GNOME/mutter/-/issues/3578

>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 17 +++++++-
>   drivers/gpu/drm/amd/display/dc/core/dc.c      | 39 +++++++++++--------
>   drivers/gpu/drm/amd/display/dc/dc.h           |  2 +
>   3 files changed, 41 insertions(+), 17 deletions(-)
> 
> 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 dd8353283bda..00a8a5959aa9 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -6819,9 +6819,14 @@ static ssize_t panel_power_savings_store(struct device *device,
>   					 const char *buf, size_t count)
>   {
>   	struct drm_connector *connector = dev_get_drvdata(device);
> +	struct amdgpu_dm_connector *aconn = to_amdgpu_dm_connector(connector);
>   	struct drm_device *dev = connector->dev;
> +	struct amdgpu_device *adev = drm_to_adev(dev);
> +	struct dc *dc = adev->dm.dc;
> +	struct pipe_ctx *pipe_ctx;
>   	long val;
>   	int ret;
> +	int i;
>   
>   	ret = kstrtol(buf, 0, &val);
>   
> @@ -6836,7 +6841,17 @@ static ssize_t panel_power_savings_store(struct device *device,
>   		ABM_LEVEL_IMMEDIATE_DISABLE;
>   	drm_modeset_unlock(&dev->mode_config.connection_mutex);
>   
> -	drm_kms_helper_hotplug_event(dev);
> +	mutex_lock(&adev->dm.dc_lock);
> +	for (i = 0; i < dc->res_pool->pipe_count; i++) {
> +		pipe_ctx = &dc->current_state->res_ctx.pipe_ctx[i];
> +
> +		if (pipe_ctx->stream &&
> +		    pipe_ctx->stream->link == aconn->dc_link) {
> +			dc_set_abm_level(dc, pipe_ctx, val);
> +			break;
> +		}
> +	}
> +	mutex_unlock(&adev->dm.dc_lock);
>   
>   	return count;
>   }
> diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c b/drivers/gpu/drm/amd/display/dc/core/dc.c
> index 3ba2acfdae2a..60081cd978b7 100644
> --- a/drivers/gpu/drm/amd/display/dc/core/dc.c
> +++ b/drivers/gpu/drm/amd/display/dc/core/dc.c
> @@ -3319,6 +3319,23 @@ static bool update_planes_and_stream_state(struct dc *dc,
>   
>   }
>   
> +void dc_set_abm_level(struct dc *dc, struct pipe_ctx *pipe_ctx, int level)
> +{
> +	struct timing_generator *tg = pipe_ctx->stream_res.tg;
> +	struct abm *abm = pipe_ctx->stream_res.abm;
> +
> +	if (!abm)
> +		return;

AFAICT this is a programmer error if this was to actually happen.
I'd think a WARN_ON() makes sense here.

> +
> +	if (tg->funcs->is_blanked && !tg->funcs->is_blanked(tg))
> +		tg->funcs->wait_for_state(tg, CRTC_STATE_VBLANK);
> +
> +	if (level == ABM_LEVEL_IMMEDIATE_DISABLE)
> +		dc->hwss.set_abm_immediate_disable(pipe_ctx);
> +	else
> +		abm->funcs->set_abm_level(abm, level);
> +}
> +
>   static void commit_planes_do_stream_update(struct dc *dc,
>   		struct dc_stream_state *stream,
>   		struct dc_stream_update *stream_update,
> @@ -3447,22 +3464,12 @@ static void commit_planes_do_stream_update(struct dc *dc,
>   				dc->link_srv->set_dpms_on(dc->current_state, pipe_ctx);
>   			}
>   
> -			if (stream_update->abm_level && pipe_ctx->stream_res.abm) {
> -				bool should_program_abm = true;
> -
> -				// if otg funcs defined check if blanked before programming
> -				if (pipe_ctx->stream_res.tg->funcs->is_blanked)
> -					if (pipe_ctx->stream_res.tg->funcs->is_blanked(pipe_ctx->stream_res.tg))
> -						should_program_abm = false;
> -
> -				if (should_program_abm) {
> -					if (*stream_update->abm_level == ABM_LEVEL_IMMEDIATE_DISABLE) {
> -						dc->hwss.set_abm_immediate_disable(pipe_ctx);
> -					} else {
> -						pipe_ctx->stream_res.abm->funcs->set_abm_level(
> -							pipe_ctx->stream_res.abm, stream->abm_level);
> -					}
> -				}
> +			if (stream_update->abm_level) {
> +				dc_set_abm_level(dc, pipe_ctx,
> +						 *stream_update->abm_level ==
> +						 ABM_LEVEL_IMMEDIATE_DISABLE ?
> +						 ABM_LEVEL_IMMEDIATE_DISABLE :
> +						 stream->abm_level);
>   			}
>   		}
>   	}
> diff --git a/drivers/gpu/drm/amd/display/dc/dc.h b/drivers/gpu/drm/amd/display/dc/dc.h
> index 7873daf72608..134ef00d9668 100644
> --- a/drivers/gpu/drm/amd/display/dc/dc.h
> +++ b/drivers/gpu/drm/amd/display/dc/dc.h
> @@ -2494,6 +2494,8 @@ void dc_z10_save_init(struct dc *dc);
>   bool dc_is_dmub_outbox_supported(struct dc *dc);
>   bool dc_enable_dmub_notifications(struct dc *dc);
>   
> +void dc_set_abm_level(struct dc *dc, struct pipe_ctx *pipe_ctx, int level);
> +
>   bool dc_abm_save_restore(
>   		struct dc *dc,
>   		struct dc_stream_state *stream,