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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux