On 5/11/2022 2:47 PM, Dmitry Baryshkov wrote:
On 12/05/2022 00:40, Kuogee Hsieh wrote:
dp_catalog_ctrl_reset() will software reset DP controller. But it will
not reset programmable registers to default value. DP driver still have
to clear mask bits to interrupt status registers to disable interrupts
after software reset of controller. This patch removes the enable flag
condition checking to always clear mask bits of interrupt status
registers to disable interrupts if enable flag is false.
This patch also will fix the potential problem happen at system
suspend where
dp_ctrl_reset_irq_ctrl() was called to try to disable HPD related
irqs but
the irq is still unmasked unexpectedly
Why is it unmasked? Especially unexpectedly.
due to dp_catalog_ctrl_reset() does not clear hpd interrupt mask bits.
This is the problem this patch try to fix it.
I will revise commit text to explain more.
and can come in while system are
suspending. This leads to bus hangs if the irq is handled after we
power down
the DP hardware because we run the irq handler and access a device
register
assuming that no irq could ever come in if we powered down the
device. We
don't know when the irq will be handled though, so it's possible the
irq is
pending from before we disable the irq in the hardware.
Please split into two patches.
this is just the same problem mention above.
Changes in v2:
-- add more details commit text
Changes in v3:
-- add synchrons_irq()
-- add atomic_t suspended
Changes in v4:
-- correct Fixes's commit ID
Fixes: 989ebe7bc446 ("drm/msm/dp: do not initialize phy until plugin
interrupt received")
Signed-off-by: Kuogee Hsieh <quic_khsieh@xxxxxxxxxxx>
---
drivers/gpu/drm/msm/dp/dp_ctrl.c | 9 +++++++--
drivers/gpu/drm/msm/dp/dp_display.c | 18 ++++++++++++++++++
2 files changed, 25 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c
b/drivers/gpu/drm/msm/dp/dp_ctrl.c
index af7a80c..f3e333e 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -1389,8 +1389,13 @@ void dp_ctrl_reset_irq_ctrl(struct dp_ctrl
*dp_ctrl, bool enable)
dp_catalog_ctrl_reset(ctrl->catalog);
- if (enable)
- dp_catalog_ctrl_enable_irq(ctrl->catalog, enable);
+ /*
+ * all dp controller programmable registers will not
+ * be reset to default value after DP_SW_RESET
+ * therefore interrupt mask bits have to be updated
+ * to enable/disable interrupts
+ */
+ dp_catalog_ctrl_enable_irq(ctrl->catalog, enable);
}
void dp_ctrl_phy_init(struct dp_ctrl *dp_ctrl)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index c388323..c34dbfc 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -98,6 +98,8 @@ struct dp_display_private {
struct dp_ctrl *ctrl;
struct dp_debug *debug;
+ atomic_t suspended;
Usage of atomic_t signifies that something is wrong here. Bool should
be enough here.
suspended flag are shared by both event_thread and display_irq_handler,
i think it need either atomic
or protected by spinlock.
+
struct dp_usbpd_cb usbpd_cb;
struct dp_display_mode dp_mode;
struct msm_dp dp_display;
@@ -187,6 +189,11 @@ static int dp_add_event(struct
dp_display_private *dp_priv, u32 event,
int pndx;
spin_lock_irqsave(&dp_priv->event_lock, flag);
+ if (atomic_read(&dp_priv->suspended)) {
+ spin_unlock_irqrestore(&dp_priv->event_lock, flag);
+ return -EPERM;
Why EPERM?
+ }
+
pndx = dp_priv->event_pndx + 1;
pndx %= DP_EVENT_Q_MAX;
if (pndx == dp_priv->event_gndx) {
@@ -454,6 +461,13 @@ static void dp_display_host_deinit(struct
dp_display_private *dp)
dp->dp_display.connector_type, dp->core_initialized,
dp->phy_initialized);
+ if (!dp->core_initialized) {
Can this happen?
not likely,
but It should not in this patch. will double check.
+ DRM_DEBUG_DP("DP core not initialized\n");
+ return;
+ }
+
+ synchronize_irq(dp->irq);
Why? If you need to run with IRQs disabled, you can use
suspend_late/early_resume. But generally it should be enough to check
for the !suspended in the IRQ.
+
dp_ctrl_reset_irq_ctrl(dp->ctrl, false);
dp_aux_deinit(dp->aux);
dp_power_deinit(dp->power);
@@ -1362,6 +1376,8 @@ static int dp_pm_resume(struct device *dev)
dp->dp_display.connector_type, dp->core_initialized,
dp->phy_initialized, dp_display->power_on);
+ atomic_set(&dp->suspended, 0);
+
/* start from disconnected state */
dp->hpd_state = ST_DISCONNECTED;
@@ -1431,6 +1447,8 @@ static int dp_pm_suspend(struct device *dev)
dp->dp_display.connector_type, dp->core_initialized,
dp->phy_initialized, dp_display->power_on);
+ atomic_inc(&dp->suspended);
+
/* mainlink enabled */
if (dp_power_clk_status(dp->power, DP_CTRL_PM))
dp_ctrl_off_link_stream(dp->ctrl);
