[Intel-gfx] [PATCH v2] Return only active connectors for get_resources ioctl

[Lisovskiy, Stanislav]

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
> 
> 
> 
-- 
Best Regards,

Lisovskiy Stanislav