[v3,09/12] drm/msm/dp: HPD handling relates to next_bridge
Commit Message
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.
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
---
Changes since v2:
- None
drivers/gpu/drm/msm/dp/dp_display.c | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)
Comments
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
@@ -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,