[PATCH 0/6] Use more atomic state in i915.

[PATCH 0/6] Use more atomic state in i915.

[Maarten Lankhorst]

Use atomic state for load detection, stop use of obsolete connector->dpms and
use atomic in intel_fb_initial_config.

Maarten Lankhorst (6):
  drm/i915: Use atomic state to obtain load detection crtc.
  drm/i915: Use atomic state for load detect in crt.
  drm/i915: Use atomic state in tv load detection.
  drm/i915: Use correct dpms for intel_enable_crt.
  kms_force_connector_basic: Add force-load-detect test
  drm/i915: Use atomic state in intel_fb_initial_config.

 drivers/gpu/drm/i915/intel_crt.c     |  10 ++-
 drivers/gpu/drm/i915/intel_display.c | 128 ++++++++++++++++++++---------------
 drivers/gpu/drm/i915/intel_drv.h     |   4 +-
 drivers/gpu/drm/i915/intel_fbdev.c   |  17 +++--
 drivers/gpu/drm/i915/intel_tv.c      |  11 ++-

-- 
2.1.0

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

[PATCH 1/6] drm/i915: Use atomic state to obtain load detection crtc.

[Maarten Lankhorst]

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 <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 128 ++++++++++++++++++++---------------
 drivers/gpu/drm/i915/intel_drv.h     |   4 +-
 2 files changed, 76 insertions(+), 56 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 4d8c9f7857db..0702ce8ec36a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -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;
 
-		return;
-	}
+	old->old_plane_state->state = plane_state->state;
+	*plane_state = *old->old_plane_state;
 
-	/* 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,
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 93ba14a3bb76..eeda7d02a4f7 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -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 *
-- 
2.1.0

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

Re: [PATCH 1/6] drm/i915: Use atomic state to obtain load detection crtc.

[Ville Syrjälä]

On Mon, Feb 01, 2016 at 02:43:57PM +0100, Maarten Lankhorst wrote:
> 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 <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 128 ++++++++++++++++++++---------------
>  drivers/gpu/drm/i915/intel_drv.h     |   4 +-
>  2 files changed, 76 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 4d8c9f7857db..0702ce8ec36a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -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;
>  
> -		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"

>  
> -	/* 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,
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 93ba14a3bb76..eeda7d02a4f7 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -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 *
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/6] drm/i915: Use atomic state to obtain load detection crtc.

[Maarten Lankhorst]

Op 01-02-16 om 17:08 schreef Ville Syrjälä:
> On Mon, Feb 01, 2016 at 02:43:57PM +0100, Maarten Lankhorst wrote:
>> 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 <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 128 ++++++++++++++++++++---------------
>>  drivers/gpu/drm/i915/intel_drv.h     |   4 +-
>>  2 files changed, 76 insertions(+), 56 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 4d8c9f7857db..0702ce8ec36a 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -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;
>>  
>> -		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"
We could create 2 atomic states, update the first one, and commit the second one in release_load_detect_pipe.

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

Re: [PATCH 1/6] drm/i915: Use atomic state to obtain load detection crtc.

[Ville Syrjälä]

On Mon, Feb 01, 2016 at 02:43:57PM +0100, Maarten Lankhorst wrote:
> 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 <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 128 ++++++++++++++++++++---------------
>  drivers/gpu/drm/i915/intel_drv.h     |   4 +-
>  2 files changed, 76 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 4d8c9f7857db..0702ce8ec36a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -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) {

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?

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/6] drm/i915: Use atomic state to obtain load detection crtc.

[Maarten Lankhorst]

Op 01-02-16 om 18:09 schreef Ville Syrjälä:
> On Mon, Feb 01, 2016 at 02:43:57PM +0100, Maarten Lankhorst wrote:
>> 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 <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 128 ++++++++++++++++++++---------------
>>  drivers/gpu/drm/i915/intel_drv.h     |   4 +-
>>  2 files changed, 76 insertions(+), 56 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 4d8c9f7857db..0702ce8ec36a 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -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) {
> 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?
I'm not sure how to proceed on that, I do think all accesses should be cleaned up,
but I'm not sure yet what the path forward is, and so I want to leave it for a different series.

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

[PATCH v2 1/6] drm/i915: Use atomic state to obtain load detection crtc, v2.

[Maarten Lankhorst]

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 <maarten.lankhorst@linux.intel.com>
---
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 4d8c9f7857db..c7ba8f4a971e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -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;
-	}
 
-	/* 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;
 
-	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,
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 93ba14a3bb76..7d10ca7797a9 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -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 *

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

Re: [PATCH v2 1/6] drm/i915: Use atomic state to obtain load detection crtc, v2.

[Ville Syrjälä]

On Tue, Feb 02, 2016 at 01:48:17PM +0100, Maarten Lankhorst wrote:
> 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 <maarten.lankhorst@linux.intel.com>
> ---
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 4d8c9f7857db..c7ba8f4a971e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -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;
> -	}
>  
> -	/* 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?

>  
> -	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,
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 93ba14a3bb76..7d10ca7797a9 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -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 *

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/6] drm/i915: Use atomic state to obtain load detection crtc, v2.

[Maarten Lankhorst]

Op 02-02-16 om 18:32 schreef Ville Syrjälä:
> On Tue, Feb 02, 2016 at 01:48:17PM +0100, Maarten Lankhorst wrote:
>> 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 <maarten.lankhorst@linux.intel.com>
>> ---
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 4d8c9f7857db..c7ba8f4a971e 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -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;
>> -	}
>>  
>> -	/* 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.

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

Re: [PATCH v2 1/6] drm/i915: Use atomic state to obtain load detection crtc, v2.

[Ville Syrjälä]

On Wed, Feb 03, 2016 at 09:57:55AM +0100, Maarten Lankhorst wrote:
> Op 02-02-16 om 18:32 schreef Ville Syrjälä:
> > On Tue, Feb 02, 2016 at 01:48:17PM +0100, Maarten Lankhorst wrote:
> >> 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 <maarten.lankhorst@linux.intel.com>
> >> ---
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index 4d8c9f7857db..c7ba8f4a971e 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -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;
> >> -	}
> >>  
> >> -	/* 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.

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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/6] drm/i915: Use atomic state to obtain load detection crtc, v2.

[Maarten Lankhorst]

Op 03-02-16 om 17:34 schreef Ville Syrjälä:
> On Wed, Feb 03, 2016 at 09:57:55AM +0100, Maarten Lankhorst wrote:
>> Op 02-02-16 om 18:32 schreef Ville Syrjälä:
>>> On Tue, Feb 02, 2016 at 01:48:17PM +0100, Maarten Lankhorst wrote:
>>>> 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 <maarten.lankhorst@linux.intel.com>
>>>> ---
>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>> index 4d8c9f7857db..c7ba8f4a971e 100644
>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>> @@ -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;
>>>> -	}
>>>>  
>>>> -	/* 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.
> Sounds fragile. Any chance of making it do the right thing (tm) without
> having to jump through such hoops?
I'm not completely sure if that's possible, I'll have to check if making intel_modeset_clear_plls only clear the mask from old_crtc_state->shared_dpll will break anything first.

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

[PATCH 1/3] drm/i915: Clear shared dpll based on old state.

[Maarten Lankhorst]

Atomic resume was preserving the dpll state because it was required
for clearing pll state correctly. If we look at the old_crtc_state
for pll to clear this is not needed and the hack can be removed.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3b6bd6e9f7ae..e496c130364d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13093,8 +13093,6 @@ static void intel_modeset_clear_plls(struct drm_atomic_state *state)
 	struct drm_device *dev = state->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_shared_dpll_config *shared_dpll = NULL;
-	struct intel_crtc *intel_crtc;
-	struct intel_crtc_state *intel_crtc_state;
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *crtc_state;
 	int i;
@@ -13103,16 +13101,16 @@ static void intel_modeset_clear_plls(struct drm_atomic_state *state)
 		return;
 
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
-		int dpll;
-
-		intel_crtc = to_intel_crtc(crtc);
-		intel_crtc_state = to_intel_crtc_state(crtc_state);
-		dpll = intel_crtc_state->shared_dpll;
+		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+		int dpll = to_intel_crtc_state(crtc->state)->shared_dpll;
 
-		if (!needs_modeset(crtc_state) || dpll == DPLL_ID_PRIVATE)
+		if (!needs_modeset(crtc_state))
 			continue;
 
-		intel_crtc_state->shared_dpll = DPLL_ID_PRIVATE;
+		to_intel_crtc_state(crtc_state)->shared_dpll = DPLL_ID_PRIVATE;
+
+		if (dpll == DPLL_ID_PRIVATE)
+			continue;
 
 		if (!shared_dpll)
 			shared_dpll = intel_atomic_get_shared_dpll_state(state);
@@ -15922,9 +15920,6 @@ void intel_display_resume(struct drm_device *dev)
 
 	state->acquire_ctx = dev->mode_config.acquire_ctx;
 
-	/* preserve complete old state, including dpll */
-	intel_atomic_get_shared_dpll_state(state);
-
 	for_each_crtc(dev, crtc) {
 		struct drm_crtc_state *crtc_state =
 			drm_atomic_get_crtc_state(state, crtc);
-- 
2.1.0

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

[PATCH 2/3] drm/i915: Use atomic helpers for suspend.

[Maarten Lankhorst]

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 <maarten.lankhorst@linux.intel.com>
---
 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(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 2b430b05f35d..a2d3b094f27c 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -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);
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 11d8414edbbe..fc9b552bfcd1 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -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);
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 8216665405eb..ef289514b97e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -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 */
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e496c130364d..4c91fd1c5222 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -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;
 
-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;
+	}
 	return ret;
 }
 
+
 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;
-	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);
+	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;
 
