Use more atomic state in i915.

Instead of restoring dpms and a flag for whether a temp fb is allocated duplicate the old plane_state and crtc_state, and restore the members we potentially touched.

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

---
 drivers/gpu/drm/i915/intel_display.c | 128 ++++++++++++++++++++---------------
 drivers/gpu/drm/i915/intel_drv.h     |   4 +-
 2 files changed, 76 insertions(+), 56 deletions(-)

drivers/gpu/drm/i915/intel_display.c +60 -48

index 4d8c9f7857db..0702ce8ec36a 100644
@@ -10361,6 +10361,7 @@ mode_fits_in_fbdev(struct drm_device *dev,
 	if (obj->base.size < mode->vdisplay * fb->pitches[0])
 		return NULL;
 
+	drm_framebuffer_reference(fb);
 	return fb;
 #else
 	return NULL;

@@ -10426,6 +10427,9 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector,
 		      encoder->base.id, encoder->name);
 
 retry:
+	old->old_pipe_config = NULL;
+	old->old_plane_state = NULL;
+
 	ret = drm_modeset_lock(&config->connection_mutex, ctx);
 	if (ret)
 		goto fail;

@@ -10441,24 +10445,15 @@ retry:
 	 */
 
 	/* See if we already have a CRTC for this connector */
-	if (encoder->crtc) {
-		crtc = encoder->crtc;
+	if (connector->state->crtc) {
+		crtc = connector->state->crtc;
 
 		ret = drm_modeset_lock(&crtc->mutex, ctx);
 		if (ret)
 			goto fail;
-		ret = drm_modeset_lock(&crtc->primary->mutex, ctx);
-		if (ret)
-			goto fail;
-
-		old->dpms_mode = connector->dpms;
-		old->load_detect_temp = false;
 
 		/* Make sure the crtc and connector are running */
-		if (connector->dpms != DRM_MODE_DPMS_ON)
-			connector->funcs->dpms(connector, DRM_MODE_DPMS_ON);
-
-		return true;
+		goto found;
 	}
 
 	/* Find an unused one (if possible) */

@@ -10466,8 +10461,15 @@ retry:
 		i++;
 		if (!(encoder->possible_crtcs & (1 << i)))
 			continue;
-		if (possible_crtc->state->enable)
+
+		ret = drm_modeset_lock(&possible_crtc->mutex, ctx);
+		if (ret)
+			goto fail;
+
+		if (possible_crtc->state->enable) {
+			drm_modeset_unlock(&possible_crtc->mutex);
 			continue;
+		}
 
 		crtc = possible_crtc;
 		break;

@@ -10481,17 +10483,19 @@ retry:
 		goto fail;
 	}
 
-	ret = drm_modeset_lock(&crtc->mutex, ctx);
-	if (ret)
-		goto fail;
+found:
+	intel_crtc = to_intel_crtc(crtc);
+
 	ret = drm_modeset_lock(&crtc->primary->mutex, ctx);
 	if (ret)
 		goto fail;
 
-	intel_crtc = to_intel_crtc(crtc);
-	old->dpms_mode = connector->dpms;
-	old->load_detect_temp = true;
-	old->release_fb = NULL;
+	old->old_pipe_config = intel_crtc_duplicate_state(crtc);
+	old->old_plane_state = intel_plane_duplicate_state(crtc->primary);
+	if (!old->old_pipe_config || !old->old_plane_state) {
+		ret = -ENOMEM;
+		goto fail;
+	}
 
 	state = drm_atomic_state_alloc(dev);
 	if (!state)

@@ -10505,7 +10509,9 @@ retry:
 		goto fail;
 	}
 
