Messages in this thread | | | Date | Wed, 21 Apr 2021 10:43:42 -0700 | From | khsieh@codeauro ... | Subject | Re: [PATCH v3 3/3] drm/msm/dp: check main link status before start aux read |
| |
On 2021-04-20 16:38, Stephen Boyd wrote: > Quoting Kuogee Hsieh (2021-04-16 10:38:51) >> 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 V3: >> -- check core_initialized before handle irq_hpd >> Signed-off-by: Kuogee Hsieh <khsieh@codeaurora.org> >> --- >> drivers/gpu/drm/msm/dp/dp_aux.c | 5 +++++ >> drivers/gpu/drm/msm/dp/dp_display.c | 14 ++++++++++---- >> drivers/gpu/drm/msm/dp/dp_link.c | 20 +++++++++++++++----- >> 3 files changed, 30 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..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; >> + } >> + > > This still makes me concerned. Any possibility to not do this and have > the phy cut the connection off and have this transfer timeout > immediately? no, we have to wait hardware AUX_NACK timeout. only this or the abort flag used last time. Last time you have kernel crash because of service irq_hpd while clock is turned off. I have add core_initialized checking wiinin irq_hpd to prevent this. I think abort flag approach is safer.
> >> aux->native = msg->request & (DP_AUX_NATIVE_WRITE & >> DP_AUX_NATIVE_READ); >> >> /* Ignore address only message */ >> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c >> b/drivers/gpu/drm/msm/dp/dp_display.c >> index 1784e11..db3f45e 100644 >> --- a/drivers/gpu/drm/msm/dp/dp_display.c >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c >> @@ -571,7 +571,7 @@ static int dp_hpd_plug_handle(struct >> dp_display_private *dp, u32 data) >> dp->hpd_state = ST_DISCONNECTED; >> >> if (ret == -ECONNRESET) { /* cable unplugged */ >> - dp->core_initialized = false; >> + DRM_ERROR("dongle unplugged = %d\n", ret); > > Is this a debug message? > >> } >> >> } else { >> @@ -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 */ >> + DRM_ERROR("dongle unplugged = %d\n", ret); > > Another debug message? > >> + } >> } >> >> 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; >> -- > > Can you split this part off into another patch? It seems to stand on > its > own as it makes the code more robust to transfer errors in the sink > parsing code.
| |