| Message ID | 20240306195031.490994-1-quic_abhinavk@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RFC] drm/msm/dp: move link_ready out of HPD event thread | expand |
On Wed, 6 Mar 2024 at 21:50, Abhinav Kumar <quic_abhinavk@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_khsieh@quicinc.com/ > > Signed-off-by: Abhinav Kumar <quic_abhinavk@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? > + > 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 >
On 3/6/2024 11:52 AM, Dmitry Baryshkov wrote: > On Wed, 6 Mar 2024 at 21:50, Abhinav Kumar <quic_abhinavk@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_khsieh@quicinc.com/ >> >> Signed-off-by: Abhinav Kumar <quic_abhinavk@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? >> + >> 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 >> > >
On Wed, 6 Mar 2024 at 21:59, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > On 3/6/2024 11:52 AM, Dmitry Baryshkov wrote: > > On Wed, 6 Mar 2024 at 21:50, Abhinav Kumar <quic_abhinavk@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_khsieh@quicinc.com/ > >> > >> Signed-off-by: Abhinav Kumar <quic_abhinavk@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 > >> > > > >
On 3/6/2024 11:50 AM, Abhinav Kumar 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_khsieh@quicinc.com/ > > Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com> Tested-by: Kuogee Hsieh <quic_khsieh@quicinc.com> Reviewed-by: Kuogee Hsieh <quic_khsieh@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; > + > 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 {
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; + 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 {
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_khsieh@quicinc.com/ Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com> --- drivers/gpu/drm/msm/dp/dp_display.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)