Re: [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@xxxxxxxxxxx>
> > 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) {