1) check sink_count before update is_connected status 2) initialize audio_comp when audio starts 3) check main link status before start aux read 4) dp_link_parse_sink_count() return immediately if aux read failed
Kuogee Hsieh (4): drm/msm/dp: check sink_count before update is_connected status drm/msm/dp: initialize audio_comp when audio starts drm/msm/dp: check main link status before start aux read drm/msm/dp: dp_link_parse_sink_count() return immediately if aux read failed
drivers/gpu/drm/msm/dp/dp_audio.c | 1 + drivers/gpu/drm/msm/dp/dp_aux.c | 5 +++++ drivers/gpu/drm/msm/dp/dp_display.c | 38 +++++++++++++++++++++++++------------ drivers/gpu/drm/msm/dp/dp_display.h | 1 + drivers/gpu/drm/msm/dp/dp_link.c | 20 ++++++++++++++----- 5 files changed, 48 insertions(+), 17 deletions(-)
Link status is different from display connected status in the case of something like an Apple dongle where the type-c plug can be connected, and therefore the link is connected, but no sink is connected until an HDMI cable is plugged into the dongle. The sink_count of DPCD of dongle will increase to 1 once an HDMI cable is plugged into the dongle so that display connected status will become true. This checking also apply at pm_resume.
Changes in v4: -- none
Fixes: 94e58e2d06e3 ("drm/msm/dp: reset dp controller only at boot up and pm_resume") Reported-by: Stephen Boyd swboyd@chromium.org Reviewed-by: Stephen Boyd swboyd@chromium.org Tested-by: Stephen Boyd swboyd@chromium.org Signed-off-by: Kuogee Hsieh khsieh@codeaurora.org --- drivers/gpu/drm/msm/dp/dp_display.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index 5a39da6..0ba71c7 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -586,10 +586,8 @@ static int dp_connect_pending_timeout(struct dp_display_private *dp, u32 data) mutex_lock(&dp->event_mutex);
state = dp->hpd_state; - if (state == ST_CONNECT_PENDING) { - dp_display_enable(dp, 0); + if (state == ST_CONNECT_PENDING) dp->hpd_state = ST_CONNECTED; - }
mutex_unlock(&dp->event_mutex);
@@ -669,10 +667,8 @@ static int dp_disconnect_pending_timeout(struct dp_display_private *dp, u32 data mutex_lock(&dp->event_mutex);
state = dp->hpd_state; - if (state == ST_DISCONNECT_PENDING) { - dp_display_disable(dp, 0); + if (state == ST_DISCONNECT_PENDING) dp->hpd_state = ST_DISCONNECTED; - }
mutex_unlock(&dp->event_mutex);
@@ -1272,7 +1268,12 @@ static int dp_pm_resume(struct device *dev)
status = dp_catalog_link_is_connected(dp->catalog);
- if (status) + /* + * can not declared display is connected unless + * HDMI cable is plugged in and sink_count of + * dongle become 1 + */ + if (status && dp->link->sink_count) dp->dp_display.is_connected = true; else dp->dp_display.is_connected = false;
Quoting Kuogee Hsieh (2021-04-21 16:37:35)
Link status is different from display connected status in the case of something like an Apple dongle where the type-c plug can be connected, and therefore the link is connected, but no sink is connected until an HDMI cable is plugged into the dongle. The sink_count of DPCD of dongle will increase to 1 once an HDMI cable is plugged into the dongle so that display connected status will become true. This checking also apply at pm_resume.
Changes in v4: -- none
Fixes: 94e58e2d06e3 ("drm/msm/dp: reset dp controller only at boot up and pm_resume") Reported-by: Stephen Boyd swboyd@chromium.org Reviewed-by: Stephen Boyd swboyd@chromium.org Tested-by: Stephen Boyd swboyd@chromium.org Signed-off-by: Kuogee Hsieh khsieh@codeaurora.org
drivers/gpu/drm/msm/dp/dp_display.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index 5a39da6..0ba71c7 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -586,10 +586,8 @@ static int dp_connect_pending_timeout(struct dp_display_private *dp, u32 data) mutex_lock(&dp->event_mutex);
state = dp->hpd_state;
if (state == ST_CONNECT_PENDING) {
dp_display_enable(dp, 0);
if (state == ST_CONNECT_PENDING) dp->hpd_state = ST_CONNECTED;
}
This part has been there since commit 8ede2ecc3e5e ("drm/msm/dp: Add DP compliance tests on Snapdragon Chipsets") so we should add that tag here too, like
Fixes: 8ede2ecc3e5e ("drm/msm/dp: Add DP compliance tests on Snapdragon Chipsets")
It would also be nice if this hunk was explained. It doesn't make sense to me that a timeout would enable the display so maybe that can be called out in the commit text about why we remove the call here.
Initialize audio_comp when audio starts and wait for audio_comp at dp_display_disable(). This will take care of both dongle unplugged and display off (suspend) cases.
Changes in v2: -- add dp_display_signal_audio_start()
Changes in v3: -- restore dp_display_handle_plugged_change() at dp_hpd_unplug_handle().
Changes in v4: -- none
Signed-off-by: Kuogee Hsieh khsieh@codeaurora.org --- drivers/gpu/drm/msm/dp/dp_audio.c | 1 + drivers/gpu/drm/msm/dp/dp_display.c | 11 +++++++++-- drivers/gpu/drm/msm/dp/dp_display.h | 1 + 3 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_audio.c b/drivers/gpu/drm/msm/dp/dp_audio.c index 82a8673..d7e4a39 100644 --- a/drivers/gpu/drm/msm/dp/dp_audio.c +++ b/drivers/gpu/drm/msm/dp/dp_audio.c @@ -527,6 +527,7 @@ int dp_audio_hw_params(struct device *dev, dp_audio_setup_acr(audio); dp_audio_safe_to_exit_level(audio); dp_audio_enable(audio, true); + dp_display_signal_audio_start(dp_display); dp_display->audio_enabled = true;
end: diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index 0ba71c7..1784e11 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -178,6 +178,15 @@ static int dp_del_event(struct dp_display_private *dp_priv, u32 event) return 0; }
+void dp_display_signal_audio_start(struct msm_dp *dp_display) +{ + struct dp_display_private *dp; + + dp = container_of(dp_display, struct dp_display_private, dp_display); + + reinit_completion(&dp->audio_comp); +} + void dp_display_signal_audio_complete(struct msm_dp *dp_display) { struct dp_display_private *dp; @@ -649,7 +658,6 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data) dp_add_event(dp, EV_DISCONNECT_PENDING_TIMEOUT, 0, DP_TIMEOUT_5_SECOND);
/* signal the disconnect event early to ensure proper teardown */ - reinit_completion(&dp->audio_comp); dp_display_handle_plugged_change(g_dp_display, false);
dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK | @@ -894,7 +902,6 @@ static int dp_display_disable(struct dp_display_private *dp, u32 data) /* wait only if audio was enabled */ if (dp_display->audio_enabled) { /* signal the disconnect event */ - reinit_completion(&dp->audio_comp); dp_display_handle_plugged_change(dp_display, false); if (!wait_for_completion_timeout(&dp->audio_comp, HZ * 5)) diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h index 6092ba1..5173c89 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.h +++ b/drivers/gpu/drm/msm/dp/dp_display.h @@ -34,6 +34,7 @@ int dp_display_get_modes(struct msm_dp *dp_display, int dp_display_request_irq(struct msm_dp *dp_display); bool dp_display_check_video_test(struct msm_dp *dp_display); int dp_display_get_test_bpp(struct msm_dp *dp_display); +void dp_display_signal_audio_start(struct msm_dp *dp_display); void dp_display_signal_audio_complete(struct msm_dp *dp_display);
#endif /* _DP_DISPLAY_H_ */
Quoting Kuogee Hsieh (2021-04-21 16:37:36)
Initialize audio_comp when audio starts and wait for audio_comp at dp_display_disable(). This will take care of both dongle unplugged and display off (suspend) cases.
Changes in v2: -- add dp_display_signal_audio_start()
Changes in v3: -- restore dp_display_handle_plugged_change() at dp_hpd_unplug_handle().
Changes in v4: -- none
Signed-off-by: Kuogee Hsieh khsieh@codeaurora.org
Reviewed-by: Stephen Boyd swboyd@chromium.org Tested-by: Stephen Boyd swboyd@chromium.org Fixes: c703d5789590 ("drm/msm/dp: trigger unplug event in msm_dp_display_disable")
Maybe when the cable is disconnected the DP phy should be shutdown and some bit in the phy could effectively "cut off" the aux channel and then NAKs would start coming through here in the DP controller I/O register space. This patch have DP aux channel read/write to return NAK immediately if DP controller connection status is in unplugged state.
Changes in V4: -- split this patch as stand alone patch
Signed-off-by: Kuogee Hsieh khsieh@codeaurora.org --- drivers/gpu/drm/msm/dp/dp_aux.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c index 7c22bfe..fae3806 100644 --- a/drivers/gpu/drm/msm/dp/dp_aux.c +++ b/drivers/gpu/drm/msm/dp/dp_aux.c @@ -343,6 +343,11 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
mutex_lock(&aux->mutex);
+ if (!dp_catalog_link_is_connected(aux->catalog)) { + ret = -ETIMEDOUT; + goto unlock_exit; + } + aux->native = msg->request & (DP_AUX_NATIVE_WRITE & DP_AUX_NATIVE_READ);
/* Ignore address only message */
Quoting Kuogee Hsieh (2021-04-21 16:37:37)
Maybe when the cable is disconnected the DP phy should be shutdown and some bit in the phy could effectively "cut off" the aux channel and then NAKs would start coming through here in the DP controller I/O register space. This patch have DP aux channel read/write to return NAK immediately if DP controller connection status is in unplugged state.
Changes in V4: -- split this patch as stand alone patch
Signed-off-by: Kuogee Hsieh khsieh@codeaurora.org
drivers/gpu/drm/msm/dp/dp_aux.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c index 7c22bfe..fae3806 100644 --- a/drivers/gpu/drm/msm/dp/dp_aux.c +++ b/drivers/gpu/drm/msm/dp/dp_aux.c @@ -343,6 +343,11 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
mutex_lock(&aux->mutex);
if (!dp_catalog_link_is_connected(aux->catalog)) {
ret = -ETIMEDOUT;
goto unlock_exit;
}
aux->native = msg->request & (DP_AUX_NATIVE_WRITE & DP_AUX_NATIVE_READ); /* Ignore address only message */
Can the code check for aux timeouts? So instead of blindly completing 'aux->comp' we would do the transfer, and then dp_aux_cmd_fifo_tx() would check to see if the completion was completed from the irq handler because of a timeout or a nack, etc. I think the code is probably racy, given that dp_aux_isr() is called from irq context, and aux_error_num is set from the irq context and tested in non-irq context. This code needs a spinlock and then to check the isr bits to figure out if it should tell the upper layers that the address was wrong, or there was a nack or a timeout, etc.
I don't think we need to check the link to see if it is connected, just look at the irq bits to see if the response was bad and letting higher layers know that should quickly cut off the transactions.
Add checking aux read/write status at both dp_link_parse_sink_count() and dp_link_parse_sink_status_filed() to avoid long timeout delay if dp aux read/write failed at timeout due to cable unplugged.
Changes in V4: -- split this patch as stand alone patch
Signed-off-by: Kuogee Hsieh khsieh@codeaurora.org --- drivers/gpu/drm/msm/dp/dp_display.c | 12 +++++++++--- drivers/gpu/drm/msm/dp/dp_link.c | 20 +++++++++++++++----- 2 files changed, 24 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index 1784e11..d1319b5 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -711,9 +711,15 @@ static int dp_irq_hpd_handle(struct dp_display_private *dp, u32 data) return 0; }
- ret = dp_display_usbpd_attention_cb(&dp->pdev->dev); - if (ret == -ECONNRESET) { /* cable unplugged */ - dp->core_initialized = false; + /* + * dp core (ahb/aux clks) must be initialized before + * irq_hpd be handled + */ + if (dp->core_initialized) { + ret = dp_display_usbpd_attention_cb(&dp->pdev->dev); + if (ret == -ECONNRESET) { /* cable unplugged */ + dp->core_initialized = false; + } }
mutex_unlock(&dp->event_mutex); diff --git a/drivers/gpu/drm/msm/dp/dp_link.c b/drivers/gpu/drm/msm/dp/dp_link.c index be986da..53ecae6 100644 --- a/drivers/gpu/drm/msm/dp/dp_link.c +++ b/drivers/gpu/drm/msm/dp/dp_link.c @@ -737,18 +737,25 @@ static int dp_link_parse_sink_count(struct dp_link *dp_link) return 0; }
-static void dp_link_parse_sink_status_field(struct dp_link_private *link) +static int dp_link_parse_sink_status_field(struct dp_link_private *link) { int len = 0;
link->prev_sink_count = link->dp_link.sink_count; - dp_link_parse_sink_count(&link->dp_link); + len = dp_link_parse_sink_count(&link->dp_link); + if (len < 0) { + DRM_ERROR("DP parse sink count failed\n"); + return len; + }
len = drm_dp_dpcd_read_link_status(link->aux, link->link_status); - if (len < DP_LINK_STATUS_SIZE) + if (len < DP_LINK_STATUS_SIZE) { DRM_ERROR("DP link status read failed\n"); - dp_link_parse_request(link); + return len; + } + + return dp_link_parse_request(link); }
/** @@ -1032,7 +1039,10 @@ int dp_link_process_request(struct dp_link *dp_link)
dp_link_reset_data(link);
- dp_link_parse_sink_status_field(link); + ret = dp_link_parse_sink_status_field(link); + if (ret) { + return ret; + }
if (link->request.test_requested == DP_TEST_LINK_EDID_READ) { dp_link->sink_request |= DP_TEST_LINK_EDID_READ;
Quoting Kuogee Hsieh (2021-04-21 16:37:38)
Add checking aux read/write status at both dp_link_parse_sink_count() and dp_link_parse_sink_status_filed() to avoid long timeout delay if
s/filed/field/
dp aux read/write failed at timeout due to cable unplugged.
Changes in V4: -- split this patch as stand alone patch
Signed-off-by: Kuogee Hsieh khsieh@codeaurora.org
Can this patch come before the one previously? And then some fixes tag be added? Otherwise looks good to me.
Reviewed-by: Stephen Boyd swboyd@chromium.org Tested-by: Stephen Boyd swboyd@chromium.org
On 04/05/2021 07:35, Stephen Boyd wrote:
Quoting Kuogee Hsieh (2021-04-21 16:37:38)
Add checking aux read/write status at both dp_link_parse_sink_count() and dp_link_parse_sink_status_filed() to avoid long timeout delay if
s/filed/field/
dp aux read/write failed at timeout due to cable unplugged.
Changes in V4: -- split this patch as stand alone patch
Signed-off-by: Kuogee Hsieh khsieh@codeaurora.org
Can this patch come before the one previously? And then some fixes tag be added? Otherwise looks good to me.
Reviewed-by: Stephen Boyd swboyd@chromium.org Tested-by: Stephen Boyd swboyd@chromium.org
Is this something that we still need to pursue/merge?
There were changes requested for this and for the previous patch, but no new versions were posted.
On 2021-11-24 23:32, Dmitry Baryshkov wrote:
On 04/05/2021 07:35, Stephen Boyd wrote:
Quoting Kuogee Hsieh (2021-04-21 16:37:38)
Add checking aux read/write status at both dp_link_parse_sink_count() and dp_link_parse_sink_status_filed() to avoid long timeout delay if
s/filed/field/
dp aux read/write failed at timeout due to cable unplugged.
Changes in V4: -- split this patch as stand alone patch
Signed-off-by: Kuogee Hsieh khsieh@codeaurora.org
Can this patch come before the one previously? And then some fixes tag be added? Otherwise looks good to me.
Reviewed-by: Stephen Boyd swboyd@chromium.org Tested-by: Stephen Boyd swboyd@chromium.org
Is this something that we still need to pursue/merge?
There were changes requested for this and for the previous patch, but no new versions were posted.
It is my fault to miss this one. The first two patches of this serial are merged. I will rebase and re submit this one to V5.10.
On Wed, Apr 21, 2021 at 4:38 PM Kuogee Hsieh khsieh@codeaurora.org wrote:
- check sink_count before update is_connected status
- initialize audio_comp when audio starts
- check main link status before start aux read
- dp_link_parse_sink_count() return immediately if aux read failed
Kuogee Hsieh (4): drm/msm/dp: check sink_count before update is_connected status drm/msm/dp: initialize audio_comp when audio starts drm/msm/dp: check main link status before start aux read drm/msm/dp: dp_link_parse_sink_count() return immediately if aux read failed
I've picked up these two in msm-next for an upcoming -fixes pull req:
drm/msm/dp: initialize audio_comp when audio starts drm/msm/dp: check sink_count before update is_connected status
BR, -R
drivers/gpu/drm/msm/dp/dp_audio.c | 1 + drivers/gpu/drm/msm/dp/dp_aux.c | 5 +++++ drivers/gpu/drm/msm/dp/dp_display.c | 38 +++++++++++++++++++++++++------------ drivers/gpu/drm/msm/dp/dp_display.h | 1 + drivers/gpu/drm/msm/dp/dp_link.c | 20 ++++++++++++++----- 5 files changed, 48 insertions(+), 17 deletions(-)
-- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
dri-devel@lists.freedesktop.org