Re: [PATCH 8/9] drm/i915/dp: Protect link training with connection mutex

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Sep 18, 2017 at 09:50:30PM +0000, Pandiyan, Dhinakaran wrote:
> On Fri, 2017-09-15 at 13:10 +0300, Ville Syrjälä wrote:
> > On Tue, Sep 12, 2017 at 04:57:29PM -0700, Dhinakaran Pandiyan wrote:
> > > The other instances of link training are protected with
> > > connection_mutex, so do the same in check_mst_status() too.
> > > 
> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_dp.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > index aab9ba31f79e..644463ba313e 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -4191,6 +4191,7 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
> > >  static int
> > >  intel_dp_check_mst_status(struct intel_dp *intel_dp)
> > >  {
> > > +	struct drm_device *dev = intel_dp_to_dev(intel_dp);
> > >  	bool bret;
> > >  	u8 esi[DP_DPRX_ESI_LEN] = { 0 };
> > >  	int ret = 0;
> > > @@ -4205,8 +4206,11 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
> > >  		if (intel_dp->active_mst_links &&
> > >  		    !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) {
> > >  			DRM_DEBUG_KMS("channel EQ not ok, retraining\n");
> > > +
> > > +			drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> > >  			intel_dp_start_link_train(intel_dp);
> > >  			intel_dp_stop_link_train(intel_dp);
> > > +			drm_modeset_unlock(&dev->mode_config.connection_mutex);
> > 
> > This can deadlock. We should not grab any modeset locks from the
> > dig_work. I had some patches at some point to move the link training to
> > the hotplug work so SST. I don't think I had the MST side really sorted
> > out at any point.
> 
> Interesting, the only lock we grab in this path from the work function
> is the connection_mutex. So, I am not clear how this will deadlock. This
> lock around link training is also at the same depth as the one around
> link training in intel_dp_short_pulse(). Wouldn't that also deadlock if
> that's the case?

Theoretically. Though I think the deadlock is only likely to happen with
MST since that requires sideband to work during a modeset when we have
connection_mutex already locked. So modeset code will be waiting for
sideband to happen, and hpd_pulse which is responsible for doing sideband
is waiting on the lock already held by the modeset. Thus we're stuck.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx