On Wed, Jun 14, 2017 at 11:08:42AM +0200, Maarten Lankhorst wrote:

The nonblocking modeset tests were failing on systems with a DP output, because lijnk training could occur during the modeset.

With normal modesets we're protected by the connection_mutex lock, but nonblocking modesets drop locks before completing. To get around that, check if the most recent crtc_state has a modeset, and if this is the case, wait for all outstanding atomic updates on that crtc to complete.

This fixes the following spew when running nonblocking tests: [ 1061.653778] ------------[ cut here ]------------ [ 1061.653781] WARNING: CPU: 1 PID: 8165 at drivers/gpu/drm/drm_irq.c:1217 drm_wait_one_vblank+0x165/0x1a0 [ 1061.653782] vblank not available on crtc 0, ret=-22 [ 1061.653783] Modules linked in: vgem snd_hda_intel i915 snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic x86_pkg_temp_thermal intel_powerclamp coretemp crct10dif_pclmul crc32_pclmul ghash_clmulni_intel snd_hda_codec snd_hwdep snd_hda_core snd_pcm mei_me mei e1000e ptp pps_core i2c_hid [last unloaded: vgem] [ 1061.653796] CPU: 1 PID: 8165 Comm: kworker/1:3 Tainted: G U W 4.10.0-rc1-CI-Custom_114+ #1 [ 1061.653797] Hardware name: GIGABYTE GB-BKi5A-7200/MFLP5AP-00, BIOS F1 07/27/2016 [ 1061.653812] Workqueue: events i915_hpd_poll_init_work [i915] [ 1061.653813] Call Trace: [ 1061.653816] dump_stack+0x67/0x92 [ 1061.653818] __warn+0xc6/0xe0 [ 1061.653819] warn_slowpath_fmt+0x4a/0x50 [ 1061.653820] ? drm_vblank_get+0x74/0xc0 [ 1061.653821] drm_wait_one_vblank+0x165/0x1a0 [ 1061.653823] ? drm_dp_dpcd_write+0x16/0x20 [ 1061.653837] ? intel_dp_set_link_train+0x44/0xe0 [i915] [ 1061.653851] intel_dp_check_link_status+0x113/0x1a0 [i915] [ 1061.653864] intel_dp_detect+0x5a1/0xa40 [i915] [ 1061.653866] drm_helper_hpd_irq_event+0x8d/0x110 [ 1061.653880] i915_hpd_poll_init_work+0xab/0xd0 [i915] [ 1061.653882] process_one_work+0x1f4/0x6d0 [ 1061.653883] ? process_one_work+0x16e/0x6d0 [ 1061.653885] worker_thread+0x49/0x4a0 [ 1061.653886] kthread+0x107/0x140 [ 1061.653888] ? process_one_work+0x6d0/0x6d0 [ 1061.653889] ? kthread_create_on_node+0x40/0x40 [ 1061.653891] ret_from_fork+0x27/0x40 [ 1061.653892] ---[ end trace f6037618668d4cbc ]--- [ 1061.656353] [drm:intel_dp_set_idle_link_train [i915]] *ERROR* Timed out waiting for DP idle patterns

Changes since v1:

• intel_dp_detect() now holds connection_mutex and has an acquire_ctx, rewrite patch to take advantage of it.

Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99272 Testcase: kms_atomic_transitions.1x-modeset-transitions-nonblocking

---

drivers/gpu/drm/i915/intel_dp.c | 72 ++++++++++++++++++++++++++++++++++------- 1 file changed, 60 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 64fa774c855b..c7a66883e453 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -4243,21 +4243,32 @@ static void 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 intel_connector *intel_connector = intel_dp->attached_connector;
• struct drm_crtc *crtc;

• WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
• if (!intel_dp_get_link_status(intel_dp, link_status)) {

• DRM_ERROR("Failed to get link status\n");
  

• crtc = intel_connector->base.state->crtc;
• if (!crtc) return;

• }

