[Intel-gfx] [PATCH v2 06/12] drm/i915: Make crtc checking use the atomic state.

[Daniel Vetter]

On Wed, Jul 29, 2015 at 02:49:45PM +0300, Ander Conselvan De Oliveira wrote:
> On Mon, 2015-07-27 at 14:35 +0200, Maarten Lankhorst wrote:
> > Instead of allocating pipe_config on the stack use the old crtc_state,
> > it's only going to freed from this point on.
> > 
> > All crtc's encoders are now only checked once during modeset.
> > 
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 118 +++++++++++++++++------------------
> >  1 file changed, 56 insertions(+), 62 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index fbb257d4728c..e3afe611a78c 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -11829,7 +11829,7 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
> >  	struct intel_crtc_state *pipe_config =
> >  		to_intel_crtc_state(crtc_state);
> >  	struct drm_atomic_state *state = crtc_state->state;
> > -	int ret, idx = crtc->base.id;
> > +	int ret;
> >  	bool mode_changed = needs_modeset(crtc_state);
> >  
> >  	if (mode_changed && !check_encoder_cloning(state, intel_crtc)) {
> > @@ -11837,10 +11837,6 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
> >  		return -EINVAL;
> >  	}
> >  
> > -	I915_STATE_WARN(crtc->state->active != intel_crtc->active,
> > -		"[CRTC:%i] mismatch between state->active(%i) and crtc->active(%i)\n",
> > -		idx, crtc->state->active, intel_crtc->active);
> > -
> >  	if (mode_changed && !crtc_state->active)
> >  		intel_crtc->atomic.update_wm_post = true;
> >  
> > @@ -12721,19 +12717,16 @@ check_encoder_state(struct drm_device *dev)
> >  
> >  	for_each_intel_encoder(dev, encoder) {
> >  		bool enabled = false;
> > -		bool active = false;
> > -		enum pipe pipe, tracked_pipe;
> > +		enum pipe pipe;
> >  
> >  		DRM_DEBUG_KMS("[ENCODER:%d:%s]\n",
> >  			      encoder->base.base.id,
> >  			      encoder->base.name);
> >  
> >  		for_each_intel_connector(dev, connector) {
> > -			if (connector->base.encoder != &encoder->base)
> > +			if (connector->base.state->best_encoder != &encoder->base)
> >  				continue;
> >  			enabled = true;
> > -			if (connector->base.dpms != DRM_MODE_DPMS_OFF)
> > -				active = true;
> >  
> >  			I915_STATE_WARN(connector->base.state->crtc !=
> >  					encoder->base.crtc,
> > @@ -12744,85 +12737,86 @@ check_encoder_state(struct drm_device *dev)
> >  		     "encoder's enabled state mismatch "
> >  		     "(expected %i, found %i)\n",
> >  		     !!encoder->base.crtc, enabled);
> > -		I915_STATE_WARN(active && !encoder->base.crtc,
> > -		     "active encoder with no crtc\n");
> > -
> > -		active = encoder->get_hw_state(encoder, &pipe);
> >  
> >  		if (!encoder->base.crtc) {
> > -			I915_STATE_WARN(active,
> > -			     "encoder detached but not turned off.\n");
> > +			bool active;
> >  
> > -			continue;
> > +			active = encoder->get_hw_state(encoder, &pipe);
> > +			I915_STATE_WARN(active,
> > +			     "encoder detached but still enabled on pipe %c.\n",
> > +			     pipe_name(pipe));
> >  		}
> > -
> > -		I915_STATE_WARN(active != encoder->base.crtc->state->active,
> > -		     "encoder's hw state doesn't match sw tracking "
> > -		     "(expected %i, found %i)\n",
> > -		     encoder->base.crtc->state->active, active);
> > -
> > -
> > -		tracked_pipe = to_intel_crtc(encoder->base.crtc)->pipe;
> > -		I915_STATE_WARN(active && pipe != tracked_pipe,
> > -		     "active encoder's pipe doesn't match"
> > -		     "(expected %i, found %i)\n",
> > -		     tracked_pipe, pipe);
> > -
> >  	}
> >  }
> >  
> >  static void
> > -check_crtc_state(struct drm_device *dev)
> > +check_crtc_state(struct drm_device *dev, struct drm_atomic_state *state)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > -	struct intel_crtc *crtc;
> >  	struct intel_encoder *encoder;
> > -	struct intel_crtc_state pipe_config;
> > +	struct drm_crtc_state *crtc_state;
> > +	struct drm_crtc *crtc;
> > +	int i;
> >  
> > -	for_each_intel_crtc(dev, crtc) {
> > +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> 
> So now we only check the state of crtcs affected by the modeset. In the
> unlikely case of a bug that changes the hw state of another crtc that
> should have been unchanged, the old code might catch the issue but the
> new one doesn't. Isn't it better to just check everything?

We can't since that other crtc might be doing some other async commit. But
eventually we should be doing a modeset on that one too and spot that
something went wrong.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch