[PATCH 0/4] [RFC] Stage shared dpll configs

[PATCH 0/4] [RFC] Stage shared dpll configs

[Ander Conselvan de Oliveira]

From: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>

Hi,

This series changes the mode set sequence so that the clock and PLL
logic that was done in the *_crtc_mode_set() hooks is done before
disabling crtcs. This avoids having to restore the old configuration
in the case of failure, since the hardware was never touched.

The changes went through only light testing in a single platform. But I'd
like to get comments on the general approach sooner rather than later,
hence this RFC.

Thanks,
Ander

Ander Conselvan de Oliveira (4):
  drm/i915: Replace some loop through encoders with
    intel_pipe_has_type()
  drm/i915: Make *_crtc_mode_set work on new_config
  drm/i915: Convert shared dpll reference count to a crtc mask
  drm/i915: Compute clocks and choose DPLLs before disabling crtcs

 drivers/gpu/drm/i915/i915_debugfs.c  |   4 +-
 drivers/gpu/drm/i915/i915_drv.h      |  11 +-
 drivers/gpu/drm/i915/intel_ddi.c     |   6 +-
 drivers/gpu/drm/i915/intel_display.c | 346 ++++++++++++++++++++++-------------
 4 files changed, 230 insertions(+), 137 deletions(-)

-- 
1.8.3.2

[PATCH 1/4] drm/i915: Replace some loop through encoders with intel_pipe_has_type()

[Ander Conselvan de Oliveira]

From: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>

In the ironlake mode set code, there was two instances of a loop through
encoders to find out if one of them has INTEL_OUTPUT_LVDS type. Simplify
the code by deleting some lines and use intel_pipe_has_type() instead.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 21 ++-------------------
 1 file changed, 2 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7b5639d..3a06c3d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7110,18 +7110,11 @@ static bool ironlake_compute_clocks(struct drm_crtc *crtc,
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_encoder *intel_encoder;
 	int refclk;
 	const intel_limit_t *limit;
 	bool ret, is_lvds = false;
 
-	for_each_encoder_on_crtc(dev, crtc, intel_encoder) {
-		switch (intel_encoder->type) {
-		case INTEL_OUTPUT_LVDS:
-			is_lvds = true;
-			break;
-		}
-	}
+	is_lvds = intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS);
 
 	refclk = ironlake_get_refclk(crtc);
 
@@ -7261,23 +7254,13 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 {
 	struct drm_device *dev = crtc->dev;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	int num_connectors = 0;
 	intel_clock_t clock, reduced_clock;
 	u32 dpll = 0, fp = 0, fp2 = 0;
 	bool ok, has_reduced_clock = false;
 	bool is_lvds = false;
-	struct intel_encoder *encoder;
 	struct intel_shared_dpll *pll;
 
-	for_each_encoder_on_crtc(dev, crtc, encoder) {
-		switch (encoder->type) {
-		case INTEL_OUTPUT_LVDS:
-			is_lvds = true;
-			break;
-		}
-
-		num_connectors++;
-	}
+	is_lvds = intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS);
 
 	WARN(!(HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)),
 	     "Unexpected PCH type %d\n", INTEL_PCH_TYPE(dev));
-- 
1.8.3.2

[PATCH 2/4] drm/i915: Make *_crtc_mode_set work on new_config

[Ander Conselvan de Oliveira]

From: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>

This shouldn't change the behavior of those functions, since they are
called after the new_config is made effective and that points to the
current config. In a follow up patch, the mode set sequence will be
changed so this is called before disabling crtcs, and in that case
those functions should work on the staged config.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c     |   4 +-
 drivers/gpu/drm/i915/intel_display.c | 167 +++++++++++++++++++++--------------
 2 files changed, 101 insertions(+), 70 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index cb5367c..619723d 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -792,7 +792,7 @@ hsw_ddi_pll_select(struct intel_crtc *intel_crtc,
 		      WRPLL_DIVIDER_REFERENCE(r2) | WRPLL_DIVIDER_FEEDBACK(n2) |
 		      WRPLL_DIVIDER_POST(p);
 
-		intel_crtc->config.dpll_hw_state.wrpll = val;
+		intel_crtc->new_config->dpll_hw_state.wrpll = val;
 
 		pll = intel_get_shared_dpll(intel_crtc);
 		if (pll == NULL) {
@@ -801,7 +801,7 @@ hsw_ddi_pll_select(struct intel_crtc *intel_crtc,
 			return false;
 		}
 
-		intel_crtc->config.ddi_pll_sel = PORT_CLK_SEL_WRPLL(pll->id);
+		intel_crtc->new_config->ddi_pll_sel = PORT_CLK_SEL_WRPLL(pll->id);
 	}
 
 	return true;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3a06c3d..9d8fe8d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -420,13 +420,32 @@ static bool intel_pipe_has_type(struct drm_crtc *crtc, int type)
 	return false;
 }
 
+/**
+ * Returns whether any output on the specified pipe will have the specified
+ * type after a staged modeset is complete, i.e., the same as
+ * intel_pipe_has_type() but looking at encoder->new_crtc instead of
+ * encoder->crtc.
+ */
+static bool intel_pipe_will_have_type(struct drm_crtc *crtc, int type)
+{
+	struct drm_device *dev = crtc->dev;
+	struct intel_encoder *encoder;
+
+	for_each_intel_encoder(dev, encoder)
+		if (encoder->new_crtc == to_intel_crtc(crtc) &&
+		    encoder->type == type)
+			return true;
+
+	return false;
+}
+
 static const intel_limit_t *intel_ironlake_limit(struct drm_crtc *crtc,
 						int refclk)
 {
 	struct drm_device *dev = crtc->dev;
 	const intel_limit_t *limit;
 
-	if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS)) {
+	if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS)) {
 		if (intel_is_dual_link_lvds(dev)) {
 			if (refclk == 100000)
 				limit = &intel_limits_ironlake_dual_lvds_100m;
@@ -449,15 +468,15 @@ static const intel_limit_t *intel_g4x_limit(struct drm_crtc *crtc)
 	struct drm_device *dev = crtc->dev;
 	const intel_limit_t *limit;
 
-	if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS)) {
+	if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS)) {
 		if (intel_is_dual_link_lvds(dev))
 			limit = &intel_limits_g4x_dual_channel_lvds;
 		else
 			limit = &intel_limits_g4x_single_channel_lvds;
-	} else if (intel_pipe_has_type(crtc, INTEL_OUTPUT_HDMI) ||
-		   intel_pipe_has_type(crtc, INTEL_OUTPUT_ANALOG)) {
+	} else if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_HDMI) ||
+		   intel_pipe_will_have_type(crtc, INTEL_OUTPUT_ANALOG)) {
 		limit = &intel_limits_g4x_hdmi;
-	} else if (intel_pipe_has_type(crtc, INTEL_OUTPUT_SDVO)) {
+	} else if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_SDVO)) {
 		limit = &intel_limits_g4x_sdvo;
 	} else /* The option is for other outputs */
 		limit = &intel_limits_i9xx_sdvo;
@@ -475,7 +494,7 @@ static const intel_limit_t *intel_limit(struct drm_crtc *crtc, int refclk)
 	else if (IS_G4X(dev)) {
 		limit = intel_g4x_limit(crtc);
 	} else if (IS_PINEVIEW(dev)) {
-		if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS))
+		if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS))
 			limit = &intel_limits_pineview_lvds;
 		else
 			limit = &intel_limits_pineview_sdvo;
@@ -484,14 +503,14 @@ static const intel_limit_t *intel_limit(struct drm_crtc *crtc, int refclk)
 	} else if (IS_VALLEYVIEW(dev)) {
 		limit = &intel_limits_vlv;
 	} else if (!IS_GEN2(dev)) {
-		if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS))
+		if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS))
 			limit = &intel_limits_i9xx_lvds;
 		else
 			limit = &intel_limits_i9xx_sdvo;
 	} else {
-		if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS))
+		if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS))
 			limit = &intel_limits_i8xx_lvds;
-		else if (intel_pipe_has_type(crtc, INTEL_OUTPUT_DVO))
+		else if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_DVO))
 			limit = &intel_limits_i8xx_dvo;
 		else
 			limit = &intel_limits_i8xx_dac;
