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 <[email protected]>
---
drivers/gpu/drm/msm/dp/dp_display.c | 20 ++++++++++++--------
1 file changed, 12 insertions(+), 8 deletions(-)
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)
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
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 <[email protected]>
The code looks OK to me, so
Reviewed-by: Stephen Boyd <[email protected]>
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 <[email protected]>
>
> The code looks OK to me, so
>
> Reviewed-by: Stephen Boyd <[email protected]>
>
> once the commit text is cleaned up to indicate the proper function
> names.
Thanks , will fix all those.