Hi Wayne, On 12.12.23 17:06, Mario Limonciello wrote: > I looked through your bugs related to this and I didn't see a reference to the > specific docking station model. > The logs mentioned "Thinkpad dock" but no model. > Could you share more about it so that AMD can try to reproduce it? Yes, it is a ThinkPad Ultra Dockingstation, part number 40AJ0135EU, see also https://support.lenovo.com/us/en/solutions/pd500173-thinkpad-ultra-docking-station-overview-and-service-parts Best regards, Oliver On 12.12.23 17:06, Mario Limonciello wrote: > On 12/12/2023 04:10, Lin, Wayne wrote: >> [Public] >> >> Hi Mario, >> >> Thanks for the help. >> My feeling is like this problem probably relates to specific dock. Need time >> to take >> further look. > > Oliver, > > I looked through your bugs related to this and I didn't see a reference to the > specific docking station model. > The logs mentioned "Thinkpad dock" but no model. > > Could you share more about it so that AMD can try to reproduce it? > >> >> Since reverting solves the issue now, feel free to add: >> Acked-by: Wayne Lin <wayne.lin@xxxxxxx> > > Sure, thanks. > >> >> Thanks, >> Wayne >> >>> -----Original Message----- >>> From: Limonciello, Mario <Mario.Limonciello@xxxxxxx> >>> Sent: Tuesday, December 12, 2023 12:15 AM >>> To: amd-gfx@xxxxxxxxxxxxxxxxxxxxx; Wentland, Harry >>> <Harry.Wentland@xxxxxxx> >>> Cc: Linux Regressions <regressions@xxxxxxxxxxxxxxx>; stable@xxxxxxxxxxxxxxx; >>> Wheeler, Daniel <Daniel.Wheeler@xxxxxxx>; Lin, Wayne >>> <Wayne.Lin@xxxxxxx>; Oliver Schmidt <oliver@xxxxxxxx> >>> Subject: Re: [PATCH] Revert "drm/amd/display: Adjust the MST resume flow" >>> >>> Ping on this one. >>> >>> On 12/5/2023 13:54, Mario Limonciello wrote: >>>> This reverts commit ec5fa9fcdeca69edf7dab5ca3b2e0ceb1c08fe9a. >>>> >>>> Reports are that this causes problems with external monitors after >>>> wake up from suspend, which is something it was directly supposed to help. >>>> >>>> Cc: Linux Regressions <regressions@xxxxxxxxxxxxxxx> >>>> Cc: stable@xxxxxxxxxxxxxxx >>>> Cc: Daniel Wheeler <daniel.wheeler@xxxxxxx> >>>> Cc: Wayne Lin <wayne.lin@xxxxxxx> >>>> Reported-by: Oliver Schmidt <oliver@xxxxxxxx> >>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218211 >>>> Link: >>>> https://forum.manjaro.org/t/problems-with-external-monitor-wake-up-aft >>>> er-suspend/151840 >>>> Link: https://gitlab.freedesktop.org/drm/amd/-/issues/3023 >>>> Signed-off-by: Mario Limonciello <mario.limonciello >>>> <mario.limonciello@xxxxxxx> >>>> --- >>>> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 93 +++-------------- >>> -- >>>> 1 file changed, 13 insertions(+), 80 deletions(-) >>>> >>>> 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 c146dc9cba92..1ba58e4ecab3 100644 >>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >>>> @@ -2363,62 +2363,14 @@ static int dm_late_init(void *handle) >>>> return detect_mst_link_for_all_connectors(adev_to_drm(adev)); >>>> } >>>> >>>> -static void resume_mst_branch_status(struct drm_dp_mst_topology_mgr >>>> *mgr) -{ >>>> - int ret; >>>> - u8 guid[16]; >>>> - u64 tmp64; >>>> - >>>> - mutex_lock(&mgr->lock); >>>> - if (!mgr->mst_primary) >>>> - goto out_fail; >>>> - >>>> - if (drm_dp_read_dpcd_caps(mgr->aux, mgr->dpcd) < 0) { >>>> - drm_dbg_kms(mgr->dev, "dpcd read failed - undocked during >>> suspend?\n"); >>>> - goto out_fail; >>>> - } >>>> - >>>> - ret = drm_dp_dpcd_writeb(mgr->aux, DP_MSTM_CTRL, >>>> - DP_MST_EN | >>>> - DP_UP_REQ_EN | >>>> - DP_UPSTREAM_IS_SRC); >>>> - if (ret < 0) { >>>> - drm_dbg_kms(mgr->dev, "mst write failed - undocked during >>> suspend?\n"); >>>> - goto out_fail; >>>> - } >>>> - >>>> - /* Some hubs forget their guids after they resume */ >>>> - ret = drm_dp_dpcd_read(mgr->aux, DP_GUID, guid, 16); >>>> - if (ret != 16) { >>>> - drm_dbg_kms(mgr->dev, "dpcd read failed - undocked during >>> suspend?\n"); >>>> - goto out_fail; >>>> - } >>>> - >>>> - if (memchr_inv(guid, 0, 16) == NULL) { >>>> - tmp64 = get_jiffies_64(); >>>> - memcpy(&guid[0], &tmp64, sizeof(u64)); >>>> - memcpy(&guid[8], &tmp64, sizeof(u64)); >>>> - >>>> - ret = drm_dp_dpcd_write(mgr->aux, DP_GUID, guid, 16); >>>> - >>>> - if (ret != 16) { >>>> - drm_dbg_kms(mgr->dev, "check mstb guid failed - >>> undocked during suspend?\n"); >>>> - goto out_fail; >>>> - } >>>> - } >>>> - >>>> - memcpy(mgr->mst_primary->guid, guid, 16); >>>> - >>>> -out_fail: >>>> - mutex_unlock(&mgr->lock); >>>> -} >>>> - >>>> static void s3_handle_mst(struct drm_device *dev, bool suspend) >>>> { >>>> struct amdgpu_dm_connector *aconnector; >>>> struct drm_connector *connector; >>>> struct drm_connector_list_iter iter; >>>> struct drm_dp_mst_topology_mgr *mgr; >>>> + int ret; >>>> + bool need_hotplug = false; >>>> >>>> drm_connector_list_iter_begin(dev, &iter); >>>> drm_for_each_connector_iter(connector, &iter) { @@ -2444,15 >>>> +2396,18 @@ static void s3_handle_mst(struct drm_device *dev, bool >>> suspend) >>>> if (!dp_is_lttpr_present(aconnector->dc_link)) >>>> try_to_configure_aux_timeout(aconnector- >>>> dc_link->ddc, >>>> LINK_AUX_DEFAULT_TIMEOUT_PERIOD); >>>> >>>> - /* TODO: move resume_mst_branch_status() into >>> drm mst resume again >>>> - * once topology probing work is pulled out from mst >>> resume into mst >>>> - * resume 2nd step. mst resume 2nd step should be >>> called after old >>>> - * state getting restored (i.e. >>> drm_atomic_helper_resume()). >>>> - */ >>>> - resume_mst_branch_status(mgr); >>>> + ret = drm_dp_mst_topology_mgr_resume(mgr, true); >>>> + if (ret < 0) { >>>> + >>> dm_helpers_dp_mst_stop_top_mgr(aconnector->dc_link->ctx, >>>> + aconnector->dc_link); >>>> + need_hotplug = true; >>>> + } >>>> } >>>> } >>>> drm_connector_list_iter_end(&iter); >>>> + >>>> + if (need_hotplug) >>>> + drm_kms_helper_hotplug_event(dev); >>>> } >>>> >>>> static int amdgpu_dm_smu_write_watermarks_table(struct >>> amdgpu_device >>>> *adev) @@ -2849,8 +2804,7 @@ static int dm_resume(void *handle) >>>> 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; >>>> - int i, r, j, ret; >>>> - bool need_hotplug = false; >>>> + int i, r, j; >>>> >>>> if (dm->dc->caps.ips_support) { >>>> dc_dmub_srv_exit_low_power_state(dm->dc); >>>> @@ -2957,7 +2911,7 @@ static int dm_resume(void *handle) >>>> continue; >>>> >>>> /* >>>> - * this is the case when traversing through already created end >>> sink >>>> + * this is the case when traversing through already created >>>> * MST connectors, should be skipped >>>> */ >>>> if (aconnector && aconnector->mst_root) @@ -3017,27 >>> +2971,6 @@ >>>> static int dm_resume(void *handle) >>>> >>>> dm->cached_state = NULL; >>>> >>>> - /* Do mst topology probing after resuming cached state*/ >>>> - drm_connector_list_iter_begin(ddev, &iter); >>>> - drm_for_each_connector_iter(connector, &iter) { >>>> - aconnector = to_amdgpu_dm_connector(connector); >>>> - if (aconnector->dc_link->type != dc_connection_mst_branch >>> || >>>> - aconnector->mst_root) >>>> - continue; >>>> - >>>> - ret = drm_dp_mst_topology_mgr_resume(&aconnector- >>>> mst_mgr, true); >>>> - >>>> - if (ret < 0) { >>>> - dm_helpers_dp_mst_stop_top_mgr(aconnector- >>>> dc_link->ctx, >>>> - aconnector->dc_link); >>>> - need_hotplug = true; >>>> - } >>>> - } >>>> - drm_connector_list_iter_end(&iter); >>>> - >>>> - if (need_hotplug) >>>> - drm_kms_helper_hotplug_event(ddev); >>>> - >>>> amdgpu_dm_irq_resume_late(adev); >>>> >>>> amdgpu_dm_smu_write_watermarks_table(adev); >> >
