Re: [PATCH v4 10/13] drm/msm/dp: Rely on hpd_enable/disable callbacks

From: Dmitry Baryshkov
Date: Mon Dec 05 2022 - 16:11:37 EST

Next message: Dmitry Baryshkov: "Re: [PATCH v4 09/13] drm/msm/dp: Don't enable HPD interrupts for edp"
Previous message: Daniel Borkmann: "Re: [PATCH bpf-next] libbpf: Optimized return value in libbpf_strerror when errno is libbpf errno"
In reply to: Bjorn Andersson: "[PATCH v4 10/13] drm/msm/dp: Rely on hpd_enable/disable callbacks"
Next in thread: Bjorn Andersson: "[PATCH v4 12/13] arm64: dts: qcom: sc8280xp-crd: Enable EDP"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 5 December 2022 20:44:30 GMT+03:00, Bjorn Andersson <quic_bjorande@xxxxxxxxxxx> wrote:
>From: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
>
>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. In other configurations the HPD notification
>might be delivered by the DRM framework from an associated bridge.
>
>This difference is not appropriately represented by the "is_edp"
>boolean, but is properly represented by the frameworks invocation of the
>hpd_enable() and hpd_disable() callbacks. Switch the current condition
>to rely on these callbacks instead.
>
>This ensures appropriate handling of the three cases; no bridge
>connected, a bridge without DRM_BRIDGE_OP_HPD and a bridge with
>DRM_BRIDGE_OP_HPD.
>
>Signed-off-by: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
>Signed-off-by: Bjorn Andersson <quic_bjorande@xxxxxxxxxxx>
>---
>
>Worth mentioning, I did look into moving the HPD enablement/disablement
>completely into these new callbacks, but that affect the entire power
>management model of the driver, so I think it's worth to tackle that in
>subsequent changes. It seems also reasonable to expect that we by such
>modifications could leave the block unclocked until the external HPD
>notification arrives...

I see... I still suppose this is the way to go in the long term.

For now:

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>

>
>Changes since v3:
>- Introduced reliance on hpd_enable/disable callbacks instead of next_bridge
>
> drivers/gpu/drm/msm/dp/dp_display.c | 35 ++++++++++++++++++++---------
> drivers/gpu/drm/msm/dp/dp_display.h | 1 +
> drivers/gpu/drm/msm/dp/dp_drm.c | 2 ++
> drivers/gpu/drm/msm/dp/dp_drm.h | 2 ++
> 4 files changed, 30 insertions(+), 10 deletions(-)
>
>diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
>index bb92c33beff8..3e464c33ff11 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.internal_hpd)
> 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.internal_hpd)
> 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.internal_hpd)
> 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.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",
>@@ -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 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,
>@@ -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.internal_hpd)
> dp_catalog_hpd_config_intr(dp->catalog,
> DP_DP_HPD_PLUG_INT_MASK |
> DP_DP_HPD_UNPLUG_INT_MASK,
>@@ -1778,6 +1777,22 @@ void dp_bridge_mode_set(struct drm_bridge *drm_bridge,
> !!(dp_display->dp_mode.drm_mode.flags & DRM_MODE_FLAG_NHSYNC);
> }
>
>+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;
>+
>+ dp_display->internal_hpd = true;
>+}
>+
>+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;
>+
>+ dp_display->internal_hpd = false;
>+}
>+
> void dp_bridge_hpd_notify(struct drm_bridge *bridge,
> enum drm_connector_status status)
> {
>@@ -1785,8 +1800,8 @@ void dp_bridge_hpd_notify(struct drm_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);
>
>- /* Without next_bridge interrupts are handled by the DP core directly */
>- if (!dp_display->next_bridge)
>+ /* Plug events are generated by the dp_display_irq_handler() */
>+ if (dp_display->internal_hpd)
> return;
>
> if (!dp->core_initialized) {
>diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h
>index dcedf021f7fe..371337d0fae2 100644
>--- a/drivers/gpu/drm/msm/dp/dp_display.h
>+++ b/drivers/gpu/drm/msm/dp/dp_display.h
>@@ -21,6 +21,7 @@ struct msm_dp {
> bool power_on;
> unsigned int connector_type;
> bool is_edp;
>+ bool internal_hpd;
>
> hdmi_codec_plugged_cb plugged_cb;
>
>diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
>index 3898366ebd5e..275370f21115 100644
>--- a/drivers/gpu/drm/msm/dp/dp_drm.c
>+++ b/drivers/gpu/drm/msm/dp/dp_drm.c
>@@ -102,6 +102,8 @@ static const struct drm_bridge_funcs dp_bridge_ops = {
> .get_modes = dp_bridge_get_modes,
> .detect = dp_bridge_detect,
> .atomic_check = dp_bridge_atomic_check,
>+ .hpd_enable = dp_bridge_hpd_enable,
>+ .hpd_disable = dp_bridge_hpd_disable,
> .hpd_notify = dp_bridge_hpd_notify,
> };
>
>diff --git a/drivers/gpu/drm/msm/dp/dp_drm.h b/drivers/gpu/drm/msm/dp/dp_drm.h
>index 79e6b2cf2d25..250f7c66201f 100644
>--- a/drivers/gpu/drm/msm/dp/dp_drm.h
>+++ b/drivers/gpu/drm/msm/dp/dp_drm.h
>@@ -32,6 +32,8 @@ enum drm_mode_status dp_bridge_mode_valid(struct drm_bridge *bridge,
> void dp_bridge_mode_set(struct drm_bridge *drm_bridge,
> const struct drm_display_mode *mode,
> const struct drm_display_mode *adjusted_mode);
>+void dp_bridge_hpd_enable(struct drm_bridge *bridge);
>+void dp_bridge_hpd_disable(struct drm_bridge *bridge);
> void dp_bridge_hpd_notify(struct drm_bridge *bridge,
> enum drm_connector_status status);
>

--
With best wishes
Dmitry

Next message: Dmitry Baryshkov: "Re: [PATCH v4 09/13] drm/msm/dp: Don't enable HPD interrupts for edp"
Previous message: Daniel Borkmann: "Re: [PATCH bpf-next] libbpf: Optimized return value in libbpf_strerror when errno is libbpf errno"
In reply to: Bjorn Andersson: "[PATCH v4 10/13] drm/msm/dp: Rely on hpd_enable/disable callbacks"
Next in thread: Bjorn Andersson: "[PATCH v4 12/13] arm64: dts: qcom: sc8280xp-crd: Enable EDP"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]