• if (!intel_encoder->base.crtc)

• WARN_ON(!drm_modeset_is_locked(&crtc->mutex));
• if (!crtc->state->active) return;

• if (!to_intel_crtc(intel_encoder->base.crtc)->active)

• if (drm_atomic_crtc_needs_modeset(crtc->state)) {

• int ret;
  

• /* 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");
  

• }

• if (!intel_dp_get_link_status(intel_dp, link_status)) {

• DRM_ERROR("Failed to get link status\n");
  

  return;

• }

  /*

  • Validate the cached values of intel_dp->link_rate and

@@ -4291,7 +4302,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); struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base; u8 sink_irq_vector = 0; u8 old_sink_count = intel_dp->sink_count;

@@ -4331,9 +4341,8 @@ 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);

• if (intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) { DRM_DEBUG_KMS("Link Training Compliance Test requested\n"); /* Send a Hotplug Uevent to userspace to start modeset */

@@ -4735,8 +4744,19 @@ intel_dp_detect(struct drm_connector *connector, connector->base.id, connector->name);

/* If full detect is not performed yet, do a full detect */

• if (!intel_dp->detect_done)

• if (!intel_dp->detect_done) {

• struct drm_crtc *crtc;
  

• int ret;
  

• crtc = connector->state->crtc;
  

• if (crtc) {
  

• 	ret = drm_modeset_lock(&crtc->mutex, ctx);
  

• 	if (ret)
  

• 		return ret;
  

• }
  

• status = intel_dp_long_pulse(intel_dp->attached_connector);

• }

  intel_dp->detect_done = false;

@@ -5028,10 +5048,38 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) }

if (!intel_dp->is_mst) {

• struct drm_modeset_acquire_ctx ctx;
  

• struct drm_connector *connector = &intel_dp->attached_connector->base;
  

• struct drm_crtc *crtc;
  

• int iret;
  

• drm_modeset_acquire_init(&ctx, 0);
  

+retry:

• iret = drm_modeset_lock(&dev->mode_config.connection_mutex, &ctx);
  

• if (iret)
  

• 	goto err;
  

• crtc = connector->state->crtc;
  

• if (crtc) {
  

• 	iret = drm_modeset_lock(&crtc->mutex, &ctx);
  

Please no modeset locks in the dig_work. Hmm. Looks like we already have it there though :( I thought we had moved the link retraining fully into the hoplug_work, but I guess not. Can you try to do that instead?

• 	if (iret)
  

• 		goto err;
  

• }
  

• if (!intel_dp_short_pulse(intel_dp)) { intel_dp->detect_done = false; goto put_power; }

+err:

• if (iret == -EDEADLK) {
  

• 	drm_modeset_backoff(&ctx);
  

• 	goto retry;
  

• }
  

• drm_modeset_drop_locks(&ctx);
  

• drm_modeset_acquire_fini(&ctx);
  

• WARN(iret, "Acquiring modeset locks failed with %i\n", iret);
  

  }

  ret = IRQ_HANDLED;

-- 2.11.0

---

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

On Wed, Jun 14, 2017 at 08:11:18PM +0200, Maarten Lankhorst wrote:

Op 14-06-17 om 16:52 schreef Ville Syrjälä:

...

On Wed, Jun 14, 2017 at 11:08:41AM +0200, Maarten Lankhorst wrote:

...

Moving the wait to a helper allows callers outside of the core to wait for pending updates to complete.

This can be used to prevent races against nonblocking modesets. Only the hw_done call in swap_state is moved to a helper, doing it for the other callers requires too many changes and I think this is the only useful one to export.

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 86d3093c6c9b..1b068ce1aea9 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -2067,6 +2067,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;
  

-ETIMEDOUT ?

ETIMEDOUT -> /* Connection timed out */

It's the standard "something timed out" errno. Used all over drm.

No I don't think that would match.