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

[Imre Deak <imre.deak@intel.com>]

On Thu, Jun 04, 2020 at 05:55:30PM +0300, Ville Syrjälä wrote:
> On Thu, Jun 04, 2020 at 12:10:38AM +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.
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_dp.c | 33 +++----------------------
> >  1 file changed, 4 insertions(+), 29 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > index 55fda074c0ad..befbcacddaa1 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -5604,7 +5604,7 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
> >  		}
> >  	}
> >  
> > -	return need_retrain;
> > +	return need_retrain ? -EINVAL : 0;
> >  }
> >  
> >  static bool
> > @@ -7255,35 +7255,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) < 0)
> >  			return IRQ_NONE;
> 
> Since we no longer need the tristate return, can you follow up
> with a conversion to bool return? I'd vote to make it match the
> semantics of intel_dp_short_pulse() so we get one step
> closer to unifying the hpd_irq handling across the board.

Ok, makes sense.

> 
> > +	} else if (!intel_dp_short_pulse(intel_dp)) {
> > +		return IRQ_NONE;
> >  	}
> >  
> >  	return IRQ_HANDLED;
> > -- 
> > 2.23.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel