| Message ID | 20181128135558.3679-1-stanislav.lisovskiy@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] Return only active connectors for get_resources ioctl | expand |
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@intel.com> > --- > 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(&conn_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. -Daniel > } > - count++; > } > } > card_res->count_connectors = count; > + DRM_DEBUG_KMS("GetResources: writing connectors end - count %d", count); > drm_connector_list_iter_end(&conn_iter); > > return ret; > -- > 2.17.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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@intel.com> > > --- > > 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(&conn_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. 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); }
On Wed, Nov 28, 2018 at 10:21:33PM +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@intel.com> > > > --- > > > 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(&conn_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. > > For entertainment and other reasons, testing the below diff would be > interesting. Now also with real locking: diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c index 4de247ddf05f..1b83567aa922 100644 --- a/drivers/gpu/drm/i915/intel_dp_mst.c +++ b/drivers/gpu/drm/i915/intel_dp_mst.c @@ -494,11 +494,16 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo static void intel_dp_register_mst_connector(struct drm_connector *connector) { struct drm_i915_private *dev_priv = to_i915(connector->dev); + struct drm_mode_config *config = &connector->dev->mode_config; if (dev_priv->fbdev) drm_fb_helper_add_one_connector(&dev_priv->fbdev->helper, connector); + spin_lock_irq(&config->connector_list_lock); + list_move(&connector->head, &config->connector_list); + spin_unlock_irq(&config->connector_list_lock); + drm_connector_register(connector); }
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@intel.com > > > > > > > --- > > > 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. > > 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); > } >
On Thu, 2018-11-29 at 07:37 +0000, Lisovskiy, Stanislav wrote: > On Wed, 2018-11-28 at 22:21 +0100, Daniel Vetter wrote: > > > > > 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. > In addition to what I said above, I checked your changes instead of proposed(also with locking) - it doesn't help, the displays get blank after unplug/plug. > > > > > > 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 > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, Nov 29, 2018 at 8:37 AM Lisovskiy, Stanislav <stanislav.lisovskiy@intel.com> 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@intel.com > > > > > > > > > --- > > > > 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
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 > > >
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(&conn_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++; } - count++; } } card_res->count_connectors = count; + DRM_DEBUG_KMS("GetResources: writing connectors end - count %d", count); drm_connector_list_iter_end(&conn_iter); return ret;
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@intel.com> --- drivers/gpu/drm/drm_mode_config.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)