[PATCH v2 00/12] DPMS updates and atomic state checking.

[PATCH v2 00/12] DPMS updates and atomic state checking.

[Maarten Lankhorst]

This patch series depends on:
"drm/atomic: Update legacy DPMS state during modesets, v3."
for the following 2 patches:
"drm/i915: Remove connectors_active."
"drm/i915: Only update mode related state if a modeset happened."

Mixed bag of fixes for -next now that the first merge happened.
Patch series deals with getting rid of intel DPMS handling and
making the state checker atomic.

The state checker is now atomic and only checks the affected
crtc's, encoders and connectors.

Maarten Lankhorst (12):
  drm/i915: Make the force_thru workaround atomic.
  drm/i915: Update atomic state when removing mst connector.
  drm/i915: Convert connector checking to atomic.
  drm/i915: Remove some unneeded checks from check_crtc_state.
  drm/i915: Remove connectors_active from state checking.
  drm/i915: Make crtc checking use the atomic state.
  drm/i915: Get rid of dpms handling.
  drm/i915: Remove connectors_active from sanitization.
  drm/i915: Remove connectors_active from intel_dp.c.
  drm/i915: Remove connectors_active.
  drm/i915: Only update mode related state if a modeset happened.
  drm/i915: Handle return value in intel_pin_and_fence_fb_obj, v2.

 drivers/gpu/drm/i915/i915_debugfs.c  |  82 ++-----
 drivers/gpu/drm/i915/intel_crt.c     |  51 +----
 drivers/gpu/drm/i915/intel_display.c | 425 +++++++++++------------------------
 drivers/gpu/drm/i915/intel_dp.c      |   9 +-
 drivers/gpu/drm/i915/intel_dp_mst.c  |  47 +++-
 drivers/gpu/drm/i915/intel_drv.h     |   5 -
 drivers/gpu/drm/i915/intel_dsi.c     |   2 +-
 drivers/gpu/drm/i915/intel_dvo.c     |  48 +---
 drivers/gpu/drm/i915/intel_hdmi.c    |   2 +-
 drivers/gpu/drm/i915/intel_lvds.c    |   2 +-
 drivers/gpu/drm/i915/intel_sdvo.c    |  49 +---
 drivers/gpu/drm/i915/intel_tv.c      |   2 +-
 12 files changed, 212 insertions(+), 512 deletions(-)

-- 
2.1.0

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

[PATCH v2 01/12] drm/i915: Make the force_thru workaround atomic.

[Maarten Lankhorst]

Set active_changed to force a modeset if the panel fitter's force
enabled.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c  | 82 +++++++++++-------------------------
 drivers/gpu/drm/i915/intel_display.c |  3 ++
 2 files changed, 27 insertions(+), 58 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 23a69307e12e..22467adee958 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3645,74 +3645,40 @@ static int ilk_pipe_crc_ctl_reg(enum intel_pipe_crc_source *source,
 	return 0;
 }
 
-static void hsw_trans_edp_pipe_A_crc_wa(struct drm_device *dev)
+static void hsw_trans_edp_pipe_A_crc_wa(struct drm_device *dev, bool enable)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *crtc =
 		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_A]);
 	struct intel_crtc_state *pipe_config;
+	struct drm_atomic_state *state;
+	int ret = 0;
 
 	drm_modeset_lock_all(dev);
-	pipe_config = to_intel_crtc_state(crtc->base.state);
-
-	/*
-	 * If we use the eDP transcoder we need to make sure that we don't
-	 * bypass the pfit, since otherwise the pipe CRC source won't work. Only
-	 * relevant on hsw with pipe A when using the always-on power well
-	 * routing.
-	 */
-	if (pipe_config->cpu_transcoder == TRANSCODER_EDP &&
-	    !pipe_config->pch_pfit.enabled) {
-		bool active = pipe_config->base.active;
-
-		if (active) {
-			intel_crtc_control(&crtc->base, false);
-			pipe_config = to_intel_crtc_state(crtc->base.state);
-		}
-
-		pipe_config->pch_pfit.force_thru = true;
-
-		intel_display_power_get(dev_priv,
-					POWER_DOMAIN_PIPE_PANEL_FITTER(PIPE_A));
-
-		if (active)
-			intel_crtc_control(&crtc->base, true);
+	state = drm_atomic_state_alloc(dev);
+	if (!state) {
+		ret = -ENOMEM;
+		goto out;
 	}
-	drm_modeset_unlock_all(dev);
-}
 
-static void hsw_undo_trans_edp_pipe_A_crc_wa(struct drm_device *dev)
-{
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_crtc *crtc =
-		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_A]);
-	struct intel_crtc_state *pipe_config;
-
-	drm_modeset_lock_all(dev);
-	/*
-	 * If we use the eDP transcoder we need to make sure that we don't
-	 * bypass the pfit, since otherwise the pipe CRC source won't work. Only
-	 * relevant on hsw with pipe A when using the always-on power well
-	 * routing.
-	 */
-	pipe_config = to_intel_crtc_state(crtc->base.state);
-	if (pipe_config->pch_pfit.force_thru) {
-		bool active = pipe_config->base.active;
-
-		if (active) {
-			intel_crtc_control(&crtc->base, false);
-			pipe_config = to_intel_crtc_state(crtc->base.state);
-		}
-
-		pipe_config->pch_pfit.force_thru = false;
+	state->acquire_ctx = drm_modeset_legacy_acquire_ctx(&crtc->base);
+	pipe_config = intel_atomic_get_crtc_state(state, crtc);
+	if (IS_ERR(pipe_config)) {
+		ret = PTR_ERR(pipe_config);
+		goto out;
+	}
 
-		intel_display_power_put(dev_priv,
-					POWER_DOMAIN_PIPE_PANEL_FITTER(PIPE_A));
+	pipe_config->pch_pfit.force_thru = enable;
+	if (pipe_config->cpu_transcoder == TRANSCODER_EDP &&
+	    pipe_config->pch_pfit.enabled != enable)
+		pipe_config->base.active_changed = true;
 
-		if (active)
-			intel_crtc_control(&crtc->base, true);
-	}
+	ret = drm_atomic_commit(state);
+out:
 	drm_modeset_unlock_all(dev);
+	WARN(ret, "Toggling workaround to %i returns %i\n", enable, ret);
+	if (ret)
+		drm_atomic_state_free(state);
 }
 
 static int ivb_pipe_crc_ctl_reg(struct drm_device *dev,
@@ -3732,7 +3698,7 @@ static int ivb_pipe_crc_ctl_reg(struct drm_device *dev,
 		break;
 	case INTEL_PIPE_CRC_SOURCE_PF:
 		if (IS_HASWELL(dev) && pipe == PIPE_A)
-			hsw_trans_edp_pipe_A_crc_wa(dev);
+			hsw_trans_edp_pipe_A_crc_wa(dev, true);
 
 		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PF_IVB;
 		break;
@@ -3844,7 +3810,7 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
 		else if (IS_VALLEYVIEW(dev))
 			vlv_undo_pipe_scramble_reset(dev, pipe);
 		else if (IS_HASWELL(dev) && pipe == PIPE_A)
-			hsw_undo_trans_edp_pipe_A_crc_wa(dev);
+			hsw_trans_edp_pipe_A_crc_wa(dev, false);
 
 		hsw_enable_ips(crtc);
 	}
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 43b0f17ad1fa..7747520bf9f6 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12160,6 +12160,7 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
 	struct intel_dpll_hw_state dpll_hw_state;
 	enum intel_dpll_id shared_dpll;
 	uint32_t ddi_pll_sel;
+	bool force_thru;
 
 	/* FIXME: before the switch to atomic started, a new pipe_config was
 	 * kzalloc'd. Code that depends on any field being zero should be
@@ -12171,6 +12172,7 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
 	shared_dpll = crtc_state->shared_dpll;
 	dpll_hw_state = crtc_state->dpll_hw_state;
 	ddi_pll_sel = crtc_state->ddi_pll_sel;
+	force_thru = crtc_state->pch_pfit.force_thru;
 
 	memset(crtc_state, 0, sizeof *crtc_state);
 
@@ -12179,6 +12181,7 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
 	crtc_state->shared_dpll = shared_dpll;
 	crtc_state->dpll_hw_state = dpll_hw_state;
 	crtc_state->ddi_pll_sel = ddi_pll_sel;
+	crtc_state->pch_pfit.force_thru = force_thru;
 }
 
 static int
-- 
2.1.0

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

[PATCH v2 02/12] drm/i915: Update atomic state when removing mst connector.

[Maarten Lankhorst]

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |  7 ------
 drivers/gpu/drm/i915/intel_dp_mst.c  | 45 +++++++++++++++++++++++++++++++++++-
 2 files changed, 44 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7747520bf9f6..3ab0a8a8e702 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12751,13 +12751,6 @@ check_encoder_state(struct drm_device *dev)
 					encoder->base.crtc,
 			     "connector's crtc doesn't match encoder crtc\n");
 		}
-		/*
-		 * for MST connectors if we unplug the connector is gone
-		 * away but the encoder is still connected to a crtc
-		 * until a modeset happens in response to the hotplug.
-		 */
-		if (!enabled && encoder->base.encoder_type == DRM_MODE_ENCODER_DPMST)
-			continue;
 
 		I915_STATE_WARN(!!encoder->base.crtc != enabled,
 		     "encoder's enabled state mismatch "
diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index 585f0a45b3f1..35f2eb59818a 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -448,6 +448,49 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
 	return connector;
 }
 
+static void
+intel_dp_unbind_mst_connector(struct drm_device *dev,
+			      struct drm_connector *connector)
+{
+	struct drm_atomic_state *state;
+	struct drm_connector_state *conn_state;
+	struct drm_crtc *crtc = connector->state->crtc;
+	int ret;
+
+	if (!crtc)
+		return;
+
+	state = drm_atomic_state_alloc(dev);
+	if (!state)
+		return;
+
+	state->acquire_ctx = dev->mode_config.acquire_ctx;
+
+	conn_state = drm_atomic_get_connector_state(state, connector);
+	ret = PTR_ERR_OR_ZERO(conn_state);
+	if (!ret)
+		ret = drm_atomic_set_crtc_for_connector(conn_state, NULL);
+
+	if (!ret)
+		ret = drm_atomic_add_affected_connectors(state, crtc);
+
+	if (!ret && !drm_atomic_connectors_for_crtc(state, crtc)) {
+		struct drm_crtc_state *crtc_state =
+			drm_atomic_get_existing_crtc_state(state, crtc);
+
+		crtc_state->active = false;
+		ret = drm_atomic_set_mode_for_crtc(crtc_state, NULL);
+	}
+
+	if (!ret)
+		ret = drm_atomic_commit(state);
+
+	if (ret)
+		drm_atomic_state_free(state);
+
+	I915_STATE_WARN_ON(ret);
+}
+
 static void intel_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
 					   struct drm_connector *connector)
 {
@@ -455,7 +498,7 @@ static void intel_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
 	struct drm_device *dev = connector->dev;
 	/* need to nuke the connector */
 	drm_modeset_lock_all(dev);
-	intel_connector_dpms(connector, DRM_MODE_DPMS_OFF);
+	intel_dp_unbind_mst_connector(dev, connector);
 	drm_modeset_unlock_all(dev);
 
 	intel_connector->unregister(intel_connector);
-- 
2.1.0

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

[PATCH v2 03/12] drm/i915: Convert connector checking to atomic.

[Maarten Lankhorst]

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_crt.c     |  2 -
 drivers/gpu/drm/i915/intel_display.c | 79 ++++++++++++++++--------------------
 drivers/gpu/drm/i915/intel_drv.h     |  1 -
 drivers/gpu/drm/i915/intel_dvo.c     |  2 -
 drivers/gpu/drm/i915/intel_sdvo.c    |  2 -
 5 files changed, 36 insertions(+), 50 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index 5d78c1feec81..9eba3dd5b434 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -280,8 +280,6 @@ static int intel_crt_dpms(struct drm_connector *connector, int mode)
 		intel_crtc_update_dpms(crtc);
 	}
 
-	intel_modeset_check_state(connector->dev);
-
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3ab0a8a8e702..ba0b68a4209d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6360,42 +6360,33 @@ static void intel_encoder_dpms(struct intel_encoder *encoder, int mode)
  * internal consistency). */
 static void intel_connector_check_state(struct intel_connector *connector)
 {
+	struct drm_crtc *crtc = connector->base.state->crtc;
+
+	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
+		      connector->base.base.id,
+		      connector->base.name);
+
 	if (connector->get_hw_state(connector)) {
-		struct intel_encoder *encoder = connector->encoder;
-		struct drm_crtc *crtc;
-		bool encoder_enabled;
-		enum pipe pipe;
+		struct drm_encoder *encoder = &connector->encoder->base;
+		struct drm_connector_state *conn_state = connector->base.state;
 
-		DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
-			      connector->base.base.id,
-			      connector->base.name);
+		I915_STATE_WARN(!crtc,
+			 "connector enabled without attached crtc\n");
 
-		/* there is no real hw state for MST connectors */
-		if (connector->mst_port)
+		if (!crtc)
 			return;
 
-		I915_STATE_WARN(connector->base.dpms == DRM_MODE_DPMS_OFF,
-		     "wrong connector dpms state\n");
-		I915_STATE_WARN(connector->base.encoder != &encoder->base,
-		     "active connector not linked to encoder\n");
+		I915_STATE_WARN(!crtc->state->active,
+		      "connector is active, but attached crtc isn't\n");
 
-		if (encoder) {
-			I915_STATE_WARN(!encoder->connectors_active,
-			     "encoder->connectors_active not set\n");
-
-			encoder_enabled = encoder->get_hw_state(encoder, &pipe);
-			I915_STATE_WARN(!encoder_enabled, "encoder not enabled\n");
-			if (I915_STATE_WARN_ON(!encoder->base.crtc))
-				return;
+		I915_STATE_WARN(conn_state->best_encoder != encoder,
+			"atomic encoder doesn't match attached encoder\n");
 
-			crtc = encoder->base.crtc;
-
-			I915_STATE_WARN(!crtc->state->enable,
-					"crtc not enabled\n");
-			I915_STATE_WARN(!to_intel_crtc(crtc)->active, "crtc not active\n");
-			I915_STATE_WARN(pipe != to_intel_crtc(crtc)->pipe,
-			     "encoder active on the wrong pipe\n");
-		}
+		I915_STATE_WARN(conn_state->crtc != encoder->crtc,
+			"attached encoder crtc differs from connector crtc\n");
+	} else {
+		I915_STATE_WARN(!crtc && connector->base.state->best_encoder,
+			"best encoder set without crtc!\n");
 	}
 }
 
@@ -6444,8 +6435,6 @@ int intel_connector_dpms(struct drm_connector *connector, int mode)
 	if (connector->encoder)
 		intel_encoder_dpms(to_intel_encoder(connector->encoder), mode);
 
-	intel_modeset_check_state(connector->dev);
-
 	return 0;
 }
 
@@ -12705,20 +12694,22 @@ static void check_wm_state(struct drm_device *dev)
 }
 
 static void
-check_connector_state(struct drm_device *dev)
+check_connector_state(struct drm_device *dev, struct drm_atomic_state *state)
 {
-	struct intel_connector *connector;
+	struct drm_connector_state *conn_state;
+	struct drm_connector *connector;
+	int i;
 
-	for_each_intel_connector(dev, connector) {
-		struct drm_encoder *encoder = connector->base.encoder;
-		struct drm_connector_state *state = connector->base.state;
+	for_each_connector_in_state(state, connector, conn_state, i) {
+		struct drm_encoder *encoder = connector->encoder;
+		struct drm_connector_state *state = connector->state;
 
 		/* This also checks the encoder/connector hw state with the
 		 * ->get_hw_state callbacks. */
-		intel_connector_check_state(connector);
+		intel_connector_check_state(to_intel_connector(connector));
 
 		I915_STATE_WARN(state->best_encoder != encoder,
-		     "connector's staged encoder doesn't match current encoder\n");
+		     "connector's atomic encoder doesn't match legacy encoder\n");
 	}
 }
 
@@ -12904,11 +12895,12 @@ check_shared_dpll_state(struct drm_device *dev)
 	}
 }
 
-void
-intel_modeset_check_state(struct drm_device *dev)
+static void
+intel_modeset_check_state(struct drm_device *dev,
+			  struct drm_atomic_state *state)
 {
 	check_wm_state(dev);
-	check_connector_state(dev);
+	check_connector_state(dev, state);
 	check_encoder_state(dev);
 	check_crtc_state(dev);
 	check_shared_dpll_state(dev);
@@ -13294,10 +13286,11 @@ static int intel_atomic_commit(struct drm_device *dev,
 
 	drm_atomic_helper_wait_for_vblanks(dev, state);
 	drm_atomic_helper_cleanup_planes(dev, state);
-	drm_atomic_state_free(state);
 
 	if (any_ms)
-		intel_modeset_check_state(dev);
+		intel_modeset_check_state(dev, state);
+
+	drm_atomic_state_free(state);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 320c9e6bd848..0da4236dc85a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -999,7 +999,6 @@ int intel_connector_init(struct intel_connector *);
 struct intel_connector *intel_connector_alloc(void);
 int intel_connector_dpms(struct drm_connector *, int mode);
 bool intel_connector_get_hw_state(struct intel_connector *connector);
-void intel_modeset_check_state(struct drm_device *dev);
 bool ibx_digital_port_connected(struct drm_i915_private *dev_priv,
 				struct intel_digital_port *port);
 void intel_connector_attach_encoder(struct intel_connector *connector,
diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c
index fd5e522abebb..600f7fb855d8 100644
--- a/drivers/gpu/drm/i915/intel_dvo.c
+++ b/drivers/gpu/drm/i915/intel_dvo.c
@@ -237,8 +237,6 @@ static int intel_dvo_dpms(struct drm_connector *connector, int mode)
 		intel_crtc_update_dpms(crtc);
 	}
 
-	intel_modeset_check_state(connector->dev);
-
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index 2c435a79d4da..8911e0e417ee 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -1550,8 +1550,6 @@ static int intel_sdvo_dpms(struct drm_connector *connector, int mode)
 		intel_sdvo_set_active_outputs(intel_sdvo, intel_sdvo->attached_output);
 	}
 
-	intel_modeset_check_state(connector->dev);
-
 	return 0;
 }
 
-- 
2.1.0

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

[PATCH v2 04/12] drm/i915: Remove some unneeded checks from check_crtc_state.

[Maarten Lankhorst]

