Messages in this thread | | | Date | Tue, 23 May 2023 01:03:26 +0300 | Subject | Re: [PATCH v3] drm/msm/dp: enable HDP plugin/unplugged interrupts at hpd_enable/disable | From | Dmitry Baryshkov <> |
| |
On 23/05/2023 01:05, Bjorn Andersson wrote: > On Mon, May 22, 2023 at 02:31:32PM -0700, Kuogee Hsieh wrote: >> The internal_hpd flag is set to true by dp_bridge_hpd_enable() and set to >> false by dp_bridge_hpd_disable() to handle GPIO pinmuxed into DP controller >> case. HDP related interrupts can not be enabled until internal_hpd is set >> to true. At current implementation dp_display_config_hpd() will initialize >> DP host controller first followed by enabling HDP related interrupts if >> internal_hpd was true at that time. Enable HDP related interrupts depends on >> internal_hpd status may leave system with DP driver host is in running state >> but without HDP related interrupts being enabled. This will prevent external >> display from being detected. Eliminated this dependency by moving HDP related >> interrupts enable/disable be done at dp_bridge_hpd_enable/disable() directly >> regardless of internal_hpd status. >> > > Thanks Kuogee, this looks quite good to me. But... > >> Changes in V3: >> -- dp_catalog_ctrl_hpd_enable() and dp_catalog_ctrl_hpd_disable() >> -- rewording ocmmit text >> >> Fixes: cd198caddea7 ("drm/msm/dp: Rely on hpd_enable/disable callbacks") >> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> >> --- >> drivers/gpu/drm/msm/dp/dp_catalog.c | 15 +++++++- >> drivers/gpu/drm/msm/dp/dp_catalog.h | 3 +- >> drivers/gpu/drm/msm/dp/dp_display.c | 70 +++++++++++-------------------------- >> 3 files changed, 37 insertions(+), 51 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c >> index 7a8cf1c..5142aeb 100644 >> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c >> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c >> @@ -620,7 +620,7 @@ void dp_catalog_hpd_config_intr(struct dp_catalog *dp_catalog, >> config & DP_DP_HPD_INT_MASK); >> } >> >> -void dp_catalog_ctrl_hpd_config(struct dp_catalog *dp_catalog) >> +void dp_catalog_ctrl_hpd_enable(struct dp_catalog *dp_catalog) >> { >> struct dp_catalog_private *catalog = container_of(dp_catalog, >> struct dp_catalog_private, dp_catalog); >> @@ -635,6 +635,19 @@ void dp_catalog_ctrl_hpd_config(struct dp_catalog *dp_catalog) >> dp_write_aux(catalog, REG_DP_DP_HPD_CTRL, DP_DP_HPD_CTRL_HPD_EN); >> } >> >> +void dp_catalog_ctrl_hpd_disable(struct dp_catalog *dp_catalog) >> +{ >> + struct dp_catalog_private *catalog = container_of(dp_catalog, >> + struct dp_catalog_private, dp_catalog); >> + >> + u32 reftimer = dp_read_aux(catalog, REG_DP_DP_HPD_REFTIMER); >> + >> + reftimer &= ~DP_DP_HPD_REFTIMER_ENABLE; >> + dp_write_aux(catalog, REG_DP_DP_HPD_REFTIMER, reftimer); >> + >> + dp_write_aux(catalog, REG_DP_DP_HPD_CTRL, 0); >> +} >> + >> static void dp_catalog_enable_sdp(struct dp_catalog_private *catalog) >> { >> /* trigger sdp */ >> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h b/drivers/gpu/drm/msm/dp/dp_catalog.h >> index 82376a2..38786e8 100644 >> --- a/drivers/gpu/drm/msm/dp/dp_catalog.h >> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.h >> @@ -104,7 +104,8 @@ bool dp_catalog_ctrl_mainlink_ready(struct dp_catalog *dp_catalog); >> void dp_catalog_ctrl_enable_irq(struct dp_catalog *dp_catalog, bool enable); >> void dp_catalog_hpd_config_intr(struct dp_catalog *dp_catalog, >> u32 intr_mask, bool en); >> -void dp_catalog_ctrl_hpd_config(struct dp_catalog *dp_catalog); >> +void dp_catalog_ctrl_hpd_enable(struct dp_catalog *dp_catalog); >> +void dp_catalog_ctrl_hpd_disable(struct dp_catalog *dp_catalog); >> void dp_catalog_ctrl_config_psr(struct dp_catalog *dp_catalog); >> void dp_catalog_ctrl_set_psr(struct dp_catalog *dp_catalog, bool enter); >> u32 dp_catalog_link_is_connected(struct dp_catalog *dp_catalog); >> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c >> index 3e13acdf..69bbc5f 100644 >> --- a/drivers/gpu/drm/msm/dp/dp_display.c >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c >> @@ -615,12 +615,6 @@ static int dp_hpd_plug_handle(struct dp_display_private *dp, u32 data) >> dp->hpd_state = ST_MAINLINK_READY; >> } >> >> - /* enable HDP irq_hpd/replug interrupt */ >> - if (dp->dp_display.internal_hpd) >> - dp_catalog_hpd_config_intr(dp->catalog, >> - DP_DP_IRQ_HPD_INT_MASK | DP_DP_HPD_REPLUG_INT_MASK, >> - true); >> - >> drm_dbg_dp(dp->drm_dev, "After, type=%d hpd_state=%d\n", >> dp->dp_display.connector_type, state); >> mutex_unlock(&dp->event_mutex); >> @@ -658,12 +652,6 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data) >> drm_dbg_dp(dp->drm_dev, "Before, type=%d hpd_state=%d\n", >> dp->dp_display.connector_type, state); >> >> - /* disable irq_hpd/replug interrupts */ >> - if (dp->dp_display.internal_hpd) >> - 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); >> >> @@ -687,10 +675,6 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data) >> return 0; >> } >> >> - /* disable HPD plug interrupts */ >> - if (dp->dp_display.internal_hpd) >> - dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, false); >> - >> /* >> * We don't need separate work for disconnect as >> * connect/attention interrupts are disabled >> @@ -706,10 +690,6 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data) >> /* signal the disconnect event early to ensure proper teardown */ >> dp_display_handle_plugged_change(&dp->dp_display, false); >> >> - /* enable HDP plug interrupt to prepare for next plugin */ >> - if (dp->dp_display.internal_hpd) >> - dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, true); >> - >> drm_dbg_dp(dp->drm_dev, "After, type=%d hpd_state=%d\n", >> dp->dp_display.connector_type, state); >> >> @@ -1082,26 +1062,6 @@ void msm_dp_snapshot(struct msm_disp_state *disp_state, struct msm_dp *dp) >> mutex_unlock(&dp_display->event_mutex); >> } >> >> -static void dp_display_config_hpd(struct dp_display_private *dp) >> -{ >> - >> - dp_display_host_init(dp); >> - dp_catalog_ctrl_hpd_config(dp->catalog); >> - >> - /* Enable plug and unplug interrupts only if requested */ >> - if (dp->dp_display.internal_hpd) >> - dp_catalog_hpd_config_intr(dp->catalog, >> - DP_DP_HPD_PLUG_INT_MASK | >> - DP_DP_HPD_UNPLUG_INT_MASK, >> - true); >> - >> - /* Enable interrupt first time >> - * we are leaving dp clocks on during disconnect >> - * and never disable interrupt >> - */ >> - enable_irq(dp->irq); > > ...we need dp->irq enabled for handling the other interrupts, otherwise > e.g. AUX transfers will time out. > > I added enable_irq(dp_priv->irq) to the EV_HPD_INIT_SETUP case below, > just for testing, and with that the patch seems to be working fine. > > > Is there any reason why we need to delay its enablement to after we > unmask the HPD interrupts?
As I wrote, I'd probably prefer to see enable_irq() and disable_irq() calls gone.
> > Regards, > Bjorn > >> -} >> - >> void dp_display_set_psr(struct msm_dp *dp_display, bool enter) >> { >> struct dp_display_private *dp; >> @@ -1176,7 +1136,7 @@ static int hpd_event_thread(void *data) >> >> switch (todo->event_id) { >> case EV_HPD_INIT_SETUP: >> - dp_display_config_hpd(dp_priv); >> + dp_display_host_init(dp_priv); >> break; >> case EV_HPD_PLUG_INT: >> dp_hpd_plug_handle(dp_priv, todo->data); >> @@ -1394,13 +1354,8 @@ static int dp_pm_resume(struct device *dev) >> /* turn on dp ctrl/phy */ >> dp_display_host_init(dp); >> >> - dp_catalog_ctrl_hpd_config(dp->catalog); >> - >> - if (dp->dp_display.internal_hpd) >> - dp_catalog_hpd_config_intr(dp->catalog, >> - DP_DP_HPD_PLUG_INT_MASK | >> - DP_DP_HPD_UNPLUG_INT_MASK, >> - true); >> + if (dp_display->is_edp) >> + dp_catalog_ctrl_hpd_enable(dp->catalog); >> >> if (dp_catalog_link_is_connected(dp->catalog)) { >> /* >> @@ -1568,7 +1523,7 @@ static int dp_display_get_next_bridge(struct msm_dp *dp) >> >> if (aux_bus && dp->is_edp) { >> dp_display_host_init(dp_priv); >> - dp_catalog_ctrl_hpd_config(dp_priv->catalog); >> + dp_catalog_ctrl_hpd_enable(dp_priv->catalog); >> dp_display_host_phy_init(dp_priv); >> enable_irq(dp_priv->irq); >> >> @@ -1801,16 +1756,33 @@ void dp_bridge_hpd_enable(struct drm_bridge *bridge) >> { >> struct msm_dp_bridge *dp_bridge = to_dp_bridge(bridge); >> struct msm_dp *dp_display = dp_bridge->dp_display; >> + struct dp_display_private *dp = container_of(dp_display, struct dp_display_private, dp_display); >> + >> + mutex_lock(&dp->event_mutex); >> + dp_catalog_ctrl_hpd_enable(dp->catalog); >> + >> + /* enable HDP interrupts */ >> + dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_INT_MASK, true); >> + >> + enable_irq(dp->irq); >> >> dp_display->internal_hpd = true; >> + mutex_unlock(&dp->event_mutex); >> } >> >> void dp_bridge_hpd_disable(struct drm_bridge *bridge) >> { >> struct msm_dp_bridge *dp_bridge = to_dp_bridge(bridge); >> struct msm_dp *dp_display = dp_bridge->dp_display; >> + struct dp_display_private *dp = container_of(dp_display, struct dp_display_private, dp_display); >> + >> + mutex_lock(&dp->event_mutex); >> + /* disable HDP interrupts */ >> + dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_INT_MASK, false); >> + dp_catalog_ctrl_hpd_disable(dp->catalog); >> >> dp_display->internal_hpd = false; >> + mutex_unlock(&dp->event_mutex); >> } >> >> void dp_bridge_hpd_notify(struct drm_bridge *bridge, >> -- >> 2.7.4 >>
-- With best wishes Dmitry
| |