| Message ID | 1440689810-29585-2-git-send-email-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Aug 27, 2015 at 06:36:48PM +0300, ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Grab the connection_mutex around MSR link retraining to protect it > against a concurrent modeset. We already do the same for SST. > > DP hpd_pulse can still otherwise race against modeset and ->detect(), so > it's not clear what will happen when both want to scribble into eg. > intel_dp->dpcd[] at the same time. But sorting it all out requires way > more thought than I'm willing to expend now. Actually I suppose this might not work out so well after all. I suppose MST depends on short HPDs during modeset due to the sideband stuff. So if we want to grab modeset locks for retraining, I suppose we'd need to move the retraining to happen from .hot_plug() which gets run from the other hotplug work, and so wouldn't interfere with sideband. > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 9e90a2b..1259c9b 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -4391,6 +4391,7 @@ update_status: > static int > intel_dp_check_mst_status(struct intel_dp *intel_dp) > { > + struct drm_device *dev = dp_to_dig_port(intel_dp)->base.base.dev; > bool bret; > > if (intel_dp->is_mst) { > @@ -4402,6 +4403,8 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp) > go_again: > if (bret == true) { > > + drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); > + > /* check link status - esi[10] = 0x200c */ > if (intel_dp->active_mst_links && > !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) { > @@ -4411,6 +4414,8 @@ go_again: > intel_dp_stop_link_train(intel_dp); > } > > + drm_modeset_unlock(&dev->mode_config.connection_mutex); > + > DRM_DEBUG_KMS("got esi %3ph\n", esi); > ret = drm_dp_mst_hpd_irq(&intel_dp->mst_mgr, esi, &handled); > > -- > 2.4.6
Hey, Op 03-09-15 om 14:11 schreef Ville Syrjälä: > On Thu, Aug 27, 2015 at 06:36:48PM +0300, ville.syrjala@linux.intel.com wrote: >> From: Ville Syrjälä <ville.syrjala@linux.intel.com> >> >> Grab the connection_mutex around MSR link retraining to protect it >> against a concurrent modeset. We already do the same for SST. >> >> DP hpd_pulse can still otherwise race against modeset and ->detect(), so >> it's not clear what will happen when both want to scribble into eg. >> intel_dp->dpcd[] at the same time. But sorting it all out requires way >> more thought than I'm willing to expend now. > Actually I suppose this might not work out so well after all. I suppose > MST depends on short HPDs during modeset due to the sideband stuff. > > So if we want to grab modeset locks for retraining, I suppose we'd > need to move the retraining to happen from .hot_plug() which gets run > from the other hotplug work, and so wouldn't interfere with sideband. > I think it would be better to have a per encoder mutex. In the future we may want to run modeset disable/enable async, in which case it may not hold the connection_mutex. If we do decide on a separate mutex then it would also be useful to also think about how to protect intel_mst_*(dis,en)able_dp. :) ~Maarten
On Wed, Sep 16, 2015 at 01:48:49PM +0200, Maarten Lankhorst wrote: > Hey, > > Op 03-09-15 om 14:11 schreef Ville Syrjälä: > > On Thu, Aug 27, 2015 at 06:36:48PM +0300, ville.syrjala@linux.intel.com wrote: > >> From: Ville Syrjälä <ville.syrjala@linux.intel.com> > >> > >> Grab the connection_mutex around MSR link retraining to protect it > >> against a concurrent modeset. We already do the same for SST. > >> > >> DP hpd_pulse can still otherwise race against modeset and ->detect(), so > >> it's not clear what will happen when both want to scribble into eg. > >> intel_dp->dpcd[] at the same time. But sorting it all out requires way > >> more thought than I'm willing to expend now. > > Actually I suppose this might not work out so well after all. I suppose > > MST depends on short HPDs during modeset due to the sideband stuff. > > > > So if we want to grab modeset locks for retraining, I suppose we'd > > need to move the retraining to happen from .hot_plug() which gets run > > from the other hotplug work, and so wouldn't interfere with sideband. > > > I think it would be better to have a per encoder mutex. In the future we may want to run modeset > disable/enable async, in which case it may not hold the connection_mutex. > > If we do decide on a separate mutex then it would also be useful to also think about how to > protect intel_mst_*(dis,en)able_dp. :) We need to sync re-training with any other modeset work. Which means another thing to get right for async modesets. -Daniel
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 9e90a2b..1259c9b 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -4391,6 +4391,7 @@ update_status: static int intel_dp_check_mst_status(struct intel_dp *intel_dp) { + struct drm_device *dev = dp_to_dig_port(intel_dp)->base.base.dev; bool bret; if (intel_dp->is_mst) { @@ -4402,6 +4403,8 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp) go_again: if (bret == true) { + drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); + /* check link status - esi[10] = 0x200c */ if (intel_dp->active_mst_links && !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) { @@ -4411,6 +4414,8 @@ go_again: intel_dp_stop_link_train(intel_dp); } + drm_modeset_unlock(&dev->mode_config.connection_mutex); + DRM_DEBUG_KMS("got esi %3ph\n", esi); ret = drm_dp_mst_hpd_irq(&intel_dp->mst_mgr, esi, &handled);