This is handled by the atomic core now, no need to check this for ourself.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 19 +------------------
 1 file changed, 1 insertion(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ba0b68a4209d..59eb6db10740 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12781,8 +12781,7 @@ check_crtc_state(struct drm_device *dev)
 	struct intel_crtc_state pipe_config;
 
 	for_each_intel_crtc(dev, crtc) {
-		bool enabled = false;
-		bool active = false;
+		bool active;
 
 		memset(&pipe_config, 0, sizeof(pipe_config));
 
@@ -12792,22 +12791,6 @@ check_crtc_state(struct drm_device *dev)
 		I915_STATE_WARN(crtc->active && !crtc->base.state->enable,
 		     "active crtc, but not enabled in sw tracking\n");
 
-		for_each_intel_encoder(dev, encoder) {
-			if (encoder->base.crtc != &crtc->base)
-				continue;
-			enabled = true;
-			if (encoder->connectors_active)
-				active = true;
-		}
-
-		I915_STATE_WARN(active != crtc->active,
-		     "crtc's computed active state doesn't match tracked active state "
-		     "(expected %i, found %i)\n", active, crtc->active);
-		I915_STATE_WARN(enabled != crtc->base.state->enable,
-		     "crtc's computed enabled state doesn't match tracked enabled state "
-		     "(expected %i, found %i)\n", enabled,
-				crtc->base.state->enable);
-
 		active = dev_priv->display.get_pipe_config(crtc,
 							   &pipe_config);
 
-- 
2.1.0

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

[PATCH v2 05/12] drm/i915: Remove connectors_active from state checking.

[Maarten Lankhorst]

Connectors are updated atomically now, so the only interaction
with the encoder is through base.crtc.

If it's NULL the encoder's not part of any crtc, and if it's
not NULL then active should be equal to crtc_state->active.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 59eb6db10740..fbb257d4728c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12728,9 +12728,6 @@ check_encoder_state(struct drm_device *dev)
 			      encoder->base.base.id,
 			      encoder->base.name);
 
-		I915_STATE_WARN(encoder->connectors_active && !encoder->base.crtc,
-		     "encoder's active_connectors set, but no crtc\n");
-
 		for_each_intel_connector(dev, connector) {
 			if (connector->base.encoder != &encoder->base)
 				continue;
@@ -12750,18 +12747,20 @@ check_encoder_state(struct drm_device *dev)
 		I915_STATE_WARN(active && !encoder->base.crtc,
 		     "active encoder with no crtc\n");
 
-		I915_STATE_WARN(encoder->connectors_active != active,
-		     "encoder's computed active state doesn't match tracked active state "
-		     "(expected %i, found %i)\n", active, encoder->connectors_active);
-
 		active = encoder->get_hw_state(encoder, &pipe);
-		I915_STATE_WARN(active != encoder->connectors_active,
+
+		if (!encoder->base.crtc) {
+			I915_STATE_WARN(active,
+			     "encoder detached but not turned off.\n");
+
+			continue;
+		}
+
+		I915_STATE_WARN(active != encoder->base.crtc->state->active,
 		     "encoder's hw state doesn't match sw tracking "
 		     "(expected %i, found %i)\n",
-		     encoder->connectors_active, active);
+		     encoder->base.crtc->state->active, active);
 
-		if (!encoder->base.crtc)
-			continue;
 
 		tracked_pipe = to_intel_crtc(encoder->base.crtc)->pipe;
 		I915_STATE_WARN(active && pipe != tracked_pipe,
-- 
2.1.0

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

[PATCH v2 06/12] drm/i915: Make crtc checking use the atomic state.

[Maarten Lankhorst]

Instead of allocating pipe_config on the stack use the old crtc_state,
it's only going to freed from this point on.

All crtc's encoders are now only checked once during modeset.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 118 +++++++++++++++++------------------
 1 file changed, 56 insertions(+), 62 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index fbb257d4728c..e3afe611a78c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11829,7 +11829,7 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
 	struct intel_crtc_state *pipe_config =
 		to_intel_crtc_state(crtc_state);
 	struct drm_atomic_state *state = crtc_state->state;
-	int ret, idx = crtc->base.id;
+	int ret;
 	bool mode_changed = needs_modeset(crtc_state);
 
 	if (mode_changed && !check_encoder_cloning(state, intel_crtc)) {
@@ -11837,10 +11837,6 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
 		return -EINVAL;
 	}
 
-	I915_STATE_WARN(crtc->state->active != intel_crtc->active,
-		"[CRTC:%i] mismatch between state->active(%i) and crtc->active(%i)\n",
-		idx, crtc->state->active, intel_crtc->active);
-
 	if (mode_changed && !crtc_state->active)
 		intel_crtc->atomic.update_wm_post = true;
 
@@ -12721,19 +12717,16 @@ check_encoder_state(struct drm_device *dev)
 
 	for_each_intel_encoder(dev, encoder) {
 		bool enabled = false;
-		bool active = false;
-		enum pipe pipe, tracked_pipe;
+		enum pipe pipe;
 
 		DRM_DEBUG_KMS("[ENCODER:%d:%s]\n",
 			      encoder->base.base.id,
 			      encoder->base.name);
 
 		for_each_intel_connector(dev, connector) {
-			if (connector->base.encoder != &encoder->base)
+			if (connector->base.state->best_encoder != &encoder->base)
 				continue;
 			enabled = true;
-			if (connector->base.dpms != DRM_MODE_DPMS_OFF)
-				active = true;
 
 			I915_STATE_WARN(connector->base.state->crtc !=
 					encoder->base.crtc,
@@ -12744,85 +12737,86 @@ check_encoder_state(struct drm_device *dev)
 		     "encoder's enabled state mismatch "
 		     "(expected %i, found %i)\n",
 		     !!encoder->base.crtc, enabled);
-		I915_STATE_WARN(active && !encoder->base.crtc,
-		     "active encoder with no crtc\n");
-
-		active = encoder->get_hw_state(encoder, &pipe);
 
 		if (!encoder->base.crtc) {
-			I915_STATE_WARN(active,
-			     "encoder detached but not turned off.\n");
+			bool active;
 
-			continue;
+			active = encoder->get_hw_state(encoder, &pipe);
+			I915_STATE_WARN(active,
+			     "encoder detached but still enabled on pipe %c.\n",
+			     pipe_name(pipe));
 		}
-
-		I915_STATE_WARN(active != encoder->base.crtc->state->active,
-		     "encoder's hw state doesn't match sw tracking "
-		     "(expected %i, found %i)\n",
-		     encoder->base.crtc->state->active, active);
-
-
-		tracked_pipe = to_intel_crtc(encoder->base.crtc)->pipe;
-		I915_STATE_WARN(active && pipe != tracked_pipe,
-		     "active encoder's pipe doesn't match"
-		     "(expected %i, found %i)\n",
-		     tracked_pipe, pipe);
-
 	}
 }
 
 static void
-check_crtc_state(struct drm_device *dev)
+check_crtc_state(struct drm_device *dev, struct drm_atomic_state *state)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_crtc *crtc;
 	struct intel_encoder *encoder;
-	struct intel_crtc_state pipe_config;
+	struct drm_crtc_state *crtc_state;
+	struct drm_crtc *crtc;
+	int i;
 
-	for_each_intel_crtc(dev, crtc) {
+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+		struct intel_crtc_state *pipe_config, *sw_config;
 		bool active;
 
-		memset(&pipe_config, 0, sizeof(pipe_config));
+		if (!needs_modeset(crtc->state))
+			continue;
 
-		DRM_DEBUG_KMS("[CRTC:%d]\n",
-			      crtc->base.base.id);
+		__drm_atomic_helper_crtc_destroy_state(crtc, crtc_state);
+		pipe_config = to_intel_crtc_state(crtc_state);
+		memset(pipe_config, 0, sizeof(*pipe_config));
+		crtc_state->crtc = crtc;
+		crtc_state->state = state;
 
-		I915_STATE_WARN(crtc->active && !crtc->base.state->enable,
-		     "active crtc, but not enabled in sw tracking\n");
+		DRM_DEBUG_KMS("[CRTC:%d]\n",
+			      crtc->base.id);
 
-		active = dev_priv->display.get_pipe_config(crtc,
-							   &pipe_config);
+		active = dev_priv->display.get_pipe_config(intel_crtc,
+							   pipe_config);
 
 		/* hw state is inconsistent with the pipe quirk */
-		if ((crtc->pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE) ||
-		    (crtc->pipe == PIPE_B && dev_priv->quirks & QUIRK_PIPEB_FORCE))
-			active = crtc->active;
+		if ((intel_crtc->pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE) ||
+		    (intel_crtc->pipe == PIPE_B && dev_priv->quirks & QUIRK_PIPEB_FORCE))
+			active = crtc->state->active;
 
-		for_each_intel_encoder(dev, encoder) {
-			enum pipe pipe;
-			if (encoder->base.crtc != &crtc->base)
-				continue;
-			if (encoder->get_hw_state(encoder, &pipe))
-				encoder->get_config(encoder, &pipe_config);
-		}
-
-		I915_STATE_WARN(crtc->active != active,
+		I915_STATE_WARN(crtc->state->active != active,
 		     "crtc active state doesn't match with hw state "
-		     "(expected %i, found %i)\n", crtc->active, active);
+		     "(expected %i, found %i)\n", crtc->state->active, active);
 
-		I915_STATE_WARN(crtc->active != crtc->base.state->active,
+		I915_STATE_WARN(intel_crtc->active != crtc->state->active,
 		     "transitional active state does not match atomic hw state "
-		     "(expected %i, found %i)\n", crtc->base.state->active, crtc->active);
+		     "(expected %i, found %i)\n", crtc->state->active, intel_crtc->active);
+
+		for_each_encoder_on_crtc(dev, crtc, encoder) {
+			enum pipe pipe;
+
+			active = encoder->get_hw_state(encoder, &pipe);
+			I915_STATE_WARN(active != crtc->state->active,
+				"[ENCODER:%i] active %i with crtc active %i\n",
+				encoder->base.base.id, active, crtc->state->active);
+
+			I915_STATE_WARN(active && intel_crtc->pipe != pipe,
+					"Encoder connected to wrong pipe %c\n",
+					pipe_name(pipe));
+
+			if (active)
+				encoder->get_config(encoder, pipe_config);
+		}
 
-		if (!active)
+		if (!crtc->state->active)
 			continue;
 
-		if (!intel_pipe_config_compare(dev, crtc->config,
-					       &pipe_config, false)) {
+		sw_config = to_intel_crtc_state(crtc->state);
+		if (!intel_pipe_config_compare(dev, sw_config,
+					       pipe_config, false)) {
 			I915_STATE_WARN(1, "pipe state doesn't match!\n");
-			intel_dump_pipe_config(crtc, &pipe_config,
+			intel_dump_pipe_config(intel_crtc, pipe_config,
 					       "[hw state]");
-			intel_dump_pipe_config(crtc, crtc->config,
+			intel_dump_pipe_config(intel_crtc, sw_config,
 					       "[sw state]");
 		}
 	}
@@ -12884,7 +12878,7 @@ intel_modeset_check_state(struct drm_device *dev,
 	check_wm_state(dev);
 	check_connector_state(dev, state);
 	check_encoder_state(dev);
-	check_crtc_state(dev);
+	check_crtc_state(dev, state);
 	check_shared_dpll_state(dev);
 }
 
-- 
2.1.0

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

[PATCH v2 07/12] drm/i915: Get rid of dpms handling.

[Maarten Lankhorst]

This is now done completely atomically.
Keep connectors_active for now, but make it mirror crtc_state->active.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_crt.c     | 49 +-----------------
 drivers/gpu/drm/i915/intel_display.c | 99 +-----------------------------------
 drivers/gpu/drm/i915/intel_dp.c      |  2 +-
 drivers/gpu/drm/i915/intel_dp_mst.c  |  2 +-
 drivers/gpu/drm/i915/intel_drv.h     |  3 --
 drivers/gpu/drm/i915/intel_dsi.c     |  2 +-
 drivers/gpu/drm/i915/intel_dvo.c     | 46 +----------------
 drivers/gpu/drm/i915/intel_hdmi.c    |  2 +-
 drivers/gpu/drm/i915/intel_lvds.c    |  2 +-
 drivers/gpu/drm/i915/intel_sdvo.c    | 47 +----------------
 drivers/gpu/drm/i915/intel_tv.c      |  2 +-
 11 files changed, 11 insertions(+), 245 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index 9eba3dd5b434..af5e43bef4a4 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -236,53 +236,6 @@ static void intel_enable_crt(struct intel_encoder *encoder)
 	intel_crt_set_dpms(encoder, crt->connector->base.dpms);
 }
 
-/* Special dpms function to support cloning between dvo/sdvo/crt. */
-static int intel_crt_dpms(struct drm_connector *connector, int mode)
-{
-	struct drm_device *dev = connector->dev;
-	struct intel_encoder *encoder = intel_attached_encoder(connector);
-	struct drm_crtc *crtc;
-	int old_dpms;
-
-	/* PCH platforms and VLV only support on/off. */
-	if (INTEL_INFO(dev)->gen >= 5 && mode != DRM_MODE_DPMS_ON)
-		mode = DRM_MODE_DPMS_OFF;
-
-	if (mode == connector->dpms)
-		return 0;
-
-	old_dpms = connector->dpms;
-	connector->dpms = mode;
-
-	/* Only need to change hw state when actually enabled */
-	crtc = encoder->base.crtc;
-	if (!crtc) {
-		encoder->connectors_active = false;
-		return 0;
-	}
-
-	/* We need the pipe to run for anything but OFF. */
-	if (mode == DRM_MODE_DPMS_OFF)
-		encoder->connectors_active = false;
-	else
-		encoder->connectors_active = true;
-
-	/* We call connector dpms manually below in case pipe dpms doesn't
-	 * change due to cloning. */
-	if (mode < old_dpms) {
-		/* From off to on, enable the pipe first. */
-		intel_crtc_update_dpms(crtc);
-
-		intel_crt_set_dpms(encoder, mode);
-	} else {
-		intel_crt_set_dpms(encoder, mode);
-
-		intel_crtc_update_dpms(crtc);
-	}
-
-	return 0;
-}
-
 static enum drm_mode_status
 intel_crt_mode_valid(struct drm_connector *connector,
 		     struct drm_display_mode *mode)
@@ -798,7 +751,7 @@ static void intel_crt_reset(struct drm_connector *connector)
 
 static const struct drm_connector_funcs intel_crt_connector_funcs = {
 	.reset = intel_crt_reset,
-	.dpms = intel_crt_dpms,
+	.dpms = drm_atomic_helper_connector_dpms,
 	.detect = intel_crt_detect,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.destroy = intel_crt_destroy,
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e3afe611a78c..ed9eba2666e2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6271,67 +6271,6 @@ free:
 	return ret;
 }
 
-/* Master function to enable/disable CRTC and corresponding power wells */
-int intel_crtc_control(struct drm_crtc *crtc, bool enable)
-{
-	struct drm_device *dev = crtc->dev;
-	struct drm_mode_config *config = &dev->mode_config;
-	struct drm_modeset_acquire_ctx *ctx = config->acquire_ctx;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	struct intel_crtc_state *pipe_config;
-	struct drm_atomic_state *state;
-	int ret;
-
-	if (enable == intel_crtc->active)
-		return 0;
-
-	if (enable && !crtc->state->enable)
-		return 0;
-
-	/* this function should be called with drm_modeset_lock_all for now */
-	if (WARN_ON(!ctx))
-		return -EIO;
-	lockdep_assert_held(&ctx->ww_ctx);
-
-	state = drm_atomic_state_alloc(dev);
-	if (WARN_ON(!state))
-		return -ENOMEM;
-
-	state->acquire_ctx = ctx;
-	state->allow_modeset = true;
-
-	pipe_config = intel_atomic_get_crtc_state(state, intel_crtc);
-	if (IS_ERR(pipe_config)) {
-		ret = PTR_ERR(pipe_config);
-		goto err;
-	}
-	pipe_config->base.active = enable;
-
-	ret = drm_atomic_commit(state);
-	if (!ret)
-		return ret;
-
-err:
-	DRM_ERROR("Updating crtc active failed with %i\n", ret);
-	drm_atomic_state_free(state);
-	return ret;
-}
-
-/**
- * Sets the power management mode of the pipe and plane.
- */
-void intel_crtc_update_dpms(struct drm_crtc *crtc)
-{
-	struct drm_device *dev = crtc->dev;
-	struct intel_encoder *intel_encoder;
-	bool enable = false;
-
-	for_each_encoder_on_crtc(dev, crtc, intel_encoder)
-		enable |= intel_encoder->connectors_active;
-
-	intel_crtc_control(crtc, enable);
-}
-
 void intel_encoder_destroy(struct drm_encoder *encoder)
 {
 	struct intel_encoder *intel_encoder = to_intel_encoder(encoder);
@@ -6340,22 +6279,6 @@ void intel_encoder_destroy(struct drm_encoder *encoder)
 	kfree(intel_encoder);
 }
 
-/* Simple dpms helper for encoders with just one connector, no cloning and only
- * one kind of off state. It clamps all !ON modes to fully OFF and changes the
- * state of the entire output pipe. */
-static void intel_encoder_dpms(struct intel_encoder *encoder, int mode)
-{
-	if (mode == DRM_MODE_DPMS_ON) {
-		encoder->connectors_active = true;
-
-		intel_crtc_update_dpms(encoder->base.crtc);
-	} else {
-		encoder->connectors_active = false;
-
-		intel_crtc_update_dpms(encoder->base.crtc);
-	}
-}
-
 /* Cross check the actual hw state with our own modeset state tracking (and it's
  * internal consistency). */
 static void intel_connector_check_state(struct intel_connector *connector)
@@ -6385,6 +6308,8 @@ static void intel_connector_check_state(struct intel_connector *connector)
 		I915_STATE_WARN(conn_state->crtc != encoder->crtc,
 			"attached encoder crtc differs from connector crtc\n");
 	} else {
+		I915_STATE_WARN(crtc && crtc->state->active,
+			"attached crtc is active, but connector isn't\n");
 		I915_STATE_WARN(!crtc && connector->base.state->best_encoder,
 			"best encoder set without crtc!\n");
 	}
@@ -6418,26 +6343,6 @@ struct intel_connector *intel_connector_alloc(void)
 	return connector;
 }
 
-/* Even simpler default implementation, if there's really no special case to
- * consider. */
-int intel_connector_dpms(struct drm_connector *connector, int mode)
-{
-	/* All the simple cases only support two dpms states. */
-	if (mode != DRM_MODE_DPMS_ON)
-		mode = DRM_MODE_DPMS_OFF;
-
-	if (mode == connector->dpms)
-		return 0;
-
-	connector->dpms = mode;
-
-	/* Only need to change hw state when actually enabled */
-	if (connector->encoder)
-		intel_encoder_dpms(to_intel_encoder(connector->encoder), mode);
-
-	return 0;
-}
-
 /* Simple connector->get_hw_state implementation for encoders that support only
  * one connector and no cloning and hence the encoder state determines the state
  * of the connector. */
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index f1b9f939b435..cea7d1785d13 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4824,7 +4824,7 @@ static void intel_dp_encoder_reset(struct drm_encoder *encoder)
 }
 
 static const struct drm_connector_funcs intel_dp_connector_funcs = {
-	.dpms = intel_connector_dpms,
+	.dpms = drm_atomic_helper_connector_dpms,
 	.detect = intel_dp_detect,
 	.force = intel_dp_force,
 	.fill_modes = drm_helper_probe_single_connector_modes,
diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index 35f2eb59818a..4bc371d19b91 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -328,7 +328,7 @@ intel_dp_mst_connector_destroy(struct drm_connector *connector)
 }
 
 static const struct drm_connector_funcs intel_dp_mst_connector_funcs = {
-	.dpms = intel_connector_dpms,
+	.dpms = drm_atomic_helper_connector_dpms,
 	.detect = intel_dp_mst_detect,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.set_property = intel_dp_mst_set_property,
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 0da4236dc85a..62073d2f83fa 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -992,12 +992,9 @@ void intel_mark_busy(struct drm_device *dev);
 void intel_mark_idle(struct drm_device *dev);
 void intel_crtc_restore_mode(struct drm_crtc *crtc);
 int intel_display_suspend(struct drm_device *dev);
-int intel_crtc_control(struct drm_crtc *crtc, bool enable);
-void intel_crtc_update_dpms(struct drm_crtc *crtc);
 void intel_encoder_destroy(struct drm_encoder *encoder);
 int intel_connector_init(struct intel_connector *);
 struct intel_connector *intel_connector_alloc(void);
-int intel_connector_dpms(struct drm_connector *, int mode);
 bool intel_connector_get_hw_state(struct intel_connector *connector);
 bool ibx_digital_port_connected(struct drm_i915_private *dev_priv,
 				struct intel_digital_port *port);
diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index 18dd7d7360a0..4a601cf90f16 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -982,7 +982,7 @@ static const struct drm_connector_helper_funcs intel_dsi_connector_helper_funcs
 };
 
 static const struct drm_connector_funcs intel_dsi_connector_funcs = {
-	.dpms = intel_connector_dpms,
+	.dpms = drm_atomic_helper_connector_dpms,
 	.detect = intel_dsi_detect,
 	.destroy = intel_dsi_connector_destroy,
 	.fill_modes = drm_helper_probe_single_connector_modes,
diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c
index 600f7fb855d8..dc532bb61d22 100644
--- a/drivers/gpu/drm/i915/intel_dvo.c
+++ b/drivers/gpu/drm/i915/intel_dvo.c
@@ -196,50 +196,6 @@ static void intel_enable_dvo(struct intel_encoder *encoder)
 	intel_dvo->dev.dev_ops->dpms(&intel_dvo->dev, true);
 }
 
-/* Special dpms function to support cloning between dvo/sdvo/crt. */
-static int intel_dvo_dpms(struct drm_connector *connector, int mode)
-{
-	struct intel_dvo *intel_dvo = intel_attached_dvo(connector);
-	struct drm_crtc *crtc;
-	struct intel_crtc_state *config;
-
-	/* dvo supports only 2 dpms states. */
-	if (mode != DRM_MODE_DPMS_ON)
-		mode = DRM_MODE_DPMS_OFF;
-
-	if (mode == connector->dpms)
-		return 0;
-
-	connector->dpms = mode;
-
-	/* Only need to change hw state when actually enabled */
-	crtc = intel_dvo->base.base.crtc;
-	if (!crtc) {
-		intel_dvo->base.connectors_active = false;
-		return 0;
-	}
-
-	/* We call connector dpms manually below in case pipe dpms doesn't
-	 * change due to cloning. */
-	if (mode == DRM_MODE_DPMS_ON) {
-		config = to_intel_crtc(crtc)->config;
-
-		intel_dvo->base.connectors_active = true;
-
-		intel_crtc_update_dpms(crtc);
-
-		intel_dvo->dev.dev_ops->dpms(&intel_dvo->dev, true);
-	} else {
-		intel_dvo->dev.dev_ops->dpms(&intel_dvo->dev, false);
-
-		intel_dvo->base.connectors_active = false;
-
-		intel_crtc_update_dpms(crtc);
-	}
-
-	return 0;
-}
-
 static enum drm_mode_status
 intel_dvo_mode_valid(struct drm_connector *connector,
 		     struct drm_display_mode *mode)
@@ -387,7 +343,7 @@ static void intel_dvo_destroy(struct drm_connector *connector)
 }
 
 static const struct drm_connector_funcs intel_dvo_connector_funcs = {
-	.dpms = intel_dvo_dpms,
+	.dpms = drm_atomic_helper_connector_dpms,
 	.detect = intel_dvo_detect,
 	.destroy = intel_dvo_destroy,
 	.fill_modes = drm_helper_probe_single_connector_modes,
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 70bad5bf1d48..51cbea8247fe 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1909,7 +1909,7 @@ static void intel_hdmi_destroy(struct drm_connector *connector)
 }
 
 static const struct drm_connector_funcs intel_hdmi_connector_funcs = {
-	.dpms = intel_connector_dpms,
+	.dpms = drm_atomic_helper_connector_dpms,
 	.detect = intel_hdmi_detect,
 	.force = intel_hdmi_force,
 	.fill_modes = drm_helper_probe_single_connector_modes,
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index cb634f48e7d9..881b5d13592e 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -549,7 +549,7 @@ static const struct drm_connector_helper_funcs intel_lvds_connector_helper_funcs
 };
 
 static const struct drm_connector_funcs intel_lvds_connector_funcs = {
-	.dpms = intel_connector_dpms,
+	.dpms = drm_atomic_helper_connector_dpms,
 	.detect = intel_lvds_detect,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.set_property = intel_lvds_set_property,
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index 8911e0e417ee..c98098e884cc 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -1508,51 +1508,6 @@ static void intel_enable_sdvo(struct intel_encoder *encoder)
 	intel_sdvo_set_active_outputs(intel_sdvo, intel_sdvo->attached_output);
 }
 
-/* Special dpms function to support cloning between dvo/sdvo/crt. */
-static int intel_sdvo_dpms(struct drm_connector *connector, int mode)
-{
-	struct drm_crtc *crtc;
-	struct intel_sdvo *intel_sdvo = intel_attached_sdvo(connector);
-
-	/* dvo supports only 2 dpms states. */
-	if (mode != DRM_MODE_DPMS_ON)
-		mode = DRM_MODE_DPMS_OFF;
-
-	if (mode == connector->dpms)
-		return 0;
-
-	connector->dpms = mode;
-
-	/* Only need to change hw state when actually enabled */
-	crtc = intel_sdvo->base.base.crtc;
-	if (!crtc) {
-		intel_sdvo->base.connectors_active = false;
-		return 0;
-	}
-
-	/* We set active outputs manually below in case pipe dpms doesn't change
-	 * due to cloning. */
-	if (mode != DRM_MODE_DPMS_ON) {
-		intel_sdvo_set_active_outputs(intel_sdvo, 0);
-		if (0)
-			intel_sdvo_set_encoder_power_state(intel_sdvo, mode);
-
-		intel_sdvo->base.connectors_active = false;
-
-		intel_crtc_update_dpms(crtc);
-	} else {
-		intel_sdvo->base.connectors_active = true;
-
-		intel_crtc_update_dpms(crtc);
-
-		if (0)
-			intel_sdvo_set_encoder_power_state(intel_sdvo, mode);
-		intel_sdvo_set_active_outputs(intel_sdvo, intel_sdvo->attached_output);
-	}
-
-	return 0;
-}
-
 static enum drm_mode_status
 intel_sdvo_mode_valid(struct drm_connector *connector,
 		      struct drm_display_mode *mode)
@@ -2190,7 +2145,7 @@ done:
 }
 
 static const struct drm_connector_funcs intel_sdvo_connector_funcs = {
-	.dpms = intel_sdvo_dpms,
+	.dpms = drm_atomic_helper_connector_dpms,
 	.detect = intel_sdvo_detect,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.set_property = intel_sdvo_set_property,
diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
index 8b9d325bda3c..0568ae6ec9dd 100644
--- a/drivers/gpu/drm/i915/intel_tv.c
+++ b/drivers/gpu/drm/i915/intel_tv.c
@@ -1509,7 +1509,7 @@ out:
 }
 
 static const struct drm_connector_funcs intel_tv_connector_funcs = {
-	.dpms = intel_connector_dpms,
+	.dpms = drm_atomic_helper_connector_dpms,
 	.detect = intel_tv_detect,
 	.destroy = intel_tv_destroy,
 	.set_property = intel_tv_set_property,
-- 
2.1.0

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

[PATCH v2 08/12] drm/i915: Remove connectors_active from sanitization.

[Maarten Lankhorst]

connectors_active will be removed, so just calculate this right here.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ed9eba2666e2..341fadb40c81 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14935,8 +14935,10 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
 	/* 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, encoder)
-		enable |= encoder->connectors_active;
+	for_each_encoder_on_crtc(dev, &crtc->base, encoder) {
+		enable = true;
+		break;
+	}
 
 	if (!enable)
 		intel_crtc_disable_noatomic(&crtc->base);
@@ -14992,6 +14994,7 @@ static void intel_sanitize_encoder(struct intel_encoder *encoder)
 {
 	struct intel_connector *connector;
 	struct drm_device *dev = encoder->base.dev;
+	bool active = false;
 
 	/* We need to check both for a crtc link (meaning that the
 	 * encoder is active and trying to read from a pipe) and the
@@ -14999,7 +15002,15 @@ static void intel_sanitize_encoder(struct intel_encoder *encoder)
 	bool has_active_crtc = encoder->base.crtc &&
 		to_intel_crtc(encoder->base.crtc)->active;
 
-	if (encoder->connectors_active && !has_active_crtc) {
+	for_each_intel_connector(dev, connector) {
+		if (connector->encoder != encoder)
+			continue;
+
+		active = true;
+		break;
+	}
+
+	if (active && !has_active_crtc) {
 		DRM_DEBUG_KMS("[ENCODER:%d:%s] has active connectors but no active pipe!\n",
 			      encoder->base.base.id,
 			      encoder->base.name);
-- 
2.1.0

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

[PATCH v2 09/12] drm/i915: Remove connectors_active from intel_dp.c.

[Maarten Lankhorst]

Now that everything's atomic, checking encoder->base.crtc is enough.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index cea7d1785d13..dccd9a0e2526 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2624,7 +2624,7 @@ static void vlv_steal_power_sequencer(struct drm_device *dev,
 		DRM_DEBUG_KMS("stealing pipe %c power sequencer from port %c\n",
 			      pipe_name(pipe), port_name(port));
 
-		WARN(encoder->connectors_active,
+		WARN(encoder->base.crtc,
 		     "stealing pipe %c power sequencer from active eDP port %c\n",
 		     pipe_name(pipe), port_name(port));
 
@@ -4240,10 +4240,7 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 
 	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
 
-	if (!intel_encoder->connectors_active)
-		return;
-
-	if (WARN_ON(!intel_encoder->base.crtc))
+	if (!intel_encoder->base.crtc)
 		return;
 
 	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
-- 
2.1.0

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

[PATCH v2 10/12] drm/i915: Remove connectors_active.

[Maarten Lankhorst]

There are no more users, byebye!

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 37 +-----------------------------------
 drivers/gpu/drm/i915/intel_drv.h     |  1 -
 2 files changed, 1 insertion(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 341fadb40c81..23175ce6583d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12196,27 +12196,12 @@ static bool intel_crtc_in_use(struct drm_crtc *crtc)
 static void
 intel_modeset_update_state(struct drm_atomic_state *state)
 {
-	struct drm_device *dev = state->dev;
-	struct intel_encoder *intel_encoder;
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *crtc_state;
-	struct drm_connector *connector;
 	int i;
 
 	intel_shared_dpll_commit(state);
 
-	for_each_intel_encoder(dev, intel_encoder) {
-		if (!intel_encoder->base.crtc)
-			continue;
-
-		crtc = intel_encoder->base.crtc;
-		crtc_state = drm_atomic_get_existing_crtc_state(state, crtc);
-		if (!crtc_state || !needs_modeset(crtc->state))
-			continue;
-
-		intel_encoder->connectors_active = false;
-	}
-
 	drm_atomic_helper_update_legacy_modeset_state(state->dev, state);
 
 	/* Double check state. */
@@ -12231,21 +12216,6 @@ intel_modeset_update_state(struct drm_atomic_state *state)
 		else
 			crtc->hwmode.crtc_clock = 0;
 	}
