| 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: Mon, 15 Nov 2021 12:23:02 -0800
From: khsieh@...eaurora.org
To: Kuogee Hsieh <quic_khsieh@...cinc.com>
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,
bjorn.andersson@...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
Subject: Re: [PATCH v4] drm/msm/dp: do not initialize phy until plugin
interrupt received
On 2021-11-09 13:38, Kuogee Hsieh wrote:
> From: Kuogee Hsieh <khsieh@...eaurora.org>
>
> Current DP drivers have regulators, clocks, irq and phy are grouped
> together within a function and executed not in a symmetric manner.
> This increase difficulty of code maintenance and limited code
> scalability.
> This patch divided the driver life cycle of operation into four states,
> resume (including booting up), dongle plugin, dongle unplugged and
> suspend.
> Regulators, core clocks and irq are grouped together and enabled at
> resume
> (or booting up) so that the DP controller is armed and ready to receive
> HPD
> plugin interrupts. HPD plugin interrupt is generated when a dongle
> plugs
> into DUT (device under test). Once HPD plugin interrupt is received, DP
> controller will initialize phy so that dpcd read/write will function
> and
> following link training can be proceeded successfully. DP phy will be
> disabled after main link is teared down at end of unplugged HPD
> interrupt
> handle triggered by dongle unplugged out of DUT. Finally regulators,
> code
> clocks and irq are disabled at corresponding suspension.
>
> 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
>
> Changes in V4:
> -- rewording commit text to match this commit changes
>
> Fixes: e91e3065a806 ("drm/msm/dp: Add DP compliance tests on
> Snapdragon Chipsets")
>
> Signed-off-by: Kuogee Hsieh <quic_khsieh@...cinc.com>
> ---
any more comments?
> 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);
Powered by blists - more mailing lists