-	connector_state->crtc = crtc;
+	ret = drm_atomic_set_crtc_for_connector(connector_state, crtc);
+	if (ret)
+		goto fail;
 
 	crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
 	if (IS_ERR(crtc_state)) {

@@ -10529,7 +10535,6 @@ retry:
 	if (fb == NULL) {
 		DRM_DEBUG_KMS("creating tmp fb for load-detection\n");
 		fb = intel_framebuffer_create_for_mode(dev, mode, 24, 32);
-		old->release_fb = fb;
 	} else
 		DRM_DEBUG_KMS("reusing fbdev for load-detection framebuffer\n");
 	if (IS_ERR(fb)) {

@@ -10541,15 +10546,16 @@ retry:
 	if (ret)
 		goto fail;
 
-	drm_mode_copy(&crtc_state->base.mode, mode);
+	drm_framebuffer_unreference(fb);
+
+	ret = drm_atomic_set_mode_for_crtc(&crtc_state->base, mode);
+	if (ret)
+		goto fail;
 
 	if (drm_atomic_commit(state)) {
 		DRM_DEBUG_KMS("failed to set mode on load-detect pipe\n");
-		if (old->release_fb)
-			old->release_fb->funcs->destroy(old->release_fb);
 		goto fail;
 	}
-	crtc->primary->crtc = crtc;
 
 	/* let the connector get through one full cycle before testing */
 	intel_wait_for_vblank(dev, intel_crtc->pipe);

@@ -10559,6 +10565,12 @@ fail:
 	drm_atomic_state_free(state);
 	state = NULL;
 
+	if (old->old_pipe_config)
+		intel_crtc_destroy_state(crtc, old->old_pipe_config);
+
+	if (old->old_plane_state)
+		intel_plane_destroy_state(crtc->primary, old->old_plane_state);
+
 	if (ret == -EDEADLK) {
 		drm_modeset_backoff(ctx);
 		goto retry;

@@ -10575,61 +10587,69 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
 	struct intel_encoder *intel_encoder =
 		intel_attached_encoder(connector);
 	struct drm_encoder *encoder = &intel_encoder->base;
-	struct drm_crtc *crtc = encoder->crtc;
+	struct drm_crtc *crtc = connector->state->crtc;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct drm_atomic_state *state;
 	struct drm_connector_state *connector_state;
 	struct intel_crtc_state *crtc_state;
+	struct drm_plane_state *plane_state;
 	int ret;
 
 	DRM_DEBUG_KMS("[CONNECTOR:%d:%s], [ENCODER:%d:%s]\n",
 		      connector->base.id, connector->name,
 		      encoder->base.id, encoder->name);
 
-	if (old->load_detect_temp) {
-		state = drm_atomic_state_alloc(dev);
-		if (!state)
-			goto fail;
+	if (!old->old_pipe_config)
+		return;
 
-		state->acquire_ctx = ctx;
+	state = drm_atomic_state_alloc(dev);
+	if (!state)
+		goto fail;
 
-		connector_state = drm_atomic_get_connector_state(state, connector);
-		if (IS_ERR(connector_state))
-			goto fail;
+	state->acquire_ctx = ctx;
 
-		crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
-		if (IS_ERR(crtc_state))
-			goto fail;
+	connector_state = drm_atomic_get_connector_state(state, connector);
+	if (IS_ERR(connector_state))
+		goto fail;
 
-		connector_state->crtc = NULL;
+	crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
+	if (IS_ERR(crtc_state))
+		goto fail;
 
-		crtc_state->base.enable = crtc_state->base.active = false;
+	plane_state = drm_atomic_get_plane_state(state, crtc->primary);
+	if (IS_ERR(plane_state))
+		goto fail;
 
-		ret = intel_modeset_setup_plane_state(state, crtc, NULL, NULL,
-						      0, 0);
+	if (!old->old_pipe_config->enable) {
+		ret = drm_atomic_set_crtc_for_connector(connector_state, NULL);
 		if (ret)
 			goto fail;
+	}
 
-		ret = drm_atomic_commit(state);
-		if (ret)
-			goto fail;
+	crtc_state->base.enable = old->old_pipe_config->enable;
+	crtc_state->base.active = old->old_pipe_config->active;
 
-		if (old->release_fb) {
-			drm_framebuffer_unregister_private(old->release_fb);
-			drm_framebuffer_unreference(old->release_fb);

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

-	/* Switch crtc and encoder back off if necessary */
-	if (old->dpms_mode != DRM_MODE_DPMS_ON)
-		connector->funcs->dpms(connector, old->dpms_mode);
+	ret = drm_atomic_commit(state);
+	if (ret)
+		goto fail;
 
+	intel_plane_destroy_state(crtc->primary, old->old_plane_state);
+	intel_crtc_destroy_state(crtc, old->old_pipe_config);
 	return;
+
 fail:
 	DRM_DEBUG_KMS("Couldn't release load detect pipe.\n");
 	drm_atomic_state_free(state);
+	intel_plane_destroy_state(crtc->primary, old->old_plane_state);
+	intel_crtc_destroy_state(crtc, old->old_pipe_config);
 }
 
 static int i9xx_pll_refclk(struct drm_device *dev,

drivers/gpu/drm/i915/intel_drv.h +2 -2

index 93ba14a3bb76..eeda7d02a4f7 100644
@@ -913,8 +913,8 @@ struct intel_unpin_work {
 
 struct intel_load_detect_pipe {
 	struct drm_framebuffer *release_fb;
-	bool load_detect_temp;
-	int dpms_mode;
+	struct drm_crtc_state *old_pipe_config;
+	struct drm_plane_state *old_plane_state;
 };
 
 static inline struct intel_encoder *

Signature · 6 lines

2.1.0

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

Quoted 230 lines

Instead of restoring dpms and a flag for whether a temp fb is allocated duplicate the old plane_state and crtc_state, and restore the members we potentially touched.

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

---
 drivers/gpu/drm/i915/intel_display.c | 128 ++++++++++++++++++++---------------
 drivers/gpu/drm/i915/intel_drv.h     |   4 +-
 2 files changed, 76 insertions(+), 56 deletions(-)

drivers/gpu/drm/i915/intel_display.c +64 -49

index 4d8c9f7857db..0702ce8ec36a 100644
@@ -10361,6 +10361,7 @@ mode_fits_in_fbdev(struct drm_device *dev,
 	if (obj->base.size < mode->vdisplay * fb->pitches[0])
 		return NULL;
 
+	drm_framebuffer_reference(fb);
 	return fb;
 #else
 	return NULL;

@@ -10426,6 +10427,9 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector,
 		      encoder->base.id, encoder->name);
 
 retry:
+	old->old_pipe_config = NULL;
+	old->old_plane_state = NULL;
+
 	ret = drm_modeset_lock(&config->connection_mutex, ctx);
 	if (ret)
 		goto fail;

@@ -10441,24 +10445,15 @@ retry:
 	 */
 
 	/* See if we already have a CRTC for this connector */
-	if (encoder->crtc) {
-		crtc = encoder->crtc;
+	if (connector->state->crtc) {
+		crtc = connector->state->crtc;
 
 		ret = drm_modeset_lock(&crtc->mutex, ctx);
 		if (ret)
 			goto fail;
-		ret = drm_modeset_lock(&crtc->primary->mutex, ctx);
-		if (ret)
-			goto fail;
-
-		old->dpms_mode = connector->dpms;
-		old->load_detect_temp = false;
 
 		/* Make sure the crtc and connector are running */
-		if (connector->dpms != DRM_MODE_DPMS_ON)
-			connector->funcs->dpms(connector, DRM_MODE_DPMS_ON);
-
-		return true;
+		goto found;
 	}
 
 	/* Find an unused one (if possible) */

@@ -10466,8 +10461,15 @@ retry:
 		i++;
 		if (!(encoder->possible_crtcs & (1 << i)))
 			continue;
-		if (possible_crtc->state->enable)
+
+		ret = drm_modeset_lock(&possible_crtc->mutex, ctx);
+		if (ret)
+			goto fail;
+
+		if (possible_crtc->state->enable) {
+			drm_modeset_unlock(&possible_crtc->mutex);
 			continue;
+		}
 
 		crtc = possible_crtc;
 		break;

@@ -10481,17 +10483,19 @@ retry:
 		goto fail;
 	}
 
-	ret = drm_modeset_lock(&crtc->mutex, ctx);
-	if (ret)
-		goto fail;
+found:
+	intel_crtc = to_intel_crtc(crtc);
+
 	ret = drm_modeset_lock(&crtc->primary->mutex, ctx);
 	if (ret)
 		goto fail;
 
-	intel_crtc = to_intel_crtc(crtc);
-	old->dpms_mode = connector->dpms;
-	old->load_detect_temp = true;
-	old->release_fb = NULL;
+	old->old_pipe_config = intel_crtc_duplicate_state(crtc);
+	old->old_plane_state = intel_plane_duplicate_state(crtc->primary);
+	if (!old->old_pipe_config || !old->old_plane_state) {
+		ret = -ENOMEM;
+		goto fail;
+	}
 
 	state = drm_atomic_state_alloc(dev);
 	if (!state)

@@ -10505,7 +10509,9 @@ retry:
 		goto fail;
 	}
 
-	connector_state->crtc = crtc;
+	ret = drm_atomic_set_crtc_for_connector(connector_state, crtc);
+	if (ret)
+		goto fail;
 
 	crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
 	if (IS_ERR(crtc_state)) {

@@ -10529,7 +10535,6 @@ retry:
 	if (fb == NULL) {
 		DRM_DEBUG_KMS("creating tmp fb for load-detection\n");
 		fb = intel_framebuffer_create_for_mode(dev, mode, 24, 32);
-		old->release_fb = fb;
 	} else
 		DRM_DEBUG_KMS("reusing fbdev for load-detection framebuffer\n");
 	if (IS_ERR(fb)) {

@@ -10541,15 +10546,16 @@ retry:
 	if (ret)
 		goto fail;
 
-	drm_mode_copy(&crtc_state->base.mode, mode);
+	drm_framebuffer_unreference(fb);
+
+	ret = drm_atomic_set_mode_for_crtc(&crtc_state->base, mode);
+	if (ret)
+		goto fail;
 
 	if (drm_atomic_commit(state)) {
 		DRM_DEBUG_KMS("failed to set mode on load-detect pipe\n");
-		if (old->release_fb)
-			old->release_fb->funcs->destroy(old->release_fb);
 		goto fail;
 	}
-	crtc->primary->crtc = crtc;
 
 	/* let the connector get through one full cycle before testing */
 	intel_wait_for_vblank(dev, intel_crtc->pipe);

@@ -10559,6 +10565,12 @@ fail:
 	drm_atomic_state_free(state);
 	state = NULL;
 
+	if (old->old_pipe_config)
+		intel_crtc_destroy_state(crtc, old->old_pipe_config);
+
+	if (old->old_plane_state)
+		intel_plane_destroy_state(crtc->primary, old->old_plane_state);
+
 	if (ret == -EDEADLK) {
 		drm_modeset_backoff(ctx);
 		goto retry;

@@ -10575,61 +10587,69 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
 	struct intel_encoder *intel_encoder =
 		intel_attached_encoder(connector);
 	struct drm_encoder *encoder = &intel_encoder->base;
-	struct drm_crtc *crtc = encoder->crtc;
+	struct drm_crtc *crtc = connector->state->crtc;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct drm_atomic_state *state;
 	struct drm_connector_state *connector_state;
 	struct intel_crtc_state *crtc_state;
+	struct drm_plane_state *plane_state;
 	int ret;
 
 	DRM_DEBUG_KMS("[CONNECTOR:%d:%s], [ENCODER:%d:%s]\n",
 		      connector->base.id, connector->name,
 		      encoder->base.id, encoder->name);
 
-	if (old->load_detect_temp) {
-		state = drm_atomic_state_alloc(dev);
-		if (!state)
-			goto fail;
+	if (!old->old_pipe_config)
+		return;
 
-		state->acquire_ctx = ctx;
+	state = drm_atomic_state_alloc(dev);
+	if (!state)
+		goto fail;
 
-		connector_state = drm_atomic_get_connector_state(state, connector);
-		if (IS_ERR(connector_state))
-			goto fail;
+	state->acquire_ctx = ctx;
 
-		crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
-		if (IS_ERR(crtc_state))
-			goto fail;
+	connector_state = drm_atomic_get_connector_state(state, connector);
+	if (IS_ERR(connector_state))
+		goto fail;
 
-		connector_state->crtc = NULL;
+	crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
+	if (IS_ERR(crtc_state))
+		goto fail;
 
-		crtc_state->base.enable = crtc_state->base.active = false;
+	plane_state = drm_atomic_get_plane_state(state, crtc->primary);
+	if (IS_ERR(plane_state))
+		goto fail;
 
-		ret = intel_modeset_setup_plane_state(state, crtc, NULL, NULL,
-						      0, 0);
+	if (!old->old_pipe_config->enable) {
+		ret = drm_atomic_set_crtc_for_connector(connector_state, NULL);
 		if (ret)
 			goto fail;
+	}
 
-		ret = drm_atomic_commit(state);
-		if (ret)
-			goto fail;
+	crtc_state->base.enable = old->old_pipe_config->enable;
+	crtc_state->base.active = old->old_pipe_config->active;
 
-		if (old->release_fb) {
-			drm_framebuffer_unregister_private(old->release_fb);
-			drm_framebuffer_unreference(old->release_fb);
-		}
+	drm_atomic_set_fb_for_plane(plane_state, old->old_plane_state->fb);
+	ret = drm_atomic_set_crtc_for_plane(plane_state, old->old_plane_state->crtc);
+	if (ret)
+		goto fail;
 

drivers/gpu/drm/i915/intel_display.c void intel_release_load_detect_pipe(struct drm_connector *connector, · line 10638

Quoted 4 lines

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

-		return;
-	}
+	old->old_plane_state->state = plane_state->state;
+	*plane_state = *old->old_plane_state;

This part looks messy. I don't think there's any reason anyone interested in load detection should need to do these sort of gymnastics manually. So there really should be some kind of nice thing to just tell atomic that:
"i have this old plane/crtc/connector/etc. state here, please just restore it"

Instead of restoring dpms and a flag for whether a temp fb is allocated duplicate the old plane_state and crtc_state, and restore the members we potentially touched.

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

---
 drivers/gpu/drm/i915/intel_display.c | 128 ++++++++++++++++++++---------------
 drivers/gpu/drm/i915/intel_drv.h     |   4 +-
 2 files changed, 76 insertions(+), 56 deletions(-)

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

index 4d8c9f7857db..0702ce8ec36a 100644
@@ -10361,6 +10361,7 @@ mode_fits_in_fbdev(struct drm_device *dev,
 	if (obj->base.size < mode->vdisplay * fb->pitches[0])
 		return NULL;
 
+	drm_framebuffer_reference(fb);
 	return fb;
 #else
 	return NULL;

@@ -10426,6 +10427,9 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector,
 		      encoder->base.id, encoder->name);
 
 retry:
+	old->old_pipe_config = NULL;
+	old->old_plane_state = NULL;
+
 	ret = drm_modeset_lock(&config->connection_mutex, ctx);
 	if (ret)
 		goto fail;

Quoted 32 lines

Instead of restoring dpms and a flag for whether a temp fb is allocated duplicate the old plane_state and crtc_state, and restore the members we potentially touched.

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

---
 drivers/gpu/drm/i915/intel_display.c | 128 ++++++++++++++++++++---------------
 drivers/gpu/drm/i915/intel_drv.h     |   4 +-
 2 files changed, 76 insertions(+), 56 deletions(-)

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

index 4d8c9f7857db..0702ce8ec36a 100644
@@ -10361,6 +10361,7 @@ mode_fits_in_fbdev(struct drm_device *dev,
 	if (obj->base.size < mode->vdisplay * fb->pitches[0])
 		return NULL;
 
+	drm_framebuffer_reference(fb);
 	return fb;
 #else
 	return NULL;

@@ -10426,6 +10427,9 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector,
 		      encoder->base.id, encoder->name);
 
 retry:
+	old->old_pipe_config = NULL;
+	old->old_plane_state = NULL;
+
 	ret = drm_modeset_lock(&config->connection_mutex, ctx);
 	if (ret)
 		goto fail;

drivers/gpu/drm/i915/intel_display.c retry: · line 10448

Quoted 6 lines

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

@@ -10441,24 +10445,15 @@ retry:
 	 */
 
 	/* See if we already have a CRTC for this connector */
-	if (encoder->crtc) {
-		crtc = encoder->crtc;
+	if (connector->state->crtc) {

All these connector->state accesses made me a bit uneasy, but we did indeed grab connection_mutex already so it should be fine. It's even more troubling seeing connector->state accessed outside intel_get_load_detect_pipe() in the later patches, but it seems it's only done if intel_get_load_detect_pipe() succeeded which means we should be holding the right lock.

Dunno, maybe there should be some comments explaining this stuff. Or maybe maybe we should have a helper to return the current connector state that also asserts that the right lock is held?

drm/i915: Use atomic state to obtain load detection crtc, v2.

Instead of restoring dpms and a flag for whether a temp fb is allocated duplicate an atomic state before the new state is committed, and commit it the old state in intel_release_load_detect_pipe.

Changes since v1:
- Use a real atomic state.

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

---

drivers/gpu/drm/i915/intel_display.c +48 -67

index 4d8c9f7857db..c7ba8f4a971e 100644
@@ -10361,6 +10361,7 @@ mode_fits_in_fbdev(struct drm_device *dev,
 	if (obj->base.size < mode->vdisplay * fb->pitches[0])
 		return NULL;
 
+	drm_framebuffer_reference(fb);
 	return fb;
 #else
 	return NULL;

@@ -10416,7 +10417,7 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector,
 	struct drm_device *dev = encoder->dev;
 	struct drm_framebuffer *fb;
 	struct drm_mode_config *config = &dev->mode_config;
-	struct drm_atomic_state *state = NULL;
+	struct drm_atomic_state *state = NULL, *restore_state = NULL;
 	struct drm_connector_state *connector_state;
 	struct intel_crtc_state *crtc_state;
 	int ret, i = -1;

@@ -10425,6 +10426,8 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector,
 		      connector->base.id, connector->name,
 		      encoder->base.id, encoder->name);
 
+	old->restore_state = NULL;
+
 retry:
 	ret = drm_modeset_lock(&config->connection_mutex, ctx);
 	if (ret)

@@ -10441,24 +10444,15 @@ retry:
 	 */
 
 	/* See if we already have a CRTC for this connector */
-	if (encoder->crtc) {
-		crtc = encoder->crtc;
+	if (connector->state->crtc) {
+		crtc = connector->state->crtc;
 
 		ret = drm_modeset_lock(&crtc->mutex, ctx);
 		if (ret)
 			goto fail;
-		ret = drm_modeset_lock(&crtc->primary->mutex, ctx);
-		if (ret)
-			goto fail;
-
-		old->dpms_mode = connector->dpms;
-		old->load_detect_temp = false;
 
 		/* Make sure the crtc and connector are running */
-		if (connector->dpms != DRM_MODE_DPMS_ON)
-			connector->funcs->dpms(connector, DRM_MODE_DPMS_ON);
-
-		return true;
+		goto found;
 	}
 
 	/* Find an unused one (if possible) */

@@ -10466,8 +10460,15 @@ retry:
 		i++;
 		if (!(encoder->possible_crtcs & (1 << i)))
 			continue;
-		if (possible_crtc->state->enable)
+
+		ret = drm_modeset_lock(&possible_crtc->mutex, ctx);
+		if (ret)
+			goto fail;
+
+		if (possible_crtc->state->enable) {
+			drm_modeset_unlock(&possible_crtc->mutex);
 			continue;
+		}
 
 		crtc = possible_crtc;
 		break;

@@ -10481,23 +10482,22 @@ retry:
 		goto fail;
 	}
 
-	ret = drm_modeset_lock(&crtc->mutex, ctx);
-	if (ret)
-		goto fail;
+found:
+	intel_crtc = to_intel_crtc(crtc);
+
 	ret = drm_modeset_lock(&crtc->primary->mutex, ctx);
 	if (ret)
 		goto fail;
 
-	intel_crtc = to_intel_crtc(crtc);
-	old->dpms_mode = connector->dpms;
-	old->load_detect_temp = true;
-	old->release_fb = NULL;
-
 	state = drm_atomic_state_alloc(dev);
-	if (!state)
-		return false;
+	restore_state = drm_atomic_state_alloc(dev);
+	if (!state || !restore_state) {
+		ret = -ENOMEM;
+		goto fail;
+	}
 
 	state->acquire_ctx = ctx;
+	restore_state->acquire_ctx = ctx;
 
 	connector_state = drm_atomic_get_connector_state(state, connector);
 	if (IS_ERR(connector_state)) {

@@ -10505,7 +10505,9 @@ retry:
 		goto fail;
 	}
 
-	connector_state->crtc = crtc;
+	ret = drm_atomic_set_crtc_for_connector(connector_state, crtc);
+	if (ret)
+		goto fail;
 
 	crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
 	if (IS_ERR(crtc_state)) {

@@ -10529,7 +10531,6 @@ retry:
 	if (fb == NULL) {
 		DRM_DEBUG_KMS("creating tmp fb for load-detection\n");
 		fb = intel_framebuffer_create_for_mode(dev, mode, 24, 32);
-		old->release_fb = fb;
 	} else
 		DRM_DEBUG_KMS("reusing fbdev for load-detection framebuffer\n");
 	if (IS_ERR(fb)) {

@@ -10541,15 +10542,28 @@ retry:
 	if (ret)
 		goto fail;
 
-	drm_mode_copy(&crtc_state->base.mode, mode);
+	drm_framebuffer_unreference(fb);
+
+	ret = drm_atomic_set_mode_for_crtc(&crtc_state->base, mode);
+	if (ret)
+		goto fail;
+
+	ret = PTR_ERR_OR_ZERO(drm_atomic_get_connector_state(restore_state, connector));
+	if (!ret)
+		ret = PTR_ERR_OR_ZERO(drm_atomic_get_crtc_state(restore_state, crtc));
+	if (!ret)
+		ret = PTR_ERR_OR_ZERO(drm_atomic_get_plane_state(restore_state, crtc->primary));
+	if (ret) {
+		DRM_DEBUG_KMS("Failed to create a copy of old state to restore: %i\n", ret);
+		goto fail;
+	}
 
 	if (drm_atomic_commit(state)) {
 		DRM_DEBUG_KMS("failed to set mode on load-detect pipe\n");
-		if (old->release_fb)
-			old->release_fb->funcs->destroy(old->release_fb);
 		goto fail;
 	}
-	crtc->primary->crtc = crtc;
+
+	old->restore_state = restore_state;
 
 	/* let the connector get through one full cycle before testing */
 	intel_wait_for_vblank(dev, intel_crtc->pipe);

@@ -10557,7 +10571,8 @@ retry:
 
 fail:
 	drm_atomic_state_free(state);
-	state = NULL;
+	drm_atomic_state_free(restore_state);
+	restore_state = state = NULL;
 
 	if (ret == -EDEADLK) {
 		drm_modeset_backoff(ctx);

@@ -10571,14 +10586,12 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
 				    struct intel_load_detect_pipe *old,
 				    struct drm_modeset_acquire_ctx *ctx)
 {
-	struct drm_device *dev = connector->dev;
 	struct intel_encoder *intel_encoder =
 		intel_attached_encoder(connector);
 	struct drm_encoder *encoder = &intel_encoder->base;
-	struct drm_crtc *crtc = encoder->crtc;
+	struct drm_crtc *crtc = connector->state->crtc;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	struct drm_atomic_state *state;
-	struct drm_connector_state *connector_state;
+	struct drm_atomic_state *state = old->restore_state;
 	struct intel_crtc_state *crtc_state;
 	int ret;
 

@@ -10586,50 +10599,17 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
 		      connector->base.id, connector->name,
 		      encoder->base.id, encoder->name);
 
-	if (old->load_detect_temp) {
-		state = drm_atomic_state_alloc(dev);
-		if (!state)
-			goto fail;
-
-		state->acquire_ctx = ctx;
-
-		connector_state = drm_atomic_get_connector_state(state, connector);
-		if (IS_ERR(connector_state))
-			goto fail;
-
-		crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
-		if (IS_ERR(crtc_state))
-			goto fail;
-
-		connector_state->crtc = NULL;
-
-		crtc_state->base.enable = crtc_state->base.active = false;
-
-		ret = intel_modeset_setup_plane_state(state, crtc, NULL, NULL,
-						      0, 0);
-		if (ret)
-			goto fail;
-
-		ret = drm_atomic_commit(state);
-		if (ret)
-			goto fail;
-
-		if (old->release_fb) {
-			drm_framebuffer_unregister_private(old->release_fb);
-			drm_framebuffer_unreference(old->release_fb);
-		}

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

-	return;
-fail:
-	DRM_DEBUG_KMS("Couldn't release load detect pipe.\n");
-	drm_atomic_state_free(state);
+	ret = drm_atomic_commit(state);
+	if (ret) {
+		DRM_DEBUG_KMS("Couldn't release load detect pipe: %i\n", ret);
+		drm_atomic_state_free(state);
+	}
 }
 
 static int i9xx_pll_refclk(struct drm_device *dev,

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

index 93ba14a3bb76..7d10ca7797a9 100644
@@ -912,9 +912,7 @@ struct intel_unpin_work {
 };
 
 struct intel_load_detect_pipe {
-	struct drm_framebuffer *release_fb;
-	bool load_detect_temp;
-	int dpms_mode;
+	struct drm_atomic_state *restore_state;
 };
 
 static inline struct intel_encoder *

Quoted 239 lines

drm/i915: Use atomic state to obtain load detection crtc, v2.

Instead of restoring dpms and a flag for whether a temp fb is allocated duplicate an atomic state before the new state is committed, and commit it the old state in intel_release_load_detect_pipe.

Changes since v1:
- Use a real atomic state.

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

---

drivers/gpu/drm/i915/intel_display.c +49 -69

index 4d8c9f7857db..c7ba8f4a971e 100644
@@ -10361,6 +10361,7 @@ mode_fits_in_fbdev(struct drm_device *dev,
 	if (obj->base.size < mode->vdisplay * fb->pitches[0])
 		return NULL;
 
+	drm_framebuffer_reference(fb);
 	return fb;
 #else
 	return NULL;

@@ -10416,7 +10417,7 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector,
 	struct drm_device *dev = encoder->dev;
 	struct drm_framebuffer *fb;
 	struct drm_mode_config *config = &dev->mode_config;
-	struct drm_atomic_state *state = NULL;
+	struct drm_atomic_state *state = NULL, *restore_state = NULL;
 	struct drm_connector_state *connector_state;
 	struct intel_crtc_state *crtc_state;
 	int ret, i = -1;

@@ -10425,6 +10426,8 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector,
 		      connector->base.id, connector->name,
 		      encoder->base.id, encoder->name);
 
+	old->restore_state = NULL;
+
 retry:
 	ret = drm_modeset_lock(&config->connection_mutex, ctx);
 	if (ret)

@@ -10441,24 +10444,15 @@ retry:
 	 */
 
 	/* See if we already have a CRTC for this connector */
-	if (encoder->crtc) {
-		crtc = encoder->crtc;
+	if (connector->state->crtc) {
+		crtc = connector->state->crtc;
 
 		ret = drm_modeset_lock(&crtc->mutex, ctx);
 		if (ret)
 			goto fail;
-		ret = drm_modeset_lock(&crtc->primary->mutex, ctx);
-		if (ret)
-			goto fail;
-
-		old->dpms_mode = connector->dpms;
-		old->load_detect_temp = false;
 
 		/* Make sure the crtc and connector are running */
-		if (connector->dpms != DRM_MODE_DPMS_ON)
-			connector->funcs->dpms(connector, DRM_MODE_DPMS_ON);
-
-		return true;
+		goto found;
 	}
 
 	/* Find an unused one (if possible) */

@@ -10466,8 +10460,15 @@ retry:
 		i++;
 		if (!(encoder->possible_crtcs & (1 << i)))
 			continue;
-		if (possible_crtc->state->enable)
+
+		ret = drm_modeset_lock(&possible_crtc->mutex, ctx);
+		if (ret)
+			goto fail;
+
+		if (possible_crtc->state->enable) {
+			drm_modeset_unlock(&possible_crtc->mutex);
 			continue;
+		}
 
 		crtc = possible_crtc;
 		break;

@@ -10481,23 +10482,22 @@ retry:
 		goto fail;
 	}
 
-	ret = drm_modeset_lock(&crtc->mutex, ctx);
-	if (ret)
-		goto fail;
+found:
+	intel_crtc = to_intel_crtc(crtc);
+
 	ret = drm_modeset_lock(&crtc->primary->mutex, ctx);
 	if (ret)
 		goto fail;
 
-	intel_crtc = to_intel_crtc(crtc);
-	old->dpms_mode = connector->dpms;
-	old->load_detect_temp = true;
-	old->release_fb = NULL;
-
 	state = drm_atomic_state_alloc(dev);
-	if (!state)
-		return false;
+	restore_state = drm_atomic_state_alloc(dev);
+	if (!state || !restore_state) {
+		ret = -ENOMEM;
+		goto fail;
+	}
 
 	state->acquire_ctx = ctx;
+	restore_state->acquire_ctx = ctx;
 
 	connector_state = drm_atomic_get_connector_state(state, connector);
 	if (IS_ERR(connector_state)) {

@@ -10505,7 +10505,9 @@ retry:
 		goto fail;
 	}
 
-	connector_state->crtc = crtc;
+	ret = drm_atomic_set_crtc_for_connector(connector_state, crtc);
+	if (ret)
+		goto fail;
 
 	crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
 	if (IS_ERR(crtc_state)) {

@@ -10529,7 +10531,6 @@ retry:
 	if (fb == NULL) {
 		DRM_DEBUG_KMS("creating tmp fb for load-detection\n");
 		fb = intel_framebuffer_create_for_mode(dev, mode, 24, 32);
-		old->release_fb = fb;
 	} else
 		DRM_DEBUG_KMS("reusing fbdev for load-detection framebuffer\n");
 	if (IS_ERR(fb)) {

@@ -10541,15 +10542,28 @@ retry:
 	if (ret)
 		goto fail;
 
-	drm_mode_copy(&crtc_state->base.mode, mode);
+	drm_framebuffer_unreference(fb);
+
+	ret = drm_atomic_set_mode_for_crtc(&crtc_state->base, mode);
+	if (ret)
+		goto fail;
+
+	ret = PTR_ERR_OR_ZERO(drm_atomic_get_connector_state(restore_state, connector));
+	if (!ret)
+		ret = PTR_ERR_OR_ZERO(drm_atomic_get_crtc_state(restore_state, crtc));
+	if (!ret)
+		ret = PTR_ERR_OR_ZERO(drm_atomic_get_plane_state(restore_state, crtc->primary));
+	if (ret) {
+		DRM_DEBUG_KMS("Failed to create a copy of old state to restore: %i\n", ret);
+		goto fail;
+	}
 
 	if (drm_atomic_commit(state)) {
 		DRM_DEBUG_KMS("failed to set mode on load-detect pipe\n");
-		if (old->release_fb)
-			old->release_fb->funcs->destroy(old->release_fb);
 		goto fail;
 	}
-	crtc->primary->crtc = crtc;
+
+	old->restore_state = restore_state;
 
 	/* let the connector get through one full cycle before testing */
 	intel_wait_for_vblank(dev, intel_crtc->pipe);

@@ -10557,7 +10571,8 @@ retry:
 
 fail:
 	drm_atomic_state_free(state);
-	state = NULL;
+	drm_atomic_state_free(restore_state);
+	restore_state = state = NULL;
 
 	if (ret == -EDEADLK) {
 		drm_modeset_backoff(ctx);

@@ -10571,14 +10586,12 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
 				    struct intel_load_detect_pipe *old,
 				    struct drm_modeset_acquire_ctx *ctx)
 {
-	struct drm_device *dev = connector->dev;
 	struct intel_encoder *intel_encoder =
 		intel_attached_encoder(connector);
 	struct drm_encoder *encoder = &intel_encoder->base;
-	struct drm_crtc *crtc = encoder->crtc;
+	struct drm_crtc *crtc = connector->state->crtc;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	struct drm_atomic_state *state;
-	struct drm_connector_state *connector_state;
+	struct drm_atomic_state *state = old->restore_state;
 	struct intel_crtc_state *crtc_state;
 	int ret;
 

@@ -10586,50 +10599,17 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
 		      connector->base.id, connector->name,
 		      encoder->base.id, encoder->name);
 
-	if (old->load_detect_temp) {
-		state = drm_atomic_state_alloc(dev);
-		if (!state)
-			goto fail;
-
-		state->acquire_ctx = ctx;
-
-		connector_state = drm_atomic_get_connector_state(state, connector);
-		if (IS_ERR(connector_state))
-			goto fail;
-
-		crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
-		if (IS_ERR(crtc_state))
-			goto fail;
-
-		connector_state->crtc = NULL;
-
-		crtc_state->base.enable = crtc_state->base.active = false;
-
-		ret = intel_modeset_setup_plane_state(state, crtc, NULL, NULL,
-						      0, 0);
-		if (ret)
-			goto fail;
-
-		ret = drm_atomic_commit(state);
-		if (ret)
-			goto fail;
-
-		if (old->release_fb) {
-			drm_framebuffer_unregister_private(old->release_fb);
-			drm_framebuffer_unreference(old->release_fb);
-		}
-
+	if (!state)
 		return;
-	}
 

drivers/gpu/drm/i915/intel_display.c void intel_release_load_detect_pipe(struct drm_connector *connector, · line 10606

Quoted 5 lines

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

-	/* Switch crtc and encoder back off if necessary */
-	if (old->dpms_mode != DRM_MODE_DPMS_ON)
-		connector->funcs->dpms(connector, old->dpms_mode);
+	crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
+	crtc_state->shared_dpll = to_intel_crtc_state(crtc->state)->shared_dpll;

Hmm. What's this shared_dpll stuff?

Op 02-02-16 om 18:32 schreef Ville Syrjälä:

Quoted 240 lines · Maarten Lankhorst

Quoted 239 lines

drm/i915: Use atomic state to obtain load detection crtc, v2.

Instead of restoring dpms and a flag for whether a temp fb is allocated duplicate an atomic state before the new state is committed, and commit it the old state in intel_release_load_detect_pipe.

Changes since v1:
- Use a real atomic state.

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

---

drivers/gpu/drm/i915/intel_display.c +49 -69

index 4d8c9f7857db..c7ba8f4a971e 100644
@@ -10361,6 +10361,7 @@ mode_fits_in_fbdev(struct drm_device *dev,
 	if (obj->base.size < mode->vdisplay * fb->pitches[0])
 		return NULL;
 
+	drm_framebuffer_reference(fb);
 	return fb;
 #else
 	return NULL;

@@ -10416,7 +10417,7 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector,
 	struct drm_device *dev = encoder->dev;
 	struct drm_framebuffer *fb;
 	struct drm_mode_config *config = &dev->mode_config;
-	struct drm_atomic_state *state = NULL;
+	struct drm_atomic_state *state = NULL, *restore_state = NULL;
 	struct drm_connector_state *connector_state;
 	struct intel_crtc_state *crtc_state;
 	int ret, i = -1;

@@ -10425,6 +10426,8 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector,
 		      connector->base.id, connector->name,
 		      encoder->base.id, encoder->name);
 
+	old->restore_state = NULL;
+
 retry:
 	ret = drm_modeset_lock(&config->connection_mutex, ctx);
 	if (ret)

@@ -10441,24 +10444,15 @@ retry:
 	 */
 
 	/* See if we already have a CRTC for this connector */
-	if (encoder->crtc) {
-		crtc = encoder->crtc;
+	if (connector->state->crtc) {
+		crtc = connector->state->crtc;
 
 		ret = drm_modeset_lock(&crtc->mutex, ctx);
 		if (ret)
 			goto fail;
-		ret = drm_modeset_lock(&crtc->primary->mutex, ctx);
-		if (ret)
-			goto fail;
-
-		old->dpms_mode = connector->dpms;
-		old->load_detect_temp = false;
 
 		/* Make sure the crtc and connector are running */
-		if (connector->dpms != DRM_MODE_DPMS_ON)
-			connector->funcs->dpms(connector, DRM_MODE_DPMS_ON);
-
-		return true;
+		goto found;
 	}
 
 	/* Find an unused one (if possible) */

@@ -10466,8 +10460,15 @@ retry:
 		i++;
 		if (!(encoder->possible_crtcs & (1 << i)))
 			continue;
-		if (possible_crtc->state->enable)
+
+		ret = drm_modeset_lock(&possible_crtc->mutex, ctx);
+		if (ret)
+			goto fail;
+
+		if (possible_crtc->state->enable) {
+			drm_modeset_unlock(&possible_crtc->mutex);
 			continue;
+		}
 
 		crtc = possible_crtc;
 		break;

@@ -10481,23 +10482,22 @@ retry:
 		goto fail;
 	}
 
-	ret = drm_modeset_lock(&crtc->mutex, ctx);
-	if (ret)
-		goto fail;
+found:
+	intel_crtc = to_intel_crtc(crtc);
+
 	ret = drm_modeset_lock(&crtc->primary->mutex, ctx);
 	if (ret)
 		goto fail;
 
-	intel_crtc = to_intel_crtc(crtc);
-	old->dpms_mode = connector->dpms;
-	old->load_detect_temp = true;
-	old->release_fb = NULL;
-
 	state = drm_atomic_state_alloc(dev);
-	if (!state)
-		return false;
+	restore_state = drm_atomic_state_alloc(dev);
+	if (!state || !restore_state) {
+		ret = -ENOMEM;
+		goto fail;
+	}
 
 	state->acquire_ctx = ctx;
+	restore_state->acquire_ctx = ctx;
 
 	connector_state = drm_atomic_get_connector_state(state, connector);
 	if (IS_ERR(connector_state)) {

@@ -10505,7 +10505,9 @@ retry:
 		goto fail;
 	}
 
-	connector_state->crtc = crtc;
+	ret = drm_atomic_set_crtc_for_connector(connector_state, crtc);
+	if (ret)
+		goto fail;
 
 	crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
 	if (IS_ERR(crtc_state)) {

@@ -10529,7 +10531,6 @@ retry:
 	if (fb == NULL) {
 		DRM_DEBUG_KMS("creating tmp fb for load-detection\n");
 		fb = intel_framebuffer_create_for_mode(dev, mode, 24, 32);
-		old->release_fb = fb;
 	} else
 		DRM_DEBUG_KMS("reusing fbdev for load-detection framebuffer\n");
 	if (IS_ERR(fb)) {

@@ -10541,15 +10542,28 @@ retry:
 	if (ret)
 		goto fail;
 
-	drm_mode_copy(&crtc_state->base.mode, mode);
+	drm_framebuffer_unreference(fb);
+
+	ret = drm_atomic_set_mode_for_crtc(&crtc_state->base, mode);
+	if (ret)
+		goto fail;
+
+	ret = PTR_ERR_OR_ZERO(drm_atomic_get_connector_state(restore_state, connector));
+	if (!ret)
+		ret = PTR_ERR_OR_ZERO(drm_atomic_get_crtc_state(restore_state, crtc));
+	if (!ret)
+		ret = PTR_ERR_OR_ZERO(drm_atomic_get_plane_state(restore_state, crtc->primary));
+	if (ret) {
+		DRM_DEBUG_KMS("Failed to create a copy of old state to restore: %i\n", ret);
+		goto fail;
+	}
 
 	if (drm_atomic_commit(state)) {
 		DRM_DEBUG_KMS("failed to set mode on load-detect pipe\n");
-		if (old->release_fb)
-			old->release_fb->funcs->destroy(old->release_fb);
 		goto fail;
 	}
-	crtc->primary->crtc = crtc;
+
+	old->restore_state = restore_state;
 
 	/* let the connector get through one full cycle before testing */
 	intel_wait_for_vblank(dev, intel_crtc->pipe);

@@ -10557,7 +10571,8 @@ retry:
 
 fail:
 	drm_atomic_state_free(state);
-	state = NULL;
+	drm_atomic_state_free(restore_state);
+	restore_state = state = NULL;
 
 	if (ret == -EDEADLK) {
 		drm_modeset_backoff(ctx);

@@ -10571,14 +10586,12 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
 				    struct intel_load_detect_pipe *old,
 				    struct drm_modeset_acquire_ctx *ctx)
 {
-	struct drm_device *dev = connector->dev;
 	struct intel_encoder *intel_encoder =
 		intel_attached_encoder(connector);
 	struct drm_encoder *encoder = &intel_encoder->base;
-	struct drm_crtc *crtc = encoder->crtc;
+	struct drm_crtc *crtc = connector->state->crtc;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	struct drm_atomic_state *state;
-	struct drm_connector_state *connector_state;
+	struct drm_atomic_state *state = old->restore_state;
 	struct intel_crtc_state *crtc_state;
 	int ret;
 

@@ -10586,50 +10599,17 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
 		      connector->base.id, connector->name,
 		      encoder->base.id, encoder->name);
 
-	if (old->load_detect_temp) {
-		state = drm_atomic_state_alloc(dev);
-		if (!state)
-			goto fail;
-
-		state->acquire_ctx = ctx;
-
-		connector_state = drm_atomic_get_connector_state(state, connector);
-		if (IS_ERR(connector_state))
-			goto fail;
-
-		crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
-		if (IS_ERR(crtc_state))
-			goto fail;
-
-		connector_state->crtc = NULL;
-
-		crtc_state->base.enable = crtc_state->base.active = false;
-
-		ret = intel_modeset_setup_plane_state(state, crtc, NULL, NULL,
-						      0, 0);
-		if (ret)
-			goto fail;
-
-		ret = drm_atomic_commit(state);
-		if (ret)
-			goto fail;
-
-		if (old->release_fb) {
-			drm_framebuffer_unregister_private(old->release_fb);
-			drm_framebuffer_unreference(old->release_fb);
-		}
-
+	if (!state)
 		return;
-	}
 

Quoted 6 lines

drivers/gpu/drm/i915/intel_display.c void intel_release_load_detect_pipe(struct drm_connector *connector, · line 10606

Quoted 5 lines

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

-	/* Switch crtc and encoder back off if necessary */
-	if (old->dpms_mode != DRM_MODE_DPMS_ON)
-		connector->funcs->dpms(connector, old->dpms_mode);
+	crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
+	crtc_state->shared_dpll = to_intel_crtc_state(crtc->state)->shared_dpll;

Hmm. What's this shared_dpll stuff?

It's for intel_modeset_clear_plls, normally we duplicate the new state and it's handled correctly, but when we duplicate the old state the pll may be different and unexpected behavior could occur otherwise.

Quoted 247 lines

Op 02-02-16 om 18:32 schreef Ville Syrjälä:

Quoted 246 lines · Maarten Lankhorst

Quoted 234 lines

drm/i915: Use atomic state to obtain load detection crtc, v2.

Instead of restoring dpms and a flag for whether a temp fb is allocated duplicate an atomic state before the new state is committed, and commit it the old state in intel_release_load_detect_pipe.

Changes since v1:
- Use a real atomic state.

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

---

drivers/gpu/drm/i915/intel_display.c +48 -67

index 4d8c9f7857db..c7ba8f4a971e 100644
@@ -10361,6 +10361,7 @@ mode_fits_in_fbdev(struct drm_device *dev,
 	if (obj->base.size < mode->vdisplay * fb->pitches[0])
 		return NULL;
 
+	drm_framebuffer_reference(fb);
 	return fb;
 #else
 	return NULL;

@@ -10416,7 +10417,7 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector,
 	struct drm_device *dev = encoder->dev;
 	struct drm_framebuffer *fb;
 	struct drm_mode_config *config = &dev->mode_config;
-	struct drm_atomic_state *state = NULL;
+	struct drm_atomic_state *state = NULL, *restore_state = NULL;
 	struct drm_connector_state *connector_state;
 	struct intel_crtc_state *crtc_state;
 	int ret, i = -1;

@@ -10425,6 +10426,8 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector,
 		      connector->base.id, connector->name,
 		      encoder->base.id, encoder->name);
 
+	old->restore_state = NULL;
+
 retry:
 	ret = drm_modeset_lock(&config->connection_mutex, ctx);
 	if (ret)

@@ -10441,24 +10444,15 @@ retry:
 	 */
 
 	/* See if we already have a CRTC for this connector */
-	if (encoder->crtc) {
-		crtc = encoder->crtc;
+	if (connector->state->crtc) {
+		crtc = connector->state->crtc;
 
 		ret = drm_modeset_lock(&crtc->mutex, ctx);
 		if (ret)
 			goto fail;
-		ret = drm_modeset_lock(&crtc->primary->mutex, ctx);
-		if (ret)
-			goto fail;
-
-		old->dpms_mode = connector->dpms;
-		old->load_detect_temp = false;
 
 		/* Make sure the crtc and connector are running */
-		if (connector->dpms != DRM_MODE_DPMS_ON)
-			connector->funcs->dpms(connector, DRM_MODE_DPMS_ON);
-
-		return true;
+		goto found;
 	}
 
 	/* Find an unused one (if possible) */

@@ -10466,8 +10460,15 @@ retry:
 		i++;
 		if (!(encoder->possible_crtcs & (1 << i)))
 			continue;
-		if (possible_crtc->state->enable)
+
+		ret = drm_modeset_lock(&possible_crtc->mutex, ctx);
+		if (ret)
+			goto fail;
+
+		if (possible_crtc->state->enable) {
+			drm_modeset_unlock(&possible_crtc->mutex);
 			continue;
+		}
 
 		crtc = possible_crtc;
 		break;

@@ -10481,23 +10482,22 @@ retry:
 		goto fail;
 	}
 
-	ret = drm_modeset_lock(&crtc->mutex, ctx);
-	if (ret)
-		goto fail;
+found:
+	intel_crtc = to_intel_crtc(crtc);
+
 	ret = drm_modeset_lock(&crtc->primary->mutex, ctx);
 	if (ret)
 		goto fail;
 
-	intel_crtc = to_intel_crtc(crtc);
-	old->dpms_mode = connector->dpms;
-	old->load_detect_temp = true;
-	old->release_fb = NULL;
-
 	state = drm_atomic_state_alloc(dev);
-	if (!state)
-		return false;
+	restore_state = drm_atomic_state_alloc(dev);
+	if (!state || !restore_state) {
+		ret = -ENOMEM;
+		goto fail;
+	}
 
 	state->acquire_ctx = ctx;
+	restore_state->acquire_ctx = ctx;
 
 	connector_state = drm_atomic_get_connector_state(state, connector);
 	if (IS_ERR(connector_state)) {

@@ -10505,7 +10505,9 @@ retry:
 		goto fail;
 	}
 
-	connector_state->crtc = crtc;
+	ret = drm_atomic_set_crtc_for_connector(connector_state, crtc);
+	if (ret)
+		goto fail;
 
 	crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
 	if (IS_ERR(crtc_state)) {

@@ -10529,7 +10531,6 @@ retry:
 	if (fb == NULL) {
 		DRM_DEBUG_KMS("creating tmp fb for load-detection\n");
 		fb = intel_framebuffer_create_for_mode(dev, mode, 24, 32);
-		old->release_fb = fb;
 	} else
 		DRM_DEBUG_KMS("reusing fbdev for load-detection framebuffer\n");
 	if (IS_ERR(fb)) {

@@ -10541,15 +10542,28 @@ retry:
 	if (ret)
 		goto fail;
 
-	drm_mode_copy(&crtc_state->base.mode, mode);
+	drm_framebuffer_unreference(fb);
+
+	ret = drm_atomic_set_mode_for_crtc(&crtc_state->base, mode);
+	if (ret)
+		goto fail;
+
+	ret = PTR_ERR_OR_ZERO(drm_atomic_get_connector_state(restore_state, connector));
+	if (!ret)
+		ret = PTR_ERR_OR_ZERO(drm_atomic_get_crtc_state(restore_state, crtc));
+	if (!ret)
+		ret = PTR_ERR_OR_ZERO(drm_atomic_get_plane_state(restore_state, crtc->primary));
+	if (ret) {
+		DRM_DEBUG_KMS("Failed to create a copy of old state to restore: %i\n", ret);
+		goto fail;
+	}
 
 	if (drm_atomic_commit(state)) {
 		DRM_DEBUG_KMS("failed to set mode on load-detect pipe\n");
-		if (old->release_fb)
-			old->release_fb->funcs->destroy(old->release_fb);
 		goto fail;
 	}
-	crtc->primary->crtc = crtc;
+
+	old->restore_state = restore_state;
 
 	/* let the connector get through one full cycle before testing */
 	intel_wait_for_vblank(dev, intel_crtc->pipe);

@@ -10557,7 +10571,8 @@ retry:
 
 fail:
 	drm_atomic_state_free(state);
-	state = NULL;
+	drm_atomic_state_free(restore_state);
+	restore_state = state = NULL;
 
 	if (ret == -EDEADLK) {
 		drm_modeset_backoff(ctx);

@@ -10571,14 +10586,12 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
 				    struct intel_load_detect_pipe *old,
 				    struct drm_modeset_acquire_ctx *ctx)
 {
-	struct drm_device *dev = connector->dev;
 	struct intel_encoder *intel_encoder =
 		intel_attached_encoder(connector);
 	struct drm_encoder *encoder = &intel_encoder->base;
-	struct drm_crtc *crtc = encoder->crtc;
+	struct drm_crtc *crtc = connector->state->crtc;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	struct drm_atomic_state *state;
-	struct drm_connector_state *connector_state;
+	struct drm_atomic_state *state = old->restore_state;
 	struct intel_crtc_state *crtc_state;
 	int ret;
 

@@ -10586,50 +10599,17 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
 		      connector->base.id, connector->name,
 		      encoder->base.id, encoder->name);
 
-	if (old->load_detect_temp) {
-		state = drm_atomic_state_alloc(dev);
-		if (!state)
-			goto fail;
-
-		state->acquire_ctx = ctx;
-
-		connector_state = drm_atomic_get_connector_state(state, connector);
-		if (IS_ERR(connector_state))
-			goto fail;
-
-		crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
-		if (IS_ERR(crtc_state))
-			goto fail;
-
-		connector_state->crtc = NULL;
-
-		crtc_state->base.enable = crtc_state->base.active = false;
-
-		ret = intel_modeset_setup_plane_state(state, crtc, NULL, NULL,
-						      0, 0);
-		if (ret)
-			goto fail;
-
-		ret = drm_atomic_commit(state);
-		if (ret)
-			goto fail;
-
-		if (old->release_fb) {
-			drm_framebuffer_unregister_private(old->release_fb);
-			drm_framebuffer_unreference(old->release_fb);
-		}

drivers/gpu/drm/i915/intel_display.c void intel_release_load_detect_pipe(struct drm_connector *connector, · line 10606

Quoted 10 lines

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

-
+	if (!state)
 		return;
-	}
 
-	/* Switch crtc and encoder back off if necessary */
-	if (old->dpms_mode != DRM_MODE_DPMS_ON)
-		connector->funcs->dpms(connector, old->dpms_mode);
+	crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
+	crtc_state->shared_dpll = to_intel_crtc_state(crtc->state)->shared_dpll;

Hmm. What's this shared_dpll stuff?

Quoted 3 lines

It's for intel_modeset_clear_plls, normally we duplicate the new state and it's handled correctly, but when we duplicate the old state the pll may be different and unexpected behavior could occur otherwise.

Sounds fragile. Any chance of making it do the right thing (tm) without having to jump through such hoops?

Instead of duplicating the functionality now that we no longer need to preserve dpll state we can move to using the upstream suspend helper.

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

---
 drivers/gpu/drm/drm_atomic_helper.c  |   3 +
 drivers/gpu/drm/i915/i915_drv.c      |   8 ---
 drivers/gpu/drm/i915/i915_drv.h      |   1 +
 drivers/gpu/drm/i915/intel_display.c | 117 ++++++++++++-----------------------
 4 files changed, 42 insertions(+), 87 deletions(-)

drivers/gpu/drm/drm_atomic_helper.c +3 -0

index 2b430b05f35d..a2d3b094f27c 100644
@@ -2102,6 +2102,9 @@ int drm_atomic_helper_resume(struct drm_device *dev,
 	err = drm_atomic_commit(state);
 	drm_modeset_unlock_all(dev);
 
+	if (err)
+		drm_atomic_state_free(state);
+
 	return err;
 }
 EXPORT_SYMBOL(drm_atomic_helper_resume);

drivers/gpu/drm/i915/i915_drv.c +0 -8

index 11d8414edbbe..fc9b552bfcd1 100644
@@ -600,13 +600,7 @@ static int i915_drm_suspend(struct drm_device *dev)
 
 	intel_suspend_gt_powersave(dev);
 
-	/*
-	 * Disable CRTCs directly since we want to preserve sw state
-	 * for _thaw. Also, power gate the CRTC power wells.
-	 */
-	drm_modeset_lock_all(dev);
 	intel_display_suspend(dev);
-	drm_modeset_unlock_all(dev);
 
 	intel_dp_mst_suspend(dev);
 

@@ -761,9 +755,7 @@ static int i915_drm_resume(struct drm_device *dev)
 		dev_priv->display.hpd_irq_setup(dev);
 	spin_unlock_irq(&dev_priv->irq_lock);
 
-	drm_modeset_lock_all(dev);
 	intel_display_resume(dev);
-	drm_modeset_unlock_all(dev);
 
 	intel_dp_mst_resume(dev);
 

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

index 8216665405eb..ef289514b97e 100644
@@ -1848,6 +1848,7 @@ struct drm_i915_private {
 
 	enum modeset_restore modeset_restore;
 	struct mutex modeset_restore_lock;
+	struct drm_atomic_state *modeset_restore_state;
 
 	struct list_head vm_list; /* Global list of all address spaces */
 	struct i915_gtt gtt; /* VM representing the global address space */

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

index e496c130364d..4c91fd1c5222 100644
@@ -6393,58 +6393,19 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
  */
 int intel_display_suspend(struct drm_device *dev)
 {
-	struct drm_mode_config *config = &dev->mode_config;
-	struct drm_modeset_acquire_ctx *ctx = config->acquire_ctx;
-	struct drm_atomic_state *state;
-	struct drm_crtc *crtc;
-	unsigned crtc_mask = 0;
-	int ret = 0;
-
-	if (WARN_ON(!ctx))
-		return 0;
-
-	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;
-
-	for_each_crtc(dev, crtc) {
-		struct drm_crtc_state *crtc_state =
-			drm_atomic_get_crtc_state(state, crtc);
-
-		ret = PTR_ERR_OR_ZERO(crtc_state);
-		if (ret)
-			goto free;
-
-		if (!crtc_state->active)
-			continue;
-
-		crtc_state->active = false;
-		crtc_mask |= 1 << drm_crtc_index(crtc);
-	}
-
-	if (crtc_mask) {
-		ret = drm_atomic_commit(state);
-
-		if (!ret) {
-			for_each_crtc(dev, crtc)
-				if (crtc_mask & (1 << drm_crtc_index(crtc)))
-					crtc->state->active = true;
-
-			return ret;
-		}
-	}
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	int ret;

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

 void intel_encoder_destroy(struct drm_encoder *encoder)
 {
 	struct intel_encoder *intel_encoder = to_intel_encoder(encoder);

@@ -15909,51 +15870,49 @@ intel_modeset_setup_hw_state(struct drm_device *dev)
 
 void intel_display_resume(struct drm_device *dev)
 {
-	struct drm_atomic_state *state = drm_atomic_state_alloc(dev);
-	struct intel_connector *conn;

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

+	i915_redisable_vga(dev);
+	dev_priv->modeset_restore_state = NULL;
 
 	if (!state)
 		return;
 
-	state->acquire_ctx = dev->mode_config.acquire_ctx;
-
-	for_each_crtc(dev, crtc) {
-		struct drm_crtc_state *crtc_state =
-			drm_atomic_get_crtc_state(state, crtc);
-
-		ret = PTR_ERR_OR_ZERO(crtc_state);
-		if (ret)
-			goto err;
+	drm_modeset_acquire_init(&ctx, 0);
+	state->acquire_ctx = &ctx;

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

-	for_each_intel_plane(dev, plane) {
-		ret = PTR_ERR_OR_ZERO(drm_atomic_get_plane_state(state, &plane->base));
-		if (ret)
-			goto err;
-	}
+retry:
+	ret = drm_modeset_lock_all_ctx(dev, &ctx);
+	if (ret == 0)
+		ret = drm_atomic_commit(state);
 
-	for_each_intel_connector(dev, conn) {
-		ret = PTR_ERR_OR_ZERO(drm_atomic_get_connector_state(state, &conn->base));
-		if (ret)
-			goto err;
+	if (ret == -EDEADLK) {
+		drm_modeset_backoff(&ctx);
+		goto retry;
 	}
 
-	intel_modeset_setup_hw_state(dev);
-
-	i915_redisable_vga(dev);
-	ret = drm_atomic_commit(state);
-	if (!ret)
-		return;
+	drm_modeset_drop_locks(&ctx);
+	drm_modeset_acquire_fini(&ctx);
 
-err:
-	DRM_ERROR("Restoring old state failed with %i\n", ret);
-	drm_atomic_state_free(state);
+	if (ret) {
+		DRM_ERROR("Restoring old state failed with %i\n", ret);
+		drm_atomic_state_free(state);
+	}
 }
 
 void intel_modeset_gem_init(struct drm_device *dev)

Signature · 1 line

2.1.0

Quoted 119 lines

Instead of duplicating the functionality now that we no longer need to preserve dpll state we can move to using the upstream suspend helper.

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

---
 drivers/gpu/drm/drm_atomic_helper.c  |   3 +
 drivers/gpu/drm/i915/i915_drv.c      |   8 ---
 drivers/gpu/drm/i915/i915_drv.h      |   1 +
 drivers/gpu/drm/i915/intel_display.c | 117 ++++++++++++-----------------------
 4 files changed, 42 insertions(+), 87 deletions(-)

drivers/gpu/drm/drm_atomic_helper.c +3 -0

index 2b430b05f35d..a2d3b094f27c 100644
@@ -2102,6 +2102,9 @@ int drm_atomic_helper_resume(struct drm_device *dev,
 	err = drm_atomic_commit(state);
 	drm_modeset_unlock_all(dev);
 
+	if (err)
+		drm_atomic_state_free(state);
+
 	return err;
 }
 EXPORT_SYMBOL(drm_atomic_helper_resume);

drivers/gpu/drm/i915/i915_drv.c +0 -8

index 11d8414edbbe..fc9b552bfcd1 100644
@@ -600,13 +600,7 @@ static int i915_drm_suspend(struct drm_device *dev)
 
 	intel_suspend_gt_powersave(dev);
 
-	/*
-	 * Disable CRTCs directly since we want to preserve sw state
-	 * for _thaw. Also, power gate the CRTC power wells.
-	 */
-	drm_modeset_lock_all(dev);
 	intel_display_suspend(dev);
-	drm_modeset_unlock_all(dev);
 
 	intel_dp_mst_suspend(dev);
 

@@ -761,9 +755,7 @@ static int i915_drm_resume(struct drm_device *dev)
 		dev_priv->display.hpd_irq_setup(dev);
 	spin_unlock_irq(&dev_priv->irq_lock);
 
-	drm_modeset_lock_all(dev);
 	intel_display_resume(dev);
-	drm_modeset_unlock_all(dev);
 
 	intel_dp_mst_resume(dev);
 

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

index 8216665405eb..ef289514b97e 100644
@@ -1848,6 +1848,7 @@ struct drm_i915_private {
 
 	enum modeset_restore modeset_restore;
 	struct mutex modeset_restore_lock;
+	struct drm_atomic_state *modeset_restore_state;
 
 	struct list_head vm_list; /* Global list of all address spaces */
 	struct i915_gtt gtt; /* VM representing the global address space */

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

index e496c130364d..4c91fd1c5222 100644
@@ -6393,58 +6393,19 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
  */
 int intel_display_suspend(struct drm_device *dev)
 {
-	struct drm_mode_config *config = &dev->mode_config;
-	struct drm_modeset_acquire_ctx *ctx = config->acquire_ctx;
-	struct drm_atomic_state *state;
-	struct drm_crtc *crtc;
-	unsigned crtc_mask = 0;
-	int ret = 0;
-
-	if (WARN_ON(!ctx))
-		return 0;
-
-	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;
-
-	for_each_crtc(dev, crtc) {
-		struct drm_crtc_state *crtc_state =
-			drm_atomic_get_crtc_state(state, crtc);
-
-		ret = PTR_ERR_OR_ZERO(crtc_state);
-		if (ret)
-			goto free;
-
-		if (!crtc_state->active)
-			continue;
-
-		crtc_state->active = false;
-		crtc_mask |= 1 << drm_crtc_index(crtc);
-	}
-
-	if (crtc_mask) {
-		ret = drm_atomic_commit(state);
-
-		if (!ret) {
-			for_each_crtc(dev, crtc)
-				if (crtc_mask & (1 << drm_crtc_index(crtc)))
-					crtc->state->active = true;
-
-			return ret;
-		}
-	}
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	int ret;

drivers/gpu/drm/i915/intel_display.c static void intel_crtc_disable_noatomic(struct drm_crtc *crtc) · line 6404

Quoted 10 lines

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

 
-free:
-	if (ret)
+	dev_priv->modeset_restore_state = drm_atomic_helper_suspend(dev);
+	ret = PTR_ERR_OR_ZERO(dev_priv->modeset_restore_state);
+	if (ret) {
 		DRM_ERROR("Suspending crtc's failed with %i\n", ret);
-	drm_atomic_state_free(state);
+		dev_priv->modeset_restore_state = NULL;
+	}

I would use a local state pointer and only assign it to modeset_restore_state once we know it's good.

drivers/gpu/drm/i915/intel_display.c static void intel_crtc_disable_noatomic(struct drm_crtc *crtc) · line 6408

Quoted 4 lines

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

 	return ret;
 }
 
+

Spurious newline.

Quoted 5 lines

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

 void intel_encoder_destroy(struct drm_encoder *encoder)
 {
 	struct intel_encoder *intel_encoder = to_intel_encoder(encoder);

@@ -15909,51 +15870,49 @@ intel_modeset_setup_hw_state(struct drm_device *dev)
 

Quoted 4 lines

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

 void intel_display_resume(struct drm_device *dev)
 {
-	struct drm_atomic_state *state = drm_atomic_state_alloc(dev);
-	struct intel_connector *conn;

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

Quoted 10 lines

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

-	struct intel_plane *plane;
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct drm_atomic_state *state = dev_priv->modeset_restore_state;
+	struct drm_modeset_acquire_ctx ctx;
 	struct drm_crtc *crtc;
-	int ret;
+	struct drm_crtc_state *crtc_state;
+	int ret, i;
+
+	intel_modeset_setup_hw_state(dev);

Isn't something in there going to scream about locking?

Quoted 18 lines

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

+	i915_redisable_vga(dev);
+	dev_priv->modeset_restore_state = NULL;
 
 	if (!state)
 		return;
 
-	state->acquire_ctx = dev->mode_config.acquire_ctx;
-
-	for_each_crtc(dev, crtc) {
-		struct drm_crtc_state *crtc_state =
-			drm_atomic_get_crtc_state(state, crtc);
-
-		ret = PTR_ERR_OR_ZERO(crtc_state);
-		if (ret)
-			goto err;
+	drm_modeset_acquire_init(&ctx, 0);
+	state->acquire_ctx = &ctx;
 

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

Quoted 9 lines

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

-		/* force a restore */
+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		/*
+		 * Force recalculation even if we restore
+		 * current state. With fast modeset this may not result
+		 * in a modeset when the state is compatible.
+		 */
 		crtc_state->mode_changed = true;
 	}

Hmm. Why do we need to set that exactly? If the current state doesn't match the "to be restored" state, shouldn't something somewhere figure out that we really need a modeset?

Op 09-02-16 om 14:37 schreef Ville Syrjälä:

Quoted 144 lines · Maarten Lankhorst

Quoted 119 lines

Instead of duplicating the functionality now that we no longer need to preserve dpll state we can move to using the upstream suspend helper.

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

---
 drivers/gpu/drm/drm_atomic_helper.c  |   3 +
 drivers/gpu/drm/i915/i915_drv.c      |   8 ---
 drivers/gpu/drm/i915/i915_drv.h      |   1 +
 drivers/gpu/drm/i915/intel_display.c | 117 ++++++++++++-----------------------
 4 files changed, 42 insertions(+), 87 deletions(-)

drivers/gpu/drm/drm_atomic_helper.c +3 -0

index 2b430b05f35d..a2d3b094f27c 100644
@@ -2102,6 +2102,9 @@ int drm_atomic_helper_resume(struct drm_device *dev,
 	err = drm_atomic_commit(state);
 	drm_modeset_unlock_all(dev);
 
+	if (err)
+		drm_atomic_state_free(state);
+
 	return err;
 }
 EXPORT_SYMBOL(drm_atomic_helper_resume);

drivers/gpu/drm/i915/i915_drv.c +0 -8

index 11d8414edbbe..fc9b552bfcd1 100644
@@ -600,13 +600,7 @@ static int i915_drm_suspend(struct drm_device *dev)
 
 	intel_suspend_gt_powersave(dev);
 
-	/*
-	 * Disable CRTCs directly since we want to preserve sw state
-	 * for _thaw. Also, power gate the CRTC power wells.
-	 */
-	drm_modeset_lock_all(dev);
 	intel_display_suspend(dev);
-	drm_modeset_unlock_all(dev);
 
 	intel_dp_mst_suspend(dev);
 

@@ -761,9 +755,7 @@ static int i915_drm_resume(struct drm_device *dev)
 		dev_priv->display.hpd_irq_setup(dev);
 	spin_unlock_irq(&dev_priv->irq_lock);
 
-	drm_modeset_lock_all(dev);
 	intel_display_resume(dev);
-	drm_modeset_unlock_all(dev);
 
 	intel_dp_mst_resume(dev);
 

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

index 8216665405eb..ef289514b97e 100644
@@ -1848,6 +1848,7 @@ struct drm_i915_private {
 
 	enum modeset_restore modeset_restore;
 	struct mutex modeset_restore_lock;
+	struct drm_atomic_state *modeset_restore_state;
 
 	struct list_head vm_list; /* Global list of all address spaces */
 	struct i915_gtt gtt; /* VM representing the global address space */

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

index e496c130364d..4c91fd1c5222 100644
@@ -6393,58 +6393,19 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
  */
 int intel_display_suspend(struct drm_device *dev)
 {
-	struct drm_mode_config *config = &dev->mode_config;
-	struct drm_modeset_acquire_ctx *ctx = config->acquire_ctx;
-	struct drm_atomic_state *state;
-	struct drm_crtc *crtc;
-	unsigned crtc_mask = 0;
-	int ret = 0;
-
-	if (WARN_ON(!ctx))
-		return 0;
-
-	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;
-
-	for_each_crtc(dev, crtc) {
-		struct drm_crtc_state *crtc_state =
-			drm_atomic_get_crtc_state(state, crtc);
-
-		ret = PTR_ERR_OR_ZERO(crtc_state);
-		if (ret)
-			goto free;
-
-		if (!crtc_state->active)
-			continue;
-
-		crtc_state->active = false;
-		crtc_mask |= 1 << drm_crtc_index(crtc);
-	}
-
-	if (crtc_mask) {
-		ret = drm_atomic_commit(state);
-
-		if (!ret) {
-			for_each_crtc(dev, crtc)
-				if (crtc_mask & (1 << drm_crtc_index(crtc)))
-					crtc->state->active = true;
-
-			return ret;
-		}
-	}
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	int ret;

drivers/gpu/drm/i915/intel_display.c static void intel_crtc_disable_noatomic(struct drm_crtc *crtc) · line 6404

Quoted 10 lines

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

 
-free:
-	if (ret)
+	dev_priv->modeset_restore_state = drm_atomic_helper_suspend(dev);
+	ret = PTR_ERR_OR_ZERO(dev_priv->modeset_restore_state);
+	if (ret) {
 		DRM_ERROR("Suspending crtc's failed with %i\n", ret);
-	drm_atomic_state_free(state);
+		dev_priv->modeset_restore_state = NULL;
+	}

I would use a local state pointer and only assign it to modeset_restore_state once we know it's good.

drivers/gpu/drm/i915/intel_display.c static void intel_crtc_disable_noatomic(struct drm_crtc *crtc) · line 6408

Quoted 4 lines

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

 	return ret;
 }
 
+

Spurious newline.

Quoted 5 lines

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

 void intel_encoder_destroy(struct drm_encoder *encoder)
 {
 	struct intel_encoder *intel_encoder = to_intel_encoder(encoder);

@@ -15909,51 +15870,49 @@ intel_modeset_setup_hw_state(struct drm_device *dev)
 

Quoted 15 lines

Quoted 4 lines

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

 void intel_display_resume(struct drm_device *dev)
 {
-	struct drm_atomic_state *state = drm_atomic_state_alloc(dev);
-	struct intel_connector *conn;

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

Quoted 10 lines

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

-	struct intel_plane *plane;
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct drm_atomic_state *state = dev_priv->modeset_restore_state;
+	struct drm_modeset_acquire_ctx ctx;
 	struct drm_crtc *crtc;
-	int ret;
+	struct drm_crtc_state *crtc_state;
+	int ret, i;
+
+	intel_modeset_setup_hw_state(dev);

Isn't something in there going to scream about locking?

I was hoping to find out by running it through CI..

I don't see what possibly could scream however, from a glance it seems nothing depends on it.

Quoted 18 lines

Quoted 18 lines

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

+	i915_redisable_vga(dev);
+	dev_priv->modeset_restore_state = NULL;
 
 	if (!state)
 		return;
 
-	state->acquire_ctx = dev->mode_config.acquire_ctx;
-
-	for_each_crtc(dev, crtc) {
-		struct drm_crtc_state *crtc_state =
-			drm_atomic_get_crtc_state(state, crtc);
-
-		ret = PTR_ERR_OR_ZERO(crtc_state);
-		if (ret)
-			goto err;
+	drm_modeset_acquire_init(&ctx, 0);
+	state->acquire_ctx = &ctx;
 

Quoted 12 lines

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

Quoted 9 lines

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

-		/* force a restore */
+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		/*
+		 * Force recalculation even if we restore
+		 * current state. With fast modeset this may not result
+		 * in a modeset when the state is compatible.
+		 */
 		crtc_state->mode_changed = true;
 	}

Hmm. Why do we need to set that exactly? If the current state doesn't match the "to be restored" state, shouldn't something somewhere figure out that we really need a modeset?

We have a whole bunch of hw state from suspend that may be different from the hw readout in resume. This is not handled by the core because it doesn't know about driver specific pll's etc. So that's what the mode_changed = true is for. It's a hint to intel_atomic_check that might get downgraded to update_pipe if i915.fastboot is true and the hw readout and resume states are compatible.

Quoted 193 lines

Op 09-02-16 om 14:37 schreef Ville Syrjälä:

Quoted 144 lines · Maarten Lankhorst

Quoted 119 lines

Instead of duplicating the functionality now that we no longer need to preserve dpll state we can move to using the upstream suspend helper.

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

---
 drivers/gpu/drm/drm_atomic_helper.c  |   3 +
 drivers/gpu/drm/i915/i915_drv.c      |   8 ---
 drivers/gpu/drm/i915/i915_drv.h      |   1 +
 drivers/gpu/drm/i915/intel_display.c | 117 ++++++++++++-----------------------
 4 files changed, 42 insertions(+), 87 deletions(-)

drivers/gpu/drm/drm_atomic_helper.c +3 -0

index 2b430b05f35d..a2d3b094f27c 100644
@@ -2102,6 +2102,9 @@ int drm_atomic_helper_resume(struct drm_device *dev,
 	err = drm_atomic_commit(state);
 	drm_modeset_unlock_all(dev);
 
+	if (err)
+		drm_atomic_state_free(state);
+
 	return err;
 }
 EXPORT_SYMBOL(drm_atomic_helper_resume);

drivers/gpu/drm/i915/i915_drv.c +0 -8

index 11d8414edbbe..fc9b552bfcd1 100644
@@ -600,13 +600,7 @@ static int i915_drm_suspend(struct drm_device *dev)
 
 	intel_suspend_gt_powersave(dev);
 
-	/*
-	 * Disable CRTCs directly since we want to preserve sw state
-	 * for _thaw. Also, power gate the CRTC power wells.
-	 */
-	drm_modeset_lock_all(dev);
 	intel_display_suspend(dev);
-	drm_modeset_unlock_all(dev);
 
 	intel_dp_mst_suspend(dev);
 

@@ -761,9 +755,7 @@ static int i915_drm_resume(struct drm_device *dev)
 		dev_priv->display.hpd_irq_setup(dev);
 	spin_unlock_irq(&dev_priv->irq_lock);
 
-	drm_modeset_lock_all(dev);
 	intel_display_resume(dev);
-	drm_modeset_unlock_all(dev);
 
 	intel_dp_mst_resume(dev);
 

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

index 8216665405eb..ef289514b97e 100644
@@ -1848,6 +1848,7 @@ struct drm_i915_private {
 
 	enum modeset_restore modeset_restore;
 	struct mutex modeset_restore_lock;
+	struct drm_atomic_state *modeset_restore_state;
 
 	struct list_head vm_list; /* Global list of all address spaces */
 	struct i915_gtt gtt; /* VM representing the global address space */

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

index e496c130364d..4c91fd1c5222 100644
@@ -6393,58 +6393,19 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
  */
 int intel_display_suspend(struct drm_device *dev)
 {
-	struct drm_mode_config *config = &dev->mode_config;
-	struct drm_modeset_acquire_ctx *ctx = config->acquire_ctx;
-	struct drm_atomic_state *state;
-	struct drm_crtc *crtc;
-	unsigned crtc_mask = 0;
-	int ret = 0;
-
-	if (WARN_ON(!ctx))
-		return 0;
-
-	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;
-
-	for_each_crtc(dev, crtc) {
-		struct drm_crtc_state *crtc_state =
-			drm_atomic_get_crtc_state(state, crtc);
-
-		ret = PTR_ERR_OR_ZERO(crtc_state);
-		if (ret)
-			goto free;
-
-		if (!crtc_state->active)
-			continue;
-
-		crtc_state->active = false;
-		crtc_mask |= 1 << drm_crtc_index(crtc);
-	}
-
-	if (crtc_mask) {
-		ret = drm_atomic_commit(state);
-
-		if (!ret) {
-			for_each_crtc(dev, crtc)
-				if (crtc_mask & (1 << drm_crtc_index(crtc)))
-					crtc->state->active = true;
-
-			return ret;
-		}
-	}
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	int ret;

drivers/gpu/drm/i915/intel_display.c static void intel_crtc_disable_noatomic(struct drm_crtc *crtc) · line 6404

Quoted 10 lines

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

 
-free:
-	if (ret)
+	dev_priv->modeset_restore_state = drm_atomic_helper_suspend(dev);
+	ret = PTR_ERR_OR_ZERO(dev_priv->modeset_restore_state);
+	if (ret) {
 		DRM_ERROR("Suspending crtc's failed with %i\n", ret);
-	drm_atomic_state_free(state);
+		dev_priv->modeset_restore_state = NULL;
+	}

I would use a local state pointer and only assign it to modeset_restore_state once we know it's good.

drivers/gpu/drm/i915/intel_display.c static void intel_crtc_disable_noatomic(struct drm_crtc *crtc) · line 6408

Quoted 4 lines

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

 	return ret;
 }
 
+

Spurious newline.

Quoted 5 lines

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

 void intel_encoder_destroy(struct drm_encoder *encoder)
 {
 	struct intel_encoder *intel_encoder = to_intel_encoder(encoder);

@@ -15909,51 +15870,49 @@ intel_modeset_setup_hw_state(struct drm_device *dev)
 

Quoted 15 lines

Quoted 4 lines

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

 void intel_display_resume(struct drm_device *dev)
 {
-	struct drm_atomic_state *state = drm_atomic_state_alloc(dev);
-	struct intel_connector *conn;

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

Quoted 10 lines

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

-	struct intel_plane *plane;
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct drm_atomic_state *state = dev_priv->modeset_restore_state;
+	struct drm_modeset_acquire_ctx ctx;
 	struct drm_crtc *crtc;
-	int ret;
+	struct drm_crtc_state *crtc_state;
+	int ret, i;
+
+	intel_modeset_setup_hw_state(dev);

Isn't something in there going to scream about locking?

I was hoping to find out by running it through CI..

I don't see what possibly could scream however, from a glance it seems nothing depends on it.

Quoted 30 lines

Quoted 17 lines

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

+	i915_redisable_vga(dev);
+	dev_priv->modeset_restore_state = NULL;
 
 	if (!state)
 		return;
 
-	state->acquire_ctx = dev->mode_config.acquire_ctx;
-
-	for_each_crtc(dev, crtc) {
-		struct drm_crtc_state *crtc_state =
-			drm_atomic_get_crtc_state(state, crtc);
-
-		ret = PTR_ERR_OR_ZERO(crtc_state);
-		if (ret)
-			goto err;
+	drm_modeset_acquire_init(&ctx, 0);
+	state->acquire_ctx = &ctx;

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

Quoted 10 lines

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

 
-		/* force a restore */
+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		/*
+		 * Force recalculation even if we restore
+		 * current state. With fast modeset this may not result
+		 * in a modeset when the state is compatible.
+		 */
 		crtc_state->mode_changed = true;
 	}

Hmm. Why do we need to set that exactly? If the current state doesn't match the "to be restored" state, shouldn't something somewhere figure out that we really need a modeset?

Quoted 1 line

We have a whole bunch of hw state from suspend that may be different from the hw readout in resume. This is not handled by the core because it doesn't know about driver specific pll's etc. So that's what the mode_changed = true is for. It's a hint to intel_atomic_check that might get downgraded to update_pipe if i915.fastboot is true and the hw readout and resume states are compatible.

Hmm. So it's a bit of a workaround due to the fact that intel_atomic_check() doesn't actually check/update everything, and instead tries to skip a bunch of stuff if the core didn't detect any meaningful changes? I guess that makes some sense as an optimization. That is, there's no need to check stuff if nothing user visible changed anyway because we should anyway end up with the same state that we had should we do a full recompute.

Quoted 193 lines

Op 09-02-16 om 14:37 schreef Ville Syrjälä:

Quoted 144 lines · Maarten Lankhorst

Quoted 119 lines

Instead of duplicating the functionality now that we no longer need to preserve dpll state we can move to using the upstream suspend helper.

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

---
 drivers/gpu/drm/drm_atomic_helper.c  |   3 +
 drivers/gpu/drm/i915/i915_drv.c      |   8 ---
 drivers/gpu/drm/i915/i915_drv.h      |   1 +
 drivers/gpu/drm/i915/intel_display.c | 117 ++++++++++++-----------------------
 4 files changed, 42 insertions(+), 87 deletions(-)

drivers/gpu/drm/drm_atomic_helper.c +3 -0

index 2b430b05f35d..a2d3b094f27c 100644
@@ -2102,6 +2102,9 @@ int drm_atomic_helper_resume(struct drm_device *dev,
 	err = drm_atomic_commit(state);
 	drm_modeset_unlock_all(dev);
 
+	if (err)
+		drm_atomic_state_free(state);
+
 	return err;
 }
 EXPORT_SYMBOL(drm_atomic_helper_resume);

drivers/gpu/drm/i915/i915_drv.c +0 -8

index 11d8414edbbe..fc9b552bfcd1 100644
@@ -600,13 +600,7 @@ static int i915_drm_suspend(struct drm_device *dev)
 
 	intel_suspend_gt_powersave(dev);
 
-	/*
-	 * Disable CRTCs directly since we want to preserve sw state
-	 * for _thaw. Also, power gate the CRTC power wells.
-	 */
-	drm_modeset_lock_all(dev);
 	intel_display_suspend(dev);
-	drm_modeset_unlock_all(dev);
 
 	intel_dp_mst_suspend(dev);
 

@@ -761,9 +755,7 @@ static int i915_drm_resume(struct drm_device *dev)
 		dev_priv->display.hpd_irq_setup(dev);
 	spin_unlock_irq(&dev_priv->irq_lock);
 
-	drm_modeset_lock_all(dev);
 	intel_display_resume(dev);
-	drm_modeset_unlock_all(dev);
 
 	intel_dp_mst_resume(dev);
 

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

index 8216665405eb..ef289514b97e 100644
@@ -1848,6 +1848,7 @@ struct drm_i915_private {
 
 	enum modeset_restore modeset_restore;
 	struct mutex modeset_restore_lock;
+	struct drm_atomic_state *modeset_restore_state;
 
 	struct list_head vm_list; /* Global list of all address spaces */
 	struct i915_gtt gtt; /* VM representing the global address space */

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

index e496c130364d..4c91fd1c5222 100644
@@ -6393,58 +6393,19 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
  */
 int intel_display_suspend(struct drm_device *dev)
 {
-	struct drm_mode_config *config = &dev->mode_config;
-	struct drm_modeset_acquire_ctx *ctx = config->acquire_ctx;
-	struct drm_atomic_state *state;
-	struct drm_crtc *crtc;
-	unsigned crtc_mask = 0;
-	int ret = 0;
-
-	if (WARN_ON(!ctx))
-		return 0;
-
-	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;
-
-	for_each_crtc(dev, crtc) {
-		struct drm_crtc_state *crtc_state =
-			drm_atomic_get_crtc_state(state, crtc);
-
-		ret = PTR_ERR_OR_ZERO(crtc_state);
-		if (ret)
-			goto free;
-
-		if (!crtc_state->active)
-			continue;
-
-		crtc_state->active = false;
-		crtc_mask |= 1 << drm_crtc_index(crtc);
-	}
-
-	if (crtc_mask) {
-		ret = drm_atomic_commit(state);
-
-		if (!ret) {
-			for_each_crtc(dev, crtc)
-				if (crtc_mask & (1 << drm_crtc_index(crtc)))
-					crtc->state->active = true;
-
-			return ret;
-		}
-	}
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	int ret;

drivers/gpu/drm/i915/intel_display.c static void intel_crtc_disable_noatomic(struct drm_crtc *crtc) · line 6404

Quoted 10 lines

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

 
-free:
-	if (ret)
+	dev_priv->modeset_restore_state = drm_atomic_helper_suspend(dev);
+	ret = PTR_ERR_OR_ZERO(dev_priv->modeset_restore_state);
+	if (ret) {
 		DRM_ERROR("Suspending crtc's failed with %i\n", ret);
-	drm_atomic_state_free(state);
+		dev_priv->modeset_restore_state = NULL;
+	}

I would use a local state pointer and only assign it to modeset_restore_state once we know it's good.

drivers/gpu/drm/i915/intel_display.c static void intel_crtc_disable_noatomic(struct drm_crtc *crtc) · line 6408

Quoted 4 lines

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

 	return ret;
 }
 
+

Spurious newline.

Quoted 5 lines

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

 void intel_encoder_destroy(struct drm_encoder *encoder)
 {
 	struct intel_encoder *intel_encoder = to_intel_encoder(encoder);

@@ -15909,51 +15870,49 @@ intel_modeset_setup_hw_state(struct drm_device *dev)
 

Quoted 15 lines

Quoted 4 lines

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

 void intel_display_resume(struct drm_device *dev)
 {
-	struct drm_atomic_state *state = drm_atomic_state_alloc(dev);
-	struct intel_connector *conn;

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

Quoted 10 lines

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

-	struct intel_plane *plane;
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct drm_atomic_state *state = dev_priv->modeset_restore_state;
+	struct drm_modeset_acquire_ctx ctx;
 	struct drm_crtc *crtc;
-	int ret;
+	struct drm_crtc_state *crtc_state;
+	int ret, i;
+
+	intel_modeset_setup_hw_state(dev);

Isn't something in there going to scream about locking?

I was hoping to find out by running it through CI..

I don't see what possibly could scream however, from a glance it seems nothing depends on it.

Quoted 30 lines

Quoted 17 lines

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

+	i915_redisable_vga(dev);
+	dev_priv->modeset_restore_state = NULL;
 
 	if (!state)
 		return;
 
-	state->acquire_ctx = dev->mode_config.acquire_ctx;
-
-	for_each_crtc(dev, crtc) {
-		struct drm_crtc_state *crtc_state =
-			drm_atomic_get_crtc_state(state, crtc);
-
-		ret = PTR_ERR_OR_ZERO(crtc_state);
-		if (ret)
-			goto err;
+	drm_modeset_acquire_init(&ctx, 0);
+	state->acquire_ctx = &ctx;

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

Quoted 10 lines

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

 
-		/* force a restore */
+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		/*
+		 * Force recalculation even if we restore
+		 * current state. With fast modeset this may not result
+		 * in a modeset when the state is compatible.
+		 */
 		crtc_state->mode_changed = true;
 	}

Hmm. Why do we need to set that exactly? If the current state doesn't match the "to be restored" state, shouldn't something somewhere figure out that we really need a modeset?

Quoted 1 line

We have a whole bunch of hw state from suspend that may be different from the hw readout in resume. This is not handled by the core because it doesn't know about driver specific pll's etc. So that's what the mode_changed = true is for. It's a hint to intel_atomic_check that might get downgraded to update_pipe if i915.fastboot is true and the hw readout and resume states are compatible.

Hmm. So it's a bit of a workaround due to the fact that intel_atomic_check() doesn't actually check/update everything, and instead tries to skip a bunch of stuff if the core didn't detect any meaningful changes? I guess that makes some sense as an optimization. That is, there's no need to check stuff if nothing user visible changed anyway because we should anyway end up with the same state that we had should we do a full recompute.

Op 09-02-16 om 15:58 schreef Ville Syrjälä:

Quoted 194 lines · Maarten Lankhorst

Quoted 193 lines

Op 09-02-16 om 14:37 schreef Ville Syrjälä:

Quoted 144 lines · Maarten Lankhorst

Quoted 119 lines

Instead of duplicating the functionality now that we no longer need to preserve dpll state we can move to using the upstream suspend helper.

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

---
 drivers/gpu/drm/drm_atomic_helper.c  |   3 +
 drivers/gpu/drm/i915/i915_drv.c      |   8 ---
 drivers/gpu/drm/i915/i915_drv.h      |   1 +
 drivers/gpu/drm/i915/intel_display.c | 117 ++++++++++++-----------------------
 4 files changed, 42 insertions(+), 87 deletions(-)

drivers/gpu/drm/drm_atomic_helper.c +3 -0

index 2b430b05f35d..a2d3b094f27c 100644
@@ -2102,6 +2102,9 @@ int drm_atomic_helper_resume(struct drm_device *dev,
 	err = drm_atomic_commit(state);
 	drm_modeset_unlock_all(dev);
 
+	if (err)
+		drm_atomic_state_free(state);
+
 	return err;
 }
 EXPORT_SYMBOL(drm_atomic_helper_resume);

drivers/gpu/drm/i915/i915_drv.c +0 -8

index 11d8414edbbe..fc9b552bfcd1 100644
@@ -600,13 +600,7 @@ static int i915_drm_suspend(struct drm_device *dev)
 
 	intel_suspend_gt_powersave(dev);
 
-	/*
-	 * Disable CRTCs directly since we want to preserve sw state
-	 * for _thaw. Also, power gate the CRTC power wells.
-	 */
-	drm_modeset_lock_all(dev);
 	intel_display_suspend(dev);
-	drm_modeset_unlock_all(dev);
 
 	intel_dp_mst_suspend(dev);
 

@@ -761,9 +755,7 @@ static int i915_drm_resume(struct drm_device *dev)
 		dev_priv->display.hpd_irq_setup(dev);
 	spin_unlock_irq(&dev_priv->irq_lock);
 
-	drm_modeset_lock_all(dev);
 	intel_display_resume(dev);
-	drm_modeset_unlock_all(dev);
 
 	intel_dp_mst_resume(dev);
 

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

index 8216665405eb..ef289514b97e 100644
@@ -1848,6 +1848,7 @@ struct drm_i915_private {
 
 	enum modeset_restore modeset_restore;
 	struct mutex modeset_restore_lock;
+	struct drm_atomic_state *modeset_restore_state;
 
 	struct list_head vm_list; /* Global list of all address spaces */
 	struct i915_gtt gtt; /* VM representing the global address space */

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

index e496c130364d..4c91fd1c5222 100644
@@ -6393,58 +6393,19 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
  */
 int intel_display_suspend(struct drm_device *dev)
 {
-	struct drm_mode_config *config = &dev->mode_config;
-	struct drm_modeset_acquire_ctx *ctx = config->acquire_ctx;
-	struct drm_atomic_state *state;
-	struct drm_crtc *crtc;
-	unsigned crtc_mask = 0;
-	int ret = 0;
-
-	if (WARN_ON(!ctx))
-		return 0;
-
-	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;
-
-	for_each_crtc(dev, crtc) {
-		struct drm_crtc_state *crtc_state =
-			drm_atomic_get_crtc_state(state, crtc);
-
-		ret = PTR_ERR_OR_ZERO(crtc_state);
-		if (ret)
-			goto free;
-
-		if (!crtc_state->active)
-			continue;
-
-		crtc_state->active = false;
-		crtc_mask |= 1 << drm_crtc_index(crtc);
-	}
-
-	if (crtc_mask) {
-		ret = drm_atomic_commit(state);
-
-		if (!ret) {
-			for_each_crtc(dev, crtc)
-				if (crtc_mask & (1 << drm_crtc_index(crtc)))
-					crtc->state->active = true;
-
-			return ret;
-		}
-	}
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	int ret;

drivers/gpu/drm/i915/intel_display.c static void intel_crtc_disable_noatomic(struct drm_crtc *crtc) · line 6404

Quoted 10 lines

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

 
-free:
-	if (ret)
+	dev_priv->modeset_restore_state = drm_atomic_helper_suspend(dev);
+	ret = PTR_ERR_OR_ZERO(dev_priv->modeset_restore_state);
+	if (ret) {
 		DRM_ERROR("Suspending crtc's failed with %i\n", ret);
-	drm_atomic_state_free(state);
+		dev_priv->modeset_restore_state = NULL;
+	}

I would use a local state pointer and only assign it to modeset_restore_state once we know it's good.

drivers/gpu/drm/i915/intel_display.c static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)

Quoted 4 lines

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

 	return ret;
 }
 
+

Spurious newline.

Quoted 5 lines

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

 void intel_encoder_destroy(struct drm_encoder *encoder)
 {
 	struct intel_encoder *intel_encoder = to_intel_encoder(encoder);

@@ -15909,51 +15870,49 @@ intel_modeset_setup_hw_state(struct drm_device *dev)
 

Quoted 15 lines

Quoted 4 lines

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

 void intel_display_resume(struct drm_device *dev)
 {
-	struct drm_atomic_state *state = drm_atomic_state_alloc(dev);
-	struct intel_connector *conn;

drivers/gpu/drm/i915/intel_display.c intel_modeset_setup_hw_state(struct drm_device *dev)

Quoted 10 lines

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

-	struct intel_plane *plane;
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct drm_atomic_state *state = dev_priv->modeset_restore_state;
+	struct drm_modeset_acquire_ctx ctx;
 	struct drm_crtc *crtc;
-	int ret;
+	struct drm_crtc_state *crtc_state;
+	int ret, i;
+
+	intel_modeset_setup_hw_state(dev);

Isn't something in there going to scream about locking?

I was hoping to find out by running it through CI..

I don't see what possibly could scream however, from a glance it seems nothing depends on it.

Quoted 30 lines

Quoted 17 lines

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

+	i915_redisable_vga(dev);
+	dev_priv->modeset_restore_state = NULL;
 
 	if (!state)
 		return;
 
-	state->acquire_ctx = dev->mode_config.acquire_ctx;
-
-	for_each_crtc(dev, crtc) {
-		struct drm_crtc_state *crtc_state =
-			drm_atomic_get_crtc_state(state, crtc);
-
-		ret = PTR_ERR_OR_ZERO(crtc_state);
-		if (ret)
-			goto err;
+	drm_modeset_acquire_init(&ctx, 0);
+	state->acquire_ctx = &ctx;

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

Quoted 10 lines

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

 
-		/* force a restore */
+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		/*
+		 * Force recalculation even if we restore
+		 * current state. With fast modeset this may not result
+		 * in a modeset when the state is compatible.
+		 */
 		crtc_state->mode_changed = true;
 	}

Hmm. Why do we need to set that exactly? If the current state doesn't match the "to be restored" state, shouldn't something somewhere figure out that we really need a modeset?

Quoted 9 lines

Quoted 1 line

We have a whole bunch of hw state from suspend that may be different from the hw readout in resume. This is not handled by the core because it doesn't know about driver specific pll's etc. So that's what the mode_changed = true is for. It's a hint to intel_atomic_check that might get downgraded to update_pipe if i915.fastboot is true and the hw readout and resume states are compatible.

Hmm. So it's a bit of a workaround due to the fact that intel_atomic_check() doesn't actually check/update everything, and instead tries to skip a bunch of stuff if the core didn't detect any meaningful changes? I guess that makes some sense as an optimization. That is, there's no need to check stuff if nothing user visible changed anyway because we should anyway end up with the same state that we had should we do a full recompute.

How about this? with --scissors

--->8----

Instead of duplicating the functionality now that we no longer need to preserve dpll state we can move to using the upstream suspend helper.

Changes since v1:
- Call hw readout with all mutexes held.
- Rework intel_display_suspend to only assign modeset_restore_state on success.

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

---

drivers/gpu/drm/i915/i915_drv.c +0 -8

index 44912ecebc1a..20e82008b8b6 100644
@@ -603,13 +603,7 @@ static int i915_drm_suspend(struct drm_device *dev)
 
 	intel_suspend_gt_powersave(dev);
 
-	/*
-	 * Disable CRTCs directly since we want to preserve sw state
-	 * for _thaw. Also, power gate the CRTC power wells.
-	 */
-	drm_modeset_lock_all(dev);
 	intel_display_suspend(dev);
-	drm_modeset_unlock_all(dev);
 
 	intel_dp_mst_suspend(dev);
 

@@ -764,9 +758,7 @@ static int i915_drm_resume(struct drm_device *dev)
 		dev_priv->display.hpd_irq_setup(dev);
 	spin_unlock_irq(&dev_priv->irq_lock);
 
-	drm_modeset_lock_all(dev);
 	intel_display_resume(dev);
-	drm_modeset_unlock_all(dev);
 
 	intel_dp_mst_resume(dev);
 

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

index 20d9dbd9f9cf..6644c2e354c1 100644
@@ -1848,6 +1848,7 @@ struct drm_i915_private {
 
 	enum modeset_restore modeset_restore;
 	struct mutex modeset_restore_lock;
+	struct drm_atomic_state *modeset_restore_state;
 
 	struct list_head vm_list; /* Global list of all address spaces */
 	struct i915_gtt gtt; /* VM representing the global address space */

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

index a18bd1296ce8..eaff32f59953 100644
@@ -6393,55 +6393,16 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
  */
 int intel_display_suspend(struct drm_device *dev)
 {
-	struct drm_mode_config *config = &dev->mode_config;
-	struct drm_modeset_acquire_ctx *ctx = config->acquire_ctx;
+	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct drm_atomic_state *state;
-	struct drm_crtc *crtc;
-	unsigned crtc_mask = 0;
-	int ret = 0;
-
-	if (WARN_ON(!ctx))
-		return 0;
-
-	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;
-
-	for_each_crtc(dev, crtc) {
-		struct drm_crtc_state *crtc_state =
-			drm_atomic_get_crtc_state(state, crtc);
-
-		ret = PTR_ERR_OR_ZERO(crtc_state);
-		if (ret)
-			goto free;
-
-		if (!crtc_state->active)
-			continue;
-
-		crtc_state->active = false;
-		crtc_mask |= 1 << drm_crtc_index(crtc);
-	}
-
-	if (crtc_mask) {
-		ret = drm_atomic_commit(state);
-
-		if (!ret) {
-			for_each_crtc(dev, crtc)
-				if (crtc_mask & (1 << drm_crtc_index(crtc)))
-					crtc->state->active = true;
-
-			return ret;
-		}
-	}
+	int ret;
 
-free:
+	state = drm_atomic_helper_suspend(dev);
+	ret = PTR_ERR_OR_ZERO(state);
 	if (ret)
 		DRM_ERROR("Suspending crtc's failed with %i\n", ret);
-	drm_atomic_state_free(state);
+	else
+		dev_priv->modeset_restore_state = state;
 	return ret;
 }
 

@@ -15914,51 +15875,57 @@ intel_modeset_setup_hw_state(struct drm_device *dev)
 
 void intel_display_resume(struct drm_device *dev)
 {
-	struct drm_atomic_state *state = drm_atomic_state_alloc(dev);
-	struct intel_connector *conn;
-	struct intel_plane *plane;
-	struct drm_crtc *crtc;
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct drm_atomic_state *state = dev_priv->modeset_restore_state;
+	struct drm_modeset_acquire_ctx ctx;
 	int ret;
+	bool setup = false;
 
-	if (!state)
-		return;
+	dev_priv->modeset_restore_state = NULL;
 
-	state->acquire_ctx = dev->mode_config.acquire_ctx;
+	drm_modeset_acquire_init(&ctx, 0);
 

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

+		setup = true;
 
-		/* force a restore */
-		crtc_state->mode_changed = true;

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

+		struct drm_crtc_state *crtc_state;
+		struct drm_crtc *crtc;
+		int i;
 
-	for_each_intel_connector(dev, conn) {
-		ret = PTR_ERR_OR_ZERO(drm_atomic_get_connector_state(state, &conn->base));
-		if (ret)
-			goto err;
+		state->acquire_ctx = &ctx;
+
+		for_each_crtc_in_state(state, crtc, crtc_state, i) {
+			/*
+			 * Force recalculation even if we restore
+			 * current state. With fast modeset this may not result
+			 * in a modeset when the state is compatible.
+			 */
+			crtc_state->mode_changed = true;
+		}
+
+		ret = drm_atomic_commit(state);
 	}
 
-	intel_modeset_setup_hw_state(dev);
+	if (ret == -EDEADLK) {
+		drm_modeset_backoff(&ctx);
+		goto retry;
+	}
 
-	i915_redisable_vga(dev);
-	ret = drm_atomic_commit(state);
-	if (!ret)
-		return;
+	drm_modeset_drop_locks(&ctx);
+	drm_modeset_acquire_fini(&ctx);
 
-err:
-	DRM_ERROR("Restoring old state failed with %i\n", ret);
-	drm_atomic_state_free(state);
+	if (ret) {
+		DRM_ERROR("Restoring old state failed with %i\n", ret);
+		drm_atomic_state_free(state);
+	}
 }
 
 void intel_modeset_gem_init(struct drm_device *dev)