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

[Daniel Vetter <daniel@ffwll.ch>]

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.

> If that is the case, I think this could also benefit from the !crtc checks the
> plane equivalent has.

Yup, otherwise helpers need to check for that. I'll fix both and all the
nits you've spotted, too.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel