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

Mario Limonciello mario.limonciello at amd.com
Wed Dec 13 14:39:00 UTC 2023


On 12/13/2023 08:17, Alex Deucher wrote:
> On Tue, Dec 12, 2023 at 9:00 PM Mario Limonciello
> <mario.limonciello at amd.com> wrote:
>>
>> On 12/12/2023 18:08, Oliver Schmidt wrote:
>>> 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
>>>
>>
>> By chance do you have access to any other dock or monitor combinations
>> that you can conclude it only happens on this dock or only a certain
>> monitor, or only a certain monitor connected to this dock?
> 
> IIRC, Wayne's patch was to fix an HP dock, I suspect reverting this
> will just break one dock to fix another.  Wayne, do you have the other
> problematic dock that this patch was needed to fix or even the
> thinkpad dock?  Would be nice to properly sort this out if possible.
> 

Oliver responded back that a firmware update for the problematic dock 
fixed it.

So in that case I'll revert it in amd-staging-drm-next.

> Alex
> 
>>
>>> 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