| Message ID | 1648656179-10347-4-git-send-email-quic_sbillaka@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for the eDP panel over aux_bus | expand |
Hi, On Wed, Mar 30, 2022 at 9:03 AM Sankeerth Billakanti <quic_sbillaka@quicinc.com> wrote: > > @@ -1374,6 +1382,12 @@ static int dp_pm_resume(struct device *dev) > dp_catalog_ctrl_hpd_config(dp->catalog); > > > + if (dp->dp_display.connector_type == DRM_MODE_CONNECTOR_DisplayPort) > + dp_catalog_hpd_config_intr(dp->catalog, > + DP_DP_HPD_PLUG_INT_MASK | > + DP_DP_HPD_UNPLUG_INT_MASK, > + true); > + nit: why are there two blank lines above? > @@ -1639,6 +1653,9 @@ void dp_bridge_enable(struct drm_bridge *drm_bridge) > return; > } > > + if (dp->connector_type == DRM_MODE_CONNECTOR_eDP) > + dp_hpd_plug_handle(dp_display, 0); > + Should you add a "pre_enable" and do it there? That would make it more symmetric with the fact that you have the countertpart in "post_disable". Overall: I'm probably not familiar enough with this code to give it a full review. I'm hoping that Dmitry knows it well enough... ;-) -Doug
Hi Doug, > On Wed, Mar 30, 2022 at 9:03 AM Sankeerth Billakanti > <quic_sbillaka@quicinc.com> wrote: > > > > @@ -1374,6 +1382,12 @@ static int dp_pm_resume(struct device *dev) > > dp_catalog_ctrl_hpd_config(dp->catalog); > > > > > > + if (dp->dp_display.connector_type == > DRM_MODE_CONNECTOR_DisplayPort) > > + dp_catalog_hpd_config_intr(dp->catalog, > > + DP_DP_HPD_PLUG_INT_MASK | > > + DP_DP_HPD_UNPLUG_INT_MASK, > > + true); > > + > > nit: why are there two blank lines above? > > Will remove a blank line. > > @@ -1639,6 +1653,9 @@ void dp_bridge_enable(struct drm_bridge > *drm_bridge) > > return; > > } > > > > + if (dp->connector_type == DRM_MODE_CONNECTOR_eDP) > > + dp_hpd_plug_handle(dp_display, 0); > > + > > Should you add a "pre_enable" and do it there? That would make it more > symmetric with the fact that you have the countertpart in "post_disable". > We want to handle the screen off or eDP disable like a cable unplug so that it can be integrated into the dp code. So, we can move plug_handle into pre_enable and the unplug_handle to pre_disable. If we do unplug in post_disable, we need to handle the state changes also. It will complicate the driver code. > > Overall: I'm probably not familiar enough with this code to give it a full > review. I'm hoping that Dmitry knows it well enough... ;-) > > > -Doug Thank you, Sankeerth
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c index b6add4e..3c16f95 100644 --- a/drivers/gpu/drm/msm/dp/dp_catalog.c +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c @@ -582,10 +582,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); diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index 274bbcf..888ff03 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -677,7 +677,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.connector_type == DRM_MODE_CONNECTOR_DisplayPort) + 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); @@ -1087,10 +1088,17 @@ void msm_dp_snapshot(struct msm_disp_state *disp_state, struct msm_dp *dp) 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.connector_type == DRM_MODE_CONNECTOR_DisplayPort) + 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 @@ -1374,6 +1382,12 @@ static int dp_pm_resume(struct device *dev) dp_catalog_ctrl_hpd_config(dp->catalog); + if (dp->dp_display.connector_type == DRM_MODE_CONNECTOR_DisplayPort) + 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 @@ -1639,6 +1653,9 @@ void dp_bridge_enable(struct drm_bridge *drm_bridge) return; } + if (dp->connector_type == DRM_MODE_CONNECTOR_eDP) + dp_hpd_plug_handle(dp_display, 0); + mutex_lock(&dp_display->event_mutex); /* stop sentinel checking */ @@ -1703,6 +1720,9 @@ void dp_bridge_post_disable(struct drm_bridge *drm_bridge) dp_display = container_of(dp, struct dp_display_private, dp_display); + if (dp->connector_type == DRM_MODE_CONNECTOR_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 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> --- drivers/gpu/drm/msm/dp/dp_catalog.c | 4 ---- drivers/gpu/drm/msm/dp/dp_display.c | 24 ++++++++++++++++++++++-- 2 files changed, 22 insertions(+), 6 deletions(-)