On Thu, 2018-11-29 at 09:48 +0100, Daniel Vetter wrote:
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.
To be honest, I didn't check anything except xf86-video-intel driver, however at least it removes immediately that output from the list, if it wasn't available with recent get_resources ioctl. So far with that patch I didn't notice any issues(been using it for 3 weeks already). However I agree, this might be not the best way to fix that. Another way I could fix that is just analyze connector state like this in ddx:
if (sna_output->serial == serial) { - if (output_check_status(sna, sna_output)) { + if (output_check_status(sna, sna_output, &status)) { DBG(("%s: output %s (id=%d), retained state\n", __FUNCTION__, output->name, sna_output->id)); sna_output->last_detect = now; @@ -5653,7 +5653,8 @@ void sna_mode_discover(struct sna *sna, bool tell) sna_output->last_detect = 0; changed |= 4; } - continue; + if (status != XF86OutputStatusDisconnected) + continue; }
DBG(("%s: removing output %s (id=%d), serial=%u [now %u]\n", __FUNCTION__, output->name, sna_output->id, sna_output->serial, serial));
xf86DrvMsg(sna->scrn->scrnIndex, X_INFO, "Disabled output %s\n", output->name); sna_output->id = 0; sna_output->last_detect = 0; output->crtc = NULL; RROutputChanged(output->randr_output, TRUE); changed |= 2;
If status is disconnected, it then calls removes the output, which does the trick.
- 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.
I'm actually already trying to implement this as reusing same connector id looks correct to me, however that fix seems to be much more complex from coding perspective and potentially bringing more issues. That is why I decided to post this patch, as at least it gives some way to cope with that ugly behaviour - considering that the bug I'm tracking was opened 7 month ago and this is still not fixed.
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