[Intel-gfx] [PATCH RESEND v3 1/3] drm/i915/dp_mst: Fix disabling MST on a port

[Imre Deak]

On Mon, Jun 08, 2020 at 01:11:44AM +0300, Souza, Jose wrote:
> On Fri, 2020-06-05 at 12:48 +0300, Imre Deak wrote:
> > Currently MST on a port can get enabled/disabled from the hotplug work
> > and get disabled from the short pulse work in a racy way. Fix this by
> > relying on the MST state checking in the hotplug work and just schedule
> > a hotplug work from the short pulse handler if some problem happened
> > during the MST interrupt handling.
> > 
> > This removes the explicit MST disabling in case of an AUX failure, but
> > if AUX fails, then probably the detection will also fail during the
> > scheduled hotplug work and it's not guaranteed that we'll see
> > intermittent errors anyway.
> > 
> > While at it also simplify the error checking of the MST interrupt
> > handler.
> > 
> > v2:
> > - Convert intel_dp_check_mst_status() to return bool. (Ville)
> > - Change the intel_dp->is_mst check to an assert, since after this patch
> >   the condition can't change after we checked it previously.
> > - Document the return value from intel_dp_check_mst_status().
> > v3:
> > - Remove the intel_dp->is_mst check from intel_dp_check_mst_status().
> >   There is no point in checking the same condition twice, even though
> >   there is a chance that the hotplug work running concurrently changes
> >   it.
> > 
> > Cc: José Roberto de Souza <jose.souza at intel.com>
> > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > Reviewed-by: José Roberto de Souza <jose.souza at intel.com> (v1)
> > Signed-off-by: Imre Deak <imre.deak at intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_dp.c | 66 ++++++++++---------------
> >  1 file changed, 26 insertions(+), 40 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > index 55fda074c0ad..42589cae766d 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -5556,35 +5556,46 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
> >  			    "Could not write test response to sink\n");
> >  }
> >  
> > -static int
> > +/**
> > + * intel_dp_check_mst_status - service any pending MST interrupts, check link status
> > + * @intel_dp: Intel DP struct
> > + *
> > + * Read any pending MST interrupts, call MST core to handle these and ack the
> > + * interrupts. Check if the main and AUX link state is ok.
> > + *
> > + * Returns:
> > + * - %true if pending interrupts were serviced (or no interrupts were
> > + *   pending) w/o detecting an error condition.
> > + * - %false if an error condition - like AUX failure or a loss of link - is
> > + *   detected, which needs servicing from the hotplug work.
> > + */
> > +static bool
> >  intel_dp_check_mst_status(struct intel_dp *intel_dp)
> >  {
> >  	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > -	bool need_retrain = false;
> > -
> > -	if (!intel_dp->is_mst)
> > -		return -EINVAL;
> > +	bool link_ok = true;
> >  
> >  	drm_WARN_ON_ONCE(&i915->drm, intel_dp->active_mst_links < 0);
> >  
> >  	for (;;) {
> >  		u8 esi[DP_DPRX_ESI_LEN] = {};
> > -		bool bret, handled;
> > +		bool handled;
> >  		int retry;
> >  
> > -		bret = intel_dp_get_sink_irq_esi(intel_dp, esi);
> > -		if (!bret) {
> > +		if (!intel_dp_get_sink_irq_esi(intel_dp, esi)) {
> >  			drm_dbg_kms(&i915->drm,
> >  				    "failed to get ESI - device may have failed\n");
> > -			return -EINVAL;
> > +			link_ok = false;
> > +
> > +			break;
> >  		}
> >  
> >  		/* check link status - esi[10] = 0x200c */
> > -		if (intel_dp->active_mst_links > 0 && !need_retrain &&
> > +		if (intel_dp->active_mst_links > 0 && link_ok &&
> >  		    !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) {
> >  			drm_dbg_kms(&i915->drm,
> >  				    "channel EQ not ok, retraining\n");
> > -			need_retrain = true;
> > +			link_ok = false;
> >  		}
> >  
> >  		drm_dbg_kms(&i915->drm, "got esi %3ph\n", esi);
> > @@ -5604,7 +5615,7 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
> >  		}
> >  	}
> >  
> > -	return need_retrain;
> > +	return link_ok;
> >  }
> >  
> >  static bool
> > @@ -7255,35 +7266,10 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
> >  	}
> >  
> >  	if (intel_dp->is_mst) {
> > -		switch (intel_dp_check_mst_status(intel_dp)) {
> > -		case -EINVAL:
> > -			/*
> > -			 * If we were in MST mode, and device is not
> > -			 * there, get out of MST mode
> > -			 */
> > -			drm_dbg_kms(&i915->drm,
> > -				    "MST device may have disappeared %d vs %d\n",
> > -				    intel_dp->is_mst,
> > -				    intel_dp->mst_mgr.mst_state);
> > -			intel_dp->is_mst = false;
> > -			drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
> > -							intel_dp->is_mst);
> > -
> > -			return IRQ_NONE;
> > -		case 1:
> > -			return IRQ_NONE;
> > -		default:
> > -			break;
> > -		}
> > -	}
> > -
> > -	if (!intel_dp->is_mst) {
> > -		bool handled;
> > -
> > -		handled = intel_dp_short_pulse(intel_dp);
> > -
> > -		if (!handled)
> > +		if (!intel_dp_check_mst_status(intel_dp))
> >  			return IRQ_NONE;
> > +	} else if (!intel_dp_short_pulse(intel_dp)) {
> > +		return IRQ_NONE;
> >  	}
> >  
> 
> Now it don't need the braces but this is minor.

Without the braces it wouldn't be correct.

> 
> Reviewed-by: José Roberto de Souza <jose.souza at intel.com>
> 
> >  	return IRQ_HANDLED;