linux-kernel - Re: [PATCH v4] drm/msm/dp: force link training for display resolution change

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 13 Jun 2022 08:42:48 -0700
From:   Kuogee Hsieh <quic_khsieh@...cinc.com>
To:     Stephen Boyd <swboyd@...omium.org>, <agross@...nel.org>,
        <airlied@...ux.ie>, <bjorn.andersson@...aro.org>,
        <daniel@...ll.ch>, <dianders@...omium.org>,
        <dmitry.baryshkov@...aro.org>, <robdclark@...il.com>,
        <sean@...rly.run>, <vkoul@...nel.org>
CC:     <quic_abhinavk@...cinc.com>, <quic_aravindh@...cinc.com>,
        <quic_sbillaka@...cinc.com>, <freedreno@...ts.freedesktop.org>,
        <dri-devel@...ts.freedesktop.org>, <linux-arm-msm@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4] drm/msm/dp: force link training for display resolution
 change


On 6/10/2022 2:04 PM, Stephen Boyd wrote:
> Quoting Kuogee Hsieh (2022-05-27 14:32:13)
>> During display resolution changes display have to be disabled first
>> followed by display enabling with new resolution. Display disable
>> will turn off both pixel clock and main link clock so that main link
>> have to be re trained during display enable to have new video stream
> s/re/re-/
>
>> flow again. At current implementation, display enable function manually
>> kicks up irq_hpd_handle which will read panel link status and start link
>> training if link status is not in sync state. However, there is rare
>> case that a particular panel links status keep staying in sync for
>> some period of time after main link had been shut down previously at
>> display disabled. Main link retraining will not be executed by
>> irq_hdp_handle() if the link status read from pane shows it is in
> s/pane/panel/
>
>> sync state. If this was happen, then video stream of newer display
>> resolution will fail to be transmitted to panel due to main link is
>> not in sync between host and panel. This patch force main link always
>> be retrained during display enable procedure to prevent this rare
>> failed case from happening. Also this implementation are more
>> efficient than manual kicking off irq_hpd_handle function.
> The description makes it sound like the link status is not updated,
> sometimes. Isn't that the real issue here? Not that link training needs
> to be done again (which it always does apparently), but that disabling
> the display doesn't wait for the link to go down. Or disabling the link
> is causing some sort of glitch on the sink causing it to report the
> status as OK when it really isn't.

As soon as mainlink teared down, sink has to reflect the mainlink at out 
of synch state in real time.

link re-training always required for display resolution changes since 
resolution changes will involve link configuration changes, such as 2 
lanes change to 1 changes or 5.7G link rate changes to 2.7G or vise 
versa. Therefore there is no need to check sinker's main link status to 
decide re training is required or not.



>> Changes in v2:
>> -- set force_link_train flag on DP only (is_edp == false)
>>
>> Changes in v3:
>> -- revise commit  text
>> -- add Fixes tag
>>
>> Changes in v4:
>> -- revise commit  text
>>
>> Fixes: 62671d2ef24b ("drm/msm/dp: fixes wrong connection state caused by failure of link train")
>> Signed-off-by: Kuogee Hsieh <quic_khsieh@...cinc.com>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
>> index c388323..370348d 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -1688,10 +1689,14 @@ void dp_bridge_enable(struct drm_bridge *drm_bridge)
>>
>>          state =  dp_display->hpd_state;
>>
>> -       if (state == ST_DISPLAY_OFF)
>> +       if (state == ST_DISPLAY_OFF) {
>>                  dp_display_host_phy_init(dp_display);
>>
>> -       dp_display_enable(dp_display, 0);
>> +               if (!dp->is_edp)
> Does this assume eDP has one resolution? I don't understand why eDP is
> special here, especially if eDP has more than one resolution available
> it seems like we would want to retrain the link regardless.
>
>> +                       force_link_train = true;
>> +       }
>> +
>> +       dp_display_enable(dp_display, force_link_train);
>>
>>          rc = dp_display_post_enable(dp);
>>          if (rc) {