[PATCH 0/3] drm/atomic: Add helper for hw_done and fix suspend.

[PATCH 0/3] drm/atomic: Add helper for hw_done and fix suspend.

[Maarten Lankhorst]

Fix hw_done in i915 and make nouveau use drm_atomic_helper_disable_all()
after fixing it to properly disable everything.

Maarten Lankhorst (3):
  drm/atomic: move waiting for hw_done to a helper
  drm/i915: Wait for pending modesets to complete before trying link
    training
  drm/atomic: Make disable_all helper fully disable the crtc.

 drivers/gpu/drm/drm_atomic_helper.c       | 102 +++++++++++++++++++--------
 drivers/gpu/drm/i915/intel_dp.c           |  51 ++++++++++----
 drivers/gpu/drm/nouveau/nouveau_display.c | 113 +-----------------------------
 include/drm/drm_atomic_helper.h           |   1 +
 4 files changed, 115 insertions(+), 152 deletions(-)

-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/3] drm/atomic: move waiting for hw_done to a helper

[Maarten Lankhorst]

This will allow i915 to perform a wait on pending modeset using the
same code.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 51 +++++++++++++++++++++++++++----------
 include/drm/drm_atomic_helper.h     |  1 +
 2 files changed, 38 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 7b71ac48b8a4..3a8a46510c36 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1936,6 +1936,41 @@ void drm_atomic_helper_cleanup_planes(struct drm_device *dev,
 EXPORT_SYMBOL(drm_atomic_helper_cleanup_planes);
 
 /**
+ * drm_atomic_helper_wait_for_hw_done - wait for all previous hw updates to complete.
+ * @crtc: crtc to wait on.
+ *
+ * This function waits for all previous hardware updates queued on @crtc
+ * to complete.
+ *
+ * Returns:
+ * 0 on success, negative error code on failure.
+ */
+int drm_atomic_helper_wait_for_hw_done(struct drm_crtc *crtc)
+{
+	struct drm_crtc_commit *commit;
+	long ret;
+
+	spin_lock(&crtc->commit_lock);
+	commit = list_first_entry_or_null(&crtc->commit_list,
+			struct drm_crtc_commit, commit_entry);
+	if (commit)
+		drm_crtc_commit_get(commit);
+	spin_unlock(&crtc->commit_lock);
+
+	if (!commit)
+		return 0;
+
+	ret = wait_for_completion_timeout(&commit->hw_done, 10 * HZ);
+	drm_crtc_commit_put(commit);
+
+	if (ret == 0)
+		return -EBUSY;
+
+	return ret > 0 ? 0 : ret;
+}
+EXPORT_SYMBOL(drm_atomic_helper_wait_for_hw_done);
+
+/**
  * drm_atomic_helper_swap_state - store atomic state into current sw state
  * @state: atomic state
  * @stall: stall for proceeding commits
@@ -1976,26 +2011,14 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
 	struct drm_crtc_state *crtc_state;
 	struct drm_plane *plane;
 	struct drm_plane_state *plane_state;
-	struct drm_crtc_commit *commit;
 
 	if (stall) {
 		for_each_crtc_in_state(state, crtc, crtc_state, i) {
-			spin_lock(&crtc->commit_lock);
-			commit = list_first_entry_or_null(&crtc->commit_list,
-					struct drm_crtc_commit, commit_entry);
-			if (commit)
-				drm_crtc_commit_get(commit);
-			spin_unlock(&crtc->commit_lock);
+			ret = drm_atomic_helper_wait_for_hw_done(crtc);
 
-			if (!commit)
-				continue;
-
-			ret = wait_for_completion_timeout(&commit->hw_done,
-							  10*HZ);
-			if (ret == 0)
+			if (ret < 0)
 				DRM_ERROR("[CRTC:%d:%s] hw_done timed out\n",
 					  crtc->base.id, crtc->name);
-			drm_crtc_commit_put(commit);
 		}
 	}
 
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index 4b2353dc34ba..e15d2fe7b274 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -77,6 +77,7 @@ void
 drm_atomic_helper_disable_planes_on_crtc(struct drm_crtc_state *old_crtc_state,
 					 bool atomic);
 
+int drm_atomic_helper_wait_for_hw_done(struct drm_crtc *crtc);
 void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
 				  bool stall);
 
-- 
2.7.4

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

[PATCH 2/3] drm/i915: Wait for pending modesets to complete before trying link training

[Maarten Lankhorst]

We're protected by the connection_mutex lock against blocking modesets,
but nonblocking modesets are performed without any locking. This is
causing WARN in drm_wait_for_vblank and in general it's better to wait
before modeset completes before trying anything.

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

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index fb12896bafee..71882d887393 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4080,24 +4080,38 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
 	u8 link_status[DP_LINK_STATUS_SIZE];
+	struct drm_modeset_acquire_ctx ctx;
+	struct intel_connector *intel_connector = intel_dp->attached_connector;
+	int ret;
+	struct drm_crtc *crtc;
 
-	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
+	drm_modeset_acquire_init(&ctx, 0);
+
+retry:
+	ret = drm_modeset_lock(&dev->mode_config.connection_mutex, &ctx);
+	if (ret)
+		goto out;
 
 	if (!intel_dp_get_link_status(intel_dp, link_status)) {
 		DRM_ERROR("Failed to get link status\n");
-		return;
+		goto out;
 	}
 
-	if (!intel_encoder->base.crtc)
-		return;
+	crtc = intel_connector->base.state->crtc;
+	if (!crtc)
+		goto out;
 
-	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
-		return;
+	ret = drm_modeset_lock(&crtc->mutex, &ctx);
+	if (ret)
+		goto out;
+
+	if (!crtc->state->active)
+		goto out;
 
 	/* FIXME: we need to synchronize this sort of stuff with hardware
 	 * readout. Currently fast link training doesn't work on boot-up. */
 	if (!intel_dp->lane_count)
-		return;
+		goto out;
 
 	/* if link training is requested we should perform it always */
 	if ((intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) ||
@@ -4105,8 +4119,26 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 		DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
 			      intel_encoder->base.name);
 
+		if (drm_atomic_crtc_needs_modeset(crtc->state)) {
+			/* wait for atomic modeset to complete */
+			ret = drm_atomic_helper_wait_for_hw_done(crtc);
+			if (ret < 0)
+				DRM_ERROR("Waiting for hw_done timed out\n");
+		}
+
 		intel_dp_retrain_link(intel_dp);
 	}
