linux-kernel - Re: [PATCH v8 1/2] drm/msm/dp: force link training for display resolution change

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <e27517c8-874e-1002-938b-1471fc338bb9@quicinc.com>
Date:   Wed, 22 Jun 2022 08:21:59 -0700
From:   Kuogee Hsieh <quic_khsieh@...cinc.com>
To:     Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
        <dri-devel@...ts.freedesktop.org>, <robdclark@...il.com>,
        <sean@...rly.run>, <swboyd@...omium.org>, <dianders@...omium.org>,
        <vkoul@...nel.org>, <daniel@...ll.ch>, <airlied@...ux.ie>,
        <agross@...nel.org>, <bjorn.andersson@...aro.org>
CC:     <quic_abhinavk@...cinc.com>, <quic_aravindh@...cinc.com>,
        <quic_sbillaka@...cinc.com>, <freedreno@...ts.freedesktop.org>,
        <linux-arm-msm@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v8 1/2] drm/msm/dp: force link training for display
 resolution change


On 6/22/2022 12:26 AM, Dmitry Baryshkov wrote:
> On 16/06/2022 20:09, Kuogee Hsieh wrote:
>> Display resolution change is implemented through drm modeset. Older
>> modeset (resolution) has to be disabled first before newer modeset
>> (resolution) can be enabled. 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. In this case, main link retraining
>> will not be executed by irq_hdp_handle(). Hence 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 will bypass irq_hpd_handle() in favor of directly call
>> dp_ctrl_on_stream() to always perform link training in regardless of
>> main link status. So that no unexpected exception resolution change
>> failure cases will happen. Also this implementation are more efficient
>> than manual kicking off irq_hpd_handle 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
>>
>> Changes in v6:
>> -- split dp_ctrl_on_stream() for phy test case
>> -- revise commit text for modeset
>>
>> Changes in v7:
>> -- drop 0 assignment at local variable (ret = 0)
>>
>> Changes in v8:
>> -- add patch to remove pixel_rate from dp_ctrl
>>
>> 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    | 31 
>> +++++++++++++++++++++++--------
>>   drivers/gpu/drm/msm/dp/dp_ctrl.h    |  3 ++-
>>   drivers/gpu/drm/msm/dp/dp_display.c | 13 ++++++-------
>>   3 files changed, 31 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c 
>> b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> index af7a80c..01028b5 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_phy_test_report(&ctrl->dp_ctrl);
>>       else
>>           DRM_ERROR("failed to enable DP link controller\n");
>>   @@ -1807,7 +1807,27 @@ static int dp_ctrl_link_retrain(struct 
>> dp_ctrl_private *ctrl)
>>       return dp_ctrl_setup_main_link(ctrl, &training_step);
>>   }
>>   -int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl)
>> +int dp_ctrl_on_stream_phy_test_report(struct dp_ctrl *dp_ctrl)
>> +{
>> +    int ret;
>> +    struct dp_ctrl_private *ctrl;
>> +
>> +    ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
>> +
>> +    ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
>
> Stephen has raised an interesting question. Comparing this to the 
> dp_ctrl_on_stream(), he noticed that we do not halve the pixel clock 
> here (if the wide bus is supported). So, the question is if this is 
> correct or not.
>
pixel is for video stream which has nothing to do phy test.

Therefore no half pixel clock rate required for phy test.

>> +
>> +    ret = dp_ctrl_enable_stream_clocks(ctrl);
>> +    if (ret) {
>> +        DRM_ERROR("Failed to start pixel clocks. ret=%d\n", ret);
>> +        return ret;
>> +    }
>> +
>> +    dp_ctrl_send_phy_test_pattern(ctrl);
>> +
>> +    return 0;
>> +}
>> +
>> +int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl, bool force_link_train)
>>   {
>>       int ret = 0;
>>       bool mainlink_ready = false;
>> @@ -1843,12 +1863,7 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl)
>>           goto end;
>>       }
>>   -    if (ctrl->link->sink_request & DP_TEST_LINK_PHY_TEST_PATTERN) {
>> -        dp_ctrl_send_phy_test_pattern(ctrl);
>> -        return 0;
>> -    }
>> -
>> -    if (!dp_ctrl_channel_eq_ok(ctrl))
>> +    if (force_link_train || !dp_ctrl_channel_eq_ok(ctrl))
>>           dp_ctrl_link_retrain(ctrl);
>>         /* stop txing train pattern to end link training */
>
>