Re: [PATCH] drm/i915: sync dp link status checks against atomic commmits

On Wed, Nov 08, 2017 at 01:57:50PM +0100, Daniel Vetter wrote:

Two bits:

• check actual atomic state, the legacy stuff can only be looked at from within the atomic_commit_tail function, since it's only protected by ordering and not by any locks.

• Make sure we don't wreak the work an ongoing nonblocking commit is doing.

I still think we need to move the retraining to the hotplug work.

v2: We need the crtc lock too, because a plane update might change it without having to acquire the connection_mutex (Maarten). Use Maarten's changes for this locking, while keeping the logic that uses the connection->commit->hw_done signal for syncing with nonblocking commits.

Cc: Manasi Navare manasi.d.navare@intel.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Ville Syrjälä ville.syrjala@linux.intel.com References: https://bugs.freedesktop.org/show_bug.cgi?id=103336 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99272 Signed-off-by: Daniel Vetter daniel.vetter@intel.com

---

drivers/gpu/drm/i915/intel_dp.c | 56 ++++++++++++++++++++++++++++++++++++----- 1 file changed, 50 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index d27c0145ac91..7cd7ab4fb431 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -4319,6 +4319,8 @@ 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_connector_state *conn_state =

• intel_dp->attached_connector->base.state;
  

  struct drm_device *dev = intel_dp_to_dev(intel_dp); u8 link_status[DP_LINK_STATUS_SIZE];

@@ -4329,10 +4331,15 @@ intel_dp_check_link_status(struct intel_dp *intel_dp) return; }

• if (!intel_encoder->base.crtc)

• if (!conn_state->crtc) return;

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

• WARN_ON(!drm_modeset_is_locked(&conn_state->crtc->mutex));

• if (!conn_state->crtc->state->active)

• return;
  

• if (!try_wait_for_completion(&conn_state->commit->hw_done)) return;

  /*

@@ -4368,7 +4375,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;

@@ -4408,9 +4414,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 */

@@ -4860,8 +4865,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;

@@ -5146,10 +5162,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);
  

• 	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.15.0