Re: [PATCH v4 19/27] drm/i915: Read hw state into an atomic state struct, v2.

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Hey,

Op 09-06-15 om 13:48 schreef Tvrtko Ursulin:
>
> On 06/01/2015 11:50 AM, Maarten Lankhorst wrote:
>> From: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
>>
>> To make this work we load the new hardware state into the
>> atomic_state, then swap it with the sw state.
>>
>> This lets us change the force restore path in setup_hw_state()
>> to use a single call to intel_mode_set() to restore all the
>> previous state.
>>
>> As a nice bonus this kills off encoder->new_encoder,
>> connector->new_enabled and crtc->new_enabled. They were used only
>> to restore the state after a modeset.
>>
>> Changes since v1:
>> - Make sure all possible planes are added with their crtc set,
>>    so they will be turned off on first modeset.
>>
>> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>
> This breaks display for me, which is eDP on SKL. At least bisect points to it. A lot of these in the logs:
>
> *ERROR* mismatch in dp_m_n.link_m (expected 701594 or 0, found 350797)
>
> And the display does not light up.

Yeah, it probably relies on better hw readout. This is partially mitigated by convert to atomic, part 3.
But it requires 2 additional patches.

commit 5a97529becb25fabf18a3507a94f892c365a4a1d
Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Date:   Mon Jun 8 11:31:28 2015 +0200

    drm/i915: update more sw state with hw state during atomic readout
    
    I've noticed the following during initial readout:
    state->adjusted_mode's non crtc_* members were not set,
    but some code relies hdisplay and vdisplay, so make sure it's
    set correctly.
    
    Also vblank was not enabled because constants were not calculated,
    this shows up in dmesg as:
    [drm:drm_vblank_enable] enabling vblank on crtc 0, ret: 0
    [drm:drm_calc_vbltimestamp_from_scanoutpos] crtc 0: Noop due to uninitialized mode.
    
    This commit fixes the regression from the following commit:
    
    commit 3bae26eb2991c00670df377cf6c3bc2b0577e82a
    Author: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
    Date:   Mon Jun 1 12:50:03 2015 +0200
    
        drm/i915: Read hw state into an atomic state struct, v2.
    
    Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90861
    Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index fb9f07b1e5ca..dc29bab527d7 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14859,8 +14859,9 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
 
 	/* restore vblank interrupts to correct state */
 	drm_crtc_vblank_reset(&crtc->base);