-		/* 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;
 	}
 
-	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)
-- 
2.1.0

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

Re: [PATCH 2/3] drm/i915: Use atomic helpers for suspend.

[Ville Syrjälä]

On Tue, Feb 09, 2016 at 01:52:22PM +0100, Maarten Lankhorst wrote:
> 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 <maarten.lankhorst@linux.intel.com>
> ---
>  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(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 2b430b05f35d..a2d3b094f27c 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -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);
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 11d8414edbbe..fc9b552bfcd1 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -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);
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 8216665405eb..ef289514b97e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -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 */
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index e496c130364d..4c91fd1c5222 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -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;
>  
> -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.

>  	return ret;
>  }
>  
> +

Spurious newline.

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

> +	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;
>  
> -		/* 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?

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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915: Use atomic helpers for suspend.

[Maarten Lankhorst]

Op 09-02-16 om 14:37 schreef Ville Syrjälä:
> On Tue, Feb 09, 2016 at 01:52:22PM +0100, Maarten Lankhorst wrote:
>> 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 <maarten.lankhorst@linux.intel.com>
>> ---
>>  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(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>> index 2b430b05f35d..a2d3b094f27c 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -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);
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> index 11d8414edbbe..fc9b552bfcd1 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -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);
>>  
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 8216665405eb..ef289514b97e 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -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 */
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index e496c130364d..4c91fd1c5222 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -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;
>>  
>> -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.
>
>>  	return ret;
>>  }
>>  
>> +
> Spurious newline.
>
>>  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;
>> -	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.
>> +	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;
>>  
>> -		/* 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.

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

Re: [PATCH 2/3] drm/i915: Use atomic helpers for suspend.

[Ville Syrjälä]

On Tue, Feb 09, 2016 at 03:05:51PM +0100, Maarten Lankhorst wrote:
> Op 09-02-16 om 14:37 schreef Ville Syrjälä:
> > On Tue, Feb 09, 2016 at 01:52:22PM +0100, Maarten Lankhorst wrote:
> >> 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 <maarten.lankhorst@linux.intel.com>
> >> ---
> >>  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(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> >> index 2b430b05f35d..a2d3b094f27c 100644
> >> --- a/drivers/gpu/drm/drm_atomic_helper.c
> >> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> >> @@ -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);
> >> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> >> index 11d8414edbbe..fc9b552bfcd1 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.c
> >> +++ b/drivers/gpu/drm/i915/i915_drv.c
> >> @@ -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);
> >>  
> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >> index 8216665405eb..ef289514b97e 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.h
> >> +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> @@ -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 */
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index e496c130364d..4c91fd1c5222 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -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;
> >>  
> >> -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.
> >
> >>  	return ret;
> >>  }
> >>  
> >> +
> > Spurious newline.
> >
> >>  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;
> >> -	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.
> >> +	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;
> >>  
> >> -		/* 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.

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.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915: Use atomic helpers for suspend.

[Maarten Lankhorst]

Op 09-02-16 om 15:58 schreef Ville Syrjälä:
> On Tue, Feb 09, 2016 at 03:05:51PM +0100, Maarten Lankhorst wrote:
>> Op 09-02-16 om 14:37 schreef Ville Syrjälä:
>>> On Tue, Feb 09, 2016 at 01:52:22PM +0100, Maarten Lankhorst wrote:
>>>> 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 <maarten.lankhorst@linux.intel.com>
>>>> ---
>>>>  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(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>>>> index 2b430b05f35d..a2d3b094f27c 100644
>>>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>>>> @@ -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);
>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>>>> index 11d8414edbbe..fc9b552bfcd1 100644
>>>> --- a/drivers/gpu/drm/i915/i915_drv.c
>>>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>>>> @@ -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);
>>>>  
>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>>> index 8216665405eb..ef289514b97e 100644
>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>>> @@ -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 */
>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>> index e496c130364d..4c91fd1c5222 100644
>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>> @@ -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;
>>>>  
>>>> -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.
>>>
>>>>  	return ret;
>>>>  }
>>>>  
>>>> +
>>> Spurious newline.
>>>
>>>>  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;
>>>> -	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.
>>>> +	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;
>>>>  
>>>> -		/* 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.
> 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.
Without the hint that there is a change it can't even do the checking since all connectors and planes aren't added.
But indeed, nothing user visible would have changed.

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

[PATCH v2 2/3] drm/i915: Use atomic helpers for suspend, v2.

[Maarten Lankhorst]

Op 09-02-16 om 15:58 schreef Ville Syrjälä:
> On Tue, Feb 09, 2016 at 03:05:51PM +0100, Maarten Lankhorst wrote:
>> Op 09-02-16 om 14:37 schreef Ville Syrjälä:
>>> On Tue, Feb 09, 2016 at 01:52:22PM +0100, Maarten Lankhorst wrote:
>>>> 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 <maarten.lankhorst@linux.intel.com>
>>>> ---
>>>>  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(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>>>> index 2b430b05f35d..a2d3b094f27c 100644
>>>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>>>> @@ -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);
>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>>>> index 11d8414edbbe..fc9b552bfcd1 100644
>>>> --- a/drivers/gpu/drm/i915/i915_drv.c
>>>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>>>> @@ -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);
>>>>  
>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>>> index 8216665405eb..ef289514b97e 100644
>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>>> @@ -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 */
>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>> index e496c130364d..4c91fd1c5222 100644
>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>> @@ -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;
>>>>  
>>>> -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.
>>>
>>>>  	return ret;
>>>>  }
>>>>  
>>>> +
>>> Spurious newline.
>>>
>>>>  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;
>>>> -	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.
>>>> +	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;
>>>>  
>>>> -		/* 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.
> 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 <maarten.lankhorst@linux.intel.com>
---
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 44912ecebc1a..20e82008b8b6 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -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);
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 20d9dbd9f9cf..6644c2e354c1 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -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 */
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a18bd1296ce8..eaff32f59953 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -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);
 
