Re: [PATCH] drm/amd/display: set remote MST sink power state on payload changes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The patch introduces calls to sleeping functions (`usleep_range` and `mutex_lock`) within the display hardware sequencer path.
--

commit 407e2ac8f541eab206fd802accc0990f9bef0e0a
Author: Maximilian Passarello <git@0xMax42.io>

drm/amd/display: set remote MST sink power state on payload changes

This commit addresses downstream MST displays keeping their backlight on
after DPMS off by setting the remote sink to D0 before allocating its
payload and to D3 before removing it.

> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> index ab15ceec4daa0..ce27024a95933 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
[ ... ]
> @@ -388,11 +389,23 @@ bool dm_helpers_dp_mst_write_payload_allocation_table(
>  	new_payload = drm_atomic_get_mst_payload_state(mst_state, aconnector->mst_output_port);
>  
>  	if (enable) {
> +		ret = drm_dp_link_power_up(&aconnector->mst_output_port->aux,
> +					   aconnector->mst_output_port->dpcd_rev);

[Severity: High]
Does calling drm_dp_link_power_up() here introduce a sleeping function into
an atomic hardware sequencer path?

The dm_helpers_dp_mst_write_payload_allocation_table() function is called by
the hardware sequencer during MST payload updates. Looking at
drm_dp_link_power_up() in drm_dp_helper.c, it internally uses usleep_range()
and takes a mutex via drm_dp_dpcd_readb() during DPCD auxiliary access.

Calling sleeping functions from atomic commit tail variants and hardware
sequencer paths can cause "scheduling while atomic" warnings, system
instability, and deadlocks.

> +		if (ret < 0)
> +			drm_dbg_dp(mst_mgr->dev,
> +				   "Failed to power up remote MST sink: %d\n", ret);
> +
>  		target_payload = new_payload;
>  
>  		/* It's OK for this to fail */
>  		drm_dp_add_payload_part1(mst_mgr, mst_state, new_payload);
>  	} else {
> +		ret = drm_dp_link_power_down(&aconnector->mst_output_port->aux,
> +					     aconnector->mst_output_port->dpcd_rev);

[Severity: High]
Similarly, does drm_dp_link_power_down() introduce sleeping operations in
the atomic disable sequence?

Like the enable path, this introduces DPCD mutex locks into an atomic
hardware sequencer path where sleeping is forbidden.

> +		if (ret < 0)
> +			drm_dbg_dp(mst_mgr->dev,
> +				   "Failed to power down remote MST sink: %d\n", ret);
> +
>  		/* construct old payload by VCPI*/
>  		dm_helpers_construct_old_payload(mst_mgr, mst_state,
>  						 new_payload, &old_payload);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260829-mst-remote-sink-power-v1-1-b9eab331e0d5@0xMax42.io?part=1