drm/i915: Set crtc_state->active to false when CRTC is disabled

With the recent modeset internal rework, we wind up setting crtc_state->enable to false, but leave crtc_state->active as true following a drmModeSetCrtc(fb=0), which is incorrect. This mismatch gets caught by drm_atomic_crtc_check() and causes subsequent atomic operations (such as plane updates while the CRTC is disabled) to fail.

Bisect points to

commit dad9a7d6d96630182fb52aae7c3856e9e7285e13
Author: Ander Conselvan de Oliveira <[email protected]>
Date: Tue Apr 21 17:13:19 2015 +0300

drm/i915: Use atomic helpers for computing changed flags

as the commit that actually triggers the regression.

Cc: Ander Conselvan de Oliveira <[email protected]>
Cc: Maarten Lankhorst <[email protected]>
Testcase: igt/kms_universal_plane

Signed-off-by Matt Roper <[email protected]>

---
My original expectation was that we'd need to be updating the in-flight CRTC state rather than the committed state as I wound up doing here. However it looks like our legacy modesets aren't yet converted to the point where they do state swaps, hence the need to update crtc->state directly.

Ander/Maarten, does this look like the right place to have 'active' updated for the time being, or should it be moved elsewhere in the modeset pipeline?

 drivers/gpu/drm/i915/intel_display.c | 2 ++
 1 file changed, 2 insertions(+)

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

index c297cdc..be166cf 100644
@@ -6128,6 +6128,8 @@ static void intel_crtc_disable(struct drm_crtc *crtc)
 
 	drm_plane_helper_disable(crtc->primary);
 
+	crtc->state->active = false;
+
 	/* Update computed state. */
 	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
 		if (!connector->encoder || !connector->encoder->crtc)

Signature · 1 line

1.8.5.1

With the recent modeset internal rework, we wind up setting crtc_state->enable to false, but leave crtc_state->active as true, which is incorrect. This mismatch gets caught by drm_atomic_crtc_check() and causes subsequent atomic operations (such as plane updates while the CRTC is disabled) to fail.

Bisect points to

commit dad9a7d6d96630182fb52aae7c3856e9e7285e13
Author: Ander Conselvan de Oliveira <[email protected]>
Date: Tue Apr 21 17:13:19 2015 +0300

drm/i915: Use atomic helpers for computing changed flags

as the commit that actually triggers the regression.

