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

[Daniel Vetter <daniel@ffwll.ch>]

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