lkml.org 
[lkml]   [2021]   [Apr]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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.

\
 
 \ /
  Last update: 2021-04-21 19:45    [W:0.070 / U:0.040 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site