[Intel-gfx] [PATCH 09/42] drm/i915: Make intel_modeset_fixup_state similar to the atomic helper.

[Daniel Vetter]

On Mon, May 11, 2015 at 04:24:45PM +0200, Maarten Lankhorst wrote:
> This should be safe.

Usual request: A few more details about what you've changed to help guide
the review would be great. E.g. which functions from the atomic helpers
you're trying to copy here exactly.

It looks like this models set_routing_links. I think it would be rather
useful to expose this to drivers as a helper function, maybe with a more
useful name like drm_atomic_helper_update_legacy_state or similar.

Another thing I've noticed is that atomic helpers lost the call to
drm_calc_timestamping_constants. Would be good to add that to the same
function.
-Daniel

> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 82 ++++++++++++++++++------------------
>  1 file changed, 41 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index a21b2e51c054..956c9964275d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11084,36 +11084,48 @@ static void intel_modeset_update_connector_atomic_state(struct drm_device *dev)
>  	}
>  }
>  
> -/* Fixup legacy state after an atomic state swap.
> +/*
> + * Fixup legacy state in a similar way to
> + * drm_atomic_helper.c:set_routing_links.
>   */
>  static void intel_modeset_fixup_state(struct drm_atomic_state *state)
>  {
> -	struct intel_crtc *crtc;
> -	struct intel_encoder *encoder;
> -	struct intel_connector *connector;
> +	struct drm_connector *connector;
> +	struct drm_crtc_state *crtc_state;
> +	struct drm_connector_state *conn_state;
> +	struct drm_crtc *crtc;
> +	int i;
>  
> -	for_each_intel_connector(state->dev, connector) {
> -		connector->base.encoder = connector->base.state->best_encoder;
> -		if (connector->base.encoder)
> -			connector->base.encoder->crtc =
> -				connector->base.state->crtc;
> +	/*
> +	 * swap crtc and connector and update legacy state,
> +	 * plane state already gets swapped
> +	 * by the plane helpers. Once .crtc_disable is fixed
> +	 * all state should be swapped before disabling crtc's.
> +	 */
> +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +		crtc->enabled = crtc->state->enable;
> +		crtc->mode = crtc->state->mode;
>  	}
>  
> -	/* Update crtc of disabled encoders */
> -	for_each_intel_encoder(state->dev, encoder) {
> -		int num_connectors = 0;
> -
> -		for_each_intel_connector(state->dev, connector)
> -			if (connector->base.encoder == &encoder->base)
> -				num_connectors++;
> +	/* clear out existing links */
> +	for_each_connector_in_state(state, connector, conn_state, i) {
> +		if (!connector->encoder)
> +			continue;
>  
> -		if (num_connectors == 0)
> -			encoder->base.crtc = NULL;
> +		WARN_ON(!connector->encoder->crtc);
> +		connector->encoder->crtc = NULL;
> +		connector->encoder = NULL;
>  	}
>  
> -	for_each_intel_crtc(state->dev, crtc) {
> -		crtc->base.enabled = crtc->base.state->enable;
> -		crtc->config = to_intel_crtc_state(crtc->base.state);
> +	for_each_connector_in_state(state, connector, conn_state, i) {
> +		if (!connector->state->crtc)
> +			continue;
> +
> +		if (WARN_ON(!connector->state->best_encoder))
> +			continue;
> +
> +		connector->encoder = connector->state->best_encoder;
> +		connector->encoder->crtc = connector->state->crtc;
>  	}
>  }
>  
> @@ -11559,9 +11571,16 @@ intel_modeset_update_state(struct drm_atomic_state *state)
>  
>  	intel_modeset_fixup_state(state);
>  
> -	/* Double check state. */
>  	for_each_crtc(dev, crtc) {
> +		/* Double check state. */
>  		WARN_ON(crtc->state->enable != intel_crtc_in_use(crtc));
> +
> +		if (!state->crtcs[drm_crtc_index(crtc)])
> +			continue;
> +
> +		to_intel_crtc(crtc)->config = to_intel_crtc_state(crtc->state);
> +		if (crtc->state->active && needs_modeset(crtc->state))
> +			drm_calc_timestamping_constants(crtc, &crtc->state->adjusted_mode);
>  	}
>  
>  	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
> @@ -12277,25 +12296,6 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc,
>  			drm_plane_helper_disable(crtc->primary);
>  	}
>  
> -	/* crtc->mode is already used by the ->mode_set callbacks, hence we need
> -	 * to set it here already despite that we pass it down the callchain.
> -	 *
> -	 * Note we'll need to fix this up when we start tracking multiple
> -	 * pipes; here we assume a single modeset_pipe and only track the
> -	 * single crtc and mode.
> -	 */
> -	if (pipe_config->base.active && needs_modeset(&pipe_config->base)) {
> -		modeset_crtc->mode = pipe_config->base.mode;
> -
> -		/*
> -		 * Calculate and store various constants which
> -		 * are later needed by vblank and swap-completion
> -		 * timestamping. They are derived from true hwmode.
> -		 */
> -		drm_calc_timestamping_constants(modeset_crtc,
> -						&pipe_config->base.adjusted_mode);
> -	}
> -
>  	/* Only after disabling all output pipelines that will be changed can we
>  	 * update the the output configuration. */
>  	intel_modeset_update_state(state);
> -- 
> 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