@@ -586,7 +605,7 @@ i9xx_find_best_dpll(const intel_limit_t *limit, struct drm_crtc *crtc,
 	intel_clock_t clock;
 	int err = target;
 
-	if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS)) {
+	if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS)) {
 		/*
 		 * For LVDS just rely on its current settings for dual-channel.
 		 * We haven't figured out how to reliably set up different
@@ -647,7 +666,7 @@ pnv_find_best_dpll(const intel_limit_t *limit, struct drm_crtc *crtc,
 	intel_clock_t clock;
 	int err = target;
 
-	if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS)) {
+	if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS)) {
 		/*
 		 * For LVDS just rely on its current settings for dual-channel.
 		 * We haven't figured out how to reliably set up different
@@ -710,7 +729,7 @@ g4x_find_best_dpll(const intel_limit_t *limit, struct drm_crtc *crtc,
 	int err_most = (target >> 8) + (target >> 9);
 	found = false;
 
-	if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS)) {
+	if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS)) {
 		if (intel_is_dual_link_lvds(dev))
 			clock.p2 = limit->p2.p2_fast;
 		else
@@ -5607,7 +5626,7 @@ static int i9xx_get_refclk(struct drm_crtc *crtc, int num_connectors)
 
 	if (IS_VALLEYVIEW(dev)) {
 		refclk = 100000;
-	} else if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS) &&
+	} else if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS) &&
 	    intel_panel_use_ssc(dev_priv) && num_connectors < 2) {
 		refclk = dev_priv->vbt.lvds_ssc_freq;
 		DRM_DEBUG_KMS("using SSC reference clock of %d kHz\n", refclk);
@@ -5758,11 +5777,11 @@ static void vlv_update_pll(struct intel_crtc *crtc)
 	if (crtc->pipe == PIPE_B)
 		dpll |= DPLL_INTEGRATED_CRI_CLK_VLV;
 	dpll |= DPLL_VCO_ENABLE;
-	crtc->config.dpll_hw_state.dpll = dpll;
+	crtc->new_config->dpll_hw_state.dpll = dpll;
 
-	dpll_md = (crtc->config.pixel_multiplier - 1)
+	dpll_md = (crtc->new_config->pixel_multiplier - 1)
 		<< DPLL_MD_UDI_MULTIPLIER_SHIFT;
-	crtc->config.dpll_hw_state.dpll_md = dpll_md;
+	crtc->new_config->dpll_hw_state.dpll_md = dpll_md;
 }
 
 static void vlv_prepare_pll(struct intel_crtc *crtc)
@@ -5858,14 +5877,16 @@ static void vlv_prepare_pll(struct intel_crtc *crtc)
 
 static void chv_update_pll(struct intel_crtc *crtc)
 {
-	crtc->config.dpll_hw_state.dpll = DPLL_SSC_REF_CLOCK_CHV |
+	crtc->new_config->dpll_hw_state.dpll = DPLL_SSC_REF_CLOCK_CHV |
 		DPLL_REFA_CLK_ENABLE_VLV | DPLL_VGA_MODE_DIS |
 		DPLL_VCO_ENABLE;
 	if (crtc->pipe != PIPE_A)
-		crtc->config.dpll_hw_state.dpll |= DPLL_INTEGRATED_CRI_CLK_VLV;
+		crtc->new_config->dpll_hw_state.dpll |=
+			DPLL_INTEGRATED_CRI_CLK_VLV;
 
-	crtc->config.dpll_hw_state.dpll_md =
-		(crtc->config.pixel_multiplier - 1) << DPLL_MD_UDI_MULTIPLIER_SHIFT;
+	crtc->new_config->dpll_hw_state.dpll_md =
+		(crtc->new_config->pixel_multiplier - 1)
+		<< DPLL_MD_UDI_MULTIPLIER_SHIFT;
 }
 
 static void chv_prepare_pll(struct intel_crtc *crtc)
@@ -5946,29 +5967,29 @@ static void i9xx_update_pll(struct intel_crtc *crtc,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u32 dpll;
 	bool is_sdvo;
-	struct dpll *clock = &crtc->config.dpll;
+	struct dpll *clock = &crtc->new_config->dpll;
 
 	i9xx_update_pll_dividers(crtc, reduced_clock);
 
-	is_sdvo = intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_SDVO) ||
-		intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_HDMI);
+	is_sdvo = intel_pipe_will_have_type(&crtc->base, INTEL_OUTPUT_SDVO) ||
+		intel_pipe_will_have_type(&crtc->base, INTEL_OUTPUT_HDMI);
 
 	dpll = DPLL_VGA_MODE_DIS;
 
-	if (intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_LVDS))
+	if (intel_pipe_will_have_type(&crtc->base, INTEL_OUTPUT_LVDS))
 		dpll |= DPLLB_MODE_LVDS;
 	else
 		dpll |= DPLLB_MODE_DAC_SERIAL;
 
 	if (IS_I945G(dev) || IS_I945GM(dev) || IS_G33(dev)) {
-		dpll |= (crtc->config.pixel_multiplier - 1)
+		dpll |= (crtc->new_config->pixel_multiplier - 1)
 			<< SDVO_MULTIPLIER_SHIFT_HIRES;
 	}
 
 	if (is_sdvo)
 		dpll |= DPLL_SDVO_HIGH_SPEED;
 
-	if (intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_DISPLAYPORT))
+	if (intel_pipe_will_have_type(&crtc->base, INTEL_OUTPUT_DISPLAYPORT))
 		dpll |= DPLL_SDVO_HIGH_SPEED;
 
 	/* compute bitmask from p1 value */
@@ -5996,21 +6017,21 @@ static void i9xx_update_pll(struct intel_crtc *crtc,
 	if (INTEL_INFO(dev)->gen >= 4)
 		dpll |= (6 << PLL_LOAD_PULSE_PHASE_SHIFT);
 
-	if (crtc->config.sdvo_tv_clock)
+	if (crtc->new_config->sdvo_tv_clock)
 		dpll |= PLL_REF_INPUT_TVCLKINBC;
-	else if (intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_LVDS) &&
+	else if (intel_pipe_will_have_type(&crtc->base, INTEL_OUTPUT_LVDS) &&
 		 intel_panel_use_ssc(dev_priv) && num_connectors < 2)
 		dpll |= PLLB_REF_INPUT_SPREADSPECTRUMIN;
 	else
 		dpll |= PLL_REF_INPUT_DREFCLK;
 
 	dpll |= DPLL_VCO_ENABLE;
-	crtc->config.dpll_hw_state.dpll = dpll;
+	crtc->new_config->dpll_hw_state.dpll = dpll;
 
 	if (INTEL_INFO(dev)->gen >= 4) {
-		u32 dpll_md = (crtc->config.pixel_multiplier - 1)
+		u32 dpll_md = (crtc->new_config->pixel_multiplier - 1)
 			<< DPLL_MD_UDI_MULTIPLIER_SHIFT;
-		crtc->config.dpll_hw_state.dpll_md = dpll_md;
+		crtc->new_config->dpll_hw_state.dpll_md = dpll_md;
 	}
 }
 
@@ -6021,13 +6042,13 @@ static void i8xx_update_pll(struct intel_crtc *crtc,
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u32 dpll;
-	struct dpll *clock = &crtc->config.dpll;
+	struct dpll *clock = &crtc->new_config->dpll;
 
 	i9xx_update_pll_dividers(crtc, reduced_clock);
 
 	dpll = DPLL_VGA_MODE_DIS;
 
-	if (intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_LVDS)) {
+	if (intel_pipe_will_have_type(&crtc->base, INTEL_OUTPUT_LVDS)) {
 		dpll |= (1 << (clock->p1 - 1)) << DPLL_FPA01_P1_POST_DIV_SHIFT;
 	} else {
 		if (clock->p1 == 2)
@@ -6038,17 +6059,18 @@ static void i8xx_update_pll(struct intel_crtc *crtc,
 			dpll |= PLL_P2_DIVIDE_BY_4;
 	}
 
-	if (!IS_I830(dev) && intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_DVO))
+	if (!IS_I830(dev) &&
+	    intel_pipe_will_have_type(&crtc->base, INTEL_OUTPUT_DVO))
 		dpll |= DPLL_DVO_2X_MODE;
 
-	if (intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_LVDS) &&
+	if (intel_pipe_will_have_type(&crtc->base, INTEL_OUTPUT_LVDS) &&
 		 intel_panel_use_ssc(dev_priv) && num_connectors < 2)
 		dpll |= PLLB_REF_INPUT_SPREADSPECTRUMIN;
 	else
 		dpll |= PLL_REF_INPUT_DREFCLK;
 
 	dpll |= DPLL_VCO_ENABLE;
-	crtc->config.dpll_hw_state.dpll = dpll;
+	crtc->new_config->dpll_hw_state.dpll = dpll;
 }
 
 static void intel_set_pipe_timings(struct intel_crtc *intel_crtc)
@@ -6258,7 +6280,10 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
 	struct intel_encoder *encoder;
 	const intel_limit_t *limit;
 
-	for_each_encoder_on_crtc(dev, crtc, encoder) {
+	for_each_intel_encoder(dev, encoder) {
+		if (encoder->new_crtc != to_intel_crtc(crtc))
+			continue;
+
 		switch (encoder->type) {
 		case INTEL_OUTPUT_LVDS:
 			is_lvds = true;
@@ -6274,7 +6299,7 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
 	if (is_dsi)
 		return 0;
 
-	if (!intel_crtc->config.clock_set) {
+	if (!intel_crtc->new_config->clock_set) {
 		refclk = i9xx_get_refclk(crtc, num_connectors);
 
 		/*
@@ -6285,7 +6310,7 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
 		 */
 		limit = intel_limit(crtc, refclk);
 		ok = dev_priv->display.find_dpll(limit, crtc,
-						 intel_crtc->config.port_clock,
+						 intel_crtc->new_config->port_clock,
 						 refclk, NULL, &clock);
 		if (!ok) {
 			DRM_ERROR("Couldn't find PLL settings for mode!\n");
@@ -6306,11 +6331,11 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
 							    &reduced_clock);
 		}
 		/* Compat-code for transition, will disappear. */
-		intel_crtc->config.dpll.n = clock.n;
-		intel_crtc->config.dpll.m1 = clock.m1;
-		intel_crtc->config.dpll.m2 = clock.m2;
-		intel_crtc->config.dpll.p1 = clock.p1;
-		intel_crtc->config.dpll.p2 = clock.p2;
+		intel_crtc->new_config->dpll.n = clock.n;
+		intel_crtc->new_config->dpll.m1 = clock.m1;
+		intel_crtc->new_config->dpll.m2 = clock.m2;
+		intel_crtc->new_config->dpll.p1 = clock.p1;
+		intel_crtc->new_config->dpll.p2 = clock.p2;
 	}
 
 	if (IS_GEN2(dev)) {
@@ -6926,7 +6951,10 @@ static int ironlake_get_refclk(struct drm_crtc *crtc)
 	int num_connectors = 0;
 	bool is_lvds = false;
 
-	for_each_encoder_on_crtc(dev, crtc, encoder) {
+	for_each_intel_encoder(dev, encoder) {
+		if (encoder->new_crtc != to_intel_crtc(crtc))
+			continue;
+
 		switch (encoder->type) {
 		case INTEL_OUTPUT_LVDS:
 			is_lvds = true;
@@ -7114,7 +7142,7 @@ static bool ironlake_compute_clocks(struct drm_crtc *crtc,
 	const intel_limit_t *limit;
 	bool ret, is_lvds = false;
 
-	is_lvds = intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS);
+	is_lvds = intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS);
 
 	refclk = ironlake_get_refclk(crtc);
 
@@ -7125,7 +7153,7 @@ static bool ironlake_compute_clocks(struct drm_crtc *crtc,
 	 */
 	limit = intel_limit(crtc, refclk);
 	ret = dev_priv->display.find_dpll(limit, crtc,
-					  to_intel_crtc(crtc)->config.port_clock,
+					  to_intel_crtc(crtc)->new_config->port_clock,
 					  refclk, NULL, clock);
 	if (!ret)
 		return false;
@@ -7175,7 +7203,10 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc,
 	int factor, num_connectors = 0;
 	bool is_lvds = false, is_sdvo = false;
 
-	for_each_encoder_on_crtc(dev, crtc, intel_encoder) {
+	for_each_intel_encoder(dev, intel_encoder) {
+		if (intel_encoder->new_crtc != to_intel_crtc(crtc))
+			continue;
+
 		switch (intel_encoder->type) {
 		case INTEL_OUTPUT_LVDS:
 			is_lvds = true;
@@ -7196,10 +7227,10 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc,
 		     dev_priv->vbt.lvds_ssc_freq == 100000) ||
 		    (HAS_PCH_IBX(dev) && intel_is_dual_link_lvds(dev)))
 			factor = 25;
-	} else if (intel_crtc->config.sdvo_tv_clock)
+	} else if (intel_crtc->new_config->sdvo_tv_clock)
 		factor = 20;
 
-	if (ironlake_needs_fb_cb_tune(&intel_crtc->config.dpll, factor))
+	if (ironlake_needs_fb_cb_tune(&intel_crtc->new_config->dpll, factor))
 		*fp |= FP_CB_TUNE;
 
 	if (fp2 && (reduced_clock->m < factor * reduced_clock->n))
@@ -7212,20 +7243,20 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc,
 	else
 		dpll |= DPLLB_MODE_DAC_SERIAL;
 
-	dpll |= (intel_crtc->config.pixel_multiplier - 1)
+	dpll |= (intel_crtc->new_config->pixel_multiplier - 1)
 		<< PLL_REF_SDVO_HDMI_MULTIPLIER_SHIFT;
 
 	if (is_sdvo)
 		dpll |= DPLL_SDVO_HIGH_SPEED;
-	if (intel_crtc->config.has_dp_encoder)
+	if (intel_crtc->new_config->has_dp_encoder)
 		dpll |= DPLL_SDVO_HIGH_SPEED;
 
 	/* compute bitmask from p1 value */
-	dpll |= (1 << (intel_crtc->config.dpll.p1 - 1)) << DPLL_FPA01_P1_POST_DIV_SHIFT;
+	dpll |= (1 << (intel_crtc->new_config->dpll.p1 - 1)) << DPLL_FPA01_P1_POST_DIV_SHIFT;
 	/* also FPA1 */
-	dpll |= (1 << (intel_crtc->config.dpll.p1 - 1)) << DPLL_FPA1_P1_POST_DIV_SHIFT;
+	dpll |= (1 << (intel_crtc->new_config->dpll.p1 - 1)) << DPLL_FPA1_P1_POST_DIV_SHIFT;
 
-	switch (intel_crtc->config.dpll.p2) {
+	switch (intel_crtc->new_config->dpll.p2) {
 	case 5:
 		dpll |= DPLL_DAC_SERIAL_P2_CLOCK_DIV_5;
 		break;
@@ -7267,22 +7298,22 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 
 	ok = ironlake_compute_clocks(crtc, &clock,
 				     &has_reduced_clock, &reduced_clock);
-	if (!ok && !intel_crtc->config.clock_set) {
+	if (!ok && !intel_crtc->new_config->clock_set) {
 		DRM_ERROR("Couldn't find PLL settings for mode!\n");
 		return -EINVAL;
 	}
 	/* Compat-code for transition, will disappear. */
-	if (!intel_crtc->config.clock_set) {
-		intel_crtc->config.dpll.n = clock.n;
-		intel_crtc->config.dpll.m1 = clock.m1;
-		intel_crtc->config.dpll.m2 = clock.m2;
-		intel_crtc->config.dpll.p1 = clock.p1;
-		intel_crtc->config.dpll.p2 = clock.p2;
+	if (!intel_crtc->new_config->clock_set) {
+		intel_crtc->new_config->dpll.n = clock.n;
+		intel_crtc->new_config->dpll.m1 = clock.m1;
+		intel_crtc->new_config->dpll.m2 = clock.m2;
+		intel_crtc->new_config->dpll.p1 = clock.p1;
+		intel_crtc->new_config->dpll.p2 = clock.p2;
 	}
 
 	/* CPU eDP is the only output that doesn't need a PCH PLL of its own. */
-	if (intel_crtc->config.has_pch_encoder) {
-		fp = i9xx_dpll_compute_fp(&intel_crtc->config.dpll);
+	if (intel_crtc->new_config->has_pch_encoder) {
+		fp = i9xx_dpll_compute_fp(&intel_crtc->new_config->dpll);
 		if (has_reduced_clock)
 			fp2 = i9xx_dpll_compute_fp(&reduced_clock);
 
@@ -7290,12 +7321,12 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 					     &fp, &reduced_clock,
 					     has_reduced_clock ? &fp2 : NULL);
 
-		intel_crtc->config.dpll_hw_state.dpll = dpll;
-		intel_crtc->config.dpll_hw_state.fp0 = fp;
+		intel_crtc->new_config->dpll_hw_state.dpll = dpll;
+		intel_crtc->new_config->dpll_hw_state.fp0 = fp;
 		if (has_reduced_clock)
-			intel_crtc->config.dpll_hw_state.fp1 = fp2;
+			intel_crtc->new_config->dpll_hw_state.fp1 = fp2;
 		else
-			intel_crtc->config.dpll_hw_state.fp1 = fp;
+			intel_crtc->new_config->dpll_hw_state.fp1 = fp;
 
 		pll = intel_get_shared_dpll(intel_crtc);
 		if (pll == NULL) {
-- 
1.8.3.2

[PATCH 3/4] drm/i915: Convert shared dpll reference count to a crtc mask

[Ander Conselvan de Oliveira]

From: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>

This will be used in a follow up patch to properly release shared DPLLs
without relying on the shared_dpll field in pipe_config.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c  |  4 +--
 drivers/gpu/drm/i915/i915_drv.h      |  4 +--
 drivers/gpu/drm/i915/intel_display.c | 63 +++++++++++++++++++++++-------------
 3 files changed, 44 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index da4036d..71885d8 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2627,8 +2627,8 @@ static int i915_shared_dplls_info(struct seq_file *m, void *unused)
 		struct intel_shared_dpll *pll = &dev_priv->shared_dplls[i];
 
 		seq_printf(m, "DPLL%i: %s, id: %i\n", i, pll->name, pll->id);
-		seq_printf(m, " refcount: %i, active: %i, on: %s\n", pll->refcount,
-			   pll->active, yesno(pll->on));
+		seq_printf(m, " crtc_mask: 0x%08x, active: %d, on: %s\n",
+			   pll->crtc_mask, pll->active, yesno(pll->on));
 		seq_printf(m, " tracked hardware state:\n");
 		seq_printf(m, " dpll:    0x%08x\n", pll->hw_state.dpll);
 		seq_printf(m, " dpll_md: 0x%08x\n", pll->hw_state.dpll_md);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ac6232b..cb6df07 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -227,8 +227,8 @@ struct intel_dpll_hw_state {
 };
 
 struct intel_shared_dpll {
-	int refcount; /* count of number of CRTCs sharing this PLL */
-	int active; /* count of number of active CRTCs (i.e. DPMS on) */
+	unsigned crtc_mask; /* mask of CRTCs sharing this PLL */
+	int  active; /* count of number of active CRTCs (i.e. DPMS on) */
 	bool on; /* is the PLL actually active? Disabled during modeset */
 	const char *name;
 	/* should match the index in the dev_priv->shared_dplls array */
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9d8fe8d..46df2db 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1778,7 +1778,7 @@ static void intel_prepare_shared_dpll(struct intel_crtc *crtc)
 	if (WARN_ON(pll == NULL))
 		return;
 
-	WARN_ON(!pll->refcount);
+	WARN_ON(!pll->crtc_mask);
 	if (pll->active == 0) {
 		DRM_DEBUG_DRIVER("setting up %s\n", pll->name);
 		WARN_ON(pll->on);
@@ -1805,7 +1805,7 @@ static void intel_enable_shared_dpll(struct intel_crtc *crtc)
 	if (WARN_ON(pll == NULL))
 		return;
 
-	if (WARN_ON(pll->refcount == 0))
+	if (WARN_ON(pll->crtc_mask == 0))
 		return;
 
 	DRM_DEBUG_KMS("enable %s (active %d, on? %d) for crtc %d\n",
@@ -1837,7 +1837,7 @@ static void intel_disable_shared_dpll(struct intel_crtc *crtc)
 	if (WARN_ON(pll == NULL))
 	       return;
 
-	if (WARN_ON(pll->refcount == 0))
+	if (WARN_ON(pll->crtc_mask == 0))
 		return;
 
 	DRM_DEBUG_KMS("disable %s (active %d, on? %d) for crtc %d\n",
@@ -3835,12 +3835,13 @@ void intel_put_shared_dpll(struct intel_crtc *crtc)
 	if (pll == NULL)
 		return;
 
-	if (pll->refcount == 0) {
-		WARN(1, "bad %s refcount\n", pll->name);
+	if (!(pll->crtc_mask & (1 << crtc->pipe))) {
+		WARN(1, "bad %s crtc mask\n", pll->name);
 		return;
 	}
 
-	if (--pll->refcount == 0) {
+	pll->crtc_mask &= ~(1 << crtc->pipe);
+	if (pll->crtc_mask == 0) {
 		WARN_ON(pll->on);
 		WARN_ON(pll->active);
 	}
@@ -3868,7 +3869,7 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc)
 		DRM_DEBUG_KMS("CRTC:%d using pre-allocated %s\n",
 			      crtc->base.base.id, pll->name);
 
-		WARN_ON(pll->refcount);
+		WARN_ON(pll->crtc_mask);
 
 		goto found;
 	}
@@ -3877,14 +3878,15 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc)
 		pll = &dev_priv->shared_dplls[i];
 
 		/* Only want to check enabled timings first */
-		if (pll->refcount == 0)
+		if (pll->crtc_mask == 0)
 			continue;
 
 		if (memcmp(&crtc->config.dpll_hw_state, &pll->hw_state,
 			   sizeof(pll->hw_state)) == 0) {
-			DRM_DEBUG_KMS("CRTC:%d sharing existing %s (refcount %d, ative %d)\n",
-				      crtc->base.base.id,
-				      pll->name, pll->refcount, pll->active);
+			DRM_DEBUG_KMS("CRTC:%d sharing existing %s "
+				      "(crtc_mask 0x%08x, active %d)\n",
+				      crtc->base.base.id, pll->name,
+				      pll->crtc_mask, pll->active);
 
 			goto found;
 		}
@@ -3893,7 +3895,7 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc)
 	/* Ok no matching timings, maybe there's a free one? */
 	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
 		pll = &dev_priv->shared_dplls[i];
-		if (pll->refcount == 0) {
+		if (pll->crtc_mask == 0) {
 			DRM_DEBUG_KMS("CRTC:%d allocated %s\n",
 				      crtc->base.base.id, pll->name);
 			goto found;
@@ -3903,14 +3905,14 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc)
 	return NULL;
 
 found:
-	if (pll->refcount == 0)
+	if (pll->crtc_mask == 0)
 		pll->hw_state = crtc->config.dpll_hw_state;
 
 	crtc->config.shared_dpll = i;
 	DRM_DEBUG_DRIVER("using %s for pipe %c\n", pll->name,
 			 pipe_name(crtc->pipe));
 
-	pll->refcount++;
+	pll->crtc_mask |= 1 << crtc->pipe;
 
 	return pll;
 }
@@ -10812,6 +10814,19 @@ check_crtc_state(struct drm_device *dev)
 	}
 }
 
+static int mask_to_refcount(unsigned mask)
+{
+	int refcount = 0;
+
+	while (mask) {
+		if (mask & 1)
+			refcount++;
+		mask >>= 1;
+	}
+
+	return refcount;
+}
+
 static void
 check_shared_dpll_state(struct drm_device *dev)
 {
@@ -10831,9 +10846,9 @@ check_shared_dpll_state(struct drm_device *dev)
 
 		active = pll->get_hw_state(dev_priv, pll, &dpll_hw_state);
 
-		WARN(pll->active > pll->refcount,
+		WARN(pll->active > mask_to_refcount(pll->crtc_mask),
 		     "more active pll users than references: %i vs %i\n",
-		     pll->active, pll->refcount);
+		     pll->active, mask_to_refcount(pll->crtc_mask));
 		WARN(pll->active && !pll->on,
 		     "pll in active use but not on in sw tracking\n");
 		WARN(pll->on && !pll->active,
@@ -10851,9 +10866,9 @@ check_shared_dpll_state(struct drm_device *dev)
 		WARN(pll->active != active_crtcs,
 		     "pll active crtcs mismatch (expected %i, found %i)\n",
 		     pll->active, active_crtcs);
-		WARN(pll->refcount != enabled_crtcs,
+		WARN(mask_to_refcount(pll->crtc_mask) != enabled_crtcs,
 		     "pll enabled crtcs mismatch (expected %i, found %i)\n",
-		     pll->refcount, enabled_crtcs);
+		     mask_to_refcount(pll->crtc_mask), enabled_crtcs);
 
 		WARN(pll->on && memcmp(&pll->hw_state, &dpll_hw_state,
 				       sizeof(dpll_hw_state)),
@@ -13270,16 +13285,18 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
 
 		pll->on = pll->get_hw_state(dev_priv, pll, &pll->hw_state);
 		pll->active = 0;
+		pll->crtc_mask = 0;
 		for_each_intel_crtc(dev, crtc) {
-			if (crtc->active && intel_crtc_to_shared_dpll(crtc) == pll)
+			if (crtc->active && intel_crtc_to_shared_dpll(crtc) == pll) {
 				pll->active++;
+				pll->crtc_mask |= 1 << crtc->pipe;
+			}
 		}
-		pll->refcount = pll->active;
 
-		DRM_DEBUG_KMS("%s hw state readout: refcount %i, on %i\n",
-			      pll->name, pll->refcount, pll->on);
+		DRM_DEBUG_KMS("%s hw state readout: crtc_mask 0x%08x, on %i\n",
+			      pll->name, pll->crtc_mask, pll->on);
 
-		if (pll->refcount)
+		if (pll->crtc_mask)
 			intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
 	}
 
-- 
1.8.3.2

[PATCH 4/4] drm/i915: Compute clocks and choose DPLLs before disabling crtcs

[Ander Conselvan de Oliveira]

From: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>

It is possible for a mode set to fail if there aren't shared DPLLS that
match the new configuration requirement or other errors in clock
computation. Since that step was executed after disabling crtcs, in the
failure case the hardware configuration is changed and needs to be
restored. Doing those things early allows the mode set to fail before
actually touching the hardware.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |   7 +++
 drivers/gpu/drm/i915/intel_ddi.c     |   2 -
 drivers/gpu/drm/i915/intel_display.c | 113 ++++++++++++++++++++++++++---------
 3 files changed, 92 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index cb6df07..0bb3b54 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -245,6 +245,12 @@ struct intel_shared_dpll {
 	bool (*get_hw_state)(struct drm_i915_private *dev_priv,
 			     struct intel_shared_dpll *pll,
 			     struct intel_dpll_hw_state *hw_state);
+
+	/* Holds the new configuration of the shared DPLLS during mode set */
+	struct {
+		unsigned crtc_mask;
+		struct intel_dpll_hw_state hw_state;
+	} staged_config;
 };
 
 /* Used by dp and fdi links */
@@ -476,6 +482,7 @@ struct drm_i915_display_funcs {
 				struct intel_crtc_config *);
 	void (*get_plane_config)(struct intel_crtc *,
 				 struct intel_plane_config *);
+	int (*crtc_compute_clock)(struct drm_crtc *crtc);
 	int (*crtc_mode_set)(struct drm_crtc *crtc,
 			     int x, int y,
 			     struct drm_framebuffer *old_fb);
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 619723d..fc6f988 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -821,8 +821,6 @@ bool intel_ddi_pll_select(struct intel_crtc *intel_crtc)
 	struct intel_encoder *intel_encoder = intel_ddi_get_crtc_encoder(crtc);
 	int clock = intel_crtc->config.port_clock;
 
-	intel_put_shared_dpll(intel_crtc);
-
 	return hsw_ddi_pll_select(intel_crtc, intel_encoder, clock);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 46df2db..4a8aff7 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3852,15 +3852,9 @@ void intel_put_shared_dpll(struct intel_crtc *crtc)
 struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
-	struct intel_shared_dpll *pll = intel_crtc_to_shared_dpll(crtc);
+	struct intel_shared_dpll *pll;
 	enum intel_dpll_id i;
 
-	if (pll) {
-		DRM_DEBUG_KMS("CRTC:%d dropping existing %s\n",
-			      crtc->base.base.id, pll->name);
-		intel_put_shared_dpll(crtc);
-	}
-
 	if (HAS_PCH_IBX(dev_priv->dev)) {
 		/* Ironlake PCH has a fixed PLL->PCH pipe mapping. */
 		i = (enum intel_dpll_id) crtc->pipe;
@@ -3869,7 +3863,7 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc)
 		DRM_DEBUG_KMS("CRTC:%d using pre-allocated %s\n",
 			      crtc->base.base.id, pll->name);
 
-		WARN_ON(pll->crtc_mask);
+		WARN_ON(pll->staged_config.crtc_mask);
 
 		goto found;
 	}
@@ -3878,16 +3872,16 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc)
 		pll = &dev_priv->shared_dplls[i];
 
 		/* Only want to check enabled timings first */
-		if (pll->crtc_mask == 0)
+		if (pll->staged_config.crtc_mask == 0)
 			continue;
 
-		if (memcmp(&crtc->config.dpll_hw_state, &pll->hw_state,
-			   sizeof(pll->hw_state)) == 0) {
-			DRM_DEBUG_KMS("CRTC:%d sharing existing %s "
-				      "(crtc_mask 0x%08x, active %d)\n",
+		if (memcmp(&crtc->new_config->dpll_hw_state,
+			   &pll->staged_config.hw_state,
+			   sizeof(pll->staged_config.hw_state)) == 0) {
+			DRM_DEBUG_KMS("CRTC:%d sharing existing %s (crtc mask 0x%08x, ative %d)\n",
 				      crtc->base.base.id, pll->name,
-				      pll->crtc_mask, pll->active);
-
+				      pll->staged_config.crtc_mask,
+				      pll->active);
 			goto found;
 		}
 	}
@@ -3895,7 +3889,7 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc)
 	/* Ok no matching timings, maybe there's a free one? */
 	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
 		pll = &dev_priv->shared_dplls[i];
-		if (pll->crtc_mask == 0) {
+		if (pll->staged_config.crtc_mask == 0) {
 			DRM_DEBUG_KMS("CRTC:%d allocated %s\n",
 				      crtc->base.base.id, pll->name);
 			goto found;
@@ -3905,18 +3899,53 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc)
 	return NULL;
 
 found:
-	if (pll->crtc_mask == 0)
-		pll->hw_state = crtc->config.dpll_hw_state;
+	if (pll->staged_config.crtc_mask == 0)
+		pll->staged_config.hw_state = crtc->new_config->dpll_hw_state;
 
-	crtc->config.shared_dpll = i;
+	crtc->new_config->shared_dpll = i;
 	DRM_DEBUG_DRIVER("using %s for pipe %c\n", pll->name,
 			 pipe_name(crtc->pipe));
 
-	pll->crtc_mask |= 1 << crtc->pipe;
+	pll->staged_config.crtc_mask |= 1 << crtc->pipe;
 
 	return pll;
 }
 
+/**
+ * intel_shared_dpll_start_config - start a new PLL staged config
+ * @dev_priv: DRM device
+ * @clear_pipes: mask of pipes that will have their PLLs freed
+ *
+ * Starts a new PLL staged config, copying the current config but
+ * releasing the references of pipes specified in clear_pipes.
+ */
+static void intel_shared_dpll_start_config(struct drm_i915_private *dev_priv,
+					   unsigned clear_pipes)
+{
+	struct intel_shared_dpll *pll;
+	enum intel_dpll_id i;
+
+	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
+		pll = &dev_priv->shared_dplls[i];
+
+		pll->staged_config.crtc_mask = pll->crtc_mask & ~clear_pipes;
+		pll->staged_config.hw_state = pll->hw_state;
+	}
+}
+
+static void intel_shared_dpll_commit(struct drm_i915_private *dev_priv)
+{
+	struct intel_shared_dpll *pll;
+	enum intel_dpll_id i;
+
+	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
+		pll = &dev_priv->shared_dplls[i];
+
+		pll->hw_state = pll->staged_config.hw_state;
+		pll->crtc_mask = pll->staged_config.crtc_mask;
+	}
+}
+
 static void cpt_verify_modeset(struct drm_device *dev, int pipe)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -6272,6 +6301,11 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
 			      int x, int y,
 			      struct drm_framebuffer *fb)
 {
+	return 0;
+}
+
+static int i9xx_crtc_compute_clock(struct drm_crtc *crtc)
+{
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
@@ -7281,9 +7315,7 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc,
 	return dpll | DPLL_VCO_ENABLE;
 }
 
-static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
-				  int x, int y,
-				  struct drm_framebuffer *fb)
+static int ironlake_crtc_compute_clock(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
@@ -7336,8 +7368,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 					 pipe_name(intel_crtc->pipe));
 			return -EINVAL;
 		}
-	} else
-		intel_put_shared_dpll(intel_crtc);
+	}
 
 	if (is_lvds && has_reduced_clock && i915.powersave)
 		intel_crtc->lowfreq_avail = true;
@@ -7347,6 +7378,13 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	return 0;
 }
 
+static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
+				  int x, int y,
+				  struct drm_framebuffer *fb)
+{
+	return 0;
+}
+
 static void intel_pch_transcoder_get_m_n(struct intel_crtc *crtc,
 					 struct intel_link_m_n *m_n)
 {
@@ -7836,9 +7874,7 @@ static void haswell_modeset_global_resources(struct drm_device *dev)
 	modeset_update_crtc_power_domains(dev);
 }
 
-static int haswell_crtc_mode_set(struct drm_crtc *crtc,
-				 int x, int y,
-				 struct drm_framebuffer *fb)
+static int haswell_crtc_compute_clock(struct drm_crtc *crtc)
 {
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 
@@ -7850,6 +7886,13 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
 	return 0;
 }
 
+static int haswell_crtc_mode_set(struct drm_crtc *crtc,
+				 int x, int y,
+				 struct drm_framebuffer *fb)
+{
+	return 0;
+}
+
 static void haswell_get_ddi_pll(struct drm_i915_private *dev_priv,
 				enum port port,
 				struct intel_crtc_config *pipe_config)
@@ -10988,6 +11031,14 @@ static int __intel_set_mode(struct drm_crtc *crtc,
 		prepare_pipes &= ~disable_pipes;
 	}
 
+	intel_shared_dpll_start_config(dev_priv, modeset_pipes | disable_pipes);
+
+	for_each_intel_crtc_masked(dev, modeset_pipes, intel_crtc) {
+		ret = dev_priv->display.crtc_compute_clock(&intel_crtc->base);
+		if (ret)
+			goto done;
+	}
+
 	for_each_intel_crtc_masked(dev, disable_pipes, intel_crtc)
 		intel_crtc_disable(&intel_crtc->base);
 
@@ -11015,6 +11066,8 @@ static int __intel_set_mode(struct drm_crtc *crtc,
 						&pipe_config->adjusted_mode);
 	}
 
+	intel_shared_dpll_commit(dev_priv);
+
 	/* Only after disabling all output pipelines that will be changed can we
 	 * update the the output configuration. */
 	intel_modeset_update_state(dev, prepare_pipes);
@@ -12580,6 +12633,7 @@ static void intel_init_display(struct drm_device *dev)
 	if (HAS_DDI(dev)) {
 		dev_priv->display.get_pipe_config = haswell_get_pipe_config;
 		dev_priv->display.get_plane_config = ironlake_get_plane_config;
+		dev_priv->display.crtc_compute_clock = haswell_crtc_compute_clock;
 		dev_priv->display.crtc_mode_set = haswell_crtc_mode_set;
 		dev_priv->display.crtc_enable = haswell_crtc_enable;
 		dev_priv->display.crtc_disable = haswell_crtc_disable;
@@ -12593,6 +12647,7 @@ static void intel_init_display(struct drm_device *dev)
 	} else if (HAS_PCH_SPLIT(dev)) {
 		dev_priv->display.get_pipe_config = ironlake_get_pipe_config;
 		dev_priv->display.get_plane_config = ironlake_get_plane_config;
+		dev_priv->display.crtc_compute_clock = ironlake_crtc_compute_clock;
 		dev_priv->display.crtc_mode_set = ironlake_crtc_mode_set;
 		dev_priv->display.crtc_enable = ironlake_crtc_enable;
 		dev_priv->display.crtc_disable = ironlake_crtc_disable;
@@ -12602,6 +12657,7 @@ static void intel_init_display(struct drm_device *dev)
 	} else if (IS_VALLEYVIEW(dev)) {
 		dev_priv->display.get_pipe_config = i9xx_get_pipe_config;
 		dev_priv->display.get_plane_config = i9xx_get_plane_config;
+		dev_priv->display.crtc_compute_clock = i9xx_crtc_compute_clock;
 		dev_priv->display.crtc_mode_set = i9xx_crtc_mode_set;
 		dev_priv->display.crtc_enable = valleyview_crtc_enable;
 		dev_priv->display.crtc_disable = i9xx_crtc_disable;
@@ -12611,6 +12667,7 @@ static void intel_init_display(struct drm_device *dev)
 	} else {
 		dev_priv->display.get_pipe_config = i9xx_get_pipe_config;
 		dev_priv->display.get_plane_config = i9xx_get_plane_config;
+		dev_priv->display.crtc_compute_clock = i9xx_crtc_compute_clock;
 		dev_priv->display.crtc_mode_set = i9xx_crtc_mode_set;
 		dev_priv->display.crtc_enable = i9xx_crtc_enable;
 		dev_priv->display.crtc_disable = i9xx_crtc_disable;
-- 
1.8.3.2

Re: [PATCH 2/4] drm/i915: Make *_crtc_mode_set work on new_config

[Daniel Vetter]

On Wed, Oct 08, 2014 at 06:32:21PM +0300, Ander Conselvan de Oliveira wrote:
> From: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
>
> This shouldn't change the behavior of those functions, since they are
> called after the new_config is made effective and that points to the
> current config. In a follow up patch, the mode set sequence will be
> changed so this is called before disabling crtcs, and in that case
> those functions should work on the staged config.
>
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>

[snip]

> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 3a06c3d..9d8fe8d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -420,13 +420,32 @@ static bool intel_pipe_has_type(struct drm_crtc *crtc, int type)
>   return false;
>  }
>
> +/**
> + * Returns whether any output on the specified pipe will have the specified
> + * type after a staged modeset is complete, i.e., the same as
> + * intel_pipe_has_type() but looking at encoder->new_crtc instead of
> + * encoder->crtc.
> + */
> +static bool intel_pipe_will_have_type(struct drm_crtc *crtc, int type)

s/drm_crtc/intel_crtc/ - general rule in the driver is to use the most
specific type to avoid massive amounts of upcasting everywhere.
> +{
> + struct drm_device *dev = crtc->dev;
> + struct intel_encoder *encoder;
> +
> + for_each_intel_encoder(dev, encoder)
> + if (encoder->new_crtc == to_intel_crtc(crtc) &&
> +    encoder->type == type)
> + return true;
> +
> + return false;
> +}

Just aside: With atomic we'll have the list of connectors in a table in
the atomic state structures (and no longer with pointers in the objects
itself). So this function might gain a few more parameters, but we can do
that later on easily.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 4/4] drm/i915: Compute clocks and choose DPLLs before disabling crtcs

[Daniel Vetter]

On Wed, Oct 08, 2014 at 06:32:23PM +0300, Ander Conselvan de Oliveira wrote:
> From: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
>
> It is possible for a mode set to fail if there aren't shared DPLLS that
> match the new configuration requirement or other errors in clock
> computation. Since that step was executed after disabling crtcs, in the
> failure case the hardware configuration is changed and needs to be
> restored. Doing those things early allows the mode set to fail before
> actually touching the hardware.
>
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>

Looks nifty and gets us to the shiny new world without too much fuzz. I
think for paranoia it would be good to split this patch here per platform
(i.e. i9xx, ilk+ and hsw+). And if we make the crtc_mode_set hook optional
we can also get rid of all the dummy functions here. And remove the caller
(yay!) at the end of the series.

Longer-term we might still want to shuffle quite a few of the
encoder-specific pll computation code into encoder->compute_config. But
that can be done as leisure if someone gets bored ;-)

