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

["Lisovskiy, Stanislav" <stanislav.lisovskiy@intel.com>]

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