-	for_each_crtc(dev, crtc) {
-		struct drm_crtc_state *crtc_state =
-			drm_atomic_get_crtc_state(state, crtc);
+retry:
+	ret = drm_modeset_lock_all_ctx(dev, &ctx);
 
-		ret = PTR_ERR_OR_ZERO(crtc_state);
-		if (ret)
-			goto err;
+	if (ret == 0 && !setup) {
+		setup = true;
 
-		/* force a restore */
-		crtc_state->mode_changed = true;
+		intel_modeset_setup_hw_state(dev);
+		i915_redisable_vga(dev);
 	}
 
-	for_each_intel_plane(dev, plane) {
-		ret = PTR_ERR_OR_ZERO(drm_atomic_get_plane_state(state, &plane->base));
-		if (ret)
-			goto err;
-	}
+	if (ret == 0 && state) {
+		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)

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

Re: [PATCH v2 2/3] drm/i915: Use atomic helpers for suspend, v2.

[Ville Syrjälä]

On Tue, Feb 16, 2016 at 10:06:14AM +0100, Maarten Lankhorst wrote:
> Op 09-02-16 om 15:58 schreef Ville Syrjälä:
> > On Tue, Feb 09, 2016 at 03:05:51PM +0100, Maarten Lankhorst wrote:
> >> Op 09-02-16 om 14:37 schreef Ville Syrjälä:
> >>> On Tue, Feb 09, 2016 at 01:52:22PM +0100, Maarten Lankhorst wrote:
> >>>> 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 <maarten.lankhorst@linux.intel.com>
> >>>> ---
> >>>>  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(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> >>>> index 2b430b05f35d..a2d3b094f27c 100644
> >>>> --- a/drivers/gpu/drm/drm_atomic_helper.c
> >>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> >>>> @@ -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);
> >>>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> >>>> index 11d8414edbbe..fc9b552bfcd1 100644
> >>>> --- a/drivers/gpu/drm/i915/i915_drv.c
> >>>> +++ b/drivers/gpu/drm/i915/i915_drv.c
> >>>> @@ -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);
> >>>>  
> >>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >>>> index 8216665405eb..ef289514b97e 100644
> >>>> --- a/drivers/gpu/drm/i915/i915_drv.h
> >>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
> >>>> @@ -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 */
> >>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >>>> index e496c130364d..4c91fd1c5222 100644
> >>>> --- a/drivers/gpu/drm/i915/intel_display.c
> >>>> +++ b/drivers/gpu/drm/i915/intel_display.c
> >>>> @@ -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;
> >>>>  
> >>>> -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.
> >>>
> >>>>  	return ret;
> >>>>  }
> >>>>  
> >>>> +
> >>> Spurious newline.
> >>>
> >>>>  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;
> >>>> -	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.
> >>>> +	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;
> >>>>  
> >>>> -		/* 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.
> > 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 <maarten.lankhorst@linux.intel.com>
> ---
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 44912ecebc1a..20e82008b8b6 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -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);
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 20d9dbd9f9cf..6644c2e354c1 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -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 */
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index a18bd1296ce8..eaff32f59953 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -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);
>  
> -	for_each_crtc(dev, crtc) {
> -		struct drm_crtc_state *crtc_state =
> -			drm_atomic_get_crtc_state(state, crtc);
> +retry:
> +	ret = drm_modeset_lock_all_ctx(dev, &ctx);
>  
> -		ret = PTR_ERR_OR_ZERO(crtc_state);
> -		if (ret)
> -			goto err;
> +	if (ret == 0 && !setup) {

I didn't consider this aspect when I suggested the idea of doing the
readout with the locks held. A bit ugly, but I suppose there's no really
neat way to go about it.

Well, in practice we shouldn't get EDEADLK from anywhere after
drm_modeset_lock_all_ctx() so we could avoid the !setup part perhaps,
but then it's going to look like no other atomic commit path which might
be worse.

> +		setup = true;
>  
> -		/* force a restore */
> -		crtc_state->mode_changed = true;
> +		intel_modeset_setup_hw_state(dev);
> +		i915_redisable_vga(dev);
>  	}
>  
> -	for_each_intel_plane(dev, plane) {
> -		ret = PTR_ERR_OR_ZERO(drm_atomic_get_plane_state(state, &plane->base));
> -		if (ret)
> -			goto err;
> -	}
> +	if (ret == 0 && state) {

Another a bit ugly thing. But yeah we do want to check the NULL state
anyway, so seems like this is the easiest way.

So yeah, I think it looks all right (apart from diff having generated
a nasty hairball instead of something you can read easily), so
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/3] drm/i915: Use atomic state to obtain load detection crtc, v3.

[Maarten Lankhorst]

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. (Ville)
Changes since v2:
- Do not preserve shared_dpll any more, no need to do so. (Ville)

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 4c91fd1c5222..ad6d23c17d48 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10332,6 +10332,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;
@@ -10387,7 +10388,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;
@@ -10396,6 +10397,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)
@@ -10412,24 +10415,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) */
@@ -10437,8 +10431,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;
@@ -10452,23 +10453,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)) {
@@ -10476,7 +10476,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)) {
@@ -10500,7 +10502,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)) {
@@ -10512,15 +10513,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);
@@ -10528,7 +10542,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);
@@ -10542,65 +10557,24 @@ 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 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_atomic_state *state = old->restore_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;
-
-		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;
-	}
-
-	/* Switch crtc and encoder back off if necessary */
-	if (old->dpms_mode != DRM_MODE_DPMS_ON)
-		connector->funcs->dpms(connector, old->dpms_mode);
 
-	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,
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 878172a45f39..335e6b24b0bc 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -910,9 +910,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 *
-- 
2.1.0

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

Re: [PATCH 3/3] drm/i915: Use atomic state to obtain load detection crtc, v3.

[Ville Syrjälä]

On Tue, Feb 09, 2016 at 01:52:23PM +0100, Maarten Lankhorst wrote:
> 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. (Ville)
> Changes since v2:
> - Do not preserve shared_dpll any more, no need to do so. (Ville)
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

lgtm 
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_display.c | 132 ++++++++++++++---------------------
>  drivers/gpu/drm/i915/intel_drv.h     |   4 +-
>  2 files changed, 54 insertions(+), 82 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 4c91fd1c5222..ad6d23c17d48 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10332,6 +10332,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;
> @@ -10387,7 +10388,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;
> @@ -10396,6 +10397,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)
> @@ -10412,24 +10415,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) */
> @@ -10437,8 +10431,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;
> @@ -10452,23 +10453,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)) {
> @@ -10476,7 +10476,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)) {
> @@ -10500,7 +10502,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)) {
> @@ -10512,15 +10513,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);
> @@ -10528,7 +10542,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);
> @@ -10542,65 +10557,24 @@ 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 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_atomic_state *state = old->restore_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;
> -
> -		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;
> -	}
> -
> -	/* Switch crtc and encoder back off if necessary */
> -	if (old->dpms_mode != DRM_MODE_DPMS_ON)
> -		connector->funcs->dpms(connector, old->dpms_mode);
>  
> -	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,
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 878172a45f39..335e6b24b0bc 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -910,9 +910,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 *
> -- 
> 2.1.0

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Clear shared dpll based on old state.

[Ville Syrjälä]

On Tue, Feb 09, 2016 at 01:52:21PM +0100, Maarten Lankhorst wrote:
> Atomic resume was preserving the dpll state because it was required
> for clearing pll state correctly. If we look at the old_crtc_state
> for pll to clear this is not needed and the hack can be removed.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 19 +++++++------------
>  1 file changed, 7 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 3b6bd6e9f7ae..e496c130364d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13093,8 +13093,6 @@ static void intel_modeset_clear_plls(struct drm_atomic_state *state)
>  	struct drm_device *dev = state->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_shared_dpll_config *shared_dpll = NULL;
> -	struct intel_crtc *intel_crtc;
> -	struct intel_crtc_state *intel_crtc_state;
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *crtc_state;
>  	int i;
> @@ -13103,16 +13101,16 @@ static void intel_modeset_clear_plls(struct drm_atomic_state *state)
>  		return;
>  
>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> -		int dpll;
> -
> -		intel_crtc = to_intel_crtc(crtc);
> -		intel_crtc_state = to_intel_crtc_state(crtc_state);
> -		dpll = intel_crtc_state->shared_dpll;
> +		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +		int dpll = to_intel_crtc_state(crtc->state)->shared_dpll;
>  
> -		if (!needs_modeset(crtc_state) || dpll == DPLL_ID_PRIVATE)
> +		if (!needs_modeset(crtc_state))
>  			continue;
>  
> -		intel_crtc_state->shared_dpll = DPLL_ID_PRIVATE;
> +		to_intel_crtc_state(crtc_state)->shared_dpll = DPLL_ID_PRIVATE;
> +
> +		if (dpll == DPLL_ID_PRIVATE)
> +			continue;

