Messages in this thread | | | Date | Fri, 14 Jan 2022 09:04:46 -0800 | Subject | Re: [PATCH v13 1/4] drm/msm/dp: do not initialize phy until plugin interrupt received | From | Kuogee Hsieh <> |
| |
On 1/13/2022 6:42 PM, Stephen Boyd wrote: > Quoting Kuogee Hsieh (2022-01-13 15:53:36) >> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c >> index 7cc4d21..b3c5404 100644 >> --- a/drivers/gpu/drm/msm/dp/dp_display.c >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c >> @@ -83,6 +83,7 @@ struct dp_display_private { >> >> /* state variables */ >> bool core_initialized; >> + bool phy_initialized; >> bool hpd_irq_on; >> bool audio_supported; >> >> @@ -372,21 +373,38 @@ 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) >> { >> - bool flip = false; >> + 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) >> +{ >> DRM_DEBUG_DP("core_initialized=%d\n", dp->core_initialized); >> if (dp->core_initialized) { > When is this true? From what I see dp_display_host_init() is only called > from two places: resume where core_initialized has been set to false > during suspend or from dp_display_config_hpd() kicked by the kthread > where core_initialized is also false. > > Also, I see that dp_display_host_deinit() is only called from suspend > now, so 'core_initialized' is almost always true, except for on the > resume path and before the kthread is started and in the case that the > driver probes but can't start the kthread for some reason (is that > real?).
Yes, that true.
The purpose of dp_display_host_init() is to configure both dp controller and hpd controller to be ready to receive
HPD plugged-in interrupts and at plugged-in handler enable dp phy. Therefore core_initialized is set to true at both booting up and resume and false at suspend.
>> DRM_DEBUG_DP("DP core already initialized\n"); >> return; >> } >> >> - 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_reset_irq_ctrl(dp->ctrl, true); >> dp_aux_init(dp->aux); >> dp->core_initialized = true; >> } >> @@ -892,12 +901,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_display->power_on = false; >> @@ -1027,7 +1043,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 >> @@ -1306,20 +1322,23 @@ 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; >> @@ -1363,7 +1382,11 @@ static int dp_pm_suspend(struct device *dev) >> if (dp_power_clk_status(dp->power, DP_CTRL_PM)) >> dp_ctrl_off_link_stream(dp->ctrl); >> >> + dp_display_host_phy_exit(dp); >> + > Why is there a newline here? > >> dp_display_host_deinit(dp); >> + } else { >> + dp_display_host_phy_exit(dp); >> } >> >> dp->hpd_state = ST_SUSPENDED; > There's a dp->core_initialized = false right here but it's not in the > diff window. It's redundant now because the hunk above is basically > > if (dp->core_initialized == true) { > ... > dp_display_host_phy_exit(dp); > dp_display_host_deinit(dp); > } else { > dp_display_host_phy_exit(dp); > } > > dp->hpd_state = ST_SUSPENDED; > > and dp_display_host_deinit() sets core_initialized to false, thus > core_initialized will be false here already. Can you remove the > duplicate assignment? yes, >> @@ -1535,7 +1558,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); >>
| |