drm/i915: push more stuff down to encoders on crtc enable/disable

Simplify CRTC enable.

v2: Don't forget DSI (Daniel)

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_ddi.c     | 8 ++++++++
 drivers/gpu/drm/i915/intel_display.c | 3 ---
 drivers/gpu/drm/i915/intel_dsi.c     | 7 ++++++-
 3 files changed, 14 insertions(+), 4 deletions(-)

drivers/gpu/drm/i915/intel_ddi.c +8 -0

index 511aa60e0176..f1adc2544ab9 100644
@@ -2207,8 +2207,16 @@ static void intel_ddi_pre_enable(struct intel_encoder *encoder,
 				 const struct intel_crtc_state *pipe_config,
 				 const struct drm_connector_state *conn_state)
 {
+	struct drm_crtc *crtc = pipe_config->base.crtc;
+	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	int pipe = intel_crtc->pipe;
 	int type = encoder->type;
 
+	WARN_ON(intel_crtc->config->has_pch_encoder);
+
+	intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
+
 	if (type == INTEL_OUTPUT_DP || type == INTEL_OUTPUT_EDP) {
 		intel_ddi_pre_enable_dp(encoder,
 					pipe_config->port_clock,

drivers/gpu/drm/i915/intel_display.c +0 -3

index 9058cdfb0649..b55944d8149b 100644
@@ -5521,9 +5521,6 @@ static void haswell_crtc_enable(struct intel_crtc_state *pipe_config,
 
 	intel_crtc->active = true;
 
-	if (!intel_crtc->config->has_pch_encoder)
-		intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
-
 	intel_encoders_pre_enable(crtc, pipe_config, old_state);
 
 	if (intel_crtc->config->has_pch_encoder)

drivers/gpu/drm/i915/intel_dsi.c +6 -1

index 20a7b004ffd7..66bbedc5fa01 100644
@@ -790,14 +790,19 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder,
 				 const struct intel_crtc_state *pipe_config,
 				 const struct drm_connector_state *conn_state)
 {
-	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
+	struct drm_crtc *crtc = pipe_config->base.crtc;
+	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	int pipe = intel_crtc->pipe;
 	enum port port;
 	u32 val;
 	bool glk_cold_boot = false;
 
 	DRM_DEBUG_KMS("\n");
 
+	intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
+
 	/*
 	 * The BIOS may leave the PLL in a wonky state where it doesn't
 	 * lock. It needs to be fully powered down to fix it.

Signature · 1 line

2.11.0

Only assign the hooks once instead of overwriting for DDI.

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_crt.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

drivers/gpu/drm/i915/intel_crt.c +8 -7

index 2abe556ccaf5..668e8c3e791d 100644
@@ -955,16 +955,11 @@ void intel_crt_init(struct drm_i915_private *dev_priv)
 
 	crt->base.power_domain = POWER_DOMAIN_PORT_CRT;
 
-	crt->base.compute_config = intel_crt_compute_config;
-	if (HAS_PCH_SPLIT(dev_priv)) {
-		crt->base.disable = pch_disable_crt;
-		crt->base.post_disable = pch_post_disable_crt;
-	} else {
-		crt->base.disable = intel_disable_crt;
-	}
 	if (I915_HAS_HOTPLUG(dev_priv) &&
 	    !dmi_check_system(intel_spurious_crt_detect))
 		crt->base.hpd_pin = HPD_CRT;
+
+	crt->base.compute_config = intel_crt_compute_config;
 	if (HAS_DDI(dev_priv)) {
 		crt->base.port = PORT_E;
 		crt->base.get_config = hsw_crt_get_config;

@@ -975,6 +970,12 @@ void intel_crt_init(struct drm_i915_private *dev_priv)
 		crt->base.disable = hsw_disable_crt;
 		crt->base.post_disable = hsw_post_disable_crt;
 	} else {
+		if (HAS_PCH_SPLIT(dev_priv)) {
+			crt->base.disable = pch_disable_crt;
+			crt->base.post_disable = pch_post_disable_crt;
+		} else {
+			crt->base.disable = intel_disable_crt;
+		}
 		crt->base.port = PORT_NONE;
 		crt->base.get_config = intel_crt_get_config;
 		crt->base.get_hw_state = intel_crt_get_hw_state;

Signature · 1 line

2.11.0

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_crt.c     |  3 +++
 drivers/gpu/drm/i915/intel_ddi.c     | 18 ++++++++++++++++++
 drivers/gpu/drm/i915/intel_display.c |  3 ---
 3 files changed, 21 insertions(+), 3 deletions(-)

drivers/gpu/drm/i915/intel_crt.c +3 -0

index 668e8c3e791d..b3094d606329 100644
@@ -255,6 +255,9 @@ static void hsw_pre_pll_enable_crt(struct intel_encoder *encoder,
 	WARN_ON(!intel_crtc->config->has_pch_encoder);
 
 	intel_set_pch_fifo_underrun_reporting(dev_priv, PIPE_A, false);
+
+	if (intel_crtc->config->shared_dpll)
+		intel_enable_shared_dpll(intel_crtc);
 }
 
 static void hsw_pre_enable_crt(struct intel_encoder *encoder,

drivers/gpu/drm/i915/intel_ddi.c +18 -0

index f1adc2544ab9..c1152c602f08 100644
@@ -2407,13 +2407,29 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder,
 	}
 }
 
+static void intel_ddi_pre_pll_enable(struct intel_encoder *encoder,
+				     const struct intel_crtc_state *pipe_config,
+				     const struct drm_connector_state *conn_state)
+{
+	struct drm_crtc *crtc = pipe_config->base.crtc;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+
+	if (intel_crtc->config->shared_dpll)
+		intel_enable_shared_dpll(intel_crtc);
+}
+
 static void bxt_ddi_pre_pll_enable(struct intel_encoder *encoder,
 				   const struct intel_crtc_state *pipe_config,
 				   const struct drm_connector_state *conn_state)
 {
+	struct drm_crtc *crtc = pipe_config->base.crtc;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	uint8_t mask = pipe_config->lane_lat_optim_mask;
 
 	bxt_ddi_phy_set_lane_optim_mask(encoder, mask);
+
+	if (intel_crtc->config->shared_dpll)
+		intel_enable_shared_dpll(intel_crtc);
 }
 
 void intel_ddi_prepare_link_retrain(struct intel_dp *intel_dp)

@@ -2714,6 +2730,8 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
 	intel_encoder->enable = intel_enable_ddi;
 	if (IS_GEN9_LP(dev_priv))
 		intel_encoder->pre_pll_enable = bxt_ddi_pre_pll_enable;
+	else
+		intel_encoder->pre_pll_enable = intel_ddi_pre_pll_enable;
 	intel_encoder->pre_enable = intel_ddi_pre_enable;
 	intel_encoder->disable = intel_disable_ddi;
 	intel_encoder->post_disable = intel_ddi_post_disable;

drivers/gpu/drm/i915/intel_display.c +0 -3

index 9f2bf3b3f759..6d0573350786 100644
@@ -5490,9 +5490,6 @@ static void haswell_crtc_enable(struct intel_crtc_state *pipe_config,
 
 	intel_encoders_pre_pll_enable(crtc, pipe_config, old_state);
 
-	if (intel_crtc->config->shared_dpll)
-		intel_enable_shared_dpll(intel_crtc);
-
 	if (intel_crtc_has_dp_encoder(intel_crtc->config))
 		intel_dp_set_m_n(intel_crtc, M1_N1);
 

Signature · 1 line

2.11.0

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_crt.c     |  3 +++
 drivers/gpu/drm/i915/intel_ddi.c     | 18 ++++++++++++++++++
 drivers/gpu/drm/i915/intel_display.c |  3 ---
 3 files changed, 21 insertions(+), 3 deletions(-)

drivers/gpu/drm/i915/intel_crt.c +0 -0

index 668e8c3e791d..b3094d606329 100644

drivers/gpu/drm/i915/intel_crt.c +0 -0

 static void hsw_pre_enable_crt(struct intel_encoder *encoder,

drivers/gpu/drm/i915/intel_ddi.c +18 -0

index f1adc2544ab9..c1152c602f08 100644
@@ -2407,13 +2407,29 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder,
 	}
 }
 
+static void intel_ddi_pre_pll_enable(struct intel_encoder *encoder,
+				     const struct intel_crtc_state *pipe_config,
+				     const struct drm_connector_state *conn_state)
+{
+	struct drm_crtc *crtc = pipe_config->base.crtc;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+
+	if (intel_crtc->config->shared_dpll)
+		intel_enable_shared_dpll(intel_crtc);
+}
+
 static void bxt_ddi_pre_pll_enable(struct intel_encoder *encoder,
 				   const struct intel_crtc_state *pipe_config,
 				   const struct drm_connector_state *conn_state)
 {
+	struct drm_crtc *crtc = pipe_config->base.crtc;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	uint8_t mask = pipe_config->lane_lat_optim_mask;
 
 	bxt_ddi_phy_set_lane_optim_mask(encoder, mask);
+
+	if (intel_crtc->config->shared_dpll)
+		intel_enable_shared_dpll(intel_crtc);
 }
 
 void intel_ddi_prepare_link_retrain(struct intel_dp *intel_dp)

@@ -2714,6 +2730,8 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
 	intel_encoder->enable = intel_enable_ddi;
 	if (IS_GEN9_LP(dev_priv))
 		intel_encoder->pre_pll_enable = bxt_ddi_pre_pll_enable;
+	else
+		intel_encoder->pre_pll_enable = intel_ddi_pre_pll_enable;
 	intel_encoder->pre_enable = intel_ddi_pre_enable;
 	intel_encoder->disable = intel_disable_ddi;
 	intel_encoder->post_disable = intel_ddi_post_disable;

drivers/gpu/drm/i915/intel_display.c +0 -3

index 9f2bf3b3f759..6d0573350786 100644
@@ -5490,9 +5490,6 @@ static void haswell_crtc_enable(struct intel_crtc_state *pipe_config,
 
 	intel_encoders_pre_pll_enable(crtc, pipe_config, old_state);
 
-	if (intel_crtc->config->shared_dpll)
-		intel_enable_shared_dpll(intel_crtc);
-
 	if (intel_crtc_has_dp_encoder(intel_crtc->config))
 		intel_dp_set_m_n(intel_crtc, M1_N1);
 

Signature · 1 line

2.11.0

Quoted 12 lines · Jani Nikula

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_crt.c     |  3 +++
 drivers/gpu/drm/i915/intel_ddi.c     | 18 ++++++++++++++++++
 drivers/gpu/drm/i915/intel_display.c |  3 ---
 3 files changed, 21 insertions(+), 3 deletions(-)

drivers/gpu/drm/i915/intel_crt.c +0 -0

index 668e8c3e791d..b3094d606329 100644

drivers/gpu/drm/i915/intel_crt.c static void hsw_pre_pll_enable_crt(struct intel_encoder *encoder, · line 261

Quoted 7 lines

drivers/gpu/drm/i915/intel_crt.c +3 -0

@@ -255,6 +255,9 @@ static void hsw_pre_pll_enable_crt(struct intel_encoder *encoder,
 	WARN_ON(!intel_crtc->config->has_pch_encoder);
 
 	intel_set_pch_fifo_underrun_reporting(dev_priv, PIPE_A, false);
+
+	if (intel_crtc->config->shared_dpll)
+		intel_enable_shared_dpll(intel_crtc);
 }

Looking at haswell_crtc_compute_clock all outputs on hsw+ need a shared dpll, except dsi. Here's my proposal:

- Drop the if condition when pushing into encoder callbacks, we statically know which one it is. With that I think this patch here looks good.

- Push the clock computation into encoders to get rid of the silly DSI special in haswell_crtc_compute_clock. Which then again will force you to get rid of all the encoder special casing in dpll_mgr->get_dpll, plus passing the encoder argument around.

I think the prettier way to do this is to pre-fill the clock we want in the encoder compute_config callback, and then also call intel_find_shared_dpll from there.

There's a lot more that should be untangled imo in intel_ddi.c and computed in compute_config of the right encoder type instead of if ladders all over, but we're slowly getting there I think.

Cheers, Daniel

Quoted 62 lines

drivers/gpu/drm/i915/intel_crt.c +0 -0

 static void hsw_pre_enable_crt(struct intel_encoder *encoder,

drivers/gpu/drm/i915/intel_ddi.c +18 -0

index f1adc2544ab9..c1152c602f08 100644
@@ -2407,13 +2407,29 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder,
 	}
 }
 
+static void intel_ddi_pre_pll_enable(struct intel_encoder *encoder,
+				     const struct intel_crtc_state *pipe_config,
+				     const struct drm_connector_state *conn_state)
+{
+	struct drm_crtc *crtc = pipe_config->base.crtc;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+
+	if (intel_crtc->config->shared_dpll)
+		intel_enable_shared_dpll(intel_crtc);
+}
+
 static void bxt_ddi_pre_pll_enable(struct intel_encoder *encoder,
 				   const struct intel_crtc_state *pipe_config,
 				   const struct drm_connector_state *conn_state)
 {
+	struct drm_crtc *crtc = pipe_config->base.crtc;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	uint8_t mask = pipe_config->lane_lat_optim_mask;
 
 	bxt_ddi_phy_set_lane_optim_mask(encoder, mask);
+
+	if (intel_crtc->config->shared_dpll)
+		intel_enable_shared_dpll(intel_crtc);
 }
 
 void intel_ddi_prepare_link_retrain(struct intel_dp *intel_dp)

@@ -2714,6 +2730,8 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
 	intel_encoder->enable = intel_enable_ddi;
 	if (IS_GEN9_LP(dev_priv))
 		intel_encoder->pre_pll_enable = bxt_ddi_pre_pll_enable;
+	else
+		intel_encoder->pre_pll_enable = intel_ddi_pre_pll_enable;
 	intel_encoder->pre_enable = intel_ddi_pre_enable;
 	intel_encoder->disable = intel_disable_ddi;
 	intel_encoder->post_disable = intel_ddi_post_disable;

drivers/gpu/drm/i915/intel_display.c +0 -3

index 9f2bf3b3f759..6d0573350786 100644
@@ -5490,9 +5490,6 @@ static void haswell_crtc_enable(struct intel_crtc_state *pipe_config,
 
 	intel_encoders_pre_pll_enable(crtc, pipe_config, old_state);
 
-	if (intel_crtc->config->shared_dpll)
-		intel_enable_shared_dpll(intel_crtc);
-
 	if (intel_crtc_has_dp_encoder(intel_crtc->config))
 		intel_dp_set_m_n(intel_crtc, M1_N1);
 

Signature · 1 line

2.11.0

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Quoted 3 lines · Jani Nikula

Only assign the hooks once instead of overwriting for DDI.

Signed-off-by Jani Nikula <[email protected]>

Reviewed-by Daniel Vetter <[email protected]>

Quoted 44 lines

---
 drivers/gpu/drm/i915/intel_crt.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

drivers/gpu/drm/i915/intel_crt.c +8 -7

index 2abe556ccaf5..668e8c3e791d 100644
@@ -955,16 +955,11 @@ void intel_crt_init(struct drm_i915_private *dev_priv)
 
 	crt->base.power_domain = POWER_DOMAIN_PORT_CRT;
 
-	crt->base.compute_config = intel_crt_compute_config;
-	if (HAS_PCH_SPLIT(dev_priv)) {
-		crt->base.disable = pch_disable_crt;
-		crt->base.post_disable = pch_post_disable_crt;
-	} else {
-		crt->base.disable = intel_disable_crt;
-	}
 	if (I915_HAS_HOTPLUG(dev_priv) &&
 	    !dmi_check_system(intel_spurious_crt_detect))
 		crt->base.hpd_pin = HPD_CRT;
+
+	crt->base.compute_config = intel_crt_compute_config;
 	if (HAS_DDI(dev_priv)) {
 		crt->base.port = PORT_E;
 		crt->base.get_config = hsw_crt_get_config;

@@ -975,6 +970,12 @@ void intel_crt_init(struct drm_i915_private *dev_priv)
 		crt->base.disable = hsw_disable_crt;
 		crt->base.post_disable = hsw_post_disable_crt;
 	} else {
+		if (HAS_PCH_SPLIT(dev_priv)) {
+			crt->base.disable = pch_disable_crt;
+			crt->base.post_disable = pch_post_disable_crt;
+		} else {
+			crt->base.disable = intel_disable_crt;
+		}
 		crt->base.port = PORT_NONE;
 		crt->base.get_config = intel_crt_get_config;
 		crt->base.get_hw_state = intel_crt_get_hw_state;

Signature · 1 line

2.11.0

Quoted 12 lines · Jani Nikula

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_crt.c     |  3 +++
 drivers/gpu/drm/i915/intel_ddi.c     | 18 ++++++++++++++++++
 drivers/gpu/drm/i915/intel_display.c |  3 ---
 3 files changed, 21 insertions(+), 3 deletions(-)

drivers/gpu/drm/i915/intel_crt.c +0 -0

index 668e8c3e791d..b3094d606329 100644

drivers/gpu/drm/i915/intel_crt.c static void hsw_pre_pll_enable_crt(struct intel_encoder *encoder, · line 261

Quoted 7 lines

drivers/gpu/drm/i915/intel_crt.c +3 -0

@@ -255,6 +255,9 @@ static void hsw_pre_pll_enable_crt(struct intel_encoder *encoder,
 	WARN_ON(!intel_crtc->config->has_pch_encoder);
 
 	intel_set_pch_fifo_underrun_reporting(dev_priv, PIPE_A, false);
+
+	if (intel_crtc->config->shared_dpll)
+		intel_enable_shared_dpll(intel_crtc);
 }

Looking at haswell_crtc_compute_clock all outputs on hsw+ need a shared dpll, except dsi. Here's my proposal:

- Drop the if condition when pushing into encoder callbacks, we statically know which one it is. With that I think this patch here looks good.

- Push the clock computation into encoders to get rid of the silly DSI special in haswell_crtc_compute_clock. Which then again will force you to get rid of all the encoder special casing in dpll_mgr->get_dpll, plus passing the encoder argument around.

I think the prettier way to do this is to pre-fill the clock we want in the encoder compute_config callback, and then also call intel_find_shared_dpll from there.

There's a lot more that should be untangled imo in intel_ddi.c and computed in compute_config of the right encoder type instead of if ladders all over, but we're slowly getting there I think.

Cheers, Daniel

Quoted 62 lines

drivers/gpu/drm/i915/intel_crt.c +0 -0

 static void hsw_pre_enable_crt(struct intel_encoder *encoder,

drivers/gpu/drm/i915/intel_ddi.c +18 -0

index f1adc2544ab9..c1152c602f08 100644
@@ -2407,13 +2407,29 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder,
 	}
 }
 
+static void intel_ddi_pre_pll_enable(struct intel_encoder *encoder,
+				     const struct intel_crtc_state *pipe_config,
+				     const struct drm_connector_state *conn_state)
+{
+	struct drm_crtc *crtc = pipe_config->base.crtc;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+
+	if (intel_crtc->config->shared_dpll)
+		intel_enable_shared_dpll(intel_crtc);
+}
+
 static void bxt_ddi_pre_pll_enable(struct intel_encoder *encoder,
 				   const struct intel_crtc_state *pipe_config,
 				   const struct drm_connector_state *conn_state)
 {
+	struct drm_crtc *crtc = pipe_config->base.crtc;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	uint8_t mask = pipe_config->lane_lat_optim_mask;
 
 	bxt_ddi_phy_set_lane_optim_mask(encoder, mask);
+
+	if (intel_crtc->config->shared_dpll)
+		intel_enable_shared_dpll(intel_crtc);
 }
 
 void intel_ddi_prepare_link_retrain(struct intel_dp *intel_dp)

@@ -2714,6 +2730,8 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
 	intel_encoder->enable = intel_enable_ddi;
 	if (IS_GEN9_LP(dev_priv))
 		intel_encoder->pre_pll_enable = bxt_ddi_pre_pll_enable;
+	else
+		intel_encoder->pre_pll_enable = intel_ddi_pre_pll_enable;
 	intel_encoder->pre_enable = intel_ddi_pre_enable;
 	intel_encoder->disable = intel_disable_ddi;
 	intel_encoder->post_disable = intel_ddi_post_disable;

drivers/gpu/drm/i915/intel_display.c +0 -3

index 9f2bf3b3f759..6d0573350786 100644
@@ -5490,9 +5490,6 @@ static void haswell_crtc_enable(struct intel_crtc_state *pipe_config,
 
 	intel_encoders_pre_pll_enable(crtc, pipe_config, old_state);
 
-	if (intel_crtc->config->shared_dpll)
-		intel_enable_shared_dpll(intel_crtc);
-
 	if (intel_crtc_has_dp_encoder(intel_crtc->config))
 		intel_dp_set_m_n(intel_crtc, M1_N1);
 

Signature · 1 line

2.11.0