Maybe this should be called old_dpll or something for a bit of extra
clarity? Anyway this makes sense to me so
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  
>  		if (!shared_dpll)
>  			shared_dpll = intel_atomic_get_shared_dpll_state(state);
> @@ -15922,9 +15920,6 @@ void intel_display_resume(struct drm_device *dev)
>  
>  	state->acquire_ctx = dev->mode_config.acquire_ctx;
>  
> -	/* preserve complete old state, including dpll */
> -	intel_atomic_get_shared_dpll_state(state);
> -
>  	for_each_crtc(dev, crtc) {
>  		struct drm_crtc_state *crtc_state =
>  			drm_atomic_get_crtc_state(state, crtc);
> -- 
> 2.1.0

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/6] drm/i915: Use atomic state to obtain load detection crtc.

[Daniel Vetter]

On Mon, Feb 01, 2016 at 02:43:57PM +0100, Maarten Lankhorst wrote:
> 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 <maarten.lankhorst@linux.intel.com>

Do we have the load-detect igt testcase now in the BAT set? We've broken
this way too many times to not do that. Imo that should be done before
merging this fix.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c | 128 ++++++++++++++++++++---------------
>  drivers/gpu/drm/i915/intel_drv.h     |   4 +-
>  2 files changed, 76 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 4d8c9f7857db..0702ce8ec36a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -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;
>  
> -		return;
> -	}
> +	old->old_plane_state->state = plane_state->state;
> +	*plane_state = *old->old_plane_state;
>  
> -	/* 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,
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 93ba14a3bb76..eeda7d02a4f7 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -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 *
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[PATCH 2/6] drm/i915: Use atomic state in crt load detection.

[Maarten Lankhorst]

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

diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index 9c89df1af036..8b37bfa6bbb3 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -469,11 +469,10 @@ static bool intel_crt_detect_ddc(struct drm_connector *connector)
 }
 
 static enum drm_connector_status
-intel_crt_load_detect(struct intel_crt *crt)
+intel_crt_load_detect(struct intel_crt *crt, uint32_t pipe)
 {
 	struct drm_device *dev = crt->base.base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	uint32_t pipe = to_intel_crtc(crt->base.base.crtc)->pipe;
 	uint32_t save_bclrpat;
 	uint32_t save_vtotal;
 	uint32_t vtotal, vactive;
@@ -642,7 +641,8 @@ intel_crt_detect(struct drm_connector *connector, bool force)
 		if (intel_crt_detect_ddc(connector))
 			status = connector_status_connected;
 		else if (INTEL_INFO(dev)->gen < 4)
-			status = intel_crt_load_detect(crt);
+			status = intel_crt_load_detect(crt,
+				to_intel_crtc(connector->state->crtc)->pipe);
 		else
 			status = connector_status_unknown;
 		intel_release_load_detect_pipe(connector, &tmp, &ctx);
-- 
2.1.0

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

[PATCH 3/6] drm/i915: Use atomic state in tv load detection.

[Maarten Lankhorst]

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

diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
index 948cbff6c62e..643f52184d2f 100644
--- a/drivers/gpu/drm/i915/intel_tv.c
+++ b/drivers/gpu/drm/i915/intel_tv.c
@@ -1178,10 +1178,9 @@ static int
 intel_tv_detect_type(struct intel_tv *intel_tv,
 		      struct drm_connector *connector)
 {
-	struct drm_encoder *encoder = &intel_tv->base.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_device *dev = encoder->dev;
+	struct drm_device *dev = connector->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u32 tv_ctl, save_tv_ctl;
 	u32 tv_dac, save_tv_dac;
@@ -1230,8 +1229,7 @@ intel_tv_detect_type(struct intel_tv *intel_tv,
 	I915_WRITE(TV_DAC, tv_dac);
 	POSTING_READ(TV_DAC);
 
-	intel_wait_for_vblank(intel_tv->base.base.dev,
-			      to_intel_crtc(intel_tv->base.base.crtc)->pipe);
+	intel_wait_for_vblank(dev, intel_crtc->pipe);
 
 	type = -1;
 	tv_dac = I915_READ(TV_DAC);
@@ -1261,8 +1259,7 @@ intel_tv_detect_type(struct intel_tv *intel_tv,
 	POSTING_READ(TV_CTL);
 
 	/* For unknown reasons the hw barfs if we don't do this vblank wait. */
