Re: [PATCH 04/19] drm/amd/display/amdgpu_dm: Use DC accessors in audio/color/backlight paths

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 2 related pre-existing issues it does not
introduce:
- [Critical] drm/amd/display: race and invalid downcast in
  amdgpu_dm_backlight_set_level() [1]
- [Medium] drm/amd/display: swallowed error in
  amdgpu_dm_update_plane_color_mgmt() [2]
--

commit c710d624f30d7a2d26cb80389657ac035d1c1f5e
Author: Dave Airlie <airlied@redhat.com>

drm/amd/display/amdgpu_dm: Use DC accessors in audio/color/backlight paths

This commit replaces direct struct dc member accesses with DC accessor
calls in the audio, backlight, and color management paths.

> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c
> index cb74b00723fda..3d268703c5514 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c
> @@ -296,7 +296,7 @@ struct dc_stream_state *dm_find_stream_with_link(
>  	struct amdgpu_display_manager *dm,
>  	struct dc_link *link)
>  {
> -	struct dc_state *cur_dc_state = dm->dc->current_state;
> +	struct dc_state *cur_dc_state = dc_get_current_state(dm->dc);
>  	struct dc_stream_state *stream = NULL;
>  	int i;
>  
> @@ -383,7 +383,7 @@ void amdgpu_dm_backlight_set_level(struct amdgpu_display_manager *dm,
>  	}
>  
>  	mutex_lock(&dm->dc_lock);
> -	if (dm->dc->caps.ips_support && dm->dc->ctx->dmub_srv->idle_allowed) {
> +	if (dc_get_caps(dm->dc)->ips_support && dc_get_ctx(dm->dc)->dmub_srv->idle_allowed) {
>  		dc_allow_idle_optimizations(dm->dc, false);
>  		reallow_idle = true;
>  	}

[Severity: Critical]
This problem wasn't introduced by this patch, but can
amdgpu_dm_backlight_set_level() race with a concurrent atomic commit and
use cur_dc_state or stream after it has been freed [1]?

amdgpu_dm_backlight_set_level() calls dm_find_stream_with_link() before
acquiring dm->dc_lock:

drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c:amdgpu_dm_backlight_set_level() {
	...
	list_for_each_entry(connector, &dm->ddev->mode_config.connector_list, head) {
		struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector);
		...
	}
	...
	link = (struct dc_link *)dm->backlight_link[bl_idx];
	stream = dm_find_stream_with_link(dm, link);
	if (!stream)
		return;
	...
	mutex_lock(&dm->dc_lock);
}

A concurrent commit can replace current_state and release the old state
and its streams while dm_find_stream_with_link() is walking it or before
dm->dc_lock is taken:

CPU0                                  CPU1
amdgpu_dm_backlight_set_level()
  dm_find_stream_with_link()
    cur_dc_state =
      dc_get_current_state(dm->dc)
    stream = cur_dc_state->streams[i]
                                      amdgpu_dm_commit_streams()
                                        mutex_lock(&dm->dc_lock)
                                        dc_commit_streams()
                                          /* replaces current_state,
                                             releases old state/streams */
                                        mutex_unlock(&dm->dc_lock)
  mutex_lock(&dm->dc_lock)
  mod_power_set_backlight_nits(...,
    stream, ...)                      <-- use-after-free

In addition, the loop at the start of amdgpu_dm_backlight_set_level()
walks mode_config.connector_list with list_for_each_entry() instead of
drm_connector_list_iter_begin()/end(), which can race with DP MST
connector removal, and unconditionally calls to_amdgpu_dm_connector() on
every entry, which causes type confusion on DRM_MODE_CONNECTOR_WRITEBACK
connectors (allocated as struct amdgpu_dm_wb_connector).

> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
> index f9dc9f46a4460..d86b9611568b8 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c

[ ... ]

> @@ -2156,7 +2162,7 @@ int amdgpu_dm_update_plane_color_mgmt(struct dm_crtc_state *crtc,
>  	struct amdgpu_device *adev = drm_to_adev(crtc->base.state->dev);
>  	struct dm_plane_state *dm_plane_state = to_dm_plane_state(plane_state);
>  	struct drm_color_ctm_3x4 *ctm = NULL;
> -	struct dc_color_caps *color_caps = NULL;
> +	const struct dc_color_caps *color_caps = NULL;
>  	bool has_crtc_cm_degamma;
>  	int ret;
>  
> @@ -2167,7 +2173,7 @@ int amdgpu_dm_update_plane_color_mgmt(struct dm_crtc_state *crtc,
>  	}
>  
>  	if (dc_plane_state->ctx && dc_plane_state->ctx->dc)
> -		color_caps = &dc_plane_state->ctx->dc->caps.color;
> +		color_caps = &dc_get_caps(dc_plane_state->ctx->dc)->color;
>  
>  	/* Initially, we can just bypass the DGM block. */
>  	dc_plane_state->in_transfer_func.type = TF_TYPE_BYPASS;

[Severity: Medium]
This problem wasn't introduced by this patch, but does
amdgpu_dm_update_plane_color_mgmt() swallow errors from
amdgpu_dm_plane_set_colorop_properties() [2]?

At the end of amdgpu_dm_update_plane_color_mgmt():

drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c:amdgpu_dm_update_plane_color_mgmt() {
	...
	if (!amdgpu_dm_plane_set_colorop_properties(plane_state, dc_plane_state))
		return 0;

	return amdgpu_dm_plane_set_color_properties(plane_state, dc_plane_state);
}

When plane_state->color_pipeline is set and
amdgpu_dm_plane_set_colorop_properties() fails with a negative error code
(such as -EINVAL or -ENOMEM), the ! check evaluates to false and execution
falls through to amdgpu_dm_plane_set_color_properties(). Because no legacy
color properties are set on a colorop commit, the fallback bypasses the
color blocks and returns 0, causing the atomic commit to succeed with
plane color management silently disabled.

[1] https://sashiko.dev/#/bug/linux-d25140b3-5901-4689-81c3-c3890f66e5a2
[2] https://sashiko.dev/#/bug/linux-2356891c-983c-48f7-a656-56667766fead

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112-amdgpu-dc-cleanup-v1-0-0fae41718ad2@redhat.com?part=4