+
+out:
+	if (ret == -EDEADLK) {
+		drm_modeset_backoff(&ctx);
+		goto retry;
+	}
+
+	drm_modeset_drop_locks(&ctx);
+	drm_modeset_acquire_fini(&ctx);
+	if (ret)
+		DRM_ERROR("Locking failed with %i\n", ret);
 }
 
 /*
@@ -4125,7 +4157,6 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 static bool
 intel_dp_short_pulse(struct intel_dp *intel_dp)
 {
-	struct drm_device *dev = intel_dp_to_dev(intel_dp);
 	u8 sink_irq_vector = 0;
 	u8 old_sink_count = intel_dp->sink_count;
 	bool ret;
@@ -4164,9 +4195,7 @@ intel_dp_short_pulse(struct intel_dp *intel_dp)
 			DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n");
 	}
 
-	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
 	intel_dp_check_link_status(intel_dp);
-	drm_modeset_unlock(&dev->mode_config.connection_mutex);
 
 	return true;
 }
@@ -4499,9 +4528,7 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
 		 * check links status, there has been known issues of
 		 * link loss triggerring long pulse!!!!
 		 */
-		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
 		intel_dp_check_link_status(intel_dp);
-		drm_modeset_unlock(&dev->mode_config.connection_mutex);
 		goto out;
 	}
 
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/3] drm/atomic: Make disable_all helper fully disable the crtc.

[Maarten Lankhorst]

It seems that nouveau requires this, so best to do this in the helper.
This allows nouveau to use the atomic suspend helper.

