On Thu, Nov 29, 2018 at 8:37 AM Lisovskiy, Stanislav <stanislav.lisovskiy@xxxxxxxxx> wrote: > > On Wed, 2018-11-28 at 22:21 +0100, Daniel Vetter wrote: > > On Wed, Nov 28, 2018 at 09:51:13PM +0100, Daniel Vetter wrote: > > > On Wed, Nov 28, 2018 at 03:55:58PM +0200, Stanislav Lisovskiy > > > wrote: > > > > Currently kernel might allocate different connector ids > > > > for the same outputs in case of DP MST, which seems to > > > > confuse userspace. There are can be different connector > > > > ids in the list, which could be assigned to the same > > > > output, while being in different states. > > > > This results in issues, like external displays staying > > > > blank after quick unplugging and plugging back(bug #106250). > > > > Returning only active DP connectors fixes the issue. > > > > > > > > v2: Removed caps from the title > > > > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106250 > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@xxxxxxxxx > > > > > > > > > --- > > > > drivers/gpu/drm/drm_mode_config.c | 16 +++++++++++----- > > > > 1 file changed, 11 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/drm_mode_config.c > > > > b/drivers/gpu/drm/drm_mode_config.c > > > > index ee80788f2c40..ec5b2b08a45e 100644 > > > > --- a/drivers/gpu/drm/drm_mode_config.c > > > > +++ b/drivers/gpu/drm/drm_mode_config.c > > > > @@ -143,6 +143,7 @@ int drm_mode_getresources(struct drm_device > > > > *dev, void *data, > > > > drm_connector_list_iter_begin(dev, &conn_iter); > > > > count = 0; > > > > connector_id = u64_to_user_ptr(card_res- > > > > >connector_id_ptr); > > > > + DRM_DEBUG_KMS("GetResources: writing connectors start"); > > > > drm_for_each_connector_iter(connector, &conn_iter) { > > > > /* only expose writeback connectors if userspace > > > > understands them */ > > > > if (!file_priv->writeback_connectors && > > > > @@ -150,15 +151,20 @@ int drm_mode_getresources(struct drm_device > > > > *dev, void *data, > > > > continue; > > > > > > > > if (drm_lease_held(file_priv, connector- > > > > >base.id)) { > > > > - if (count < card_res->count_connectors > > > > && > > > > - put_user(connector->base.id, > > > > connector_id + count)) { > > > > - drm_connector_list_iter_end(&con > > > > n_iter); > > > > - return -EFAULT; > > > > + if (connector->connector_type != > > > > DRM_MODE_CONNECTOR_DisplayPort || > > > > + connector->status != > > > > connector_status_disconnected) { > > > > + if (count < card_res- > > > > >count_connectors && > > > > + put_user(connector->base.id, > > > > connector_id + count)) { > > > > + drm_connector_list_iter_ > > > > end(&conn_iter); > > > > + return -EFAULT; > > > > + } > > > > + DRM_DEBUG_KMS("GetResources: > > > > connector %s", connector->name); > > > > + count++; > > > > > > I tried to read the bug and I have no idea what's going on here. > > > Userspace > > > is supposed to shut off outputs that are disconnected, whether > > > that's DP, > > > DP MST or something else shouldn't matter. New connectors can > > > come&go as > > > they see fit. Also not really something special. > > > > > > Why do we need to dynamically hide an output here? Note that this > > > also > > > affects normal DP ports, which I have no idea is actually what you > > > want to > > > do or not. > > This bug is real and easily reproducible with recent drm-tip. > Unplugging and then quickly plugging back periodically leaves both my > external displays connected to the docking station blank, there are > also many duplicate bugs for this, which I simply didn't track. This > patch at least fixes that annoying thing. > The userspace seems to get confused when we are returning two different > connector ids, one in disconnected state and another in connected state > for the same output. This results in userspace believing that nothing > had changed and drm_mode_setcrtc call is not required( I have done > traces confirming that theory). > > This could be also fixed in userspace by checking connectors more > carefully - that fix I've also implemented for Intel DDX and attached > to the bug, however seems that this happens also for Wayland. I didn't check wayland (well, weston or any of the others, wayland doesn't exist as an implementation), but -modesetting and -amdgpu. And they all have the same issue. It's roughly: - You unplug - Kernel sends out uevent - Userspace is slow with shutting down that output - You replug - Kernel creates a new connector (it's always a new connector from the kernel's pov for MST, which might or might not happen to have the same id as a previous one). - Userspace sees the hotplug, and uses the PATH property to check whether it has seen this mst connector already. This is needed on X since you can't unplug connectors in xrandr ever, so reusing old ones is a good idea. - Userspace is confused. Your fix essentially goes back to really old MST world where the kernel yanked the MST output right away, without waiting for userspace. That's kinda not nice. Except it's only hidden, not even shut off, so userspace has a hard time shutting it down. It also has the fundamental problem that if you yank&replug fast enough (fast enough that userspace can't see the in-between unplugged state), then nothing happens, and userspace is again confused. Aside: You want Chris patch to implement your idea. There's a few ways to fix this in proper fashion I think: - We wait on the kernel side with registering the new connector until userspace has caught up. Would be a bit a hack, and I haven't figured out a good way yet where to put that hack. Probably the least hacky way would be: 1. In getresources we scan all the previous connector's PATH property, if it matches we don't (yet) expose that connector. This should only expose the older connector. 2. In the connector cleanup work we need to fire another uevent, to make sure userspace notices. My little patch to order connectors the other way round was kinda aimed at implementing this the cheap way (I think userspace ignores all but the last connector it sees), but I guess that didn't work. - 2nd option, and what I think we should aim for long-term: Kernel reuses the same connector if it notices that userspace hasn't cleaned it up yet. This is going to be real tricky to implement from a object lifetime pov. Since the link won't work anymore we also need to send a link_status update to inform userspace that it needs to do a modeset (like in other cases with DP link issues). Lyude's port lifetime rework should at least get us closer to this I think. Cheers, Daniel > > > > For entertainment and other reasons, testing the below diff would be > > interesting. > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c > > b/drivers/gpu/drm/i915/intel_dp_mst.c > > index 4de247ddf05f..e1b66396c83b 100644 > > --- a/drivers/gpu/drm/i915/intel_dp_mst.c > > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c > > @@ -499,6 +499,8 @@ static void > > intel_dp_register_mst_connector(struct drm_connector *connector) > > drm_fb_helper_add_one_connector(&dev_priv->fbdev- > > >helper, > > connector); > > > > + list_move(&connector->head, &connector->dev- > > >mode_config.connector_list); > > + > > drm_connector_register(connector); > > } > > > -- > Best Regards, > > Lisovskiy Stanislav -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
