| Message ID | 1650618666-15342-3-git-send-email-quic_sbillaka@quicinc.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for the eDP panel over aux_bus | expand |
Hi Stephen, >Quoting Sankeerth Billakanti (2022-04-22 02:11:04) >> The panel-edp enables the eDP panel power during probe, get_modes and >> pre-enable. The eDP connect and disconnect interrupts for the eDP/DP >> controller are directly dependent on panel power. As eDP display can >> be assumed as always connected, the controller driver can skip the eDP >> connect and disconnect interrupts. Any disruption in the link status >> will be indicated via the IRQ_HPD interrupts. >> >> So, the eDP controller driver can just enable the IRQ_HPD and replug >> interrupts. The DP controller driver still needs to enable all the >> interrupts. >> >> Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com> > >The sprinkling of if conditions and manual driving of the DP plug/unplug state >machine is pretty convoluted. To make it better the driver needs an overhaul. >Anyway, it looks mostly fine to me except for this replug interrupt question >below. Otherwise > >Reviewed-by: Stephen Boyd <swboyd@chromium.org> > >> int dp_catalog_ctrl_get_interrupt(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 055681a..dea4de9 100644 >> --- a/drivers/gpu/drm/msm/dp/dp_display.c >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c >> @@ -1096,6 +1097,13 @@ 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 for external DisplayPort */ >> + if (!dp->dp_display.is_edp) >> + dp_catalog_hpd_config_intr(dp->catalog, >> + DP_DP_HPD_PLUG_INT_MASK | >> + DP_DP_HPD_UNPLUG_INT_MASK, >> + true); >> + > >It seems like only the plug and unplug is enabled for DP here. Is replug >enabled for eDP when it shouldn't be? > By default, all the interrupts are masked. This function is not executed for eDP anymore because the host_init, phy_init and enable_irq are all done from modeset_init for eDP with aux_bus. So, none of the eDP hpd interrupts are enabled or unmasked before pre-enable. The replug interrupt is unmasked for DP and eDP from the dp_hpd_plug_handle() and masked from dp_hpd_unplug_handle(). >> /* Enable interrupt first time >> * we are leaving dp clocks on during disconnect >> * and never disable interrupt
Quoting Sankeerth Billakanti (QUIC) (2022-04-24 19:55:29) > >Quoting Sankeerth Billakanti (2022-04-22 02:11:04) > > > >> int dp_catalog_ctrl_get_interrupt(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 055681a..dea4de9 100644 > >> --- a/drivers/gpu/drm/msm/dp/dp_display.c > >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c > >> @@ -1096,6 +1097,13 @@ 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 for external DisplayPort */ > >> + if (!dp->dp_display.is_edp) > >> + dp_catalog_hpd_config_intr(dp->catalog, > >> + DP_DP_HPD_PLUG_INT_MASK | > >> + DP_DP_HPD_UNPLUG_INT_MASK, > >> + true); > >> + > > > >It seems like only the plug and unplug is enabled for DP here. Is replug > >enabled for eDP when it shouldn't be? > > > > By default, all the interrupts are masked. This function is not executed for eDP > anymore because the host_init, phy_init and enable_irq are all done from > modeset_init for eDP with aux_bus. So, none of the eDP hpd interrupts are > enabled or unmasked before pre-enable. > > The replug interrupt is unmasked for DP and eDP from the dp_hpd_plug_handle() > and masked from dp_hpd_unplug_handle(). Why is replug enabled for eDP?
Hi Stephen, >> >> int dp_catalog_ctrl_get_interrupt(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 055681a..dea4de9 100644 >> >> --- a/drivers/gpu/drm/msm/dp/dp_display.c >> >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c >> >> @@ -1096,6 +1097,13 @@ 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 for external DisplayPort */ >> >> + if (!dp->dp_display.is_edp) >> >> + dp_catalog_hpd_config_intr(dp->catalog, >> >> + DP_DP_HPD_PLUG_INT_MASK | >> >> + DP_DP_HPD_UNPLUG_INT_MASK, >> >> + true); >> >> + >> > >> >It seems like only the plug and unplug is enabled for DP here. Is >> >replug enabled for eDP when it shouldn't be? >> > >> >> By default, all the interrupts are masked. This function is not >> executed for eDP anymore because the host_init, phy_init and >> enable_irq are all done from modeset_init for eDP with aux_bus. So, >> none of the eDP hpd interrupts are enabled or unmasked before pre- >enable. >> >> The replug interrupt is unmasked for DP and eDP from the >> dp_hpd_plug_handle() and masked from dp_hpd_unplug_handle(). > >Why is replug enabled for eDP? As the eDP panel is assumed to be always connected, just enabling the IRQ_HPD is sufficient. The REPLUG is enabled or unmasked along with IRQ_HPD in code. I did not remove the REPLUG event support for eDP so that we have an option to see if the eDP panel can undergo a short disconnect/connect cycle after pre-enable (while the panel power is enabled). REPLUG can be generated for eDP if, a) the panel power turns off and on OR b) the sink itself issues a fast disconnect-connect. REPLUG event initiated by sink is rare and we observed it only during the DP compliance test. Some more information on HPD events generated by the source: Replug interrupt is something our controller HW supports and not part of the DP/eDP specification. The programmed values for HPD on the HW controller indicates the following: 1. The HOTPLUG interrupt will be generated if the HPD line is continuously high for 100ms. 2. Similarly, UNPLUG interrupt will be generated when the HPD line transitions from high to low and remains low for 100ms. 3. IRQ_HPD will be generated when the HPD line transitions from high to low and remains low for less than 2ms. 4. REPLUG will be generated if the HPD line remains low for more than 2ms but less than 100ms. According to the DP spec, replug event should be considered as a disconnect and then connect. To answer your question, I did not remove REPLUG support for eDP because I felt it will not affect the eDP normal functioning in anyway. Thank you, Sankeerth
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c index fac815f..df9670d 100644 --- a/drivers/gpu/drm/msm/dp/dp_catalog.c +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c @@ -569,10 +569,6 @@ void dp_catalog_ctrl_hpd_config(struct dp_catalog *dp_catalog) u32 reftimer = dp_read_aux(catalog, REG_DP_DP_HPD_REFTIMER); - /* enable HPD plug and unplug interrupts */ - dp_catalog_hpd_config_intr(dp_catalog, - DP_DP_HPD_PLUG_INT_MASK | DP_DP_HPD_UNPLUG_INT_MASK, true); - /* Configure REFTIMER and enable it */ reftimer |= DP_DP_HPD_REFTIMER_ENABLE; dp_write_aux(catalog, REG_DP_DP_HPD_REFTIMER, reftimer); @@ -599,13 +595,21 @@ u32 dp_catalog_hpd_get_intr_status(struct dp_catalog *dp_catalog) { struct dp_catalog_private *catalog = container_of(dp_catalog, struct dp_catalog_private, dp_catalog); - int isr = 0; + int isr, mask; isr = dp_read_aux(catalog, REG_DP_DP_HPD_INT_STATUS); dp_write_aux(catalog, REG_DP_DP_HPD_INT_ACK, (isr & DP_DP_HPD_INT_MASK)); + mask = dp_read_aux(catalog, REG_DP_DP_HPD_INT_MASK); - return isr; + /* + * We only want to return interrupts that are unmasked to the caller. + * However, the interrupt status field also contains other + * informational bits about the HPD state status, so we only mask + * out the part of the register that tells us about which interrupts + * are pending. + */ + return isr & (mask | ~DP_DP_HPD_INT_MASK); } int dp_catalog_ctrl_get_interrupt(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 055681a..dea4de9 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -683,7 +683,8 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data) dp_display_handle_plugged_change(&dp->dp_display, false); /* enable HDP plug interrupt to prepare for next plugin */ - dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, true); + if (!dp->dp_display.is_edp) + dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, true); DRM_DEBUG_DP("After, type=%d hpd_state=%d\n", dp->dp_display.connector_type, state); @@ -1096,6 +1097,13 @@ 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 for external DisplayPort */ + if (!dp->dp_display.is_edp) + 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 @@ -1381,6 +1389,12 @@ static int dp_pm_resume(struct device *dev) dp_catalog_ctrl_hpd_config(dp->catalog); + if (!dp->dp_display.is_edp) + dp_catalog_hpd_config_intr(dp->catalog, + DP_DP_HPD_PLUG_INT_MASK | + DP_DP_HPD_UNPLUG_INT_MASK, + true); + if (dp_catalog_link_is_connected(dp->catalog)) { /* * set sink to normal operation mode -- D0 @@ -1659,6 +1673,9 @@ void dp_bridge_enable(struct drm_bridge *drm_bridge) return; } + if (dp->is_edp) + dp_hpd_plug_handle(dp_display, 0); + mutex_lock(&dp_display->event_mutex); /* stop sentinel checking */ @@ -1723,6 +1740,9 @@ void dp_bridge_post_disable(struct drm_bridge *drm_bridge) dp_display = container_of(dp, struct dp_display_private, dp_display); + if (dp->is_edp) + dp_hpd_unplug_handle(dp_display, 0); + mutex_lock(&dp_display->event_mutex); /* stop sentinel checking */
The panel-edp enables the eDP panel power during probe, get_modes and pre-enable. The eDP connect and disconnect interrupts for the eDP/DP controller are directly dependent on panel power. As eDP display can be assumed as always connected, the controller driver can skip the eDP connect and disconnect interrupts. Any disruption in the link status will be indicated via the IRQ_HPD interrupts. So, the eDP controller driver can just enable the IRQ_HPD and replug interrupts. The DP controller driver still needs to enable all the interrupts. Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com> --- Changes in v9: - add comment explaining the interrupt status register Changes in v8: - add comment explaining the interrupt status return Changes in v7: - reordered the patch in the series - modified the return statement for isr - connector check modified to just check for eDP drivers/gpu/drm/msm/dp/dp_catalog.c | 16 ++++++++++------ drivers/gpu/drm/msm/dp/dp_display.c | 22 +++++++++++++++++++++- 2 files changed, 31 insertions(+), 7 deletions(-)