v2: Update to alter in-flight state rather than already-committed state
(first version was accidentally based on a midpoint of Ander's modeset rework series, before his final patches that add proper state swapping to the legacy modeset path).

Cc: Ander Conselvan de Oliveira <[email protected]>
Cc: Maarten Lankhorst <[email protected]>
Testcase: igt/kms_universal_plane

Signed-off-by Matt Roper <[email protected]>

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

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

index c297cdc..981478a 100644

Quoted 34 lines · Matt Roper

With the recent modeset internal rework, we wind up setting crtc_state->enable to false, but leave crtc_state->active as true, which is incorrect. This mismatch gets caught by drm_atomic_crtc_check() and causes subsequent atomic operations (such as plane updates while the CRTC is disabled) to fail.

Bisect points to

commit dad9a7d6d96630182fb52aae7c3856e9e7285e13
Author: Ander Conselvan de Oliveira <[email protected]>
Date: Tue Apr 21 17:13:19 2015 +0300

drm/i915: Use atomic helpers for computing changed flags

as the commit that actually triggers the regression.

v2: Update to alter in-flight state rather than already-committed state
(first version was accidentally based on a midpoint of Ander's modeset rework series, before his final patches that add proper state swapping to the legacy modeset path).

Cc: Ander Conselvan de Oliveira <[email protected]>
Cc: Maarten Lankhorst <[email protected]>
Testcase: igt/kms_universal_plane

Signed-off-by Matt Roper <[email protected]>

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

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

index c297cdc..981478a 100644

drivers/gpu/drm/i915/intel_display.c static int __intel_set_mode(struct drm_crtc *modeset_crtc, · line 12343

Quoted 4 lines

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

@@ -12340,6 +12340,7 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc,
 			continue;
 
 		if (!crtc_state->enable) {
+			crtc_state->active = false;

I believe a more appropriate fix would be to set crtc_state->active when we set crtc_state->enable to false in intel_modeset_stage_output_state() and the HW state read out. But I think Maarten does that in his later patch series, so as a stopgap solution this is fine.

Reviewed-by Ander Conselvan de Oliveira <[email protected]>

With the recent modeset internal rework, we wind up setting crtc_state->enable to false, but leave crtc_state->active as true following a drmModeSetCrtc(fb=0), which is incorrect. This mismatch gets caught by drm_atomic_crtc_check() and causes subsequent atomic operations (such as plane updates while the CRTC is disabled) to fail.

Bisect points to

commit dad9a7d6d96630182fb52aae7c3856e9e7285e13
Author: Ander Conselvan de Oliveira <[email protected]>
Date: Tue Apr 21 17:13:19 2015 +0300

drm/i915: Use atomic helpers for computing changed flags

as the commit that actually triggers the regression.

Cc Matt Roper <[email protected]>

Cc Ander Conselvan de Oliveira <[email protected]>

Reported-and-Tested-by Kenneth Graunke <[email protected]>

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

---
The original patch for this is wrong, we should keep enable and active in sync when changed. Please revert "drm/i915: Set crtc_state->active to false when CRTC is disabled (v2)".

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

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

index 22e6644f755e..e223209e2d81 100644
@@ -9868,7 +9868,7 @@ retry:
 		goto fail;
 	}
 
-	crtc_state->base.enable = true;
+	crtc_state->base.active = crtc_state->base.enable = true;
 
 	if (!mode)
 		mode = &load_detect_mode;

@@ -9965,7 +9965,7 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
 		connector_state->best_encoder = NULL;
 		connector_state->crtc = NULL;
 
-		crtc_state->base.enable = false;
+		crtc_state->base.enable = crtc_state->base.active = false;
 
 		ret = intel_modeset_setup_plane_state(state, crtc, NULL, NULL,
 						      0, 0);

@@ -12492,7 +12492,8 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc)
 			continue;
 		}
 
-		crtc_state->base.enable = intel_crtc->new_enabled;
+		crtc_state->base.active = crtc_state->base.enable =
+			intel_crtc->new_enabled;
 
 		if (&intel_crtc->base == crtc)
 			drm_mode_copy(&crtc_state->base.mode, &crtc->mode);

@@ -12617,11 +12618,16 @@ intel_modeset_stage_output_state(struct drm_device *dev,
 	}
 
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		bool has_connectors;
+
 		ret = drm_atomic_add_affected_connectors(state, crtc);
 		if (ret)
 			return ret;
 
-		crtc_state->enable = drm_atomic_connectors_for_crtc(state, crtc);
+		has_connectors = !!drm_atomic_connectors_for_crtc(state, crtc);
+		if (has_connectors != crtc_state->enable)
+			crtc_state->enable =
+			crtc_state->active = has_connectors;
 	}
 
 	ret = intel_modeset_setup_plane_state(state, set->crtc, set->mode,

@@ -14598,6 +14604,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
 
 		WARN_ON(crtc->active);
 		crtc->base.state->enable = false;
+		crtc->base.state->active = false;
 		crtc->base.enabled = false;
 	}
 

@@ -14626,6 +14633,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
 			      crtc->active ? "enabled" : "disabled");
 
 		crtc->base.state->enable = crtc->active;
+		crtc->base.state->active = crtc->active;
 		crtc->base.enabled = crtc->active;
 
 		/* Because we only establish the connector -> encoder ->

@@ -14764,6 +14772,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
 								 crtc->config);
 
 		crtc->base.state->enable = crtc->active;
+		crtc->base.state->active = crtc->active;
 		crtc->base.enabled = crtc->active;
 
 		plane_state = to_intel_plane_state(primary->state);

Signature · 6 lines

2.1.0

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