Cc: nouveau@lists.freedesktop.org
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c       |  51 ++++++++++----
 drivers/gpu/drm/nouveau/nouveau_display.c | 113 +-----------------------------
 2 files changed, 38 insertions(+), 126 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 3a8a46510c36..f010a11874b9 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2436,9 +2436,13 @@ int drm_atomic_helper_disable_all(struct drm_device *dev,
 				  struct drm_modeset_acquire_ctx *ctx)
 {
 	struct drm_atomic_state *state;
+	struct drm_connector_state *conn_state;
 	struct drm_connector *conn;
-	struct drm_connector_list_iter conn_iter;
-	int err;
+	struct drm_plane_state *plane_state;
+	struct drm_plane *plane;
+	struct drm_crtc_state *crtc_state;
+	struct drm_crtc *crtc;
+	int ret, i;
 
 	state = drm_atomic_state_alloc(dev);
 	if (!state)
@@ -2446,29 +2450,48 @@ int drm_atomic_helper_disable_all(struct drm_device *dev,
 
 	state->acquire_ctx = ctx;
 
-	drm_connector_list_iter_get(dev, &conn_iter);
-	drm_for_each_connector_iter(conn, &conn_iter) {
-		struct drm_crtc *crtc = conn->state->crtc;
-		struct drm_crtc_state *crtc_state;
-
-		if (!crtc || conn->dpms != DRM_MODE_DPMS_ON)
-			continue;
-
+	drm_for_each_crtc(crtc, dev) {
 		crtc_state = drm_atomic_get_crtc_state(state, crtc);
 		if (IS_ERR(crtc_state)) {
-			err = PTR_ERR(crtc_state);
+			ret = PTR_ERR(crtc_state);
 			goto free;
 		}
 
 		crtc_state->active = false;
+
+		ret = drm_atomic_set_mode_prop_for_crtc(crtc_state, NULL);
+		if (ret < 0)
+			goto free;
+
+		ret = drm_atomic_add_affected_planes(state, crtc);
+		if (ret < 0)
+			goto free;
+
+		ret = drm_atomic_add_affected_connectors(state, crtc);
+		if (ret < 0)
+			goto free;
 	}
 
-	err = drm_atomic_commit(state);
+	for_each_connector_in_state(state, conn, conn_state, i) {
+		ret = drm_atomic_set_crtc_for_connector(conn_state, NULL);
+		if (ret < 0)
+			goto free;
+	}
+
+	for_each_plane_in_state(state, plane, plane_state, i) {
+		ret = drm_atomic_set_crtc_for_plane(plane_state, NULL);
+		if (ret < 0)
+			goto free;
+
+		drm_atomic_set_fb_for_plane(plane_state, NULL);
+	}
+
+	ret = drm_atomic_commit(state);
 free:
-	drm_connector_list_iter_put(&conn_iter);
 	drm_atomic_state_put(state);
-	return err;
+	return ret;
 }
+
 EXPORT_SYMBOL(drm_atomic_helper_disable_all);
 
 /**
diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
index add353e230f4..09f195f8756f 100644
--- a/drivers/gpu/drm/nouveau/nouveau_display.c
+++ b/drivers/gpu/drm/nouveau/nouveau_display.c
@@ -621,117 +621,6 @@ nouveau_display_destroy(struct drm_device *dev)
 	kfree(disp);
 }
 
-static int
-nouveau_atomic_disable_connector(struct drm_atomic_state *state,
-				 struct drm_connector *connector)
-{
-	struct drm_connector_state *connector_state;
-	struct drm_crtc *crtc;
-	struct drm_crtc_state *crtc_state;
-	struct drm_plane_state *plane_state;
-	struct drm_plane *plane;
-	int ret;
-
-	if (!(crtc = connector->state->crtc))
-		return 0;
-
-	connector_state = drm_atomic_get_connector_state(state, connector);
-	if (IS_ERR(connector_state))
-		return PTR_ERR(connector_state);
-
-	ret = drm_atomic_set_crtc_for_connector(connector_state, NULL);
-	if (ret)
-		return ret;
-
-	crtc_state = drm_atomic_get_crtc_state(state, crtc);
-	if (IS_ERR(crtc_state))
-		return PTR_ERR(crtc_state);
-
-	ret = drm_atomic_set_mode_for_crtc(crtc_state, NULL);
-	if (ret)
-		return ret;
-
-	crtc_state->active = false;
-
-	drm_for_each_plane_mask(plane, connector->dev, crtc_state->plane_mask) {
-		plane_state = drm_atomic_get_plane_state(state, plane);
-		if (IS_ERR(plane_state))
-			return PTR_ERR(plane_state);
-
-		ret = drm_atomic_set_crtc_for_plane(plane_state, NULL);
-		if (ret)
-			return ret;
-
-		drm_atomic_set_fb_for_plane(plane_state, NULL);
-	}
-
-	return 0;
-}
-
-static int
-nouveau_atomic_disable(struct drm_device *dev,
-		       struct drm_modeset_acquire_ctx *ctx)
-{
-	struct drm_atomic_state *state;
-	struct drm_connector *connector;
-	int ret;
-
-	state = drm_atomic_state_alloc(dev);
-	if (!state)
-		return -ENOMEM;
-
-	state->acquire_ctx = ctx;
-
-	drm_for_each_connector(connector, dev) {
-		ret = nouveau_atomic_disable_connector(state, connector);
-		if (ret)
-			break;
-	}
-
-	if (ret == 0)
-		ret = drm_atomic_commit(state);
-	drm_atomic_state_put(state);
-	return ret;
-}
-
-static struct drm_atomic_state *
-nouveau_atomic_suspend(struct drm_device *dev)
-{
-	struct drm_modeset_acquire_ctx ctx;
-	struct drm_atomic_state *state;
-	int ret;
-
-	drm_modeset_acquire_init(&ctx, 0);
-
-retry:
-	ret = drm_modeset_lock_all_ctx(dev, &ctx);
-	if (ret < 0) {
-		state = ERR_PTR(ret);
-		goto unlock;
-	}
-
-	state = drm_atomic_helper_duplicate_state(dev, &ctx);
-	if (IS_ERR(state))
-		goto unlock;
-
-	ret = nouveau_atomic_disable(dev, &ctx);
-	if (ret < 0) {
-		drm_atomic_state_put(state);
-		state = ERR_PTR(ret);
-		goto unlock;
-	}
-
-unlock:
-	if (PTR_ERR(state) == -EDEADLK) {
-		drm_modeset_backoff(&ctx);
-		goto retry;
-	}
-
-	drm_modeset_drop_locks(&ctx);
-	drm_modeset_acquire_fini(&ctx);
-	return state;
-}
-
 int
 nouveau_display_suspend(struct drm_device *dev, bool runtime)
 {
@@ -740,7 +629,7 @@ nouveau_display_suspend(struct drm_device *dev, bool runtime)
 
 	if (drm_drv_uses_atomic_modeset(dev)) {
 		if (!runtime) {
-			disp->suspend = nouveau_atomic_suspend(dev);
+			disp->suspend = drm_atomic_helper_suspend(dev);
 			if (IS_ERR(disp->suspend)) {
 				int ret = PTR_ERR(disp->suspend);
 				disp->suspend = NULL;
-- 
2.7.4

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

✓ Fi.CI.BAT: success for drm/atomic: Add helper for hw_done and fix suspend.

[Patchwork]

== Series Details ==

Series: drm/atomic: Add helper for hw_done and fix suspend.
URL   : https://patchwork.freedesktop.org/series/17712/
State : success

== Summary ==

Series 17712v1 drm/atomic: Add helper for hw_done and fix suspend.
https://patchwork.freedesktop.org/api/1.0/series/17712/revisions/1/mbox/

Test kms_force_connector_basic:
        Subgroup force-connector-state:
                dmesg-warn -> PASS       (fi-snb-2520m)
        Subgroup prune-stale-modes:
                skip       -> PASS       (fi-ivb-3520m)

fi-bdw-5557u     total:246  pass:232  dwarn:0   dfail:0   fail:0   skip:14 
fi-bsw-n3050     total:246  pass:207  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:246  pass:224  dwarn:0   dfail:0   fail:0   skip:22 
fi-bxt-t5700     total:82   pass:69   dwarn:0   dfail:0   fail:0   skip:12 
fi-byt-j1900     total:246  pass:219  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:246  pass:215  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:246  pass:227  dwarn:0   dfail:0   fail:0   skip:19 
fi-hsw-4770r     total:246  pass:227  dwarn:0   dfail:0   fail:0   skip:19 
fi-ivb-3520m     total:246  pass:225  dwarn:0   dfail:0   fail:0   skip:21 
fi-ivb-3770      total:246  pass:225  dwarn:0   dfail:0   fail:0   skip:21 
fi-kbl-7500u     total:246  pass:225  dwarn:0   dfail:0   fail:0   skip:21 
fi-skl-6260u     total:246  pass:233  dwarn:0   dfail:0   fail:0   skip:13 
fi-skl-6700hq    total:246  pass:226  dwarn:0   dfail:0   fail:0   skip:20 
fi-skl-6700k     total:246  pass:222  dwarn:3   dfail:0   fail:0   skip:21 
fi-skl-6770hq    total:246  pass:233  dwarn:0   dfail:0   fail:0   skip:13 
fi-snb-2520m     total:246  pass:215  dwarn:0   dfail:0   fail:0   skip:31 
fi-snb-2600      total:246  pass:214  dwarn:0   dfail:0   fail:0   skip:32 

9ea6a075c23ea914695d57944c0e74cff0c6bff4 drm-tip: 2017y-01m-09d-16h-11m-21s UTC integration manifest
3e39051 drm/atomic: Make disable_all helper fully disable the crtc.
644e019 drm/i915: Wait for pending modesets to complete before trying link training
89362fc drm/atomic: move waiting for hw_done to a helper

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3458/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/atomic: move waiting for hw_done to a helper

[Ville Syrjälä]

On Mon, Jan 09, 2017 at 06:06:56PM +0100, Maarten Lankhorst wrote:
> This will allow i915 to perform a wait on pending modeset using the
> same code.

I don't like commit messages that refer to the subject like this. I
can't even see the subject when I'm replying so I have approximately
zero idea what's being said here.

> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 51 +++++++++++++++++++++++++++----------
>  include/drm/drm_atomic_helper.h     |  1 +
>  2 files changed, 38 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 7b71ac48b8a4..3a8a46510c36 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1936,6 +1936,41 @@ void drm_atomic_helper_cleanup_planes(struct drm_device *dev,
>  EXPORT_SYMBOL(drm_atomic_helper_cleanup_planes);
>  
>  /**
> + * drm_atomic_helper_wait_for_hw_done - wait for all previous hw updates to complete.
> + * @crtc: crtc to wait on.
> + *
> + * This function waits for all previous hardware updates queued on @crtc
> + * to complete.
> + *
> + * Returns:
> + * 0 on success, negative error code on failure.
> + */
> +int drm_atomic_helper_wait_for_hw_done(struct drm_crtc *crtc)
> +{
> +	struct drm_crtc_commit *commit;
> +	long ret;
> +
> +	spin_lock(&crtc->commit_lock);
> +	commit = list_first_entry_or_null(&crtc->commit_list,
> +			struct drm_crtc_commit, commit_entry);
> +	if (commit)
> +		drm_crtc_commit_get(commit);
> +	spin_unlock(&crtc->commit_lock);
> +
> +	if (!commit)
> +		return 0;
> +
> +	ret = wait_for_completion_timeout(&commit->hw_done, 10 * HZ);
> +	drm_crtc_commit_put(commit);
> +
> +	if (ret == 0)
> +		return -EBUSY;
> +
> +	return ret > 0 ? 0 : ret;
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_wait_for_hw_done);

Seems sane enough. I was pondering if we could share the code with the
other hw_done wait as well. But it grabs a different commit from the
list, so not sure if we'd end up with less code or not.

Hmm. maybe pass the commit index from the caller all the way from the
caller to preceeding_commit() (which would obviously need a rename)?

> +
> +/**
>   * drm_atomic_helper_swap_state - store atomic state into current sw state
>   * @state: atomic state
>   * @stall: stall for proceeding commits
> @@ -1976,26 +2011,14 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
>  	struct drm_crtc_state *crtc_state;
>  	struct drm_plane *plane;
>  	struct drm_plane_state *plane_state;
> -	struct drm_crtc_commit *commit;
>  
>  	if (stall) {
>  		for_each_crtc_in_state(state, crtc, crtc_state, i) {
> -			spin_lock(&crtc->commit_lock);
> -			commit = list_first_entry_or_null(&crtc->commit_list,
> -					struct drm_crtc_commit, commit_entry);
> -			if (commit)
> -				drm_crtc_commit_get(commit);
> -			spin_unlock(&crtc->commit_lock);
> +			ret = drm_atomic_helper_wait_for_hw_done(crtc);
>  
> -			if (!commit)
> -				continue;
> -
> -			ret = wait_for_completion_timeout(&commit->hw_done,
> -							  10*HZ);
> -			if (ret == 0)
> +			if (ret < 0)
>  				DRM_ERROR("[CRTC:%d:%s] hw_done timed out\n",
>  					  crtc->base.id, crtc->name);
> -			drm_crtc_commit_put(commit);
>  		}
>  	}
>  
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index 4b2353dc34ba..e15d2fe7b274 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -77,6 +77,7 @@ void
>  drm_atomic_helper_disable_planes_on_crtc(struct drm_crtc_state *old_crtc_state,
>  					 bool atomic);
>  
> +int drm_atomic_helper_wait_for_hw_done(struct drm_crtc *crtc);
>  void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
>  				  bool stall);
>  
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://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

Re: [PATCH 2/3] drm/i915: Wait for pending modesets to complete before trying link training

[Ville Syrjälä]

On Mon, Jan 09, 2017 at 06:06:57PM +0100, Maarten Lankhorst wrote:
> We're protected by the connection_mutex lock against blocking modesets,
> but nonblocking modesets are performed without any locking. This is
> causing WARN in drm_wait_for_vblank and in general it's better to wait
> before modeset completes before trying anything.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 51 +++++++++++++++++++++++++++++++----------
>  1 file changed, 39 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index fb12896bafee..71882d887393 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4080,24 +4080,38 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>  	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
>  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
>  	u8 link_status[DP_LINK_STATUS_SIZE];
> +	struct drm_modeset_acquire_ctx ctx;
> +	struct intel_connector *intel_connector = intel_dp->attached_connector;
> +	int ret;
> +	struct drm_crtc *crtc;
>  
> -	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
> +	drm_modeset_acquire_init(&ctx, 0);
> +
> +retry:
> +	ret = drm_modeset_lock(&dev->mode_config.connection_mutex, &ctx);
> +	if (ret)
> +		goto out;
>  
>  	if (!intel_dp_get_link_status(intel_dp, link_status)) {
>  		DRM_ERROR("Failed to get link status\n");
> -		return;
> +		goto out;
>  	}
>  
> -	if (!intel_encoder->base.crtc)
> -		return;
> +	crtc = intel_connector->base.state->crtc;
> +	if (!crtc)
> +		goto out;
>  
> -	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
> -		return;
> +	ret = drm_modeset_lock(&crtc->mutex, &ctx);
> +	if (ret)
> +		goto out;
> +
> +	if (!crtc->state->active)
> +		goto out;
>  
>  	/* FIXME: we need to synchronize this sort of stuff with hardware
>  	 * readout. Currently fast link training doesn't work on boot-up. */
>  	if (!intel_dp->lane_count)
> -		return;
> +		goto out;
>  
>  	/* if link training is requested we should perform it always */
>  	if ((intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) ||
> @@ -4105,8 +4119,26 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>  		DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
>  			      intel_encoder->base.name);
>  
> +		if (drm_atomic_crtc_needs_modeset(crtc->state)) {
> +			/* wait for atomic modeset to complete */
> +			ret = drm_atomic_helper_wait_for_hw_done(crtc);
> +			if (ret < 0)
> +				DRM_ERROR("Waiting for hw_done timed out\n");

I wonder if we should just skip the hw frobbing in this case. I guess it
might still restore the link if the commit just somehow got stuck
somewhere non-critical.

> +		}
> +
>  		intel_dp_retrain_link(intel_dp);
>  	}
> +
> +out:
> +	if (ret == -EDEADLK) {
> +		drm_modeset_backoff(&ctx);
> +		goto retry;
> +	}
> +
> +	drm_modeset_drop_locks(&ctx);
> +	drm_modeset_acquire_fini(&ctx);
> +	if (ret)
> +		DRM_ERROR("Locking failed with %i\n", ret);

This seems incorrect if we timed out on the wait. Maybe just
clear ret after the hw_done wait?

>  }
>  
>  /*
> @@ -4125,7 +4157,6 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>  static bool
>  intel_dp_short_pulse(struct intel_dp *intel_dp)
>  {
> -	struct drm_device *dev = intel_dp_to_dev(intel_dp);
>  	u8 sink_irq_vector = 0;
>  	u8 old_sink_count = intel_dp->sink_count;
>  	bool ret;
> @@ -4164,9 +4195,7 @@ intel_dp_short_pulse(struct intel_dp *intel_dp)
>  			DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n");
>  	}
>  
> -	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>  	intel_dp_check_link_status(intel_dp);
> -	drm_modeset_unlock(&dev->mode_config.connection_mutex);
>  
>  	return true;
>  }
> @@ -4499,9 +4528,7 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
>  		 * check links status, there has been known issues of
>  		 * link loss triggerring long pulse!!!!
>  		 */
> -		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>  		intel_dp_check_link_status(intel_dp);
> -		drm_modeset_unlock(&dev->mode_config.connection_mutex);
>  		goto out;
>  	}
>  
> -- 
> 2.7.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [Intel-gfx] [PATCH 1/3] drm/atomic: move waiting for hw_done to a helper

[Maarten Lankhorst]

Op 26-01-17 om 19:29 schreef Ville Syrjälä:
> On Mon, Jan 09, 2017 at 06:06:56PM +0100, Maarten Lankhorst wrote:
>> This will allow i915 to perform a wait on pending modeset using the
>> same code.
> I don't like commit messages that refer to the subject like this. I
> can't even see the subject when I'm replying so I have approximately
> zero idea what's being said here.
>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/drm_atomic_helper.c | 51 +++++++++++++++++++++++++++----------
>>  include/drm/drm_atomic_helper.h     |  1 +
>>  2 files changed, 38 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>> index 7b71ac48b8a4..3a8a46510c36 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -1936,6 +1936,41 @@ void drm_atomic_helper_cleanup_planes(struct drm_device *dev,
>>  EXPORT_SYMBOL(drm_atomic_helper_cleanup_planes);
>>  
>>  /**
>> + * drm_atomic_helper_wait_for_hw_done - wait for all previous hw updates to complete.
>> + * @crtc: crtc to wait on.
>> + *
>> + * This function waits for all previous hardware updates queued on @crtc
>> + * to complete.
>> + *
>> + * Returns:
>> + * 0 on success, negative error code on failure.
>> + */
>> +int drm_atomic_helper_wait_for_hw_done(struct drm_crtc *crtc)
>> +{
>> +	struct drm_crtc_commit *commit;
>> +	long ret;
>> +
>> +	spin_lock(&crtc->commit_lock);
>> +	commit = list_first_entry_or_null(&crtc->commit_list,
>> +			struct drm_crtc_commit, commit_entry);
>> +	if (commit)
>> +		drm_crtc_commit_get(commit);
>> +	spin_unlock(&crtc->commit_lock);
>> +
>> +	if (!commit)
>> +		return 0;
>> +
>> +	ret = wait_for_completion_timeout(&commit->hw_done, 10 * HZ);
>> +	drm_crtc_commit_put(commit);
>> +
>> +	if (ret == 0)
>> +		return -EBUSY;
>> +
>> +	return ret > 0 ? 0 : ret;
>> +}
>> +EXPORT_SYMBOL(drm_atomic_helper_wait_for_hw_done);
> Seems sane enough. I was pondering if we could share the code with the
> other hw_done wait as well. But it grabs a different commit from the
> list, so not sure if we'd end up with less code or not.
>
> Hmm. maybe pass the commit index from the caller all the way from the
> caller to preceeding_commit() (which would obviously need a rename)?

I think this helper is useful for waiting for crtc idle, I'm not sure it's useful in
general to wait for other commits, since we probably want to wait for all previous
commits to be completed.

~Maarten

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/i915: Wait for pending modesets to complete before trying link training

[Maarten Lankhorst]

Op 26-01-17 om 19:31 schreef Ville Syrjälä:
> On Mon, Jan 09, 2017 at 06:06:57PM +0100, Maarten Lankhorst wrote:
>> We're protected by the connection_mutex lock against blocking modesets,
>> but nonblocking modesets are performed without any locking. This is
>> causing WARN in drm_wait_for_vblank and in general it's better to wait
>> before modeset completes before trying anything.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_dp.c | 51 +++++++++++++++++++++++++++++++----------
>>  1 file changed, 39 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index fb12896bafee..71882d887393 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -4080,24 +4080,38 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>>  	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
>>  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
>>  	u8 link_status[DP_LINK_STATUS_SIZE];
>> +	struct drm_modeset_acquire_ctx ctx;
>> +	struct intel_connector *intel_connector = intel_dp->attached_connector;
>> +	int ret;
>> +	struct drm_crtc *crtc;
>>  
>> -	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
>> +	drm_modeset_acquire_init(&ctx, 0);
>> +
>> +retry:
>> +	ret = drm_modeset_lock(&dev->mode_config.connection_mutex, &ctx);
>> +	if (ret)
>> +		goto out;
>>  
>>  	if (!intel_dp_get_link_status(intel_dp, link_status)) {
>>  		DRM_ERROR("Failed to get link status\n");
>> -		return;
>> +		goto out;
>>  	}
>>  
>> -	if (!intel_encoder->base.crtc)
>> -		return;
>> +	crtc = intel_connector->base.state->crtc;
>> +	if (!crtc)
>> +		goto out;
>>  
>> -	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
>> -		return;
>> +	ret = drm_modeset_lock(&crtc->mutex, &ctx);
>> +	if (ret)
>> +		goto out;
>> +
>> +	if (!crtc->state->active)
>> +		goto out;
>>  
>>  	/* FIXME: we need to synchronize this sort of stuff with hardware
>>  	 * readout. Currently fast link training doesn't work on boot-up. */
>>  	if (!intel_dp->lane_count)
>> -		return;
>> +		goto out;
>>  
>>  	/* if link training is requested we should perform it always */
>>  	if ((intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) ||
>> @@ -4105,8 +4119,26 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>>  		DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
>>  			      intel_encoder->base.name);
>>  
>> +		if (drm_atomic_crtc_needs_modeset(crtc->state)) {
>> +			/* wait for atomic modeset to complete */
>> +			ret = drm_atomic_helper_wait_for_hw_done(crtc);
>> +			if (ret < 0)
>> +				DRM_ERROR("Waiting for hw_done timed out\n");
> I wonder if we should just skip the hw frobbing in this case. I guess it
> might still restore the link if the commit just somehow got stuck
> somewhere non-critical.
Hm,  we could check if the link training is still required after the wait.
Maybe even do the wait unlocked, since it might take an arbitrary amount of time
and then retry?
>> +		}
>> +
>>  		intel_dp_retrain_link(intel_dp);
>>  	}
>> +
>> +out:
>> +	if (ret == -EDEADLK) {
>> +		drm_modeset_backoff(&ctx);
>> +		goto retry;
>> +	}
>> +
>> +	drm_modeset_drop_locks(&ctx);
>> +	drm_modeset_acquire_fini(&ctx);
>> +	if (ret)
>> +		DRM_ERROR("Locking failed with %i\n", ret);
> This seems incorrect if we timed out on the wait. Maybe just
> clear ret after the hw_done wait?
>
>>  }
>>  
>>  /*
>> @@ -4125,7 +4157,6 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>>  static bool
>>  intel_dp_short_pulse(struct intel_dp *intel_dp)
>>  {
>> -	struct drm_device *dev = intel_dp_to_dev(intel_dp);
>>  	u8 sink_irq_vector = 0;
>>  	u8 old_sink_count = intel_dp->sink_count;
>>  	bool ret;
>> @@ -4164,9 +4195,7 @@ intel_dp_short_pulse(struct intel_dp *intel_dp)
>>  			DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n");
>>  	}
>>  
>> -	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>>  	intel_dp_check_link_status(intel_dp);
>> -	drm_modeset_unlock(&dev->mode_config.connection_mutex);
>>  
>>  	return true;
>>  }
>> @@ -4499,9 +4528,7 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
>>  		 * check links status, there has been known issues of
>>  		 * link loss triggerring long pulse!!!!
>>  		 */
>> -		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>>  		intel_dp_check_link_status(intel_dp);
>> -		drm_modeset_unlock(&dev->mode_config.connection_mutex);
>>  		goto out;
>>  	}
>>  
>> -- 
>> 2.7.4
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel


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

