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(-)
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
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 <[email protected]>
Reviewed-by: Stephen Boyd <[email protected]>
Tested-by: Stephen Boyd <[email protected]>
Signed-off-by: Kuogee Hsieh <[email protected]>
---
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;
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
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 <[email protected]>
---
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 */
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
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 <[email protected]>
---
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_ */
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
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 <[email protected]>
> Reviewed-by: Stephen Boyd <[email protected]>
> Tested-by: Stephen Boyd <[email protected]>
> Signed-off-by: Kuogee Hsieh <[email protected]>
> ---
> 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.
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 <[email protected]>
> ---
Reviewed-by: Stephen Boyd <[email protected]>
Tested-by: Stephen Boyd <[email protected]>
Fixes: c703d5789590 ("drm/msm/dp: trigger unplug event in
msm_dp_display_disable")
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 <[email protected]>
> ---
> 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.
On Wed, Apr 21, 2021 at 4:38 PM Kuogee Hsieh <[email protected]> wrote:
>
> 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
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
>