| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 14 Jun 2022 13:27:12 -0700
From: Kuogee Hsieh <quic_khsieh@...cinc.com>
To: Stephen Boyd <swboyd@...omium.org>, <agross@...nel.org>,
<airlied@...ux.ie>, <bjorn.andersson@...aro.org>,
<daniel@...ll.ch>, <dianders@...omium.org>,
<dmitry.baryshkov@...aro.org>, <robdclark@...il.com>,
<sean@...rly.run>, <vkoul@...nel.org>
CC: <quic_abhinavk@...cinc.com>, <quic_aravindh@...cinc.com>,
<quic_sbillaka@...cinc.com>, <freedreno@...ts.freedesktop.org>,
<dri-devel@...ts.freedesktop.org>, <linux-arm-msm@...r.kernel.org>,
<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v5] drm/msm/dp: force link training for display resolution
change
On 6/14/2022 1:38 AM, Stephen Boyd wrote:
> Quoting Kuogee Hsieh (2022-06-13 14:48:37)
>> During display resolution changes display have to be disabled first
>> followed by display enabling with new resolution. Display disable
>> will turn off both pixel clock and main link clock so that main link
>> have to be re-trained during display enable to have new video stream
>> flow again. At current implementation, display enable function manually
>> kicks up irq_hpd_handle which will read panel link status and start link
>> training if link status is not in sync state. However, there is rare
>> case that a particular panel links status keep staying in sync for
>> some period of time after main link had been shut down previously at
>> display disabled. Main link retraining will not be executed by
>> irq_hdp_handle() if the link status read from panel shows it is in
>> sync state. If this was happen, then video stream of newer display
>> resolution will fail to be transmitted to panel due to main link is
>> not in sync between host and panel. This patch force main link always
>> be retrained during display enable procedure to prevent this rare
>> failed case from happening. Also this implementation are more
>> efficient than manual kicking off irq_hpd_handle function.
> How is resolution change different from disabling and enabling the
> display? The commit text talks about resolution changes, but the code
> doesn't compare resolutions from before and after to know when to
> retrain the link. Can the code be made to actually do what the commit
> text says? It would be clearer if the code looked for actual resolution
> changes instead of hooking the dp_bridge_enable() function.
>
>> Changes in v2:
>> -- set force_link_train flag on DP only (is_edp == false)
>>
>> Changes in v3:
>> -- revise commit text
>> -- add Fixes tag
>>
>> Changes in v4:
>> -- revise commit text
>>
>> Changes in v5:
>> -- fix spelling at commit text
>>
>> Fixes: 62671d2ef24b ("drm/msm/dp: fixes wrong connection state caused by failure of link train")
>> Signed-off-by: Kuogee Hsieh <quic_khsieh@...cinc.com>
>> ---
>> drivers/gpu/drm/msm/dp/dp_ctrl.c | 6 +++---
>> drivers/gpu/drm/msm/dp/dp_ctrl.h | 2 +-
>> drivers/gpu/drm/msm/dp/dp_display.c | 15 ++++++++-------
>> 3 files changed, 12 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> index af7a80c..bea93eb 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> @@ -1551,7 +1551,7 @@ static int dp_ctrl_process_phy_test_request(struct dp_ctrl_private *ctrl)
>>
>> ret = dp_ctrl_on_link(&ctrl->dp_ctrl);
>> if (!ret)
>> - ret = dp_ctrl_on_stream(&ctrl->dp_ctrl);
>> + ret = dp_ctrl_on_stream(&ctrl->dp_ctrl, false);
> Does this even matter if it's true or false? The 'sink_request' has
> DP_TEST_LINK_PHY_TEST_PATTERN set from what I can tell, and then
> dp_ctrl_on_stream() bails out before calling dp_ctrl_link_retrain()
> anyway. It would be nice if we could split dp_ctrl_on_stream() so that
> the part after the check for the sink request is a different function
> that is called by dp_display.c and then this code can call the 'prepare'
> function that does the first part. Then we can ignore the testing path
> in the code, and possibly remove the conditional in dp_ctrl_on_stream()?
>
>> else
>> DRM_ERROR("failed to enable DP link controller\n");
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
>> index c388323..370348d 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -872,7 +872,7 @@ static int dp_display_enable(struct dp_display_private *dp, u32 data)
>> return 0;
>> }
>>
>> - rc = dp_ctrl_on_stream(dp->ctrl);
>> + rc = dp_ctrl_on_stream(dp->ctrl, data);
>> if (!rc)
>> dp_display->power_on = true;
>>
>> @@ -1654,6 +1654,7 @@ void dp_bridge_enable(struct drm_bridge *drm_bridge)
>> int rc = 0;
>> struct dp_display_private *dp_display;
>> u32 state;
>> + bool force_link_train = false;
>>
>> dp_display = container_of(dp, struct dp_display_private, dp_display);
>> if (!dp_display->dp_mode.drm_mode.clock) {
>> @@ -1688,10 +1689,14 @@ void dp_bridge_enable(struct drm_bridge *drm_bridge)
>>
>> state = dp_display->hpd_state;
>>
>> - if (state == ST_DISPLAY_OFF)
>> + if (state == ST_DISPLAY_OFF) {
>> dp_display_host_phy_init(dp_display);
>>
>> - dp_display_enable(dp_display, 0);
>> + if (!dp->is_edp)
> I didn't see any answer to my question about why edp is special on v4.
> Can you at least add a comment to the code about why edp doesn't need to
> unconditionally retrain, but DP does?
Sorry, missed this one.
This is my mistake, both DP and eDP are same. will remove is_edp flag
checking.
>
>> + force_link_train = true;
>> + }
>> +
>> + dp_display_enable(dp_display, force_link_train);
>>
>> rc = dp_display_post_enable(dp);
>> if (rc) {
Powered by blists - more mailing lists