-
-	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
-		if (!connector->encoder || !connector->encoder->crtc)
-			continue;
-
-		crtc = connector->encoder->crtc;
-		crtc_state = drm_atomic_get_existing_crtc_state(state, crtc);
-		if (!crtc_state || !needs_modeset(crtc->state))
-			continue;
-
-		if (crtc->state->active) {
-			intel_encoder = to_intel_encoder(connector->encoder);
-			intel_encoder->connectors_active = true;
-		}
-	}
 }
 
 static bool intel_fuzzy_clock_check(int clock1, int clock2)
@@ -14965,10 +14935,8 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
 		 *  actually up, hence no need to break them. */
 		WARN_ON(crtc->active);
 
-		for_each_encoder_on_crtc(dev, &crtc->base, encoder) {
-			WARN_ON(encoder->connectors_active);
+		for_each_encoder_on_crtc(dev, &crtc->base, encoder)
 			encoder->base.crtc = NULL;
-		}
 	}
 
 	if (crtc->active || HAS_GMCH_DISPLAY(dev)) {
@@ -15027,7 +14995,6 @@ static void intel_sanitize_encoder(struct intel_encoder *encoder)
 				encoder->post_disable(encoder);
 		}
 		encoder->base.crtc = NULL;
-		encoder->connectors_active = false;
 
 		/* Inconsistent output/port/pipe state happens presumably due to
 		 * a bug in one of the get_hw_state functions. Or someplace else
@@ -15189,7 +15156,6 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
 			encoder->base.crtc = NULL;
 		}
 
-		encoder->connectors_active = false;
 		DRM_DEBUG_KMS("[ENCODER:%d:%s] hw state readout: %s, pipe %c\n",
 			      encoder->base.base.id,
 			      encoder->base.name,
@@ -15200,7 +15166,6 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
 	for_each_intel_connector(dev, connector) {
 		if (connector->get_hw_state(connector)) {
 			connector->base.dpms = DRM_MODE_DPMS_ON;
-			connector->encoder->connectors_active = true;
 			connector->base.encoder = &connector->encoder->base;
 		} else {
 			connector->base.dpms = DRM_MODE_DPMS_OFF;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 62073d2f83fa..c30bbb22381b 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -133,7 +133,6 @@ struct intel_encoder {
 
 	enum intel_output_type type;
 	unsigned int cloneable;
-	bool connectors_active;
 	void (*hot_plug)(struct intel_encoder *);
 	bool (*compute_config)(struct intel_encoder *,
 			       struct intel_crtc_state *);
-- 
2.1.0

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

[PATCH v2 11/12] drm/i915: Only update mode related state if a modeset happened.

[Maarten Lankhorst]

The rest will be a noop anyway, since without modeset there will be
no updated dplls and no modeset state to update.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 30 +++++++-----------------------
 1 file changed, 7 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 23175ce6583d..280629911d41 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12181,33 +12181,15 @@ fail:
 	return ret;
 }
 
-static bool intel_crtc_in_use(struct drm_crtc *crtc)
-{
-	struct drm_encoder *encoder;
-	struct drm_device *dev = crtc->dev;
-
-	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head)
-		if (encoder->crtc == crtc)
-			return true;
-
-	return false;
-}
-
 static void
-intel_modeset_update_state(struct drm_atomic_state *state)
+intel_modeset_update_crtc_state(struct drm_atomic_state *state)
 {
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *crtc_state;
 	int i;
 
-	intel_shared_dpll_commit(state);
-
-	drm_atomic_helper_update_legacy_modeset_state(state->dev, state);
-
 	/* Double check state. */
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
-		WARN_ON(crtc->state->enable != intel_crtc_in_use(crtc));
-
 		to_intel_crtc(crtc)->config = to_intel_crtc_state(crtc->state);
 
 		/* Update hwmode for vblank functions */
@@ -13109,12 +13091,14 @@ static int intel_atomic_commit(struct drm_device *dev,
 
 	/* Only after disabling all output pipelines that will be changed can we
 	 * update the the output configuration. */
-	intel_modeset_update_state(state);
+	intel_modeset_update_crtc_state(state);
 
-	/* The state has been swaped above, so state actually contains the
-	 * old state now. */
-	if (any_ms)
+	if (any_ms) {
+		intel_shared_dpll_commit(state);
+
+		drm_atomic_helper_update_legacy_modeset_state(state->dev, state);
 		modeset_update_crtc_power_domains(state);
+	}
 
 	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
-- 
2.1.0

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

[PATCH v2 12/12] drm/i915: Handle return value in intel_pin_and_fence_fb_obj, v2.

[Maarten Lankhorst]

-EDEADLK has special meaning in atomic, but get_fence may call
i915_find_fence_reg which can return -EDEADLK.

This has special meaning in the atomic world, so convert the error
to -EBUSY for this case.

Changes since v1:
- Add comment in the code.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 280629911d41..e65222a2a207 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2395,7 +2395,18 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
 	 * a fence as the cost is not that onerous.
 	 */
 	ret = i915_gem_object_get_fence(obj);
-	if (ret)
+	if (ret == -EDEADLK) {
+		/*
+		 * -EDEADLK means there are no free fences
+		 * no pending flips.
+		 *
+		 * This is propagated to atomic, but it uses
+		 * -EDEADLK to force a locking recovery, so
+		 * change the returned error to -EBUSY.
+		 */
+		ret = -EBUSY;
+		goto err_unpin;
+	} else if (ret)
 		goto err_unpin;
 
 	i915_gem_object_pin_fence(obj);
-- 
2.1.0

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

Re: [PATCH v2 01/12] drm/i915: Make the force_thru workaround atomic.

[Daniel Vetter]

On Mon, Jul 27, 2015 at 02:35:30PM +0200, Maarten Lankhorst wrote:
> Set active_changed to force a modeset if the panel fitter's force
> enabled.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Hm, shouldn't our fancy fastset logic be able to detect that we've changed
pfit change here and force a full modeset? Or am I blind again?

Abusing active_changed for this feels a bit tricksy tbh, can't we use
mode_changed for this? mode_changed is kinda for "crtc configuration that
needs a full modeset changed", not just for modes. active_changed is
"enable/disable it, but strictly speaking no need to recompute stuff".

At least that's how the atomic helpers treat it.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_debugfs.c  | 82 +++++++++++-------------------------
>  drivers/gpu/drm/i915/intel_display.c |  3 ++
>  2 files changed, 27 insertions(+), 58 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 23a69307e12e..22467adee958 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -3645,74 +3645,40 @@ static int ilk_pipe_crc_ctl_reg(enum intel_pipe_crc_source *source,
>  	return 0;
>  }
>  
> -static void hsw_trans_edp_pipe_A_crc_wa(struct drm_device *dev)
> +static void hsw_trans_edp_pipe_A_crc_wa(struct drm_device *dev, bool enable)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *crtc =
>  		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_A]);
>  	struct intel_crtc_state *pipe_config;
> +	struct drm_atomic_state *state;
> +	int ret = 0;
>  
>  	drm_modeset_lock_all(dev);
> -	pipe_config = to_intel_crtc_state(crtc->base.state);
> -
> -	/*
> -	 * If we use the eDP transcoder we need to make sure that we don't
> -	 * bypass the pfit, since otherwise the pipe CRC source won't work. Only
> -	 * relevant on hsw with pipe A when using the always-on power well
> -	 * routing.
> -	 */
> -	if (pipe_config->cpu_transcoder == TRANSCODER_EDP &&
> -	    !pipe_config->pch_pfit.enabled) {
> -		bool active = pipe_config->base.active;
> -
> -		if (active) {
> -			intel_crtc_control(&crtc->base, false);
> -			pipe_config = to_intel_crtc_state(crtc->base.state);
> -		}
> -
> -		pipe_config->pch_pfit.force_thru = true;
> -
> -		intel_display_power_get(dev_priv,
> -					POWER_DOMAIN_PIPE_PANEL_FITTER(PIPE_A));
> -
> -		if (active)
> -			intel_crtc_control(&crtc->base, true);
> +	state = drm_atomic_state_alloc(dev);
> +	if (!state) {
> +		ret = -ENOMEM;
> +		goto out;
>  	}
> -	drm_modeset_unlock_all(dev);
> -}
>  
> -static void hsw_undo_trans_edp_pipe_A_crc_wa(struct drm_device *dev)
> -{
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct intel_crtc *crtc =
> -		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_A]);
> -	struct intel_crtc_state *pipe_config;
> -
> -	drm_modeset_lock_all(dev);
> -	/*
> -	 * If we use the eDP transcoder we need to make sure that we don't
> -	 * bypass the pfit, since otherwise the pipe CRC source won't work. Only
> -	 * relevant on hsw with pipe A when using the always-on power well
> -	 * routing.
> -	 */
> -	pipe_config = to_intel_crtc_state(crtc->base.state);
> -	if (pipe_config->pch_pfit.force_thru) {
> -		bool active = pipe_config->base.active;
> -
> -		if (active) {
> -			intel_crtc_control(&crtc->base, false);
> -			pipe_config = to_intel_crtc_state(crtc->base.state);
> -		}
> -
> -		pipe_config->pch_pfit.force_thru = false;
> +	state->acquire_ctx = drm_modeset_legacy_acquire_ctx(&crtc->base);
> +	pipe_config = intel_atomic_get_crtc_state(state, crtc);
> +	if (IS_ERR(pipe_config)) {
> +		ret = PTR_ERR(pipe_config);
> +		goto out;
> +	}
>  
> -		intel_display_power_put(dev_priv,
> -					POWER_DOMAIN_PIPE_PANEL_FITTER(PIPE_A));
> +	pipe_config->pch_pfit.force_thru = enable;
> +	if (pipe_config->cpu_transcoder == TRANSCODER_EDP &&
> +	    pipe_config->pch_pfit.enabled != enable)
> +		pipe_config->base.active_changed = true;
>  
> -		if (active)
> -			intel_crtc_control(&crtc->base, true);
> -	}
> +	ret = drm_atomic_commit(state);
> +out:
>  	drm_modeset_unlock_all(dev);
> +	WARN(ret, "Toggling workaround to %i returns %i\n", enable, ret);
> +	if (ret)
> +		drm_atomic_state_free(state);
>  }
>  
>  static int ivb_pipe_crc_ctl_reg(struct drm_device *dev,
> @@ -3732,7 +3698,7 @@ static int ivb_pipe_crc_ctl_reg(struct drm_device *dev,
>  		break;
>  	case INTEL_PIPE_CRC_SOURCE_PF:
>  		if (IS_HASWELL(dev) && pipe == PIPE_A)
> -			hsw_trans_edp_pipe_A_crc_wa(dev);
> +			hsw_trans_edp_pipe_A_crc_wa(dev, true);
>  
>  		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PF_IVB;
>  		break;
> @@ -3844,7 +3810,7 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
>  		else if (IS_VALLEYVIEW(dev))
>  			vlv_undo_pipe_scramble_reset(dev, pipe);
>  		else if (IS_HASWELL(dev) && pipe == PIPE_A)
> -			hsw_undo_trans_edp_pipe_A_crc_wa(dev);
> +			hsw_trans_edp_pipe_A_crc_wa(dev, false);
>  
>  		hsw_enable_ips(crtc);
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 43b0f17ad1fa..7747520bf9f6 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12160,6 +12160,7 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
>  	struct intel_dpll_hw_state dpll_hw_state;
>  	enum intel_dpll_id shared_dpll;
>  	uint32_t ddi_pll_sel;
> +	bool force_thru;
>  
>  	/* FIXME: before the switch to atomic started, a new pipe_config was
>  	 * kzalloc'd. Code that depends on any field being zero should be
> @@ -12171,6 +12172,7 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
>  	shared_dpll = crtc_state->shared_dpll;
>  	dpll_hw_state = crtc_state->dpll_hw_state;
>  	ddi_pll_sel = crtc_state->ddi_pll_sel;
> +	force_thru = crtc_state->pch_pfit.force_thru;
>  
>  	memset(crtc_state, 0, sizeof *crtc_state);
>  
> @@ -12179,6 +12181,7 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
>  	crtc_state->shared_dpll = shared_dpll;
>  	crtc_state->dpll_hw_state = dpll_hw_state;
>  	crtc_state->ddi_pll_sel = ddi_pll_sel;
> +	crtc_state->pch_pfit.force_thru = force_thru;
>  }
>  
>  static int
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 01/12] drm/i915: Make the force_thru workaround atomic.

[Maarten Lankhorst]

Op 27-07-15 om 16:04 schreef Daniel Vetter:
> On Mon, Jul 27, 2015 at 02:35:30PM +0200, Maarten Lankhorst wrote:
>> Set active_changed to force a modeset if the panel fitter's force
>> enabled.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Hm, shouldn't our fancy fastset logic be able to detect that we've changed
> pfit change here and force a full modeset? Or am I blind again?
>
> Abusing active_changed for this feels a bit tricksy tbh, can't we use
> mode_changed for this? mode_changed is kinda for "crtc configuration that
> needs a full modeset changed", not just for modes. active_changed is
> "enable/disable it, but strictly speaking no need to recompute stuff".
>
> At least that's how the atomic helpers treat it.
>
I think for !PIPE_A it's ok, but pipe_a + edp can use a different power well iirc.
I wasn't sure how that was treated so I went for active_changed instead of mode_changed.

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

Re: [PATCH v2 01/12] drm/i915: Make the force_thru workaround atomic.

[Daniel Vetter]

On Tue, Jul 28, 2015 at 09:57:37AM +0200, Maarten Lankhorst wrote:
> Op 27-07-15 om 16:04 schreef Daniel Vetter:
> > On Mon, Jul 27, 2015 at 02:35:30PM +0200, Maarten Lankhorst wrote:
> >> Set active_changed to force a modeset if the panel fitter's force
> >> enabled.
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Hm, shouldn't our fancy fastset logic be able to detect that we've changed
> > pfit change here and force a full modeset? Or am I blind again?
> >
> > Abusing active_changed for this feels a bit tricksy tbh, can't we use
> > mode_changed for this? mode_changed is kinda for "crtc configuration that
> > needs a full modeset changed", not just for modes. active_changed is
> > "enable/disable it, but strictly speaking no need to recompute stuff".
> >
> > At least that's how the atomic helpers treat it.
> >
> I think for !PIPE_A it's ok, but pipe_a + edp can use a different power well iirc.
> I wasn't sure how that was treated so I went for active_changed instead of mode_changed.

Following up from our irc discussion: mode_changed is indeed not a good
idea since with the fastset tricks we might accidentally ellide the
modeset if intel_pipe_config_compare doesn't catch it (and right now it
won't). But I still think active_changed is abuse since active_changed
should _not_ result in state recomputation really, the entire point of
crtc_state->active is that you can always flip it and it will never fail
due to lack of resources.

But with your latest patches we have connector_changed to track routing
changes, and force_thru is very much a routing change. I think using that
would be best.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 12/12] drm/i915: Handle return value in intel_pin_and_fence_fb_obj, v2.

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6874
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
ILK                                  297/297              297/297
SNB                                  315/315              315/315
IVB                                  342/342              342/342
BYT                                  284/284              284/284
HSW                                  378/378              378/378
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2.1 01/12] drm/i915: Make the force_thru workaround atomic, v2.

[Maarten Lankhorst]

Set connectors_changed to force a modeset if the panel fitter's force
enabled on eDP.

Changes since v1:
- Use connectors_changed instead of active_changed because it's a
  routing update.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 23a69307e12e..d1c643a82267 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3645,74 +3645,40 @@ static int ilk_pipe_crc_ctl_reg(enum intel_pipe_crc_source *source,
 	return 0;
 }
 
-static void hsw_trans_edp_pipe_A_crc_wa(struct drm_device *dev)
+static void hsw_trans_edp_pipe_A_crc_wa(struct drm_device *dev, bool enable)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *crtc =
 		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_A]);
 	struct intel_crtc_state *pipe_config;
+	struct drm_atomic_state *state;
+	int ret = 0;
 
 	drm_modeset_lock_all(dev);
-	pipe_config = to_intel_crtc_state(crtc->base.state);
-
-	/*
-	 * If we use the eDP transcoder we need to make sure that we don't
-	 * bypass the pfit, since otherwise the pipe CRC source won't work. Only
-	 * relevant on hsw with pipe A when using the always-on power well
-	 * routing.
-	 */
-	if (pipe_config->cpu_transcoder == TRANSCODER_EDP &&
-	    !pipe_config->pch_pfit.enabled) {
-		bool active = pipe_config->base.active;
-
-		if (active) {
-			intel_crtc_control(&crtc->base, false);
-			pipe_config = to_intel_crtc_state(crtc->base.state);
-		}
-
-		pipe_config->pch_pfit.force_thru = true;
-
-		intel_display_power_get(dev_priv,
-					POWER_DOMAIN_PIPE_PANEL_FITTER(PIPE_A));
-
-		if (active)
-			intel_crtc_control(&crtc->base, true);
+	state = drm_atomic_state_alloc(dev);
+	if (!state) {
+		ret = -ENOMEM;
+		goto out;
 	}
-	drm_modeset_unlock_all(dev);
-}
 
-static void hsw_undo_trans_edp_pipe_A_crc_wa(struct drm_device *dev)
-{
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_crtc *crtc =
-		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_A]);
-	struct intel_crtc_state *pipe_config;
-
-	drm_modeset_lock_all(dev);
-	/*
-	 * If we use the eDP transcoder we need to make sure that we don't
-	 * bypass the pfit, since otherwise the pipe CRC source won't work. Only
-	 * relevant on hsw with pipe A when using the always-on power well
-	 * routing.
-	 */
-	pipe_config = to_intel_crtc_state(crtc->base.state);
-	if (pipe_config->pch_pfit.force_thru) {
-		bool active = pipe_config->base.active;
-
-		if (active) {
-			intel_crtc_control(&crtc->base, false);
-			pipe_config = to_intel_crtc_state(crtc->base.state);
-		}
-
-		pipe_config->pch_pfit.force_thru = false;
+	state->acquire_ctx = drm_modeset_legacy_acquire_ctx(&crtc->base);
+	pipe_config = intel_atomic_get_crtc_state(state, crtc);
+	if (IS_ERR(pipe_config)) {
+		ret = PTR_ERR(pipe_config);
+		goto out;
+	}
 
-		intel_display_power_put(dev_priv,
-					POWER_DOMAIN_PIPE_PANEL_FITTER(PIPE_A));
+	pipe_config->pch_pfit.force_thru = enable;
+	if (pipe_config->cpu_transcoder == TRANSCODER_EDP &&
+	    pipe_config->pch_pfit.enabled != enable)
+		pipe_config->base.connectors_changed = true;
 
-		if (active)
-			intel_crtc_control(&crtc->base, true);
-	}
+	ret = drm_atomic_commit(state);
+out:
 	drm_modeset_unlock_all(dev);
+	WARN(ret, "Toggling workaround to %i returns %i\n", enable, ret);
+	if (ret)
+		drm_atomic_state_free(state);
 }
 
 static int ivb_pipe_crc_ctl_reg(struct drm_device *dev,
@@ -3732,7 +3698,7 @@ static int ivb_pipe_crc_ctl_reg(struct drm_device *dev,
 		break;
 	case INTEL_PIPE_CRC_SOURCE_PF:
 		if (IS_HASWELL(dev) && pipe == PIPE_A)
-			hsw_trans_edp_pipe_A_crc_wa(dev);
+			hsw_trans_edp_pipe_A_crc_wa(dev, true);
 
 		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PF_IVB;
 		break;
@@ -3844,7 +3810,7 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
 		else if (IS_VALLEYVIEW(dev))
 			vlv_undo_pipe_scramble_reset(dev, pipe);
 		else if (IS_HASWELL(dev) && pipe == PIPE_A)
-			hsw_undo_trans_edp_pipe_A_crc_wa(dev);
+			hsw_trans_edp_pipe_A_crc_wa(dev, false);
 
 		hsw_enable_ips(crtc);
 	}
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 43b0f17ad1fa..7747520bf9f6 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12160,6 +12160,7 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
 	struct intel_dpll_hw_state dpll_hw_state;
 	enum intel_dpll_id shared_dpll;
 	uint32_t ddi_pll_sel;
+	bool force_thru;
 
 	/* FIXME: before the switch to atomic started, a new pipe_config was
 	 * kzalloc'd. Code that depends on any field being zero should be
@@ -12171,6 +12172,7 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
 	shared_dpll = crtc_state->shared_dpll;
 	dpll_hw_state = crtc_state->dpll_hw_state;
 	ddi_pll_sel = crtc_state->ddi_pll_sel;
+	force_thru = crtc_state->pch_pfit.force_thru;
 
 	memset(crtc_state, 0, sizeof *crtc_state);
 
@@ -12179,6 +12181,7 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
 	crtc_state->shared_dpll = shared_dpll;
 	crtc_state->dpll_hw_state = dpll_hw_state;
 	crtc_state->ddi_pll_sel = ddi_pll_sel;
+	crtc_state->pch_pfit.force_thru = force_thru;
 }
 
 static int

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

Re: [PATCH v2 02/12] drm/i915: Update atomic state when removing mst connector.

[Ander Conselvan De Oliveira]

Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>

On Mon, 2015-07-27 at 14:35 +0200, Maarten Lankhorst wrote:
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |  7 ------
>  drivers/gpu/drm/i915/intel_dp_mst.c  | 45 
> +++++++++++++++++++++++++++++++++++-
>  2 files changed, 44 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index 7747520bf9f6..3ab0a8a8e702 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12751,13 +12751,6 @@ check_encoder_state(struct drm_device *dev)
>  					encoder->base.crtc,
>  			     "connector's crtc doesn't match encoder 
> crtc\n");
>  		}
> -		/*
> -		 * for MST connectors if we unplug the connector is 
> gone
> -		 * away but the encoder is still connected to a crtc
> -		 * until a modeset happens in response to the 
> hotplug.
> -		 */
> -		if (!enabled && encoder->base.encoder_type == 
> DRM_MODE_ENCODER_DPMST)
> -			continue;
>  
>  		I915_STATE_WARN(!!encoder->base.crtc != enabled,
>  		     "encoder's enabled state mismatch "
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c 
> b/drivers/gpu/drm/i915/intel_dp_mst.c
> index 585f0a45b3f1..35f2eb59818a 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -448,6 +448,49 @@ static struct drm_connector 
> *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
>  	return connector;
>  }
>  
> +static void
> +intel_dp_unbind_mst_connector(struct drm_device *dev,
> +			      struct drm_connector *connector)
> +{
> +	struct drm_atomic_state *state;
> +	struct drm_connector_state *conn_state;
> +	struct drm_crtc *crtc = connector->state->crtc;
> +	int ret;
> +
> +	if (!crtc)
> +		return;
> +
> +	state = drm_atomic_state_alloc(dev);
> +	if (!state)
> +		return;
> +
> +	state->acquire_ctx = dev->mode_config.acquire_ctx;
> +
> +	conn_state = drm_atomic_get_connector_state(state, 
> connector);
> +	ret = PTR_ERR_OR_ZERO(conn_state);
> +	if (!ret)
> +		ret = drm_atomic_set_crtc_for_connector(conn_state, 
> NULL);
> +
> +	if (!ret)
> +		ret = drm_atomic_add_affected_connectors(state, 
> crtc);
> +
> +	if (!ret && !drm_atomic_connectors_for_crtc(state, crtc)) {
> +		struct drm_crtc_state *crtc_state =
> +			drm_atomic_get_existing_crtc_state(state, 
> crtc);
> +
> +		crtc_state->active = false;
> +		ret = drm_atomic_set_mode_for_crtc(crtc_state, 
> NULL);
> +	}
> +
> +	if (!ret)
> +		ret = drm_atomic_commit(state);
> +
> +	if (ret)
> +		drm_atomic_state_free(state);
> +
> +	I915_STATE_WARN_ON(ret);
> +}
> +
>  static void intel_dp_destroy_mst_connector(struct 
> drm_dp_mst_topology_mgr *mgr,
>  					   struct drm_connector 
> *connector)
>  {
> @@ -455,7 +498,7 @@ static void intel_dp_destroy_mst_connector(struct 
> drm_dp_mst_topology_mgr *mgr,
>  	struct drm_device *dev = connector->dev;
>  	/* need to nuke the connector */
>  	drm_modeset_lock_all(dev);
> -	intel_connector_dpms(connector, DRM_MODE_DPMS_OFF);
> +	intel_dp_unbind_mst_connector(dev, connector);
>  	drm_modeset_unlock_all(dev);
>  
>  	intel_connector->unregister(intel_connector);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 03/12] drm/i915: Convert connector checking to atomic.

[Ander Conselvan De Oliveira]

On Mon, 2015-07-27 at 14:35 +0200, Maarten Lankhorst wrote:
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_crt.c     |  2 -
 drivers/gpu/drm/i915/intel_display.c | 79 ++++++++++++++++--------------------
 drivers/gpu/drm/i915/intel_drv.h     |  1 -
 drivers/gpu/drm/i915/intel_dvo.c     |  2 -
 drivers/gpu/drm/i915/intel_sdvo.c    |  2 -
 5 files changed, 36 insertions(+), 50 deletions(-)


diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index 5d78c1feec81..9eba3dd5b434 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -280,8 +280,6 @@ static int intel_crt_dpms(struct drm_connector *connector, int mode)
 		intel_crtc_update_dpms(crtc);
 	}
 
-	intel_modeset_check_state(connector->dev);
-
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3ab0a8a8e702..ba0b68a4209d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6360,42 +6360,33 @@ static void intel_encoder_dpms(struct intel_encoder *encoder, int mode)
  * internal consistency). */
 static void intel_connector_check_state(struct intel_connector *connector)
 {
+	struct drm_crtc *crtc = connector->base.state->crtc;
+
+	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
+		      connector->base.base.id,
+		      connector->base.name);
+
 	if (connector->get_hw_state(connector)) {
-		struct intel_encoder *encoder = connector->encoder;
-		struct drm_crtc *crtc;
-		bool encoder_enabled;
-		enum pipe pipe;
+		struct drm_encoder *encoder = &connector->encoder->base;
+		struct drm_connector_state *conn_state = connector->base.state;
 
-		DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
-			      connector->base.base.id,
-			      connector->base.name);
+		I915_STATE_WARN(!crtc,
+			 "connector enabled without attached crtc\n");
 
-		/* there is no real hw state for MST connectors */
-		if (connector->mst_port)

Should the above deletion be part of the previous patch?

> +		if (!crtc)
 			return;
 
-		I915_STATE_WARN(connector->base.dpms == DRM_MODE_DPMS_OFF,
-		     "wrong connector dpms state\n");
-		I915_STATE_WARN(connector->base.encoder != &encoder->base,
-		     "active connector not linked to encoder\n");
+		I915_STATE_WARN(!crtc->state->active,
+		      "connector is active, but attached crtc isn't\n");
 
-		if (encoder) {
-			I915_STATE_WARN(!encoder->connectors_active,
-			     "encoder->connectors_active not set\n");

The converted version doesn't check connectors_active. I see it is
later removed in the patch series, so I assume it is left out on
purpose, but I think this is worth mentioning in the commit message.

> -
-			encoder_enabled = encoder->get_hw_state(encoder, &pipe);
-			I915_STATE_WARN(!encoder_enabled, "encoder not enabled\n");
-			if (I915_STATE_WARN_ON(!encoder->base.crtc))
-				return;

These are also left out. It looks like check_encoder_state() performs
the same check, but I think the commit message should mention this if
that's the case.

> +		I915_STATE_WARN(conn_state->best_encoder != encoder,
+			"atomic encoder doesn't match attached encoder\n");
 
-			crtc = encoder->base.crtc;
-
-			I915_STATE_WARN(!crtc->state->enable,
-					"crtc not enabled\n");
-			I915_STATE_WARN(!to_intel_crtc(crtc)->active, "crtc not active\n");
-			I915_STATE_WARN(pipe != to_intel_crtc(crtc)->pipe,
-			     "encoder active on the wrong pipe\n");
-		}
+		I915_STATE_WARN(conn_state->crtc != encoder->crtc,
+			"attached encoder crtc differs from connector crtc\n");
+	} else {
+		I915_STATE_WARN(!crtc && connector->base.state->best_encoder,
+			"best encoder set without crtc!\n");
 	}
 }
 
@@ -6444,8 +6435,6 @@ int intel_connector_dpms(struct drm_connector *connector, int mode)
 	if (connector->encoder)
 		intel_encoder_dpms(to_intel_encoder(connector->encoder), mode);
 
-	intel_modeset_check_state(connector->dev);
-
 	return 0;
 }
 
@@ -12705,20 +12694,22 @@ static void check_wm_state(struct drm_device *dev)
 }
 
 static void
-check_connector_state(struct drm_device *dev)
+check_connector_state(struct drm_device *dev, struct drm_atomic_state *state)

This is called after a state swap, so I think the state variable should
be called old_state for clarity.

>  {
-	struct intel_connector *connector;
+	struct drm_connector_state *conn_state;
+	struct drm_connector *connector;
+	int i;
 
-	for_each_intel_connector(dev, connector) {
-		struct drm_encoder *encoder = connector->base.encoder;
-		struct drm_connector_state *state = connector->base.state;
+	for_each_connector_in_state(state, connector, conn_state, i) {
+		struct drm_encoder *encoder = connector->encoder;
+		struct drm_connector_state *state = connector->state;
 
 		/* This also checks the encoder/connector hw state with the
 		 * ->get_hw_state callbacks. */
-		intel_connector_check_state(connector);
+		intel_connector_check_state(to_intel_connector(connector));
 
 		I915_STATE_WARN(state->best_encoder != encoder,
-		     "connector's staged encoder doesn't match current encoder\n");
+		     "connector's atomic encoder doesn't match legacy encoder\n");
 	}
 }
 
@@ -12904,11 +12895,12 @@ check_shared_dpll_state(struct drm_device *dev)
 	}
 }
 
-void
-intel_modeset_check_state(struct drm_device *dev)
+static void
+intel_modeset_check_state(struct drm_device *dev,
+			  struct drm_atomic_state *state)

old_state here too.


>  {
 	check_wm_state(dev);
-	check_connector_state(dev);
+	check_connector_state(dev, state);
 	check_encoder_state(dev);
 	check_crtc_state(dev);
 	check_shared_dpll_state(dev);
@@ -13294,10 +13286,11 @@ static int intel_atomic_commit(struct drm_device *dev,
 
 	drm_atomic_helper_wait_for_vblanks(dev, state);
 	drm_atomic_helper_cleanup_planes(dev, state);
-	drm_atomic_state_free(state);
 
 	if (any_ms)
-		intel_modeset_check_state(dev);
+		intel_modeset_check_state(dev, state);
+
+	drm_atomic_state_free(state);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 320c9e6bd848..0da4236dc85a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -999,7 +999,6 @@ int intel_connector_init(struct intel_connector *);
 struct intel_connector *intel_connector_alloc(void);
 int intel_connector_dpms(struct drm_connector *, int mode);
 bool intel_connector_get_hw_state(struct intel_connector *connector);
-void intel_modeset_check_state(struct drm_device *dev);
 bool ibx_digital_port_connected(struct drm_i915_private *dev_priv,
 				struct intel_digital_port *port);
 void intel_connector_attach_encoder(struct intel_connector *connector,
diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c
index fd5e522abebb..600f7fb855d8 100644
--- a/drivers/gpu/drm/i915/intel_dvo.c
+++ b/drivers/gpu/drm/i915/intel_dvo.c
@@ -237,8 +237,6 @@ static int intel_dvo_dpms(struct drm_connector *connector, int mode)
 		intel_crtc_update_dpms(crtc);
 	}
 
-	intel_modeset_check_state(connector->dev);
-
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index 2c435a79d4da..8911e0e417ee 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -1550,8 +1550,6 @@ static int intel_sdvo_dpms(struct drm_connector *connector, int mode)
 		intel_sdvo_set_active_outputs(intel_sdvo, intel_sdvo->attached_output);
 	}
 
-	intel_modeset_check_state(connector->dev);
-

How does removing the checks from the _dpms() functions relate to
converting the connector checking to atomic? If I understood correctly,
the checks aren't necessary anymore because intel_crtc_control() is
atomic, and that already performs a check. But if that's right this
should be a separate patch. Or did I miss something?

Ander

>  	return 0;
 }
 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 04/12] drm/i915: Remove some unneeded checks from check_crtc_state.

[Ander Conselvan De Oliveira]

Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>