Cheers, Daniel


> ---
>  drivers/gpu/drm/i915/i915_drv.h      |   7 +++
>  drivers/gpu/drm/i915/intel_ddi.c     |   2 -
>  drivers/gpu/drm/i915/intel_display.c | 113 ++++++++++++++++++++++++++---------
>  3 files changed, 92 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index cb6df07..0bb3b54 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -245,6 +245,12 @@ struct intel_shared_dpll {
>   bool (*get_hw_state)(struct drm_i915_private *dev_priv,
>       struct intel_shared_dpll *pll,
>       struct intel_dpll_hw_state *hw_state);
> +
> + /* Holds the new configuration of the shared DPLLS during mode set */
> + struct {
> + unsigned crtc_mask;
> + struct intel_dpll_hw_state hw_state;
> + } staged_config;
>  };
>
>  /* Used by dp and fdi links */
> @@ -476,6 +482,7 @@ struct drm_i915_display_funcs {
>   struct intel_crtc_config *);
>   void (*get_plane_config)(struct intel_crtc *,
>   struct intel_plane_config *);
> + int (*crtc_compute_clock)(struct drm_crtc *crtc);
>   int (*crtc_mode_set)(struct drm_crtc *crtc,
>       int x, int y,
>       struct drm_framebuffer *old_fb);
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 619723d..fc6f988 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -821,8 +821,6 @@ bool intel_ddi_pll_select(struct intel_crtc *intel_crtc)
>   struct intel_encoder *intel_encoder = intel_ddi_get_crtc_encoder(crtc);
>   int clock = intel_crtc->config.port_clock;
>
> - intel_put_shared_dpll(intel_crtc);
> -
>   return hsw_ddi_pll_select(intel_crtc, intel_encoder, clock);
>  }
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 46df2db..4a8aff7 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3852,15 +3852,9 @@ void intel_put_shared_dpll(struct intel_crtc *crtc)
>  struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc)
>  {
>   struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> - struct intel_shared_dpll *pll = intel_crtc_to_shared_dpll(crtc);
> + struct intel_shared_dpll *pll;
>   enum intel_dpll_id i;
>
> - if (pll) {
> - DRM_DEBUG_KMS("CRTC:%d dropping existing %s\n",
> -      crtc->base.base.id, pll->name);
> - intel_put_shared_dpll(crtc);
> - }
> -
>   if (HAS_PCH_IBX(dev_priv->dev)) {
>   /* Ironlake PCH has a fixed PLL->PCH pipe mapping. */
>   i = (enum intel_dpll_id) crtc->pipe;
> @@ -3869,7 +3863,7 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc)
>   DRM_DEBUG_KMS("CRTC:%d using pre-allocated %s\n",
>        crtc->base.base.id, pll->name);
>
> - WARN_ON(pll->crtc_mask);
> + WARN_ON(pll->staged_config.crtc_mask);
>
>   goto found;
>   }
> @@ -3878,16 +3872,16 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc)
>   pll = &dev_priv->shared_dplls[i];
>
>   /* Only want to check enabled timings first */
> - if (pll->crtc_mask == 0)
> + if (pll->staged_config.crtc_mask == 0)
>   continue;
>
> - if (memcmp(&crtc->config.dpll_hw_state, &pll->hw_state,
> -   sizeof(pll->hw_state)) == 0) {
> - DRM_DEBUG_KMS("CRTC:%d sharing existing %s "
> -      "(crtc_mask 0x%08x, active %d)\n",
> + if (memcmp(&crtc->new_config->dpll_hw_state,
> +   &pll->staged_config.hw_state,
> +   sizeof(pll->staged_config.hw_state)) == 0) {
> + DRM_DEBUG_KMS("CRTC:%d sharing existing %s (crtc mask 0x%08x, ative %d)\n",
>        crtc->base.base.id, pll->name,
> -      pll->crtc_mask, pll->active);
> -
> +      pll->staged_config.crtc_mask,
> +      pll->active);
>   goto found;
>   }
>   }
> @@ -3895,7 +3889,7 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc)
>   /* Ok no matching timings, maybe there's a free one? */
>   for (i = 0; i < dev_priv->num_shared_dpll; i++) {
>   pll = &dev_priv->shared_dplls[i];
> - if (pll->crtc_mask == 0) {
> + if (pll->staged_config.crtc_mask == 0) {
>   DRM_DEBUG_KMS("CRTC:%d allocated %s\n",
>        crtc->base.base.id, pll->name);
>   goto found;
> @@ -3905,18 +3899,53 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc)
>   return NULL;
>
>  found:
> - if (pll->crtc_mask == 0)
> - pll->hw_state = crtc->config.dpll_hw_state;
> + if (pll->staged_config.crtc_mask == 0)
> + pll->staged_config.hw_state = crtc->new_config->dpll_hw_state;
>
> - crtc->config.shared_dpll = i;
> + crtc->new_config->shared_dpll = i;
>   DRM_DEBUG_DRIVER("using %s for pipe %c\n", pll->name,
>   pipe_name(crtc->pipe));
>
> - pll->crtc_mask |= 1 << crtc->pipe;
> + pll->staged_config.crtc_mask |= 1 << crtc->pipe;
>
>   return pll;
>  }
>
> +/**
> + * intel_shared_dpll_start_config - start a new PLL staged config
> + * @dev_priv: DRM device
> + * @clear_pipes: mask of pipes that will have their PLLs freed
> + *
> + * Starts a new PLL staged config, copying the current config but
> + * releasing the references of pipes specified in clear_pipes.
> + */
> +static void intel_shared_dpll_start_config(struct drm_i915_private *dev_priv,
> +   unsigned clear_pipes)
> +{
> + struct intel_shared_dpll *pll;
> + enum intel_dpll_id i;
> +
> + for (i = 0; i < dev_priv->num_shared_dpll; i++) {
> + pll = &dev_priv->shared_dplls[i];
> +
> + pll->staged_config.crtc_mask = pll->crtc_mask & ~clear_pipes;
> + pll->staged_config.hw_state = pll->hw_state;
> + }
> +}
> +
> +static void intel_shared_dpll_commit(struct drm_i915_private *dev_priv)
> +{
> + struct intel_shared_dpll *pll;
> + enum intel_dpll_id i;
> +
> + for (i = 0; i < dev_priv->num_shared_dpll; i++) {
> + pll = &dev_priv->shared_dplls[i];
> +
> + pll->hw_state = pll->staged_config.hw_state;
> + pll->crtc_mask = pll->staged_config.crtc_mask;
> + }
> +}
> +
>  static void cpt_verify_modeset(struct drm_device *dev, int pipe)
>  {
>   struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -6272,6 +6301,11 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
>        int x, int y,
>        struct drm_framebuffer *fb)
>  {
> + return 0;
> +}
> +
> +static int i9xx_crtc_compute_clock(struct drm_crtc *crtc)
> +{
>   struct drm_device *dev = crtc->dev;
>   struct drm_i915_private *dev_priv = dev->dev_private;
>   struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> @@ -7281,9 +7315,7 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc,
>   return dpll | DPLL_VCO_ENABLE;
>  }
>
> -static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
> -  int x, int y,
> -  struct drm_framebuffer *fb)
> +static int ironlake_crtc_compute_clock(struct drm_crtc *crtc)
>  {
>   struct drm_device *dev = crtc->dev;
>   struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> @@ -7336,8 +7368,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>   pipe_name(intel_crtc->pipe));
>   return -EINVAL;
>   }
> - } else
> - intel_put_shared_dpll(intel_crtc);
> + }
>
>   if (is_lvds && has_reduced_clock && i915.powersave)
>   intel_crtc->lowfreq_avail = true;
> @@ -7347,6 +7378,13 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>   return 0;
>  }
>
> +static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
> +  int x, int y,
> +  struct drm_framebuffer *fb)
> +{
> + return 0;
> +}
> +
>  static void intel_pch_transcoder_get_m_n(struct intel_crtc *crtc,
>   struct intel_link_m_n *m_n)
>  {
> @@ -7836,9 +7874,7 @@ static void haswell_modeset_global_resources(struct drm_device *dev)
>   modeset_update_crtc_power_domains(dev);
>  }
>
> -static int haswell_crtc_mode_set(struct drm_crtc *crtc,
> - int x, int y,
> - struct drm_framebuffer *fb)
> +static int haswell_crtc_compute_clock(struct drm_crtc *crtc)
>  {
>   struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>
> @@ -7850,6 +7886,13 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
>   return 0;
>  }
>
> +static int haswell_crtc_mode_set(struct drm_crtc *crtc,
> + int x, int y,
> + struct drm_framebuffer *fb)
> +{
> + return 0;
> +}
> +
>  static void haswell_get_ddi_pll(struct drm_i915_private *dev_priv,
>   enum port port,
>   struct intel_crtc_config *pipe_config)
> @@ -10988,6 +11031,14 @@ static int __intel_set_mode(struct drm_crtc *crtc,
>   prepare_pipes &= ~disable_pipes;
>   }
>
> + intel_shared_dpll_start_config(dev_priv, modeset_pipes | disable_pipes);
> +
> + for_each_intel_crtc_masked(dev, modeset_pipes, intel_crtc) {
> + ret = dev_priv->display.crtc_compute_clock(&intel_crtc->base);
> + if (ret)
> + goto done;
> + }
> +
>   for_each_intel_crtc_masked(dev, disable_pipes, intel_crtc)
>   intel_crtc_disable(&intel_crtc->base);
>
> @@ -11015,6 +11066,8 @@ static int __intel_set_mode(struct drm_crtc *crtc,
>   &pipe_config->adjusted_mode);
>   }
>
> + intel_shared_dpll_commit(dev_priv);
> +
>   /* Only after disabling all output pipelines that will be changed can we
>   * update the the output configuration. */
>   intel_modeset_update_state(dev, prepare_pipes);
> @@ -12580,6 +12633,7 @@ static void intel_init_display(struct drm_device *dev)
>   if (HAS_DDI(dev)) {
>   dev_priv->display.get_pipe_config = haswell_get_pipe_config;
>   dev_priv->display.get_plane_config = ironlake_get_plane_config;
> + dev_priv->display.crtc_compute_clock = haswell_crtc_compute_clock;
>   dev_priv->display.crtc_mode_set = haswell_crtc_mode_set;
>   dev_priv->display.crtc_enable = haswell_crtc_enable;
>   dev_priv->display.crtc_disable = haswell_crtc_disable;
> @@ -12593,6 +12647,7 @@ static void intel_init_display(struct drm_device *dev)
>   } else if (HAS_PCH_SPLIT(dev)) {
>   dev_priv->display.get_pipe_config = ironlake_get_pipe_config;
>   dev_priv->display.get_plane_config = ironlake_get_plane_config;
> + dev_priv->display.crtc_compute_clock = ironlake_crtc_compute_clock;
>   dev_priv->display.crtc_mode_set = ironlake_crtc_mode_set;
>   dev_priv->display.crtc_enable = ironlake_crtc_enable;
>   dev_priv->display.crtc_disable = ironlake_crtc_disable;
> @@ -12602,6 +12657,7 @@ static void intel_init_display(struct drm_device *dev)
>   } else if (IS_VALLEYVIEW(dev)) {
>   dev_priv->display.get_pipe_config = i9xx_get_pipe_config;
>   dev_priv->display.get_plane_config = i9xx_get_plane_config;
> + dev_priv->display.crtc_compute_clock = i9xx_crtc_compute_clock;
>   dev_priv->display.crtc_mode_set = i9xx_crtc_mode_set;
>   dev_priv->display.crtc_enable = valleyview_crtc_enable;
>   dev_priv->display.crtc_disable = i9xx_crtc_disable;
> @@ -12611,6 +12667,7 @@ static void intel_init_display(struct drm_device *dev)
>   } else {
>   dev_priv->display.get_pipe_config = i9xx_get_pipe_config;
>   dev_priv->display.get_plane_config = i9xx_get_plane_config;
> + dev_priv->display.crtc_compute_clock = i9xx_crtc_compute_clock;
>   dev_priv->display.crtc_mode_set = i9xx_crtc_mode_set;
>   dev_priv->display.crtc_enable = i9xx_crtc_enable;
>   dev_priv->display.crtc_disable = i9xx_crtc_disable;
> --
> 1.8.3.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 4/4] drm/i915: Compute clocks and choose DPLLs before disabling crtcs

[Jani Nikula]

On Thu, 09 Oct 2014, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Oct 08, 2014 at 06:32:23PM +0300, Ander Conselvan de Oliveira wrote:
>> From: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
>>
>> It is possible for a mode set to fail if there aren't shared DPLLS that
>> match the new configuration requirement or other errors in clock
>> computation. Since that step was executed after disabling crtcs, in the
>> failure case the hardware configuration is changed and needs to be
>> restored. Doing those things early allows the mode set to fail before
>> actually touching the hardware.
>>
>> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
>
> Looks nifty and gets us to the shiny new world without too much fuzz. I
> think for paranoia it would be good to split this patch here per platform
> (i.e. i9xx, ilk+ and hsw+). And if we make the crtc_mode_set hook optional
> we can also get rid of all the dummy functions here. And remove the caller
> (yay!) at the end of the series.

Should we add WARN_ON(!encoder->new_crtc) all around?

Jani.

>
> Longer-term we might still want to shuffle quite a few of the
> encoder-specific pll computation code into encoder->compute_config. But
> that can be done as leisure if someone gets bored ;-)
>
> Cheers, Daniel
>
>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h      |   7 +++
>>  drivers/gpu/drm/i915/intel_ddi.c     |   2 -
>>  drivers/gpu/drm/i915/intel_display.c | 113 ++++++++++++++++++++++++++---------
>>  3 files changed, 92 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index cb6df07..0bb3b54 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -245,6 +245,12 @@ struct intel_shared_dpll {
>>   bool (*get_hw_state)(struct drm_i915_private *dev_priv,
>>       struct intel_shared_dpll *pll,
>>       struct intel_dpll_hw_state *hw_state);
>> +
>> + /* Holds the new configuration of the shared DPLLS during mode set */
>> + struct {
>> + unsigned crtc_mask;
>> + struct intel_dpll_hw_state hw_state;
>> + } staged_config;
>>  };
>>
>>  /* Used by dp and fdi links */
>> @@ -476,6 +482,7 @@ struct drm_i915_display_funcs {
>>   struct intel_crtc_config *);
>>   void (*get_plane_config)(struct intel_crtc *,
>>   struct intel_plane_config *);
>> + int (*crtc_compute_clock)(struct drm_crtc *crtc);
>>   int (*crtc_mode_set)(struct drm_crtc *crtc,
>>       int x, int y,
>>       struct drm_framebuffer *old_fb);
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> index 619723d..fc6f988 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -821,8 +821,6 @@ bool intel_ddi_pll_select(struct intel_crtc *intel_crtc)
>>   struct intel_encoder *intel_encoder = intel_ddi_get_crtc_encoder(crtc);
>>   int clock = intel_crtc->config.port_clock;
>>
>> - intel_put_shared_dpll(intel_crtc);
>> -
>>   return hsw_ddi_pll_select(intel_crtc, intel_encoder, clock);
>>  }
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 46df2db..4a8aff7 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -3852,15 +3852,9 @@ void intel_put_shared_dpll(struct intel_crtc *crtc)
>>  struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc)
>>  {
>>   struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
>> - struct intel_shared_dpll *pll = intel_crtc_to_shared_dpll(crtc);
>> + struct intel_shared_dpll *pll;
>>   enum intel_dpll_id i;
>>
>> - if (pll) {
>> - DRM_DEBUG_KMS("CRTC:%d dropping existing %s\n",
>> -      crtc->base.base.id, pll->name);
>> - intel_put_shared_dpll(crtc);
>> - }
>> -
>>   if (HAS_PCH_IBX(dev_priv->dev)) {
>>   /* Ironlake PCH has a fixed PLL->PCH pipe mapping. */
>>   i = (enum intel_dpll_id) crtc->pipe;
>> @@ -3869,7 +3863,7 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc)
>>   DRM_DEBUG_KMS("CRTC:%d using pre-allocated %s\n",
>>        crtc->base.base.id, pll->name);
>>
>> - WARN_ON(pll->crtc_mask);
>> + WARN_ON(pll->staged_config.crtc_mask);
>>
>>   goto found;
>>   }
>> @@ -3878,16 +3872,16 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc)
>>   pll = &dev_priv->shared_dplls[i];
>>
>>   /* Only want to check enabled timings first */
>> - if (pll->crtc_mask == 0)
>> + if (pll->staged_config.crtc_mask == 0)
>>   continue;
>>
>> - if (memcmp(&crtc->config.dpll_hw_state, &pll->hw_state,
>> -   sizeof(pll->hw_state)) == 0) {
>> - DRM_DEBUG_KMS("CRTC:%d sharing existing %s "
>> -      "(crtc_mask 0x%08x, active %d)\n",
>> + if (memcmp(&crtc->new_config->dpll_hw_state,
>> +   &pll->staged_config.hw_state,
>> +   sizeof(pll->staged_config.hw_state)) == 0) {
>> + DRM_DEBUG_KMS("CRTC:%d sharing existing %s (crtc mask 0x%08x, ative %d)\n",
>>        crtc->base.base.id, pll->name,
>> -      pll->crtc_mask, pll->active);
>> -
>> +      pll->staged_config.crtc_mask,
>> +      pll->active);
>>   goto found;
>>   }
>>   }
>> @@ -3895,7 +3889,7 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc)
>>   /* Ok no matching timings, maybe there's a free one? */
>>   for (i = 0; i < dev_priv->num_shared_dpll; i++) {
>>   pll = &dev_priv->shared_dplls[i];
>> - if (pll->crtc_mask == 0) {
>> + if (pll->staged_config.crtc_mask == 0) {
>>   DRM_DEBUG_KMS("CRTC:%d allocated %s\n",
>>        crtc->base.base.id, pll->name);
>>   goto found;
>> @@ -3905,18 +3899,53 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc)
>>   return NULL;
>>
>>  found:
>> - if (pll->crtc_mask == 0)
>> - pll->hw_state = crtc->config.dpll_hw_state;
>> + if (pll->staged_config.crtc_mask == 0)
>> + pll->staged_config.hw_state = crtc->new_config->dpll_hw_state;
>>
>> - crtc->config.shared_dpll = i;
>> + crtc->new_config->shared_dpll = i;
>>   DRM_DEBUG_DRIVER("using %s for pipe %c\n", pll->name,
>>   pipe_name(crtc->pipe));
>>
>> - pll->crtc_mask |= 1 << crtc->pipe;
>> + pll->staged_config.crtc_mask |= 1 << crtc->pipe;
>>
>>   return pll;
>>  }
>>
>> +/**
>> + * intel_shared_dpll_start_config - start a new PLL staged config
>> + * @dev_priv: DRM device
>> + * @clear_pipes: mask of pipes that will have their PLLs freed
>> + *
>> + * Starts a new PLL staged config, copying the current config but
>> + * releasing the references of pipes specified in clear_pipes.
>> + */
>> +static void intel_shared_dpll_start_config(struct drm_i915_private *dev_priv,
>> +   unsigned clear_pipes)
>> +{
>> + struct intel_shared_dpll *pll;
>> + enum intel_dpll_id i;
>> +
>> + for (i = 0; i < dev_priv->num_shared_dpll; i++) {
>> + pll = &dev_priv->shared_dplls[i];
>> +
>> + pll->staged_config.crtc_mask = pll->crtc_mask & ~clear_pipes;
>> + pll->staged_config.hw_state = pll->hw_state;
>> + }
>> +}
>> +
>> +static void intel_shared_dpll_commit(struct drm_i915_private *dev_priv)
>> +{
>> + struct intel_shared_dpll *pll;
>> + enum intel_dpll_id i;
>> +
>> + for (i = 0; i < dev_priv->num_shared_dpll; i++) {
>> + pll = &dev_priv->shared_dplls[i];
>> +
>> + pll->hw_state = pll->staged_config.hw_state;
>> + pll->crtc_mask = pll->staged_config.crtc_mask;
>> + }
>> +}
>> +
>>  static void cpt_verify_modeset(struct drm_device *dev, int pipe)
>>  {
>>   struct drm_i915_private *dev_priv = dev->dev_private;
>> @@ -6272,6 +6301,11 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
>>        int x, int y,
>>        struct drm_framebuffer *fb)
>>  {
>> + return 0;
>> +}
>> +
>> +static int i9xx_crtc_compute_clock(struct drm_crtc *crtc)
>> +{
>>   struct drm_device *dev = crtc->dev;
>>   struct drm_i915_private *dev_priv = dev->dev_private;
>>   struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>> @@ -7281,9 +7315,7 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc,
>>   return dpll | DPLL_VCO_ENABLE;
>>  }
>>
>> -static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>> -  int x, int y,
>> -  struct drm_framebuffer *fb)
>> +static int ironlake_crtc_compute_clock(struct drm_crtc *crtc)
>>  {
>>   struct drm_device *dev = crtc->dev;
>>   struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>> @@ -7336,8 +7368,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>>   pipe_name(intel_crtc->pipe));
>>   return -EINVAL;
>>   }
>> - } else
>> - intel_put_shared_dpll(intel_crtc);
>> + }
>>
>>   if (is_lvds && has_reduced_clock && i915.powersave)
>>   intel_crtc->lowfreq_avail = true;
>> @@ -7347,6 +7378,13 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>>   return 0;
>>  }
>>
>> +static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>> +  int x, int y,
>> +  struct drm_framebuffer *fb)
>> +{
>> + return 0;
>> +}
>> +
>>  static void intel_pch_transcoder_get_m_n(struct intel_crtc *crtc,
>>   struct intel_link_m_n *m_n)
>>  {
>> @@ -7836,9 +7874,7 @@ static void haswell_modeset_global_resources(struct drm_device *dev)
>>   modeset_update_crtc_power_domains(dev);
>>  }
>>
>> -static int haswell_crtc_mode_set(struct drm_crtc *crtc,
>> - int x, int y,
>> - struct drm_framebuffer *fb)
>> +static int haswell_crtc_compute_clock(struct drm_crtc *crtc)
>>  {
>>   struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>>
>> @@ -7850,6 +7886,13 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
>>   return 0;
>>  }
>>
>> +static int haswell_crtc_mode_set(struct drm_crtc *crtc,
>> + int x, int y,
>> + struct drm_framebuffer *fb)
>> +{
>> + return 0;
>> +}
>> +
>>  static void haswell_get_ddi_pll(struct drm_i915_private *dev_priv,
>>   enum port port,
>>   struct intel_crtc_config *pipe_config)
>> @@ -10988,6 +11031,14 @@ static int __intel_set_mode(struct drm_crtc *crtc,
>>   prepare_pipes &= ~disable_pipes;
>>   }
>>
>> + intel_shared_dpll_start_config(dev_priv, modeset_pipes | disable_pipes);
>> +
>> + for_each_intel_crtc_masked(dev, modeset_pipes, intel_crtc) {
>> + ret = dev_priv->display.crtc_compute_clock(&intel_crtc->base);
>> + if (ret)
>> + goto done;
>> + }
>> +
>>   for_each_intel_crtc_masked(dev, disable_pipes, intel_crtc)
>>   intel_crtc_disable(&intel_crtc->base);
>>
>> @@ -11015,6 +11066,8 @@ static int __intel_set_mode(struct drm_crtc *crtc,
>>   &pipe_config->adjusted_mode);
>>   }
>>
>> + intel_shared_dpll_commit(dev_priv);
>> +
>>   /* Only after disabling all output pipelines that will be changed can we
>>   * update the the output configuration. */
>>   intel_modeset_update_state(dev, prepare_pipes);
>> @@ -12580,6 +12633,7 @@ static void intel_init_display(struct drm_device *dev)
>>   if (HAS_DDI(dev)) {
>>   dev_priv->display.get_pipe_config = haswell_get_pipe_config;
>>   dev_priv->display.get_plane_config = ironlake_get_plane_config;
>> + dev_priv->display.crtc_compute_clock = haswell_crtc_compute_clock;
>>   dev_priv->display.crtc_mode_set = haswell_crtc_mode_set;
>>   dev_priv->display.crtc_enable = haswell_crtc_enable;
>>   dev_priv->display.crtc_disable = haswell_crtc_disable;
>> @@ -12593,6 +12647,7 @@ static void intel_init_display(struct drm_device *dev)
>>   } else if (HAS_PCH_SPLIT(dev)) {
>>   dev_priv->display.get_pipe_config = ironlake_get_pipe_config;
>>   dev_priv->display.get_plane_config = ironlake_get_plane_config;
>> + dev_priv->display.crtc_compute_clock = ironlake_crtc_compute_clock;
>>   dev_priv->display.crtc_mode_set = ironlake_crtc_mode_set;
>>   dev_priv->display.crtc_enable = ironlake_crtc_enable;
>>   dev_priv->display.crtc_disable = ironlake_crtc_disable;
>> @@ -12602,6 +12657,7 @@ static void intel_init_display(struct drm_device *dev)
>>   } else if (IS_VALLEYVIEW(dev)) {
>>   dev_priv->display.get_pipe_config = i9xx_get_pipe_config;
>>   dev_priv->display.get_plane_config = i9xx_get_plane_config;
>> + dev_priv->display.crtc_compute_clock = i9xx_crtc_compute_clock;
>>   dev_priv->display.crtc_mode_set = i9xx_crtc_mode_set;
>>   dev_priv->display.crtc_enable = valleyview_crtc_enable;
>>   dev_priv->display.crtc_disable = i9xx_crtc_disable;
>> @@ -12611,6 +12667,7 @@ static void intel_init_display(struct drm_device *dev)
>>   } else {
>>   dev_priv->display.get_pipe_config = i9xx_get_pipe_config;
>>   dev_priv->display.get_plane_config = i9xx_get_plane_config;
>> + dev_priv->display.crtc_compute_clock = i9xx_crtc_compute_clock;
>>   dev_priv->display.crtc_mode_set = i9xx_crtc_mode_set;
>>   dev_priv->display.crtc_enable = i9xx_crtc_enable;
>>   dev_priv->display.crtc_disable = i9xx_crtc_disable;
>> --
>> 1.8.3.2
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 2/4] drm/i915: Make *_crtc_mode_set work on new_config

[Daniel Vetter]

On Wed, Oct 08, 2014 at 06:32:21PM +0300, Ander Conselvan de Oliveira wrote:
> From: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
>
> This shouldn't change the behavior of those functions, since they are
> called after the new_config is made effective and that points to the
> current config. In a follow up patch, the mode set sequence will be
> changed so this is called before disabling crtcs, and in that case
> those functions should work on the staged config.
>
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c     |   4 +-
>  drivers/gpu/drm/i915/intel_display.c | 167 +++++++++++++++++++++--------------
>  2 files changed, 101 insertions(+), 70 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index cb5367c..619723d 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -792,7 +792,7 @@ hsw_ddi_pll_select(struct intel_crtc *intel_crtc,
>        WRPLL_DIVIDER_REFERENCE(r2) | WRPLL_DIVIDER_FEEDBACK(n2) |
>        WRPLL_DIVIDER_POST(p);
>
> - intel_crtc->config.dpll_hw_state.wrpll = val;
> + intel_crtc->new_config->dpll_hw_state.wrpll = val;
>
>   pll = intel_get_shared_dpll(intel_crtc);
>   if (pll == NULL) {
> @@ -801,7 +801,7 @@ hsw_ddi_pll_select(struct intel_crtc *intel_crtc,
>   return false;
>   }
>
> - intel_crtc->config.ddi_pll_sel = PORT_CLK_SEL_WRPLL(pll->id);
> + intel_crtc->new_config->ddi_pll_sel = PORT_CLK_SEL_WRPLL(pll->id);
>   }
>
>   return true;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 3a06c3d..9d8fe8d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -420,13 +420,32 @@ static bool intel_pipe_has_type(struct drm_crtc *crtc, int type)
>   return false;
>  }
>
> +/**
> + * Returns whether any output on the specified pipe will have the specified
> + * type after a staged modeset is complete, i.e., the same as
> + * intel_pipe_has_type() but looking at encoder->new_crtc instead of
> + * encoder->crtc.
> + */
> +static bool intel_pipe_will_have_type(struct drm_crtc *crtc, int type)

