[Intel-gfx] [PATCH] drm/atomic-helper: Check encoder/crtc constraints

[Ville Syrjälä]

On Thu, Nov 19, 2015 at 03:02:09PM +0100, Daniel Vetter wrote:
> On Thu, Nov 19, 2015 at 12:12:28PM +0200, Ville Syrjälä wrote:
> > On Wed, Nov 18, 2015 at 06:46:48PM +0100, Daniel Vetter wrote:
> > > This was totally lost when I originally created the atomic helpers.
> > > 
> > > We probably should also check possible_clones in the helpers, but
> > > since the legacy ones didn't do that this is for a separate patch.
> > > 
> > > Reported-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > Cc: Daniel Stone <daniels at collabora.com>
> > > Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> > 
> > Reviewed-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > Tested-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > 
> > But the rest of update_connector_routing() still looks somewhat bonkers
> > to me. For one, it assumes that both the old and new crtc for the
> > connector are part of the atomic state, but drm_atomic_set_crtc_for_connector()
> > only adds the new crtc, not the old one.
> 
> Nothing bonkers about it since drm_atomic_get_connector_state ensures that
> the corresponding crtc state is part of the update. Same logic applies for
> planes.

Ah, that's where it was. OK, so it's just seems bonkers in other ways ;)

Let's say we have as a starting point:
crtc0 -> enc0 -> conn0
crtc1 -> enc1 -> conn1

Then we do an atomic_ioctl()
-> set conn0 -> NULL
-> set conn1 -> crtc0
-> set conn2 -> crtc1

And if enc1 is the best_encoder for both conn1 and conn2,
it looks to me like we would silently end up with just:
crtc1 -> enc1 -> conn2

Even though the user asked for:
crtc0 -> conn1
crtc1 -> conn2

Also the code seems to assume that a single encoder can't drive multiple
connectors at once. Is that by design or an accident?

So I'm thinking maybe we should first check for routing conflicts in the
new state and fail if any were found. After that we could go through the
old state to steal stuff, and we should disallow stealing for the atomic
ioctl totally and only allow if from setcrtc.

> -Daniel
> 
> > 
> > > ---
> > >  drivers/gpu/drm/drm_atomic_helper.c | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > > index 0c6f62168776..cfdc9931b08a 100644
> > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > @@ -210,6 +210,14 @@ update_connector_routing(struct drm_atomic_state *state, int conn_idx)
> > >  		return -EINVAL;
> > >  	}
> > >  
> > > +	if (!drm_encoder_crtc_ok(new_encoder, connector_state->crtc)) {
> > > +		DRM_DEBUG_ATOMIC("[ENCODER:%d:%s] incompatible with [CRTC:%d]\n",
> > > +				 new_encoder->base.id,
> > > +				 new_encoder->name,
> > > +				 connector_state->crtc->base.id);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > >  	if (new_encoder == connector_state->best_encoder) {
> > >  		DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] keeps [ENCODER:%d:%s], now on [CRTC:%d]\n",
> > >  				 connector->base.id,
> > > -- 
> > > 2.5.1
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel OTC