DPMS updates and atomic state checking.

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

Signed-off-by Maarten Lankhorst <[email protected]>

---
 drivers/gpu/drm/i915/i915_debugfs.c  | 82 +++++++++++-------------------------
 drivers/gpu/drm/i915/intel_display.c |  3 ++
 2 files changed, 27 insertions(+), 58 deletions(-)

drivers/gpu/drm/i915/i915_debugfs.c +24 -58

index 23a69307e12e..22467adee958 100644
@@ -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);
 	}

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

index 43b0f17ad1fa..7747520bf9f6 100644
@@ -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

Signature · 6 lines

2.1.0

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 4 lines

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

Signed-off-by Maarten Lankhorst <[email protected]>

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.

Op 27-07-15 om 16:04 schreef Daniel Vetter:

Quoted 13 lines · Maarten Lankhorst

Quoted 4 lines

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

Signed-off-by Maarten Lankhorst <[email protected]>

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".

Quoted 2 lines

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.

Signed-off-by Maarten Lankhorst <[email protected]>

---
 drivers/gpu/drm/i915/intel_display.c |  7 ------
 drivers/gpu/drm/i915/intel_dp_mst.c  | 45 
+++++++++++++++++++++++++++++++++++-
 2 files changed, 44 insertions(+), 8 deletions(-)

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

index 7747520bf9f6..3ab0a8a8e702 100644
@@ -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

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

-		 * away but the encoder is still connected to a crtc
-		 * until a modeset happens in response to the 

hotplug.

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

-		 */
-		if (!enabled && encoder->base.encoder_type == 

DRM_MODE_ENCODER_DPMST)

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

-			continue;
 
 		I915_STATE_WARN(!!encoder->base.crtc != enabled,
 		     "encoder's enabled state mismatch "

drivers/gpu/drm/i915/intel_dp_mst.c +25 -0

index 585f0a45b3f1..35f2eb59818a 100644
@@ -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);

drivers/gpu/drm/i915/intel_dp_mst.c +4 -0

+
+	if (!ret && !drm_atomic_connectors_for_crtc(state, crtc)) {
+		struct drm_crtc_state *crtc_state =
+			drm_atomic_get_existing_crtc_state(state, 

crtc);

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

+
+		crtc_state->active = false;
+		ret = drm_atomic_set_mode_for_crtc(crtc_state, 

NULL);

drivers/gpu/drm/i915/intel_dp_mst.c +11 -0

+	}
+
+	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)
{

drivers/gpu/drm/i915/intel_dp_mst.c +1 -1

@@ -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);

Signed-off-by Maarten Lankhorst <[email protected]>

---
  drivers/gpu/drm/i915/intel_display.c |  7 ------
  drivers/gpu/drm/i915/intel_dp_mst.c  | 45 +++++++++++++++++++++++++++++++++++-
  2 files changed, 44 insertions(+), 8 deletions(-)

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

index 7747520bf9f6..3ab0a8a8e702 100644
@@ -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 "

drivers/gpu/drm/i915/intel_dp_mst.c +2 -0

index 585f0a45b3f1..35f2eb59818a 100644
@@ -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,

drivers/gpu/drm/i915/intel_dp_mst.c +32 -1

+	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);

Quoted 35 lines

Signed-off-by Maarten Lankhorst <[email protected]>

---
  drivers/gpu/drm/i915/intel_display.c |  7 ------
  drivers/gpu/drm/i915/intel_dp_mst.c  | 45 +++++++++++++++++++++++++++++++++++-
  2 files changed, 44 insertions(+), 8 deletions(-)

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

index 7747520bf9f6..3ab0a8a8e702 100644
@@ -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 "

drivers/gpu/drm/i915/intel_dp_mst.c +4 -0

index 585f0a45b3f1..35f2eb59818a 100644
@@ -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;

drivers/gpu/drm/i915/intel_dp_mst.c static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo · line 462

Quoted 7 lines

drivers/gpu/drm/i915/intel_dp_mst.c +7 -0

+    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 ?

-

