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

[Daniel Vetter]

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.

Thoughts?
-Daniel

>  		}
> -	} else {
> -		intel_modeset_update_staged_output_state(dev);
>  	}
>  
>  	intel_modeset_check_state(dev);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index f85761494dd1..1e892098eea2 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -515,7 +515,6 @@ struct intel_crtc {
>  
>  	struct intel_initial_plane_config plane_config;
>  	struct intel_crtc_state *config;
> -	bool new_enabled;
>  
>  	/* reset counter value when the last flip was submitted */
>  	unsigned int reset_counter;
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch