[Intel-gfx] [PATCH] drm: Allow modeset on unregisted connectors unconditionally

[Daniel Vetter]

On Mon, May 20, 2019 at 10:09:24PM +0300, Imre Deak wrote:
> On Mon, May 20, 2019 at 08:37:46PM +0200, Daniel Vetter wrote:
> > On Mon, May 20, 2019 at 08:41:09PM +0300, Imre Deak wrote:
> > > We allowed modesetting an unregistered connector only in the case the
> > > mode is getting disabled on the connector.
> > > 
> > > The reason for this check was the lack of proper refcounting for the
> > > backing memory objects. That problem has been solved meanwhile so there
> > > is no reason any more to reject the modesetting in general.
> > 
> > I'm not parsing this at all ... maybe references to the commits that fix
> > this? Or do you mean the refcounting work for all the things hanging of
> > connectors, including the entire mst tree?
> 
> Yes the check was added to solve the issue related to the removal of MST
> connectors that could happen asynchronously wrt. a modeset referring to
> that MST connector.  That could happen since the MST core doesn't hold
> any locks (for instance the connection_mutex) during removing an MST
> connector that would prevent doing a modeset at the same time.
> 
> Adding the refcounting for such MST connectors (via the
> drm_connector_get()/drm_connector_put()) got rid of the above problem.

We added the check way after that stuff landed. Before all the connector
reworking connectors were forcefully disabled by the kernel. The idea
behind this check is to make sure that that userspace notices a connector
is gone (only thing that's not allowed is enabling it, you can keep
pageflipping). I think we've always had behaviour like ever since mst (all
userspace has some "oops mst connector probably gone" failure catching
around modesets).

So no idea what all blows up if we stop catching userspace this way.

Now very much possible I'm getting all this wrong again or missing
something, this stuff is often way over my head. But I'm really vary of
breaking userspace here. E.g. just the drm_connector_get/put lifetime
changes results in some userspace breaking if you unplug/replug fast
enough, because then it doesn't notice the connector change anymore. I
couldn't figure out a way to paper over that regression without
reintroduce the rather broken and oops-prone old connector lifetime
management.

> > > The check
> > > for that also makes driver internal modesets more cumbersome where we
> > > need to add exemptions for the cases where we do need to allow the
> > > modeset even for unregistered connectors. One such case is the
> > > restoration of the mode during resume.
> > 
> > Yeah this one actually makes sense to me. We could still keep this check
> > here, but for the atomic ioctl only when called from userspace. But iirc
> > Lyude also said she has some plans here, so no idea whether that all fits.
> > 
> > > Simplify things by removing the unneeded check. I can't see how
> > > modesetting an unregistered connector can cause any problem and the race
> > > (described in the code comment) can anyway result in such a modeset (if
> > > the connector is unregistered right after the check).
> > 
> > Not saying we don't need this, but there's fairly enormous amounts of
> > history behind all this stuff, and lots of discussions. Would be good to
> > at least reference those, so we have a good story for when this then all
> > goes wrong again.
> 
> I still don't see why this check is needed. There is no justification
> for it - besides the original reason for it as discussed above about the
> refcounting problem, which is solved now - so I think we should remove
> it, instead of just making it a special case for the user space modeset.
> 
> As I wrote a user space modeset can end up anyway doing a modeset on an
> unregistered connector when the unregistering - by MST core - happens just
> right after the check.

Yup. Always been like that.
-Daniel

> 
> > -Daniel
> > 
> > > 
> > > Cc: Lyude Paul <lyude at redhat.com>
> > > Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> > > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > Signed-off-by: Imre Deak <imre.deak at intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_atomic_helper.c | 29 ++---------------------------
> > >  1 file changed, 2 insertions(+), 27 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > > index 2e0cb4246cbd..e94e69483498 100644
> > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > @@ -319,33 +319,6 @@ update_connector_routing(struct drm_atomic_state *state,
> > >  		return 0;
> > >  	}
> > >  
> > > -	crtc_state = drm_atomic_get_new_crtc_state(state,
> > > -						   new_connector_state->crtc);
> > > -	/*
> > > -	 * For compatibility with legacy users, we want to make sure that
> > > -	 * we allow DPMS On->Off modesets on unregistered connectors. Modesets
> > > -	 * which would result in anything else must be considered invalid, to
> > > -	 * avoid turning on new displays on dead connectors.
> > > -	 *
> > > -	 * Since the connector can be unregistered at any point during an
> > > -	 * atomic check or commit, this is racy. But that's OK: all we care
> > > -	 * about is ensuring that userspace can't do anything but shut off the
> > > -	 * display on a connector that was destroyed after it's been notified,
> > > -	 * not before.
> > > -	 *
> > > -	 * Additionally, we also want to ignore connector registration when
> > > -	 * we're trying to restore an atomic state during system resume since
> > > -	 * there's a chance the connector may have been destroyed during the
> > > -	 * process, but it's better to ignore that then cause
> > > -	 * drm_atomic_helper_resume() to fail.
> > > -	 */
> > > -	if (!state->duplicated && drm_connector_is_unregistered(connector) &&
> > > -	    crtc_state->active) {
> > > -		DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] is not registered\n",
> > > -				 connector->base.id, connector->name);
> > > -		return -EINVAL;
> > > -	}
> > > -
> > >  	funcs = connector->helper_private;
> > >  
> > >  	if (funcs->atomic_best_encoder)
> > > @@ -390,6 +363,8 @@ update_connector_routing(struct drm_atomic_state *state,
> > >  
> > >  	set_best_encoder(state, new_connector_state, new_encoder);
> > >  
> > > +	crtc_state = drm_atomic_get_new_crtc_state(state,
> > > +						   new_connector_state->crtc);
> > >  	crtc_state->connectors_changed = true;
> > >  
> > >  	DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] using [ENCODER:%d:%s] on [CRTC:%d:%s]\n",
> > > -- 
> > > 2.17.1
> > > 
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch

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