Re: [PATCH 2/3] drm/i915: Wait for pending modesets to complete before trying link training

[Ville Syrjälä]

On Thu, Feb 09, 2017 at 12:27:12PM +0100, Maarten Lankhorst wrote:
> Op 26-01-17 om 19:31 schreef Ville Syrjälä:
> > On Mon, Jan 09, 2017 at 06:06:57PM +0100, Maarten Lankhorst wrote:
> >> We're protected by the connection_mutex lock against blocking modesets,
> >> but nonblocking modesets are performed without any locking. This is
> >> causing WARN in drm_wait_for_vblank and in general it's better to wait
> >> before modeset completes before trying anything.
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_dp.c | 51 +++++++++++++++++++++++++++++++----------
> >>  1 file changed, 39 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> >> index fb12896bafee..71882d887393 100644
> >> --- a/drivers/gpu/drm/i915/intel_dp.c
> >> +++ b/drivers/gpu/drm/i915/intel_dp.c
> >> @@ -4080,24 +4080,38 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
> >>  	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
> >>  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
> >>  	u8 link_status[DP_LINK_STATUS_SIZE];
> >> +	struct drm_modeset_acquire_ctx ctx;
> >> +	struct intel_connector *intel_connector = intel_dp->attached_connector;
> >> +	int ret;
> >> +	struct drm_crtc *crtc;
> >>  
> >> -	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
> >> +	drm_modeset_acquire_init(&ctx, 0);
> >> +
> >> +retry:
> >> +	ret = drm_modeset_lock(&dev->mode_config.connection_mutex, &ctx);
> >> +	if (ret)
> >> +		goto out;
> >>  
> >>  	if (!intel_dp_get_link_status(intel_dp, link_status)) {
> >>  		DRM_ERROR("Failed to get link status\n");
> >> -		return;
> >> +		goto out;
> >>  	}
> >>  
> >> -	if (!intel_encoder->base.crtc)
> >> -		return;
> >> +	crtc = intel_connector->base.state->crtc;
> >> +	if (!crtc)
> >> +		goto out;
> >>  
> >> -	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
> >> -		return;
> >> +	ret = drm_modeset_lock(&crtc->mutex, &ctx);
> >> +	if (ret)
> >> +		goto out;
> >> +
> >> +	if (!crtc->state->active)
> >> +		goto out;
> >>  
> >>  	/* FIXME: we need to synchronize this sort of stuff with hardware
> >>  	 * readout. Currently fast link training doesn't work on boot-up. */
> >>  	if (!intel_dp->lane_count)
> >> -		return;
> >> +		goto out;
> >>  
> >>  	/* if link training is requested we should perform it always */
> >>  	if ((intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) ||
> >> @@ -4105,8 +4119,26 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
> >>  		DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
> >>  			      intel_encoder->base.name);
> >>  
> >> +		if (drm_atomic_crtc_needs_modeset(crtc->state)) {
> >> +			/* wait for atomic modeset to complete */
> >> +			ret = drm_atomic_helper_wait_for_hw_done(crtc);
> >> +			if (ret < 0)
> >> +				DRM_ERROR("Waiting for hw_done timed out\n");
> > I wonder if we should just skip the hw frobbing in this case. I guess it
> > might still restore the link if the commit just somehow got stuck
> > somewhere non-critical.
> Hm,  we could check if the link training is still required after the wait.
> Maybe even do the wait unlocked, since it might take an arbitrary amount of time
> and then retry?

I'm not sure there's much point in trying to make this fancy. Link
problems should not happen with any regularity.


> >> +		}
> >> +
> >>  		intel_dp_retrain_link(intel_dp);
> >>  	}
> >> +
> >> +out:
> >> +	if (ret == -EDEADLK) {
> >> +		drm_modeset_backoff(&ctx);
> >> +		goto retry;
> >> +	}
> >> +
> >> +	drm_modeset_drop_locks(&ctx);
> >> +	drm_modeset_acquire_fini(&ctx);
> >> +	if (ret)
> >> +		DRM_ERROR("Locking failed with %i\n", ret);
> > This seems incorrect if we timed out on the wait. Maybe just
> > clear ret after the hw_done wait?
> >
> >>  }
> >>  
> >>  /*
> >> @@ -4125,7 +4157,6 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
> >>  static bool
> >>  intel_dp_short_pulse(struct intel_dp *intel_dp)
> >>  {
> >> -	struct drm_device *dev = intel_dp_to_dev(intel_dp);
> >>  	u8 sink_irq_vector = 0;
> >>  	u8 old_sink_count = intel_dp->sink_count;
> >>  	bool ret;
> >> @@ -4164,9 +4195,7 @@ intel_dp_short_pulse(struct intel_dp *intel_dp)
> >>  			DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n");
> >>  	}
> >>  
> >> -	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> >>  	intel_dp_check_link_status(intel_dp);
> >> -	drm_modeset_unlock(&dev->mode_config.connection_mutex);
> >>  
> >>  	return true;
> >>  }
> >> @@ -4499,9 +4528,7 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
> >>  		 * check links status, there has been known issues of
> >>  		 * link loss triggerring long pulse!!!!
> >>  		 */
> >> -		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> >>  		intel_dp_check_link_status(intel_dp);
> >> -		drm_modeset_unlock(&dev->mode_config.connection_mutex);
> >>  		goto out;
> >>  	}
> >>  
> >> -- 
> >> 2.7.4
> >>
> >> _______________________________________________
> >> dri-devel mailing list
> >> dri-devel@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

