[PATCH 06/17] drm: Global atomic state handling

[Daniel Vetter]

On Tue, Nov 04, 2014 at 10:30:37PM +0100, Daniel Vetter wrote:
> On Tue, Nov 04, 2014 at 03:31:07PM -0500, Sean Paul wrote:
> > On Sun, Nov 02, 2014 at 02:19:19PM +0100, Daniel Vetter wrote:
> > > +drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
> > > +  struct drm_crtc *crtc)
> > > +{
> > > + struct drm_crtc_state *crtc_state;
> > > +
> > > + crtc_state = drm_atomic_get_crtc_state(conn_state->state, crtc);
> > > + if (IS_ERR(crtc_state))
> > > + return PTR_ERR(crtc_state);
> > > +
> > > + conn_state->crtc = crtc;
> > > +
> > > + DRM_DEBUG_KMS("Link connector state %p to [CRTC:%d]\n",
> > > +      conn_state, crtc->base.id);
> > > +
> > > + return 0;
> > > +}
> > > +EXPORT_SYMBOL(drm_atomic_set_crtc_for_connector);
> > 
> > In drm_atomic_helper.c, you're assigning directly to conn_state->crtc. I think
> > you should probably be using this helper (as it doesn't seem to be used
> > anywhere). I realize this happens in a different patch, but I want to make sure
> > I don't miss it later :-)
> 
> Indeed this is a bug, and I didn't catch it because I couldn't test
> connector stealing. Scenario:
> 
> Connector A is active on CRTC 0.
> 
> Userspace does a legacy setCrtc with connector A and CRTC 1.
> 
> -> We should steal the connector and disable CRTC 0 (since it doesn't have
> any other connectors) completely. But because the helpers didn't pull in
> the state for CRTC this doesn't happen.

Actually there's a very good chance we'll get away: If we steal the
connector we'll likely also steal its old encoder, and the encoder
stealing already grabs the crtc on its own. Still, better safe than sorry.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch