[PATCH] drm/i915: hw state readout&check support for cpu_transcoder

[PATCH] drm/i915: hw state readout&check support for cpu_transcoder

[Daniel Vetter]

This allows us to drop a bunch of ugly hacks and finally implement
what

commit cc464b2a17c59adedbdc02cc54341d630354edc3
Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
Date:   Fri Jan 25 16:59:16 2013 -0200

    drm/i915: set TRANSCODER_EDP even earlier

tried to achieve, but that was reverted again in

commit bba2181c49f1dddf8b592804a1b53cc1a3cf408a
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Fri Mar 22 10:53:40 2013 +0100

    Revert "drm/i915: set TRANSCODER_EDP even earlier"

Now we should always have a consistent cpu_transcoder in the
pipe_config.

Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_ddi.c     |  4 ++
 drivers/gpu/drm/i915/intel_display.c | 90 +++++++++++++-----------------------
 2 files changed, 37 insertions(+), 57 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 3ff4de6..c1a5669 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1448,9 +1448,13 @@ static bool intel_ddi_compute_config(struct intel_encoder *encoder,
 				     struct intel_crtc_config *pipe_config)
 {
 	int type = encoder->type;
+	int port = intel_ddi_get_encoder_port(encoder);
 
 	WARN(type == INTEL_OUTPUT_UNKNOWN, "compute_config() on unknown output!\n");
 
+	if (port == PORT_A)
+		pipe_config->cpu_transcoder = TRANSCODER_EDP;
+
 	if (type == INTEL_OUTPUT_HDMI)
 		return intel_hdmi_compute_config(encoder, pipe_config);
 	else
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3b7fae1..ac9ad1a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3566,12 +3566,6 @@ static void ironlake_crtc_off(struct drm_crtc *crtc)
 
 static void haswell_crtc_off(struct drm_crtc *crtc)
 {
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-
-	/* Stop saying we're using TRANSCODER_EDP because some other CRTC might
-	 * start using it. */
-	intel_crtc->config.cpu_transcoder = (enum transcoder) intel_crtc->pipe;
-
 	intel_ddi_put_crtc_pll(crtc);
 }
 
@@ -5035,6 +5029,8 @@ static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	uint32_t tmp;
 
+	pipe_config->cpu_transcoder = crtc->pipe;
+
 	tmp = I915_READ(PIPECONF(crtc->pipe));
 	if (!(tmp & PIPECONF_ENABLE))
 		return false;
@@ -5786,8 +5782,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	WARN(!(HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)),
 	     "Unexpected PCH type %d\n", INTEL_PCH_TYPE(dev));
 
-	intel_crtc->config.cpu_transcoder = pipe;
-
 	ok = ironlake_compute_clocks(crtc, adjusted_mode, &clock,
 				     &has_reduced_clock, &reduced_clock);
 	if (!ok) {
@@ -5910,6 +5904,8 @@ static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	uint32_t tmp;
 
+	pipe_config->cpu_transcoder = crtc->pipe;
+
 	tmp = I915_READ(PIPECONF(crtc->pipe));
 	if (!(tmp & PIPECONF_ENABLE))
 		return false;
@@ -5985,11 +5981,6 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
 		num_connectors++;
 	}
 
-	if (is_cpu_edp)
-		intel_crtc->config.cpu_transcoder = TRANSCODER_EDP;
-	else
-		intel_crtc->config.cpu_transcoder = pipe;
-
 	/* We are not sure yet this won't happen. */
 	WARN(!HAS_PCH_LPT(dev), "Unexpected PCH type %d\n",
 	     INTEL_PCH_TYPE(dev));
@@ -6045,14 +6036,36 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
 {
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	enum transcoder cpu_transcoder = crtc->config.cpu_transcoder;
 	uint32_t tmp;
 
+	pipe_config->cpu_transcoder = crtc->pipe;
+	tmp = (TRANS_DDI_FUNC_CTL(TRANSCODER_EDP));
+	if (tmp & TRANS_DDI_FUNC_ENABLE) {
+		enum pipe trans_edp_pipe;
+		switch (tmp & TRANS_DDI_EDP_INPUT_MASK) {
+		default:
+			WARN(1, "unknown pipe linked to edp transcoder\n");
+		case TRANS_DDI_EDP_INPUT_A_ONOFF:
+		case TRANS_DDI_EDP_INPUT_A_ON:
+			trans_edp_pipe = PIPE_A;
+			break;
+		case TRANS_DDI_EDP_INPUT_B_ONOFF:
+			trans_edp_pipe = PIPE_A;
+			break;
+		case TRANS_DDI_EDP_INPUT_C_ONOFF:
+			trans_edp_pipe = PIPE_A;
+			break;
+		}
+
+		if (trans_edp_pipe == crtc->pipe)
+			pipe_config->cpu_transcoder = TRANSCODER_EDP;
+	}
+
 	if (!intel_using_power_well(dev_priv->dev) &&
-	    cpu_transcoder != TRANSCODER_EDP)
+	    pipe_config->cpu_transcoder != TRANSCODER_EDP)
 		return false;
 
-	tmp = I915_READ(PIPECONF(cpu_transcoder));
+	tmp = I915_READ(PIPECONF(pipe_config->cpu_transcoder));
 	if (!(tmp & PIPECONF_ENABLE))
 		return false;
 
@@ -6061,7 +6074,7 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
 	 * DDI E. So just check whether this pipe is wired to DDI E and whether
 	 * the PCH transcoder is on.
 	 */
-	tmp = I915_READ(TRANS_DDI_FUNC_CTL(cpu_transcoder));
+	tmp = I915_READ(TRANS_DDI_FUNC_CTL(pipe_config->cpu_transcoder));
 	if ((tmp & TRANS_DDI_PORT_MASK) == TRANS_DDI_SELECT_PORT(PORT_E) &&
 	    I915_READ(LPT_TRANSCONF) & TRANS_ENABLE) {
 		pipe_config->has_pch_encoder = true;
@@ -7812,6 +7825,7 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
 
 	drm_mode_copy(&pipe_config->adjusted_mode, mode);
 	drm_mode_copy(&pipe_config->requested_mode, mode);
+	pipe_config->cpu_transcoder = to_intel_crtc(crtc)->pipe;
 
 	plane_bpp = pipe_config_set_bpp(crtc, fb, pipe_config);
 	if (plane_bpp < 0)
@@ -8061,6 +8075,8 @@ intel_pipe_config_compare(struct intel_crtc_config *current_config,
 		return false; \
 	}
 
+	PIPE_CONF_CHECK_I(cpu_transcoder);
+
 	PIPE_CONF_CHECK_I(has_pch_encoder);
 	PIPE_CONF_CHECK_I(fdi_lanes);
 	PIPE_CONF_CHECK_I(fdi_m_n.gmch_m);
@@ -8192,7 +8208,6 @@ intel_modeset_check_state(struct drm_device *dev)
 		     "(expected %i, found %i)\n", enabled, crtc->base.enabled);
 
 		memset(&pipe_config, 0, sizeof(pipe_config));
-		pipe_config.cpu_transcoder = crtc->config.cpu_transcoder;
 		active = dev_priv->display.get_pipe_config(crtc,
 							   &pipe_config);
 		WARN(crtc->active != active,
@@ -8255,12 +8270,10 @@ static int __intel_set_mode(struct drm_crtc *crtc,
 	 * to set it here already despite that we pass it down the callchain.
 	 */
 	if (modeset_pipes) {
-		enum transcoder tmp = to_intel_crtc(crtc)->config.cpu_transcoder;
 		crtc->mode = *mode;
 		/* mode_set/enable/disable functions rely on a correct pipe
 		 * config. */
 		to_intel_crtc(crtc)->config = *pipe_config;
-		to_intel_crtc(crtc)->config.cpu_transcoder = tmp;
 	}
 
 	/* Only after disabling all output pipelines that will be changed can we
@@ -8671,7 +8684,6 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 	/* Swap pipes & planes for FBC on pre-965 */
 	intel_crtc->pipe = pipe;
 	intel_crtc->plane = pipe;
-	intel_crtc->config.cpu_transcoder = pipe;
 	if (IS_MOBILE(dev) && IS_GEN3(dev)) {
 		DRM_DEBUG_KMS("swapping pipes & planes for FBC\n");
 		intel_crtc->plane = !pipe;
@@ -9538,50 +9550,14 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	enum pipe pipe;
-	u32 tmp;
 	struct drm_plane *plane;
 	struct intel_crtc *crtc;
 	struct intel_encoder *encoder;
 	struct intel_connector *connector;
 
-	if (HAS_DDI(dev)) {
-		tmp = I915_READ(TRANS_DDI_FUNC_CTL(TRANSCODER_EDP));
-
-		if (tmp & TRANS_DDI_FUNC_ENABLE) {
-			switch (tmp & TRANS_DDI_EDP_INPUT_MASK) {
-			case TRANS_DDI_EDP_INPUT_A_ON:
-			case TRANS_DDI_EDP_INPUT_A_ONOFF:
-				pipe = PIPE_A;
-				break;
-			case TRANS_DDI_EDP_INPUT_B_ONOFF:
-				pipe = PIPE_B;
-				break;
-			case TRANS_DDI_EDP_INPUT_C_ONOFF:
-				pipe = PIPE_C;
-				break;
-			default:
-				/* A bogus value has been programmed, disable
-				 * the transcoder */
-				WARN(1, "Bogus eDP source %08x\n", tmp);
-				intel_ddi_disable_transcoder_func(dev_priv,
-						TRANSCODER_EDP);
-				goto setup_pipes;
-			}
-
-			crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
-			crtc->config.cpu_transcoder = TRANSCODER_EDP;
-
-			DRM_DEBUG_KMS("Pipe %c using transcoder EDP\n",
-				      pipe_name(pipe));
-		}
-	}
-
-setup_pipes:
 	list_for_each_entry(crtc, &dev->mode_config.crtc_list,
 			    base.head) {
-		enum transcoder tmp = crtc->config.cpu_transcoder;
 		memset(&crtc->config, 0, sizeof(crtc->config));
-		crtc->config.cpu_transcoder = tmp;
 
 		crtc->active = dev_priv->display.get_pipe_config(crtc,
 								 &crtc->config);
-- 
1.8.1.4

[PATCH] drm/i915: hw state readout&check support for cpu_transcoder

[Daniel Vetter]

This allows us to drop a bunch of ugly hacks and finally implement
what

commit cc464b2a17c59adedbdc02cc54341d630354edc3
Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
Date:   Fri Jan 25 16:59:16 2013 -0200

    drm/i915: set TRANSCODER_EDP even earlier

tried to achieve, but that was reverted again in

commit bba2181c49f1dddf8b592804a1b53cc1a3cf408a
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Fri Mar 22 10:53:40 2013 +0100

    Revert "drm/i915: set TRANSCODER_EDP even earlier"

Now we should always have a consistent cpu_transcoder in the
pipe_config.

v2: Fix up the code as spotted by Paulo:
- read the register for real
- assign the right pipes
- break out if the hw state doesn't make sense

Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_ddi.c     |  4 ++
 drivers/gpu/drm/i915/intel_display.c | 91 ++++++++++++++----------------------
 2 files changed, 38 insertions(+), 57 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 8c4f2aa..baaf670 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1458,9 +1458,13 @@ static bool intel_ddi_compute_config(struct intel_encoder *encoder,
 				     struct intel_crtc_config *pipe_config)
 {
 	int type = encoder->type;
+	int port = intel_ddi_get_encoder_port(encoder);
 
 	WARN(type == INTEL_OUTPUT_UNKNOWN, "compute_config() on unknown output!\n");
 
+	if (port == PORT_A)
+		pipe_config->cpu_transcoder = TRANSCODER_EDP;
+
 	if (type == INTEL_OUTPUT_HDMI)
 		return intel_hdmi_compute_config(encoder, pipe_config);
 	else
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 219094f..6dcb3a6 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3566,12 +3566,6 @@ static void ironlake_crtc_off(struct drm_crtc *crtc)
 
 static void haswell_crtc_off(struct drm_crtc *crtc)
 {
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-
-	/* Stop saying we're using TRANSCODER_EDP because some other CRTC might
-	 * start using it. */
-	intel_crtc->config.cpu_transcoder = (enum transcoder) intel_crtc->pipe;
-
 	intel_ddi_put_crtc_pll(crtc);
 }
 
@@ -5038,6 +5032,8 @@ static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	uint32_t tmp;
 
+	pipe_config->cpu_transcoder = crtc->pipe;
+
 	tmp = I915_READ(PIPECONF(crtc->pipe));
 	if (!(tmp & PIPECONF_ENABLE))
 		return false;
@@ -5789,8 +5785,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	WARN(!(HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)),
 	     "Unexpected PCH type %d\n", INTEL_PCH_TYPE(dev));
 
-	intel_crtc->config.cpu_transcoder = pipe;
-
 	ok = ironlake_compute_clocks(crtc, adjusted_mode, &clock,
 				     &has_reduced_clock, &reduced_clock);
 	if (!ok) {
@@ -5913,6 +5907,8 @@ static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	uint32_t tmp;
 
+	pipe_config->cpu_transcoder = crtc->pipe;
+
 	tmp = I915_READ(PIPECONF(crtc->pipe));
 	if (!(tmp & PIPECONF_ENABLE))
 		return false;
@@ -5987,11 +5983,6 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
 		num_connectors++;
 	}
 
-	if (is_cpu_edp)
-		intel_crtc->config.cpu_transcoder = TRANSCODER_EDP;
-	else
-		intel_crtc->config.cpu_transcoder = pipe;
-
 	/* We are not sure yet this won't happen. */
 	WARN(!HAS_PCH_LPT(dev), "Unexpected PCH type %d\n",
 	     INTEL_PCH_TYPE(dev));
@@ -6047,14 +6038,37 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
 {
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	enum transcoder cpu_transcoder = crtc->config.cpu_transcoder;
 	uint32_t tmp;
 
+	pipe_config->cpu_transcoder = crtc->pipe;
+	tmp = I915_READ(TRANS_DDI_FUNC_CTL(TRANSCODER_EDP));
+	if (tmp & TRANS_DDI_FUNC_ENABLE) {
+		enum pipe trans_edp_pipe;
+		switch (tmp & TRANS_DDI_EDP_INPUT_MASK) {
+		default:
+			WARN(1, "unknown pipe linked to edp transcoder\n");
+			break;
+		case TRANS_DDI_EDP_INPUT_A_ONOFF:
+		case TRANS_DDI_EDP_INPUT_A_ON:
+			trans_edp_pipe = PIPE_A;
+			break;
+		case TRANS_DDI_EDP_INPUT_B_ONOFF:
+			trans_edp_pipe = PIPE_B;
+			break;
+		case TRANS_DDI_EDP_INPUT_C_ONOFF:
+			trans_edp_pipe = PIPE_C;
+			break;
+		}
+
+		if (trans_edp_pipe == crtc->pipe)
+			pipe_config->cpu_transcoder = TRANSCODER_EDP;
+	}
+
 	if (!intel_display_power_enabled(dev,
-			POWER_DOMAIN_TRANSCODER(cpu_transcoder)))
+			POWER_DOMAIN_TRANSCODER(pipe_config->cpu_transcoder)))
 		return false;
 
-	tmp = I915_READ(PIPECONF(cpu_transcoder));
+	tmp = I915_READ(PIPECONF(pipe_config->cpu_transcoder));
 	if (!(tmp & PIPECONF_ENABLE))
 		return false;
 
@@ -6063,7 +6077,7 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
 	 * DDI E. So just check whether this pipe is wired to DDI E and whether
 	 * the PCH transcoder is on.
 	 */
-	tmp = I915_READ(TRANS_DDI_FUNC_CTL(cpu_transcoder));
+	tmp = I915_READ(TRANS_DDI_FUNC_CTL(pipe_config->cpu_transcoder));
 	if ((tmp & TRANS_DDI_PORT_MASK) == TRANS_DDI_SELECT_PORT(PORT_E) &&
 	    I915_READ(LPT_TRANSCONF) & TRANS_ENABLE) {
 		pipe_config->has_pch_encoder = true;
@@ -7814,6 +7828,7 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
 
 	drm_mode_copy(&pipe_config->adjusted_mode, mode);
 	drm_mode_copy(&pipe_config->requested_mode, mode);
+	pipe_config->cpu_transcoder = to_intel_crtc(crtc)->pipe;
 
 	plane_bpp = pipe_config_set_bpp(crtc, fb, pipe_config);
 	if (plane_bpp < 0)
@@ -8063,6 +8078,8 @@ intel_pipe_config_compare(struct intel_crtc_config *current_config,
 		return false; \
 	}
 
+	PIPE_CONF_CHECK_I(cpu_transcoder);
+
 	PIPE_CONF_CHECK_I(has_pch_encoder);
 	PIPE_CONF_CHECK_I(fdi_lanes);
 	PIPE_CONF_CHECK_I(fdi_m_n.gmch_m);
@@ -8206,7 +8223,6 @@ intel_modeset_check_state(struct drm_device *dev)
 		     "crtc's computed enabled state doesn't match tracked enabled state "
 		     "(expected %i, found %i)\n", enabled, crtc->base.enabled);
 
-		pipe_config.cpu_transcoder = crtc->config.cpu_transcoder;
 		active = dev_priv->display.get_pipe_config(crtc,
 							   &pipe_config);
 		WARN(crtc->active != active,
@@ -8269,12 +8285,10 @@ static int __intel_set_mode(struct drm_crtc *crtc,
 	 * to set it here already despite that we pass it down the callchain.
 	 */
 	if (modeset_pipes) {
-		enum transcoder tmp = to_intel_crtc(crtc)->config.cpu_transcoder;
 		crtc->mode = *mode;
 		/* mode_set/enable/disable functions rely on a correct pipe
 		 * config. */
 		to_intel_crtc(crtc)->config = *pipe_config;
-		to_intel_crtc(crtc)->config.cpu_transcoder = tmp;
 	}
 
 	/* Only after disabling all output pipelines that will be changed can we
@@ -8685,7 +8699,6 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 	/* Swap pipes & planes for FBC on pre-965 */
 	intel_crtc->pipe = pipe;
 	intel_crtc->plane = pipe;
-	intel_crtc->config.cpu_transcoder = pipe;
 	if (IS_MOBILE(dev) && IS_GEN3(dev)) {
 		DRM_DEBUG_KMS("swapping pipes & planes for FBC\n");
 		intel_crtc->plane = !pipe;
@@ -9557,50 +9570,14 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	enum pipe pipe;
-	u32 tmp;
 	struct drm_plane *plane;
 	struct intel_crtc *crtc;
 	struct intel_encoder *encoder;
 	struct intel_connector *connector;
 
-	if (HAS_DDI(dev)) {
-		tmp = I915_READ(TRANS_DDI_FUNC_CTL(TRANSCODER_EDP));
-
-		if (tmp & TRANS_DDI_FUNC_ENABLE) {
-			switch (tmp & TRANS_DDI_EDP_INPUT_MASK) {
-			case TRANS_DDI_EDP_INPUT_A_ON:
-			case TRANS_DDI_EDP_INPUT_A_ONOFF:
-				pipe = PIPE_A;
-				break;
-			case TRANS_DDI_EDP_INPUT_B_ONOFF:
-				pipe = PIPE_B;
-				break;
-			case TRANS_DDI_EDP_INPUT_C_ONOFF:
-				pipe = PIPE_C;
-				break;
-			default:
-				/* A bogus value has been programmed, disable
-				 * the transcoder */
-				WARN(1, "Bogus eDP source %08x\n", tmp);
-				intel_ddi_disable_transcoder_func(dev_priv,
-						TRANSCODER_EDP);
-				goto setup_pipes;
-			}
-
-			crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
-			crtc->config.cpu_transcoder = TRANSCODER_EDP;
-
-			DRM_DEBUG_KMS("Pipe %c using transcoder EDP\n",
-				      pipe_name(pipe));
-		}
-	}
-
-setup_pipes:
 	list_for_each_entry(crtc, &dev->mode_config.crtc_list,
 			    base.head) {
-		enum transcoder tmp = crtc->config.cpu_transcoder;
 		memset(&crtc->config, 0, sizeof(crtc->config));
-		crtc->config.cpu_transcoder = tmp;
 
 		crtc->active = dev_priv->display.get_pipe_config(crtc,
 								 &crtc->config);
-- 
1.8.1.4

[PATCH 1/3] drm/i915: hw state readout&check support for cpu_transcoder

[Daniel Vetter]

This allows us to drop a bunch of ugly hacks and finally implement
what

commit cc464b2a17c59adedbdc02cc54341d630354edc3
Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
Date:   Fri Jan 25 16:59:16 2013 -0200

    drm/i915: set TRANSCODER_EDP even earlier

tried to achieve, but that was reverted again in

commit bba2181c49f1dddf8b592804a1b53cc1a3cf408a
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Fri Mar 22 10:53:40 2013 +0100

    Revert "drm/i915: set TRANSCODER_EDP even earlier"

Now we should always have a consistent cpu_transcoder in the
pipe_config.

v2: Fix up the code as spotted by Paulo:
- read the register for real
- assign the right pipes
- break out if the hw state doesn't make sense

v3: Shut up gcc.

Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_ddi.c     |  4 ++
 drivers/gpu/drm/i915/intel_display.c | 90 +++++++++++++-----------------------
 2 files changed, 37 insertions(+), 57 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 1fdd3b0..9649df8 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1291,9 +1291,13 @@ static bool intel_ddi_compute_config(struct intel_encoder *encoder,
 				     struct intel_crtc_config *pipe_config)
 {
 	int type = encoder->type;
+	int port = intel_ddi_get_encoder_port(encoder);
 
 	WARN(type == INTEL_OUTPUT_UNKNOWN, "compute_config() on unknown output!\n");
 
+	if (port == PORT_A)
+		pipe_config->cpu_transcoder = TRANSCODER_EDP;
+
 	if (type == INTEL_OUTPUT_HDMI)
 		return intel_hdmi_compute_config(encoder, pipe_config);
 	else
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f9ec773..a42a7fd 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3567,12 +3567,6 @@ static void ironlake_crtc_off(struct drm_crtc *crtc)
 
 static void haswell_crtc_off(struct drm_crtc *crtc)
 {
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-
-	/* Stop saying we're using TRANSCODER_EDP because some other CRTC might
-	 * start using it. */
-	intel_crtc->config.cpu_transcoder = (enum transcoder) intel_crtc->pipe;
-
 	intel_ddi_put_crtc_pll(crtc);
 }
 
@@ -5023,6 +5017,8 @@ static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	uint32_t tmp;
 
+	pipe_config->cpu_transcoder = crtc->pipe;
+
 	tmp = I915_READ(PIPECONF(crtc->pipe));
 	if (!(tmp & PIPECONF_ENABLE))
 		return false;
@@ -5745,8 +5741,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	WARN(!(HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)),
 	     "Unexpected PCH type %d\n", INTEL_PCH_TYPE(dev));
 
-	intel_crtc->config.cpu_transcoder = pipe;
-
 	ok = ironlake_compute_clocks(crtc, adjusted_mode, &clock,
 				     &has_reduced_clock, &reduced_clock);
 	if (!ok) {
@@ -5882,6 +5876,8 @@ static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	uint32_t tmp;
 
+	pipe_config->cpu_transcoder = crtc->pipe;
+
 	tmp = I915_READ(PIPECONF(crtc->pipe));
 	if (!(tmp & PIPECONF_ENABLE))
 		return false;
@@ -5958,11 +5954,6 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
 		num_connectors++;
 	}
 
-	if (is_cpu_edp)
-		intel_crtc->config.cpu_transcoder = TRANSCODER_EDP;
-	else
-		intel_crtc->config.cpu_transcoder = pipe;
-
 	/* We are not sure yet this won't happen. */
 	WARN(!HAS_PCH_LPT(dev), "Unexpected PCH type %d\n",
 	     INTEL_PCH_TYPE(dev));
@@ -6016,15 +6007,37 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
 {
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	enum transcoder cpu_transcoder = crtc->config.cpu_transcoder;
 	enum intel_display_power_domain pfit_domain;
 	uint32_t tmp;
 
+	pipe_config->cpu_transcoder = crtc->pipe;
+	tmp = I915_READ(TRANS_DDI_FUNC_CTL(TRANSCODER_EDP));
+	if (tmp & TRANS_DDI_FUNC_ENABLE) {
+		enum pipe trans_edp_pipe;
+		switch (tmp & TRANS_DDI_EDP_INPUT_MASK) {
+		default:
+			WARN(1, "unknown pipe linked to edp transcoder\n");
+		case TRANS_DDI_EDP_INPUT_A_ONOFF:
+		case TRANS_DDI_EDP_INPUT_A_ON:
+			trans_edp_pipe = PIPE_A;
+			break;
+		case TRANS_DDI_EDP_INPUT_B_ONOFF:
+			trans_edp_pipe = PIPE_B;
+			break;
+		case TRANS_DDI_EDP_INPUT_C_ONOFF:
+			trans_edp_pipe = PIPE_C;
+			break;
+		}
+
+		if (trans_edp_pipe == crtc->pipe)
+			pipe_config->cpu_transcoder = TRANSCODER_EDP;
+	}
+
 	if (!intel_display_power_enabled(dev,
-			POWER_DOMAIN_TRANSCODER(cpu_transcoder)))
+			POWER_DOMAIN_TRANSCODER(pipe_config->cpu_transcoder)))
 		return false;
 
-	tmp = I915_READ(PIPECONF(cpu_transcoder));
+	tmp = I915_READ(PIPECONF(pipe_config->cpu_transcoder));
 	if (!(tmp & PIPECONF_ENABLE))
 		return false;
 
@@ -6033,7 +6046,7 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
 	 * DDI E. So just check whether this pipe is wired to DDI E and whether
 	 * the PCH transcoder is on.
 	 */
-	tmp = I915_READ(TRANS_DDI_FUNC_CTL(cpu_transcoder));
+	tmp = I915_READ(TRANS_DDI_FUNC_CTL(pipe_config->cpu_transcoder));
 	if ((tmp & TRANS_DDI_PORT_MASK) == TRANS_DDI_SELECT_PORT(PORT_E) &&
 	    I915_READ(LPT_TRANSCONF) & TRANS_ENABLE) {
 		pipe_config->has_pch_encoder = true;
@@ -7788,6 +7801,7 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
 
 	drm_mode_copy(&pipe_config->adjusted_mode, mode);
 	drm_mode_copy(&pipe_config->requested_mode, mode);
+	pipe_config->cpu_transcoder = to_intel_crtc(crtc)->pipe;
 
 	plane_bpp = pipe_config_set_bpp(crtc, fb, pipe_config);
 	if (plane_bpp < 0)
@@ -8038,6 +8052,8 @@ intel_pipe_config_compare(struct drm_device *dev,
 		return false; \
 	}
 
+	PIPE_CONF_CHECK_I(cpu_transcoder);
+
 	PIPE_CONF_CHECK_I(has_pch_encoder);
 	PIPE_CONF_CHECK_I(fdi_lanes);
 	PIPE_CONF_CHECK_I(fdi_m_n.gmch_m);
@@ -8189,7 +8205,6 @@ intel_modeset_check_state(struct drm_device *dev)
 		     "crtc's computed enabled state doesn't match tracked enabled state "
 		     "(expected %i, found %i)\n", enabled, crtc->base.enabled);
 
-		pipe_config.cpu_transcoder = crtc->config.cpu_transcoder;
 		active = dev_priv->display.get_pipe_config(crtc,
 							   &pipe_config);
 		WARN(crtc->active != active,
@@ -8252,12 +8267,10 @@ static int __intel_set_mode(struct drm_crtc *crtc,
 	 * to set it here already despite that we pass it down the callchain.
 	 */
 	if (modeset_pipes) {
-		enum transcoder tmp = to_intel_crtc(crtc)->config.cpu_transcoder;
 		crtc->mode = *mode;
 		/* mode_set/enable/disable functions rely on a correct pipe
 		 * config. */
 		to_intel_crtc(crtc)->config = *pipe_config;
-		to_intel_crtc(crtc)->config.cpu_transcoder = tmp;
 	}
 
 	/* Only after disabling all output pipelines that will be changed can we
@@ -8683,7 +8696,6 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 	/* Swap pipes & planes for FBC on pre-965 */
 	intel_crtc->pipe = pipe;
 	intel_crtc->plane = pipe;
-	intel_crtc->config.cpu_transcoder = pipe;
 	if (IS_MOBILE(dev) && IS_GEN3(dev)) {
 		DRM_DEBUG_KMS("swapping pipes & planes for FBC\n");
 		intel_crtc->plane = !pipe;
@@ -9549,50 +9561,14 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	enum pipe pipe;
-	u32 tmp;
 	struct drm_plane *plane;
 	struct intel_crtc *crtc;
 	struct intel_encoder *encoder;
 	struct intel_connector *connector;
 
-	if (HAS_DDI(dev)) {
-		tmp = I915_READ(TRANS_DDI_FUNC_CTL(TRANSCODER_EDP));
-
-		if (tmp & TRANS_DDI_FUNC_ENABLE) {
-			switch (tmp & TRANS_DDI_EDP_INPUT_MASK) {
-			case TRANS_DDI_EDP_INPUT_A_ON:
-			case TRANS_DDI_EDP_INPUT_A_ONOFF:
-				pipe = PIPE_A;
-				break;
-			case TRANS_DDI_EDP_INPUT_B_ONOFF:
-				pipe = PIPE_B;
-				break;
-			case TRANS_DDI_EDP_INPUT_C_ONOFF:
-				pipe = PIPE_C;
-				break;
-			default:
-				/* A bogus value has been programmed, disable
-				 * the transcoder */
-				WARN(1, "Bogus eDP source %08x\n", tmp);
-				intel_ddi_disable_transcoder_func(dev_priv,
-						TRANSCODER_EDP);
-				goto setup_pipes;
-			}
-
-			crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
-			crtc->config.cpu_transcoder = TRANSCODER_EDP;
-
-			DRM_DEBUG_KMS("Pipe %c using transcoder EDP\n",
-				      pipe_name(pipe));
-		}
-	}
-
-setup_pipes:
 	list_for_each_entry(crtc, &dev->mode_config.crtc_list,
 			    base.head) {
-		enum transcoder tmp = crtc->config.cpu_transcoder;
 		memset(&crtc->config, 0, sizeof(crtc->config));
-		crtc->config.cpu_transcoder = tmp;
 
 		crtc->active = dev_priv->display.get_pipe_config(crtc,
 								 &crtc->config);
-- 
1.8.1.4

[PATCH 2/3] drm/i915: fixup i915_pipe_enabled check in i915_irq.c

[Daniel Vetter]

Well, as well as we can without completely revamping the drm vblank
code. The issue are that
- The vblank code needs to work on both ums and kms.
- It deals always deals with pipes.
- It doesn't take any of the kms locks.

The last part is not really fixable without revamping the drm vblank
code, since the drm core <-> driver interactions is a veritable pile
of spaghettis. But the other pieces can be fixed by switching on the
MODESET driver flag and either checking the hw state directly (ums
case) or just querying our sw tracking (with broken locking, but
that's not worse than what we've had).

Note that this essentially reverts

commit 702e7a56af3780d8b3a717f698209bef44187bb0
Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
Date:   Tue Oct 23 18:29:59 2012 -0200

    drm/i915: convert PIPECONF to use transcoder instead of pipe

for the ums case, which will fix a NULL deref (since we really don't
have any crtcs set up).

But the real reason to do this is to drop our reliance on the
cpu_transcoder: By only checking intel_crtc->active we don't need to
make sure that the pipe_config (or at least the cpu_transcoder)
contain safe values even when the pipe is off.

Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: Damien Lespiau <damien.lespiau@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_irq.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 879c4cc..d2efc5e 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -381,14 +381,16 @@ static int
 i915_pipe_enabled(struct drm_device *dev, int pipe)
 {
 	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
-	enum transcoder cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv,
-								      pipe);
 
-	if (!intel_display_power_enabled(dev,
-		POWER_DOMAIN_TRANSCODER(cpu_transcoder)))
-		return false;
+	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
+		/* Locking is horribly broken here, but whatever. */
+		struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
+		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 
-	return I915_READ(PIPECONF(cpu_transcoder)) & PIPECONF_ENABLE;
+		return intel_crtc->active;
+	} else {
+		return I915_READ(PIPECONF(pipe)) & PIPECONF_ENABLE;
+	}
 }
 
 /* Called from drm generic code, passed a 'crtc', which
-- 
1.8.1.4

[PATCH 3/3] drm/i915: add basic pipe config dump support

[Daniel Vetter]

All this pipe config abstraction adds another layer of complexity, so
it's good to have better visibility into what's going on exactly.
Doesn't dump out everything yet, and some bits are a bit duplicated
but this should be a good start.

v2: Remove a few more now redudant debug output lines.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 74 +++++++++++++++++++++---------------
 1 file changed, 44 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a42a7fd..5efb00d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3621,18 +3621,12 @@ static void i9xx_pfit_enable(struct intel_crtc *crtc)
 	if (!crtc->config.gmch_pfit.control)
 		return;
 
-	WARN_ON(I915_READ(PFIT_CONTROL) & PFIT_ENABLE);
-	assert_pipe_disabled(dev_priv, crtc->pipe);
-
 	/*
-	 * Enable automatic panel scaling so that non-native modes
-	 * fill the screen.  The panel fitter should only be
-	 * adjusted whilst the pipe is disabled, according to
-	 * register description and PRM.
+	 * The panel fitter should only be adjusted whilst the pipe is disabled,
+	 * according to register description and PRM.
 	 */
-	DRM_DEBUG_KMS("applying panel-fitter: %x, %x\n",
-		      pipe_config->gmch_pfit.control,
-		      pipe_config->gmch_pfit.pgm_ratios);
+	WARN_ON(I915_READ(PFIT_CONTROL) & PFIT_ENABLE);
+	assert_pipe_disabled(dev_priv, crtc->pipe);
 
 	I915_WRITE(PFIT_PGM_RATIOS, pipe_config->gmch_pfit.pgm_ratios);
 	I915_WRITE(PFIT_CONTROL, pipe_config->gmch_pfit.control);
@@ -4955,9 +4949,6 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
 			dspcntr |= DISPPLANE_SEL_PIPE_B;
 	}
 
-	DRM_DEBUG_KMS("Mode for pipe %c:\n", pipe_name(pipe));
-	drm_mode_debug_printmodeline(mode);
-
 	intel_set_pipe_timings(intel_crtc, mode, adjusted_mode);
 
 	/* pipesrc and dspsize control the size that is scaled from,
@@ -5759,9 +5750,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	/* Ensure that the cursor is valid for the new mode before changing... */
 	intel_crtc_update_cursor(crtc, true);
 
-	DRM_DEBUG_KMS("Mode for pipe %c:\n", pipe_name(pipe));
-	drm_mode_debug_printmodeline(mode);
-
 	/* CPU eDP is the only output that doesn't need a PCH PLL of its own. */
 	if (intel_crtc->config.has_pch_encoder) {
 		struct intel_pch_pll *pll;
@@ -5972,9 +5960,6 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
 	/* Ensure that the cursor is valid for the new mode before changing... */
 	intel_crtc_update_cursor(crtc, true);
 
-	DRM_DEBUG_KMS("Mode for pipe %c:\n", pipe_name(pipe));
-	drm_mode_debug_printmodeline(mode);
-
 	if (intel_crtc->config.has_dp_encoder)
 		intel_dp_set_m_n(intel_crtc);
 
@@ -7783,6 +7768,35 @@ pipe_config_set_bpp(struct drm_crtc *crtc,
 	return bpp;
 }
 
+static void intel_dump_pipe_config(struct intel_crtc *crtc,
+				   struct intel_crtc_config *pipe_config,
+				   const char *context)
+{
+	DRM_DEBUG_KMS("[CRTC:%d]%s config for pipe %c\n", crtc->base.base.id,
+		      context, pipe_name(crtc->pipe));
+
+	DRM_DEBUG_KMS("cpu_transcoder: %i\n", pipe_name(pipe_config->cpu_transcoder));
+	DRM_DEBUG_KMS("pipe bpp: %i, dithering: %i\n",
+		      pipe_config->pipe_bpp, pipe_config->dither);
+	DRM_DEBUG_KMS("fdi/pch: %i, lanes: %i, gmch_m: %i, gmch_n %i, link_m: %i, link_n: %i, tu: %i\n",
+		      pipe_config->has_pch_encoder,
+		      pipe_config->fdi_lanes,
+		      pipe_config->fdi_m_n.gmch_m, pipe_config->fdi_m_n.gmch_n,
+		      pipe_config->fdi_m_n.link_m, pipe_config->fdi_m_n.link_n,
+		      pipe_config->fdi_m_n.tu);
+	DRM_DEBUG_KMS("requested mode:\n");
+	drm_mode_debug_printmodeline(&pipe_config->requested_mode);
+	DRM_DEBUG_KMS("adjusted mode:\n");
+	drm_mode_debug_printmodeline(&pipe_config->adjusted_mode);
+	DRM_DEBUG_KMS("gmch pfit: control: 0x%08x, ratios: 0x%08x, lvds border: 0x%08x\n",
+		      pipe_config->gmch_pfit.control,
+		      pipe_config->gmch_pfit.pgm_ratios,
+		      pipe_config->gmch_pfit.lvds_border_bits);
+	DRM_DEBUG_KMS("pch pfit: pos: 0x%08x, size: 0x%08x\n",
+		      pipe_config->pch_pfit.pos,
+		      pipe_config->pch_pfit.size);
+}
+
 static struct intel_crtc_config *
 intel_modeset_pipe_config(struct drm_crtc *crtc,
 			  struct drm_framebuffer *fb,
@@ -7853,8 +7867,6 @@ encoder_retry:
 		goto encoder_retry;
 	}
 
-	DRM_DEBUG_KMS("[CRTC:%d]\n", crtc->base.id);
-
 	pipe_config->dither = pipe_config->pipe_bpp != plane_bpp;
 	DRM_DEBUG_KMS("plane bpp: %i, pipe bpp: %i, dithering: %i\n",
 		      plane_bpp, pipe_config->pipe_bpp, pipe_config->dither);
@@ -8211,9 +8223,14 @@ intel_modeset_check_state(struct drm_device *dev)
 		     "crtc active state doesn't match with hw state "
 		     "(expected %i, found %i)\n", crtc->active, active);
 
-		WARN(active &&
-		     !intel_pipe_config_compare(dev, &crtc->config, &pipe_config),
-		     "pipe state doesn't match!\n");
+		if (active &&
+		    !intel_pipe_config_compare(dev, &crtc->config, &pipe_config)) {
+			WARN(1, "pipe state doesn't match!\n");
+			intel_dump_pipe_config(crtc, &pipe_config,
+					       "[hw state]");
+			intel_dump_pipe_config(crtc, &crtc->config,
+					       "[sw state]");
+		}
 	}
 }
 
@@ -8253,6 +8270,8 @@ static int __intel_set_mode(struct drm_crtc *crtc,
 
 			goto out;
 		}
+		intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config,
+				       "[modeset]");
 	}
 
 	for_each_intel_crtc_masked(dev, disable_pipes, intel_crtc)
@@ -8609,12 +8628,6 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
 		goto fail;
 
 	if (config->mode_changed) {
-		if (set->mode) {
-			DRM_DEBUG_KMS("attempting to set mode from"
-					" userspace\n");
-			drm_mode_debug_printmodeline(set->mode);
-		}
-
 		ret = intel_set_mode(set->crtc, set->mode,
 				     set->x, set->y, set->fb);
 	} else if (config->fb_changed) {
@@ -9629,6 +9642,7 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
 	for_each_pipe(pipe) {
 		crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
 		intel_sanitize_crtc(crtc);
+		intel_dump_pipe_config(crtc, &crtc->config, "[setup_hw_state]");
 	}
 
 	if (force_restore) {
-- 
1.8.1.4

Re: [PATCH 1/3] drm/i915: hw state readout&check support for cpu_transcoder

[Paulo Zanoni]

2013/5/21 Daniel Vetter <daniel.vetter@ffwll.ch>:
> This allows us to drop a bunch of ugly hacks and finally implement
> what
>
> commit cc464b2a17c59adedbdc02cc54341d630354edc3
> Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Date:   Fri Jan 25 16:59:16 2013 -0200
>
>     drm/i915: set TRANSCODER_EDP even earlier
>
> tried to achieve, but that was reverted again in
>
> commit bba2181c49f1dddf8b592804a1b53cc1a3cf408a
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Fri Mar 22 10:53:40 2013 +0100
>
>     Revert "drm/i915: set TRANSCODER_EDP even earlier"
>
> Now we should always have a consistent cpu_transcoder in the
> pipe_config.
>
> v2: Fix up the code as spotted by Paulo:
> - read the register for real
> - assign the right pipes
> - break out if the hw state doesn't make sense
>
> v3: Shut up gcc.
>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c     |  4 ++
>  drivers/gpu/drm/i915/intel_display.c | 90 +++++++++++++-----------------------
>  2 files changed, 37 insertions(+), 57 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 1fdd3b0..9649df8 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1291,9 +1291,13 @@ static bool intel_ddi_compute_config(struct intel_encoder *encoder,
>                                      struct intel_crtc_config *pipe_config)
>  {
>         int type = encoder->type;
> +       int port = intel_ddi_get_encoder_port(encoder);
>
>         WARN(type == INTEL_OUTPUT_UNKNOWN, "compute_config() on unknown output!\n");
>
> +       if (port == PORT_A)
> +               pipe_config->cpu_transcoder = TRANSCODER_EDP;
> +
>         if (type == INTEL_OUTPUT_HDMI)
>                 return intel_hdmi_compute_config(encoder, pipe_config);
>         else
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f9ec773..a42a7fd 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3567,12 +3567,6 @@ static void ironlake_crtc_off(struct drm_crtc *crtc)
>
>  static void haswell_crtc_off(struct drm_crtc *crtc)
>  {
> -       struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -
> -       /* Stop saying we're using TRANSCODER_EDP because some other CRTC might
> -        * start using it. */
> -       intel_crtc->config.cpu_transcoder = (enum transcoder) intel_crtc->pipe;
> -
>         intel_ddi_put_crtc_pll(crtc);
>  }
>
> @@ -5023,6 +5017,8 @@ static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         uint32_t tmp;
>
> +       pipe_config->cpu_transcoder = crtc->pipe;
> +
>         tmp = I915_READ(PIPECONF(crtc->pipe));
>         if (!(tmp & PIPECONF_ENABLE))
>                 return false;
> @@ -5745,8 +5741,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>         WARN(!(HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)),
>              "Unexpected PCH type %d\n", INTEL_PCH_TYPE(dev));
>
> -       intel_crtc->config.cpu_transcoder = pipe;
> -
>         ok = ironlake_compute_clocks(crtc, adjusted_mode, &clock,
>                                      &has_reduced_clock, &reduced_clock);
>         if (!ok) {
> @@ -5882,6 +5876,8 @@ static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         uint32_t tmp;
>
> +       pipe_config->cpu_transcoder = crtc->pipe;
> +
>         tmp = I915_READ(PIPECONF(crtc->pipe));
>         if (!(tmp & PIPECONF_ENABLE))
>                 return false;
> @@ -5958,11 +5954,6 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
>                 num_connectors++;
>         }
>
> -       if (is_cpu_edp)
> -               intel_crtc->config.cpu_transcoder = TRANSCODER_EDP;
> -       else
> -               intel_crtc->config.cpu_transcoder = pipe;
> -
>         /* We are not sure yet this won't happen. */
>         WARN(!HAS_PCH_LPT(dev), "Unexpected PCH type %d\n",
>              INTEL_PCH_TYPE(dev));
> @@ -6016,15 +6007,37 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>  {
>         struct drm_device *dev = crtc->base.dev;
>         struct drm_i915_private *dev_priv = dev->dev_private;
> -       enum transcoder cpu_transcoder = crtc->config.cpu_transcoder;
>         enum intel_display_power_domain pfit_domain;
>         uint32_t tmp;
>
> +       pipe_config->cpu_transcoder = crtc->pipe;
> +       tmp = I915_READ(TRANS_DDI_FUNC_CTL(TRANSCODER_EDP));
> +       if (tmp & TRANS_DDI_FUNC_ENABLE) {
> +               enum pipe trans_edp_pipe;
> +               switch (tmp & TRANS_DDI_EDP_INPUT_MASK) {
> +               default:
> +                       WARN(1, "unknown pipe linked to edp transcoder\n");

Since there's no "break" we'll assign PIPE_A and then we'll probably
break pipe A on this case. OTOH this case should never really happen,
so I'm not sure if we care. So this is an optional bikeshedding.

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Also briefly tested, seems to work.

> +               case TRANS_DDI_EDP_INPUT_A_ONOFF:
> +               case TRANS_DDI_EDP_INPUT_A_ON:
> +                       trans_edp_pipe = PIPE_A;
> +                       break;
> +               case TRANS_DDI_EDP_INPUT_B_ONOFF:
> +                       trans_edp_pipe = PIPE_B;
> +                       break;
> +               case TRANS_DDI_EDP_INPUT_C_ONOFF:
> +                       trans_edp_pipe = PIPE_C;
> +                       break;
> +               }
> +
> +               if (trans_edp_pipe == crtc->pipe)
> +                       pipe_config->cpu_transcoder = TRANSCODER_EDP;
> +       }
> +
>         if (!intel_display_power_enabled(dev,
> -                       POWER_DOMAIN_TRANSCODER(cpu_transcoder)))
> +                       POWER_DOMAIN_TRANSCODER(pipe_config->cpu_transcoder)))
>                 return false;
>
> -       tmp = I915_READ(PIPECONF(cpu_transcoder));
> +       tmp = I915_READ(PIPECONF(pipe_config->cpu_transcoder));
>         if (!(tmp & PIPECONF_ENABLE))
>                 return false;
>
> @@ -6033,7 +6046,7 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>          * DDI E. So just check whether this pipe is wired to DDI E and whether
>          * the PCH transcoder is on.
>          */
> -       tmp = I915_READ(TRANS_DDI_FUNC_CTL(cpu_transcoder));
> +       tmp = I915_READ(TRANS_DDI_FUNC_CTL(pipe_config->cpu_transcoder));
>         if ((tmp & TRANS_DDI_PORT_MASK) == TRANS_DDI_SELECT_PORT(PORT_E) &&
>             I915_READ(LPT_TRANSCONF) & TRANS_ENABLE) {
>                 pipe_config->has_pch_encoder = true;
> @@ -7788,6 +7801,7 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
>
>         drm_mode_copy(&pipe_config->adjusted_mode, mode);
>         drm_mode_copy(&pipe_config->requested_mode, mode);
> +       pipe_config->cpu_transcoder = to_intel_crtc(crtc)->pipe;
>
>         plane_bpp = pipe_config_set_bpp(crtc, fb, pipe_config);
>         if (plane_bpp < 0)
> @@ -8038,6 +8052,8 @@ intel_pipe_config_compare(struct drm_device *dev,
>                 return false; \
>         }
>
> +       PIPE_CONF_CHECK_I(cpu_transcoder);
> +
>         PIPE_CONF_CHECK_I(has_pch_encoder);
>         PIPE_CONF_CHECK_I(fdi_lanes);
>         PIPE_CONF_CHECK_I(fdi_m_n.gmch_m);
> @@ -8189,7 +8205,6 @@ intel_modeset_check_state(struct drm_device *dev)
>                      "crtc's computed enabled state doesn't match tracked enabled state "
>                      "(expected %i, found %i)\n", enabled, crtc->base.enabled);
>
> -               pipe_config.cpu_transcoder = crtc->config.cpu_transcoder;
>                 active = dev_priv->display.get_pipe_config(crtc,
>                                                            &pipe_config);
>                 WARN(crtc->active != active,
> @@ -8252,12 +8267,10 @@ static int __intel_set_mode(struct drm_crtc *crtc,
>          * to set it here already despite that we pass it down the callchain.
>          */
>         if (modeset_pipes) {
> -               enum transcoder tmp = to_intel_crtc(crtc)->config.cpu_transcoder;
>                 crtc->mode = *mode;
>                 /* mode_set/enable/disable functions rely on a correct pipe
>                  * config. */
>                 to_intel_crtc(crtc)->config = *pipe_config;
> -               to_intel_crtc(crtc)->config.cpu_transcoder = tmp;
>         }
>
>         /* Only after disabling all output pipelines that will be changed can we
> @@ -8683,7 +8696,6 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
>         /* Swap pipes & planes for FBC on pre-965 */
>         intel_crtc->pipe = pipe;
>         intel_crtc->plane = pipe;
> -       intel_crtc->config.cpu_transcoder = pipe;
>         if (IS_MOBILE(dev) && IS_GEN3(dev)) {
>                 DRM_DEBUG_KMS("swapping pipes & planes for FBC\n");
>                 intel_crtc->plane = !pipe;
> @@ -9549,50 +9561,14 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         enum pipe pipe;
> -       u32 tmp;
>         struct drm_plane *plane;
>         struct intel_crtc *crtc;
>         struct intel_encoder *encoder;
>         struct intel_connector *connector;
>
> -       if (HAS_DDI(dev)) {
> -               tmp = I915_READ(TRANS_DDI_FUNC_CTL(TRANSCODER_EDP));
> -
> -               if (tmp & TRANS_DDI_FUNC_ENABLE) {
> -                       switch (tmp & TRANS_DDI_EDP_INPUT_MASK) {
> -                       case TRANS_DDI_EDP_INPUT_A_ON:
> -                       case TRANS_DDI_EDP_INPUT_A_ONOFF:
> -                               pipe = PIPE_A;
> -                               break;
> -                       case TRANS_DDI_EDP_INPUT_B_ONOFF:
> -                               pipe = PIPE_B;
> -                               break;
> -                       case TRANS_DDI_EDP_INPUT_C_ONOFF:
> -                               pipe = PIPE_C;
> -                               break;
> -                       default:
> -                               /* A bogus value has been programmed, disable
> -                                * the transcoder */
> -                               WARN(1, "Bogus eDP source %08x\n", tmp);
> -                               intel_ddi_disable_transcoder_func(dev_priv,
> -                                               TRANSCODER_EDP);
> -                               goto setup_pipes;
> -                       }
> -
> -                       crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
> -                       crtc->config.cpu_transcoder = TRANSCODER_EDP;
> -
> -                       DRM_DEBUG_KMS("Pipe %c using transcoder EDP\n",
> -                                     pipe_name(pipe));
> -               }
> -       }
> -
> -setup_pipes:
>         list_for_each_entry(crtc, &dev->mode_config.crtc_list,
>                             base.head) {
> -               enum transcoder tmp = crtc->config.cpu_transcoder;
>                 memset(&crtc->config, 0, sizeof(crtc->config));
> -               crtc->config.cpu_transcoder = tmp;
>
>                 crtc->active = dev_priv->display.get_pipe_config(crtc,
>                                                                  &crtc->config);
> --
> 1.8.1.4
>



-- 
Paulo Zanoni

Re: [PATCH 2/3] drm/i915: fixup i915_pipe_enabled check in i915_irq.c

[Paulo Zanoni]

2013/5/21 Daniel Vetter <daniel.vetter@ffwll.ch>:
> Well, as well as we can without completely revamping the drm vblank
> code. The issue are that
> - The vblank code needs to work on both ums and kms.
> - It deals always deals with pipes.
> - It doesn't take any of the kms locks.
>
> The last part is not really fixable without revamping the drm vblank
> code, since the drm core <-> driver interactions is a veritable pile
> of spaghettis. But the other pieces can be fixed by switching on the
> MODESET driver flag and either checking the hw state directly (ums
> case) or just querying our sw tracking (with broken locking, but
> that's not worse than what we've had).
>
> Note that this essentially reverts
>
> commit 702e7a56af3780d8b3a717f698209bef44187bb0
> Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Date:   Tue Oct 23 18:29:59 2012 -0200
>
>     drm/i915: convert PIPECONF to use transcoder instead of pipe
>
> for the ums case, which will fix a NULL deref (since we really don't
> have any crtcs set up).
>
> But the real reason to do this is to drop our reliance on the
> cpu_transcoder: By only checking intel_crtc->active we don't need to
> make sure that the pipe_config (or at least the cpu_transcoder)
> contain safe values even when the pipe is off.
>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Damien Lespiau <damien.lespiau@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 879c4cc..d2efc5e 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -381,14 +381,16 @@ static int
>  i915_pipe_enabled(struct drm_device *dev, int pipe)
>  {
>         drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
> -       enum transcoder cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv,
> -                                                                     pipe);
>
> -       if (!intel_display_power_enabled(dev,
> -               POWER_DOMAIN_TRANSCODER(cpu_transcoder)))
> -               return false;
> +       if (drm_core_check_feature(dev, DRIVER_MODESET)) {
> +               /* Locking is horribly broken here, but whatever. */

We could add also have a TODO or FIXME tag here.

Looks correct.
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> +               struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
> +               struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>
> -       return I915_READ(PIPECONF(cpu_transcoder)) & PIPECONF_ENABLE;
> +               return intel_crtc->active;
> +       } else {
> +               return I915_READ(PIPECONF(pipe)) & PIPECONF_ENABLE;
> +       }
>  }
>
>  /* Called from drm generic code, passed a 'crtc', which
> --
> 1.8.1.4
>



-- 
Paulo Zanoni

Re: [PATCH 3/3] drm/i915: add basic pipe config dump support

[Paulo Zanoni]

2013/5/21 Daniel Vetter <daniel.vetter@ffwll.ch>:
> All this pipe config abstraction adds another layer of complexity, so
> it's good to have better visibility into what's going on exactly.
> Doesn't dump out everything yet, and some bits are a bit duplicated
> but this should be a good start.
>
> v2: Remove a few more now redudant debug output lines.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 74 +++++++++++++++++++++---------------
>  1 file changed, 44 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index a42a7fd..5efb00d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3621,18 +3621,12 @@ static void i9xx_pfit_enable(struct intel_crtc *crtc)
>         if (!crtc->config.gmch_pfit.control)
>                 return;
>
> -       WARN_ON(I915_READ(PFIT_CONTROL) & PFIT_ENABLE);
> -       assert_pipe_disabled(dev_priv, crtc->pipe);
> -
>         /*
> -        * Enable automatic panel scaling so that non-native modes
> -        * fill the screen.  The panel fitter should only be
> -        * adjusted whilst the pipe is disabled, according to
> -        * register description and PRM.
> +        * The panel fitter should only be adjusted whilst the pipe is disabled,
> +        * according to register description and PRM.
>          */
> -       DRM_DEBUG_KMS("applying panel-fitter: %x, %x\n",
> -                     pipe_config->gmch_pfit.control,
> -                     pipe_config->gmch_pfit.pgm_ratios);
> +       WARN_ON(I915_READ(PFIT_CONTROL) & PFIT_ENABLE);
> +       assert_pipe_disabled(dev_priv, crtc->pipe);
>
>         I915_WRITE(PFIT_PGM_RATIOS, pipe_config->gmch_pfit.pgm_ratios);
>         I915_WRITE(PFIT_CONTROL, pipe_config->gmch_pfit.control);
> @@ -4955,9 +4949,6 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
>                         dspcntr |= DISPPLANE_SEL_PIPE_B;
>         }
>
> -       DRM_DEBUG_KMS("Mode for pipe %c:\n", pipe_name(pipe));
> -       drm_mode_debug_printmodeline(mode);
> -
>         intel_set_pipe_timings(intel_crtc, mode, adjusted_mode);
>
>         /* pipesrc and dspsize control the size that is scaled from,
> @@ -5759,9 +5750,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>         /* Ensure that the cursor is valid for the new mode before changing... */
>         intel_crtc_update_cursor(crtc, true);
>
> -       DRM_DEBUG_KMS("Mode for pipe %c:\n", pipe_name(pipe));
> -       drm_mode_debug_printmodeline(mode);
> -
>         /* CPU eDP is the only output that doesn't need a PCH PLL of its own. */
>         if (intel_crtc->config.has_pch_encoder) {
>                 struct intel_pch_pll *pll;
> @@ -5972,9 +5960,6 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
>         /* Ensure that the cursor is valid for the new mode before changing... */
>         intel_crtc_update_cursor(crtc, true);
>
> -       DRM_DEBUG_KMS("Mode for pipe %c:\n", pipe_name(pipe));
> -       drm_mode_debug_printmodeline(mode);
> -
>         if (intel_crtc->config.has_dp_encoder)
>                 intel_dp_set_m_n(intel_crtc);
>
> @@ -7783,6 +7768,35 @@ pipe_config_set_bpp(struct drm_crtc *crtc,
>         return bpp;
>  }
>
> +static void intel_dump_pipe_config(struct intel_crtc *crtc,
> +                                  struct intel_crtc_config *pipe_config,
> +                                  const char *context)
> +{
> +       DRM_DEBUG_KMS("[CRTC:%d]%s config for pipe %c\n", crtc->base.base.id,
> +                     context, pipe_name(crtc->pipe));
> +
> +       DRM_DEBUG_KMS("cpu_transcoder: %i\n", pipe_name(pipe_config->cpu_transcoder));

You should use %c, not %i. We also have the transcoder_name macro,
which will be useful if we ever decide we want to print "EDP" for the
EDP transcoder.


> +       DRM_DEBUG_KMS("pipe bpp: %i, dithering: %i\n",
> +                     pipe_config->pipe_bpp, pipe_config->dither);
> +       DRM_DEBUG_KMS("fdi/pch: %i, lanes: %i, gmch_m: %i, gmch_n %i, link_m: %i, link_n: %i, tu: %i\n",

You're missing a ':' character after gmch_n.


> +                     pipe_config->has_pch_encoder,
> +                     pipe_config->fdi_lanes,
> +                     pipe_config->fdi_m_n.gmch_m, pipe_config->fdi_m_n.gmch_n,
> +                     pipe_config->fdi_m_n.link_m, pipe_config->fdi_m_n.link_n,

These 4 are unsigned, so maybe %u or 0x%x?


> +                     pipe_config->fdi_m_n.tu);
> +       DRM_DEBUG_KMS("requested mode:\n");
> +       drm_mode_debug_printmodeline(&pipe_config->requested_mode);
> +       DRM_DEBUG_KMS("adjusted mode:\n");
> +       drm_mode_debug_printmodeline(&pipe_config->adjusted_mode);
> +       DRM_DEBUG_KMS("gmch pfit: control: 0x%08x, ratios: 0x%08x, lvds border: 0x%08x\n",
> +                     pipe_config->gmch_pfit.control,
> +                     pipe_config->gmch_pfit.pgm_ratios,
> +                     pipe_config->gmch_pfit.lvds_border_bits);
> +       DRM_DEBUG_KMS("pch pfit: pos: 0x%08x, size: 0x%08x\n",
> +                     pipe_config->pch_pfit.pos,
> +                     pipe_config->pch_pfit.size);

You could maybe break pos and size into x and y. Or just wait until
someone fully splits the struct itself into more pieces, as Ville
suggested a few days ago.


> +}
> +
>  static struct intel_crtc_config *
>  intel_modeset_pipe_config(struct drm_crtc *crtc,
>                           struct drm_framebuffer *fb,
> @@ -7853,8 +7867,6 @@ encoder_retry:
>                 goto encoder_retry;
>         }
>
> -       DRM_DEBUG_KMS("[CRTC:%d]\n", crtc->base.id);
> -
>         pipe_config->dither = pipe_config->pipe_bpp != plane_bpp;
>         DRM_DEBUG_KMS("plane bpp: %i, pipe bpp: %i, dithering: %i\n",
>                       plane_bpp, pipe_config->pipe_bpp, pipe_config->dither);
> @@ -8211,9 +8223,14 @@ intel_modeset_check_state(struct drm_device *dev)
>                      "crtc active state doesn't match with hw state "
>                      "(expected %i, found %i)\n", crtc->active, active);
>
> -               WARN(active &&
> -                    !intel_pipe_config_compare(dev, &crtc->config, &pipe_config),
> -                    "pipe state doesn't match!\n");
> +               if (active &&
> +                   !intel_pipe_config_compare(dev, &crtc->config, &pipe_config)) {
> +                       WARN(1, "pipe state doesn't match!\n");
> +                       intel_dump_pipe_config(crtc, &pipe_config,
> +                                              "[hw state]");
> +                       intel_dump_pipe_config(crtc, &crtc->config,
> +                                              "[sw state]");
> +               }
>         }
>  }
>
> @@ -8253,6 +8270,8 @@ static int __intel_set_mode(struct drm_crtc *crtc,
>
>                         goto out;
>                 }
> +               intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config,
> +                                      "[modeset]");
>         }
>
>         for_each_intel_crtc_masked(dev, disable_pipes, intel_crtc)
> @@ -8609,12 +8628,6 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
>                 goto fail;
>
>         if (config->mode_changed) {
> -               if (set->mode) {
> -                       DRM_DEBUG_KMS("attempting to set mode from"
> -                                       " userspace\n");
> -                       drm_mode_debug_printmodeline(set->mode);
> -               }
> -
>                 ret = intel_set_mode(set->crtc, set->mode,
>                                      set->x, set->y, set->fb);
>         } else if (config->fb_changed) {

Patch doesn't apply anymore due to a recent code change here.

Besides this, I also booted a kernel with this patch, and on the first
time we print these things, almost everything is zero, even stuff like
pipe_bpp, even for enabled pipes. Is this expected?



> @@ -9629,6 +9642,7 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
>         for_each_pipe(pipe) {
>                 crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
>                 intel_sanitize_crtc(crtc);
> +               intel_dump_pipe_config(crtc, &crtc->config, "[setup_hw_state]");
>         }
>
>         if (force_restore) {
> --
> 1.8.1.4
>



-- 
Paulo Zanoni

Re: [PATCH 3/3] drm/i915: add basic pipe config dump support

[Daniel Vetter]

On Mon, May 27, 2013 at 06:30:17PM -0300, Paulo Zanoni wrote:
> 2013/5/21 Daniel Vetter <daniel.vetter@ffwll.ch>:
> Besides this, I also booted a kernel with this patch, and on the first
> time we print these things, almost everything is zero, even stuff like
> pipe_bpp, even for enabled pipes. Is this expected?

Lack of hw state readout support for those. One of the goals of this is
that it's easier to see what pipe config the kernel is actually dealing
with, and knowing that we have no clue about the pipe bpp is useuful.

I'll respin the patch with your other comments fixed up.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/3] drm/i915: hw state readout&check support for cpu_transcoder

[Daniel Vetter]

On Mon, May 27, 2013 at 05:45:53PM -0300, Paulo Zanoni wrote:
> 2013/5/21 Daniel Vetter <daniel.vetter@ffwll.ch>:
> > +       pipe_config->cpu_transcoder = crtc->pipe;
> > +       tmp = I915_READ(TRANS_DDI_FUNC_CTL(TRANSCODER_EDP));
> > +       if (tmp & TRANS_DDI_FUNC_ENABLE) {
> > +               enum pipe trans_edp_pipe;
> > +               switch (tmp & TRANS_DDI_EDP_INPUT_MASK) {
> > +               default:
> > +                       WARN(1, "unknown pipe linked to edp transcoder\n");
> 
> Since there's no "break" we'll assign PIPE_A and then we'll probably
> break pipe A on this case. OTOH this case should never really happen,
> so I'm not sure if we care. So this is an optional bikeshedding.

Generally I prefer to have as little fallback code as possible for such
impossible cases. Avoiding the break makes sure we have at elast a valid
pipe value and so hopefully increase our chances that the following code
survives. No matter what we're pretty much guaranteed to end up with a
black screen.

> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Thanks for the review, first 2 patches are merged to dinq now.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH] drm/i915: add basic pipe config dump support

[Daniel Vetter]

All this pipe config abstraction adds another layer of complexity, so
it's good to have better visibility into what's going on exactly.
Doesn't dump out everything yet, and some bits are a bit duplicated
but this should be a good start.

Note that at boot-up a lot of the fields are 0 even for enabled pipes,
this is simply because our hw state readout code doesn't support
everything.

v2: Remove a few more now redudant debug output lines.

v3: Review from Paulo
- use transcoder_name
- fix up format specifiers
- add missing ':' in debug output

Cc: Paulo Zanoni <przanoni@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 74 +++++++++++++++++++++---------------
 1 file changed, 44 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 57fd85e..87d8bc9 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3528,18 +3528,12 @@ static void i9xx_pfit_enable(struct intel_crtc *crtc)
 	if (!crtc->config.gmch_pfit.control)
 		return;
 
-	WARN_ON(I915_READ(PFIT_CONTROL) & PFIT_ENABLE);
-	assert_pipe_disabled(dev_priv, crtc->pipe);
-
 	/*
-	 * Enable automatic panel scaling so that non-native modes
-	 * fill the screen.  The panel fitter should only be
-	 * adjusted whilst the pipe is disabled, according to
-	 * register description and PRM.
+	 * The panel fitter should only be adjusted whilst the pipe is disabled,
+	 * according to register description and PRM.
 	 */
-	DRM_DEBUG_KMS("applying panel-fitter: %x, %x\n",
-		      pipe_config->gmch_pfit.control,
-		      pipe_config->gmch_pfit.pgm_ratios);
+	WARN_ON(I915_READ(PFIT_CONTROL) & PFIT_ENABLE);
+	assert_pipe_disabled(dev_priv, crtc->pipe);
 
 	I915_WRITE(PFIT_PGM_RATIOS, pipe_config->gmch_pfit.pgm_ratios);
 	I915_WRITE(PFIT_CONTROL, pipe_config->gmch_pfit.control);
@@ -4862,9 +4856,6 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
 			dspcntr |= DISPPLANE_SEL_PIPE_B;
 	}
 
-	DRM_DEBUG_KMS("Mode for pipe %c:\n", pipe_name(pipe));
-	drm_mode_debug_printmodeline(mode);
-
 	intel_set_pipe_timings(intel_crtc, mode, adjusted_mode);
 
 	/* pipesrc and dspsize control the size that is scaled from,
@@ -5666,9 +5657,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	/* Ensure that the cursor is valid for the new mode before changing... */
 	intel_crtc_update_cursor(crtc, true);
 
-	DRM_DEBUG_KMS("Mode for pipe %c:\n", pipe_name(pipe));
-	drm_mode_debug_printmodeline(mode);
-
 	/* CPU eDP is the only output that doesn't need a PCH PLL of its own. */
 	if (intel_crtc->config.has_pch_encoder) {
 		struct intel_pch_pll *pll;
@@ -5879,9 +5867,6 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
 	/* Ensure that the cursor is valid for the new mode before changing... */
 	intel_crtc_update_cursor(crtc, true);
 
-	DRM_DEBUG_KMS("Mode for pipe %c:\n", pipe_name(pipe));
-	drm_mode_debug_printmodeline(mode);
-
 	if (intel_crtc->config.has_dp_encoder)
 		intel_dp_set_m_n(intel_crtc);
 
@@ -7690,6 +7675,35 @@ pipe_config_set_bpp(struct drm_crtc *crtc,
 	return bpp;
 }
 
+static void intel_dump_pipe_config(struct intel_crtc *crtc,
+				   struct intel_crtc_config *pipe_config,
+				   const char *context)
+{
+	DRM_DEBUG_KMS("[CRTC:%d]%s config for pipe %c\n", crtc->base.base.id,
+		      context, pipe_name(crtc->pipe));
+
+	DRM_DEBUG_KMS("cpu_transcoder: %c\n", transcoder_name(pipe_config->cpu_transcoder));
+	DRM_DEBUG_KMS("pipe bpp: %i, dithering: %i\n",
+		      pipe_config->pipe_bpp, pipe_config->dither);
+	DRM_DEBUG_KMS("fdi/pch: %i, lanes: %i, gmch_m: %u, gmch_n: %u, link_m: %u, link_n: %u, tu: %u\n",
+		      pipe_config->has_pch_encoder,
+		      pipe_config->fdi_lanes,
+		      pipe_config->fdi_m_n.gmch_m, pipe_config->fdi_m_n.gmch_n,
+		      pipe_config->fdi_m_n.link_m, pipe_config->fdi_m_n.link_n,
+		      pipe_config->fdi_m_n.tu);
+	DRM_DEBUG_KMS("requested mode:\n");
+	drm_mode_debug_printmodeline(&pipe_config->requested_mode);
+	DRM_DEBUG_KMS("adjusted mode:\n");
+	drm_mode_debug_printmodeline(&pipe_config->adjusted_mode);
+	DRM_DEBUG_KMS("gmch pfit: control: 0x%08x, ratios: 0x%08x, lvds border: 0x%08x\n",
+		      pipe_config->gmch_pfit.control,
+		      pipe_config->gmch_pfit.pgm_ratios,
+		      pipe_config->gmch_pfit.lvds_border_bits);
+	DRM_DEBUG_KMS("pch pfit: pos: 0x%08x, size: 0x%08x\n",
+		      pipe_config->pch_pfit.pos,
+		      pipe_config->pch_pfit.size);
+}
+
 static struct intel_crtc_config *
 intel_modeset_pipe_config(struct drm_crtc *crtc,
 			  struct drm_framebuffer *fb,
@@ -7760,8 +7774,6 @@ encoder_retry:
 		goto encoder_retry;
 	}
 
-	DRM_DEBUG_KMS("[CRTC:%d]\n", crtc->base.id);
-
 	pipe_config->dither = pipe_config->pipe_bpp != plane_bpp;
 	DRM_DEBUG_KMS("plane bpp: %i, pipe bpp: %i, dithering: %i\n",
 		      plane_bpp, pipe_config->pipe_bpp, pipe_config->dither);
@@ -8118,9 +8130,14 @@ intel_modeset_check_state(struct drm_device *dev)
 		     "crtc active state doesn't match with hw state "
 		     "(expected %i, found %i)\n", crtc->active, active);
 
-		WARN(active &&
-		     !intel_pipe_config_compare(dev, &crtc->config, &pipe_config),
-		     "pipe state doesn't match!\n");
+		if (active &&
+		    !intel_pipe_config_compare(dev, &crtc->config, &pipe_config)) {
+			WARN(1, "pipe state doesn't match!\n");
+			intel_dump_pipe_config(crtc, &pipe_config,
+					       "[hw state]");
+			intel_dump_pipe_config(crtc, &crtc->config,
+					       "[sw state]");
+		}
 	}
 }
 
@@ -8160,6 +8177,8 @@ static int __intel_set_mode(struct drm_crtc *crtc,
 
 			goto out;
 		}
+		intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config,
+				       "[modeset]");
 	}
 
 	for_each_intel_crtc_masked(dev, disable_pipes, intel_crtc)
