| Message ID | 1677263398-13801-1-git-send-email-quic_khsieh@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/msm/dp: check core_initialized flag at both host_init() and host_deinit() | expand |
Quoting Kuogee Hsieh (2023-02-24 10:29:58) > There is a reboot/suspend test case where system suspend is forced during > system booting up. Since host_init() of external DP is executed at hpd dp_display_host_init()? > thread context, this test case may created a scenario that host_deinit() dp_display_host_deinit()? > from pm_suspend() run before host_init() if hpd thread has no chance to > run during booting up while suspend request command was issued. > At this scenario system will crash at aux register access at host_deinit() > since aux clock had not yet been enabled by host_init(). Therefore we The aux clk is enabled in dp_power_clk_enable() right? Can you clarify? > have to ensure aux clock enabled by checking core_initialized flag before > access aux registers at pm_suspend. I'd much more like to get rid of 'core_initialized'. What is preventing us from enabling the power (i.e. dp_power_init()), or at least enough clks and pm runtime state, during probe? That would fix this problem and also clean things up. As I understand, the device is half initialized in probe and half initialized in the kthread. If we put all power management into the runtime PM ops and synced that state during probe so that runtime PM state matched device probe state we could make runtime PM be the only suspend function and then push the power state tracking into the device core. > > Fixes: 989ebe7bc446 ("drm/msm/dp: do not initialize phy until plugin interrupt received") > Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> The code looks OK to me, so Reviewed-by: Stephen Boyd <swboyd@chromium.org> once the commit text is cleaned up to indicate the proper function names.
Hi Stephen On 2/27/2023 11:53 AM, Stephen Boyd wrote: > Quoting Kuogee Hsieh (2023-02-24 10:29:58) >> There is a reboot/suspend test case where system suspend is forced during >> system booting up. Since host_init() of external DP is executed at hpd > > dp_display_host_init()? > >> thread context, this test case may created a scenario that host_deinit() > > dp_display_host_deinit()? > ack for both. >> from pm_suspend() run before host_init() if hpd thread has no chance to >> run during booting up while suspend request command was issued. >> At this scenario system will crash at aux register access at host_deinit() >> since aux clock had not yet been enabled by host_init(). Therefore we > > The aux clk is enabled in dp_power_clk_enable() right? Can you clarify? > Yes, thats right. Its mapped to core_*** in the dts hence the name goes to the DP_CORE_PM case in the dp_power_clk_enable() 3092 clocks = <&dispcc DISP_CC_MDSS_AHB_CLK>, 3093 <&dispcc DISP_CC_MDSS_DP_AUX_CLK>, 3094 <&dispcc DISP_CC_MDSS_DP_LINK_CLK>, 3095 <&dispcc DISP_CC_MDSS_DP_LINK_INTF_CLK>, 3096 <&dispcc DISP_CC_MDSS_DP_PIXEL_CLK>; 3097 clock-names = "core_iface", "core_aux" >> have to ensure aux clock enabled by checking core_initialized flag before >> access aux registers at pm_suspend. > > I'd much more like to get rid of 'core_initialized'. What is preventing > us from enabling the power (i.e. dp_power_init()), or at least enough > clks and pm runtime state, during probe? That would fix this problem and > also clean things up. As I understand, the device is half initialized in > probe and half initialized in the kthread. If we put all power > management into the runtime PM ops and synced that state during probe so > that runtime PM state matched device probe state we could make runtime > PM be the only suspend function and then push the power state tracking > into the device core. > You are correct. https://patchwork.freedesktop.org/patch/523879/ will be doing that. Its still in review. >> >> Fixes: 989ebe7bc446 ("drm/msm/dp: do not initialize phy until plugin interrupt received") >> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> > > The code looks OK to me, so > > Reviewed-by: Stephen Boyd <swboyd@chromium.org> > > once the commit text is cleaned up to indicate the proper function > names. Thanks , will fix all those.
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index bde1a7c..1850738 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -460,10 +460,12 @@ static void dp_display_host_init(struct dp_display_private *dp) dp->dp_display.connector_type, dp->core_initialized, dp->phy_initialized); - dp_power_init(dp->power, false); - dp_ctrl_reset_irq_ctrl(dp->ctrl, true); - dp_aux_init(dp->aux); - dp->core_initialized = true; + if (!dp->core_initialized) { + dp_power_init(dp->power, false); + dp_ctrl_reset_irq_ctrl(dp->ctrl, true); + dp_aux_init(dp->aux); + dp->core_initialized = true; + } } static void dp_display_host_deinit(struct dp_display_private *dp) @@ -472,10 +474,12 @@ static void dp_display_host_deinit(struct dp_display_private *dp) dp->dp_display.connector_type, dp->core_initialized, dp->phy_initialized); - dp_ctrl_reset_irq_ctrl(dp->ctrl, false); - dp_aux_deinit(dp->aux); - dp_power_deinit(dp->power); - dp->core_initialized = false; + if (dp->core_initialized) { + dp_ctrl_reset_irq_ctrl(dp->ctrl, false); + dp_aux_deinit(dp->aux); + dp_power_deinit(dp->power); + dp->core_initialized = false; + } } static int dp_display_usbpd_configure_cb(struct device *dev)
There is a reboot/suspend test case where system suspend is forced during system booting up. Since host_init() of external DP is executed at hpd thread context, this test case may created a scenario that host_deinit() from pm_suspend() run before host_init() if hpd thread has no chance to run during booting up while suspend request command was issued. At this scenario system will crash at aux register access at host_deinit() since aux clock had not yet been enabled by host_init(). Therefore we have to ensure aux clock enabled by checking core_initialized flag before access aux registers at pm_suspend. Fixes: 989ebe7bc446 ("drm/msm/dp: do not initialize phy until plugin interrupt received") Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> --- drivers/gpu/drm/msm/dp/dp_display.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)