Handle aborted suspend better
Subsystems (3)
cover Handle aborted suspend better
From: Mario Limonciello <[email protected]>
Chris Bainbridge reported some list corruption occurring around the suspend sequence when an aborted suspend occurs.
I couldn't reproduce this specific problem, but when I tried I found some other issues where the cached DM state isn't properly destroyed.
This is because there isn't a complete() callback to match the prepare() callback used by amdgpu. Normally the PM core will call complete() after every suspend attempt (succesful or not).
Mario Limonciello (3):
drm/amd: Add support for a complete pmops action drm/amd/display: Stop storing failures into adev->dm.cached_state drm/amd/display: Destroy cached state in complete() callback
drivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 + drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 22 +++ drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 2 +- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 125 +++++++++++------- drivers/gpu/drm/amd/include/amd_shared.h | 1 + 5 files changed, 103 insertions(+), 48 deletions(-)
Signature · 1 line
2.43.0
PATCH 1/3 drm/amd: Add support for a complete pmops action
From: Mario Limonciello <[email protected]>
complete() callbacks are supposed to handle reversing anything that occurred during prepare() callbacks. They'll be called on every power state transition, and will also be called if the sequence is failed (such as an aborted suspend).
Add support for IP blocks to support this action.
--- drivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 + drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 22 ++++++++++++++++++++++ drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 2 +- drivers/gpu/drm/amd/include/amd_shared.h | 1 + 4 files changed, 25 insertions(+), 1 deletion(-)
drivers/gpu/drm/amd/amdgpu/amdgpu.h +1 -0
@@ -1619,6 +1619,7 @@ void amdgpu_driver_release_kms(struct drm_device *dev);int amdgpu_device_ip_suspend (struct amdgpu_device *adev);int amdgpu_device_prepare (struct drm_device *dev); +void amdgpu_device_complete (struct drm_device *dev);int amdgpu_device_suspend (struct drm_device *dev,bool fbcon);int amdgpu_device_resume (struct drm_device *dev,bool fbcon);u32 amdgpu_get_vblank_counter_kms (struct drm_crtc *crtc);
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +22 -0
@@ -5034,6 +5034,28 @@ int amdgpu_device_prepare(struct drm_device *dev)return 0 ; } +/** +* amdgpu_device_complete - complete power state transition +* +* @dev: drm dev pointer +* +* Undo the changes from amdgpu_device_prepare. This will be +* called on all resume transitions, including those that failed. +*/ +void amdgpu_device_complete (struct drm_device *dev) +{ +struct amdgpu_device *adev =drm_to_adev (dev); +int i; + +for (i =0 ; i < adev->num_ip_blocks; i++) { +if (!adev->ip_blocks[i].status.valid) +continue ; +if (!adev->ip_blocks[i].version->funcs->complete) +continue ; + adev->ip_blocks[i].version->funcs->complete (&adev->ip_blocks[i]); + } +} +/** * amdgpu_device_suspend - initiate device suspend *
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +1 -1
@@ -2570,7 +2570,7 @@ static int amdgpu_pmops_prepare(struct device *dev)static void amdgpu_pmops_complete (struct device *dev) { -/* nothing to do */ +amdgpu_device_complete (dev_get_drvdata (dev)); }static int amdgpu_pmops_suspend (struct device *dev)
drivers/gpu/drm/amd/include/amd_shared.h +1 -0
@@ -427,6 +427,7 @@ struct amd_ip_funcs {int (*prepare_suspend)(struct amdgpu_ip_block *ip_block);int (*suspend)(struct amdgpu_ip_block *ip_block);int (*resume)(struct amdgpu_ip_block *ip_block); +void (*complete)(struct amdgpu_ip_block *ip_block);bool (*is_idle)(struct amdgpu_ip_block *ip_block);int (*wait_for_idle)(struct amdgpu_ip_block *ip_block);bool (*check_soft_reset)(struct amdgpu_ip_block *ip_block);
Signature · 1 line
2.43.0
PATCH 2/3 drm/amd/display: Stop storing failures into adev->dm.cached_state
In reply to Mario LimoncielloFrom: Mario Limonciello <[email protected]>
If drm_atomic_helper_suspend() has failed for any reason, it's stored in adev->dm.cached_state. This isn't expected because the resume (or complete()) sequence will attempt to use the stored state to resume.
--- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 25 +++++++++++++------ 1 file changed, 18 insertions(+), 7 deletions(-)
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +18 -7
@@ -3060,6 +3060,19 @@ static void hpd_rx_irq_work_suspend(struct amdgpu_display_manager *dm) } } +static int dm_cache_state (struct amdgpu_device *adev) +{ +int r; + + adev->dm.cached_state =drm_atomic_helper_suspend (adev_to_drm (adev)); +if (IS_ERR (adev->dm.cached_state)) { + r =PTR_ERR (adev->dm.cached_state); + adev->dm.cached_state =NULL ; + } + +return adev->dm.cached_state ?0 : r; +} +static int dm_prepare_suspend (struct amdgpu_ip_block *ip_block) {struct amdgpu_device *adev = ip_block->adev;
@@ -3068,11 +3081,8 @@ static int dm_prepare_suspend(struct amdgpu_ip_block *ip_block)return 0 ;WARN_ON (adev->dm.cached_state); - adev->dm.cached_state =drm_atomic_helper_suspend (adev_to_drm (adev)); -if (IS_ERR (adev->dm.cached_state)) -return PTR_ERR (adev->dm.cached_state); -return 0 ; +return dm_cache_state (adev); }static int dm_suspend (struct amdgpu_ip_block *ip_block)
@@ -3106,9 +3116,10 @@ static int dm_suspend(struct amdgpu_ip_block *ip_block) }if (!adev->dm.cached_state) { - adev->dm.cached_state =drm_atomic_helper_suspend (adev_to_drm (adev)); -if (IS_ERR (adev->dm.cached_state)) -return PTR_ERR (adev->dm.cached_state); +int r =dm_cache_state (adev); + +if (r) +return r; }s3_handle_hdmi_cec (adev_to_drm (adev), true);
Signature · 1 line
2.43.0
PATCH 3/3 drm/amd/display: Destroy cached state in complete() callback
In reply to Mario LimoncielloFrom: Mario Limonciello <[email protected]>
[Why]
When the suspend sequence has been aborted after prepare() but before suspend() the resume() callback never gets called. The PM core will call complete() when this happens. As the state has been cached in prepare() it needs to be destroyed in complete() if it's still around.
[How]
Create a helper for destroying cached state and call it both in resume() and complete() callbacks. If resume has been called the state will be destroyed and it's a no-op for complete(). If resume hasn't been called (such as an aborted suspend) then destroy the state in complete().
--- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 100 +++++++++++------- 1 file changed, 60 insertions(+), 40 deletions(-)
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +60 -40
@@ -3073,6 +3073,64 @@ static int dm_cache_state(struct amdgpu_device *adev)return adev->dm.cached_state ?0 : r; } +static void dm_destroy_cached_state (struct amdgpu_device *adev) +{ +struct amdgpu_display_manager *dm = &adev->dm; +struct drm_device *ddev =adev_to_drm (adev); +struct dm_plane_state *dm_new_plane_state; +struct drm_plane_state *new_plane_state; +struct dm_crtc_state *dm_new_crtc_state; +struct drm_crtc_state *new_crtc_state; +struct drm_plane *plane; +struct drm_crtc *crtc; +int i; + +if (!dm->cached_state) +return ; + +/* Force mode set in atomic commit */ +for_each_new_crtc_in_state (dm->cached_state, crtc, new_crtc_state, i) { + new_crtc_state->active_changed = true; + dm_new_crtc_state =to_dm_crtc_state (new_crtc_state); +reset_freesync_config_for_crtc (dm_new_crtc_state); + } + +/* +* atomic_check is expected to create the dc states. We need to release +* them here, since they were duplicated as part of the suspend +* procedure. +*/ +for_each_new_crtc_in_state (dm->cached_state, crtc, new_crtc_state, i) { + dm_new_crtc_state =to_dm_crtc_state (new_crtc_state); +if (dm_new_crtc_state->stream) { +WARN_ON (kref_read (&dm_new_crtc_state->stream->refcount) >1 ); +dc_stream_release (dm_new_crtc_state->stream); + dm_new_crtc_state->stream =NULL ; + } + dm_new_crtc_state->base.color_mgmt_changed = true; + } + +for_each_new_plane_in_state (dm->cached_state, plane, new_plane_state, i) { + dm_new_plane_state =to_dm_plane_state (new_plane_state); +if (dm_new_plane_state->dc_state) { +WARN_ON (kref_read (&dm_new_plane_state->dc_state->refcount) >1 ); +dc_plane_state_release (dm_new_plane_state->dc_state); + dm_new_plane_state->dc_state =NULL ; + } + } + +drm_atomic_helper_resume (ddev, dm->cached_state); + + dm->cached_state =NULL ; +} + +static void dm_complete (struct amdgpu_ip_block *ip_block) +{ +struct amdgpu_device *adev = ip_block->adev; + +dm_destroy_cached_state (adev); +} +static int dm_prepare_suspend (struct amdgpu_ip_block *ip_block) {struct amdgpu_device *adev = ip_block->adev;
@@ -3306,12 +3364,6 @@ static int dm_resume(struct amdgpu_ip_block *ip_block)struct amdgpu_dm_connector *aconnector;struct drm_connector *connector;struct drm_connector_list_iter iter; -struct drm_crtc *crtc; -struct drm_crtc_state *new_crtc_state; -struct dm_crtc_state *dm_new_crtc_state; -struct drm_plane *plane; -struct drm_plane_state *new_plane_state; -struct dm_plane_state *dm_new_plane_state;struct dm_atomic_state *dm_state =to_dm_atomic_state (dm->atomic_obj.state);enum dc_connection_type new_connection_type = dc_connection_none;struct dc_state *dc_state;
@@ -3468,40 +3520,7 @@ static int dm_resume(struct amdgpu_ip_block *ip_block) }drm_connector_list_iter_end (&iter); -/* Force mode set in atomic commit */ -for_each_new_crtc_in_state (dm->cached_state, crtc, new_crtc_state, i) { - new_crtc_state->active_changed = true; - dm_new_crtc_state =to_dm_crtc_state (new_crtc_state); -reset_freesync_config_for_crtc (dm_new_crtc_state); - } - -/* -* atomic_check is expected to create the dc states. We need to release -* them here, since they were duplicated as part of the suspend -* procedure. -*/ -for_each_new_crtc_in_state (dm->cached_state, crtc, new_crtc_state, i) { - dm_new_crtc_state =to_dm_crtc_state (new_crtc_state); -if (dm_new_crtc_state->stream) { -WARN_ON (kref_read (&dm_new_crtc_state->stream->refcount) >1 ); -dc_stream_release (dm_new_crtc_state->stream); - dm_new_crtc_state->stream =NULL ; - } - dm_new_crtc_state->base.color_mgmt_changed = true; - } - -for_each_new_plane_in_state (dm->cached_state, plane, new_plane_state, i) { - dm_new_plane_state =to_dm_plane_state (new_plane_state); -if (dm_new_plane_state->dc_state) { -WARN_ON (kref_read (&dm_new_plane_state->dc_state->refcount) >1 ); -dc_plane_state_release (dm_new_plane_state->dc_state); - dm_new_plane_state->dc_state =NULL ; - } - } - -drm_atomic_helper_resume (ddev, dm->cached_state); - - dm->cached_state =NULL ; +dm_destroy_cached_state (adev);/* Do mst topology probing after resuming cached state*/ drm_connector_list_iter_begin (ddev, &iter);
@@ -3550,6 +3569,7 @@ static const struct amd_ip_funcs amdgpu_dm_funcs = { .prepare_suspend = dm_prepare_suspend, .suspend = dm_suspend, .resume = dm_resume, + .complete = dm_complete, .is_idle = dm_is_idle, .wait_for_idle = dm_wait_for_idle, .check_soft_reset = dm_check_soft_reset,
Signature · 1 line
2.43.0
Quoted 19 lines · Mario Limonciello
From: Mario Limonciello <[email protected]>
Chris Bainbridge reported some list corruption occurring around the suspend sequence when an aborted suspend occurs.
I couldn't reproduce this specific problem, but when I tried I found some other issues where the cached DM state isn't properly destroyed.
This is because there isn't a
complete()callback to match theprepare()callback used by amdgpu. Normally the PM core will callcomplete()after every suspend attempt (succesful or not).Mario Limonciello (3):
drm/amd: Add support for a complete pmops action drm/amd/display: Stop storing failures intoadev->dm.cached_statedrm/amd/display: Destroy cached state incomplete()callback
Quoted 10 lines
drivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 + drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 22 +++ drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 2 +- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 125 +++++++++++------- drivers/gpu/drm/amd/include/amd_shared.h | 1 + 5 files changed, 103 insertions(+), 48 deletions(-)Signature · 1 line
2.43.0
I tested with 30 suspends and the dm_prepare_suspend / amdgpu_device_prepare error did not appear. The list corruption error remain but that bisects to:
aa7a9275ab81 ("PM: sleep: Suspend async parents after suspending children").
I applied your patch series to the parent of that commit, tested, and there were no errors. So this issue looks fixed but the other issue remains, email sent: https://lore.kernel.org/all/[email protected]/
This patchset passed promotion tests along with DC 3.2.337 too.
Quoted 26 lines · Mario Limonciello
From: Mario Limonciello <[email protected]>
Chris Bainbridge reported some list corruption occurring around the suspend sequence when an aborted suspend occurs.
I couldn't reproduce this specific problem, but when I tried I found some other issues where the cached DM state isn't properly destroyed.
This is because there isn't a
complete()callback to match theprepare()callback used by amdgpu. Normally the PM core will callcomplete()after every suspend attempt (succesful or not).Mario Limonciello (3):
drm/amd: Add support for a complete pmops action drm/amd/display: Stop storing failures intoadev->dm.cached_statedrm/amd/display: Destroy cached state incomplete()callbackdrivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 + drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 22 +++ drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 2 +- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 125 +++++++++++------- drivers/gpu/drm/amd/include/amd_shared.h | 1 + 5 files changed, 103 insertions(+), 48 deletions(-)