@@ -8516,12 +8535,6 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
 		goto fail;
 
 	if (config->mode_changed) {
-		if (set->mode) {
-			DRM_DEBUG_KMS("attempting to set mode from"
-					" userspace\n");
-			drm_mode_debug_printmodeline(set->mode);
-		}
-
 		ret = intel_set_mode(set->crtc, set->mode,
 				     set->x, set->y, set->fb);
 	} else if (config->fb_changed) {
@@ -9536,6 +9549,7 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
 	for_each_pipe(pipe) {
 		crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
 		intel_sanitize_crtc(crtc);
+		intel_dump_pipe_config(crtc, &crtc->config, "[setup_hw_state]");
 	}
 
 	if (force_restore) {
-- 
1.8.1.4

Re: [PATCH] drm/i915: add basic pipe config dump support

[Paulo Zanoni]

2013/5/28 Daniel Vetter <daniel.vetter@ffwll.ch>:
> All this pipe config abstraction adds another layer of complexity, so
> it's good to have better visibility into what's going on exactly.
> Doesn't dump out everything yet, and some bits are a bit duplicated
> but this should be a good start.
>
> Note that at boot-up a lot of the fields are 0 even for enabled pipes,
> this is simply because our hw state readout code doesn't support
> everything.
>
> v2: Remove a few more now redudant debug output lines.
>
> v3: Review from Paulo
> - use transcoder_name
> - fix up format specifiers
> - add missing ':' in debug output
>
> Cc: Paulo Zanoni <przanoni@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_display.c | 74 +++++++++++++++++++++---------------
>  1 file changed, 44 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 57fd85e..87d8bc9 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3528,18 +3528,12 @@ static void i9xx_pfit_enable(struct intel_crtc *crtc)
>         if (!crtc->config.gmch_pfit.control)
>                 return;
>
> -       WARN_ON(I915_READ(PFIT_CONTROL) & PFIT_ENABLE);
> -       assert_pipe_disabled(dev_priv, crtc->pipe);
> -
>         /*
> -        * Enable automatic panel scaling so that non-native modes
> -        * fill the screen.  The panel fitter should only be
> -        * adjusted whilst the pipe is disabled, according to
> -        * register description and PRM.
> +        * The panel fitter should only be adjusted whilst the pipe is disabled,
> +        * according to register description and PRM.
>          */
> -       DRM_DEBUG_KMS("applying panel-fitter: %x, %x\n",
> -                     pipe_config->gmch_pfit.control,
> -                     pipe_config->gmch_pfit.pgm_ratios);
> +       WARN_ON(I915_READ(PFIT_CONTROL) & PFIT_ENABLE);
> +       assert_pipe_disabled(dev_priv, crtc->pipe);
>
>         I915_WRITE(PFIT_PGM_RATIOS, pipe_config->gmch_pfit.pgm_ratios);
>         I915_WRITE(PFIT_CONTROL, pipe_config->gmch_pfit.control);
> @@ -4862,9 +4856,6 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
>                         dspcntr |= DISPPLANE_SEL_PIPE_B;
>         }
>
> -       DRM_DEBUG_KMS("Mode for pipe %c:\n", pipe_name(pipe));
> -       drm_mode_debug_printmodeline(mode);
> -
>         intel_set_pipe_timings(intel_crtc, mode, adjusted_mode);
>
>         /* pipesrc and dspsize control the size that is scaled from,
> @@ -5666,9 +5657,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>         /* Ensure that the cursor is valid for the new mode before changing... */
>         intel_crtc_update_cursor(crtc, true);
>
> -       DRM_DEBUG_KMS("Mode for pipe %c:\n", pipe_name(pipe));
> -       drm_mode_debug_printmodeline(mode);
> -
>         /* CPU eDP is the only output that doesn't need a PCH PLL of its own. */
>         if (intel_crtc->config.has_pch_encoder) {
>                 struct intel_pch_pll *pll;
> @@ -5879,9 +5867,6 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
>         /* Ensure that the cursor is valid for the new mode before changing... */
>         intel_crtc_update_cursor(crtc, true);
>
> -       DRM_DEBUG_KMS("Mode for pipe %c:\n", pipe_name(pipe));
> -       drm_mode_debug_printmodeline(mode);
> -
>         if (intel_crtc->config.has_dp_encoder)
>                 intel_dp_set_m_n(intel_crtc);
>
> @@ -7690,6 +7675,35 @@ pipe_config_set_bpp(struct drm_crtc *crtc,
>         return bpp;
>  }
>
> +static void intel_dump_pipe_config(struct intel_crtc *crtc,
> +                                  struct intel_crtc_config *pipe_config,
> +                                  const char *context)
> +{
> +       DRM_DEBUG_KMS("[CRTC:%d]%s config for pipe %c\n", crtc->base.base.id,
> +                     context, pipe_name(crtc->pipe));
> +
> +       DRM_DEBUG_KMS("cpu_transcoder: %c\n", transcoder_name(pipe_config->cpu_transcoder));
> +       DRM_DEBUG_KMS("pipe bpp: %i, dithering: %i\n",
> +                     pipe_config->pipe_bpp, pipe_config->dither);
> +       DRM_DEBUG_KMS("fdi/pch: %i, lanes: %i, gmch_m: %u, gmch_n: %u, link_m: %u, link_n: %u, tu: %u\n",
> +                     pipe_config->has_pch_encoder,
> +                     pipe_config->fdi_lanes,
> +                     pipe_config->fdi_m_n.gmch_m, pipe_config->fdi_m_n.gmch_n,
> +                     pipe_config->fdi_m_n.link_m, pipe_config->fdi_m_n.link_n,
> +                     pipe_config->fdi_m_n.tu);
> +       DRM_DEBUG_KMS("requested mode:\n");
> +       drm_mode_debug_printmodeline(&pipe_config->requested_mode);
> +       DRM_DEBUG_KMS("adjusted mode:\n");
> +       drm_mode_debug_printmodeline(&pipe_config->adjusted_mode);
> +       DRM_DEBUG_KMS("gmch pfit: control: 0x%08x, ratios: 0x%08x, lvds border: 0x%08x\n",
> +                     pipe_config->gmch_pfit.control,
> +                     pipe_config->gmch_pfit.pgm_ratios,
> +                     pipe_config->gmch_pfit.lvds_border_bits);
> +       DRM_DEBUG_KMS("pch pfit: pos: 0x%08x, size: 0x%08x\n",
> +                     pipe_config->pch_pfit.pos,
> +                     pipe_config->pch_pfit.size);
> +}
> +
>  static struct intel_crtc_config *
>  intel_modeset_pipe_config(struct drm_crtc *crtc,
>                           struct drm_framebuffer *fb,
> @@ -7760,8 +7774,6 @@ encoder_retry:
>                 goto encoder_retry;
>         }
>
> -       DRM_DEBUG_KMS("[CRTC:%d]\n", crtc->base.id);
> -
>         pipe_config->dither = pipe_config->pipe_bpp != plane_bpp;
>         DRM_DEBUG_KMS("plane bpp: %i, pipe bpp: %i, dithering: %i\n",
>                       plane_bpp, pipe_config->pipe_bpp, pipe_config->dither);
> @@ -8118,9 +8130,14 @@ intel_modeset_check_state(struct drm_device *dev)
>                      "crtc active state doesn't match with hw state "
>                      "(expected %i, found %i)\n", crtc->active, active);
>
> -               WARN(active &&
> -                    !intel_pipe_config_compare(dev, &crtc->config, &pipe_config),
> -                    "pipe state doesn't match!\n");
> +               if (active &&
> +                   !intel_pipe_config_compare(dev, &crtc->config, &pipe_config)) {
> +                       WARN(1, "pipe state doesn't match!\n");
> +                       intel_dump_pipe_config(crtc, &pipe_config,
> +                                              "[hw state]");
> +                       intel_dump_pipe_config(crtc, &crtc->config,
> +                                              "[sw state]");
> +               }
>         }
>  }
>
> @@ -8160,6 +8177,8 @@ static int __intel_set_mode(struct drm_crtc *crtc,
>
>                         goto out;
>                 }
> +               intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config,
> +                                      "[modeset]");
>         }
>
>         for_each_intel_crtc_masked(dev, disable_pipes, intel_crtc)
> @@ -8516,12 +8535,6 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
>                 goto fail;
>
>         if (config->mode_changed) {
> -               if (set->mode) {
> -                       DRM_DEBUG_KMS("attempting to set mode from"
> -                                       " userspace\n");
> -                       drm_mode_debug_printmodeline(set->mode);
> -               }
> -
>                 ret = intel_set_mode(set->crtc, set->mode,
>                                      set->x, set->y, set->fb);
>         } else if (config->fb_changed) {
> @@ -9536,6 +9549,7 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
>         for_each_pipe(pipe) {
>                 crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
>                 intel_sanitize_crtc(crtc);
> +               intel_dump_pipe_config(crtc, &crtc->config, "[setup_hw_state]");
>         }
>
>         if (force_restore) {
> --
> 1.8.1.4
>



-- 
Paulo Zanoni

Re: [PATCH] drm/i915: add basic pipe config dump support

[Daniel Vetter]

On Tue, May 28, 2013 at 11:17:34AM -0300, Paulo Zanoni wrote:
> 2013/5/28 Daniel Vetter <daniel.vetter@ffwll.ch>:
> > All this pipe config abstraction adds another layer of complexity, so
> > it's good to have better visibility into what's going on exactly.
> > Doesn't dump out everything yet, and some bits are a bit duplicated
> > but this should be a good start.
> >
> > Note that at boot-up a lot of the fields are 0 even for enabled pipes,
> > this is simply because our hw state readout code doesn't support
> > everything.
> >
> > v2: Remove a few more now redudant debug output lines.
> >
> > v3: Review from Paulo
> > - use transcoder_name
> > - fix up format specifiers
> > - add missing ':' in debug output
> >
> > Cc: Paulo Zanoni <przanoni@gmail.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Queued for -next, thanks for the review.
-Daniel
> 
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 74 +++++++++++++++++++++---------------
> >  1 file changed, 44 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 57fd85e..87d8bc9 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -3528,18 +3528,12 @@ static void i9xx_pfit_enable(struct intel_crtc *crtc)
> >         if (!crtc->config.gmch_pfit.control)
> >                 return;
> >
> > -       WARN_ON(I915_READ(PFIT_CONTROL) & PFIT_ENABLE);
> > -       assert_pipe_disabled(dev_priv, crtc->pipe);
> > -
> >         /*
> > -        * Enable automatic panel scaling so that non-native modes
> > -        * fill the screen.  The panel fitter should only be
> > -        * adjusted whilst the pipe is disabled, according to
> > -        * register description and PRM.
> > +        * The panel fitter should only be adjusted whilst the pipe is disabled,
> > +        * according to register description and PRM.
> >          */
> > -       DRM_DEBUG_KMS("applying panel-fitter: %x, %x\n",
> > -                     pipe_config->gmch_pfit.control,
> > -                     pipe_config->gmch_pfit.pgm_ratios);
> > +       WARN_ON(I915_READ(PFIT_CONTROL) & PFIT_ENABLE);
> > +       assert_pipe_disabled(dev_priv, crtc->pipe);
> >
> >         I915_WRITE(PFIT_PGM_RATIOS, pipe_config->gmch_pfit.pgm_ratios);
> >         I915_WRITE(PFIT_CONTROL, pipe_config->gmch_pfit.control);
> > @@ -4862,9 +4856,6 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
> >                         dspcntr |= DISPPLANE_SEL_PIPE_B;
> >         }
> >
> > -       DRM_DEBUG_KMS("Mode for pipe %c:\n", pipe_name(pipe));
> > -       drm_mode_debug_printmodeline(mode);
> > -
> >         intel_set_pipe_timings(intel_crtc, mode, adjusted_mode);
> >
> >         /* pipesrc and dspsize control the size that is scaled from,
> > @@ -5666,9 +5657,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
> >         /* Ensure that the cursor is valid for the new mode before changing... */
> >         intel_crtc_update_cursor(crtc, true);
> >
> > -       DRM_DEBUG_KMS("Mode for pipe %c:\n", pipe_name(pipe));
> > -       drm_mode_debug_printmodeline(mode);
> > -
> >         /* CPU eDP is the only output that doesn't need a PCH PLL of its own. */
> >         if (intel_crtc->config.has_pch_encoder) {
> >                 struct intel_pch_pll *pll;
> > @@ -5879,9 +5867,6 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
> >         /* Ensure that the cursor is valid for the new mode before changing... */
> >         intel_crtc_update_cursor(crtc, true);
> >
> > -       DRM_DEBUG_KMS("Mode for pipe %c:\n", pipe_name(pipe));
> > -       drm_mode_debug_printmodeline(mode);
> > -
> >         if (intel_crtc->config.has_dp_encoder)
> >                 intel_dp_set_m_n(intel_crtc);
> >
> > @@ -7690,6 +7675,35 @@ pipe_config_set_bpp(struct drm_crtc *crtc,
> >         return bpp;
> >  }
> >
> > +static void intel_dump_pipe_config(struct intel_crtc *crtc,
> > +                                  struct intel_crtc_config *pipe_config,
> > +                                  const char *context)
> > +{
> > +       DRM_DEBUG_KMS("[CRTC:%d]%s config for pipe %c\n", crtc->base.base.id,
> > +                     context, pipe_name(crtc->pipe));
> > +
> > +       DRM_DEBUG_KMS("cpu_transcoder: %c\n", transcoder_name(pipe_config->cpu_transcoder));
> > +       DRM_DEBUG_KMS("pipe bpp: %i, dithering: %i\n",
> > +                     pipe_config->pipe_bpp, pipe_config->dither);
> > +       DRM_DEBUG_KMS("fdi/pch: %i, lanes: %i, gmch_m: %u, gmch_n: %u, link_m: %u, link_n: %u, tu: %u\n",
> > +                     pipe_config->has_pch_encoder,
> > +                     pipe_config->fdi_lanes,
> > +                     pipe_config->fdi_m_n.gmch_m, pipe_config->fdi_m_n.gmch_n,
> > +                     pipe_config->fdi_m_n.link_m, pipe_config->fdi_m_n.link_n,
> > +                     pipe_config->fdi_m_n.tu);
> > +       DRM_DEBUG_KMS("requested mode:\n");
> > +       drm_mode_debug_printmodeline(&pipe_config->requested_mode);
> > +       DRM_DEBUG_KMS("adjusted mode:\n");
> > +       drm_mode_debug_printmodeline(&pipe_config->adjusted_mode);
> > +       DRM_DEBUG_KMS("gmch pfit: control: 0x%08x, ratios: 0x%08x, lvds border: 0x%08x\n",
> > +                     pipe_config->gmch_pfit.control,
> > +                     pipe_config->gmch_pfit.pgm_ratios,
> > +                     pipe_config->gmch_pfit.lvds_border_bits);
> > +       DRM_DEBUG_KMS("pch pfit: pos: 0x%08x, size: 0x%08x\n",
> > +                     pipe_config->pch_pfit.pos,
> > +                     pipe_config->pch_pfit.size);
> > +}
> > +
> >  static struct intel_crtc_config *
> >  intel_modeset_pipe_config(struct drm_crtc *crtc,
> >                           struct drm_framebuffer *fb,
> > @@ -7760,8 +7774,6 @@ encoder_retry:
> >                 goto encoder_retry;
> >         }
> >
> > -       DRM_DEBUG_KMS("[CRTC:%d]\n", crtc->base.id);
> > -
> >         pipe_config->dither = pipe_config->pipe_bpp != plane_bpp;
> >         DRM_DEBUG_KMS("plane bpp: %i, pipe bpp: %i, dithering: %i\n",
> >                       plane_bpp, pipe_config->pipe_bpp, pipe_config->dither);
> > @@ -8118,9 +8130,14 @@ intel_modeset_check_state(struct drm_device *dev)
> >                      "crtc active state doesn't match with hw state "
> >                      "(expected %i, found %i)\n", crtc->active, active);
> >
> > -               WARN(active &&
> > -                    !intel_pipe_config_compare(dev, &crtc->config, &pipe_config),
> > -                    "pipe state doesn't match!\n");
> > +               if (active &&
> > +                   !intel_pipe_config_compare(dev, &crtc->config, &pipe_config)) {
> > +                       WARN(1, "pipe state doesn't match!\n");
> > +                       intel_dump_pipe_config(crtc, &pipe_config,
> > +                                              "[hw state]");
> > +                       intel_dump_pipe_config(crtc, &crtc->config,
> > +                                              "[sw state]");
> > +               }
> >         }
> >  }
> >
> > @@ -8160,6 +8177,8 @@ static int __intel_set_mode(struct drm_crtc *crtc,
> >
> >                         goto out;
> >                 }
> > +               intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config,
> > +                                      "[modeset]");
> >         }
> >
> >         for_each_intel_crtc_masked(dev, disable_pipes, intel_crtc)
> > @@ -8516,12 +8535,6 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
> >                 goto fail;
> >
> >         if (config->mode_changed) {
> > -               if (set->mode) {
> > -                       DRM_DEBUG_KMS("attempting to set mode from"
> > -                                       " userspace\n");
> > -                       drm_mode_debug_printmodeline(set->mode);
> > -               }
> > -
> >                 ret = intel_set_mode(set->crtc, set->mode,
> >                                      set->x, set->y, set->fb);
> >         } else if (config->fb_changed) {
> > @@ -9536,6 +9549,7 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
> >         for_each_pipe(pipe) {
> >                 crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
> >                 intel_sanitize_crtc(crtc);
> > +               intel_dump_pipe_config(crtc, &crtc->config, "[setup_hw_state]");
> >         }
> >
> >         if (force_restore) {
> > --
> > 1.8.1.4
> >
> 
> 
> 
> -- 
> Paulo Zanoni

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