[Freedreno] [PATCH v4] drm/msm/dp: force link training for display resolution change

[Kuogee Hsieh]

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 at quicinc.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) {