| Message ID | 20221026032624.30871-10-quic_bjorande@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | drm/msm: Add SC8280XP support | expand |
On 26 October 2022 06:26:21 EEST, Bjorn Andersson <quic_bjorande@quicinc.com> wrote: >From: Bjorn Andersson <bjorn.andersson@linaro.org> > >The DisplayPort controller's internal HPD interrupt handling is used for >cases where the HPD signal is connected to a GPIO which is pinmuxed into >the DisplayPort controller. > >Most of the logic for enabling and disabling the HPD-related interrupts >is conditioned on the presence of an EDP panel, but more generically >designs that has a downstream drm_bridge (next_bridge) could use this to >handle the HPD interrupts, instead of the internal mechanism. > >So replace the current is_edp-based guards with a check for the presence >of next_bridge. This does not sound correct. The next bridge might be a dummy bridge, not supporting the hpd. Please change this to use the enable_hpd()/disable_hpd() callbacks. This way the drm_bridge_connector framework will make sure to enable hpd handling for the bridge that is actually supposed to generate hpd events. > >Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> >Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com> >--- > >Changes since v2: >- None
On Wed, Oct 26, 2022 at 09:08:49AM +0300, Dmitry Baryshkov wrote: > > > On 26 October 2022 06:26:21 EEST, Bjorn Andersson <quic_bjorande@quicinc.com> wrote: > >From: Bjorn Andersson <bjorn.andersson@linaro.org> > > > >The DisplayPort controller's internal HPD interrupt handling is used for > >cases where the HPD signal is connected to a GPIO which is pinmuxed into > >the DisplayPort controller. > > > >Most of the logic for enabling and disabling the HPD-related interrupts > >is conditioned on the presence of an EDP panel, but more generically > >designs that has a downstream drm_bridge (next_bridge) could use this to > >handle the HPD interrupts, instead of the internal mechanism. > > > >So replace the current is_edp-based guards with a check for the presence > >of next_bridge. > > This does not sound correct. The next bridge might be a dummy bridge, > not supporting the hpd. I only considered checking for the Chrome case, where the output isn't modelled and we have to rely on the internal HPD logic. Checking that next_bridge is present and will deliver us hpd events sounds somewhat reasonable. But if I understand the code correctly, panel-edp isn't handing us hpd events - and we still don't want the internal HPD logic to trigger. So I presume I would need to check that this isn't a EDP controller and that we're going to get external HPD events? If so, clean you please give me some pointer on how to check if next_bridge will provide us with hpd signaling or not? PS. Which dummy bridge do you have in mind? > Please change this to use the enable_hpd()/disable_hpd() callbacks. > This way the drm_bridge_connector framework will make sure to enable > hpd handling for the bridge that is actually supposed to generate hpd > events. > The drm_bridge_connector_init() call in dp_drm_connector_init() does this for us already. Regards, Bjorn > > > > >Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > >Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com> > >--- > > > >Changes since v2: > >- None > > > -- > With best wishes > Dmitry
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index 3d365950de0f..224ae3aa07c4 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -610,7 +610,7 @@ static int dp_hpd_plug_handle(struct dp_display_private *dp, u32 data) } /* enable HDP irq_hpd/replug interrupt */ - if (!dp->dp_display.is_edp) + if (!dp->dp_display.next_bridge) dp_catalog_hpd_config_intr(dp->catalog, DP_DP_IRQ_HPD_INT_MASK | DP_DP_HPD_REPLUG_INT_MASK, true); @@ -653,7 +653,7 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data) dp->dp_display.connector_type, state); /* disable irq_hpd/replug interrupts */ - if (!dp->dp_display.is_edp) + if (!dp->dp_display.next_bridge) dp_catalog_hpd_config_intr(dp->catalog, DP_DP_IRQ_HPD_INT_MASK | DP_DP_HPD_REPLUG_INT_MASK, false); @@ -682,7 +682,7 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data) } /* disable HPD plug interrupts */ - if (!dp->dp_display.is_edp) + if (!dp->dp_display.next_bridge) dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, false); /* @@ -701,7 +701,7 @@ 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 */ - if (!dp->dp_display.is_edp) + if (!dp->dp_display.next_bridge) 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", @@ -1086,8 +1086,8 @@ 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) + /* Enable plug and unplug interrupts only if not handled by next_bridge */ + if (!dp->dp_display.next_bridge) dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK | DP_DP_HPD_UNPLUG_INT_MASK, @@ -1379,8 +1379,7 @@ static int dp_pm_resume(struct device *dev) dp_catalog_ctrl_hpd_config(dp->catalog); - - if (!dp->dp_display.is_edp) + if (!dp->dp_display.next_bridge) dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK | DP_DP_HPD_UNPLUG_INT_MASK,