s/drm_crtc/intel_crtc/. In general we use the most specific type
for internal functions to avoid tons of upcasting. So you might want to
throw a patch on top to convert the existing has_type for consistency.
-Daniel

> +{
> + struct drm_device *dev = crtc->dev;
> + struct intel_encoder *encoder;
> +
> + for_each_intel_encoder(dev, encoder)
> + if (encoder->new_crtc == to_intel_crtc(crtc) &&
> +    encoder->type == type)
> + return true;
> +
> + return false;
> +}
> +
>  static const intel_limit_t *intel_ironlake_limit(struct drm_crtc *crtc,
>   int refclk)
>  {
>   struct drm_device *dev = crtc->dev;
>   const intel_limit_t *limit;
>
> - if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS)) {
> + if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS)) {
>   if (intel_is_dual_link_lvds(dev)) {
>   if (refclk == 100000)
>   limit = &intel_limits_ironlake_dual_lvds_100m;
> @@ -449,15 +468,15 @@ static const intel_limit_t *intel_g4x_limit(struct drm_crtc *crtc)
>   struct drm_device *dev = crtc->dev;
>   const intel_limit_t *limit;
>
> - if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS)) {
> + if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS)) {
>   if (intel_is_dual_link_lvds(dev))
>   limit = &intel_limits_g4x_dual_channel_lvds;
>   else
>   limit = &intel_limits_g4x_single_channel_lvds;
> - } else if (intel_pipe_has_type(crtc, INTEL_OUTPUT_HDMI) ||
> -   intel_pipe_has_type(crtc, INTEL_OUTPUT_ANALOG)) {
> + } else if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_HDMI) ||
> +   intel_pipe_will_have_type(crtc, INTEL_OUTPUT_ANALOG)) {
>   limit = &intel_limits_g4x_hdmi;
> - } else if (intel_pipe_has_type(crtc, INTEL_OUTPUT_SDVO)) {
> + } else if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_SDVO)) {
>   limit = &intel_limits_g4x_sdvo;
>   } else /* The option is for other outputs */
>   limit = &intel_limits_i9xx_sdvo;
> @@ -475,7 +494,7 @@ static const intel_limit_t *intel_limit(struct drm_crtc *crtc, int refclk)
>   else if (IS_G4X(dev)) {
>   limit = intel_g4x_limit(crtc);
>   } else if (IS_PINEVIEW(dev)) {
> - if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS))
> + if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS))
>   limit = &intel_limits_pineview_lvds;
>   else
>   limit = &intel_limits_pineview_sdvo;
> @@ -484,14 +503,14 @@ static const intel_limit_t *intel_limit(struct drm_crtc *crtc, int refclk)
>   } else if (IS_VALLEYVIEW(dev)) {
>   limit = &intel_limits_vlv;
>   } else if (!IS_GEN2(dev)) {
> - if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS))
> + if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS))
>   limit = &intel_limits_i9xx_lvds;
>   else
>   limit = &intel_limits_i9xx_sdvo;
>   } else {
> - if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS))
> + if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS))
>   limit = &intel_limits_i8xx_lvds;
> - else if (intel_pipe_has_type(crtc, INTEL_OUTPUT_DVO))
> + else if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_DVO))
>   limit = &intel_limits_i8xx_dvo;
>   else
>   limit = &intel_limits_i8xx_dac;
> @@ -586,7 +605,7 @@ i9xx_find_best_dpll(const intel_limit_t *limit, struct drm_crtc *crtc,
>   intel_clock_t clock;
>   int err = target;
>
> - if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS)) {
> + if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS)) {
>   /*
>   * For LVDS just rely on its current settings for dual-channel.
>   * We haven't figured out how to reliably set up different
> @@ -647,7 +666,7 @@ pnv_find_best_dpll(const intel_limit_t *limit, struct drm_crtc *crtc,
>   intel_clock_t clock;
>   int err = target;
>
> - if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS)) {
> + if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS)) {
>   /*
>   * For LVDS just rely on its current settings for dual-channel.
>   * We haven't figured out how to reliably set up different
> @@ -710,7 +729,7 @@ g4x_find_best_dpll(const intel_limit_t *limit, struct drm_crtc *crtc,
>   int err_most = (target >> 8) + (target >> 9);
>   found = false;
>
> - if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS)) {
> + if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS)) {
>   if (intel_is_dual_link_lvds(dev))
>   clock.p2 = limit->p2.p2_fast;
>   else
> @@ -5607,7 +5626,7 @@ static int i9xx_get_refclk(struct drm_crtc *crtc, int num_connectors)
>
>   if (IS_VALLEYVIEW(dev)) {
>   refclk = 100000;
> - } else if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS) &&
> + } else if (intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS) &&
>      intel_panel_use_ssc(dev_priv) && num_connectors < 2) {
>   refclk = dev_priv->vbt.lvds_ssc_freq;
>   DRM_DEBUG_KMS("using SSC reference clock of %d kHz\n", refclk);
> @@ -5758,11 +5777,11 @@ static void vlv_update_pll(struct intel_crtc *crtc)
>   if (crtc->pipe == PIPE_B)
>   dpll |= DPLL_INTEGRATED_CRI_CLK_VLV;
>   dpll |= DPLL_VCO_ENABLE;
> - crtc->config.dpll_hw_state.dpll = dpll;
> + crtc->new_config->dpll_hw_state.dpll = dpll;
>
> - dpll_md = (crtc->config.pixel_multiplier - 1)
> + dpll_md = (crtc->new_config->pixel_multiplier - 1)
>   << DPLL_MD_UDI_MULTIPLIER_SHIFT;
> - crtc->config.dpll_hw_state.dpll_md = dpll_md;
> + crtc->new_config->dpll_hw_state.dpll_md = dpll_md;
>  }
>
>  static void vlv_prepare_pll(struct intel_crtc *crtc)
> @@ -5858,14 +5877,16 @@ static void vlv_prepare_pll(struct intel_crtc *crtc)
>
>  static void chv_update_pll(struct intel_crtc *crtc)
>  {
> - crtc->config.dpll_hw_state.dpll = DPLL_SSC_REF_CLOCK_CHV |
> + crtc->new_config->dpll_hw_state.dpll = DPLL_SSC_REF_CLOCK_CHV |
>   DPLL_REFA_CLK_ENABLE_VLV | DPLL_VGA_MODE_DIS |
>   DPLL_VCO_ENABLE;
>   if (crtc->pipe != PIPE_A)
> - crtc->config.dpll_hw_state.dpll |= DPLL_INTEGRATED_CRI_CLK_VLV;
> + crtc->new_config->dpll_hw_state.dpll |=
> + DPLL_INTEGRATED_CRI_CLK_VLV;
>
> - crtc->config.dpll_hw_state.dpll_md =
> - (crtc->config.pixel_multiplier - 1) << DPLL_MD_UDI_MULTIPLIER_SHIFT;
> + crtc->new_config->dpll_hw_state.dpll_md =
> + (crtc->new_config->pixel_multiplier - 1)
> + << DPLL_MD_UDI_MULTIPLIER_SHIFT;
>  }
>
>  static void chv_prepare_pll(struct intel_crtc *crtc)
> @@ -5946,29 +5967,29 @@ static void i9xx_update_pll(struct intel_crtc *crtc,
>   struct drm_i915_private *dev_priv = dev->dev_private;
>   u32 dpll;
>   bool is_sdvo;
> - struct dpll *clock = &crtc->config.dpll;
> + struct dpll *clock = &crtc->new_config->dpll;
>
>   i9xx_update_pll_dividers(crtc, reduced_clock);
>
> - is_sdvo = intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_SDVO) ||
> - intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_HDMI);
> + is_sdvo = intel_pipe_will_have_type(&crtc->base, INTEL_OUTPUT_SDVO) ||
> + intel_pipe_will_have_type(&crtc->base, INTEL_OUTPUT_HDMI);
>
>   dpll = DPLL_VGA_MODE_DIS;
>
> - if (intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_LVDS))
> + if (intel_pipe_will_have_type(&crtc->base, INTEL_OUTPUT_LVDS))
>   dpll |= DPLLB_MODE_LVDS;
>   else
>   dpll |= DPLLB_MODE_DAC_SERIAL;
>
>   if (IS_I945G(dev) || IS_I945GM(dev) || IS_G33(dev)) {
> - dpll |= (crtc->config.pixel_multiplier - 1)
> + dpll |= (crtc->new_config->pixel_multiplier - 1)
>   << SDVO_MULTIPLIER_SHIFT_HIRES;
>   }
>
>   if (is_sdvo)
>   dpll |= DPLL_SDVO_HIGH_SPEED;
>
> - if (intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_DISPLAYPORT))
> + if (intel_pipe_will_have_type(&crtc->base, INTEL_OUTPUT_DISPLAYPORT))
>   dpll |= DPLL_SDVO_HIGH_SPEED;
>
>   /* compute bitmask from p1 value */
> @@ -5996,21 +6017,21 @@ static void i9xx_update_pll(struct intel_crtc *crtc,
>   if (INTEL_INFO(dev)->gen >= 4)
>   dpll |= (6 << PLL_LOAD_PULSE_PHASE_SHIFT);
>
> - if (crtc->config.sdvo_tv_clock)
> + if (crtc->new_config->sdvo_tv_clock)
>   dpll |= PLL_REF_INPUT_TVCLKINBC;
> - else if (intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_LVDS) &&
> + else if (intel_pipe_will_have_type(&crtc->base, INTEL_OUTPUT_LVDS) &&
>   intel_panel_use_ssc(dev_priv) && num_connectors < 2)
>   dpll |= PLLB_REF_INPUT_SPREADSPECTRUMIN;
>   else
>   dpll |= PLL_REF_INPUT_DREFCLK;
>
>   dpll |= DPLL_VCO_ENABLE;
> - crtc->config.dpll_hw_state.dpll = dpll;
> + crtc->new_config->dpll_hw_state.dpll = dpll;
>
>   if (INTEL_INFO(dev)->gen >= 4) {
> - u32 dpll_md = (crtc->config.pixel_multiplier - 1)
> + u32 dpll_md = (crtc->new_config->pixel_multiplier - 1)
>   << DPLL_MD_UDI_MULTIPLIER_SHIFT;
> - crtc->config.dpll_hw_state.dpll_md = dpll_md;
> + crtc->new_config->dpll_hw_state.dpll_md = dpll_md;
>   }
>  }
>
> @@ -6021,13 +6042,13 @@ static void i8xx_update_pll(struct intel_crtc *crtc,
>   struct drm_device *dev = crtc->base.dev;
>   struct drm_i915_private *dev_priv = dev->dev_private;
>   u32 dpll;
> - struct dpll *clock = &crtc->config.dpll;
> + struct dpll *clock = &crtc->new_config->dpll;
>
>   i9xx_update_pll_dividers(crtc, reduced_clock);
>
>   dpll = DPLL_VGA_MODE_DIS;
>
> - if (intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_LVDS)) {
> + if (intel_pipe_will_have_type(&crtc->base, INTEL_OUTPUT_LVDS)) {
>   dpll |= (1 << (clock->p1 - 1)) << DPLL_FPA01_P1_POST_DIV_SHIFT;
>   } else {
>   if (clock->p1 == 2)
> @@ -6038,17 +6059,18 @@ static void i8xx_update_pll(struct intel_crtc *crtc,
>   dpll |= PLL_P2_DIVIDE_BY_4;
>   }
>
> - if (!IS_I830(dev) && intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_DVO))
> + if (!IS_I830(dev) &&
> +    intel_pipe_will_have_type(&crtc->base, INTEL_OUTPUT_DVO))
>   dpll |= DPLL_DVO_2X_MODE;
>
> - if (intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_LVDS) &&
> + if (intel_pipe_will_have_type(&crtc->base, INTEL_OUTPUT_LVDS) &&
>   intel_panel_use_ssc(dev_priv) && num_connectors < 2)
>   dpll |= PLLB_REF_INPUT_SPREADSPECTRUMIN;
>   else
>   dpll |= PLL_REF_INPUT_DREFCLK;
>
>   dpll |= DPLL_VCO_ENABLE;
> - crtc->config.dpll_hw_state.dpll = dpll;
> + crtc->new_config->dpll_hw_state.dpll = dpll;
>  }
>
>  static void intel_set_pipe_timings(struct intel_crtc *intel_crtc)
> @@ -6258,7 +6280,10 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
>   struct intel_encoder *encoder;
>   const intel_limit_t *limit;
>
> - for_each_encoder_on_crtc(dev, crtc, encoder) {
> + for_each_intel_encoder(dev, encoder) {
> + if (encoder->new_crtc != to_intel_crtc(crtc))
> + continue;
> +
>   switch (encoder->type) {
>   case INTEL_OUTPUT_LVDS:
>   is_lvds = true;
> @@ -6274,7 +6299,7 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
>   if (is_dsi)
>   return 0;
>
> - if (!intel_crtc->config.clock_set) {
> + if (!intel_crtc->new_config->clock_set) {
>   refclk = i9xx_get_refclk(crtc, num_connectors);
>
>   /*
> @@ -6285,7 +6310,7 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
>   */
>   limit = intel_limit(crtc, refclk);
>   ok = dev_priv->display.find_dpll(limit, crtc,
> - intel_crtc->config.port_clock,
> + intel_crtc->new_config->port_clock,
>   refclk, NULL, &clock);
>   if (!ok) {
>   DRM_ERROR("Couldn't find PLL settings for mode!\n");
> @@ -6306,11 +6331,11 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
>      &reduced_clock);
>   }
>   /* Compat-code for transition, will disappear. */
> - intel_crtc->config.dpll.n = clock.n;
> - intel_crtc->config.dpll.m1 = clock.m1;
> - intel_crtc->config.dpll.m2 = clock.m2;
> - intel_crtc->config.dpll.p1 = clock.p1;
> - intel_crtc->config.dpll.p2 = clock.p2;
> + intel_crtc->new_config->dpll.n = clock.n;
> + intel_crtc->new_config->dpll.m1 = clock.m1;
> + intel_crtc->new_config->dpll.m2 = clock.m2;
> + intel_crtc->new_config->dpll.p1 = clock.p1;
> + intel_crtc->new_config->dpll.p2 = clock.p2;
>   }
>
>   if (IS_GEN2(dev)) {
> @@ -6926,7 +6951,10 @@ static int ironlake_get_refclk(struct drm_crtc *crtc)
>   int num_connectors = 0;
>   bool is_lvds = false;
>
> - for_each_encoder_on_crtc(dev, crtc, encoder) {
> + for_each_intel_encoder(dev, encoder) {
> + if (encoder->new_crtc != to_intel_crtc(crtc))
> + continue;
> +
>   switch (encoder->type) {
>   case INTEL_OUTPUT_LVDS:
>   is_lvds = true;
> @@ -7114,7 +7142,7 @@ static bool ironlake_compute_clocks(struct drm_crtc *crtc,
>   const intel_limit_t *limit;
>   bool ret, is_lvds = false;
>
> - is_lvds = intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS);
> + is_lvds = intel_pipe_will_have_type(crtc, INTEL_OUTPUT_LVDS);
>
>   refclk = ironlake_get_refclk(crtc);
>
> @@ -7125,7 +7153,7 @@ static bool ironlake_compute_clocks(struct drm_crtc *crtc,
>   */
>   limit = intel_limit(crtc, refclk);
>   ret = dev_priv->display.find_dpll(limit, crtc,
> -  to_intel_crtc(crtc)->config.port_clock,
> +  to_intel_crtc(crtc)->new_config->port_clock,
>    refclk, NULL, clock);
>   if (!ret)
>   return false;
> @@ -7175,7 +7203,10 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc,
>   int factor, num_connectors = 0;
>   bool is_lvds = false, is_sdvo = false;
>
> - for_each_encoder_on_crtc(dev, crtc, intel_encoder) {
> + for_each_intel_encoder(dev, intel_encoder) {
> + if (intel_encoder->new_crtc != to_intel_crtc(crtc))
> + continue;
> +
>   switch (intel_encoder->type) {
>   case INTEL_OUTPUT_LVDS:
>   is_lvds = true;
> @@ -7196,10 +7227,10 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc,
>       dev_priv->vbt.lvds_ssc_freq == 100000) ||
>      (HAS_PCH_IBX(dev) && intel_is_dual_link_lvds(dev)))
>   factor = 25;
> - } else if (intel_crtc->config.sdvo_tv_clock)
> + } else if (intel_crtc->new_config->sdvo_tv_clock)
>   factor = 20;
>
> - if (ironlake_needs_fb_cb_tune(&intel_crtc->config.dpll, factor))
> + if (ironlake_needs_fb_cb_tune(&intel_crtc->new_config->dpll, factor))
>   *fp |= FP_CB_TUNE;
>
>   if (fp2 && (reduced_clock->m < factor * reduced_clock->n))
> @@ -7212,20 +7243,20 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc,
>   else
>   dpll |= DPLLB_MODE_DAC_SERIAL;
>
> - dpll |= (intel_crtc->config.pixel_multiplier - 1)
> + dpll |= (intel_crtc->new_config->pixel_multiplier - 1)
>   << PLL_REF_SDVO_HDMI_MULTIPLIER_SHIFT;
>
>   if (is_sdvo)
>   dpll |= DPLL_SDVO_HIGH_SPEED;
> - if (intel_crtc->config.has_dp_encoder)
> + if (intel_crtc->new_config->has_dp_encoder)
>   dpll |= DPLL_SDVO_HIGH_SPEED;
>
>   /* compute bitmask from p1 value */
> - dpll |= (1 << (intel_crtc->config.dpll.p1 - 1)) << DPLL_FPA01_P1_POST_DIV_SHIFT;
> + dpll |= (1 << (intel_crtc->new_config->dpll.p1 - 1)) << DPLL_FPA01_P1_POST_DIV_SHIFT;
>   /* also FPA1 */
> - dpll |= (1 << (intel_crtc->config.dpll.p1 - 1)) << DPLL_FPA1_P1_POST_DIV_SHIFT;
> + dpll |= (1 << (intel_crtc->new_config->dpll.p1 - 1)) << DPLL_FPA1_P1_POST_DIV_SHIFT;
>
> - switch (intel_crtc->config.dpll.p2) {
> + switch (intel_crtc->new_config->dpll.p2) {
>   case 5:
>   dpll |= DPLL_DAC_SERIAL_P2_CLOCK_DIV_5;
>   break;
> @@ -7267,22 +7298,22 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>
>   ok = ironlake_compute_clocks(crtc, &clock,
>       &has_reduced_clock, &reduced_clock);
> - if (!ok && !intel_crtc->config.clock_set) {
> + if (!ok && !intel_crtc->new_config->clock_set) {
>   DRM_ERROR("Couldn't find PLL settings for mode!\n");
>   return -EINVAL;
>   }
>   /* Compat-code for transition, will disappear. */
> - if (!intel_crtc->config.clock_set) {
> - intel_crtc->config.dpll.n = clock.n;
> - intel_crtc->config.dpll.m1 = clock.m1;
> - intel_crtc->config.dpll.m2 = clock.m2;
> - intel_crtc->config.dpll.p1 = clock.p1;
> - intel_crtc->config.dpll.p2 = clock.p2;
> + if (!intel_crtc->new_config->clock_set) {
> + intel_crtc->new_config->dpll.n = clock.n;
> + intel_crtc->new_config->dpll.m1 = clock.m1;
> + intel_crtc->new_config->dpll.m2 = clock.m2;
> + intel_crtc->new_config->dpll.p1 = clock.p1;
> + intel_crtc->new_config->dpll.p2 = clock.p2;
>   }
>
>   /* CPU eDP is the only output that doesn't need a PCH PLL of its own. */
> - if (intel_crtc->config.has_pch_encoder) {
> - fp = i9xx_dpll_compute_fp(&intel_crtc->config.dpll);
> + if (intel_crtc->new_config->has_pch_encoder) {
> + fp = i9xx_dpll_compute_fp(&intel_crtc->new_config->dpll);
>   if (has_reduced_clock)
>   fp2 = i9xx_dpll_compute_fp(&reduced_clock);
>
> @@ -7290,12 +7321,12 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>       &fp, &reduced_clock,
>       has_reduced_clock ? &fp2 : NULL);
>
> - intel_crtc->config.dpll_hw_state.dpll = dpll;
> - intel_crtc->config.dpll_hw_state.fp0 = fp;
> + intel_crtc->new_config->dpll_hw_state.dpll = dpll;
> + intel_crtc->new_config->dpll_hw_state.fp0 = fp;
>   if (has_reduced_clock)
> - intel_crtc->config.dpll_hw_state.fp1 = fp2;
> + intel_crtc->new_config->dpll_hw_state.fp1 = fp2;
>   else
> - intel_crtc->config.dpll_hw_state.fp1 = fp;
> + intel_crtc->new_config->dpll_hw_state.fp1 = fp;
>
>   pll = intel_get_shared_dpll(intel_crtc);
>   if (pll == NULL) {
> --
> 1.8.3.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/4] drm/i915: Make *_crtc_mode_set work on new_config

[Ander Conselvan de Oliveira]

On 10/09/2014 12:11 PM, Daniel Vetter wrote:
> On Wed, Oct 08, 2014 at 06:32:21PM +0300, Ander Conselvan de Oliveira wrote:
>> From: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
>>
>> This shouldn't change the behavior of those functions, since they are
>> called after the new_config is made effective and that points to the
>> current config. In a follow up patch, the mode set sequence will be
>> changed so this is called before disabling crtcs, and in that case
>> those functions should work on the staged config.
>>
>> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
>> ---

[snip]

>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 3a06c3d..9d8fe8d 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -420,13 +420,32 @@ static bool intel_pipe_has_type(struct drm_crtc *crtc, int type)
>>    return false;
>>   }
>>
>> +/**
>> + * Returns whether any output on the specified pipe will have the specified
>> + * type after a staged modeset is complete, i.e., the same as
>> + * intel_pipe_has_type() but looking at encoder->new_crtc instead of
>> + * encoder->crtc.
>> + */
>> +static bool intel_pipe_will_have_type(struct drm_crtc *crtc, int type)
>
> s/drm_crtc/intel_crtc/. In general we use the most specific type
> for internal functions to avoid tons of upcasting. So you might want to
> throw a patch on top to convert the existing has_type for consistency.

What about the functions in drm_i915_display_funcs (find_dpll, 
crtc_mode_set)? Are these not considered internal functions or is this 
leftover from before this rule was in place?

If I write that consistency patch it might be easier to just convert 
everything.

Ander
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [PATCH 2/4] drm/i915: Make *_crtc_mode_set work on new_config

[Daniel Vetter]

On Thu, Oct 09, 2014 at 03:18:03PM +0300, Ander Conselvan de Oliveira wrote:
> On 10/09/2014 12:11 PM, Daniel Vetter wrote:
> >On Wed, Oct 08, 2014 at 06:32:21PM +0300, Ander Conselvan de Oliveira wrote:
> >>From: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> >>
> >>This shouldn't change the behavior of those functions, since they are
> >>called after the new_config is made effective and that points to the
> >>current config. In a follow up patch, the mode set sequence will be
> >>changed so this is called before disabling crtcs, and in that case
> >>those functions should work on the staged config.
> >>
> >>Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> >>---
> 
> [snip]
> 
> >>diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >>index 3a06c3d..9d8fe8d 100644
> >>--- a/drivers/gpu/drm/i915/intel_display.c
> >>+++ b/drivers/gpu/drm/i915/intel_display.c
> >>@@ -420,13 +420,32 @@ static bool intel_pipe_has_type(struct drm_crtc *crtc, int type)
> >>   return false;
> >>  }
> >>
> >>+/**
> >>+ * Returns whether any output on the specified pipe will have the specified
> >>+ * type after a staged modeset is complete, i.e., the same as
> >>+ * intel_pipe_has_type() but looking at encoder->new_crtc instead of
> >>+ * encoder->crtc.
> >>+ */
> >>+static bool intel_pipe_will_have_type(struct drm_crtc *crtc, int type)
> >
> >s/drm_crtc/intel_crtc/. In general we use the most specific type
> >for internal functions to avoid tons of upcasting. So you might want to
> >throw a patch on top to convert the existing has_type for consistency.
> 
> What about the functions in drm_i915_display_funcs (find_dpll,
> crtc_mode_set)? Are these not considered internal functions or is this
> leftover from before this rule was in place?
> 
> If I write that consistency patch it might be easier to just convert
> everything.

leftovers, and we've been slowly converting existing code over to using
intel structures over the past 2 years. I certainly won't stop you to
clean up all that stuff, but it's a bit of work. So I'm ok if you just
have the vfunc signature use the intel_ types and then immediately convert
to a drm_ type again to avoid code churn.

Or just convert it all if you feel like it.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/4] drm/i915: Make *_crtc_mode_set work on new_config

[Daniel Vetter]

On Sun, Oct 19, 2014 at 04:28:57PM +0200, Daniel Vetter wrote:
> On Thu, Oct 09, 2014 at 03:18:03PM +0300, Ander Conselvan de Oliveira wrote:
> > On 10/09/2014 12:11 PM, Daniel Vetter wrote:
> > >On Wed, Oct 08, 2014 at 06:32:21PM +0300, Ander Conselvan de Oliveira wrote:
> > >>From: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> > >>
> > >>This shouldn't change the behavior of those functions, since they are
> > >>called after the new_config is made effective and that points to the
> > >>current config. In a follow up patch, the mode set sequence will be
> > >>changed so this is called before disabling crtcs, and in that case
> > >>those functions should work on the staged config.
> > >>
> > >>Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> > >>---
> > 
> > [snip]
> > 
> > >>diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > >>index 3a06c3d..9d8fe8d 100644
> > >>--- a/drivers/gpu/drm/i915/intel_display.c
> > >>+++ b/drivers/gpu/drm/i915/intel_display.c
> > >>@@ -420,13 +420,32 @@ static bool intel_pipe_has_type(struct drm_crtc *crtc, int type)
> > >>   return false;
> > >>  }
> > >>
> > >>+/**
> > >>+ * Returns whether any output on the specified pipe will have the specified
> > >>+ * type after a staged modeset is complete, i.e., the same as
> > >>+ * intel_pipe_has_type() but looking at encoder->new_crtc instead of
> > >>+ * encoder->crtc.
> > >>+ */
> > >>+static bool intel_pipe_will_have_type(struct drm_crtc *crtc, int type)
> > >
> > >s/drm_crtc/intel_crtc/. In general we use the most specific type
> > >for internal functions to avoid tons of upcasting. So you might want to
> > >throw a patch on top to convert the existing has_type for consistency.
> > 
> > What about the functions in drm_i915_display_funcs (find_dpll,
> > crtc_mode_set)? Are these not considered internal functions or is this
> > leftover from before this rule was in place?
> > 
> > If I write that consistency patch it might be easier to just convert
> > everything.
> 
> leftovers, and we've been slowly converting existing code over to using
> intel structures over the past 2 years. I certainly won't stop you to
> clean up all that stuff, but it's a bit of work. So I'm ok if you just
> have the vfunc signature use the intel_ types and then immediately convert
> to a drm_ type again to avoid code churn.
> 
> Or just convert it all if you feel like it.

But if you do that please split out that rote conversion into a separate
patch for easier reviewing. Either at the start of the series (so I can
merge it right away) or at the end (only done once everything is merged)
to avoid needless rebase pain.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/4] drm/i915: Make *_crtc_mode_set work on new_config

[Ander Conselvan de Oliveira]

On 10/19/2014 05:30 PM, Daniel Vetter wrote:
> On Sun, Oct 19, 2014 at 04:28:57PM +0200, Daniel Vetter wrote:
>> On Thu, Oct 09, 2014 at 03:18:03PM +0300, Ander Conselvan de Oliveira wrote:
>>> On 10/09/2014 12:11 PM, Daniel Vetter wrote:
>>>> On Wed, Oct 08, 2014 at 06:32:21PM +0300, Ander Conselvan de Oliveira wrote:
>>>>> From: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
>>>>>
>>>>> This shouldn't change the behavior of those functions, since they are
>>>>> called after the new_config is made effective and that points to the
>>>>> current config. In a follow up patch, the mode set sequence will be
>>>>> changed so this is called before disabling crtcs, and in that case
>>>>> those functions should work on the staged config.
>>>>>
>>>>> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
>>>>> ---
>>>
>>> [snip]
>>>
>>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>>> index 3a06c3d..9d8fe8d 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>>> @@ -420,13 +420,32 @@ static bool intel_pipe_has_type(struct drm_crtc *crtc, int type)
>>>>>    return false;
>>>>>   }
>>>>>
>>>>> +/**
>>>>> + * Returns whether any output on the specified pipe will have the specified
>>>>> + * type after a staged modeset is complete, i.e., the same as
>>>>> + * intel_pipe_has_type() but looking at encoder->new_crtc instead of
>>>>> + * encoder->crtc.
>>>>> + */
>>>>> +static bool intel_pipe_will_have_type(struct drm_crtc *crtc, int type)
>>>>
>>>> s/drm_crtc/intel_crtc/. In general we use the most specific type
>>>> for internal functions to avoid tons of upcasting. So you might want to
>>>> throw a patch on top to convert the existing has_type for consistency.
>>>
>>> What about the functions in drm_i915_display_funcs (find_dpll,
>>> crtc_mode_set)? Are these not considered internal functions or is this
>>> leftover from before this rule was in place?
>>>
>>> If I write that consistency patch it might be easier to just convert
>>> everything.
>>
>> leftovers, and we've been slowly converting existing code over to using
>> intel structures over the past 2 years. I certainly won't stop you to
>> clean up all that stuff, but it's a bit of work. So I'm ok if you just
>> have the vfunc signature use the intel_ types and then immediately convert
>> to a drm_ type again to avoid code churn.
>>
>> Or just convert it all if you feel like it.
>
> But if you do that please split out that rote conversion into a separate
> patch for easier reviewing. Either at the start of the series (so I can
> merge it right away) or at the end (only done once everything is merged)
> to avoid needless rebase pain.

I went down the "covert all" route. I sent the prep patches as a 
separate series now.

Thanks,
Ander