On Mon, 2015-07-27 at 14:35 +0200, Maarten Lankhorst wrote:
> This is handled by the atomic core now, no need to check this for 
> ourself.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 19 +------------------
>  1 file changed, 1 insertion(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index ba0b68a4209d..59eb6db10740 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12781,8 +12781,7 @@ check_crtc_state(struct drm_device *dev)
>  	struct intel_crtc_state pipe_config;
>  
>  	for_each_intel_crtc(dev, crtc) {
> -		bool enabled = false;
> -		bool active = false;
> +		bool active;
>  
>  		memset(&pipe_config, 0, sizeof(pipe_config));
>  
> @@ -12792,22 +12791,6 @@ check_crtc_state(struct drm_device *dev)
>  		I915_STATE_WARN(crtc->active && !crtc->base.state
> ->enable,
>  		     "active crtc, but not enabled in sw 
> tracking\n");
>  
> -		for_each_intel_encoder(dev, encoder) {
> -			if (encoder->base.crtc != &crtc->base)
> -				continue;
> -			enabled = true;
> -			if (encoder->connectors_active)
> -				active = true;
> -		}
> -
> -		I915_STATE_WARN(active != crtc->active,
> -		     "crtc's computed active state doesn't match 
> tracked active state "
> -		     "(expected %i, found %i)\n", active, crtc
> ->active);
> -		I915_STATE_WARN(enabled != crtc->base.state->enable,
> -		     "crtc's computed enabled state doesn't match 
> tracked enabled state "
> -		     "(expected %i, found %i)\n", enabled,
> -				crtc->base.state->enable);
> -
>  		active = dev_priv->display.get_pipe_config(crtc,
>  							  
>  &pipe_config);
>  
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 05/12] drm/i915: Remove connectors_active from state checking.

[Ander Conselvan De Oliveira]

Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>

On Mon, 2015-07-27 at 14:35 +0200, Maarten Lankhorst wrote:
> Connectors are updated atomically now, so the only interaction
> with the encoder is through base.crtc.
> 
> If it's NULL the encoder's not part of any crtc, and if it's
> not NULL then active should be equal to crtc_state->active.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index 59eb6db10740..fbb257d4728c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12728,9 +12728,6 @@ check_encoder_state(struct drm_device *dev)
>  			      encoder->base.base.id,
>  			      encoder->base.name);
>  
> -		I915_STATE_WARN(encoder->connectors_active && 
> !encoder->base.crtc,
> -		     "encoder's active_connectors set, but no 
> crtc\n");
> -
>  		for_each_intel_connector(dev, connector) {
>  			if (connector->base.encoder != &encoder
> ->base)
>  				continue;
> @@ -12750,18 +12747,20 @@ check_encoder_state(struct drm_device *dev)
>  		I915_STATE_WARN(active && !encoder->base.crtc,
>  		     "active encoder with no crtc\n");
>  
> -		I915_STATE_WARN(encoder->connectors_active != 
> active,
> -		     "encoder's computed active state doesn't match 
> tracked active state "
> -		     "(expected %i, found %i)\n", active, encoder
> ->connectors_active);
> -
>  		active = encoder->get_hw_state(encoder, &pipe);
> -		I915_STATE_WARN(active != encoder
> ->connectors_active,
> +
> +		if (!encoder->base.crtc) {
> +			I915_STATE_WARN(active,
> +			     "encoder detached but not turned 
> off.\n");
> +
> +			continue;
> +		}
> +
> +		I915_STATE_WARN(active != encoder->base.crtc->state
> ->active,
>  		     "encoder's hw state doesn't match sw tracking "
>  		     "(expected %i, found %i)\n",
> -		     encoder->connectors_active, active);
> +		     encoder->base.crtc->state->active, active);
>  
> -		if (!encoder->base.crtc)
> -			continue;
>  
>  		tracked_pipe = to_intel_crtc(encoder->base.crtc)
> ->pipe;
>  		I915_STATE_WARN(active && pipe != tracked_pipe,
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 03/12] drm/i915: Convert connector checking to atomic.

[Maarten Lankhorst]

Op 28-07-15 om 15:13 schreef Ander Conselvan De Oliveira:
> On Mon, 2015-07-27 at 14:35 +0200, Maarten Lankhorst wrote:
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_crt.c     |  2 -
>  drivers/gpu/drm/i915/intel_display.c | 79 ++++++++++++++++--------------------
>  drivers/gpu/drm/i915/intel_drv.h     |  1 -
>  drivers/gpu/drm/i915/intel_dvo.c     |  2 -
>  drivers/gpu/drm/i915/intel_sdvo.c    |  2 -
>  5 files changed, 36 insertions(+), 50 deletions(-)
>
>
> diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
> index 5d78c1feec81..9eba3dd5b434 100644
> --- a/drivers/gpu/drm/i915/intel_crt.c
> +++ b/drivers/gpu/drm/i915/intel_crt.c
> @@ -280,8 +280,6 @@ static int intel_crt_dpms(struct drm_connector *connector, int mode)
>  		intel_crtc_update_dpms(crtc);
>  	}
>  
> -	intel_modeset_check_state(connector->dev);
> -
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 3ab0a8a8e702..ba0b68a4209d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6360,42 +6360,33 @@ static void intel_encoder_dpms(struct intel_encoder *encoder, int mode)
>   * internal consistency). */
>  static void intel_connector_check_state(struct intel_connector *connector)
>  {
> +	struct drm_crtc *crtc = connector->base.state->crtc;
> +
> +	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> +		      connector->base.base.id,
> +		      connector->base.name);
> +
>  	if (connector->get_hw_state(connector)) {
> -		struct intel_encoder *encoder = connector->encoder;
> -		struct drm_crtc *crtc;
> -		bool encoder_enabled;
> -		enum pipe pipe;
> +		struct drm_encoder *encoder = &connector->encoder->base;
> +		struct drm_connector_state *conn_state = connector->base.state;
>  
> -		DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> -			      connector->base.base.id,
> -			      connector->base.name);
> +		I915_STATE_WARN(!crtc,
> +			 "connector enabled without attached crtc\n");
>  
> -		/* there is no real hw state for MST connectors */
> -		if (connector->mst_port)
>
> Should the above deletion be part of the previous patch?
Huh weird, indeed..
>> +		if (!crtc)
>  			return;
>  
> -		I915_STATE_WARN(connector->base.dpms == DRM_MODE_DPMS_OFF,
> -		     "wrong connector dpms state\n");
> -		I915_STATE_WARN(connector->base.encoder != &encoder->base,
> -		     "active connector not linked to encoder\n");
> +		I915_STATE_WARN(!crtc->state->active,
> +		      "connector is active, but attached crtc isn't\n");
>  
> -		if (encoder) {
> -			I915_STATE_WARN(!encoder->connectors_active,
> -			     "encoder->connectors_active not set\n");
>
> The converted version doesn't check connectors_active. I see it is
> later removed in the patch series, so I assume it is left out on
> purpose, but I think this is worth mentioning in the commit message.
>
>> -
> -			encoder_enabled = encoder->get_hw_state(encoder, &pipe);
> -			I915_STATE_WARN(!encoder_enabled, "encoder not enabled\n");
> -			if (I915_STATE_WARN_ON(!encoder->base.crtc))
> -				return;
>
> These are also left out. It looks like check_encoder_state() performs
> the same check, but I think the commit message should mention this if
> that's the case.
It's the case.
>> +		I915_STATE_WARN(conn_state->best_encoder != encoder,
> +			"atomic encoder doesn't match attached encoder\n");
>  
> -			crtc = encoder->base.crtc;
> -
> -			I915_STATE_WARN(!crtc->state->enable,
> -					"crtc not enabled\n");
> -			I915_STATE_WARN(!to_intel_crtc(crtc)->active, "crtc not active\n");
> -			I915_STATE_WARN(pipe != to_intel_crtc(crtc)->pipe,
> -			     "encoder active on the wrong pipe\n");
> -		}
> +		I915_STATE_WARN(conn_state->crtc != encoder->crtc,
> +			"attached encoder crtc differs from connector crtc\n");
> +	} else {
> +		I915_STATE_WARN(!crtc && connector->base.state->best_encoder,
> +			"best encoder set without crtc!\n");
>  	}
>  }
>  
> @@ -6444,8 +6435,6 @@ int intel_connector_dpms(struct drm_connector *connector, int mode)
>  	if (connector->encoder)
>  		intel_encoder_dpms(to_intel_encoder(connector->encoder), mode);
>  
> -	intel_modeset_check_state(connector->dev);
> -
>  	return 0;
>  }
>  
> @@ -12705,20 +12694,22 @@ static void check_wm_state(struct drm_device *dev)
>  }
>  
>  static void
> -check_connector_state(struct drm_device *dev)
> +check_connector_state(struct drm_device *dev, struct drm_atomic_state *state)
>
> This is called after a state swap, so I think the state variable should
> be called old_state for clarity.
Ok.
>>  {
> -	struct intel_connector *connector;
> +	struct drm_connector_state *conn_state;
> +	struct drm_connector *connector;
> +	int i;
>  
> -	for_each_intel_connector(dev, connector) {
> -		struct drm_encoder *encoder = connector->base.encoder;
> -		struct drm_connector_state *state = connector->base.state;
> +	for_each_connector_in_state(state, connector, conn_state, i) {
> +		struct drm_encoder *encoder = connector->encoder;
> +		struct drm_connector_state *state = connector->state;
>  
>  		/* This also checks the encoder/connector hw state with the
>  		 * ->get_hw_state callbacks. */
> -		intel_connector_check_state(connector);
> +		intel_connector_check_state(to_intel_connector(connector));
>  
>  		I915_STATE_WARN(state->best_encoder != encoder,
> -		     "connector's staged encoder doesn't match current encoder\n");
> +		     "connector's atomic encoder doesn't match legacy encoder\n");
>  	}
>  }
>  
> @@ -12904,11 +12895,12 @@ check_shared_dpll_state(struct drm_device *dev)
>  	}
>  }
>  
> -void
> -intel_modeset_check_state(struct drm_device *dev)
> +static void
> +intel_modeset_check_state(struct drm_device *dev,
> +			  struct drm_atomic_state *state)
>
> old_state here too.
>
>
>>  {
>  	check_wm_state(dev);
> -	check_connector_state(dev);
> +	check_connector_state(dev, state);
>  	check_encoder_state(dev);
>  	check_crtc_state(dev);
>  	check_shared_dpll_state(dev);
> @@ -13294,10 +13286,11 @@ static int intel_atomic_commit(struct drm_device *dev,
>  
>  	drm_atomic_helper_wait_for_vblanks(dev, state);
>  	drm_atomic_helper_cleanup_planes(dev, state);
> -	drm_atomic_state_free(state);
>  
>  	if (any_ms)
> -		intel_modeset_check_state(dev);
> +		intel_modeset_check_state(dev, state);
> +
> +	drm_atomic_state_free(state);
>  
>  	return 0;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 320c9e6bd848..0da4236dc85a 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -999,7 +999,6 @@ int intel_connector_init(struct intel_connector *);
>  struct intel_connector *intel_connector_alloc(void);
>  int intel_connector_dpms(struct drm_connector *, int mode);
>  bool intel_connector_get_hw_state(struct intel_connector *connector);
> -void intel_modeset_check_state(struct drm_device *dev);
>  bool ibx_digital_port_connected(struct drm_i915_private *dev_priv,
>  				struct intel_digital_port *port);
>  void intel_connector_attach_encoder(struct intel_connector *connector,
> diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c
> index fd5e522abebb..600f7fb855d8 100644
> --- a/drivers/gpu/drm/i915/intel_dvo.c
> +++ b/drivers/gpu/drm/i915/intel_dvo.c
> @@ -237,8 +237,6 @@ static int intel_dvo_dpms(struct drm_connector *connector, int mode)
>  		intel_crtc_update_dpms(crtc);
>  	}
>  
> -	intel_modeset_check_state(connector->dev);
> -
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> index 2c435a79d4da..8911e0e417ee 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -1550,8 +1550,6 @@ static int intel_sdvo_dpms(struct drm_connector *connector, int mode)
>  		intel_sdvo_set_active_outputs(intel_sdvo, intel_sdvo->attached_output);
>  	}
>  
> -	intel_modeset_check_state(connector->dev);
> -
>
> How does removing the checks from the _dpms() functions relate to
> converting the connector checking to atomic? If I understood correctly,
> the checks aren't necessary anymore because intel_crtc_control() is
> atomic, and that already performs a check. But if that's right this
> should be a separate patch. Or did I miss something?
>
Indeed, but there is no state left to check here, since we would have a null pointer.

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

Re: [PATCH v2 06/12] drm/i915: Make crtc checking use the atomic state.

[Ander Conselvan De Oliveira]

On Mon, 2015-07-27 at 14:35 +0200, Maarten Lankhorst wrote:
> Instead of allocating pipe_config on the stack use the old crtc_state,
> it's only going to freed from this point on.
> 
> All crtc's encoders are now only checked once during modeset.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 118 +++++++++++++++++------------------
>  1 file changed, 56 insertions(+), 62 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index fbb257d4728c..e3afe611a78c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11829,7 +11829,7 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
>  	struct intel_crtc_state *pipe_config =
>  		to_intel_crtc_state(crtc_state);
>  	struct drm_atomic_state *state = crtc_state->state;
> -	int ret, idx = crtc->base.id;
> +	int ret;
>  	bool mode_changed = needs_modeset(crtc_state);
>  
>  	if (mode_changed && !check_encoder_cloning(state, intel_crtc)) {
> @@ -11837,10 +11837,6 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
>  		return -EINVAL;
>  	}
>  
> -	I915_STATE_WARN(crtc->state->active != intel_crtc->active,
> -		"[CRTC:%i] mismatch between state->active(%i) and crtc->active(%i)\n",
> -		idx, crtc->state->active, intel_crtc->active);
> -
>  	if (mode_changed && !crtc_state->active)
>  		intel_crtc->atomic.update_wm_post = true;
>  
> @@ -12721,19 +12717,16 @@ check_encoder_state(struct drm_device *dev)
>  
>  	for_each_intel_encoder(dev, encoder) {
>  		bool enabled = false;
> -		bool active = false;
> -		enum pipe pipe, tracked_pipe;
> +		enum pipe pipe;
>  
>  		DRM_DEBUG_KMS("[ENCODER:%d:%s]\n",
>  			      encoder->base.base.id,
>  			      encoder->base.name);
>  
>  		for_each_intel_connector(dev, connector) {
> -			if (connector->base.encoder != &encoder->base)
> +			if (connector->base.state->best_encoder != &encoder->base)
>  				continue;
>  			enabled = true;
> -			if (connector->base.dpms != DRM_MODE_DPMS_OFF)
> -				active = true;
>  
>  			I915_STATE_WARN(connector->base.state->crtc !=
>  					encoder->base.crtc,
> @@ -12744,85 +12737,86 @@ check_encoder_state(struct drm_device *dev)
>  		     "encoder's enabled state mismatch "
>  		     "(expected %i, found %i)\n",
>  		     !!encoder->base.crtc, enabled);
> -		I915_STATE_WARN(active && !encoder->base.crtc,
> -		     "active encoder with no crtc\n");
> -
> -		active = encoder->get_hw_state(encoder, &pipe);
>  
>  		if (!encoder->base.crtc) {
> -			I915_STATE_WARN(active,
> -			     "encoder detached but not turned off.\n");
> +			bool active;
>  
> -			continue;
> +			active = encoder->get_hw_state(encoder, &pipe);
> +			I915_STATE_WARN(active,
> +			     "encoder detached but still enabled on pipe %c.\n",
> +			     pipe_name(pipe));
>  		}
> -
> -		I915_STATE_WARN(active != encoder->base.crtc->state->active,
> -		     "encoder's hw state doesn't match sw tracking "
> -		     "(expected %i, found %i)\n",
> -		     encoder->base.crtc->state->active, active);
> -
> -
> -		tracked_pipe = to_intel_crtc(encoder->base.crtc)->pipe;
> -		I915_STATE_WARN(active && pipe != tracked_pipe,
> -		     "active encoder's pipe doesn't match"
> -		     "(expected %i, found %i)\n",
> -		     tracked_pipe, pipe);
> -
>  	}
>  }
>  
>  static void
> -check_crtc_state(struct drm_device *dev)
> +check_crtc_state(struct drm_device *dev, struct drm_atomic_state *state)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct intel_crtc *crtc;
>  	struct intel_encoder *encoder;
> -	struct intel_crtc_state pipe_config;
> +	struct drm_crtc_state *crtc_state;
> +	struct drm_crtc *crtc;
> +	int i;
>  
> -	for_each_intel_crtc(dev, crtc) {
> +	for_each_crtc_in_state(state, crtc, crtc_state, i) {

So now we only check the state of crtcs affected by the modeset. In the
unlikely case of a bug that changes the hw state of another crtc that
should have been unchanged, the old code might catch the issue but the
new one doesn't. Isn't it better to just check everything?


Ander

> +		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +		struct intel_crtc_state *pipe_config, *sw_config;
>  		bool active;
>  
> -		memset(&pipe_config, 0, sizeof(pipe_config));
> +		if (!needs_modeset(crtc->state))
> +			continue;
>  
> -		DRM_DEBUG_KMS("[CRTC:%d]\n",
> -			      crtc->base.base.id);
> +		__drm_atomic_helper_crtc_destroy_state(crtc, crtc_state);
> +		pipe_config = to_intel_crtc_state(crtc_state);
> +		memset(pipe_config, 0, sizeof(*pipe_config));
> +		crtc_state->crtc = crtc;
> +		crtc_state->state = state;
>  
> -		I915_STATE_WARN(crtc->active && !crtc->base.state->enable,
> -		     "active crtc, but not enabled in sw tracking\n");
> +		DRM_DEBUG_KMS("[CRTC:%d]\n",
> +			      crtc->base.id);
>  
> -		active = dev_priv->display.get_pipe_config(crtc,
> -							   &pipe_config);
> +		active = dev_priv->display.get_pipe_config(intel_crtc,
> +							   pipe_config);
>  
>  		/* hw state is inconsistent with the pipe quirk */
> -		if ((crtc->pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE) ||
> -		    (crtc->pipe == PIPE_B && dev_priv->quirks & QUIRK_PIPEB_FORCE))
> -			active = crtc->active;
> +		if ((intel_crtc->pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE) ||
> +		    (intel_crtc->pipe == PIPE_B && dev_priv->quirks & QUIRK_PIPEB_FORCE))
> +			active = crtc->state->active;
>  
> -		for_each_intel_encoder(dev, encoder) {
> -			enum pipe pipe;
> -			if (encoder->base.crtc != &crtc->base)
> -				continue;
> -			if (encoder->get_hw_state(encoder, &pipe))
> -				encoder->get_config(encoder, &pipe_config);
> -		}
> -
> -		I915_STATE_WARN(crtc->active != active,
> +		I915_STATE_WARN(crtc->state->active != active,
>  		     "crtc active state doesn't match with hw state "
> -		     "(expected %i, found %i)\n", crtc->active, active);
> +		     "(expected %i, found %i)\n", crtc->state->active, active);
>  
> -		I915_STATE_WARN(crtc->active != crtc->base.state->active,
> +		I915_STATE_WARN(intel_crtc->active != crtc->state->active,
>  		     "transitional active state does not match atomic hw state "
> -		     "(expected %i, found %i)\n", crtc->base.state->active, crtc->active);
> +		     "(expected %i, found %i)\n", crtc->state->active, intel_crtc->active);
> +
> +		for_each_encoder_on_crtc(dev, crtc, encoder) {
> +			enum pipe pipe;
> +
> +			active = encoder->get_hw_state(encoder, &pipe);
> +			I915_STATE_WARN(active != crtc->state->active,
> +				"[ENCODER:%i] active %i with crtc active %i\n",
> +				encoder->base.base.id, active, crtc->state->active);
> +
> +			I915_STATE_WARN(active && intel_crtc->pipe != pipe,
> +					"Encoder connected to wrong pipe %c\n",
> +					pipe_name(pipe));
> +
> +			if (active)
> +				encoder->get_config(encoder, pipe_config);
> +		}
>  
> -		if (!active)
> +		if (!crtc->state->active)
>  			continue;
>  
> -		if (!intel_pipe_config_compare(dev, crtc->config,
> -					       &pipe_config, false)) {
> +		sw_config = to_intel_crtc_state(crtc->state);
> +		if (!intel_pipe_config_compare(dev, sw_config,
> +					       pipe_config, false)) {
>  			I915_STATE_WARN(1, "pipe state doesn't match!\n");
> -			intel_dump_pipe_config(crtc, &pipe_config,
> +			intel_dump_pipe_config(intel_crtc, pipe_config,
>  					       "[hw state]");
> -			intel_dump_pipe_config(crtc, crtc->config,
> +			intel_dump_pipe_config(intel_crtc, sw_config,
>  					       "[sw state]");
>  		}
>  	}
> @@ -12884,7 +12878,7 @@ intel_modeset_check_state(struct drm_device *dev,
>  	check_wm_state(dev);
>  	check_connector_state(dev, state);
>  	check_encoder_state(dev);
> -	check_crtc_state(dev);
> +	check_crtc_state(dev, state);
>  	check_shared_dpll_state(dev);
>  }
>  
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 06/12] drm/i915: Make crtc checking use the atomic state.

[Daniel Vetter]

On Wed, Jul 29, 2015 at 02:49:45PM +0300, Ander Conselvan De Oliveira wrote:
> On Mon, 2015-07-27 at 14:35 +0200, Maarten Lankhorst wrote:
> > Instead of allocating pipe_config on the stack use the old crtc_state,
> > it's only going to freed from this point on.
> > 
> > All crtc's encoders are now only checked once during modeset.
> > 
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 118 +++++++++++++++++------------------
> >  1 file changed, 56 insertions(+), 62 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index fbb257d4728c..e3afe611a78c 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -11829,7 +11829,7 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
> >  	struct intel_crtc_state *pipe_config =
> >  		to_intel_crtc_state(crtc_state);
> >  	struct drm_atomic_state *state = crtc_state->state;
> > -	int ret, idx = crtc->base.id;
> > +	int ret;
> >  	bool mode_changed = needs_modeset(crtc_state);
> >  
> >  	if (mode_changed && !check_encoder_cloning(state, intel_crtc)) {
> > @@ -11837,10 +11837,6 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
> >  		return -EINVAL;
> >  	}
> >  
> > -	I915_STATE_WARN(crtc->state->active != intel_crtc->active,
> > -		"[CRTC:%i] mismatch between state->active(%i) and crtc->active(%i)\n",
> > -		idx, crtc->state->active, intel_crtc->active);
> > -
> >  	if (mode_changed && !crtc_state->active)
> >  		intel_crtc->atomic.update_wm_post = true;
> >  
> > @@ -12721,19 +12717,16 @@ check_encoder_state(struct drm_device *dev)
> >  
> >  	for_each_intel_encoder(dev, encoder) {
> >  		bool enabled = false;
> > -		bool active = false;
> > -		enum pipe pipe, tracked_pipe;
> > +		enum pipe pipe;
> >  
> >  		DRM_DEBUG_KMS("[ENCODER:%d:%s]\n",
> >  			      encoder->base.base.id,
> >  			      encoder->base.name);
> >  
> >  		for_each_intel_connector(dev, connector) {
> > -			if (connector->base.encoder != &encoder->base)
> > +			if (connector->base.state->best_encoder != &encoder->base)
> >  				continue;
> >  			enabled = true;
> > -			if (connector->base.dpms != DRM_MODE_DPMS_OFF)
> > -				active = true;
> >  
> >  			I915_STATE_WARN(connector->base.state->crtc !=
> >  					encoder->base.crtc,
> > @@ -12744,85 +12737,86 @@ check_encoder_state(struct drm_device *dev)
> >  		     "encoder's enabled state mismatch "
> >  		     "(expected %i, found %i)\n",
> >  		     !!encoder->base.crtc, enabled);
> > -		I915_STATE_WARN(active && !encoder->base.crtc,
> > -		     "active encoder with no crtc\n");
> > -
> > -		active = encoder->get_hw_state(encoder, &pipe);
> >  
> >  		if (!encoder->base.crtc) {
> > -			I915_STATE_WARN(active,
> > -			     "encoder detached but not turned off.\n");
> > +			bool active;
> >  
> > -			continue;
> > +			active = encoder->get_hw_state(encoder, &pipe);
> > +			I915_STATE_WARN(active,
> > +			     "encoder detached but still enabled on pipe %c.\n",
> > +			     pipe_name(pipe));
> >  		}
> > -
> > -		I915_STATE_WARN(active != encoder->base.crtc->state->active,
> > -		     "encoder's hw state doesn't match sw tracking "
> > -		     "(expected %i, found %i)\n",
> > -		     encoder->base.crtc->state->active, active);
> > -
> > -
> > -		tracked_pipe = to_intel_crtc(encoder->base.crtc)->pipe;
> > -		I915_STATE_WARN(active && pipe != tracked_pipe,
> > -		     "active encoder's pipe doesn't match"
> > -		     "(expected %i, found %i)\n",
> > -		     tracked_pipe, pipe);
> > -
> >  	}
> >  }
> >  
> >  static void
> > -check_crtc_state(struct drm_device *dev)
> > +check_crtc_state(struct drm_device *dev, struct drm_atomic_state *state)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > -	struct intel_crtc *crtc;
> >  	struct intel_encoder *encoder;
> > -	struct intel_crtc_state pipe_config;
> > +	struct drm_crtc_state *crtc_state;
> > +	struct drm_crtc *crtc;
> > +	int i;
> >  
> > -	for_each_intel_crtc(dev, crtc) {
> > +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> 
> So now we only check the state of crtcs affected by the modeset. In the
> unlikely case of a bug that changes the hw state of another crtc that
> should have been unchanged, the old code might catch the issue but the
> new one doesn't. Isn't it better to just check everything?

We can't since that other crtc might be doing some other async commit. But
eventually we should be doing a modeset on that one too and spot that
something went wrong.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 06/12] drm/i915: Make crtc checking use the atomic state.

[Ander Conselvan De Oliveira]

On Wed, 2015-07-29 at 14:04 +0200, Daniel Vetter wrote:
> On Wed, Jul 29, 2015 at 02:49:45PM +0300, Ander Conselvan De Oliveira 
> wrote:
> > On Mon, 2015-07-27 at 14:35 +0200, Maarten Lankhorst wrote:
> > > Instead of allocating pipe_config on the stack use the old 
> > > crtc_state,
> > > it's only going to freed from this point on.
> > > 
> > > All crtc's encoders are now only checked once during modeset.
> > > 
> > > Signed-off-by: Maarten Lankhorst <
> > > maarten.lankhorst@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 118 +++++++++++++++++----
> > > --------------
> > >  1 file changed, 56 insertions(+), 62 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c 
> > > b/drivers/gpu/drm/i915/intel_display.c
> > > index fbb257d4728c..e3afe611a78c 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -11829,7 +11829,7 @@ static int intel_crtc_atomic_check(struct 
> > > drm_crtc *crtc,
> > >  	struct intel_crtc_state *pipe_config =
> > >  		to_intel_crtc_state(crtc_state);
> > >  	struct drm_atomic_state *state = crtc_state->state;
> > > -	int ret, idx = crtc->base.id;
> > > +	int ret;
> > >  	bool mode_changed = needs_modeset(crtc_state);
> > >  
> > >  	if (mode_changed && !check_encoder_cloning(state, 
> > > intel_crtc)) {
> > > @@ -11837,10 +11837,6 @@ static int 
> > > intel_crtc_atomic_check(struct drm_crtc *crtc,
> > >  		return -EINVAL;
> > >  	}
> > >  
> > > -	I915_STATE_WARN(crtc->state->active != intel_crtc
> > > ->active,
> > > -		"[CRTC:%i] mismatch between state->active(%i) 
> > > and crtc->active(%i)\n",
> > > -		idx, crtc->state->active, intel_crtc->active);
> > > -
> > >  	if (mode_changed && !crtc_state->active)
> > >  		intel_crtc->atomic.update_wm_post = true;
> > >  
> > > @@ -12721,19 +12717,16 @@ check_encoder_state(struct drm_device 
> > > *dev)
> > >  
> > >  	for_each_intel_encoder(dev, encoder) {
> > >  		bool enabled = false;
> > > -		bool active = false;
> > > -		enum pipe pipe, tracked_pipe;
> > > +		enum pipe pipe;
> > >  
> > >  		DRM_DEBUG_KMS("[ENCODER:%d:%s]\n",
> > >  			      encoder->base.base.id,
> > >  			      encoder->base.name);
> > >  
> > >  		for_each_intel_connector(dev, connector) {
> > > -			if (connector->base.encoder != &encoder
> > > ->base)
> > > +			if (connector->base.state->best_encoder 
> > > != &encoder->base)
> > >  				continue;
> > >  			enabled = true;
> > > -			if (connector->base.dpms != 
> > > DRM_MODE_DPMS_OFF)
> > > -				active = true;
> > >  
> > >  			I915_STATE_WARN(connector->base.state
> > > ->crtc !=
> > >  					encoder->base.crtc,
> > > @@ -12744,85 +12737,86 @@ check_encoder_state(struct drm_device 
> > > *dev)
> > >  		     "encoder's enabled state mismatch "
> > >  		     "(expected %i, found %i)\n",
> > >  		     !!encoder->base.crtc, enabled);
> > > -		I915_STATE_WARN(active && !encoder->base.crtc,
> > > -		     "active encoder with no crtc\n");
> > > -
> > > -		active = encoder->get_hw_state(encoder, &pipe);
> > >  
> > >  		if (!encoder->base.crtc) {
> > > -			I915_STATE_WARN(active,
> > > -			     "encoder detached but not turned 
> > > off.\n");
> > > +			bool active;
> > >  
> > > -			continue;
> > > +			active = encoder->get_hw_state(encoder, 
> > > &pipe);
> > > +			I915_STATE_WARN(active,
> > > +			     "encoder detached but still enabled 
> > > on pipe %c.\n",
> > > +			     pipe_name(pipe));
> > >  		}
> > > -
> > > -		I915_STATE_WARN(active != encoder->base.crtc
> > > ->state->active,
> > > -		     "encoder's hw state doesn't match sw 
> > > tracking "
> > > -		     "(expected %i, found %i)\n",
> > > -		     encoder->base.crtc->state->active, active);
> > > -
> > > -
> > > -		tracked_pipe = to_intel_crtc(encoder->base.crtc)
> > > ->pipe;
> > > -		I915_STATE_WARN(active && pipe != tracked_pipe,
> > > -		     "active encoder's pipe doesn't match"
> > > -		     "(expected %i, found %i)\n",
> > > -		     tracked_pipe, pipe);
> > > -
> > >  	}
> > >  }
> > >  
> > >  static void
> > > -check_crtc_state(struct drm_device *dev)
> > > +check_crtc_state(struct drm_device *dev, struct drm_atomic_state 
> > > *state)
> > >  {
> > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > > -	struct intel_crtc *crtc;
> > >  	struct intel_encoder *encoder;
> > > -	struct intel_crtc_state pipe_config;
> > > +	struct drm_crtc_state *crtc_state;
> > > +	struct drm_crtc *crtc;
> > > +	int i;
> > >  
> > > -	for_each_intel_crtc(dev, crtc) {
> > > +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> > 
> > So now we only check the state of crtcs affected by the modeset. In 
> > the
> > unlikely case of a bug that changes the hw state of another crtc 
> > that
> > should have been unchanged, the old code might catch the issue but 
> > the
> > new one doesn't. Isn't it better to just check everything?
> 
> We can't since that other crtc might be doing some other async 
> commit. But
> eventually we should be doing a modeset on that one too and spot that
> something went wrong.

Right, I forgot about async.

Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>

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

Re: [PATCH v2 06/12] drm/i915: Make crtc checking use the atomic state.

[Maarten Lankhorst]

Op 29-07-15 om 14:31 schreef Ander Conselvan De Oliveira:
> On Wed, 2015-07-29 at 14:04 +0200, Daniel Vetter wrote:
>> On Wed, Jul 29, 2015 at 02:49:45PM +0300, Ander Conselvan De Oliveira 
>> wrote:
>>> On Mon, 2015-07-27 at 14:35 +0200, Maarten Lankhorst wrote:
>>>> Instead of allocating pipe_config on the stack use the old 
>>>> crtc_state,
>>>> it's only going to freed from this point on.
>>>>
>>>> All crtc's encoders are now only checked once during modeset.
>>>>
>>>> Signed-off-by: Maarten Lankhorst <
>>>> maarten.lankhorst@linux.intel.com>
>>>> ---
>>>>  drivers/gpu/drm/i915/intel_display.c | 118 +++++++++++++++++----
>>>> --------------
>>>>  1 file changed, 56 insertions(+), 62 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c 
>>>> b/drivers/gpu/drm/i915/intel_display.c
>>>> index fbb257d4728c..e3afe611a78c 100644
>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>> @@ -11829,7 +11829,7 @@ static int intel_crtc_atomic_check(struct 
>>>> drm_crtc *crtc,
>>>>  	struct intel_crtc_state *pipe_config =
>>>>  		to_intel_crtc_state(crtc_state);
>>>>  	struct drm_atomic_state *state = crtc_state->state;
>>>> -	int ret, idx = crtc->base.id;
>>>> +	int ret;
>>>>  	bool mode_changed = needs_modeset(crtc_state);
>>>>  
>>>>  	if (mode_changed && !check_encoder_cloning(state, 
>>>> intel_crtc)) {
>>>> @@ -11837,10 +11837,6 @@ static int 
>>>> intel_crtc_atomic_check(struct drm_crtc *crtc,
>>>>  		return -EINVAL;
>>>>  	}
>>>>  
>>>> -	I915_STATE_WARN(crtc->state->active != intel_crtc
>>>> ->active,
>>>> -		"[CRTC:%i] mismatch between state->active(%i) 
>>>> and crtc->active(%i)\n",
>>>> -		idx, crtc->state->active, intel_crtc->active);
>>>> -
>>>>  	if (mode_changed && !crtc_state->active)
>>>>  		intel_crtc->atomic.update_wm_post = true;
>>>>  
>>>> @@ -12721,19 +12717,16 @@ check_encoder_state(struct drm_device 
>>>> *dev)
>>>>  
>>>>  	for_each_intel_encoder(dev, encoder) {
>>>>  		bool enabled = false;
>>>> -		bool active = false;
>>>> -		enum pipe pipe, tracked_pipe;
>>>> +		enum pipe pipe;
>>>>  
>>>>  		DRM_DEBUG_KMS("[ENCODER:%d:%s]\n",
>>>>  			      encoder->base.base.id,
>>>>  			      encoder->base.name);
>>>>  
>>>>  		for_each_intel_connector(dev, connector) {
>>>> -			if (connector->base.encoder != &encoder
>>>> ->base)
>>>> +			if (connector->base.state->best_encoder 
>>>> != &encoder->base)
>>>>  				continue;
>>>>  			enabled = true;
>>>> -			if (connector->base.dpms != 
>>>> DRM_MODE_DPMS_OFF)
>>>> -				active = true;
>>>>  
>>>>  			I915_STATE_WARN(connector->base.state
>>>> ->crtc !=
>>>>  					encoder->base.crtc,
>>>> @@ -12744,85 +12737,86 @@ check_encoder_state(struct drm_device 
>>>> *dev)
>>>>  		     "encoder's enabled state mismatch "
>>>>  		     "(expected %i, found %i)\n",
>>>>  		     !!encoder->base.crtc, enabled);
>>>> -		I915_STATE_WARN(active && !encoder->base.crtc,
>>>> -		     "active encoder with no crtc\n");
>>>> -
>>>> -		active = encoder->get_hw_state(encoder, &pipe);
>>>>  
>>>>  		if (!encoder->base.crtc) {
>>>> -			I915_STATE_WARN(active,
>>>> -			     "encoder detached but not turned 
>>>> off.\n");
>>>> +			bool active;
>>>>  
>>>> -			continue;
>>>> +			active = encoder->get_hw_state(encoder, 
>>>> &pipe);
>>>> +			I915_STATE_WARN(active,
>>>> +			     "encoder detached but still enabled 
>>>> on pipe %c.\n",
>>>> +			     pipe_name(pipe));
>>>>  		}
>>>> -
>>>> -		I915_STATE_WARN(active != encoder->base.crtc
>>>> ->state->active,
>>>> -		     "encoder's hw state doesn't match sw 
>>>> tracking "
>>>> -		     "(expected %i, found %i)\n",
>>>> -		     encoder->base.crtc->state->active, active);
>>>> -
>>>> -
>>>> -		tracked_pipe = to_intel_crtc(encoder->base.crtc)
>>>> ->pipe;
>>>> -		I915_STATE_WARN(active && pipe != tracked_pipe,
>>>> -		     "active encoder's pipe doesn't match"
>>>> -		     "(expected %i, found %i)\n",
>>>> -		     tracked_pipe, pipe);
>>>> -
>>>>  	}
>>>>  }
>>>>  
>>>>  static void
>>>> -check_crtc_state(struct drm_device *dev)
>>>> +check_crtc_state(struct drm_device *dev, struct drm_atomic_state 
>>>> *state)
>>>>  {
>>>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>>>> -	struct intel_crtc *crtc;
>>>>  	struct intel_encoder *encoder;
>>>> -	struct intel_crtc_state pipe_config;
>>>> +	struct drm_crtc_state *crtc_state;
>>>> +	struct drm_crtc *crtc;
>>>> +	int i;
>>>>  
>>>> -	for_each_intel_crtc(dev, crtc) {
>>>> +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
>>> So now we only check the state of crtcs affected by the modeset. In 
>>> the
>>> unlikely case of a bug that changes the hw state of another crtc 
>>> that
>>> should have been unchanged, the old code might catch the issue but 
>>> the
>>> new one doesn't. Isn't it better to just check everything?
>> We can't since that other crtc might be doing some other async 
>> commit. But
>> eventually we should be doing a modeset on that one too and spot that
>> something went wrong.
> Right, I forgot about async.
>
> Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>
async is not the only issue, crtc->state may disappear underneath too. :-)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 08/12] drm/i915: Remove connectors_active from sanitization.

[Ander Conselvan De Oliveira]

On Mon, 2015-07-27 at 14:35 +0200, Maarten Lankhorst wrote:
> connectors_active will be removed, so just calculate this right here.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index ed9eba2666e2..341fadb40c81 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14935,8 +14935,10 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>  	/* 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, encoder)
> -		enable |= encoder->connectors_active;
> +	for_each_encoder_on_crtc(dev, &crtc->base, encoder) {
> +		enable = true;
> +		break;
> +	}
>  
>  	if (!enable)
>  		intel_crtc_disable_noatomic(&crtc->base);
> @@ -14992,6 +14994,7 @@ static void intel_sanitize_encoder(struct intel_encoder *encoder)
>  {
>  	struct intel_connector *connector;
>  	struct drm_device *dev = encoder->base.dev;
> +	bool active = false;
>  
>  	/* We need to check both for a crtc link (meaning that the
>  	 * encoder is active and trying to read from a pipe) and the
> @@ -14999,7 +15002,15 @@ static void intel_sanitize_encoder(struct intel_encoder *encoder)
>  	bool has_active_crtc = encoder->base.crtc &&
>  		to_intel_crtc(encoder->base.crtc)->active;
>  
> -	if (encoder->connectors_active && !has_active_crtc) {
> +	for_each_intel_connector(dev, connector) {
> +		if (connector->encoder != encoder)

Shouldn't this be connector->base.encoder?

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

Re: [PATCH v2 09/12] drm/i915: Remove connectors_active from intel_dp.c.

[Ander Conselvan De Oliveira]

On Mon, 2015-07-27 at 14:35 +0200, Maarten Lankhorst wrote:
> Now that everything's atomic, checking encoder->base.crtc is enough.

Don't you need to check encoder->base.crtc->state->active too?

> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index cea7d1785d13..dccd9a0e2526 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -2624,7 +2624,7 @@ static void vlv_steal_power_sequencer(struct drm_device *dev,
>  		DRM_DEBUG_KMS("stealing pipe %c power sequencer from port %c\n",
>  			      pipe_name(pipe), port_name(port));
>  
> -		WARN(encoder->connectors_active,
> +		WARN(encoder->base.crtc,
>  		     "stealing pipe %c power sequencer from active eDP port %c\n",
>  		     pipe_name(pipe), port_name(port));
>  
> @@ -4240,10 +4240,7 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>  
>  	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
>  
> -	if (!intel_encoder->connectors_active)
> -		return;
> -
> -	if (WARN_ON(!intel_encoder->base.crtc))
> +	if (!intel_encoder->base.crtc)
>  		return;
>  
>  	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 09/12] drm/i915: Remove connectors_active from intel_dp.c.

[Maarten Lankhorst]

Op 29-07-15 om 15:26 schreef Ander Conselvan De Oliveira:
> On Mon, 2015-07-27 at 14:35 +0200, Maarten Lankhorst wrote:
>> Now that everything's atomic, checking encoder->base.crtc is enough.
> Don't you need to check encoder->base.crtc->state->active too?
Not sure, I think stealing a encoder being bound to any crtc is probably good enough reason to warn.
We probably don't hold the right lock to dereference crtc->state.

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

Re: [PATCH v2 08/12] drm/i915: Remove connectors_active from sanitization.

[Maarten Lankhorst]

Op 29-07-15 om 15:09 schreef Ander Conselvan De Oliveira:
> On Mon, 2015-07-27 at 14:35 +0200, Maarten Lankhorst wrote:
>> connectors_active will be removed, so just calculate this right here.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 17 ++++++++++++++---
>>  1 file changed, 14 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index ed9eba2666e2..341fadb40c81 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -14935,8 +14935,10 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>>  	/* 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, encoder)
>> -		enable |= encoder->connectors_active;
>> +	for_each_encoder_on_crtc(dev, &crtc->base, encoder) {
>> +		enable = true;
>> +		break;
>> +	}
>>  
>>  	if (!enable)
>>  		intel_crtc_disable_noatomic(&crtc->base);
>> @@ -14992,6 +14994,7 @@ static void intel_sanitize_encoder(struct intel_encoder *encoder)
>>  {
>>  	struct intel_connector *connector;
>>  	struct drm_device *dev = encoder->base.dev;
>> +	bool active = false;
>>  
>>  	/* We need to check both for a crtc link (meaning that the
>>  	 * encoder is active and trying to read from a pipe) and the
>> @@ -14999,7 +15002,15 @@ static void intel_sanitize_encoder(struct intel_encoder *encoder)
>>  	bool has_active_crtc = encoder->base.crtc &&
>>  		to_intel_crtc(encoder->base.crtc)->active;
>>  
>> -	if (encoder->connectors_active && !has_active_crtc) {
>> +	for_each_intel_connector(dev, connector) {
>> +		if (connector->encoder != encoder)
> Shouldn't this be connector->base.encoder?
>
> Ander
Well ideally connector->state->best_encoder, but yeah looks like we haven't set that up here yet, so connector->base.encoder is probably the right member to check..

~Maarten

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

Re: [PATCH v2 09/12] drm/i915: Remove connectors_active from intel_dp.c.

[Ander Conselvan De Oliveira]

On Thu, 2015-07-30 at 08:54 +0200, Maarten Lankhorst wrote:
> Op 29-07-15 om 15:26 schreef Ander Conselvan De Oliveira:
> > On Mon, 2015-07-27 at 14:35 +0200, Maarten Lankhorst wrote:
> > > Now that everything's atomic, checking encoder->base.crtc is enough.
> > Don't you need to check encoder->base.crtc->state->active too?
> Not sure, I think stealing a encoder being bound to any crtc is probably good enough reason to 
> warn.
> We probably don't hold the right lock to dereference crtc->state.

Fair enough. Add that to the commit message and you can add

Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>

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

Re: [PATCH v2 11/12] drm/i915: Only update mode related state if a modeset happened.

[Ander Conselvan De Oliveira]

Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>

On Mon, 2015-07-27 at 14:35 +0200, Maarten Lankhorst wrote:
> The rest will be a noop anyway, since without modeset there will be
> no updated dplls and no modeset state to update.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 30 +++++++-----------------------
>  1 file changed, 7 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 23175ce6583d..280629911d41 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12181,33 +12181,15 @@ fail:
>  	return ret;
>  }
>  
> -static bool intel_crtc_in_use(struct drm_crtc *crtc)
> -{
> -	struct drm_encoder *encoder;
> -	struct drm_device *dev = crtc->dev;
> -
> -	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head)
> -		if (encoder->crtc == crtc)
> -			return true;
> -
> -	return false;
> -}
> -
>  static void
> -intel_modeset_update_state(struct drm_atomic_state *state)
> +intel_modeset_update_crtc_state(struct drm_atomic_state *state)
>  {
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *crtc_state;
>  	int i;
>  
> -	intel_shared_dpll_commit(state);
> -
> -	drm_atomic_helper_update_legacy_modeset_state(state->dev, state);
> -
>  	/* Double check state. */
>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> -		WARN_ON(crtc->state->enable != intel_crtc_in_use(crtc));
> -
>  		to_intel_crtc(crtc)->config = to_intel_crtc_state(crtc->state);
>  
>  		/* Update hwmode for vblank functions */
> @@ -13109,12 +13091,14 @@ static int intel_atomic_commit(struct drm_device *dev,
>  
>  	/* Only after disabling all output pipelines that will be changed can we
>  	 * update the the output configuration. */
> -	intel_modeset_update_state(state);
> +	intel_modeset_update_crtc_state(state);
>  
> -	/* The state has been swaped above, so state actually contains the
> -	 * old state now. */
> -	if (any_ms)
> +	if (any_ms) {
> +		intel_shared_dpll_commit(state);
> +
> +		drm_atomic_helper_update_legacy_modeset_state(state->dev, state);
>  		modeset_update_crtc_power_domains(state);
> +	}
>  
>  	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2.1 2.5/12] drm/i915: Validate the state after an atomic modeset, only, and pass the state.

[Maarten Lankhorst]

First step in removing dpms and validating atomic state.

There can still be a mismatch in the connector state because the dpms
callbacks are still used, but this can not happen immediately after a modeset.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_crt.c     |  2 --
 drivers/gpu/drm/i915/intel_display.c | 12 ++++++------
 drivers/gpu/drm/i915/intel_drv.h     |  1 -
 drivers/gpu/drm/i915/intel_dvo.c     |  2 --
 drivers/gpu/drm/i915/intel_sdvo.c    |  2 --
 5 files changed, 6 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index 5d78c1feec81..9eba3dd5b434 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -280,8 +280,6 @@ static int intel_crt_dpms(struct drm_connector *connector, int mode)
 		intel_crtc_update_dpms(crtc);
 	}
 
-	intel_modeset_check_state(connector->dev);
-
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index dfe4407b0390..77b4da7e698c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6440,8 +6440,6 @@ int intel_connector_dpms(struct drm_connector *connector, int mode)
 	if (connector->encoder)
 		intel_encoder_dpms(to_intel_encoder(connector->encoder), mode);
 
-	intel_modeset_check_state(connector->dev);
-
 	return 0;
 }
 
@@ -12900,8 +12898,9 @@ check_shared_dpll_state(struct drm_device *dev)
 	}
 }
 
-void
-intel_modeset_check_state(struct drm_device *dev)
+static void
+intel_modeset_check_state(struct drm_device *dev,
+			  struct drm_atomic_state *old_state)
 {
 	check_wm_state(dev);
 	check_connector_state(dev);
@@ -13290,10 +13289,11 @@ static int intel_atomic_commit(struct drm_device *dev,
 
 	drm_atomic_helper_wait_for_vblanks(dev, state);
 	drm_atomic_helper_cleanup_planes(dev, state);
-	drm_atomic_state_free(state);
 
 	if (any_ms)
-		intel_modeset_check_state(dev);
+		intel_modeset_check_state(dev, state);
+
+	drm_atomic_state_free(state);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 320c9e6bd848..0da4236dc85a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -999,7 +999,6 @@ int intel_connector_init(struct intel_connector *);
 struct intel_connector *intel_connector_alloc(void);
 int intel_connector_dpms(struct drm_connector *, int mode);
 bool intel_connector_get_hw_state(struct intel_connector *connector);
-void intel_modeset_check_state(struct drm_device *dev);
 bool ibx_digital_port_connected(struct drm_i915_private *dev_priv,
 				struct intel_digital_port *port);
 void intel_connector_attach_encoder(struct intel_connector *connector,
diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c
index fd5e522abebb..600f7fb855d8 100644
--- a/drivers/gpu/drm/i915/intel_dvo.c
+++ b/drivers/gpu/drm/i915/intel_dvo.c
@@ -237,8 +237,6 @@ static int intel_dvo_dpms(struct drm_connector *connector, int mode)
 		intel_crtc_update_dpms(crtc);
 	}
 
-	intel_modeset_check_state(connector->dev);
-
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index 2c435a79d4da..8911e0e417ee 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -1550,8 +1550,6 @@ static int intel_sdvo_dpms(struct drm_connector *connector, int mode)
 		intel_sdvo_set_active_outputs(intel_sdvo, intel_sdvo->attached_output);
 	}
 
-	intel_modeset_check_state(connector->dev);
-
 	return 0;
 }
 
-- 
2.1.0


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

[PATCH v2.1 03/12] drm/i915: Convert connector checking to atomic, v2.

[Maarten Lankhorst]

Right now dpms callbacks can still fiddle with the connector state,
but it can only turn connectors off.

This is remediated by only checking crtc->state->active when the
connector is active, and ignore crtc->state->active when the
connector is off.

connectors_active is no longer checked, and will be removed later
in this series together with dpms.

Another check for !encoder->crtc is performed by check_encoder_state
too, so it can be removed.

Changes since v1:
- Add commit message.
- rename state to old_state.
- Move deletion of mst_port check to mst patch.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 66 +++++++++++++++++-------------------
 1 file changed, 32 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 77b4da7e698c..876071ad4681 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6360,38 +6360,33 @@ static void intel_encoder_dpms(struct intel_encoder *encoder, int mode)
  * internal consistency). */
 static void intel_connector_check_state(struct intel_connector *connector)
 {
-	if (connector->get_hw_state(connector)) {
-		struct intel_encoder *encoder = connector->encoder;
-		struct drm_crtc *crtc;
-		bool encoder_enabled;
-		enum pipe pipe;
+	struct drm_crtc *crtc = connector->base.state->crtc;
 
-		DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
-			      connector->base.base.id,
-			      connector->base.name);
+	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
+		      connector->base.base.id,
+		      connector->base.name);
 
-		I915_STATE_WARN(connector->base.dpms == DRM_MODE_DPMS_OFF,
-		     "wrong connector dpms state\n");
-		I915_STATE_WARN(connector->base.encoder != &encoder->base,
-		     "active connector not linked to encoder\n");
+	if (connector->get_hw_state(connector)) {
+		struct drm_encoder *encoder = &connector->encoder->base;
+		struct drm_connector_state *conn_state = connector->base.state;
 
-		if (encoder) {
-			I915_STATE_WARN(!encoder->connectors_active,
-			     "encoder->connectors_active not set\n");
+		I915_STATE_WARN(!crtc,
+			 "connector enabled without attached crtc\n");
 
-			encoder_enabled = encoder->get_hw_state(encoder, &pipe);
-			I915_STATE_WARN(!encoder_enabled, "encoder not enabled\n");
-			if (I915_STATE_WARN_ON(!encoder->base.crtc))
-				return;
+		if (!crtc)
+			return;
 
-			crtc = encoder->base.crtc;
+		I915_STATE_WARN(!crtc->state->active,
+		      "connector is active, but attached crtc isn't\n");
 
-			I915_STATE_WARN(!crtc->state->enable,
-					"crtc not enabled\n");
-			I915_STATE_WARN(!to_intel_crtc(crtc)->active, "crtc not active\n");
-			I915_STATE_WARN(pipe != to_intel_crtc(crtc)->pipe,
-			     "encoder active on the wrong pipe\n");
-		}
+		I915_STATE_WARN(conn_state->best_encoder != encoder,
+			"atomic encoder doesn't match attached encoder\n");
+
+		I915_STATE_WARN(conn_state->crtc != encoder->crtc,
+			"attached encoder crtc differs from connector crtc\n");
+	} else {
+		I915_STATE_WARN(!crtc && connector->base.state->best_encoder,
+			"best encoder set without crtc!\n");
 	}
 }
 
@@ -12699,20 +12694,23 @@ static void check_wm_state(struct drm_device *dev)
 }
 
 static void
-check_connector_state(struct drm_device *dev)
+check_connector_state(struct drm_device *dev,
+		      struct drm_atomic_state *old_state)
 {
-	struct intel_connector *connector;
+	struct drm_connector_state *old_conn_state;
+	struct drm_connector *connector;
+	int i;
 
-	for_each_intel_connector(dev, connector) {
-		struct drm_encoder *encoder = connector->base.encoder;
-		struct drm_connector_state *state = connector->base.state;
+	for_each_connector_in_state(old_state, connector, old_conn_state, i) {
+		struct drm_encoder *encoder = connector->encoder;
+		struct drm_connector_state *state = connector->state;
 
 		/* This also checks the encoder/connector hw state with the
 		 * ->get_hw_state callbacks. */
-		intel_connector_check_state(connector);
+		intel_connector_check_state(to_intel_connector(connector));
 
 		I915_STATE_WARN(state->best_encoder != encoder,
-		     "connector's staged encoder doesn't match current encoder\n");
+		     "connector's atomic encoder doesn't match legacy encoder\n");
 	}
 }
 
@@ -12903,7 +12901,7 @@ intel_modeset_check_state(struct drm_device *dev,
 			  struct drm_atomic_state *old_state)
 {
 	check_wm_state(dev);
-	check_connector_state(dev);
+	check_connector_state(dev, old_state);
 	check_encoder_state(dev);
 	check_crtc_state(dev);
 	check_shared_dpll_state(dev);
-- 
2.1.0


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

[PATCH v2.1 06/12] drm/i915: Make crtc checking use the atomic state, v2.

[Maarten Lankhorst]

Instead of allocating pipe_config on the stack use the old
crtc_state, it's only going to freed from this point on.

All crtc' are now only checked once during modeset,
because false positives can happen with encoders after
dpms changes and to limit the amount of errors for 1 failure.

Changes since v1:
- crtc_state -> old_crtc_state
- state -> old_state

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>
---
 drivers/gpu/drm/i915/intel_display.c | 118 +++++++++++++++++------------------
 1 file changed, 56 insertions(+), 62 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 129931f3013e..4babecd1c99f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11829,7 +11829,7 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
 	struct intel_crtc_state *pipe_config =
 		to_intel_crtc_state(crtc_state);
 	struct drm_atomic_state *state = crtc_state->state;
-	int ret, idx = crtc->base.id;
+	int ret;
 	bool mode_changed = needs_modeset(crtc_state);
 
 	if (mode_changed && !check_encoder_cloning(state, intel_crtc)) {
@@ -11837,10 +11837,6 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
 		return -EINVAL;
 	}
 
-	I915_STATE_WARN(crtc->state->active != intel_crtc->active,
-		"[CRTC:%i] mismatch between state->active(%i) and crtc->active(%i)\n",
-		idx, crtc->state->active, intel_crtc->active);
-
 	if (mode_changed && !crtc_state->active)
 		intel_crtc->atomic.update_wm_post = true;
 
@@ -12722,19 +12718,16 @@ check_encoder_state(struct drm_device *dev)
 
 	for_each_intel_encoder(dev, encoder) {
 		bool enabled = false;
-		bool active = false;
-		enum pipe pipe, tracked_pipe;
+		enum pipe pipe;
 
 		DRM_DEBUG_KMS("[ENCODER:%d:%s]\n",
 			      encoder->base.base.id,
 			      encoder->base.name);
 
 		for_each_intel_connector(dev, connector) {
-			if (connector->base.encoder != &encoder->base)
+			if (connector->base.state->best_encoder != &encoder->base)
 				continue;
 			enabled = true;
-			if (connector->base.dpms != DRM_MODE_DPMS_OFF)
-				active = true;
 
 			I915_STATE_WARN(connector->base.state->crtc !=
 					encoder->base.crtc,
@@ -12745,85 +12738,86 @@ check_encoder_state(struct drm_device *dev)
 		     "encoder's enabled state mismatch "
 		     "(expected %i, found %i)\n",
 		     !!encoder->base.crtc, enabled);
-		I915_STATE_WARN(active && !encoder->base.crtc,
-		     "active encoder with no crtc\n");
-
-		active = encoder->get_hw_state(encoder, &pipe);
 
 		if (!encoder->base.crtc) {
-			I915_STATE_WARN(active,
-			     "encoder detached but not turned off.\n");
+			bool active;
 
-			continue;
+			active = encoder->get_hw_state(encoder, &pipe);
+			I915_STATE_WARN(active,
+			     "encoder detached but still enabled on pipe %c.\n",
+			     pipe_name(pipe));
 		}
-
-		I915_STATE_WARN(active != encoder->base.crtc->state->active,
-		     "encoder's hw state doesn't match sw tracking "
-		     "(expected %i, found %i)\n",
-		     encoder->base.crtc->state->active, active);
-
-
-		tracked_pipe = to_intel_crtc(encoder->base.crtc)->pipe;
-		I915_STATE_WARN(active && pipe != tracked_pipe,
-		     "active encoder's pipe doesn't match"
-		     "(expected %i, found %i)\n",
-		     tracked_pipe, pipe);
-
 	}
 }
 
 static void
-check_crtc_state(struct drm_device *dev)
+check_crtc_state(struct drm_device *dev, struct drm_atomic_state *old_state)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_crtc *crtc;
 	struct intel_encoder *encoder;
-	struct intel_crtc_state pipe_config;
+	struct drm_crtc_state *old_crtc_state;
+	struct drm_crtc *crtc;
+	int i;
 
-	for_each_intel_crtc(dev, crtc) {
+	for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) {
+		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+		struct intel_crtc_state *pipe_config, *sw_config;
 		bool active;
 
-		memset(&pipe_config, 0, sizeof(pipe_config));
+		if (!needs_modeset(crtc->state))
+			continue;
 
-		DRM_DEBUG_KMS("[CRTC:%d]\n",
-			      crtc->base.base.id);
+		__drm_atomic_helper_crtc_destroy_state(crtc, old_crtc_state);
+		pipe_config = to_intel_crtc_state(old_crtc_state);
+		memset(pipe_config, 0, sizeof(*pipe_config));
+		pipe_config->base.crtc = crtc;
+		pipe_config->base.state = old_state;
 
-		I915_STATE_WARN(crtc->active && !crtc->base.state->enable,
-		     "active crtc, but not enabled in sw tracking\n");
+		DRM_DEBUG_KMS("[CRTC:%d]\n",
+			      crtc->base.id);
 
-		active = dev_priv->display.get_pipe_config(crtc,
-							   &pipe_config);
+		active = dev_priv->display.get_pipe_config(intel_crtc,
+							   pipe_config);
 
 		/* hw state is inconsistent with the pipe quirk */
-		if ((crtc->pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE) ||
-		    (crtc->pipe == PIPE_B && dev_priv->quirks & QUIRK_PIPEB_FORCE))
-			active = crtc->active;
-
-		for_each_intel_encoder(dev, encoder) {
-			enum pipe pipe;
-			if (encoder->base.crtc != &crtc->base)
-				continue;
-			if (encoder->get_hw_state(encoder, &pipe))
-				encoder->get_config(encoder, &pipe_config);
-		}
+		if ((intel_crtc->pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE) ||
+		    (intel_crtc->pipe == PIPE_B && dev_priv->quirks & QUIRK_PIPEB_FORCE))
+			active = crtc->state->active;
 
-		I915_STATE_WARN(crtc->active != active,
+		I915_STATE_WARN(crtc->state->active != active,
 		     "crtc active state doesn't match with hw state "
-		     "(expected %i, found %i)\n", crtc->active, active);
+		     "(expected %i, found %i)\n", crtc->state->active, active);
 
-		I915_STATE_WARN(crtc->active != crtc->base.state->active,
+		I915_STATE_WARN(intel_crtc->active != crtc->state->active,
 		     "transitional active state does not match atomic hw state "
-		     "(expected %i, found %i)\n", crtc->base.state->active, crtc->active);
+		     "(expected %i, found %i)\n", crtc->state->active, intel_crtc->active);
+
+		for_each_encoder_on_crtc(dev, crtc, encoder) {
+			enum pipe pipe;
+
+			active = encoder->get_hw_state(encoder, &pipe);
+			I915_STATE_WARN(active != crtc->state->active,
+				"[ENCODER:%i] active %i with crtc active %i\n",
+				encoder->base.base.id, active, crtc->state->active);
+
+			I915_STATE_WARN(active && intel_crtc->pipe != pipe,
+					"Encoder connected to wrong pipe %c\n",
+					pipe_name(pipe));
+
+			if (active)
+				encoder->get_config(encoder, pipe_config);
+		}
 
-		if (!active)
+		if (!crtc->state->active)
 			continue;
 
-		if (!intel_pipe_config_compare(dev, crtc->config,
-					       &pipe_config, false)) {
+		sw_config = to_intel_crtc_state(crtc->state);
+		if (!intel_pipe_config_compare(dev, sw_config,
+					       pipe_config, false)) {
 			I915_STATE_WARN(1, "pipe state doesn't match!\n");
-			intel_dump_pipe_config(crtc, &pipe_config,
+			intel_dump_pipe_config(intel_crtc, pipe_config,
 					       "[hw state]");
-			intel_dump_pipe_config(crtc, crtc->config,
+			intel_dump_pipe_config(intel_crtc, sw_config,
 					       "[sw state]");
 		}
 	}
@@ -12885,7 +12879,7 @@ intel_modeset_check_state(struct drm_device *dev,
 	check_wm_state(dev);
 	check_connector_state(dev, old_state);
 	check_encoder_state(dev);
-	check_crtc_state(dev);
+	check_crtc_state(dev, old_state);
 	check_shared_dpll_state(dev);
 }
 
-- 
2.1.0


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

[PATCH v2.1 08/12] drm/i915: Remove connectors_active from sanitization, v2.

[Maarten Lankhorst]

connectors_active will be removed, so just calculate this instead.

Changes since v1:
- Look for the right pointer in intel_sanitize_encoder.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>
---
 drivers/gpu/drm/i915/intel_display.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 4837d14a5f8e..ff0a22f223e6 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14936,8 +14936,10 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
 	/* 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, encoder)
-		enable |= encoder->connectors_active;
+	for_each_encoder_on_crtc(dev, &crtc->base, encoder) {
+		enable = true;
+		break;
+	}
 
 	if (!enable)
 		intel_crtc_disable_noatomic(&crtc->base);
@@ -14993,6 +14995,7 @@ static void intel_sanitize_encoder(struct intel_encoder *encoder)
 {
 	struct intel_connector *connector;
 	struct drm_device *dev = encoder->base.dev;
+	bool active = false;
 
 	/* We need to check both for a crtc link (meaning that the
 	 * encoder is active and trying to read from a pipe) and the
@@ -15000,7 +15003,15 @@ static void intel_sanitize_encoder(struct intel_encoder *encoder)
 	bool has_active_crtc = encoder->base.crtc &&
 		to_intel_crtc(encoder->base.crtc)->active;
 
-	if (encoder->connectors_active && !has_active_crtc) {
+	for_each_intel_connector(dev, connector) {
+		if (connector->base.encoder != &encoder->base)
+			continue;
+
+		active = true;
+		break;
+	}
+
+	if (active && !has_active_crtc) {
 		DRM_DEBUG_KMS("[ENCODER:%d:%s] has active connectors but no active pipe!\n",
 			      encoder->base.base.id,
 			      encoder->base.name);
-- 
2.1.0


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

[PATCH v2.1 09/12] drm/i915: Remove connectors_active from intel_dp.c, v2.

[Maarten Lankhorst]

Now that everything's atomic, checking encoder->base.crtc is enough.
This function doesn't have the locks to dereference crtc->state, but
stealing an encoder bound to any crtc is probably enough reason to warn.

Changes since v1:
- Commit message.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 3c243dee72c2..da347a8996a6 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2624,7 +2624,7 @@ static void vlv_steal_power_sequencer(struct drm_device *dev,
 		DRM_DEBUG_KMS("stealing pipe %c power sequencer from port %c\n",
 			      pipe_name(pipe), port_name(port));
 
-		WARN(encoder->connectors_active,
+		WARN(encoder->base.crtc,
 		     "stealing pipe %c power sequencer from active eDP port %c\n",
 		     pipe_name(pipe), port_name(port));
 
@@ -4241,10 +4241,7 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 
 	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
 
-	if (!intel_encoder->connectors_active)
-		return;
-
-	if (WARN_ON(!intel_encoder->base.crtc))
+	if (!intel_encoder->base.crtc)
 		return;
 
 	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
-- 
2.1.0


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

Re: [PATCH v2.1 2.5/12] drm/i915: Validate the state after an atomic modeset, only, and pass the state.

[Ander Conselvan De Oliveira]

Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>

On Thu, 2015-07-30 at 14:57 +0200, Maarten Lankhorst wrote:
> First step in removing dpms and validating atomic state.
> 
> There can still be a mismatch in the connector state because the dpms
> callbacks are still used, but this can not happen immediately after a modeset.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_crt.c     |  2 --
>  drivers/gpu/drm/i915/intel_display.c | 12 ++++++------
>  drivers/gpu/drm/i915/intel_drv.h     |  1 -
>  drivers/gpu/drm/i915/intel_dvo.c     |  2 --
>  drivers/gpu/drm/i915/intel_sdvo.c    |  2 --
>  5 files changed, 6 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
> index 5d78c1feec81..9eba3dd5b434 100644
> --- a/drivers/gpu/drm/i915/intel_crt.c
> +++ b/drivers/gpu/drm/i915/intel_crt.c
> @@ -280,8 +280,6 @@ static int intel_crt_dpms(struct drm_connector *connector, int mode)
>  		intel_crtc_update_dpms(crtc);
>  	}
>  
> -	intel_modeset_check_state(connector->dev);
> -
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index dfe4407b0390..77b4da7e698c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6440,8 +6440,6 @@ int intel_connector_dpms(struct drm_connector *connector, int mode)
>  	if (connector->encoder)
>  		intel_encoder_dpms(to_intel_encoder(connector->encoder), mode);
>  
> -	intel_modeset_check_state(connector->dev);
> -
>  	return 0;
>  }
>  
> @@ -12900,8 +12898,9 @@ check_shared_dpll_state(struct drm_device *dev)
>  	}
>  }
>  
> -void
> -intel_modeset_check_state(struct drm_device *dev)
> +static void
> +intel_modeset_check_state(struct drm_device *dev,
> +			  struct drm_atomic_state *old_state)
>  {
>  	check_wm_state(dev);
>  	check_connector_state(dev);
> @@ -13290,10 +13289,11 @@ static int intel_atomic_commit(struct drm_device *dev,
>  
>  	drm_atomic_helper_wait_for_vblanks(dev, state);
>  	drm_atomic_helper_cleanup_planes(dev, state);
> -	drm_atomic_state_free(state);
>  
>  	if (any_ms)
> -		intel_modeset_check_state(dev);
> +		intel_modeset_check_state(dev, state);
> +
> +	drm_atomic_state_free(state);
>  
>  	return 0;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 320c9e6bd848..0da4236dc85a 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -999,7 +999,6 @@ int intel_connector_init(struct intel_connector *);
>  struct intel_connector *intel_connector_alloc(void);
>  int intel_connector_dpms(struct drm_connector *, int mode);
>  bool intel_connector_get_hw_state(struct intel_connector *connector);
> -void intel_modeset_check_state(struct drm_device *dev);
>  bool ibx_digital_port_connected(struct drm_i915_private *dev_priv,
>  				struct intel_digital_port *port);
>  void intel_connector_attach_encoder(struct intel_connector *connector,
> diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c
> index fd5e522abebb..600f7fb855d8 100644
> --- a/drivers/gpu/drm/i915/intel_dvo.c
> +++ b/drivers/gpu/drm/i915/intel_dvo.c
> @@ -237,8 +237,6 @@ static int intel_dvo_dpms(struct drm_connector *connector, int mode)
>  		intel_crtc_update_dpms(crtc);
>  	}
>  
> -	intel_modeset_check_state(connector->dev);
> -
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> index 2c435a79d4da..8911e0e417ee 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -1550,8 +1550,6 @@ static int intel_sdvo_dpms(struct drm_connector *connector, int mode)
>  		intel_sdvo_set_active_outputs(intel_sdvo, intel_sdvo->attached_output);
>  	}
>  
> -	intel_modeset_check_state(connector->dev);
> -
>  	return 0;
>  }
>  
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2.1 03/12] drm/i915: Convert connector checking to atomic, v2.

[Ander Conselvan De Oliveira]

On Thu, 2015-07-30 at 14:57 +0200, Maarten Lankhorst wrote:
> Right now dpms callbacks can still fiddle with the connector state,
> but it can only turn connectors off.
> 
> This is remediated by only checking crtc->state->active when the
> connector is active, and ignore crtc->state->active when the
> connector is off.
> 
> connectors_active is no longer checked, and will be removed later
> in this series together with dpms.
> 
> Another check for !encoder->crtc is performed by check_encoder_state
> too, so it can be removed.
> 
> Changes since v1:
> - Add commit message.
> - rename state to old_state.
> - Move deletion of mst_port check to mst patch.

I didn't see the respin of that patch, so this one didn't apply cleanly for me. I had to edit the
mst patch first.

Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>

> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 66 +++++++++++++++++-------------------
>  1 file changed, 32 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 77b4da7e698c..876071ad4681 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6360,38 +6360,33 @@ static void intel_encoder_dpms(struct intel_encoder *encoder, int mode)
>   * internal consistency). */
>  static void intel_connector_check_state(struct intel_connector *connector)
>  {
> -	if (connector->get_hw_state(connector)) {
> -		struct intel_encoder *encoder = connector->encoder;
> -		struct drm_crtc *crtc;
> -		bool encoder_enabled;
> -		enum pipe pipe;
> +	struct drm_crtc *crtc = connector->base.state->crtc;
>  
> -		DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> -			      connector->base.base.id,
> -			      connector->base.name);
> +	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> +		      connector->base.base.id,
> +		      connector->base.name);
>  
> -		I915_STATE_WARN(connector->base.dpms == DRM_MODE_DPMS_OFF,
> -		     "wrong connector dpms state\n");
> -		I915_STATE_WARN(connector->base.encoder != &encoder->base,
> -		     "active connector not linked to encoder\n");
> +	if (connector->get_hw_state(connector)) {
> +		struct drm_encoder *encoder = &connector->encoder->base;
> +		struct drm_connector_state *conn_state = connector->base.state;
>  
> -		if (encoder) {
> -			I915_STATE_WARN(!encoder->connectors_active,
> -			     "encoder->connectors_active not set\n");
> +		I915_STATE_WARN(!crtc,
> +			 "connector enabled without attached crtc\n");
>  
> -			encoder_enabled = encoder->get_hw_state(encoder, &pipe);
> -			I915_STATE_WARN(!encoder_enabled, "encoder not enabled\n");
> -			if (I915_STATE_WARN_ON(!encoder->base.crtc))
> -				return;
> +		if (!crtc)
> +			return;
>  
> -			crtc = encoder->base.crtc;
> +		I915_STATE_WARN(!crtc->state->active,
> +		      "connector is active, but attached crtc isn't\n");
>  
> -			I915_STATE_WARN(!crtc->state->enable,
> -					"crtc not enabled\n");
> -			I915_STATE_WARN(!to_intel_crtc(crtc)->active, "crtc not active\n");
> -			I915_STATE_WARN(pipe != to_intel_crtc(crtc)->pipe,
> -			     "encoder active on the wrong pipe\n");
> -		}
> +		I915_STATE_WARN(conn_state->best_encoder != encoder,
> +			"atomic encoder doesn't match attached encoder\n");
> +
> +		I915_STATE_WARN(conn_state->crtc != encoder->crtc,
> +			"attached encoder crtc differs from connector crtc\n");
> +	} else {
> +		I915_STATE_WARN(!crtc && connector->base.state->best_encoder,
> +			"best encoder set without crtc!\n");
>  	}
>  }
>  
> @@ -12699,20 +12694,23 @@ static void check_wm_state(struct drm_device *dev)
>  }
>  
>  static void
> -check_connector_state(struct drm_device *dev)
> +check_connector_state(struct drm_device *dev,
> +		      struct drm_atomic_state *old_state)
>  {
> -	struct intel_connector *connector;
> +	struct drm_connector_state *old_conn_state;
> +	struct drm_connector *connector;
> +	int i;
>  
> -	for_each_intel_connector(dev, connector) {
> -		struct drm_encoder *encoder = connector->base.encoder;
> -		struct drm_connector_state *state = connector->base.state;
> +	for_each_connector_in_state(old_state, connector, old_conn_state, i) {
> +		struct drm_encoder *encoder = connector->encoder;
> +		struct drm_connector_state *state = connector->state;
>  
>  		/* This also checks the encoder/connector hw state with the
>  		 * ->get_hw_state callbacks. */
> -		intel_connector_check_state(connector);
> +		intel_connector_check_state(to_intel_connector(connector));
>  
>  		I915_STATE_WARN(state->best_encoder != encoder,
> -		     "connector's staged encoder doesn't match current encoder\n");
> +		     "connector's atomic encoder doesn't match legacy encoder\n");
>  	}
>  }
>  
> @@ -12903,7 +12901,7 @@ intel_modeset_check_state(struct drm_device *dev,
>  			  struct drm_atomic_state *old_state)
>  {
>  	check_wm_state(dev);
> -	check_connector_state(dev);
> +	check_connector_state(dev, old_state);
>  	check_encoder_state(dev);
>  	check_crtc_state(dev);
>  	check_shared_dpll_state(dev);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 07/12] drm/i915: Get rid of dpms handling.

[Ander Conselvan De Oliveira]

Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>

On Mon, 2015-07-27 at 14:35 +0200, Maarten Lankhorst wrote:
> This is now done completely atomically.
> Keep connectors_active for now, but make it mirror crtc_state->active.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_crt.c     | 49 +-----------------
>  drivers/gpu/drm/i915/intel_display.c | 99 +-----------------------------------
>  drivers/gpu/drm/i915/intel_dp.c      |  2 +-
>  drivers/gpu/drm/i915/intel_dp_mst.c  |  2 +-
>  drivers/gpu/drm/i915/intel_drv.h     |  3 --
>  drivers/gpu/drm/i915/intel_dsi.c     |  2 +-
>  drivers/gpu/drm/i915/intel_dvo.c     | 46 +----------------
>  drivers/gpu/drm/i915/intel_hdmi.c    |  2 +-
>  drivers/gpu/drm/i915/intel_lvds.c    |  2 +-
>  drivers/gpu/drm/i915/intel_sdvo.c    | 47 +----------------
>  drivers/gpu/drm/i915/intel_tv.c      |  2 +-
>  11 files changed, 11 insertions(+), 245 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
> index 9eba3dd5b434..af5e43bef4a4 100644
> --- a/drivers/gpu/drm/i915/intel_crt.c
> +++ b/drivers/gpu/drm/i915/intel_crt.c
> @@ -236,53 +236,6 @@ static void intel_enable_crt(struct intel_encoder *encoder)
>  	intel_crt_set_dpms(encoder, crt->connector->base.dpms);
>  }
>  
> -/* Special dpms function to support cloning between dvo/sdvo/crt. */
> -static int intel_crt_dpms(struct drm_connector *connector, int mode)
> -{
> -	struct drm_device *dev = connector->dev;
> -	struct intel_encoder *encoder = intel_attached_encoder(connector);
> -	struct drm_crtc *crtc;
> -	int old_dpms;
> -
> -	/* PCH platforms and VLV only support on/off. */
> -	if (INTEL_INFO(dev)->gen >= 5 && mode != DRM_MODE_DPMS_ON)
> -		mode = DRM_MODE_DPMS_OFF;
> -
> -	if (mode == connector->dpms)
> -		return 0;
> -
> -	old_dpms = connector->dpms;
> -	connector->dpms = mode;
> -
> -	/* Only need to change hw state when actually enabled */
> -	crtc = encoder->base.crtc;
> -	if (!crtc) {
> -		encoder->connectors_active = false;
> -		return 0;
> -	}
> -
> -	/* We need the pipe to run for anything but OFF. */
> -	if (mode == DRM_MODE_DPMS_OFF)
> -		encoder->connectors_active = false;
> -	else
> -		encoder->connectors_active = true;
> -
> -	/* We call connector dpms manually below in case pipe dpms doesn't
> -	 * change due to cloning. */
> -	if (mode < old_dpms) {
> -		/* From off to on, enable the pipe first. */
> -		intel_crtc_update_dpms(crtc);
> -
> -		intel_crt_set_dpms(encoder, mode);
> -	} else {
> -		intel_crt_set_dpms(encoder, mode);
> -
> -		intel_crtc_update_dpms(crtc);
> -	}
> -
> -	return 0;
> -}
> -
>  static enum drm_mode_status
>  intel_crt_mode_valid(struct drm_connector *connector,
>  		     struct drm_display_mode *mode)
> @@ -798,7 +751,7 @@ static void intel_crt_reset(struct drm_connector *connector)
>  
>  static const struct drm_connector_funcs intel_crt_connector_funcs = {
>  	.reset = intel_crt_reset,
> -	.dpms = intel_crt_dpms,
> +	.dpms = drm_atomic_helper_connector_dpms,
>  	.detect = intel_crt_detect,
>  	.fill_modes = drm_helper_probe_single_connector_modes,
>  	.destroy = intel_crt_destroy,
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index e3afe611a78c..ed9eba2666e2 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6271,67 +6271,6 @@ free:
>  	return ret;
>  }
>  
> -/* Master function to enable/disable CRTC and corresponding power wells */
> -int intel_crtc_control(struct drm_crtc *crtc, bool enable)
> -{
> -	struct drm_device *dev = crtc->dev;
> -	struct drm_mode_config *config = &dev->mode_config;
> -	struct drm_modeset_acquire_ctx *ctx = config->acquire_ctx;
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	struct intel_crtc_state *pipe_config;
> -	struct drm_atomic_state *state;
> -	int ret;
> -
> -	if (enable == intel_crtc->active)
> -		return 0;
> -
> -	if (enable && !crtc->state->enable)
> -		return 0;
> -
> -	/* this function should be called with drm_modeset_lock_all for now */
> -	if (WARN_ON(!ctx))
> -		return -EIO;
> -	lockdep_assert_held(&ctx->ww_ctx);
> -
> -	state = drm_atomic_state_alloc(dev);
> -	if (WARN_ON(!state))
> -		return -ENOMEM;
> -
> -	state->acquire_ctx = ctx;
> -	state->allow_modeset = true;
> -
> -	pipe_config = intel_atomic_get_crtc_state(state, intel_crtc);
> -	if (IS_ERR(pipe_config)) {
> -		ret = PTR_ERR(pipe_config);
> -		goto err;
> -	}
> -	pipe_config->base.active = enable;
> -
> -	ret = drm_atomic_commit(state);
> -	if (!ret)
> -		return ret;
> -
> -err:
> -	DRM_ERROR("Updating crtc active failed with %i\n", ret);
> -	drm_atomic_state_free(state);
> -	return ret;
> -}
> -
> -/**
> - * Sets the power management mode of the pipe and plane.
> - */
> -void intel_crtc_update_dpms(struct drm_crtc *crtc)
> -{
> -	struct drm_device *dev = crtc->dev;
> -	struct intel_encoder *intel_encoder;
> -	bool enable = false;
> -
> -	for_each_encoder_on_crtc(dev, crtc, intel_encoder)
> -		enable |= intel_encoder->connectors_active;
> -
> -	intel_crtc_control(crtc, enable);
> -}
> -
>  void intel_encoder_destroy(struct drm_encoder *encoder)
>  {
>  	struct intel_encoder *intel_encoder = to_intel_encoder(encoder);
> @@ -6340,22 +6279,6 @@ void intel_encoder_destroy(struct drm_encoder *encoder)
>  	kfree(intel_encoder);
>  }
>  
> -/* Simple dpms helper for encoders with just one connector, no cloning and only
> - * one kind of off state. It clamps all !ON modes to fully OFF and changes the
> - * state of the entire output pipe. */
> -static void intel_encoder_dpms(struct intel_encoder *encoder, int mode)
> -{
> -	if (mode == DRM_MODE_DPMS_ON) {
> -		encoder->connectors_active = true;
> -
> -		intel_crtc_update_dpms(encoder->base.crtc);
> -	} else {
> -		encoder->connectors_active = false;
> -
> -		intel_crtc_update_dpms(encoder->base.crtc);
> -	}
> -}
> -
>  /* Cross check the actual hw state with our own modeset state tracking (and it's
>   * internal consistency). */
>  static void intel_connector_check_state(struct intel_connector *connector)
> @@ -6385,6 +6308,8 @@ static void intel_connector_check_state(struct intel_connector *connector)
>  		I915_STATE_WARN(conn_state->crtc != encoder->crtc,
>  			"attached encoder crtc differs from connector crtc\n");
>  	} else {
> +		I915_STATE_WARN(crtc && crtc->state->active,
> +			"attached crtc is active, but connector isn't\n");
>  		I915_STATE_WARN(!crtc && connector->base.state->best_encoder,
>  			"best encoder set without crtc!\n");
>  	}
> @@ -6418,26 +6343,6 @@ struct intel_connector *intel_connector_alloc(void)
>  	return connector;
>  }
>  
> -/* Even simpler default implementation, if there's really no special case to
> - * consider. */
> -int intel_connector_dpms(struct drm_connector *connector, int mode)
> -{
> -	/* All the simple cases only support two dpms states. */
> -	if (mode != DRM_MODE_DPMS_ON)
> -		mode = DRM_MODE_DPMS_OFF;
> -
> -	if (mode == connector->dpms)
> -		return 0;
> -
> -	connector->dpms = mode;
> -
> -	/* Only need to change hw state when actually enabled */
> -	if (connector->encoder)
> -		intel_encoder_dpms(to_intel_encoder(connector->encoder), mode);
> -
> -	return 0;
> -}
> -
>  /* Simple connector->get_hw_state implementation for encoders that support only
>   * one connector and no cloning and hence the encoder state determines the state
>   * of the connector. */
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index f1b9f939b435..cea7d1785d13 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4824,7 +4824,7 @@ static void intel_dp_encoder_reset(struct drm_encoder *encoder)
>  }
>  
>  static const struct drm_connector_funcs intel_dp_connector_funcs = {
> -	.dpms = intel_connector_dpms,
> +	.dpms = drm_atomic_helper_connector_dpms,
>  	.detect = intel_dp_detect,
>  	.force = intel_dp_force,
>  	.fill_modes = drm_helper_probe_single_connector_modes,
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index 35f2eb59818a..4bc371d19b91 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -328,7 +328,7 @@ intel_dp_mst_connector_destroy(struct drm_connector *connector)
>  }
>  
>  static const struct drm_connector_funcs intel_dp_mst_connector_funcs = {
> -	.dpms = intel_connector_dpms,
> +	.dpms = drm_atomic_helper_connector_dpms,
>  	.detect = intel_dp_mst_detect,
>  	.fill_modes = drm_helper_probe_single_connector_modes,
>  	.set_property = intel_dp_mst_set_property,
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 0da4236dc85a..62073d2f83fa 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -992,12 +992,9 @@ void intel_mark_busy(struct drm_device *dev);
>  void intel_mark_idle(struct drm_device *dev);
>  void intel_crtc_restore_mode(struct drm_crtc *crtc);
>  int intel_display_suspend(struct drm_device *dev);
> -int intel_crtc_control(struct drm_crtc *crtc, bool enable);
> -void intel_crtc_update_dpms(struct drm_crtc *crtc);
>  void intel_encoder_destroy(struct drm_encoder *encoder);
>  int intel_connector_init(struct intel_connector *);
>  struct intel_connector *intel_connector_alloc(void);
> -int intel_connector_dpms(struct drm_connector *, int mode);
>  bool intel_connector_get_hw_state(struct intel_connector *connector);
>  bool ibx_digital_port_connected(struct drm_i915_private *dev_priv,
>  				struct intel_digital_port *port);
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index 18dd7d7360a0..4a601cf90f16 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -982,7 +982,7 @@ static const struct drm_connector_helper_funcs 
> intel_dsi_connector_helper_funcs
>  };
>  
>  static const struct drm_connector_funcs intel_dsi_connector_funcs = {
> -	.dpms = intel_connector_dpms,
> +	.dpms = drm_atomic_helper_connector_dpms,
>  	.detect = intel_dsi_detect,
>  	.destroy = intel_dsi_connector_destroy,
>  	.fill_modes = drm_helper_probe_single_connector_modes,
> diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c
> index 600f7fb855d8..dc532bb61d22 100644
> --- a/drivers/gpu/drm/i915/intel_dvo.c
> +++ b/drivers/gpu/drm/i915/intel_dvo.c
> @@ -196,50 +196,6 @@ static void intel_enable_dvo(struct intel_encoder *encoder)
>  	intel_dvo->dev.dev_ops->dpms(&intel_dvo->dev, true);
>  }
>  
> -/* Special dpms function to support cloning between dvo/sdvo/crt. */
> -static int intel_dvo_dpms(struct drm_connector *connector, int mode)
> -{
> -	struct intel_dvo *intel_dvo = intel_attached_dvo(connector);
> -	struct drm_crtc *crtc;
> -	struct intel_crtc_state *config;
> -
> -	/* dvo supports only 2 dpms states. */
> -	if (mode != DRM_MODE_DPMS_ON)
> -		mode = DRM_MODE_DPMS_OFF;
> -
> -	if (mode == connector->dpms)
> -		return 0;
> -
> -	connector->dpms = mode;
> -
> -	/* Only need to change hw state when actually enabled */
> -	crtc = intel_dvo->base.base.crtc;
> -	if (!crtc) {
> -		intel_dvo->base.connectors_active = false;
> -		return 0;
> -	}
> -
> -	/* We call connector dpms manually below in case pipe dpms doesn't
> -	 * change due to cloning. */
> -	if (mode == DRM_MODE_DPMS_ON) {
> -		config = to_intel_crtc(crtc)->config;
> -
> -		intel_dvo->base.connectors_active = true;
> -
> -		intel_crtc_update_dpms(crtc);
> -
> -		intel_dvo->dev.dev_ops->dpms(&intel_dvo->dev, true);
> -	} else {
> -		intel_dvo->dev.dev_ops->dpms(&intel_dvo->dev, false);
> -
> -		intel_dvo->base.connectors_active = false;
> -
> -		intel_crtc_update_dpms(crtc);
> -	}
> -
> -	return 0;
> -}
> -
>  static enum drm_mode_status
>  intel_dvo_mode_valid(struct drm_connector *connector,
>  		     struct drm_display_mode *mode)
> @@ -387,7 +343,7 @@ static void intel_dvo_destroy(struct drm_connector *connector)
>  }
>  
>  static const struct drm_connector_funcs intel_dvo_connector_funcs = {
> -	.dpms = intel_dvo_dpms,
> +	.dpms = drm_atomic_helper_connector_dpms,
>  	.detect = intel_dvo_detect,
>  	.destroy = intel_dvo_destroy,
>  	.fill_modes = drm_helper_probe_single_connector_modes,
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 70bad5bf1d48..51cbea8247fe 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -1909,7 +1909,7 @@ static void intel_hdmi_destroy(struct drm_connector *connector)
>  }
>  
>  static const struct drm_connector_funcs intel_hdmi_connector_funcs = {
> -	.dpms = intel_connector_dpms,
> +	.dpms = drm_atomic_helper_connector_dpms,
>  	.detect = intel_hdmi_detect,
>  	.force = intel_hdmi_force,
>  	.fill_modes = drm_helper_probe_single_connector_modes,
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index cb634f48e7d9..881b5d13592e 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -549,7 +549,7 @@ static const struct drm_connector_helper_funcs 
> intel_lvds_connector_helper_funcs
>  };
>  
>  static const struct drm_connector_funcs intel_lvds_connector_funcs = {
> -	.dpms = intel_connector_dpms,
> +	.dpms = drm_atomic_helper_connector_dpms,
>  	.detect = intel_lvds_detect,
>  	.fill_modes = drm_helper_probe_single_connector_modes,
>  	.set_property = intel_lvds_set_property,
> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> index 8911e0e417ee..c98098e884cc 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -1508,51 +1508,6 @@ static void intel_enable_sdvo(struct intel_encoder *encoder)
>  	intel_sdvo_set_active_outputs(intel_sdvo, intel_sdvo->attached_output);
>  }
>  
> -/* Special dpms function to support cloning between dvo/sdvo/crt. */
> -static int intel_sdvo_dpms(struct drm_connector *connector, int mode)
> -{
> -	struct drm_crtc *crtc;
> -	struct intel_sdvo *intel_sdvo = intel_attached_sdvo(connector);
> -
> -	/* dvo supports only 2 dpms states. */
> -	if (mode != DRM_MODE_DPMS_ON)
> -		mode = DRM_MODE_DPMS_OFF;
> -
> -	if (mode == connector->dpms)
> -		return 0;
> -
> -	connector->dpms = mode;
> -
> -	/* Only need to change hw state when actually enabled */
> -	crtc = intel_sdvo->base.base.crtc;
> -	if (!crtc) {
> -		intel_sdvo->base.connectors_active = false;
> -		return 0;
> -	}
> -
> -	/* We set active outputs manually below in case pipe dpms doesn't change
> -	 * due to cloning. */
> -	if (mode != DRM_MODE_DPMS_ON) {
> -		intel_sdvo_set_active_outputs(intel_sdvo, 0);
> -		if (0)
> -			intel_sdvo_set_encoder_power_state(intel_sdvo, mode);
> -
> -		intel_sdvo->base.connectors_active = false;
> -
> -		intel_crtc_update_dpms(crtc);
> -	} else {
> -		intel_sdvo->base.connectors_active = true;
> -
> -		intel_crtc_update_dpms(crtc);
> -
> -		if (0)
> -			intel_sdvo_set_encoder_power_state(intel_sdvo, mode);
> -		intel_sdvo_set_active_outputs(intel_sdvo, intel_sdvo->attached_output);
> -	}
> -
> -	return 0;
> -}
> -
>  static enum drm_mode_status
>  intel_sdvo_mode_valid(struct drm_connector *connector,
>  		      struct drm_display_mode *mode)
> @@ -2190,7 +2145,7 @@ done:
>  }
>  
>  static const struct drm_connector_funcs intel_sdvo_connector_funcs = {
> -	.dpms = intel_sdvo_dpms,
> +	.dpms = drm_atomic_helper_connector_dpms,
>  	.detect = intel_sdvo_detect,
>  	.fill_modes = drm_helper_probe_single_connector_modes,
>  	.set_property = intel_sdvo_set_property,
> diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
> index 8b9d325bda3c..0568ae6ec9dd 100644
> --- a/drivers/gpu/drm/i915/intel_tv.c
> +++ b/drivers/gpu/drm/i915/intel_tv.c
> @@ -1509,7 +1509,7 @@ out:
>  }
>  
>  static const struct drm_connector_funcs intel_tv_connector_funcs = {
> -	.dpms = intel_connector_dpms,
> +	.dpms = drm_atomic_helper_connector_dpms,
>  	.detect = intel_tv_detect,
>  	.destroy = intel_tv_destroy,
>  	.set_property = intel_tv_set_property,
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 10/12] drm/i915: Remove connectors_active.

[Ander Conselvan De Oliveira]

Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>

On Mon, 2015-07-27 at 14:35 +0200, Maarten Lankhorst wrote:
> There are no more users, byebye!
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 37 +-----------------------------------
>  drivers/gpu/drm/i915/intel_drv.h     |  1 -
>  2 files changed, 1 insertion(+), 37 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 341fadb40c81..23175ce6583d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12196,27 +12196,12 @@ static bool intel_crtc_in_use(struct drm_crtc *crtc)
>  static void
>  intel_modeset_update_state(struct drm_atomic_state *state)
>  {
> -	struct drm_device *dev = state->dev;
> -	struct intel_encoder *intel_encoder;
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *crtc_state;
> -	struct drm_connector *connector;
>  	int i;
>  
>  	intel_shared_dpll_commit(state);
>  
> -	for_each_intel_encoder(dev, intel_encoder) {
> -		if (!intel_encoder->base.crtc)
> -			continue;
> -
> -		crtc = intel_encoder->base.crtc;
> -		crtc_state = drm_atomic_get_existing_crtc_state(state, crtc);
> -		if (!crtc_state || !needs_modeset(crtc->state))
> -			continue;
> -
> -		intel_encoder->connectors_active = false;
> -	}
> -
>  	drm_atomic_helper_update_legacy_modeset_state(state->dev, state);
>  
>  	/* Double check state. */
> @@ -12231,21 +12216,6 @@ intel_modeset_update_state(struct drm_atomic_state *state)
>  		else
>  			crtc->hwmode.crtc_clock = 0;
>  	}
> -
> -	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
> -		if (!connector->encoder || !connector->encoder->crtc)
> -			continue;
> -
> -		crtc = connector->encoder->crtc;
> -		crtc_state = drm_atomic_get_existing_crtc_state(state, crtc);
> -		if (!crtc_state || !needs_modeset(crtc->state))
> -			continue;
> -
> -		if (crtc->state->active) {
> -			intel_encoder = to_intel_encoder(connector->encoder);
> -			intel_encoder->connectors_active = true;
> -		}
> -	}
>  }
>  
>  static bool intel_fuzzy_clock_check(int clock1, int clock2)
> @@ -14965,10 +14935,8 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>  		 *  actually up, hence no need to break them. */
>  		WARN_ON(crtc->active);
>  
> -		for_each_encoder_on_crtc(dev, &crtc->base, encoder) {
> -			WARN_ON(encoder->connectors_active);
> +		for_each_encoder_on_crtc(dev, &crtc->base, encoder)
>  			encoder->base.crtc = NULL;
> -		}
>  	}
>  
>  	if (crtc->active || HAS_GMCH_DISPLAY(dev)) {
> @@ -15027,7 +14995,6 @@ static void intel_sanitize_encoder(struct intel_encoder *encoder)
>  				encoder->post_disable(encoder);
>  		}
>  		encoder->base.crtc = NULL;
> -		encoder->connectors_active = false;
>  
>  		/* Inconsistent output/port/pipe state happens presumably due to
>  		 * a bug in one of the get_hw_state functions. Or someplace else
> @@ -15189,7 +15156,6 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>  			encoder->base.crtc = NULL;
>  		}
>  
> -		encoder->connectors_active = false;
>  		DRM_DEBUG_KMS("[ENCODER:%d:%s] hw state readout: %s, pipe %c\n",
>  			      encoder->base.base.id,
>  			      encoder->base.name,
> @@ -15200,7 +15166,6 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>  	for_each_intel_connector(dev, connector) {
>  		if (connector->get_hw_state(connector)) {
>  			connector->base.dpms = DRM_MODE_DPMS_ON;
> -			connector->encoder->connectors_active = true;
>  			connector->base.encoder = &connector->encoder->base;
>  		} else {
>  			connector->base.dpms = DRM_MODE_DPMS_OFF;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 62073d2f83fa..c30bbb22381b 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -133,7 +133,6 @@ struct intel_encoder {
>  
>  	enum intel_output_type type;
>  	unsigned int cloneable;
> -	bool connectors_active;
>  	void (*hot_plug)(struct intel_encoder *);
>  	bool (*compute_config)(struct intel_encoder *,
>  			       struct intel_crtc_state *);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2.1 2.7/12] drm/i915: Update atomic state when removing mst connector, v2.

[Maarten Lankhorst]

Fully remove the MST connector from the atomic state, and remove the
early returns in check_*_state for MST connectors.

With atomic the state can be made consistent all the time.

Changes since v1:
- Remove the MST check in intel_connector_check_state too.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>
---
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7747520bf9f6..dfe4407b0390 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6370,10 +6370,6 @@ static void intel_connector_check_state(struct intel_connector *connector)
 			      connector->base.base.id,
 			      connector->base.name);
 
-		/* there is no real hw state for MST connectors */
-		if (connector->mst_port)
-			return;
-
 		I915_STATE_WARN(connector->base.dpms == DRM_MODE_DPMS_OFF,
 		     "wrong connector dpms state\n");
 		I915_STATE_WARN(connector->base.encoder != &encoder->base,
@@ -12751,13 +12747,6 @@ check_encoder_state(struct drm_device *dev)
 					encoder->base.crtc,
 			     "connector's crtc doesn't match encoder crtc\n");
 		}