-- 
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: Wait for pending modesets to complete before trying link training

[Maarten Lankhorst]

Op 09-02-17 om 13:34 schreef Ville Syrjälä:
> On Thu, Feb 09, 2017 at 12:27:12PM +0100, Maarten Lankhorst wrote:
>> Op 26-01-17 om 19:31 schreef Ville Syrjälä:
>>> On Mon, Jan 09, 2017 at 06:06:57PM +0100, Maarten Lankhorst wrote:
>>>> We're protected by the connection_mutex lock against blocking modesets,
>>>> but nonblocking modesets are performed without any locking. This is
>>>> causing WARN in drm_wait_for_vblank and in general it's better to wait
>>>> before modeset completes before trying anything.
>>>>
>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> ---
>>>>  drivers/gpu/drm/i915/intel_dp.c | 51 +++++++++++++++++++++++++++++++----------
>>>>  1 file changed, 39 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>>> index fb12896bafee..71882d887393 100644
>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>>> @@ -4080,24 +4080,38 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>>>>  	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
>>>>  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
>>>>  	u8 link_status[DP_LINK_STATUS_SIZE];
>>>> +	struct drm_modeset_acquire_ctx ctx;
>>>> +	struct intel_connector *intel_connector = intel_dp->attached_connector;
>>>> +	int ret;
>>>> +	struct drm_crtc *crtc;
>>>>  
>>>> -	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
>>>> +	drm_modeset_acquire_init(&ctx, 0);
>>>> +
>>>> +retry:
>>>> +	ret = drm_modeset_lock(&dev->mode_config.connection_mutex, &ctx);
>>>> +	if (ret)
>>>> +		goto out;
>>>>  
>>>>  	if (!intel_dp_get_link_status(intel_dp, link_status)) {
>>>>  		DRM_ERROR("Failed to get link status\n");
>>>> -		return;
>>>> +		goto out;
>>>>  	}
>>>>  
>>>> -	if (!intel_encoder->base.crtc)
>>>> -		return;
>>>> +	crtc = intel_connector->base.state->crtc;
>>>> +	if (!crtc)
>>>> +		goto out;
>>>>  
>>>> -	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
>>>> -		return;
>>>> +	ret = drm_modeset_lock(&crtc->mutex, &ctx);
>>>> +	if (ret)
>>>> +		goto out;
>>>> +
>>>> +	if (!crtc->state->active)
>>>> +		goto out;
>>>>  
>>>>  	/* FIXME: we need to synchronize this sort of stuff with hardware
>>>>  	 * readout. Currently fast link training doesn't work on boot-up. */
>>>>  	if (!intel_dp->lane_count)
>>>> -		return;
>>>> +		goto out;
>>>>  
>>>>  	/* if link training is requested we should perform it always */
>>>>  	if ((intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) ||
>>>> @@ -4105,8 +4119,26 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>>>>  		DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
>>>>  			      intel_encoder->base.name);
>>>>  
>>>> +		if (drm_atomic_crtc_needs_modeset(crtc->state)) {
>>>> +			/* wait for atomic modeset to complete */
>>>> +			ret = drm_atomic_helper_wait_for_hw_done(crtc);
>>>> +			if (ret < 0)
>>>> +				DRM_ERROR("Waiting for hw_done timed out\n");
>>> I wonder if we should just skip the hw frobbing in this case. I guess it
>>> might still restore the link if the commit just somehow got stuck
>>> somewhere non-critical.
>> Hm,  we could check if the link training is still required after the wait.
>> Maybe even do the wait unlocked, since it might take an arbitrary amount of time
>> and then retry?
> I'm not sure there's much point in trying to make this fancy. Link
> problems should not happen with any regularity.
Indeed, lets just do the simple thing then.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel