On Wed, Nov 08, 2017 at 10:09:32AM -0800, Manasi Navare wrote: > On Wed, Nov 08, 2017 at 02:28:23PM +0100, Daniel Vetter wrote: > > On Wed, Nov 08, 2017 at 03:26:15PM +0200, Ville Syrjälä wrote: > > > On Wed, Nov 08, 2017 at 02:11:46PM +0100, Daniel Vetter wrote: > > > > On Wed, Nov 08, 2017 at 03:04:58PM +0200, Ville Syrjälä wrote: > > > > > 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. > > > > > > > > Why? One of the patch series Maarten mentioned says there's a deadlock > > > > with dp aux, but it seems to be all just fine when CI retrains. > > This retraining here would be not because of the training failing in the commit but > when we get a short pulse right so when the link was already up and running? > > > > > > > I guess the deadlock is possible only with MST, maybe. Currently MST > > > has it's own copy of the retraining code without the locks. > > > > > > At one point I started to rewrite the entire sink irq handling into a > > > much nicer shape, also unifying the approach between MST and SST. IIRC > > > I did still make the mistake of having some kind of higher level MST > > > vs. SST split, but I think the proper solution is to combine the two > > > almost entirely. I think we should even be using the ESI registers > > > with SST for DPCD 1.2+. Currently we use ESI for MST and non-ESI for > > > SST. Sadly I've not found the time to continue that work (same story > > > with the "shutdown displays prior to rebooting to keep Dell MST > > > monitors working" thing). > > > > Yeah, merging sst and mst more definitely sounds like a good idea. I've > > also been toying with it since forever. > > > > > > And even if there is a deadlock, it's there already, so not sure why we > > > > need to block this bugfix on a different bugfix. Which seems to be what > > > > you're suggesting here (but it's a bit sparse). > > > > > > I guess what I'm really saying is that we shouldn't stop here. > > > > Fully agreed. > > -Daniel > > > > > > > > > -Daniel > > > > > > > > > > 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@xxxxxxxxx> > > > > > > Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx> > > > > > > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > > > > > 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@xxxxxxxxx> > > > > > > --- > > > > > > 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; > > I dont think we need intel_encoder, can we remove this? > > Manasi > Sorry didnt realize we still need to use intel_encoder in the DRM_DEBUG_KMS. Manasi > > > > > > + 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 > > > > > > > > > > -- > > > > > Ville Syrjälä > > > > > Intel OTC > > > > > > > > -- > > > > Daniel Vetter > > > > Software Engineer, Intel Corporation > > > > http://blog.ffwll.ch > > > > > > -- > > > Ville Syrjälä > > > Intel OTC > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
