| Message ID | 1620251521-29999-1-git-send-email-khsieh@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drm/msm/dp: handle irq_hpd with sink_count = 0 correctly | expand |
Quoting Kuogee Hsieh (2021-05-05 14:52:01) > irq_hpd interrupt should be handled after dongle plugged in and > before dongle unplugged. Hence irq_hpd interrupt is enabled at > the end of the plugin handle and disabled at the beginning of > unplugged handle. Current irq_hpd with sink_count = 0 is wrongly > handled same as the dongle unplugged which tears down the mainlink > and disables the phy. This patch fixes this problem by only tearing > down the mainlink but keeping phy enabled at irq_hpd with > sink_count = 0 handle so that next irq_hpe with sink_count =1 can be > handled by setup mainlink only. > > Signed-off-by: Kuogee Hsieh <khsieh@codeaurora.org> > --- It works for me, but I question the poking into phy internals below. > drivers/gpu/drm/msm/dp/dp_catalog.c | 5 +-- > drivers/gpu/drm/msm/dp/dp_ctrl.c | 78 +++++++++++++++++++++++++++++++++---- > drivers/gpu/drm/msm/dp/dp_ctrl.h | 2 + > drivers/gpu/drm/msm/dp/dp_display.c | 47 +++++++++++++++++----- > 4 files changed, 111 insertions(+), 21 deletions(-) > > diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c > index b1a9b1b..f4f53f2 100644 > --- a/drivers/gpu/drm/msm/dp/dp_catalog.c > +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c > @@ -582,10 +582,9 @@ void dp_catalog_ctrl_hpd_config(struct dp_catalog *dp_catalog) > > u32 reftimer = dp_read_aux(catalog, REG_DP_DP_HPD_REFTIMER); > > - /* enable HPD interrupts */ > + /* enable HPD plug and unplug interrupts */ > dp_catalog_hpd_config_intr(dp_catalog, > - DP_DP_HPD_PLUG_INT_MASK | DP_DP_IRQ_HPD_INT_MASK > - | DP_DP_HPD_UNPLUG_INT_MASK | DP_DP_HPD_REPLUG_INT_MASK, true); > + DP_DP_HPD_PLUG_INT_MASK | DP_DP_HPD_UNPLUG_INT_MASK, true); > > /* Configure REFTIMER and enable it */ > reftimer |= DP_DP_HPD_REFTIMER_ENABLE; > diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c > index 8d59eb9..5922259 100644 > --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c > +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c > @@ -1334,8 +1334,10 @@ static int dp_ctrl_enable_mainlink_clocks(struct dp_ctrl_private *ctrl) > dp_ctrl_set_clock_rate(ctrl, DP_CTRL_PM, "ctrl_link", > ctrl->link->link_params.rate * 1000); > > - phy_configure(phy, &dp_io->phy_opts); > - phy_power_on(phy); > + if (!phy->power_count) { > + phy_configure(phy, &dp_io->phy_opts); > + phy_power_on(phy); > + } > > ret = dp_power_clk_enable(ctrl->power, DP_CTRL_PM, true); > if (ret) > @@ -1414,6 +1416,10 @@ void dp_ctrl_host_deinit(struct dp_ctrl *dp_ctrl) > phy = dp_io->phy; > > dp_catalog_ctrl_enable_irq(ctrl->catalog, false); > + > + if (phy->power_count) > + phy_power_off(phy); > + > phy_exit(phy); > > DRM_DEBUG_DP("Host deinitialized successfully\n"); > @@ -1445,7 +1451,6 @@ static int dp_ctrl_reinitialize_mainlink(struct dp_ctrl_private *ctrl) > > dp_catalog_ctrl_mainlink_ctrl(ctrl->catalog, false); > opts_dp->lanes = ctrl->link->link_params.num_lanes; > - phy_configure(phy, &dp_io->phy_opts); > /* > * Disable and re-enable the mainlink clock since the > * link clock might have been adjusted as part of the > @@ -1456,9 +1461,13 @@ static int dp_ctrl_reinitialize_mainlink(struct dp_ctrl_private *ctrl) > DRM_ERROR("Failed to disable clocks. ret=%d\n", ret); > return ret; > } > - phy_power_off(phy); > - /* hw recommended delay before re-enabling clocks */ > - msleep(20); > + > + if (phy->power_count) { I don't believe members of 'phy' are supposed to be looked at by various phy consumer drivers. Vinod, is that right? > + phy_configure(phy, &dp_io->phy_opts); > + phy_power_off(phy); > + /* hw recommended delay before re-enabling clocks */ > + msleep(20); > + } > > ret = dp_ctrl_enable_mainlink_clocks(ctrl); > if (ret) { > @@ -1487,7 +1496,9 @@ static int dp_ctrl_deinitialize_mainlink(struct dp_ctrl_private *ctrl) > DRM_ERROR("Failed to disable link clocks. ret=%d\n", ret); > } > > - phy_power_off(phy); > + if (phy->power_count) > + phy_power_off(phy); > + Given that init/power_on are both refcounted, maybe the code could call phy_init()/phy_exit() when hpd changes, i.e. the link is up and phy_power_on()/phy_power_off() when the mainlink is supposed to be enabled (I guess when we're going to display bit to the screen?). That way we don't have to check the counts. > phy_exit(phy); > > return 0;
On 10-05-21, 11:15, Stephen Boyd wrote: > Quoting Kuogee Hsieh (2021-05-05 14:52:01) > > @@ -1414,6 +1416,10 @@ void dp_ctrl_host_deinit(struct dp_ctrl *dp_ctrl) > > phy = dp_io->phy; > > > > dp_catalog_ctrl_enable_irq(ctrl->catalog, false); > > + > > + if (phy->power_count) > > + phy_power_off(phy); > > + > > phy_exit(phy); > > > > DRM_DEBUG_DP("Host deinitialized successfully\n"); > > @@ -1445,7 +1451,6 @@ static int dp_ctrl_reinitialize_mainlink(struct dp_ctrl_private *ctrl) > > > > dp_catalog_ctrl_mainlink_ctrl(ctrl->catalog, false); > > opts_dp->lanes = ctrl->link->link_params.num_lanes; > > - phy_configure(phy, &dp_io->phy_opts); > > /* > > * Disable and re-enable the mainlink clock since the > > * link clock might have been adjusted as part of the > > @@ -1456,9 +1461,13 @@ static int dp_ctrl_reinitialize_mainlink(struct dp_ctrl_private *ctrl) > > DRM_ERROR("Failed to disable clocks. ret=%d\n", ret); > > return ret; > > } > > - phy_power_off(phy); > > - /* hw recommended delay before re-enabling clocks */ > > - msleep(20); > > + > > + if (phy->power_count) { > > I don't believe members of 'phy' are supposed to be looked at by various > phy consumer drivers. Vinod, is that right? That is correct, we should not be doing that. And IMO this code is redundant, the phy core will check power_count and invoke drivers .power_off accordingly, so should be removed... Thanks
On 2021-05-11 22:27, Vinod Koul wrote: > On 10-05-21, 11:15, Stephen Boyd wrote: >> Quoting Kuogee Hsieh (2021-05-05 14:52:01) >> > @@ -1414,6 +1416,10 @@ void dp_ctrl_host_deinit(struct dp_ctrl *dp_ctrl) >> > phy = dp_io->phy; >> > >> > dp_catalog_ctrl_enable_irq(ctrl->catalog, false); >> > + >> > + if (phy->power_count) >> > + phy_power_off(phy); >> > + >> > phy_exit(phy); >> > >> > DRM_DEBUG_DP("Host deinitialized successfully\n"); >> > @@ -1445,7 +1451,6 @@ static int dp_ctrl_reinitialize_mainlink(struct dp_ctrl_private *ctrl) >> > >> > dp_catalog_ctrl_mainlink_ctrl(ctrl->catalog, false); >> > opts_dp->lanes = ctrl->link->link_params.num_lanes; >> > - phy_configure(phy, &dp_io->phy_opts); >> > /* >> > * Disable and re-enable the mainlink clock since the >> > * link clock might have been adjusted as part of the >> > @@ -1456,9 +1461,13 @@ static int dp_ctrl_reinitialize_mainlink(struct dp_ctrl_private *ctrl) >> > DRM_ERROR("Failed to disable clocks. ret=%d\n", ret); >> > return ret; >> > } >> > - phy_power_off(phy); >> > - /* hw recommended delay before re-enabling clocks */ >> > - msleep(20); >> > + >> > + if (phy->power_count) { >> >> I don't believe members of 'phy' are supposed to be looked at by >> various >> phy consumer drivers. Vinod, is that right? > > That is correct, we should not be doing that. And IMO this code is > redundant, the phy core will check power_count and invoke drivers > .power_off accordingly, so should be removed... > > Thanks ok, v2 patch uploaded to address this issue.
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c index b1a9b1b..f4f53f2 100644 --- a/drivers/gpu/drm/msm/dp/dp_catalog.c +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c @@ -582,10 +582,9 @@ void dp_catalog_ctrl_hpd_config(struct dp_catalog *dp_catalog) u32 reftimer = dp_read_aux(catalog, REG_DP_DP_HPD_REFTIMER); - /* enable HPD interrupts */ + /* enable HPD plug and unplug interrupts */ dp_catalog_hpd_config_intr(dp_catalog, - DP_DP_HPD_PLUG_INT_MASK | DP_DP_IRQ_HPD_INT_MASK - | DP_DP_HPD_UNPLUG_INT_MASK | DP_DP_HPD_REPLUG_INT_MASK, true); + DP_DP_HPD_PLUG_INT_MASK | DP_DP_HPD_UNPLUG_INT_MASK, true); /* Configure REFTIMER and enable it */ reftimer |= DP_DP_HPD_REFTIMER_ENABLE; diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c index 8d59eb9..5922259 100644 --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c @@ -1334,8 +1334,10 @@ static int dp_ctrl_enable_mainlink_clocks(struct dp_ctrl_private *ctrl) dp_ctrl_set_clock_rate(ctrl, DP_CTRL_PM, "ctrl_link", ctrl->link->link_params.rate * 1000); - phy_configure(phy, &dp_io->phy_opts); - phy_power_on(phy); + if (!phy->power_count) { + phy_configure(phy, &dp_io->phy_opts); + phy_power_on(phy); + } ret = dp_power_clk_enable(ctrl->power, DP_CTRL_PM, true); if (ret) @@ -1414,6 +1416,10 @@ void dp_ctrl_host_deinit(struct dp_ctrl *dp_ctrl) phy = dp_io->phy; dp_catalog_ctrl_enable_irq(ctrl->catalog, false); + + if (phy->power_count) + phy_power_off(phy); + phy_exit(phy); DRM_DEBUG_DP("Host deinitialized successfully\n"); @@ -1445,7 +1451,6 @@ static int dp_ctrl_reinitialize_mainlink(struct dp_ctrl_private *ctrl) dp_catalog_ctrl_mainlink_ctrl(ctrl->catalog, false); opts_dp->lanes = ctrl->link->link_params.num_lanes; - phy_configure(phy, &dp_io->phy_opts); /* * Disable and re-enable the mainlink clock since the * link clock might have been adjusted as part of the @@ -1456,9 +1461,13 @@ static int dp_ctrl_reinitialize_mainlink(struct dp_ctrl_private *ctrl) DRM_ERROR("Failed to disable clocks. ret=%d\n", ret); return ret; } - phy_power_off(phy); - /* hw recommended delay before re-enabling clocks */ - msleep(20); + + if (phy->power_count) { + phy_configure(phy, &dp_io->phy_opts); + phy_power_off(phy); + /* hw recommended delay before re-enabling clocks */ + msleep(20); + } ret = dp_ctrl_enable_mainlink_clocks(ctrl); if (ret) { @@ -1487,7 +1496,9 @@ static int dp_ctrl_deinitialize_mainlink(struct dp_ctrl_private *ctrl) DRM_ERROR("Failed to disable link clocks. ret=%d\n", ret); } - phy_power_off(phy); + if (phy->power_count) + phy_power_off(phy); + phy_exit(phy); return 0; @@ -1811,6 +1822,55 @@ int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl) return ret; } +int dp_ctrl_off_link_stream(struct dp_ctrl *dp_ctrl) +{ + struct dp_ctrl_private *ctrl; + int ret = 0; + + if (!dp_ctrl) + return -EINVAL; + + ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl); + + dp_catalog_ctrl_mainlink_ctrl(ctrl->catalog, false); + + ret = dp_power_clk_enable(ctrl->power, DP_STREAM_PM, false); + if (ret) + DRM_ERROR("Failed to disable pixel clocks. ret=%d\n", ret); + + ret = dp_power_clk_enable(ctrl->power, DP_CTRL_PM, false); + if (ret) + DRM_ERROR("Failed to disable link clocks. ret=%d\n", ret); + + DRM_DEBUG_DP("DP off link/stream done\n"); + return ret; +} + +int dp_ctrl_off_phy(struct dp_ctrl *dp_ctrl) +{ + struct dp_ctrl_private *ctrl; + struct dp_io *dp_io; + struct phy *phy; + int ret = 0; + + if (!dp_ctrl) + return -EINVAL; + + 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); + + if (phy->power_count) + phy_power_off(phy); + + phy_exit(phy); + + DRM_DEBUG_DP("DP off phy done\n"); + return ret; +} + int dp_ctrl_off(struct dp_ctrl *dp_ctrl) { struct dp_ctrl_private *ctrl; @@ -1838,7 +1898,9 @@ int dp_ctrl_off(struct dp_ctrl *dp_ctrl) DRM_ERROR("Failed to disable link clocks. ret=%d\n", ret); } - phy_power_off(phy); + if (phy->power_count) + phy_power_off(phy); + phy_exit(phy); DRM_DEBUG_DP("DP off done\n"); diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.h b/drivers/gpu/drm/msm/dp/dp_ctrl.h index a836bd3..1c0901d 100644 --- a/drivers/gpu/drm/msm/dp/dp_ctrl.h +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.h @@ -23,6 +23,8 @@ 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); +int 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); diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index 0ba71c7..e372789 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -570,6 +570,10 @@ static int dp_hpd_plug_handle(struct dp_display_private *dp, u32 data) dp_add_event(dp, EV_CONNECT_PENDING_TIMEOUT, 0, tout); } + /* enable HDP irq_hpd/replug interrupt */ + dp_catalog_hpd_config_intr(dp->catalog, DP_DP_IRQ_HPD_INT_MASK + | DP_DP_HPD_REPLUG_INT_MASK, true); + mutex_unlock(&dp->event_mutex); /* uevent will complete connection part */ @@ -619,7 +623,26 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data) mutex_lock(&dp->event_mutex); state = dp->hpd_state; - if (state == ST_DISCONNECT_PENDING || state == ST_DISCONNECTED) { + + /* disable irq_hpd/replug interrupts */ + dp_catalog_hpd_config_intr(dp->catalog, DP_DP_IRQ_HPD_INT_MASK + | DP_DP_HPD_REPLUG_INT_MASK, false); + + /* unplugged, no more irq_hpd handle */ + dp_del_event(dp, EV_IRQ_HPD_INT); + + if (state == ST_DISCONNECTED) { + /* triggered by irq_hdp with sink_count = 0 */ + if (dp->link->sink_count == 0) { + dp_ctrl_off_phy(dp->ctrl); + hpd->hpd_high = 0; + dp->core_initialized = false; + } + mutex_unlock(&dp->event_mutex); + return 0; + } + + if (state == ST_DISCONNECT_PENDING) { mutex_unlock(&dp->event_mutex); return 0; } @@ -633,9 +656,8 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data) dp->hpd_state = ST_DISCONNECT_PENDING; - /* disable HPD plug interrupt until disconnect is done */ - dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK - | DP_DP_IRQ_HPD_INT_MASK, false); + /* disable HPD plug interrupts */ + dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, false); hpd->hpd_high = 0; @@ -652,8 +674,8 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data) reinit_completion(&dp->audio_comp); dp_display_handle_plugged_change(g_dp_display, false); - dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK | - DP_DP_IRQ_HPD_INT_MASK, true); + /* enable HDP plug interrupt to prepare for next plugin */ + dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, true); /* uevent will complete disconnection part */ mutex_unlock(&dp->event_mutex); @@ -684,7 +706,7 @@ static int dp_irq_hpd_handle(struct dp_display_private *dp, u32 data) /* irq_hpd can happen at either connected or disconnected state */ state = dp->hpd_state; - if (state == ST_DISPLAY_OFF) { + if (state == ST_DISPLAY_OFF || state == ST_SUSPENDED) { mutex_unlock(&dp->event_mutex); return 0; } @@ -903,9 +925,13 @@ static int dp_display_disable(struct dp_display_private *dp, u32 data) dp_display->audio_enabled = false; - dp_ctrl_off(dp->ctrl); - - dp->core_initialized = false; + /* triggered by irq_hpd with sink_coutn = 0 */ + if (dp->link->sink_count == 0) { + dp_ctrl_off_link_stream(dp->ctrl); + } else { + dp_ctrl_off(dp->ctrl); + dp->core_initialized = false; + } dp_display->power_on = false; @@ -1512,6 +1538,7 @@ int msm_dp_display_disable(struct msm_dp *dp, struct drm_encoder *encoder) } mutex_unlock(&dp_display->event_mutex); + return rc; }
irq_hpd interrupt should be handled after dongle plugged in and before dongle unplugged. Hence irq_hpd interrupt is enabled at the end of the plugin handle and disabled at the beginning of unplugged handle. Current irq_hpd with sink_count = 0 is wrongly handled same as the dongle unplugged which tears down the mainlink and disables the phy. This patch fixes this problem by only tearing down the mainlink but keeping phy enabled at irq_hpd with sink_count = 0 handle so that next irq_hpe with sink_count =1 can be handled by setup mainlink only. Signed-off-by: Kuogee Hsieh <khsieh@codeaurora.org> --- drivers/gpu/drm/msm/dp/dp_catalog.c | 5 +-- drivers/gpu/drm/msm/dp/dp_ctrl.c | 78 +++++++++++++++++++++++++++++++++---- drivers/gpu/drm/msm/dp/dp_ctrl.h | 2 + drivers/gpu/drm/msm/dp/dp_display.c | 47 +++++++++++++++++----- 4 files changed, 111 insertions(+), 21 deletions(-)