{

{

return 0;

Signed-off-by Maarten Lankhorst <[email protected]>

---
 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(-)

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

index 5d78c1feec81..9eba3dd5b434 100644
@@ -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;
 }
 

drivers/gpu/drm/i915/intel_display.c +10 -9

index 3ab0a8a8e702..ba0b68a4209d 100644
@@ -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?

Quoted 1 line

(quoted diff) +1 -0

+		if (!crtc)

drivers/gpu/drm/i915/intel_display.c +2 -7

 			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.

Quoted 1 line

-

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

-			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.

Quoted 1 line

(quoted diff) +1 -0

+		I915_STATE_WARN(conn_state->best_encoder != encoder,

drivers/gpu/drm/i915/intel_display.c +7 -11

+			"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.

Quoted 1 line

{

drivers/gpu/drm/i915/intel_display.c +11 -8

-	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.

Quoted 1 line

{

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

 	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;
 }

drivers/gpu/drm/i915/intel_drv.h +0 -1

index 320c9e6bd848..0da4236dc85a 100644
@@ -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,

drivers/gpu/drm/i915/intel_dvo.c +0 -2

index fd5e522abebb..600f7fb855d8 100644
@@ -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;
 }
 

drivers/gpu/drm/i915/intel_sdvo.c +0 -2

index 2c435a79d4da..8911e0e417ee 100644
@@ -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?

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 <[email protected]>

---
 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(-)

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

index 5d78c1feec81..9eba3dd5b434 100644
@@ -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;
 }
 

drivers/gpu/drm/i915/intel_display.c +6 -6

index dfe4407b0390..77b4da7e698c 100644
@@ -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;
 }

drivers/gpu/drm/i915/intel_drv.h +0 -1

index 320c9e6bd848..0da4236dc85a 100644
@@ -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,

drivers/gpu/drm/i915/intel_dvo.c +0 -2

index fd5e522abebb..600f7fb855d8 100644
@@ -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;
 }
 

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

index 2c435a79d4da..8911e0e417ee 100644

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 <[email protected]>

---
 drivers/gpu/drm/i915/intel_display.c | 66 +++++++++++++++++-------------------
 1 file changed, 32 insertions(+), 34 deletions(-)

drivers/gpu/drm/i915/intel_display.c +31 -33

index 77b4da7e698c..876071ad4681 100644
@@ -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");
 	}
 }
 

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

Signed-off-by Maarten Lankhorst <[email protected]>

---
 drivers/gpu/drm/i915/intel_display.c | 19 +------------------
 1 file changed, 1 insertion(+), 18 deletions(-)

drivers/gpu/drm/i915/intel_display.c +1 -13

index ba0b68a4209d..59eb6db10740 100644
@@ -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 "

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

-		     "(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 "

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 <[email protected]>

---
 drivers/gpu/drm/i915/intel_display.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

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

index 59eb6db10740..fbb257d4728c 100644
@@ -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) {

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

 			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 "

drivers/gpu/drm/i915/intel_display.c +4 -5

-		     "(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");

drivers/gpu/drm/i915/intel_display.c +5 -1

+
+			continue;
+		}
+
+		I915_STATE_WARN(active != encoder->base.crtc->state
->active,
 		     "encoder's hw state doesn't match sw tracking "

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 <[email protected]>

---
 drivers/gpu/drm/i915/intel_display.c | 118 +++++++++++++++++------------------
 1 file changed, 56 insertions(+), 62 deletions(-)

drivers/gpu/drm/i915/intel_display.c +9 -31

index fbb257d4728c..e3afe611a78c 100644
@@ -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)
 {

drivers/gpu/drm/i915/intel_display.c +42 -27

+		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]");
 		}
 	}

Quoted 104 lines · 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 <[email protected]>

---
 drivers/gpu/drm/i915/intel_display.c | 118 +++++++++++++++++------------------
 1 file changed, 56 insertions(+), 62 deletions(-)

drivers/gpu/drm/i915/intel_display.c +12 -33

index fbb257d4728c..e3afe611a78c 100644
@@ -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;
 

drivers/gpu/drm/i915/intel_display.c check_encoder_state(struct drm_device *dev) · line 12761

Quoted 2 lines

drivers/gpu/drm/i915/intel_display.c +1 -1

-	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?

Quoted 127 lines · Ander Conselvan De Oliveira

Quoted 126 lines · 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 <
[email protected]>

---
 drivers/gpu/drm/i915/intel_display.c | 118 +++++++++++++++++----
--------------
 1 file changed, 56 insertions(+), 62 deletions(-)

drivers/gpu/drm/i915/intel_display.c +1 -1

index fbb257d4728c..e3afe611a78c 100644
@@ -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)) {

drivers/gpu/drm/i915/intel_display.c +7 -23

@@ -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 "

drivers/gpu/drm/i915/intel_display.c +1 -12

-		     "(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)
{

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

 	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;

Quoted 10 lines

drivers/gpu/drm/i915/intel_display.c check_encoder_state(struct drm_device *dev) · line 12761

Quoted 2 lines

drivers/gpu/drm/i915/intel_display.c +1 -1

-	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.

Quoted 138 lines

Quoted 136 lines · Ander Conselvan De Oliveira

Quoted 118 lines · 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 <
[email protected]>

---
 drivers/gpu/drm/i915/intel_display.c | 118 +++++++++++++++++----
--------------
 1 file changed, 56 insertions(+), 62 deletions(-)

drivers/gpu/drm/i915/intel_display.c +1 -1

index fbb257d4728c..e3afe611a78c 100644
@@ -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)) {

drivers/gpu/drm/i915/intel_display.c +7 -23

@@ -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 "

drivers/gpu/drm/i915/intel_display.c +1 -12

-		     "(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)
{

drivers/gpu/drm/i915/intel_display.c check_encoder_state(struct drm_device *dev) · line 12761

Quoted 10 lines

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

 	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?

Quoted 4 lines

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 <[email protected]>

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

Signed-off-by Maarten Lankhorst <[email protected]>

---
 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(-)

drivers/gpu/drm/i915/intel_crt.c +1 -48

index 9eba3dd5b434..af5e43bef4a4 100644
@@ -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,

drivers/gpu/drm/i915/intel_display.c +2 -97

index e3afe611a78c..ed9eba2666e2 100644
@@ -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. */

drivers/gpu/drm/i915/intel_dp.c +1 -1

index f1b9f939b435..cea7d1785d13 100644
@@ -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,

drivers/gpu/drm/i915/intel_dp_mst.c +1 -1

index 35f2eb59818a..4bc371d19b91 100644
@@ -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,

drivers/gpu/drm/i915/intel_drv.h +0 -3

index 0da4236dc85a..62073d2f83fa 100644
@@ -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);

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

index 18dd7d7360a0..4a601cf90f16 100644
@@ -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 = {

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

-	.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,

drivers/gpu/drm/i915/intel_dvo.c +1 -45

index 600f7fb855d8..dc532bb61d22 100644
@@ -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,

drivers/gpu/drm/i915/intel_hdmi.c +1 -1

index 70bad5bf1d48..51cbea8247fe 100644
@@ -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,

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

index cb634f48e7d9..881b5d13592e 100644
@@ -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 = {

drivers/gpu/drm/i915/intel_lvds.c +1 -1

-	.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,

drivers/gpu/drm/i915/intel_sdvo.c +1 -46

index 8911e0e417ee..c98098e884cc 100644
@@ -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,

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

index 8b9d325bda3c..0568ae6ec9dd 100644

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

Signed-off-by Maarten Lankhorst <[email protected]>

---
 drivers/gpu/drm/i915/intel_display.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

drivers/gpu/drm/i915/intel_display.c +5 -2

index ed9eba2666e2..341fadb40c81 100644
@@ -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

Quoted 36 lines

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

Signed-off-by Maarten Lankhorst <[email protected]>

---
 drivers/gpu/drm/i915/intel_display.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

drivers/gpu/drm/i915/intel_display.c +5 -2

index ed9eba2666e2..341fadb40c81 100644
@@ -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;