* [PATCH 2/2] drm/msm/dp: do not re initialize of audio_comp
@ 2021-04-12 17:03 Kuogee Hsieh
2021-04-13 3:44 ` Stephen Boyd
0 siblings, 1 reply; 2+ messages in thread
From: Kuogee Hsieh @ 2021-04-12 17:03 UTC (permalink / raw)
To: robdclark, sean, swboyd
Cc: tanmay, abhinavk, aravindh, khsieh, airlied, daniel,
linux-arm-msm, dri-devel, freedreno, linux-kernel
At dp_display_disable(), do not re initialize audio_comp if
hdp_state == ST_DISCONNECT_PENDING (unplug event) to avoid
race condition which cause 5 second timeout expired. Also
add abort mechanism to reduce time spinning at dp_aux_transfer()
during dpcd read if type-c connection had been broken.
Signed-off-by: Kuogee Hsieh <khsieh@codeaurora.org>
---
drivers/gpu/drm/msm/dp/dp_aux.c | 18 ++++++++++++++++++
drivers/gpu/drm/msm/dp/dp_aux.h | 1 +
drivers/gpu/drm/msm/dp/dp_display.c | 16 ++++++++++++----
drivers/gpu/drm/msm/dp/dp_link.c | 20 +++++++++++++++-----
4 files changed, 46 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
index 7c22bfe..e5ece8c 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -28,6 +28,7 @@ struct dp_aux_private {
u32 offset;
u32 segment;
u32 isr;
+ atomic_t aborted;
struct drm_dp_aux dp_aux;
};
@@ -343,6 +344,11 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
mutex_lock(&aux->mutex);
+ if (atomic_read(&aux->aborted)) {
+ ret = -ETIMEDOUT;
+ goto unlock_exit;
+ }
+
aux->native = msg->request & (DP_AUX_NATIVE_WRITE & DP_AUX_NATIVE_READ);
/* Ignore address only message */
@@ -533,3 +539,15 @@ void dp_aux_put(struct drm_dp_aux *dp_aux)
devm_kfree(aux->dev, aux);
}
+
+void dp_aux_abort(struct drm_dp_aux *dp_aux, bool abort)
+{
+ struct dp_aux_private *aux;
+
+ if (!dp_aux)
+ return;
+
+ aux = container_of(dp_aux, struct dp_aux_private, dp_aux);
+
+ atomic_set(&aux->aborted, abort);
+}
diff --git a/drivers/gpu/drm/msm/dp/dp_aux.h b/drivers/gpu/drm/msm/dp/dp_aux.h
index f8b8ba9..c17df7f 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.h
+++ b/drivers/gpu/drm/msm/dp/dp_aux.h
@@ -23,6 +23,7 @@ void dp_aux_isr(struct drm_dp_aux *dp_aux);
void dp_aux_init(struct drm_dp_aux *dp_aux);
void dp_aux_deinit(struct drm_dp_aux *dp_aux);
void dp_aux_reconfig(struct drm_dp_aux *dp_aux);
+void dp_aux_abort(struct drm_dp_aux *dp_aux, bool abort);
struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog);
void dp_aux_put(struct drm_dp_aux *aux);
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 4992a049..8960333 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -898,8 +898,10 @@ 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 (dp->hpd_state != ST_DISCONNECT_PENDING) {
+ reinit_completion(&dp->audio_comp);
+ dp_display_handle_plugged_change(dp_display, false);
+ }
if (!wait_for_completion_timeout(&dp->audio_comp,
HZ * 5))
DRM_ERROR("audio comp timeout\n");
@@ -1137,20 +1139,26 @@ static irqreturn_t dp_display_irq_handler(int irq, void *dev_id)
/* hpd related interrupts */
if (hpd_isr_status & DP_DP_HPD_PLUG_INT_MASK ||
hpd_isr_status & DP_DP_HPD_REPLUG_INT_MASK) {
+ dp_aux_abort(dp->aux, false);
dp_add_event(dp, EV_HPD_PLUG_INT, 0, 0);
}
if (hpd_isr_status & DP_DP_IRQ_HPD_INT_MASK) {
/* stop sentinel connect pending checking */
+ dp_aux_abort(dp->aux, false);
dp_del_event(dp, EV_CONNECT_PENDING_TIMEOUT);
dp_add_event(dp, EV_IRQ_HPD_INT, 0, 0);
}
- if (hpd_isr_status & DP_DP_HPD_REPLUG_INT_MASK)
+ if (hpd_isr_status & DP_DP_HPD_REPLUG_INT_MASK) {
+ dp_aux_abort(dp->aux, false);
dp_add_event(dp, EV_HPD_REPLUG_INT, 0, 0);
+ }
- if (hpd_isr_status & DP_DP_HPD_UNPLUG_INT_MASK)
+ if (hpd_isr_status & DP_DP_HPD_UNPLUG_INT_MASK) {
+ dp_aux_abort(dp->aux, true);
dp_add_event(dp, EV_HPD_UNPLUG_INT, 0, 0);
+ }
}
/* DP controller isr */
diff --git a/drivers/gpu/drm/msm/dp/dp_link.c b/drivers/gpu/drm/msm/dp/dp_link.c
index be986da..d35b18e 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 lparse 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;
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH 2/2] drm/msm/dp: do not re initialize of audio_comp
2021-04-12 17:03 [PATCH 2/2] drm/msm/dp: do not re initialize of audio_comp Kuogee Hsieh
@ 2021-04-13 3:44 ` Stephen Boyd
0 siblings, 0 replies; 2+ messages in thread
From: Stephen Boyd @ 2021-04-13 3:44 UTC (permalink / raw)
To: Kuogee Hsieh, robdclark, sean
Cc: tanmay, abhinavk, aravindh, khsieh, airlied, daniel,
linux-arm-msm, dri-devel, freedreno, linux-kernel
Quoting Kuogee Hsieh (2021-04-12 10:03:23)
> At dp_display_disable(), do not re initialize audio_comp if
> hdp_state == ST_DISCONNECT_PENDING (unplug event) to avoid
> race condition which cause 5 second timeout expired.
More details please.
> Also
> add abort mechanism to reduce time spinning at dp_aux_transfer()
> during dpcd read if type-c connection had been broken.
Please split this to a different patch.
>
> Signed-off-by: Kuogee Hsieh <khsieh@codeaurora.org>
> ---
> drivers/gpu/drm/msm/dp/dp_aux.c | 18 ++++++++++++++++++
> drivers/gpu/drm/msm/dp/dp_aux.h | 1 +
> drivers/gpu/drm/msm/dp/dp_display.c | 16 ++++++++++++----
> drivers/gpu/drm/msm/dp/dp_link.c | 20 +++++++++++++++-----
> 4 files changed, 46 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
> index 7c22bfe..e5ece8c 100644
> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> @@ -28,6 +28,7 @@ struct dp_aux_private {
> u32 offset;
> u32 segment;
> u32 isr;
> + atomic_t aborted;
Why is it an atomic?
>
> struct drm_dp_aux dp_aux;
> };
> @@ -343,6 +344,11 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
>
> mutex_lock(&aux->mutex);
>
> + if (atomic_read(&aux->aborted)) {
> + ret = -ETIMEDOUT;
> + goto unlock_exit;
> + }
> +
Cool, it's checked inside a mutex.
> aux->native = msg->request & (DP_AUX_NATIVE_WRITE & DP_AUX_NATIVE_READ);
>
> /* Ignore address only message */
> @@ -533,3 +539,15 @@ void dp_aux_put(struct drm_dp_aux *dp_aux)
>
> devm_kfree(aux->dev, aux);
> }
> +
> +void dp_aux_abort(struct drm_dp_aux *dp_aux, bool abort)
> +{
> + struct dp_aux_private *aux;
> +
> + if (!dp_aux)
> + return;
> +
> + aux = container_of(dp_aux, struct dp_aux_private, dp_aux);
> +
> + atomic_set(&aux->aborted, abort);
> +}
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 4992a049..8960333 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -898,8 +898,10 @@ 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 (dp->hpd_state != ST_DISCONNECT_PENDING) {
> + reinit_completion(&dp->audio_comp);
> + dp_display_handle_plugged_change(dp_display, false);
> + }
> if (!wait_for_completion_timeout(&dp->audio_comp,
> HZ * 5))
> DRM_ERROR("audio comp timeout\n");
This hunk is the first part of the patch and should be split away to one
for itself, with appropriate Fixes tag and a proper explanation.
> @@ -1137,20 +1139,26 @@ static irqreturn_t dp_display_irq_handler(int irq, void *dev_id)
> /* hpd related interrupts */
> if (hpd_isr_status & DP_DP_HPD_PLUG_INT_MASK ||
> hpd_isr_status & DP_DP_HPD_REPLUG_INT_MASK) {
> + dp_aux_abort(dp->aux, false);
> dp_add_event(dp, EV_HPD_PLUG_INT, 0, 0);
> }
>
> if (hpd_isr_status & DP_DP_IRQ_HPD_INT_MASK) {
> /* stop sentinel connect pending checking */
> + dp_aux_abort(dp->aux, false);
> dp_del_event(dp, EV_CONNECT_PENDING_TIMEOUT);
> dp_add_event(dp, EV_IRQ_HPD_INT, 0, 0);
> }
>
> - if (hpd_isr_status & DP_DP_HPD_REPLUG_INT_MASK)
> + if (hpd_isr_status & DP_DP_HPD_REPLUG_INT_MASK) {
> + dp_aux_abort(dp->aux, false);
> dp_add_event(dp, EV_HPD_REPLUG_INT, 0, 0);
> + }
>
> - if (hpd_isr_status & DP_DP_HPD_UNPLUG_INT_MASK)
> + if (hpd_isr_status & DP_DP_HPD_UNPLUG_INT_MASK) {
> + dp_aux_abort(dp->aux, true);
Ok, so it seems that we want to stop trying aux transfers if the unplug
irq comes in? That's a pretty big sledge hammer to stop a transfer in
the middle of progress. Why doesn't the hardware timeout and stop or the
dpcd reads in this DP driver fail and start bailing out when the cable
is disconnected? Having to inject that synthetically is not great. Is
there some sort of AUX channel "status" bit that can be read from the
aux registers in the DP hardware to see if the connection was lost?
> dp_add_event(dp, EV_HPD_UNPLUG_INT, 0, 0);
> + }
> }
>
> /* DP controller isr */
> diff --git a/drivers/gpu/drm/msm/dp/dp_link.c b/drivers/gpu/drm/msm/dp/dp_link.c
> index be986da..d35b18e 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 lparse sink count failed\n");
s/lparse/parse/?
> + 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;
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2021-04-13 3:44 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-12 17:03 [PATCH 2/2] drm/msm/dp: do not re initialize of audio_comp Kuogee Hsieh
2021-04-13 3:44 ` Stephen Boyd
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).