Re: [PATCH 07/12] drm/i915: Move DP link retraining into intel_dp_detect()

[Manasi Navare <manasi.d.navare@intel.com>]

On Fri, Jul 29, 2016 at 12:52:55PM +0300, Ville Syrjälä wrote:
> On Thu, Jul 28, 2016 at 05:36:14PM -0700, Manasi Navare wrote:
> > On Thu, Jul 28, 2016 at 11:15:22PM +0300, Ville Syrjälä wrote:
> > > On Thu, Jul 28, 2016 at 12:48:53PM -0700, Manasi Navare wrote:
> > > > On Thu, Jul 28, 2016 at 05:50:43PM +0300, ville.syrjala@linux.intel.com wrote:
> > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > 
> > > > > DP link retraining needs to grab some modeset locks to not race with
> > > > > modesets, so we can't really do it safely from the hpd_pulse, lest we
> > > > > risk deadlocking due to MST sideband stuff.
> > > > > 
> > > > > Move the link retraining to happen from the hotplug work instead.
> > > > > Doing at the end of intel_dp_detect() seems like a good place in case
> > > > > the sink already got disconnected, in which case retraining is
> > > > > pointless.
> > > > > 
> > > > > To determine if we need to schedule the hotplug work, we'll just check
> > > > > the sink lane status without locks from hpd_pulse. A little racy
> > > > > still eg. due to useing intel_dp->lane_count, but no less racy than
> > > > > what we already had. We'll repeat the check in from intel_dp_detect()
> > > > > with proper locking, where we'll also check if the link as actually
> > > > > active or not.
> > > > > 
> > > > > Cc: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> > > > > Cc: Jim Bride <jim.bride@linux.intel.com>
> > > > > Cc: Manasi D Navare <manasi.d.navare@intel.com>
> > > > > Cc: Durgadoss R <durgadoss.r@intel.com>
> > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_dp.c | 154 +++++++++++++++++-----------------------
> > > > >  1 file changed, 66 insertions(+), 88 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > > > index 4a4184c21989..675b83f57a07 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > > @@ -3842,15 +3842,6 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
> > > > >  		bret = intel_dp_get_sink_irq_esi(intel_dp, esi);
> > > > >  go_again:
> > > > >  		if (bret == true) {
> > > > > -
> > > > > -			/* check link status - esi[10] = 0x200c */
> > > > > -			if (intel_dp->active_streams &&
> > > > > -			    !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) {
> > > > > -				DRM_DEBUG_KMS("channel EQ not ok, retraining\n");
> > > > > -				intel_dp_start_link_train(intel_dp);
> > > > > -				intel_dp_stop_link_train(intel_dp);
> > > > > -			}
> > > > > -
> > > > >  			DRM_DEBUG_KMS("got esi %3ph\n", esi);
> > > > >  			ret = drm_dp_mst_hpd_irq(&intel_dp->mst_mgr, esi, &handled);
> > > > >  
> > > > > @@ -3886,34 +3877,42 @@ go_again:
> > > > >  	return -EINVAL;
> > > > >  }
> > > > >  
> > > > > -static void
> > > > > -intel_dp_check_link_status(struct intel_dp *intel_dp)
> > > > > +static bool
> > > > > +intel_dp_link_needs_retrain(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];
> > > > >  
> > > > > -	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
> > > > > +	if (intel_dp->active_streams == 0)
> > > > > +		return false;
> > > > > +
> > > > > +	if (intel_dp->compliance_test_type == DP_TEST_LINK_TRAINING)
> > > > > +		return true;
> > > > >  
> > > > >  	if (!intel_dp_get_link_status(intel_dp, link_status)) {
> > > > >  		DRM_ERROR("Failed to get link status\n");
> > > > > -		return;
> > > > > +		return false;
> > > > >  	}
> > > > >  
> > > > > -	if (!intel_encoder->base.crtc)
> > > > > -		return;
> > > > > +	return !drm_dp_channel_eq_ok(link_status, intel_dp->lane_count);
> > > > 
> > > > According to the DP spec, we should also check for the clock recovery bit in DPCD
> > > > We should also add a check drm_dp_clock_recovery_ok(link_status, intel_dp->lane_count)
> > > 
> > > I think drm_dp_channel_eq_ok() checks all the bits: INTERLANE_ALIGN_DONE,
> > > CR_DONE, EQ_DONE, and SYMBOL_LOCKED.
> > 
> > It only checks for EQ_DONE and INTERLANE_ALIGN_DONE.
> 
> It checks DP_CHANNEL_EQ_BITS which is
> 
> #define DP_CHANNEL_EQ_BITS (DP_LANE_CR_DONE |           \
>                             DP_LANE_CHANNEL_EQ_DONE |   \
>                             DP_LANE_SYMBOL_LOCKED)
>

Ok looks good then.

Manasi 
> > drm_dp_clock_recovery_ok() checks for DP_LANE_CR_DONE bit.
> > > 
> 
> -- 
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx