drm/amd/display: Fix ISM dc_lock deadlock during suspend
PATCH 1/1 drm/amd/display: Fix ISM dc_lock deadlock during suspend
From: Ray Wu <[email protected]>
[ Upstream commit 3714fe242592e3699ac5e2c19d68b275a210be7d ]
CachyOS users reported a regression in shutdown/reboot behavior on 7.1
kernels: the display turns off, but the machine does not power down. Reverting ISM fixes the regression, and this upstream fix addresses the same ISM dc_lock/workqueue deadlock in the suspend/shutdown paths.
[Why]
System hang observed during suspend/resume while video is playing. amdgpu_dm_ism_disable() is called under dc_lock and waits for ISM delayed work via disable_delayed_work_sync(). The work handlers themselves take dc_lock, producing an ABBA deadlock when a worker is in flight at suspend time.
[How]
Split the disable path into two phases with opposite locking contracts:
1. amdgpu_dm_ism_disable() -- quiesces workers, must NOT hold dc_lock.
2. amdgpu_dm_ism_force_full_power() (new) -- drives the ISM FSM back to FULL_POWER_RUNNING, must hold dc_lock.
--- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 25 +++++++-- .../drm/amd/display/amdgpu_dm/amdgpu_dm_ism.c | 56 ++++++++++++++++--- .../drm/amd/display/amdgpu_dm/amdgpu_dm_ism.h | 1 + 3 files changed, 70 insertions(+), 12 deletions(-)
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +21 -4
@@ -2260,9 +2260,16 @@ static void amdgpu_dm_fini(struct amdgpu_device *adev) adev->dm.idle_workqueue =NULL ; } -/* Disable ISM before dc_destroy() invalidates dm->dc */ +/* +* Disable ISM before dc_destroy() invalidates dm->dc. +* +* Quiesce workers first without dc_lock (they take dc_lock +* themselves, so syncing under it would deadlock), then drive the +* FSM back to FULL_POWER_RUNNING under dc_lock. +*/ +amdgpu_dm_ism_disable (&adev->dm); scoped_guard(mutex, &adev->dm.dc_lock) -amdgpu_dm_ism_disable (&adev->dm); +amdgpu_dm_ism_force_full_power (&adev->dm);amdgpu_dm_destroy_drm_device (&adev->dm);
@@ -3290,9 +3297,14 @@ static int dm_suspend(struct amdgpu_ip_block *ip_block)if (amdgpu_in_reset (adev)) {enum dc_status res; +/* Quiesce ISM workers before taking dc_lock (workers take +* dc_lock themselves; syncing under it would deadlock). +*/ +amdgpu_dm_ism_disable (dm); +mutex_lock (&dm->dc_lock); -amdgpu_dm_ism_disable (dm); +amdgpu_dm_ism_force_full_power (dm);dc_allow_idle_optimizations (adev->dm.dc, false); dm->cached_dc_state =dc_state_create_copy (dm->dc->current_state);
@@ -3326,8 +3338,13 @@ static int dm_suspend(struct amdgpu_ip_block *ip_block)amdgpu_dm_irq_suspend (adev); +/* +* Quiesce ISM workers before taking dc_lock (workers take dc_lock +* themselves; syncing under it would deadlock). +*/ +amdgpu_dm_ism_disable (dm); scoped_guard(mutex, &dm->dc_lock) -amdgpu_dm_ism_disable (dm); +amdgpu_dm_ism_force_full_power (dm);hpd_rx_irq_work_suspend (dm);
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_ism.c +48 -8
82 lines not on this page · raw message
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_ism.h +1 -0
@@ -146,6 +146,7 @@ void amdgpu_dm_ism_fini(struct amdgpu_dm_ism *ism);void amdgpu_dm_ism_commit_event (struct amdgpu_dm_ism *ism,enum amdgpu_dm_ism_event event);void amdgpu_dm_ism_disable (struct amdgpu_display_manager *dm); +void amdgpu_dm_ism_force_full_power (struct amdgpu_display_manager *dm);void amdgpu_dm_ism_enable (struct amdgpu_display_manager *dm);#endif
Signature · 1 line
2.54.0
Quoted 8 lines
From: Ray Wu <[email protected]>
[ Upstream commit
3714fe242592e3699ac5e2c19d68b275a210be7d]CachyOS users reported a regression in shutdown/reboot behavior on 7.1
kernels: the display turns off, but the machine does not power down. Reverting ISM fixes the regression, and this upstream fix addresses the same ISM dc_lock/workqueue deadlock in the suspend/shutdown paths.
Queued for 7.1, thanks.
Signature · 2 lines
Thanks, Sasha
[email protected]
AI reviewThank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [Critical] The idle_workqueue is freed without correctly cancelling pending work, leading to a Use-After-Free and workqueue corruption.
- [High] Missing mutex_unlock(&dm->dc_lock) on error paths in the GPU reset path of dm_suspend, leading to a permanent deadlock. --
commit 147ffd2a09d9a0380b42af11648a4a34c2fd7170
Author: Ray Wu <[email protected]> drm/amd/display: Fix ISM dc_lock deadlock during suspend
This patch fixes an ABBA deadlock during suspend and resume by splitting the ISM disable path. Workers are first quiesced without dc_lock, and then the FSM is driven back to FULL_POWER_RUNNING under dc_lock.
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c static void amdgpu_dm_fini(struct amdgpu_device *adev) · line 2262
Quoted 8 lines
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +0
-0@@ -2260,9 +2260,16 @@ static void amdgpu_dm_fini(struct amdgpu_device *adev) adev->dm.idle_workqueue =NULL ; }
[Severity: Critical]
This is a pre-existing issue, but is it possible for the idle_workqueue to be freed while work is still pending?
In amdgpu_dm_fini(), flush_work() is only called if idle_workqueue->running is true:
if (adev->dm.idle_workqueue->running) {
adev->dm.idle_workqueue->enable = false;
flush_work(&adev->dm.idle_workqueue->work);
}kfree(adev->dm.idle_workqueue);
If schedule_work() was called but the worker hasn't started executing yet, running remains false.
This means the cleanup logic will skip flush_work() and immediately kfree() the structure. When the workqueue later processes the queued item, it will access freed memory, causing a use-after-free.
[ ... ]
Quoted 6 lines
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +2
-0@@ -3290,9 +3297,14 @@ static int dm_suspend(struct amdgpu_ip_block *ip_block)if (amdgpu_in_reset (adev)) {enum dc_status res; +/* Quiesce ISM workers before taking dc_lock (workers take +* dc_lock themselves; syncing under it would deadlock).
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c static int dm_suspend(struct amdgpu_ip_block *ip_block) · line 3310
Quoted 10 lines
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +4
-1+*/ +amdgpu_dm_ism_disable (dm); +mutex_lock (&dm->dc_lock); -amdgpu_dm_ism_disable (dm); +amdgpu_dm_ism_force_full_power (dm);dc_allow_idle_optimizations (adev->dm.dc, false); dm->cached_dc_state =dc_state_create_copy (dm->dc->current_state);
[Severity: High]
This isn't a bug introduced by this patch, but does the error path for amdgpu_dm_commit_zero_streams() leave dm->dc_lock permanently locked?
Just below this hunk in dm_suspend(), the function handles a failure by returning immediately:
res = amdgpu_dm_commit_zero_streams(dm->dc);
if (res != DC_OK) {
drm_err(adev_to_drm(adev), "Failed to commit zero streams: %d\n", res);
return -EINVAL;
}Since mutex_lock(&dm->dc_lock) was acquired right before this, the early return skips unlocking the mutex.
This leaves the dc_lock permanently acquired, leading to a system deadlock on subsequent accesses to the display manager.
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1