-	if (crtc->active) {
+	if (crtc->base.state->active) {
 		update_scanline_offset(crtc);
+		drm_calc_timestamping_constants(&crtc->base, &crtc->base.hwmode);
 		drm_crtc_vblank_on(&crtc->base);
 	}
 
@@ -15307,6 +15308,8 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
 		if (crtc->enabled) {
 			intel_mode_from_pipe_config(&crtc->state->mode,
 				to_intel_crtc_state(crtc->state));
+			intel_mode_from_pipe_config(&crtc->state->adjusted_mode,
+				to_intel_crtc_state(crtc->state));
 
 			drm_mode_copy(&crtc->mode, &crtc->state->mode);
 			drm_mode_copy(&crtc->hwmode,

commit d0f7e7ae8a151e9d018e2dbf36a5afba812bab4f
Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Date:   Tue Jun 9 09:01:17 2015 +0200

    drm/i915: only perform a single modeset in intel_modeset_setup_hw_state

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 29ae92e5c8a9..77a553e21a7a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11994,24 +11994,35 @@ static bool intel_fuzzy_clock_check(int clock1, int clock2)
 static bool
 intel_pipe_config_compare(struct drm_device *dev,
 			  struct intel_crtc_state *current_config,
-			  struct intel_crtc_state *pipe_config)
+			  struct intel_crtc_state *pipe_config,
+			  bool check_only)
 {
+	bool ret = true;
+
+#define INTEL_ERR_OR_DBG_KMS(fmt, ...) \
+	do { \
+		if (check_only) \
+			DRM_ERROR(fmt, ##__VA_ARGS__); \
+		else \
+			DRM_DEBUG_KMS(fmt, ##__VA_ARGS__); \
+	} while (0)
+
 #define PIPE_CONF_CHECK_X(name)	\
 	if (current_config->name != pipe_config->name) { \
-		DRM_ERROR("mismatch in " #name " " \
+		INTEL_ERR_OR_DBG_KMS("mismatch in " #name " " \
 			  "(expected 0x%08x, found 0x%08x)\n", \
 			  current_config->name, \
 			  pipe_config->name); \
-		return false; \
+		ret = false; \
 	}
 
 #define PIPE_CONF_CHECK_I(name)	\
 	if (current_config->name != pipe_config->name) { \
-		DRM_ERROR("mismatch in " #name " " \
+		INTEL_ERR_OR_DBG_KMS("mismatch in " #name " " \
 			  "(expected %i, found %i)\n", \
 			  current_config->name, \
 			  pipe_config->name); \
-		return false; \
+		ret = false; \
 	}
 
 /* This is required for BDW+ where there is only one set of registers for
@@ -12022,30 +12033,30 @@ intel_pipe_config_compare(struct drm_device *dev,
 #define PIPE_CONF_CHECK_I_ALT(name, alt_name) \
 	if ((current_config->name != pipe_config->name) && \
 		(current_config->alt_name != pipe_config->name)) { \
-			DRM_ERROR("mismatch in " #name " " \
+			INTEL_ERR_OR_DBG_KMS("mismatch in " #name " " \
 				  "(expected %i or %i, found %i)\n", \
 				  current_config->name, \
 				  current_config->alt_name, \
 				  pipe_config->name); \
-			return false; \
+			ret = false; \
 	}
 
 #define PIPE_CONF_CHECK_FLAGS(name, mask)	\
 	if ((current_config->name ^ pipe_config->name) & (mask)) { \
-		DRM_ERROR("mismatch in " #name "(" #mask ") "	   \
+		INTEL_ERR_OR_DBG_KMS("mismatch in " #name "(" #mask ") "	   \
 			  "(expected %i, found %i)\n", \
 			  current_config->name & (mask), \
 			  pipe_config->name & (mask)); \
-		return false; \
+		ret = false; \
 	}
 
 #define PIPE_CONF_CHECK_CLOCK_FUZZY(name) \
 	if (!intel_fuzzy_clock_check(current_config->name, pipe_config->name)) { \
-		DRM_ERROR("mismatch in " #name " " \
+		INTEL_ERR_OR_DBG_KMS("mismatch in " #name " " \
 			  "(expected %i, found %i)\n", \
 			  current_config->name, \
 			  pipe_config->name); \
-		return false; \
+		ret = false; \
 	}
 
 #define PIPE_CONF_QUIRK(quirk)	\
@@ -12179,8 +12190,9 @@ intel_pipe_config_compare(struct drm_device *dev,
 #undef PIPE_CONF_CHECK_FLAGS
 #undef PIPE_CONF_CHECK_CLOCK_FUZZY
 #undef PIPE_CONF_QUIRK
+#undef INTEL_ERR_OR_DBG_KMS
 
-	return true;
+	return ret;
 }
 
 static void check_wm_state(struct drm_device *dev)
@@ -12377,7 +12389,7 @@ check_crtc_state(struct drm_device *dev)
 		     "(expected %i, found %i)\n", crtc->base.state->active, crtc->active);
 
 		if (active &&
-		    !intel_pipe_config_compare(dev, crtc->config, &pipe_config)) {
+		    !intel_pipe_config_compare(dev, crtc->config, &pipe_config, false)) {
 			I915_STATE_WARN(1, "pipe state doesn't match!\n");
 			intel_dump_pipe_config(crtc, &pipe_config,
 					       "[hw state]");
@@ -12734,6 +12746,12 @@ static int intel_atomic_commit(struct drm_device *dev,
 			intel_crtc->active = false;
 			intel_disable_shared_dpll(intel_crtc);
 		}
+
+		/* FIXME: Move this to i9xx_crtc_disable when it gets a pointer
+		 * to the old crtc_state. */
+		if (to_intel_crtc_state(crtc_state)->quirks &
+		    PIPE_CONFIG_QUIRK_WRONG_PLANE)
+			intel_crtc->plane = !intel_crtc->plane;
 	}
 
 	/* Only after disabling all output pipelines that will be changed can we
@@ -14464,13 +14482,16 @@ intel_check_plane_mapping(struct intel_crtc *crtc)
 }
 
 static void intel_sanitize_crtc(struct intel_crtc *crtc,
-				struct intel_crtc_state *pipe_config)
+				struct intel_crtc_state *pipe_config,
+				bool force_restore)
 {
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_encoder *intel_encoder;
+	struct drm_atomic_state *state = pipe_config->base.state;
+	struct intel_crtc_state *hw_state =
+		to_intel_crtc_state(crtc->base.state);
 	u32 reg;
-	bool enable;
 
 	/* Clear any frame start delays used for debugging left by the BIOS */
 	reg = PIPECONF(crtc->config->cpu_transcoder);
@@ -14484,28 +14505,64 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc,
 		drm_crtc_vblank_on(&crtc->base);
 	}
 
+	/* set up current state */
+	if (!force_restore && hw_state->base.active) {
+		bool enable;
+
+		memcpy(pipe_config, hw_state, sizeof(*pipe_config));
+		__drm_atomic_helper_crtc_duplicate_state(&crtc->base, &pipe_config->base);
+		pipe_config->base.state = state;
+
+		enable = false;
+		for_each_encoder_on_crtc(dev, &crtc->base, intel_encoder)
+			enable |= intel_encoder->connectors_active;
+
+		pipe_config->base.active = !!enable;
+	}
+
 	/* We need to sanitize the plane -> pipe mapping first because this will
 	 * disable the crtc (and hence change the state) if it is wrong. Note
 	 * that gen4+ has a fixed plane -> pipe mapping.  */
 	if (INTEL_INFO(dev)->gen < 4 && !intel_check_plane_mapping(crtc)) {
-		bool plane;
-
 		DRM_DEBUG_KMS("[CRTC:%d] wrong plane connection detected!\n",
 			      crtc->base.base.id);
 
 		/* Pipe has the wrong plane attached and the plane is active.
 		 * Temporarily change the plane mapping and disable everything
 		 * ...  */
-		plane = crtc->plane;
-		to_intel_plane_state(crtc->base.primary->state)->visible = true;
-		crtc->base.primary->crtc = &crtc->base;
-		crtc->plane = !plane;
-		intel_crtc_control(&crtc->base, false, true);
-		crtc->plane = plane;
+		hw_state->quirks |=
+			PIPE_CONFIG_QUIRK_WRONG_PLANE;
+
+		crtc->plane = !crtc->plane;
+
+		if (force_restore)
+			pipe_config->base.mode_changed = true;
+		else
+			pipe_config->base.active = false;
 	}
 
-	if (dev_priv->quirks & QUIRK_PIPEA_FORCE &&
-	    crtc->pipe == PIPE_A && (!pipe_config || !pipe_config->base.active)) {
+	/* XXX: This is not active right now */
+	if (hw_state->base.active && pipe_config->base.active &&
+	    !i915.fastboot) {
+		struct intel_crtc_state sw_state;
+
+		memset(&sw_state, 0, sizeof(sw_state));
+		sw_state.base = pipe_config->base;
+		sw_state.scaler_state = pipe_config->scaler_state;
+		sw_state.shared_dpll = pipe_config->shared_dpll;
+		sw_state.dpll_hw_state = pipe_config->dpll_hw_state;
+		sw_state.ddi_pll_sel = pipe_config->ddi_pll_sel;
+
+		intel_modeset_pipe_config(&crtc->base, &sw_state);
+
+		/* Check if we need to force a modeset */
+		if (!intel_pipe_config_compare(dev, &sw_state, hw_state, true))
+			pipe_config->base.mode_changed = true;
+	}
+
+
+	if (dev_priv->quirks & QUIRK_PIPEA_FORCE && !hw_state->base.active &&
+	    crtc->pipe == PIPE_A && !pipe_config->base.active) {
 		/* BIOS forgot to enable pipe A, this mostly happens after
 		 * resume. Force-enable the pipe to fix this, the update_dpms
 		 * call below we restore the pipe to the right state, but leave
@@ -14513,19 +14570,24 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc,
 		intel_enable_pipe_a(dev);
 	}
 
-	/* Adjust the state of the output pipe according to whether we
-	 * have active connectors/encoders */
-	enable = false;
-	for_each_encoder_on_crtc(dev, &crtc->base, intel_encoder)
-		enable |= intel_encoder->connectors_active;
+	/* not restoring state, kill off all connectors and disable this thing */
+	if (!force_restore && !pipe_config->base.active) {
+		struct drm_connector_state *conn_state;
+		struct drm_connector *connector;
+		int i, ret;
 
-	/* only turn off separately if configuration's not restored,
-	 * if it's restored it will change mode or turn off anyway.
-	 */
-	if (!enable && crtc->base.state->active && !pipe_config)
-		intel_crtc_control(&crtc->base, false, true);
+		ret = drm_atomic_set_mode_for_crtc(&pipe_config->base, NULL);
+
+		for_each_connector_in_state(state, connector, conn_state, i) {
+			if (conn_state->crtc != &crtc->base)
+				continue;
 
-	if (crtc->active || HAS_GMCH_DISPLAY(dev)) {
+			ret = drm_atomic_set_crtc_for_connector(conn_state, NULL);
+			WARN_ON(ret);
+		}
+	}
+
+	if (hw_state->base.active || HAS_GMCH_DISPLAY(dev)) {
 		/*
 		 * We start out with underrun reporting disabled to avoid races.
 		 * For correct bookkeeping mark this on active crtcs.
@@ -14581,6 +14643,7 @@ static void intel_sanitize_encoder(struct intel_encoder *encoder)
 		for_each_intel_connector(dev, connector) {
 			if (connector->encoder != encoder)
 				continue;
+
 			connector->base.dpms = DRM_MODE_DPMS_OFF;
 			connector->base.encoder = NULL;
 		}
@@ -14887,7 +14950,7 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
 	struct intel_encoder *encoder;
 	struct drm_atomic_state *state;
 	struct intel_shared_dpll_config shared_dplls[I915_NUM_PLLS];
-	int i;
+	int i, ret;
 
 	state = intel_modeset_readout_hw_state(dev);
 	if (IS_ERR(state)) {
@@ -14897,6 +14960,7 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
 
 	/* swap sw/hw state */
 	drm_atomic_helper_swap_state(dev, state);
+
 	intel_atomic_duplicate_dpll_state(dev_priv, shared_dplls);
 	intel_shared_dpll_commit(state);
 	memcpy(to_intel_atomic_state(state)->shared_dpll,
@@ -14919,31 +14983,19 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
 		crtc_state->planes_changed = false;
 
 		if (crtc->state->enable) {
-			intel_mode_from_pipe_config(&crtc->state->mode,
+			intel_mode_from_pipe_config(&crtc->mode,
 				to_intel_crtc_state(crtc->state));
 			intel_mode_from_pipe_config(&crtc->state->adjusted_mode,
 				to_intel_crtc_state(crtc->state));
 
-			drm_mode_copy(&crtc->mode, &crtc->state->mode);
+			if (drm_atomic_set_mode_for_crtc(crtc->state, &crtc->mode))
+				drm_mode_copy(&crtc->state->mode, &crtc->mode);
+
 			drm_mode_copy(&crtc->hwmode,
 				      &crtc->state->adjusted_mode);
-
-			/* Check if we need to force a modeset */
-			if (to_intel_crtc_state(crtc_state)->has_audio !=
-			    to_intel_crtc_state(crtc->state)->has_audio)
-				crtc_state->mode_changed = true;
-
-			if (to_intel_crtc_state(crtc_state)->has_infoframe !=
-			    to_intel_crtc_state(crtc->state)->has_infoframe)
-				crtc_state->mode_changed = true;
 		}
 
-		intel_sanitize_crtc(intel_crtc, !force_restore ? NULL :
-				    to_intel_crtc_state(crtc_state));
-
-		/* turn CRTC off if a modeset is requested. */
-		if (crtc_state->mode_changed && !force_restore)
-			intel_crtc_control(crtc, false, true);
+		intel_sanitize_crtc(intel_crtc, to_intel_crtc_state(crtc_state), force_restore);
 
 		/*
 		 * sanitize_crtc may have forced an update of crtc->state,
@@ -14973,17 +15025,10 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
 	else if (HAS_PCH_SPLIT(dev))
 		ilk_wm_get_hw_state(dev);
 
-	if (force_restore) {
-		int ret;
-
-		i915_redisable_vga(dev);
-
-		ret = drm_atomic_commit(state);
-		if (ret) {
-			DRM_ERROR("Failed to restore previous mode\n");
-			drm_atomic_state_free(state);
-		}
-	} else {
+	i915_redisable_vga(dev);
+	ret = drm_atomic_commit(state);
+	if (ret) {
+		DRM_ERROR("Failed to restore previous mode\n");
 		drm_atomic_state_free(state);
 	}
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 2bf6873a5b89..2d19b5d67d9d 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -330,6 +330,7 @@ struct intel_crtc_state {
 #define PIPE_CONFIG_QUIRK_MODE_SYNC_FLAGS	(1<<0) /* unreliable sync mode.flags */
 #define PIPE_CONFIG_QUIRK_INHERITED_MODE	(1<<1) /* mode inherited from firmware */
 #define PIPE_CONFIG_QUIRK_INITIAL_PLANES	(1<<2) /* planes are in unknown state */
+#define PIPE_CONFIG_QUIRK_WRONG_PLANE		(1<<3) /* intel_crtc->plane is wrong */
 	unsigned long quirks;
 
 	/* Pipe source size (ie. panel fitter input size)

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx