| Message ID | 20241202-hpd_display_off-v1-1-8d0551847753@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | drm/msm/dp: ST_DISPLAY_OFF hpd cleanup | expand |
On 12/3/2024 5:50 AM, Dmitry Baryshkov wrote: > On Mon, Dec 02, 2024 at 04:39:00PM -0800, Abhinav Kumar wrote: >> In commit 8ede2ecc3e5ee ("drm/msm/dp: Add DP compliance tests on Snapdragon Chipsets"), >> checks were introduced to avoid handling any plug or irq hpd events in >> ST_DISPLAY_OFF state. >> >> Even if we do get hpd events, after the bridge was disabled, >> it should get handled. Moreover, its unclear under what circumstances >> these events will fire because ST_DISPLAY_OFF means that the link was >> still connected but only the bridge was disabled. If the link was untouched, >> then interrupts shouldn't fire. > > What about the link being untouched, but the monitor being toggled > somehow, which might generate HPD / attention events? > To confirm my understanding of this, I tested this case again with and without a dongle on sc7180. Without a dongle - When the monitor is powered off and powered on, without physically disturbing the cable, it still generates a hotplug disconnect event when powered off and hotplug connect event when its powered on again. It gets handled the same way as a user would hotplug it using the cable. With a dongle - When the monitor is powered off , nothing happens and it stays in connected state. When the monitor is powered back on, it generates a hotplug disable followed by hotplug enable event. This behavior is consistent with or without this series with this dongle. So from the DP driver point of view, for both these cases, its just another hotlplug disconnect/connect. Both these cases still work fine with these changes. >> >> Even in the case of the DP compliance equipment, it should be raising these >> interrupts during the start of the test which is usually accompanied with either >> a HPD pulse or a IRQ HPD but after the bridge is disabled it should be fine >> to handle these anyway. In the absence of a better reason to keep these checks, >> drop these and if any other issues do arise, it should be handled in a different >> way. >> >> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com> >> --- >> drivers/gpu/drm/msm/dp/dp_display.c | 10 ---------- >> 1 file changed, 10 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c >> index aba925aab7ad..992184cc17e4 100644 >> --- a/drivers/gpu/drm/msm/dp/dp_display.c >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c >> @@ -562,11 +562,6 @@ static int msm_dp_hpd_plug_handle(struct msm_dp_display_private *dp, u32 data) >> drm_dbg_dp(dp->drm_dev, "Before, type=%d hpd_state=%d\n", >> dp->msm_dp_display.connector_type, state); >> >> - if (state == ST_DISPLAY_OFF) { >> - mutex_unlock(&dp->event_mutex); >> - return 0; >> - } >> - >> if (state == ST_MAINLINK_READY || state == ST_CONNECTED) { >> mutex_unlock(&dp->event_mutex); >> return 0; >> @@ -689,11 +684,6 @@ static int msm_dp_irq_hpd_handle(struct msm_dp_display_private *dp, u32 data) >> drm_dbg_dp(dp->drm_dev, "Before, type=%d hpd_state=%d\n", >> dp->msm_dp_display.connector_type, state); >> >> - if (state == ST_DISPLAY_OFF) { >> - mutex_unlock(&dp->event_mutex); >> - return 0; >> - } >> - >> if (state == ST_MAINLINK_READY || state == ST_DISCONNECT_PENDING) { >> /* wait until ST_CONNECTED */ >> msm_dp_add_event(dp, EV_IRQ_HPD_INT, 0, 1); /* delay = 1 */ >> >> -- >> 2.34.1 >> >
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index aba925aab7ad..992184cc17e4 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -562,11 +562,6 @@ static int msm_dp_hpd_plug_handle(struct msm_dp_display_private *dp, u32 data) drm_dbg_dp(dp->drm_dev, "Before, type=%d hpd_state=%d\n", dp->msm_dp_display.connector_type, state); - if (state == ST_DISPLAY_OFF) { - mutex_unlock(&dp->event_mutex); - return 0; - } - if (state == ST_MAINLINK_READY || state == ST_CONNECTED) { mutex_unlock(&dp->event_mutex); return 0; @@ -689,11 +684,6 @@ static int msm_dp_irq_hpd_handle(struct msm_dp_display_private *dp, u32 data) drm_dbg_dp(dp->drm_dev, "Before, type=%d hpd_state=%d\n", dp->msm_dp_display.connector_type, state); - if (state == ST_DISPLAY_OFF) { - mutex_unlock(&dp->event_mutex); - return 0; - } - if (state == ST_MAINLINK_READY || state == ST_DISCONNECT_PENDING) { /* wait until ST_CONNECTED */ msm_dp_add_event(dp, EV_IRQ_HPD_INT, 0, 1); /* delay = 1 */
In commit 8ede2ecc3e5ee ("drm/msm/dp: Add DP compliance tests on Snapdragon Chipsets"), checks were introduced to avoid handling any plug or irq hpd events in ST_DISPLAY_OFF state. Even if we do get hpd events, after the bridge was disabled, it should get handled. Moreover, its unclear under what circumstances these events will fire because ST_DISPLAY_OFF means that the link was still connected but only the bridge was disabled. If the link was untouched, then interrupts shouldn't fire. Even in the case of the DP compliance equipment, it should be raising these interrupts during the start of the test which is usually accompanied with either a HPD pulse or a IRQ HPD but after the bridge is disabled it should be fine to handle these anyway. In the absence of a better reason to keep these checks, drop these and if any other issues do arise, it should be handled in a different way. Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com> --- drivers/gpu/drm/msm/dp/dp_display.c | 10 ---------- 1 file changed, 10 deletions(-)