| Message ID | 1478564850-19766-1-git-send-email-dhinakaran.pandiyan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Nov 07, 2016 at 04:27:30PM -0800, Dhinakaran Pandiyan wrote: > Hotplugging a monitor in DP MST configuration triggers short pulses. > Although the short pulse handling path ends up in the MST hotplug function, > we do not perform a detect before sending the hotplug uvent. This leads to > the connector status not being updated when the userspace calls > DRM_IOCTL_MODE_GETCONNECTOR. > > So, let's call the connector function ->detect() to update the connector > status before sending the uevent. > > v2: Update connector status inside mode_config mutex (Ville) > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > --- > drivers/gpu/drm/i915/intel_dp_mst.c | 22 +++++++++++++++++++++- > 1 file changed, 21 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c > index 3ffbd69..8e36a96 100644 > --- a/drivers/gpu/drm/i915/intel_dp_mst.c > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c > @@ -492,8 +492,28 @@ static void intel_dp_mst_hotplug(struct drm_dp_mst_topology_mgr *mgr) > struct intel_dp *intel_dp = container_of(mgr, struct intel_dp, mst_mgr); > struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > struct drm_device *dev = intel_dig_port->base.base.dev; > + struct intel_connector *intel_connector; > + bool changed = false; > + enum drm_connector_status old_status; > + struct drm_mode_config *mode_config = &dev->mode_config; > + > + mutex_lock(&mode_config->mutex); > + for_each_intel_connector(dev, intel_connector) { > + struct drm_connector *connector = &(intel_connector->base); > + > + if (intel_connector->mst_port != intel_dp) > + continue; > + > + old_status = connector->status; > + connector->status = connector->funcs->detect(connector, false); > + > + if (old_status != connector->status) > + changed = true; > + } > + mutex_unlock(&mode_config->mutex); To help reviewers it might be nice to have a short explanation of the locking situation in the commit message, mainly why is it safe to simply take mode_config.mutex here. > > - drm_kms_helper_hotplug_event(dev); > + if (changed) > + drm_kms_helper_hotplug_event(dev); > } > > static const struct drm_dp_mst_topology_cbs mst_cbs = { > -- > 2.7.4
On Mon, Nov 07, 2016 at 04:27:30PM -0800, Dhinakaran Pandiyan wrote: > Hotplugging a monitor in DP MST configuration triggers short pulses. > Although the short pulse handling path ends up in the MST hotplug function, > we do not perform a detect before sending the hotplug uvent. This leads to > the connector status not being updated when the userspace calls > DRM_IOCTL_MODE_GETCONNECTOR. > > So, let's call the connector function ->detect() to update the connector > status before sending the uevent. > > v2: Update connector status inside mode_config mutex (Ville) > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> Possibly Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98306 > --- > drivers/gpu/drm/i915/intel_dp_mst.c | 22 +++++++++++++++++++++- > 1 file changed, 21 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c > index 3ffbd69..8e36a96 100644 > --- a/drivers/gpu/drm/i915/intel_dp_mst.c > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c > @@ -492,8 +492,28 @@ static void intel_dp_mst_hotplug(struct drm_dp_mst_topology_mgr *mgr) > struct intel_dp *intel_dp = container_of(mgr, struct intel_dp, mst_mgr); > struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > struct drm_device *dev = intel_dig_port->base.base.dev; > + struct intel_connector *intel_connector; > + bool changed = false; > + enum drm_connector_status old_status; > + struct drm_mode_config *mode_config = &dev->mode_config; > + > + mutex_lock(&mode_config->mutex); > + for_each_intel_connector(dev, intel_connector) { > + struct drm_connector *connector = &(intel_connector->base); > + > + if (intel_connector->mst_port != intel_dp) > + continue; > + > + old_status = connector->status; > + connector->status = connector->funcs->detect(connector, false); > + > + if (old_status != connector->status) > + changed = true; > + } > + mutex_unlock(&mode_config->mutex); > > - drm_kms_helper_hotplug_event(dev); > + if (changed) > + drm_kms_helper_hotplug_event(dev); > } > > static const struct drm_dp_mst_topology_cbs mst_cbs = { > -- > 2.7.4
On Tue, 2016-11-08 at 13:04 +0200, Ville Syrjälä wrote: > On Mon, Nov 07, 2016 at 04:27:30PM -0800, Dhinakaran Pandiyan wrote: > > Hotplugging a monitor in DP MST configuration triggers short pulses. > > Although the short pulse handling path ends up in the MST hotplug function, > > we do not perform a detect before sending the hotplug uvent. This leads to > > the connector status not being updated when the userspace calls > > DRM_IOCTL_MODE_GETCONNECTOR. > > > > So, let's call the connector function ->detect() to update the connector > > status before sending the uevent. > > > > v2: Update connector status inside mode_config mutex (Ville) > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > --- > > drivers/gpu/drm/i915/intel_dp_mst.c | 22 +++++++++++++++++++++- > > 1 file changed, 21 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c > > index 3ffbd69..8e36a96 100644 > > --- a/drivers/gpu/drm/i915/intel_dp_mst.c > > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c > > @@ -492,8 +492,28 @@ static void intel_dp_mst_hotplug(struct drm_dp_mst_topology_mgr *mgr) > > struct intel_dp *intel_dp = container_of(mgr, struct intel_dp, mst_mgr); > > struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > > struct drm_device *dev = intel_dig_port->base.base.dev; > > + struct intel_connector *intel_connector; > > + bool changed = false; > > + enum drm_connector_status old_status; > > + struct drm_mode_config *mode_config = &dev->mode_config; > > + > > + mutex_lock(&mode_config->mutex); > > + for_each_intel_connector(dev, intel_connector) { > > + struct drm_connector *connector = &(intel_connector->base); > > + > > + if (intel_connector->mst_port != intel_dp) > > + continue; > > + > > + old_status = connector->status; > > + connector->status = connector->funcs->detect(connector, false); > > + > > + if (old_status != connector->status) > > + changed = true; > > + } > > + mutex_unlock(&mode_config->mutex); > > To help reviewers it might be nice to have a short explanation of the > locking situation in the commit message, mainly why is it safe to simply > take mode_config.mutex here. > I can't convince myself this locking is correct yet. drm_dp_send_link_address(), one of the callers of this function is recursive. drm_dp_send_link_address()->drm_dp_add_port()->drm_dp_send_link_address() I am wondering if hpd_pulse should return IRQ_NONE so that we can have ->detect() called from the hotplug work function. For this to work, we'll need to change drm_dp_mst_hpd_irq(). -DK > > > > - drm_kms_helper_hotplug_event(dev); > > + if (changed) > > + drm_kms_helper_hotplug_event(dev); > > } > > > > static const struct drm_dp_mst_topology_cbs mst_cbs = { > > -- > > 2.7.4 >
On Fri, Nov 11, 2016 at 08:43:53PM +0000, Pandiyan, Dhinakaran wrote: > On Tue, 2016-11-08 at 13:04 +0200, Ville Syrjälä wrote: > > On Mon, Nov 07, 2016 at 04:27:30PM -0800, Dhinakaran Pandiyan wrote: > > > Hotplugging a monitor in DP MST configuration triggers short pulses. > > > Although the short pulse handling path ends up in the MST hotplug function, > > > we do not perform a detect before sending the hotplug uvent. This leads to > > > the connector status not being updated when the userspace calls > > > DRM_IOCTL_MODE_GETCONNECTOR. > > > > > > So, let's call the connector function ->detect() to update the connector > > > status before sending the uevent. > > > > > > v2: Update connector status inside mode_config mutex (Ville) > > > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_dp_mst.c | 22 +++++++++++++++++++++- > > > 1 file changed, 21 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c > > > index 3ffbd69..8e36a96 100644 > > > --- a/drivers/gpu/drm/i915/intel_dp_mst.c > > > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c > > > @@ -492,8 +492,28 @@ static void intel_dp_mst_hotplug(struct drm_dp_mst_topology_mgr *mgr) > > > struct intel_dp *intel_dp = container_of(mgr, struct intel_dp, mst_mgr); > > > struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > > > struct drm_device *dev = intel_dig_port->base.base.dev; > > > + struct intel_connector *intel_connector; > > > + bool changed = false; > > > + enum drm_connector_status old_status; > > > + struct drm_mode_config *mode_config = &dev->mode_config; > > > + > > > + mutex_lock(&mode_config->mutex); > > > + for_each_intel_connector(dev, intel_connector) { > > > + struct drm_connector *connector = &(intel_connector->base); > > > + > > > + if (intel_connector->mst_port != intel_dp) > > > + continue; > > > + > > > + old_status = connector->status; > > > + connector->status = connector->funcs->detect(connector, false); > > > + > > > + if (old_status != connector->status) > > > + changed = true; > > > + } > > > + mutex_unlock(&mode_config->mutex); > > > > To help reviewers it might be nice to have a short explanation of the > > locking situation in the commit message, mainly why is it safe to simply > > take mode_config.mutex here. > > > > I can't convince myself this locking is correct yet. > drm_dp_send_link_address(), one of the callers of this function is > recursive. > drm_dp_send_link_address()->drm_dp_add_port()->drm_dp_send_link_address() Hmm. I wonder how deep that can get. As in is there an upper bound on the stack usage... > > I am wondering if hpd_pulse should return IRQ_NONE so that we can have > ->detect() called from the hotplug work function. For this to work, > we'll need to change drm_dp_mst_hpd_irq(). Does the topology manager do all the processing in a blocking fashion from the dig_port_work, or does is schedule additional works and return before they're done? If it all happens from the dig_port_work, then I guess it should be possible to just set some flag from the mst .hotplug() hook and check that flag after all the processing is done to know whether the schedule the a full detect work. I don't think you can just return IRQ_NONE from the short pulse since that would schedule the full hotplug on the main DP connector, not the MST one. In fact our current hotplug code entirely skips MST connectors. But you could have a separate work perhaps just to deal with the MST connectors, or we'd need to redo the main hotplug handling in some way to handle MST connectors as well.
On Mon, Nov 07, 2016 at 04:27:30PM -0800, Dhinakaran Pandiyan wrote: > Hotplugging a monitor in DP MST configuration triggers short pulses. > Although the short pulse handling path ends up in the MST hotplug function, > we do not perform a detect before sending the hotplug uvent. This leads to > the connector status not being updated when the userspace calls > DRM_IOCTL_MODE_GETCONNECTOR. > > So, let's call the connector function ->detect() to update the connector > status before sending the uevent. > > v2: Update connector status inside mode_config mutex (Ville) > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> quick comments from me on irc: <danvet> dhnkrn, really tricky I think <danvet> dhnkrn, also feels wrong from an entire design pov <danvet> I'd expect the topology manager could tell us from which exact downstream port the short pulse came <danvet> which means we should be able to get at that connector without taking the global lock <danvet> i.e. at least avoid the for_each_connector <danvet> on the detect itself, I need to think <danvet> but we might need to essentially postpone the ->detect work to the hotplug worker <danvet> except that it's not a direct hpd from our chip <danvet> but a downstream one <danvet> this is going to be fun <dhnkrn> Hmm .. Does updating the connector status need the lock or is it the detect() ? <danvet> yup <danvet> and we might need to do additional dpcd transactions (e.g. for load detect probing on downstream ports) <danvet> or at least edid reads <danvet> both recurse back into dp mst helper code like ville said <danvet> dhnkrn, so instead of doing the detect work from the hotplug code <danvet> put them on some list <danvet> need to be careful in case they are already there <danvet> list only protected with dedicated lock <danvet> then launch the hpd worker <danvet> and walk that list from there, with the mode_config.mutex to do the full-on probe <danvet> we can't call ->detect from dig_port_work, and walking the entire connector list in there is also not a good idea imo <danvet> dhnkrn, but that's just my preliminary analysis from like 10 minutes of thinking and checking what ville raised <danvet> needs more thought probably <danvet> dhnkrn, to clarify your question: both connector->status and calling ->detect need mode_config.mutex <danvet> that mutex is what protects output probe state<Vtec234> ccr: i might have udev configured incorrectly then Cheers, Daniel > --- > drivers/gpu/drm/i915/intel_dp_mst.c | 22 +++++++++++++++++++++- > 1 file changed, 21 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c > index 3ffbd69..8e36a96 100644 > --- a/drivers/gpu/drm/i915/intel_dp_mst.c > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c > @@ -492,8 +492,28 @@ static void intel_dp_mst_hotplug(struct drm_dp_mst_topology_mgr *mgr) > struct intel_dp *intel_dp = container_of(mgr, struct intel_dp, mst_mgr); > struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > struct drm_device *dev = intel_dig_port->base.base.dev; > + struct intel_connector *intel_connector; > + bool changed = false; > + enum drm_connector_status old_status; > + struct drm_mode_config *mode_config = &dev->mode_config; > + > + mutex_lock(&mode_config->mutex); > + for_each_intel_connector(dev, intel_connector) { > + struct drm_connector *connector = &(intel_connector->base); > + > + if (intel_connector->mst_port != intel_dp) > + continue; > + > + old_status = connector->status; > + connector->status = connector->funcs->detect(connector, false); > + > + if (old_status != connector->status) > + changed = true; > + } > + mutex_unlock(&mode_config->mutex); > > - drm_kms_helper_hotplug_event(dev); > + if (changed) > + drm_kms_helper_hotplug_event(dev); > } > > static const struct drm_dp_mst_topology_cbs mst_cbs = { > -- > 2.7.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Nov 11, 2016 at 10:21:39PM +0100, Daniel Vetter wrote: > On Mon, Nov 07, 2016 at 04:27:30PM -0800, Dhinakaran Pandiyan wrote: > > Hotplugging a monitor in DP MST configuration triggers short pulses. > > Although the short pulse handling path ends up in the MST hotplug function, > > we do not perform a detect before sending the hotplug uvent. This leads to > > the connector status not being updated when the userspace calls > > DRM_IOCTL_MODE_GETCONNECTOR. > > > > So, let's call the connector function ->detect() to update the connector > > status before sending the uevent. > > > > v2: Update connector status inside mode_config mutex (Ville) > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > quick comments from me on irc: > > <danvet> dhnkrn, really tricky I think > <danvet> dhnkrn, also feels wrong from an entire design pov > <danvet> I'd expect the topology manager could tell us from which exact downstream port the short pulse came > <danvet> which means we should be able to get at that connector without taking the global lock > <danvet> i.e. at least avoid the for_each_connector > <danvet> on the detect itself, I need to think > <danvet> but we might need to essentially postpone the ->detect work to the hotplug worker > <danvet> except that it's not a direct hpd from our chip > <danvet> but a downstream one > <danvet> this is going to be fun > <dhnkrn> Hmm .. Does updating the connector status need the lock or is it the detect() ? > <danvet> yup > <danvet> and we might need to do additional dpcd transactions (e.g. for load detect probing on downstream ports) > <danvet> or at least edid reads > <danvet> both recurse back into dp mst helper code like ville said > <danvet> dhnkrn, so instead of doing the detect work from the hotplug code > <danvet> put them on some list > <danvet> need to be careful in case they are already there > <danvet> list only protected with dedicated lock > <danvet> then launch the hpd worker > <danvet> and walk that list from there, with the mode_config.mutex to do the full-on probe > <danvet> we can't call ->detect from dig_port_work, and walking the entire connector list in there is also not a good idea imo > <danvet> dhnkrn, but that's just my preliminary analysis from like 10 minutes of thinking and checking what ville raised > <danvet> needs more thought probably > <danvet> dhnkrn, to clarify your question: both connector->status and calling ->detect need mode_config.mutex > <danvet> that mutex is what protects output probe state<Vtec234> ccr: i might have udev configured incorrectly then > > Cheers, Daniel > > --- > > drivers/gpu/drm/i915/intel_dp_mst.c | 22 +++++++++++++++++++++- > > 1 file changed, 21 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c > > index 3ffbd69..8e36a96 100644 > > --- a/drivers/gpu/drm/i915/intel_dp_mst.c > > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c > > @@ -492,8 +492,28 @@ static void intel_dp_mst_hotplug(struct drm_dp_mst_topology_mgr *mgr) > > struct intel_dp *intel_dp = container_of(mgr, struct intel_dp, mst_mgr); > > struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > > struct drm_device *dev = intel_dig_port->base.base.dev; > > + struct intel_connector *intel_connector; > > + bool changed = false; > > + enum drm_connector_status old_status; > > + struct drm_mode_config *mode_config = &dev->mode_config; > > + > > + mutex_lock(&mode_config->mutex); > > + for_each_intel_connector(dev, intel_connector) { > > + struct drm_connector *connector = &(intel_connector->base); > > + > > + if (intel_connector->mst_port != intel_dp) > > + continue; > > + > > + old_status = connector->status; > > + connector->status = connector->funcs->detect(connector, false); > > + > > + if (old_status != connector->status) > > + changed = true; > > + } > > + mutex_unlock(&mode_config->mutex); > > > > - drm_kms_helper_hotplug_event(dev); I just realized that this call here will call down into the fbdev helper, which already does the above ->detect calls. At least if there's no compositor running. So I wonder why the deadlock ville pointed out isn't blowing up already ... -Daniel > > + if (changed) > > + drm_kms_helper_hotplug_event(dev); > > } > > > > static const struct drm_dp_mst_topology_cbs mst_cbs = { > > -- > > 2.7.4 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
On Fri, 2016-11-11 at 23:05 +0200, Ville Syrjälä wrote: > On Fri, Nov 11, 2016 at 08:43:53PM +0000, Pandiyan, Dhinakaran wrote: > > On Tue, 2016-11-08 at 13:04 +0200, Ville Syrjälä wrote: > > > On Mon, Nov 07, 2016 at 04:27:30PM -0800, Dhinakaran Pandiyan wrote: > > > > Hotplugging a monitor in DP MST configuration triggers short pulses. > > > > Although the short pulse handling path ends up in the MST hotplug function, > > > > we do not perform a detect before sending the hotplug uvent. This leads to > > > > the connector status not being updated when the userspace calls > > > > DRM_IOCTL_MODE_GETCONNECTOR. > > > > > > > > So, let's call the connector function ->detect() to update the connector > > > > status before sending the uevent. > > > > > > > > v2: Update connector status inside mode_config mutex (Ville) > > > > > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/intel_dp_mst.c | 22 +++++++++++++++++++++- > > > > 1 file changed, 21 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c > > > > index 3ffbd69..8e36a96 100644 > > > > --- a/drivers/gpu/drm/i915/intel_dp_mst.c > > > > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c > > > > @@ -492,8 +492,28 @@ static void intel_dp_mst_hotplug(struct drm_dp_mst_topology_mgr *mgr) > > > > struct intel_dp *intel_dp = container_of(mgr, struct intel_dp, mst_mgr); > > > > struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > > > > struct drm_device *dev = intel_dig_port->base.base.dev; > > > > + struct intel_connector *intel_connector; > > > > + bool changed = false; > > > > + enum drm_connector_status old_status; > > > > + struct drm_mode_config *mode_config = &dev->mode_config; > > > > + > > > > + mutex_lock(&mode_config->mutex); > > > > + for_each_intel_connector(dev, intel_connector) { > > > > + struct drm_connector *connector = &(intel_connector->base); > > > > + > > > > + if (intel_connector->mst_port != intel_dp) > > > > + continue; > > > > + > > > > + old_status = connector->status; > > > > + connector->status = connector->funcs->detect(connector, false); > > > > + > > > > + if (old_status != connector->status) > > > > + changed = true; > > > > + } > > > > + mutex_unlock(&mode_config->mutex); > > > > > > To help reviewers it might be nice to have a short explanation of the > > > locking situation in the commit message, mainly why is it safe to simply > > > take mode_config.mutex here. > > > > > > > I can't convince myself this locking is correct yet. > > drm_dp_send_link_address(), one of the callers of this function is > > recursive. > > drm_dp_send_link_address()->drm_dp_add_port()->drm_dp_send_link_address() > > Hmm. I wonder how deep that can get. As in is there an upper bound on > the stack usage... > The upper bound is going to be the number of downstream ports for the branch device that was hotplugged. I went through the code again, the recursive call that I mentioned above is done before hotplug() is called. It does look like we are safe there. But, what is going to be a problem is, as Daniel pointed out, intel_dp_mst_hotplug()->drm_kms_helper_hotplug_event() ->output_poll_changed(dev) ->drm_fb_helper_hotplug_event() This sequence leads to an recursive call of the mode_config.mutex with fbdev_emulation enabled. > > > > I am wondering if hpd_pulse should return IRQ_NONE so that we can have > > ->detect() called from the hotplug work function. For this to work, > > we'll need to change drm_dp_mst_hpd_irq(). > > Does the topology manager do all the processing in a blocking fashion > from the dig_port_work, or does is schedule additional works and return > before they're done? drm_dp_mst_link_probe_work() gets scheduled inside drm_dp_update_port() when a device was plugged in. -DK > > If it all happens from the dig_port_work, then I guess it should be > possible to just set some flag from the mst .hotplug() hook and check > that flag after all the processing is done to know whether the schedule > the a full detect work. I don't think you can just return IRQ_NONE from > the short pulse since that would schedule the full hotplug on the main > DP connector, not the MST one. In fact our current hotplug code entirely > skips MST connectors. But you could have a separate work perhaps just > to deal with the MST connectors, or we'd need to redo the main hotplug > handling in some way to handle MST connectors as well. >
On Sun, 2016-11-13 at 11:39 +0100, Daniel Vetter wrote: > On Fri, Nov 11, 2016 at 10:21:39PM +0100, Daniel Vetter wrote: > > On Mon, Nov 07, 2016 at 04:27:30PM -0800, Dhinakaran Pandiyan wrote: > > > Hotplugging a monitor in DP MST configuration triggers short pulses. > > > Although the short pulse handling path ends up in the MST hotplug function, > > > we do not perform a detect before sending the hotplug uvent. This leads to > > > the connector status not being updated when the userspace calls > > > DRM_IOCTL_MODE_GETCONNECTOR. > > > > > > So, let's call the connector function ->detect() to update the connector > > > status before sending the uevent. > > > > > > v2: Update connector status inside mode_config mutex (Ville) > > > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > > quick comments from me on irc: > > > > <danvet> dhnkrn, really tricky I think > > <danvet> dhnkrn, also feels wrong from an entire design pov > > <danvet> I'd expect the topology manager could tell us from which exact downstream port the short pulse came > > <danvet> which means we should be able to get at that connector without taking the global lock > > <danvet> i.e. at least avoid the for_each_connector > > <danvet> on the detect itself, I need to think > > <danvet> but we might need to essentially postpone the ->detect work to the hotplug worker > > <danvet> except that it's not a direct hpd from our chip > > <danvet> but a downstream one > > <danvet> this is going to be fun > > <dhnkrn> Hmm .. Does updating the connector status need the lock or is it the detect() ? > > <danvet> yup > > <danvet> and we might need to do additional dpcd transactions (e.g. for load detect probing on downstream ports) > > <danvet> or at least edid reads > > <danvet> both recurse back into dp mst helper code like ville said > > <danvet> dhnkrn, so instead of doing the detect work from the hotplug code > > <danvet> put them on some list > > <danvet> need to be careful in case they are already there > > <danvet> list only protected with dedicated lock > > <danvet> then launch the hpd worker > > <danvet> and walk that list from there, with the mode_config.mutex to do the full-on probe > > <danvet> we can't call ->detect from dig_port_work, and walking the entire connector list in there is also not a good idea imo > > <danvet> dhnkrn, but that's just my preliminary analysis from like 10 minutes of thinking and checking what ville raised > > <danvet> needs more thought probably > > <danvet> dhnkrn, to clarify your question: both connector->status and calling ->detect need mode_config.mutex > > <danvet> that mutex is what protects output probe state<Vtec234> ccr: i might have udev configured incorrectly then > > > > Cheers, Daniel > > > --- > > > drivers/gpu/drm/i915/intel_dp_mst.c | 22 +++++++++++++++++++++- > > > 1 file changed, 21 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c > > > index 3ffbd69..8e36a96 100644 > > > --- a/drivers/gpu/drm/i915/intel_dp_mst.c > > > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c > > > @@ -492,8 +492,28 @@ static void intel_dp_mst_hotplug(struct drm_dp_mst_topology_mgr *mgr) > > > struct intel_dp *intel_dp = container_of(mgr, struct intel_dp, mst_mgr); > > > struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > > > struct drm_device *dev = intel_dig_port->base.base.dev; > > > + struct intel_connector *intel_connector; > > > + bool changed = false; > > > + enum drm_connector_status old_status; > > > + struct drm_mode_config *mode_config = &dev->mode_config; > > > + > > > + mutex_lock(&mode_config->mutex); > > > + for_each_intel_connector(dev, intel_connector) { > > > + struct drm_connector *connector = &(intel_connector->base); > > > + > > > + if (intel_connector->mst_port != intel_dp) > > > + continue; > > > + > > > + old_status = connector->status; > > > + connector->status = connector->funcs->detect(connector, false); > > > + > > > + if (old_status != connector->status) > > > + changed = true; > > > + } > > > + mutex_unlock(&mode_config->mutex); > > > > > > - drm_kms_helper_hotplug_event(dev); > > I just realized that this call here will call down into the fbdev helper, > which already does the above ->detect calls. At least if there's no > compositor running. So I wonder why the deadlock ville pointed out isn't > blowing up already ... > -Daniel > You are right, that call sequence does end up taking the mode_config.mutex lock recursively. Are you referring to CI results for this patch? -DK > > > + if (changed) > > > + drm_kms_helper_hotplug_event(dev); > > > } > > > > > > static const struct drm_dp_mst_topology_cbs mst_cbs = { > > > -- > > > 2.7.4 > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch >
On Thu, Nov 17, 2016 at 01:53:31AM +0000, Pandiyan, Dhinakaran wrote: > On Sun, 2016-11-13 at 11:39 +0100, Daniel Vetter wrote: > > On Fri, Nov 11, 2016 at 10:21:39PM +0100, Daniel Vetter wrote: > > > On Mon, Nov 07, 2016 at 04:27:30PM -0800, Dhinakaran Pandiyan wrote: > > > > Hotplugging a monitor in DP MST configuration triggers short pulses. > > > > Although the short pulse handling path ends up in the MST hotplug function, > > > > we do not perform a detect before sending the hotplug uvent. This leads to > > > > the connector status not being updated when the userspace calls > > > > DRM_IOCTL_MODE_GETCONNECTOR. > > > > > > > > So, let's call the connector function ->detect() to update the connector > > > > status before sending the uevent. > > > > > > > > v2: Update connector status inside mode_config mutex (Ville) > > > > > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > > > > quick comments from me on irc: > > > > > > <danvet> dhnkrn, really tricky I think > > > <danvet> dhnkrn, also feels wrong from an entire design pov > > > <danvet> I'd expect the topology manager could tell us from which exact downstream port the short pulse came > > > <danvet> which means we should be able to get at that connector without taking the global lock > > > <danvet> i.e. at least avoid the for_each_connector > > > <danvet> on the detect itself, I need to think > > > <danvet> but we might need to essentially postpone the ->detect work to the hotplug worker > > > <danvet> except that it's not a direct hpd from our chip > > > <danvet> but a downstream one > > > <danvet> this is going to be fun > > > <dhnkrn> Hmm .. Does updating the connector status need the lock or is it the detect() ? > > > <danvet> yup > > > <danvet> and we might need to do additional dpcd transactions (e.g. for load detect probing on downstream ports) > > > <danvet> or at least edid reads > > > <danvet> both recurse back into dp mst helper code like ville said > > > <danvet> dhnkrn, so instead of doing the detect work from the hotplug code > > > <danvet> put them on some list > > > <danvet> need to be careful in case they are already there > > > <danvet> list only protected with dedicated lock > > > <danvet> then launch the hpd worker > > > <danvet> and walk that list from there, with the mode_config.mutex to do the full-on probe > > > <danvet> we can't call ->detect from dig_port_work, and walking the entire connector list in there is also not a good idea imo > > > <danvet> dhnkrn, but that's just my preliminary analysis from like 10 minutes of thinking and checking what ville raised > > > <danvet> needs more thought probably > > > <danvet> dhnkrn, to clarify your question: both connector->status and calling ->detect need mode_config.mutex > > > <danvet> that mutex is what protects output probe state<Vtec234> ccr: i might have udev configured incorrectly then > > > > > > Cheers, Daniel > > > > --- > > > > drivers/gpu/drm/i915/intel_dp_mst.c | 22 +++++++++++++++++++++- > > > > 1 file changed, 21 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c > > > > index 3ffbd69..8e36a96 100644 > > > > --- a/drivers/gpu/drm/i915/intel_dp_mst.c > > > > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c > > > > @@ -492,8 +492,28 @@ static void intel_dp_mst_hotplug(struct drm_dp_mst_topology_mgr *mgr) > > > > struct intel_dp *intel_dp = container_of(mgr, struct intel_dp, mst_mgr); > > > > struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > > > > struct drm_device *dev = intel_dig_port->base.base.dev; > > > > + struct intel_connector *intel_connector; > > > > + bool changed = false; > > > > + enum drm_connector_status old_status; > > > > + struct drm_mode_config *mode_config = &dev->mode_config; > > > > + > > > > + mutex_lock(&mode_config->mutex); > > > > + for_each_intel_connector(dev, intel_connector) { > > > > + struct drm_connector *connector = &(intel_connector->base); > > > > + > > > > + if (intel_connector->mst_port != intel_dp) > > > > + continue; > > > > + > > > > + old_status = connector->status; > > > > + connector->status = connector->funcs->detect(connector, false); > > > > + > > > > + if (old_status != connector->status) > > > > + changed = true; > > > > + } > > > > + mutex_unlock(&mode_config->mutex); > > > > > > > > - drm_kms_helper_hotplug_event(dev); > > > > I just realized that this call here will call down into the fbdev helper, > > which already does the above ->detect calls. At least if there's no > > compositor running. So I wonder why the deadlock ville pointed out isn't > > blowing up already ... > > -Daniel > > > > You are right, that call sequence does end up taking the > mode_config.mutex lock recursively. Are you referring to CI results for > this patch? Well I'm confused, since per our discussion here it should blow up, but it doesn't. What are we missing? CI can't be trusted on this since we can't (yet) do MST hotplug cycles, hence it doesn't exercise this codepath. -Daniel
On Thu, 2016-11-17 at 08:53 +0100, Daniel Vetter wrote: > On Thu, Nov 17, 2016 at 01:53:31AM +0000, Pandiyan, Dhinakaran wrote: > > On Sun, 2016-11-13 at 11:39 +0100, Daniel Vetter wrote: > > > On Fri, Nov 11, 2016 at 10:21:39PM +0100, Daniel Vetter wrote: > > > > On Mon, Nov 07, 2016 at 04:27:30PM -0800, Dhinakaran Pandiyan wrote: > > > > > Hotplugging a monitor in DP MST configuration triggers short pulses. > > > > > Although the short pulse handling path ends up in the MST hotplug function, > > > > > we do not perform a detect before sending the hotplug uvent. This leads to > > > > > the connector status not being updated when the userspace calls > > > > > DRM_IOCTL_MODE_GETCONNECTOR. > > > > > > > > > > So, let's call the connector function ->detect() to update the connector > > > > > status before sending the uevent. > > > > > > > > > > v2: Update connector status inside mode_config mutex (Ville) > > > > > > > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > > > > > > quick comments from me on irc: > > > > > > > > <danvet> dhnkrn, really tricky I think > > > > <danvet> dhnkrn, also feels wrong from an entire design pov > > > > <danvet> I'd expect the topology manager could tell us from which exact downstream port the short pulse came > > > > <danvet> which means we should be able to get at that connector without taking the global lock > > > > <danvet> i.e. at least avoid the for_each_connector > > > > <danvet> on the detect itself, I need to think > > > > <danvet> but we might need to essentially postpone the ->detect work to the hotplug worker > > > > <danvet> except that it's not a direct hpd from our chip > > > > <danvet> but a downstream one > > > > <danvet> this is going to be fun > > > > <dhnkrn> Hmm .. Does updating the connector status need the lock or is it the detect() ? > > > > <danvet> yup > > > > <danvet> and we might need to do additional dpcd transactions (e.g. for load detect probing on downstream ports) > > > > <danvet> or at least edid reads > > > > <danvet> both recurse back into dp mst helper code like ville said > > > > <danvet> dhnkrn, so instead of doing the detect work from the hotplug code > > > > <danvet> put them on some list > > > > <danvet> need to be careful in case they are already there > > > > <danvet> list only protected with dedicated lock > > > > <danvet> then launch the hpd worker > > > > <danvet> and walk that list from there, with the mode_config.mutex to do the full-on probe > > > > <danvet> we can't call ->detect from dig_port_work, and walking the entire connector list in there is also not a good idea imo > > > > <danvet> dhnkrn, but that's just my preliminary analysis from like 10 minutes of thinking and checking what ville raised > > > > <danvet> needs more thought probably > > > > <danvet> dhnkrn, to clarify your question: both connector->status and calling ->detect need mode_config.mutex > > > > <danvet> that mutex is what protects output probe state<Vtec234> ccr: i might have udev configured incorrectly then > > > > > > > > Cheers, Daniel > > > > > --- > > > > > drivers/gpu/drm/i915/intel_dp_mst.c | 22 +++++++++++++++++++++- > > > > > 1 file changed, 21 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c > > > > > index 3ffbd69..8e36a96 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_dp_mst.c > > > > > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c > > > > > @@ -492,8 +492,28 @@ static void intel_dp_mst_hotplug(struct drm_dp_mst_topology_mgr *mgr) > > > > > struct intel_dp *intel_dp = container_of(mgr, struct intel_dp, mst_mgr); > > > > > struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > > > > > struct drm_device *dev = intel_dig_port->base.base.dev; > > > > > + struct intel_connector *intel_connector; > > > > > + bool changed = false; > > > > > + enum drm_connector_status old_status; > > > > > + struct drm_mode_config *mode_config = &dev->mode_config; > > > > > + > > > > > + mutex_lock(&mode_config->mutex); > > > > > + for_each_intel_connector(dev, intel_connector) { > > > > > + struct drm_connector *connector = &(intel_connector->base); > > > > > + > > > > > + if (intel_connector->mst_port != intel_dp) > > > > > + continue; > > > > > + > > > > > + old_status = connector->status; > > > > > + connector->status = connector->funcs->detect(connector, false); > > > > > + > > > > > + if (old_status != connector->status) > > > > > + changed = true; > > > > > + } > > > > > + mutex_unlock(&mode_config->mutex); > > > > > > > > > > - drm_kms_helper_hotplug_event(dev); > > > > > > I just realized that this call here will call down into the fbdev helper, > > > which already does the above ->detect calls. At least if there's no > > > compositor running. So I wonder why the deadlock ville pointed out isn't > > > blowing up already ... > > > -Daniel > > > > > > > You are right, that call sequence does end up taking the > > mode_config.mutex lock recursively. Are you referring to CI results for > > this patch? > > Well I'm confused, since per our discussion here it should blow up, but it > doesn't. What are we missing? CI can't be trusted on this since we can't > (yet) do MST hotplug cycles, hence it doesn't exercise this codepath. > -Daniel The mutex is unlocked after the ->detect()'s that I added are completed. The hotplug event is sent only after that. Or am I missing something here?
diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c index 3ffbd69..8e36a96 100644 --- a/drivers/gpu/drm/i915/intel_dp_mst.c +++ b/drivers/gpu/drm/i915/intel_dp_mst.c @@ -492,8 +492,28 @@ static void intel_dp_mst_hotplug(struct drm_dp_mst_topology_mgr *mgr) struct intel_dp *intel_dp = container_of(mgr, struct intel_dp, mst_mgr); struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); struct drm_device *dev = intel_dig_port->base.base.dev; + struct intel_connector *intel_connector; + bool changed = false; + enum drm_connector_status old_status; + struct drm_mode_config *mode_config = &dev->mode_config; + + mutex_lock(&mode_config->mutex); + for_each_intel_connector(dev, intel_connector) { + struct drm_connector *connector = &(intel_connector->base); + + if (intel_connector->mst_port != intel_dp) + continue; + + old_status = connector->status; + connector->status = connector->funcs->detect(connector, false); + + if (old_status != connector->status) + changed = true; + } + mutex_unlock(&mode_config->mutex); - drm_kms_helper_hotplug_event(dev); + if (changed) + drm_kms_helper_hotplug_event(dev); } static const struct drm_dp_mst_topology_cbs mst_cbs = {
Hotplugging a monitor in DP MST configuration triggers short pulses. Although the short pulse handling path ends up in the MST hotplug function, we do not perform a detect before sending the hotplug uvent. This leads to the connector status not being updated when the userspace calls DRM_IOCTL_MODE_GETCONNECTOR. So, let's call the connector function ->detect() to update the connector status before sending the uevent. v2: Update connector status inside mode_config mutex (Ville) Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> --- drivers/gpu/drm/i915/intel_dp_mst.c | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-)