| 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
| ||
|
Message-ID: <d25ccef7-2d01-c337-ef5f-62a29de16a0e@quicinc.com>
Date: Tue, 9 Nov 2021 08:41:33 -0800
From: Kuogee Hsieh <quic_khsieh@...cinc.com>
To: Bjorn Andersson <bjorn.andersson@...aro.org>
CC: <robdclark@...il.com>, <sean@...rly.run>, <swboyd@...omium.org>,
<vkoul@...nel.org>, <daniel@...ll.ch>, <airlied@...ux.ie>,
<agross@...nel.org>, <dmitry.baryshkov@...aro.org>,
<quic_abhinavk@...cinc.com>, <aravindh@...eaurora.org>,
<freedreno@...ts.freedesktop.org>,
<dri-devel@...ts.freedesktop.org>, <linux-arm-msm@...r.kernel.org>,
<linux-kernel@...r.kernel.org>,
Kuogee Hsieh <khsieh@...eaurora.org>
Subject: Re: [PATCH v3] drm/msm/dp: do not initialize phy until plugin
interrupt received
On 11/8/2021 4:29 PM, Bjorn Andersson wrote:
> On Mon 08 Nov 15:42 PST 2021, Kuogee Hsieh wrote:
>
>> From: Kuogee Hsieh <khsieh@...eaurora.org>
>>
>> Combo phy supports both USB and DP simultaneously. There may has a
>> possible conflict during phy initialization phase between USB and
>> DP driver which may cause USB phy timeout when USB tries to power
>> up its phy. This patch has the DP driver not initialize its phy
>> during DP driver initialization phase. Instead DP driver only enable
>> required regulators and clocks so that it is able to receive HPD
>> interrupts after completion of initialization phase. DP driver will
>> initialize its phy when HPD plug-in interrupt received.
>> This patch also provides a positive side effects which balance regulator
>> enable count since regulator only enabled at initialize phase and resume
>> and disabled at followed suspend.
>>
> I thought you said that you (someone) will attempt to resolve the
> described error in the QMP driver, making this patch unnecessary.
>
> That said, are there any other benefits to this patch? I.e. does it make
> sense not to initialize the phy when there's no cable connected for
> power purposed etc?
> If so this patch looks reasonable in itself, but the commit message is
> wrong.
>
> Thanks,
> Bjorn
sorry for the confusing.
I will redo the commit test.
Thanks,
>> Changes in V2:
>> -- removed unnecessary dp_ctrl NULL check
>> -- removed unnecessary phy init_count and power_count DRM_DEBUG_DP logs
>> -- remove flip parameter out of dp_ctrl_irq_enable()
>> -- add fixes tag
>>
>> Changes in V3:
>> -- call dp_display_host_phy_init() instead of dp_ctrl_phy_init() at dp_display_host_init() for eDP
>>
>> Fixes: e91e3065a806 ("drm/msm/dp: Add DP compliance tests on Snapdragon Chipsets")
>>
>> Signed-off-by: Kuogee Hsieh <quic_khsieh@...cinc.com>
>> ---
>> drivers/gpu/drm/msm/dp/dp_ctrl.c | 87 ++++++++++++++++---------------------
>> drivers/gpu/drm/msm/dp/dp_ctrl.h | 9 ++--
>> drivers/gpu/drm/msm/dp/dp_display.c | 83 ++++++++++++++++++++++++++---------
>> 3 files changed, 105 insertions(+), 74 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> index 7ec155d..4788e8c 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> @@ -1364,60 +1364,54 @@ static int dp_ctrl_enable_stream_clocks(struct dp_ctrl_private *ctrl)
>> return ret;
>> }
>>
>> -int dp_ctrl_host_init(struct dp_ctrl *dp_ctrl, bool flip, bool reset)
>> +void dp_ctrl_irq_enable(struct dp_ctrl *dp_ctrl)
>> +{
>> + struct dp_ctrl_private *ctrl;
>> +
>> + ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
>> +
>> + dp_catalog_ctrl_reset(ctrl->catalog);
>> +
>> + dp_catalog_ctrl_enable_irq(ctrl->catalog, true);
>> +}
>> +
>> +void dp_ctrl_irq_disable(struct dp_ctrl *dp_ctrl)
>> +{
>> + struct dp_ctrl_private *ctrl;
>> +
>> + ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
>> +
>> + dp_catalog_ctrl_reset(ctrl->catalog);
>> +
>> + dp_catalog_ctrl_enable_irq(ctrl->catalog, false);
>> +}
>> +
>> +void dp_ctrl_phy_init(struct dp_ctrl *dp_ctrl)
>> {
>> struct dp_ctrl_private *ctrl;
>> struct dp_io *dp_io;
>> struct phy *phy;
>>
>> - if (!dp_ctrl) {
>> - DRM_ERROR("Invalid input data\n");
>> - return -EINVAL;
>> - }
>> -
>> ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
>> dp_io = &ctrl->parser->io;
>> phy = dp_io->phy;
>>
>> - ctrl->dp_ctrl.orientation = flip;
>> -
>> - if (reset)
>> - dp_catalog_ctrl_reset(ctrl->catalog);
>> -
>> - DRM_DEBUG_DP("flip=%d\n", flip);
>> dp_catalog_ctrl_phy_reset(ctrl->catalog);
>> phy_init(phy);
>> - dp_catalog_ctrl_enable_irq(ctrl->catalog, true);
>> -
>> - return 0;
>> }
>>
>> -/**
>> - * dp_ctrl_host_deinit() - Uninitialize DP controller
>> - * @dp_ctrl: Display Port Driver data
>> - *
>> - * Perform required steps to uninitialize DP controller
>> - * and its resources.
>> - */
>> -void dp_ctrl_host_deinit(struct dp_ctrl *dp_ctrl)
>> +void dp_ctrl_phy_exit(struct dp_ctrl *dp_ctrl)
>> {
>> struct dp_ctrl_private *ctrl;
>> struct dp_io *dp_io;
>> struct phy *phy;
>>
>> - if (!dp_ctrl) {
>> - DRM_ERROR("Invalid input data\n");
>> - return;
>> - }
>> -
>> ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
>> dp_io = &ctrl->parser->io;
>> phy = dp_io->phy;
>>
>> - dp_catalog_ctrl_enable_irq(ctrl->catalog, false);
>> + dp_catalog_ctrl_phy_reset(ctrl->catalog);
>> phy_exit(phy);
>> -
>> - DRM_DEBUG_DP("Host deinitialized successfully\n");
>> }
>>
>> static bool dp_ctrl_use_fixed_nvid(struct dp_ctrl_private *ctrl)
>> @@ -1895,8 +1889,14 @@ int dp_ctrl_off_link_stream(struct dp_ctrl *dp_ctrl)
>> return ret;
>> }
>>
>> + DRM_DEBUG_DP("Before, phy=%x init_count=%d power_on=%d\n",
>> + (u32)(uintptr_t)phy, phy->init_count, phy->power_count);
>> +
>> phy_power_off(phy);
>>
>> + DRM_DEBUG_DP("After, phy=%x init_count=%d power_on=%d\n",
>> + (u32)(uintptr_t)phy, phy->init_count, phy->power_count);
>> +
>> /* aux channel down, reinit phy */
>> phy_exit(phy);
>> phy_init(phy);
>> @@ -1905,23 +1905,6 @@ int dp_ctrl_off_link_stream(struct dp_ctrl *dp_ctrl)
>> return ret;
>> }
>>
>> -void dp_ctrl_off_phy(struct dp_ctrl *dp_ctrl)
>> -{
>> - struct dp_ctrl_private *ctrl;
>> - struct dp_io *dp_io;
>> - struct phy *phy;
>> -
>> - ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
>> - dp_io = &ctrl->parser->io;
>> - phy = dp_io->phy;
>> -
>> - dp_catalog_ctrl_reset(ctrl->catalog);
>> -
>> - phy_exit(phy);
>> -
>> - DRM_DEBUG_DP("DP off phy done\n");
>> -}
>> -
>> int dp_ctrl_off(struct dp_ctrl *dp_ctrl)
>> {
>> struct dp_ctrl_private *ctrl;
>> @@ -1949,10 +1932,14 @@ int dp_ctrl_off(struct dp_ctrl *dp_ctrl)
>> DRM_ERROR("Failed to disable link clocks. ret=%d\n", ret);
>> }
>>
>> + DRM_DEBUG_DP("Before, phy=%x init_count=%d power_on=%d\n",
>> + (u32)(uintptr_t)phy, phy->init_count, phy->power_count);
>> +
>> phy_power_off(phy);
>> - phy_exit(phy);
>>
>> - DRM_DEBUG_DP("DP off done\n");
>> + DRM_DEBUG_DP("After, phy=%x init_count=%d power_on=%d\n",
>> + (u32)(uintptr_t)phy, phy->init_count, phy->power_count);
>> +
>> return ret;
>> }
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.h b/drivers/gpu/drm/msm/dp/dp_ctrl.h
>> index 2363a2d..30f9414 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.h
>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.h
>> @@ -19,12 +19,9 @@ struct dp_ctrl {
>> u32 pixel_rate;
>> };
>>
>> -int dp_ctrl_host_init(struct dp_ctrl *dp_ctrl, bool flip, bool reset);
>> -void dp_ctrl_host_deinit(struct dp_ctrl *dp_ctrl);
>> int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl);
>> int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl);
>> int dp_ctrl_off_link_stream(struct dp_ctrl *dp_ctrl);
>> -void dp_ctrl_off_phy(struct dp_ctrl *dp_ctrl);
>> int dp_ctrl_off(struct dp_ctrl *dp_ctrl);
>> void dp_ctrl_push_idle(struct dp_ctrl *dp_ctrl);
>> void dp_ctrl_isr(struct dp_ctrl *dp_ctrl);
>> @@ -34,4 +31,10 @@ struct dp_ctrl *dp_ctrl_get(struct device *dev, struct dp_link *link,
>> struct dp_power *power, struct dp_catalog *catalog,
>> struct dp_parser *parser);
>>
>> +void dp_ctrl_irq_enable(struct dp_ctrl *dp_ctrl);
>> +void dp_ctrl_irq_disable(struct dp_ctrl *dp_ctrl);
>> +void dp_ctrl_phy_init(struct dp_ctrl *dp_ctrl);
>> +void dp_ctrl_phy_exit(struct dp_ctrl *dp_ctrl);
>> +void dp_ctrl_irq_phy_exit(struct dp_ctrl *dp_ctrl);
>> +
>> #endif /* _DP_CTRL_H_ */
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
>> index e41dd40..2f113ff 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -84,6 +84,7 @@ struct dp_display_private {
>>
>> /* state variables */
>> bool core_initialized;
>> + bool phy_initialized;
>> bool hpd_irq_on;
>> bool audio_supported;
>>
>> @@ -387,7 +388,29 @@ static int dp_display_process_hpd_high(struct dp_display_private *dp)
>> return rc;
>> }
>>
>> -static void dp_display_host_init(struct dp_display_private *dp, int reset)
>> +static void dp_display_host_phy_init(struct dp_display_private *dp)
>> +{
>> + DRM_DEBUG_DP("core_init=%d phy_init=%d\n",
>> + dp->core_initialized, dp->phy_initialized);
>> +
>> + if (!dp->phy_initialized) {
>> + dp_ctrl_phy_init(dp->ctrl);
>> + dp->phy_initialized = true;
>> + }
>> +}
>> +
>> +static void dp_display_host_phy_exit(struct dp_display_private *dp)
>> +{
>> + DRM_DEBUG_DP("core_init=%d phy_init=%d\n",
>> + dp->core_initialized, dp->phy_initialized);
>> +
>> + if (dp->phy_initialized) {
>> + dp_ctrl_phy_exit(dp->ctrl);
>> + dp->phy_initialized = false;
>> + }
>> +}
>> +
>> +static void dp_display_host_init(struct dp_display_private *dp)
>> {
>> bool flip = false;
>>
>> @@ -400,8 +423,16 @@ static void dp_display_host_init(struct dp_display_private *dp, int reset)
>> if (dp->usbpd->orientation == ORIENTATION_CC2)
>> flip = true;
>>
>> - dp_power_init(dp->power, flip);
>> - dp_ctrl_host_init(dp->ctrl, flip, reset);
>> + dp_power_init(dp->power, false);
>> + dp_ctrl_irq_enable(dp->ctrl);
>> +
>> + /*
>> + * eDP is the embedded primary display and has its own phy
>> + * initialize phy immediately
>> + */
>> + if (dp->dp_display.connector_type == DRM_MODE_CONNECTOR_eDP)
>> + dp_display_host_phy_init(dp);
>> +
>> dp_aux_init(dp->aux);
>> dp->core_initialized = true;
>> }
>> @@ -413,7 +444,7 @@ static void dp_display_host_deinit(struct dp_display_private *dp)
>> return;
>> }
>>
>> - dp_ctrl_host_deinit(dp->ctrl);
>> + dp_ctrl_irq_disable(dp->ctrl);
>> dp_aux_deinit(dp->aux);
>> dp_power_deinit(dp->power);
>>
>> @@ -424,7 +455,7 @@ static int dp_display_usbpd_configure_cb(struct device *dev)
>> {
>> struct dp_display_private *dp = dev_get_dp_display_private(dev);
>>
>> - dp_display_host_init(dp, true);
>> + dp_display_host_phy_init(dp);
>>
>> return dp_display_process_hpd_high(dp);
>> }
>> @@ -551,7 +582,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;
>> + dp->phy_initialized = false;
>> }
>>
>> } else {
>> @@ -623,9 +654,8 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
>> if (state == ST_DISCONNECTED) {
>> /* triggered by irq_hdp with sink_count = 0 */
>> if (dp->link->sink_count == 0) {
>> - dp_ctrl_off_phy(dp->ctrl);
>> + dp_display_host_phy_exit(dp);
>> hpd->hpd_high = 0;
>> - dp->core_initialized = false;
>> }
>> mutex_unlock(&dp->event_mutex);
>> return 0;
>> @@ -716,7 +746,7 @@ static int dp_irq_hpd_handle(struct dp_display_private *dp, u32 data)
>>
>> ret = dp_display_usbpd_attention_cb(&dp->pdev->dev);
>> if (ret == -ECONNRESET) { /* cable unplugged */
>> - dp->core_initialized = false;
>> + dp->phy_initialized = false;
>> }
>> DRM_DEBUG_DP("hpd_state=%d\n", state);
>>
>> @@ -918,12 +948,19 @@ static int dp_display_disable(struct dp_display_private *dp, u32 data)
>>
>> dp_display->audio_enabled = false;
>>
>> - /* triggered by irq_hpd with sink_count = 0 */
>> if (dp->link->sink_count == 0) {
>> + /*
>> + * irq_hpd with sink_count = 0
>> + * hdmi unplugged out of dongle
>> + */
>> dp_ctrl_off_link_stream(dp->ctrl);
>> } else {
>> + /*
>> + * unplugged interrupt
>> + * dongle unplugged out of DUT
>> + */
>> dp_ctrl_off(dp->ctrl);
>> - dp->core_initialized = false;
>> + dp_display_host_phy_exit(dp);
>> }
>>
>> dp_power_panel_on(dp->power, false);
>> @@ -1059,7 +1096,7 @@ void msm_dp_snapshot(struct msm_disp_state *disp_state, struct msm_dp *dp)
>> static void dp_display_config_hpd(struct dp_display_private *dp)
>> {
>>
>> - dp_display_host_init(dp, true);
>> + dp_display_host_init(dp);
>> dp_catalog_ctrl_hpd_config(dp->catalog);
>>
>> /* Enable interrupt first time
>> @@ -1338,20 +1375,22 @@ static int dp_pm_resume(struct device *dev)
>> dp->hpd_state = ST_DISCONNECTED;
>>
>> /* turn on dp ctrl/phy */
>> - dp_display_host_init(dp, true);
>> + dp_display_host_init(dp);
>>
>> dp_catalog_ctrl_hpd_config(dp->catalog);
>>
>> - /*
>> - * set sink to normal operation mode -- D0
>> - * before dpcd read
>> - */
>> - dp_link_psm_config(dp->link, &dp->panel->link_info, false);
>> -
>> if (dp_catalog_link_is_connected(dp->catalog)) {
>> + /*
>> + * set sink to normal operation mode -- D0
>> + * before dpcd read
>> + */
>> + dp_display_host_phy_init(dp);
>> + dp_link_psm_config(dp->link, &dp->panel->link_info, false);
>> sink_count = drm_dp_read_sink_count(dp->aux);
>> if (sink_count < 0)
>> sink_count = 0;
>> +
>> + dp_display_host_phy_exit(dp);
>> }
>>
>> dp->link->sink_count = sink_count;
>> @@ -1399,6 +1438,8 @@ static int dp_pm_suspend(struct device *dev)
>> dp_display_host_deinit(dp);
>> }
>>
>> + dp_display_host_phy_exit(dp);
>> +
>> dp->hpd_state = ST_SUSPENDED;
>>
>> /* host_init will be called at pm_resume */
>> @@ -1473,7 +1514,7 @@ void msm_dp_irq_postinstall(struct msm_dp *dp_display)
>> enable_irq(dp->irq);
>> dp_hpd_connect(dp->usbpd, true);
>> } else {
>> - dp_add_event(dp, EV_HPD_INIT_SETUP, 0, dp->id * 10);
>> + dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 0);
>> }
>> }
>>
>> @@ -1567,7 +1608,7 @@ int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder)
>> state = dp_display->hpd_state;
>>
>> if (state == ST_DISPLAY_OFF)
>> - dp_display_host_init(dp_display, true);
>> + dp_display_host_phy_init(dp_display);
>>
>> dp_display_enable(dp_display, 0);
>>
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> a Linux Foundation Collaborative Project
>>
Powered by blists - more mailing lists