| Message ID | 20170912235730.3492-8-dhinakaran.pandiyan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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? > > > } > > > > DRM_DEBUG_KMS("got esi %3ph\n", esi); > > -- > > 2.11.0 >
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.
On Tue, 2017-09-19 at 15:42 +0300, Ville Syrjälä wrote: > 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. > disable_dp and enable_dp are the ones that use sideband messages during modeset. But, there doesn't seem to be any real dependency on the sideband messages, mostly because there is not much we do differently when a downstream reply does not arrive. Having said that, now that I know you (moving link training over to hotplug work) and Maarten (link training vs modeset) already have code to deal with this properly, this patch should be dropped. Thanks for the review!
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); } DRM_DEBUG_KMS("got esi %3ph\n", esi);
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(+)