On 2026-03-06 07:06, Yussuf Khalil wrote: > [Some people who received this message don't often get email from > [email protected]. Learn why this is important at > https://aka.ms/LearnAboutSenderIdentification ] > > Starting with commit 17ce8a6907f7 ("drm/amd/display: Add dsc pre-validation in > atomic check"), amdgpu resets the CRTC state mode_changed flag to false when > recomputing the DSC configuration results in no timing change for a particular > stream. > > However, this is incorrect in scenarios where a change in MST/DSC > configuration > happens in the same KMS commit as another (unrelated) mode change. For > example, > the integrated panel of a laptop may be configured differently (e.g., HDR > enabled/disabled) depending on whether external screens are attached. In this > case, plugging in external DP-MST screens may result in the mode_changed flag > being dropped incorrectly for the integrated panel if its DSC configuration > did not change during precomputation in pre_validate_dsc(). > > At this point, however, dm_update_crtc_state() has already created new streams > for CRTCs with DSC-independent mode changes. In turn, > amdgpu_dm_commit_streams() will never release the old stream, resulting in a > memory leak. amdgpu_dm_atomic_commit_tail() will never acquire a reference to > the new stream either, which manifests as a use-after-free when the stream > gets > disabled later on: > > BUG: KASAN: use-after-free in dc_stream_release+0x25/0x90 [amdgpu] > Write of size 4 at addr ffff88813d836524 by task kworker/9:9/29977 > > Workqueue: events drm_mode_rmfb_work_fn > Call Trace: > <TASK> > dump_stack_lvl+0x6e/0xa0 > print_address_description.constprop.0+0x88/0x320 > ? dc_stream_release+0x25/0x90 [amdgpu] > print_report+0xfc/0x1ff > ? srso_alias_return_thunk+0x5/0xfbef5 > ? __virt_addr_valid+0x225/0x4e0 > ? dc_stream_release+0x25/0x90 [amdgpu] > kasan_report+0xe1/0x180 > ? dc_stream_release+0x25/0x90 [amdgpu] > kasan_check_range+0x125/0x200 > dc_stream_release+0x25/0x90 [amdgpu] > dc_state_destruct+0x14d/0x5c0 [amdgpu] > dc_state_release.part.0+0x4e/0x130 [amdgpu] > dm_atomic_destroy_state+0x3f/0x70 [amdgpu] > drm_atomic_state_default_clear+0x8ee/0xf30 > ? drm_mode_object_put.part.0+0xb1/0x130 > __drm_atomic_state_free+0x15c/0x2d0 > atomic_remove_fb+0x67e/0x980 > > Since there is no reliable way of figuring out whether a CRTC has unrelated > mode changes pending at the time of DSC validation, remember the value of the > mode_changed flag from before the point where a CRTC was marked as potentially > affected by a change in DSC configuration. Reset the mode_changed flag to this > earlier value instead in pre_validate_dsc(). > > Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/5004 > Fixes: 17ce8a6907f7 ("drm/amd/display: Add dsc pre-validation in atomic > check") > Signed-off-by: Yussuf Khalil <[email protected]>
Thanks for this commit. It looks great. Reviewed-by: Harry Wentland <[email protected]> Harry > --- > drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 +++++ > drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 1 + > drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c | 4 +++- > 3 files changed, 9 insertions(+), 1 deletion(-) > > 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 b3d6f2cd8..4efd77477 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > @@ -12523,6 +12523,11 @@ static int amdgpu_dm_atomic_check(struct drm_device > *dev, > } > > if (dc_resource_is_dsc_encoding_supported(dc)) { > + for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, > new_crtc_state, i) { > + dm_new_crtc_state = to_dm_crtc_state(new_crtc_state); > + dm_new_crtc_state->mode_changed_independent_from_dsc > = new_crtc_state->mode_changed; > + } > + > for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, > new_crtc_state, i) { > if (drm_atomic_crtc_needs_modeset(new_crtc_state)) { > ret = add_affected_mst_dsc_crtcs(state, crtc); > 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 800813671..d15812d51 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h > @@ -984,6 +984,7 @@ struct dm_crtc_state { > > bool freesync_vrr_info_changed; > > + bool mode_changed_independent_from_dsc; > bool dsc_force_changed; > bool vrr_supported; > struct mod_freesync_config freesync_config; > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c > index 7be50e8c0..5d8c4c702 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c > @@ -1744,9 +1744,11 @@ int pre_validate_dsc(struct drm_atomic_state *state, > int ind = find_crtc_index_in_state_by_stream(state, > stream); > > if (ind >= 0) { > + struct dm_crtc_state *dm_new_crtc_state = > to_dm_crtc_state(state->crtcs[ind].new_state); > + > DRM_INFO_ONCE("%s:%d MST_DSC no mode changed > for stream 0x%p\n", > __func__, __LINE__, stream); > - state->crtcs[ind].new_state->mode_changed = 0; > + dm_new_crtc_state->base.mode_changed = > dm_new_crtc_state->mode_changed_independent_from_dsc; > } > } > } > -- > 2.53.0 > >
