[PATCH v2] Return only active connectors for get_resources ioctl

[PATCH v2] Return only active connectors for get_resources ioctl

[Stanislav Lisovskiy]

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++;
 			}
-			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

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

[Daniel Vetter]

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Daniel Vetter]

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);
 }
 
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Daniel Vetter]

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);
 }
 
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Lisovskiy, Stanislav]

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);
>  }
>  
-- 
Best Regards,

Lisovskiy Stanislav

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

[Lisovskiy, Stanislav]

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

Lisovskiy Stanislav

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

[Daniel Vetter]

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



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [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