-	intel_wait_for_vblank(intel_tv->base.base.dev,
-			      to_intel_crtc(intel_tv->base.base.crtc)->pipe);
+	intel_wait_for_vblank(dev, intel_crtc->pipe);
 
 	/* Restore interrupt config */
 	if (connector->polled & DRM_CONNECTOR_POLL_HPD) {
-- 
2.1.0

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

[PATCH 4/6] drm/i915: Use correct dpms for intel_enable_crt.

[Maarten Lankhorst]

With the conversion to atomic only on/off are still supported.
The rest is mapped to one of those, and when enable is called
DPMS_ON should be true.

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

diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index 8b37bfa6bbb3..8a5a2e576d76 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -206,9 +206,7 @@ static void pch_post_disable_crt(struct intel_encoder *encoder)
 
 static void intel_enable_crt(struct intel_encoder *encoder)
 {
-	struct intel_crt *crt = intel_encoder_to_crt(encoder);
-
-	intel_crt_set_dpms(encoder, crt->connector->base.dpms);
+	intel_crt_set_dpms(encoder, DRM_MODE_DPMS_ON);
 }
 
 static enum drm_mode_status
-- 
2.1.0

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

[IGT PATCH 5/6] kms_force_connector_basic: Add force-load-detect test

[Maarten Lankhorst]

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
diff --git a/tests/kms_force_connector_basic.c b/tests/kms_force_connector_basic.c
index bd80caeffd82..f827d0008f7b 100644
--- a/tests/kms_force_connector_basic.c
+++ b/tests/kms_force_connector_basic.c
@@ -52,6 +52,8 @@ static void reset_connectors(void)
 
 		drmModeFreeConnector(connector);
 	}
+
+	igt_set_module_param_int("load_detect_test", 0);
 }
 
 static int opt_handler(int opt, int opt_index, void *data)
@@ -108,6 +110,17 @@ int main(int argc, char **argv)
 		igt_skip_on(vga_connector->connection == DRM_MODE_CONNECTED);
 	}
 
+	igt_subtest("force-load-detect") {
+		igt_set_module_param_int("load_detect_test", 1);
+
+		temp = drmModeGetConnectorCurrent(drm_fd,
+						  vga_connector->connector_id);
+
+		igt_assert(temp->connection != DRM_MODE_UNKNOWNCONNECTION);
+
+		drmModeFreeConnector(temp);
+	}
+
 	igt_subtest("force-connector-state") {
 		igt_display_t display;
 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [IGT PATCH 5/6] kms_force_connector_basic: Add force-load-detect test

[Daniel Vetter]

On Mon, Feb 01, 2016 at 02:44:01PM +0100, Maarten Lankhorst wrote:
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
> diff --git a/tests/kms_force_connector_basic.c b/tests/kms_force_connector_basic.c
> index bd80caeffd82..f827d0008f7b 100644
> --- a/tests/kms_force_connector_basic.c
> +++ b/tests/kms_force_connector_basic.c
> @@ -52,6 +52,8 @@ static void reset_connectors(void)
>  
>  		drmModeFreeConnector(connector);
>  	}
> +
> +	igt_set_module_param_int("load_detect_test", 0);
>  }
>  
>  static int opt_handler(int opt, int opt_index, void *data)
> @@ -108,6 +110,17 @@ int main(int argc, char **argv)
>  		igt_skip_on(vga_connector->connection == DRM_MODE_CONNECTED);
>  	}
>  
> +	igt_subtest("force-load-detect") {
> +		igt_set_module_param_int("load_detect_test", 1);

Ah here's the testcase. We need to unset this module parameter again at
the end of the testcase - atm the magic exithandler stuff only works on
binary exit, not when leaving a subtests (for things set within a
subtest).

With that done Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

We should probably fix that issue in the exit handler code, but that's
another task really.
-Daniel

> +
> +		temp = drmModeGetConnectorCurrent(drm_fd,
> +						  vga_connector->connector_id);
> +
> +		igt_assert(temp->connection != DRM_MODE_UNKNOWNCONNECTION);
> +
> +		drmModeFreeConnector(temp);
> +	}
> +
>  	igt_subtest("force-connector-state") {
>  		igt_display_t display;
>  
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [IGT PATCH 5/6] kms_force_connector_basic: Add force-load-detect test

[Maarten Lankhorst]

Op 11-02-16 om 09:59 schreef Daniel Vetter:
> On Mon, Feb 01, 2016 at 02:44:01PM +0100, Maarten Lankhorst wrote:
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>> diff --git a/tests/kms_force_connector_basic.c b/tests/kms_force_connector_basic.c
>> index bd80caeffd82..f827d0008f7b 100644
>> --- a/tests/kms_force_connector_basic.c
>> +++ b/tests/kms_force_connector_basic.c
>> @@ -52,6 +52,8 @@ static void reset_connectors(void)
>>  
>>  		drmModeFreeConnector(connector);
>>  	}
>> +
>> +	igt_set_module_param_int("load_detect_test", 0);
>>  }
>>  
>>  static int opt_handler(int opt, int opt_index, void *data)
>> @@ -108,6 +110,17 @@ int main(int argc, char **argv)
>>  		igt_skip_on(vga_connector->connection == DRM_MODE_CONNECTED);
>>  	}
>>  
>> +	igt_subtest("force-load-detect") {
>> +		igt_set_module_param_int("load_detect_test", 1);
> Ah here's the testcase. We need to unset this module parameter again at
> the end of the testcase - atm the magic exithandler stuff only works on
> binary exit, not when leaving a subtests (for things set within a
> subtest).
>
> With that done Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> We should probably fix that issue in the exit handler code, but that's
> another task really.
>
Yeah, but pushed the fixed version for now. Thanks!
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 6/6] drm/i915: Use atomic state in intel_fb_initial_config.