-		/*
-		 * for MST connectors if we unplug the connector is gone
-		 * away but the encoder is still connected to a crtc
-		 * until a modeset happens in response to the hotplug.
-		 */
-		if (!enabled && encoder->base.encoder_type == DRM_MODE_ENCODER_DPMST)
-			continue;
 
 		I915_STATE_WARN(!!encoder->base.crtc != enabled,
 		     "encoder's enabled state mismatch "
diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index 585f0a45b3f1..35f2eb59818a 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -448,6 +448,49 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
 	return connector;
 }
 
+static void
+intel_dp_unbind_mst_connector(struct drm_device *dev,
+			      struct drm_connector *connector)
+{
+	struct drm_atomic_state *state;
+	struct drm_connector_state *conn_state;
+	struct drm_crtc *crtc = connector->state->crtc;
+	int ret;
+
+	if (!crtc)
+		return;
+
+	state = drm_atomic_state_alloc(dev);
+	if (!state)
+		return;
+
+	state->acquire_ctx = dev->mode_config.acquire_ctx;
+
+	conn_state = drm_atomic_get_connector_state(state, connector);
+	ret = PTR_ERR_OR_ZERO(conn_state);
+	if (!ret)
+		ret = drm_atomic_set_crtc_for_connector(conn_state, NULL);
+
+	if (!ret)
+		ret = drm_atomic_add_affected_connectors(state, crtc);
+
+	if (!ret && !drm_atomic_connectors_for_crtc(state, crtc)) {
+		struct drm_crtc_state *crtc_state =
+			drm_atomic_get_existing_crtc_state(state, crtc);
+
+		crtc_state->active = false;
+		ret = drm_atomic_set_mode_for_crtc(crtc_state, NULL);
+	}
+
+	if (!ret)
+		ret = drm_atomic_commit(state);
+
+	if (ret)
+		drm_atomic_state_free(state);
+
+	I915_STATE_WARN_ON(ret);
+}
+
 static void intel_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
 					   struct drm_connector *connector)
 {
@@ -455,7 +498,7 @@ static void intel_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
 	struct drm_device *dev = connector->dev;
 	/* need to nuke the connector */
 	drm_modeset_lock_all(dev);
-	intel_connector_dpms(connector, DRM_MODE_DPMS_OFF);
+	intel_dp_unbind_mst_connector(dev, connector);
 	drm_modeset_unlock_all(dev);
 
 	intel_connector->unregister(intel_connector);

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

Re: [PATCH v2 02/12] drm/i915: Update atomic state when removing mst connector.

[Sivakumar Thulasimani]

On 7/27/2015 6:05 PM, Maarten Lankhorst wrote:
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/intel_display.c |  7 ------
>   drivers/gpu/drm/i915/intel_dp_mst.c  | 45 +++++++++++++++++++++++++++++++++++-
>   2 files changed, 44 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 7747520bf9f6..3ab0a8a8e702 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12751,13 +12751,6 @@ check_encoder_state(struct drm_device *dev)
>   					encoder->base.crtc,
>   			     "connector's crtc doesn't match encoder crtc\n");
>   		}
> -		/*
> -		 * for MST connectors if we unplug the connector is gone
> -		 * away but the encoder is still connected to a crtc
> -		 * until a modeset happens in response to the hotplug.
> -		 */
> -		if (!enabled && encoder->base.encoder_type == DRM_MODE_ENCODER_DPMST)
> -			continue;
>   
>   		I915_STATE_WARN(!!encoder->base.crtc != enabled,
>   		     "encoder's enabled state mismatch "
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index 585f0a45b3f1..35f2eb59818a 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -448,6 +448,49 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
>   	return connector;
>   }
>   
> +static void
> +intel_dp_unbind_mst_connector(struct drm_device *dev,
> +			      struct drm_connector *connector)
> +{
> +	struct drm_atomic_state *state;
> +	struct drm_connector_state *conn_state;
> +	struct drm_crtc *crtc = connector->state->crtc;
> +	int ret;
> +
> +	if (!crtc)
> +		return;
> +
why cant we call drm_atomic_helper_set_config with just crtc copied into 
struct drm_mode_set ?
> +	state = drm_atomic_state_alloc(dev);
> +	if (!state)
> +		return;
> +
> +	state->acquire_ctx = dev->mode_config.acquire_ctx;
> +
> +	conn_state = drm_atomic_get_connector_state(state, connector);
> +	ret = PTR_ERR_OR_ZERO(conn_state);
> +	if (!ret)
> +		ret = drm_atomic_set_crtc_for_connector(conn_state, NULL);
> +
> +	if (!ret)
> +		ret = drm_atomic_add_affected_connectors(state, crtc);
> +
> +	if (!ret && !drm_atomic_connectors_for_crtc(state, crtc)) {
> +		struct drm_crtc_state *crtc_state =
> +			drm_atomic_get_existing_crtc_state(state, crtc);
> +
> +		crtc_state->active = false;
> +		ret = drm_atomic_set_mode_for_crtc(crtc_state, NULL);
> +	}
> +
> +	if (!ret)
> +		ret = drm_atomic_commit(state);
> +
> +	if (ret)
> +		drm_atomic_state_free(state);
> +
> +	I915_STATE_WARN_ON(ret);
> +}
> +
>   static void intel_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
>   					   struct drm_connector *connector)
>   {
> @@ -455,7 +498,7 @@ static void intel_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
>   	struct drm_device *dev = connector->dev;
>   	/* need to nuke the connector */
>   	drm_modeset_lock_all(dev);
> -	intel_connector_dpms(connector, DRM_MODE_DPMS_OFF);
> +	intel_dp_unbind_mst_connector(dev, connector);
>   	drm_modeset_unlock_all(dev);
>   
>   	intel_connector->unregister(intel_connector);

-- 
regards,
Sivakumar

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

Re: [PATCH v2 02/12] drm/i915: Update atomic state when removing mst connector.

[Maarten Lankhorst]

Op 06-08-15 om 07:34 schreef Sivakumar Thulasimani:
>
>
> On 7/27/2015 6:05 PM, Maarten Lankhorst wrote:
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_display.c |  7 ------
>>   drivers/gpu/drm/i915/intel_dp_mst.c  | 45 +++++++++++++++++++++++++++++++++++-
>>   2 files changed, 44 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 7747520bf9f6..3ab0a8a8e702 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -12751,13 +12751,6 @@ check_encoder_state(struct drm_device *dev)
>>                       encoder->base.crtc,
>>                    "connector's crtc doesn't match encoder crtc\n");
>>           }
>> -        /*
>> -         * for MST connectors if we unplug the connector is gone
>> -         * away but the encoder is still connected to a crtc
>> -         * until a modeset happens in response to the hotplug.
>> -         */
>> -        if (!enabled && encoder->base.encoder_type == DRM_MODE_ENCODER_DPMST)
>> -            continue;
>>             I915_STATE_WARN(!!encoder->base.crtc != enabled,
>>                "encoder's enabled state mismatch "
>> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
>> index 585f0a45b3f1..35f2eb59818a 100644
>> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
>> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
>> @@ -448,6 +448,49 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
>>       return connector;
>>   }
>>   +static void
>> +intel_dp_unbind_mst_connector(struct drm_device *dev,
>> +                  struct drm_connector *connector)
>> +{
>> +    struct drm_atomic_state *state;
>> +    struct drm_connector_state *conn_state;
>> +    struct drm_crtc *crtc = connector->state->crtc;
>> +    int ret;
>> +
>> +    if (!crtc)
>> +        return;
>> +
> why cant we call drm_atomic_helper_set_config with just crtc copied into struct drm_mode_set ?

Excellent idea! I think I overengineered this a little by trying to keep the crtc active after disabling if there are other connectors.

However I don't think it's allowed because cloneable is not set on the mst encoder. So new version coming up!

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