On Wed, 6 Mar 2024 at 21:59, Abhinav Kumar <quic_abhin...@quicinc.com> wrote:
>
>
>
> On 3/6/2024 11:52 AM, Dmitry Baryshkov wrote:
> > On Wed, 6 Mar 2024 at 21:50, Abhinav Kumar <quic_abhin...@quicinc.com>
> > wrote:
> >>
> >> There are cases where the userspace might still send another
> >> frame after the HPD disconnect causing a modeset cycle after
> >> a disconnect. This messes the internal state machine of MSM DP driver
> >> and can lead to a crash as there can be an imbalance between
> >> bridge_disable() and bridge_enable().
> >>
> >> This was also previously reported on [1] for which [2] was posted
> >> and helped resolve the issue by rejecting commits if the DP is not
> >> in connected state.
> >>
> >> The change resolved the bug but there can also be another race condition.
> >> If hpd_event_thread does not pick up the EV_USER_NOTIFICATION and process
> >> it
> >> link_ready will also not be set to false allowing the frame to sneak in.
> >>
> >> Lets move setting link_ready outside of hpd_event_thread() processing to
> >> eliminate a window of race condition.
> >>
> >> [1] : https://gitlab.freedesktop.org/drm/msm/-/issues/17
> >> [2] :
> >> https://lore.kernel.org/all/1664408211-25314-1-git-send-email-quic_khs...@quicinc.com/
> >>
> >> Signed-off-by: Abhinav Kumar <quic_abhin...@quicinc.com>
> >> ---
> >> drivers/gpu/drm/msm/dp/dp_display.c | 7 +++++--
> >> 1 file changed, 5 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> >> b/drivers/gpu/drm/msm/dp/dp_display.c
> >> index 068d44eeaa07..e00092904ccc 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> >> @@ -345,8 +345,6 @@ static int dp_display_send_hpd_notification(struct
> >> dp_display_private *dp,
> >>
> >> dp->panel->downstream_ports);
> >> }
> >>
> >> - dp->dp_display.link_ready = hpd;
> >> -
> >> drm_dbg_dp(dp->drm_dev, "type=%d hpd=%d\n",
> >> dp->dp_display.connector_type, hpd);
> >> drm_bridge_hpd_notify(bridge, dp->dp_display.link_ready);
> >> @@ -399,6 +397,8 @@ static int dp_display_process_hpd_high(struct
> >> dp_display_private *dp)
> >> goto end;
> >> }
> >>
> >> + dp->dp_display.link_ready = true;
> >
> > Do we need any kind of locking now?
> >
>
> hmm ... correct me if I have missed any flows but I think all paths
> where we will set link_ready are already protected by event_mutex?
I didn't check the source code, that's why I was asking. If it's
protected by event_mutex, then it should be fine.
>
> >> +
> >> dp_add_event(dp, EV_USER_NOTIFICATION, true, 0);
> >>
> >> end:
> >> @@ -466,6 +466,8 @@ static int dp_display_notify_disconnect(struct device
> >> *dev)
> >> {
> >> struct dp_display_private *dp = dev_get_dp_display_private(dev);
> >>
> >> + dp->dp_display.link_ready = false;
> >> +
> >> dp_add_event(dp, EV_USER_NOTIFICATION, false, 0);
> >>
> >> return 0;
> >> @@ -487,6 +489,7 @@ static int
> >> dp_display_handle_port_status_changed(struct dp_display_private *dp)
> >> drm_dbg_dp(dp->drm_dev, "sink count is zero, nothing to
> >> do\n");
> >> if (dp->hpd_state != ST_DISCONNECTED) {
> >> dp->hpd_state = ST_DISCONNECT_PENDING;
> >> + dp->dp_display.link_ready = false;
> >> dp_add_event(dp, EV_USER_NOTIFICATION, false, 0);
> >> }
> >> } else {
> >> --
> >> 2.34.1
> >>
> >
> >
--
With best wishes
Dmitry
