Re: [PATCH 07/42] drm/i915: Get rid of crtc->new_enabled, v2.

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, May 11, 2015 at 07:33:01PM +0200, Daniel Vetter wrote:
> On Mon, May 11, 2015 at 04:24:43PM +0200, Maarten Lankhorst wrote:
> > @@ -14679,7 +14646,8 @@ static bool primary_get_hw_state(struct intel_crtc *crtc)
> >  	return I915_READ(DSPCNTR(crtc->plane)) & DISPLAY_PLANE_ENABLE;
> >  }
> >  
> > -static void intel_modeset_readout_hw_state(struct drm_device *dev)
> > +static void intel_modeset_readout_hw_state(struct drm_device *dev,
> > +					   unsigned *crtc_mask)
> 
> Why is crtc_mask a paramter?
> 
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	enum pipe pipe;
> > @@ -14688,6 +14656,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
> >  	struct intel_connector *connector;
> >  	int i;
> >  
> > +	*crtc_mask = 0;
> >  	for_each_intel_crtc(dev, crtc) {
> >  		struct drm_plane *primary = crtc->base.primary;
> >  		struct intel_plane_state *plane_state;
> > @@ -14696,6 +14665,9 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
> >  
> >  		crtc->config->quirks |= PIPE_CONFIG_QUIRK_INHERITED_MODE;
> >  
> > +		if (crtc->active)
> > +			*crtc_mask |= drm_crtc_index(&crtc->base);
> > +
> >  		crtc->active = dev_priv->display.get_pipe_config(crtc,
> >  								 crtc->config);
> >  
> > @@ -14778,7 +14750,9 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
> >  	struct intel_encoder *encoder;
> >  	int i;
> >  
> > -	intel_modeset_readout_hw_state(dev);
> > +	unsigned crtc_mask;
> > +
> > +	intel_modeset_readout_hw_state(dev, &crtc_mask);
> >  
> >  	/*
> >  	 * Now that we have the config, copy it to each CRTC struct
> > @@ -14837,10 +14811,9 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
> >  			struct drm_crtc *crtc =
> >  				dev_priv->pipe_to_crtc_mapping[pipe];
> >  
> > -			intel_crtc_restore_mode(crtc);
> > +			if (crtc_mask & (1 << drm_crtc_index(crtc)))
> > +				intel_crtc_restore_mode(crtc);
> 
> this seems a bit backwards. If we push the crtc loop down into
> intel_crct_restore_mode (and call it intel_restore_mode or so) then we
> could reuse the ->enable state from atomic. I.e.
> 
> 	/* all the code in intel_crtc_restore_mode except the set_mode
> 	 * call at the bottom */
> 
> 	for_crc_in_atomic_state(crtc_state) 
> 		if (crtc_state->enable)
> 			intel_set_mode
> 
> That would look more atomic imo (we'll just need to replace this loop with
> a drm_atomic_commit once we're there). And contain the restore state
> tracking to one function instead of spreading it over readout_hw_state.

This doesn't work, and we actually need to pull the atomic state copying
out of restore_mode up into readout_hw_state so that we can use it instead
of ->new_* pointers and intel_crtc->config. And that needs to happen
_before_ remove the new_ pointers ofc.

Otherwise (i.e. after this patch) well overwrite object linking in
readout_hw_state and then restore_mode will do something silly.

Or do I still miss something?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx