From: khsieh@codeaurora.org
To: Stephen Boyd <swboyd@chromium.org>
Cc: freedreno@lists.freedesktop.org, airlied@linux.ie,
linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org,
abhinavk@codeaurora.org, dri-devel@lists.freedesktop.org,
aravindh@codeaurora.org, sean@poorly.run
Subject: Re: [PATCH v3 3/3] drm/msm/dp: check main link status before start aux read
Date: Wed, 21 Apr 2021 10:43:42 -0700 [thread overview]
Message-ID: <4da92917bb65490f500faf7bf9b7003f@codeaurora.org> (raw)
In-Reply-To: <161896193053.46595.7590816467281538002@swboyd.mtv.corp.google.com>
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.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2021-04-21 17:44 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-16 17:38 [PATCH v3 3/3] drm/msm/dp: check main link status before start aux read Kuogee Hsieh
2021-04-20 23:38 ` Stephen Boyd
2021-04-21 17:43 ` khsieh [this message]
-- strict thread matches above, loose matches on Subject: below --
2021-04-15 22:46 [PATCH v2 " Stephen Boyd
2021-04-15 23:28 ` [PATCH v3 " Kuogee Hsieh
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4da92917bb65490f500faf7bf9b7003f@codeaurora.org \
--to=khsieh@codeaurora.org \
--cc=abhinavk@codeaurora.org \
--cc=airlied@linux.ie \
--cc=aravindh@codeaurora.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=freedreno@lists.freedesktop.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=sean@poorly.run \
--cc=swboyd@chromium.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).