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.lankho...@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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
