[PATCH] Revert "drm/amd/display: Adjust the MST resume flow"

Oliver Schmidt oliver at luced.de
Wed Dec 13 00:08:18 UTC 2023


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 at amd.com>
> 
> Sure, thanks.
> 
>>
>> Thanks,
>> Wayne
>>
>>> -----Original Message-----
>>> From: Limonciello, Mario <Mario.Limonciello at amd.com>
>>> Sent: Tuesday, December 12, 2023 12:15 AM
>>> To: amd-gfx at lists.freedesktop.org; Wentland, Harry
>>> <Harry.Wentland at amd.com>
>>> Cc: Linux Regressions <regressions at lists.linux.dev>; stable at vger.kernel.org;
>>> Wheeler, Daniel <Daniel.Wheeler at amd.com>; Lin, Wayne
>>> <Wayne.Lin at amd.com>; Oliver Schmidt <oliver at luced.de>
>>> 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 at lists.linux.dev>
>>>> Cc: stable at vger.kernel.org
>>>> Cc: Daniel Wheeler <daniel.wheeler at amd.com>
>>>> Cc: Wayne Lin <wayne.lin at amd.com>
>>>> Reported-by: Oliver Schmidt <oliver at luced.de>
>>>> 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 at amd.com>
>>>> ---
>>>>    .../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);
>>
> 


More information about the amd-gfx mailing list