[Maarten Lankhorst]

This is another step in removing legacy state.

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

diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 09840f4380f9..97a91e631915 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -406,8 +406,8 @@ retry:
 			continue;
 		}
 
-		encoder = connector->encoder;
-		if (!encoder || WARN_ON(!encoder->crtc)) {
+		encoder = connector->state->best_encoder;
+		if (!encoder || WARN_ON(!connector->state->crtc)) {
 			if (connector->force > DRM_FORCE_OFF)
 				goto bail;
 
@@ -420,7 +420,7 @@ retry:
 
 		num_connectors_enabled++;
 
-		new_crtc = intel_fb_helper_crtc(fb_helper, encoder->crtc);
+		new_crtc = intel_fb_helper_crtc(fb_helper, connector->state->crtc);
 
 		/*
 		 * Make sure we're not trying to drive multiple connectors
@@ -466,17 +466,22 @@ retry:
 			 * usually contains. But since our current
 			 * code puts a mode derived from the post-pfit timings
 			 * into crtc->mode this works out correctly.
+			 *
+			 * This is crtc->mode and not crtc->state->mode for the
+			 * fastboot check to work correctly. crtc_state->mode has
+			 * I915_MODE_FLAG_INHERITED, which we clear to force check
+			 * state.
 			 */
 			DRM_DEBUG_KMS("looking for current mode on connector %s\n",
 				      connector->name);
-			modes[i] = &encoder->crtc->mode;
+			modes[i] = &connector->state->crtc->mode;
 		}
 		crtcs[i] = new_crtc;
 
 		DRM_DEBUG_KMS("connector %s on pipe %c [CRTC:%d]: %dx%d%s\n",
 			      connector->name,
-			      pipe_name(to_intel_crtc(encoder->crtc)->pipe),
-			      encoder->crtc->base.id,
+			      pipe_name(to_intel_crtc(connector->state->crtc)->pipe),
+			      connector->state->crtc->base.id,
 			      modes[i]->hdisplay, modes[i]->vdisplay,
 			      modes[i]->flags & DRM_MODE_FLAG_INTERLACE ? "i" :"");
 
-- 
2.1.0

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

Re: [PATCH 6/6] drm/i915: Use atomic state in intel_fb_initial_config.

[Ville Syrjälä]

On Mon, Feb 01, 2016 at 02:44:02PM +0100, Maarten Lankhorst wrote:
> This is another step in removing legacy state.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Patches 2,3,4,6 lgtm.
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Though I'd still prefer some locking asserts to avoid having to go
double check all the callers.

> ---
>  drivers/gpu/drm/i915/intel_fbdev.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 09840f4380f9..97a91e631915 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -406,8 +406,8 @@ retry:
>  			continue;
>  		}
>  
> -		encoder = connector->encoder;
> -		if (!encoder || WARN_ON(!encoder->crtc)) {
> +		encoder = connector->state->best_encoder;
> +		if (!encoder || WARN_ON(!connector->state->crtc)) {
>  			if (connector->force > DRM_FORCE_OFF)
>  				goto bail;
>  
> @@ -420,7 +420,7 @@ retry:
>  
>  		num_connectors_enabled++;
>  
> -		new_crtc = intel_fb_helper_crtc(fb_helper, encoder->crtc);
> +		new_crtc = intel_fb_helper_crtc(fb_helper, connector->state->crtc);
>  
>  		/*
>  		 * Make sure we're not trying to drive multiple connectors
> @@ -466,17 +466,22 @@ retry:
>  			 * usually contains. But since our current
>  			 * code puts a mode derived from the post-pfit timings
>  			 * into crtc->mode this works out correctly.
> +			 *
> +			 * This is crtc->mode and not crtc->state->mode for the
> +			 * fastboot check to work correctly. crtc_state->mode has
> +			 * I915_MODE_FLAG_INHERITED, which we clear to force check
> +			 * state.
>  			 */
>  			DRM_DEBUG_KMS("looking for current mode on connector %s\n",
>  				      connector->name);
> -			modes[i] = &encoder->crtc->mode;
> +			modes[i] = &connector->state->crtc->mode;
>  		}
>  		crtcs[i] = new_crtc;
>  
>  		DRM_DEBUG_KMS("connector %s on pipe %c [CRTC:%d]: %dx%d%s\n",
>  			      connector->name,
> -			      pipe_name(to_intel_crtc(encoder->crtc)->pipe),
> -			      encoder->crtc->base.id,
> +			      pipe_name(to_intel_crtc(connector->state->crtc)->pipe),
> +			      connector->state->crtc->base.id,
>  			      modes[i]->hdisplay, modes[i]->vdisplay,
>  			      modes[i]->flags & DRM_MODE_FLAG_INTERLACE ? "i" :"");
>  
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx