[Intel-gfx] [PATCH 05/19] drm/i915: Update dummy connector atomic state with current config

[Konduru, Chandra]

> -----Original Message-----
> From: Conselvan De Oliveira, Ander
> Sent: Friday, March 13, 2015 2:49 AM
> To: intel-gfx at lists.freedesktop.org
> Cc: Konduru, Chandra; Conselvan De Oliveira, Ander
> Subject: [PATCH 05/19] drm/i915: Update dummy connector atomic state with
> current config
> 
> Keep that state updated so that we can write code that depends on it on the
> follow up patches.
> 
> v2: Fix BUG() due to stale connector_state->crtc value. (Chandra)
> Signed-off-by: Ander Conselvan de Oliveira
> <ander.conselvan.de.oliveira at intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 41 ++++++++++++++++++++++++++++----
> ----
>  1 file changed, 32 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 62b9021..abdbd0c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10114,6 +10114,27 @@ static void
> intel_modeset_update_staged_output_state(struct drm_device *dev)
>  	}
>  }
> 
> +/* Transitional helper to copy current connector/encoder state to
> + * connector->state. This is needed so that code that is partially
> + * converted to atomic does the right thing.
> + */
> +static void intel_modeset_update_connector_atomic_state(struct
> +drm_device *dev) {
> +	struct intel_connector *connector;
> +
> +	for_each_intel_connector(dev, connector) {
> +		if (connector->base.encoder) {
> +			connector->base.state->best_encoder =
> +				connector->base.encoder;
> +			connector->base.state->crtc =
> +				connector->base.encoder->crtc;
> +		} else {
> +			connector->base.state->best_encoder = NULL;
> +			connector->base.state->crtc = NULL;
> +		}
> +	}
> +}
> +
>  /**
>   * intel_modeset_commit_output_state
>   *
> @@ -10137,6 +10158,8 @@ static void
> intel_modeset_commit_output_state(struct drm_device *dev)
>  		crtc->base.state->enable = crtc->new_enabled;
>  		crtc->base.enabled = crtc->new_enabled;
>  	}
> +
> +	intel_modeset_update_connector_atomic_state(dev);
>  }
> 
>  static void
> @@ -12876,15 +12899,13 @@ static void intel_setup_outputs(struct
> drm_device *dev)
>  	 * be removed since we'll be setting up real connector state, which
>  	 * will contain Intel-specific properties.
>  	 */
> -	if (drm_core_check_feature(dev, DRIVER_ATOMIC)) {
> -		list_for_each_entry(connector,
> -				    &dev->mode_config.connector_list,
> -				    head) {
> -			if (!WARN_ON(connector->state)) {
> -				connector->state =
> -					kzalloc(sizeof(*connector->state),
> -						GFP_KERNEL);
> -			}
> +	/* FIXME: need to update the comment above. */

Can you fix the comment instead of adding another FIXME to fix it later?

> +	list_for_each_entry(connector,
> +			    &dev->mode_config.connector_list,
> +			    head) {
> +		if (!WARN_ON(connector->state)) {

In intel_modeset_stage_output_state() there is a call to setup
connector state:
	connector_state = drm_atomic_get_connector_state(state, &connector->base);
Is that call is covering modeset via crtc_set_config() but not during init flow, so 
doing it here?  
If that is the case, we probably know that connector->state
is never being set, so why have WARN_ON()?

As lower level compute code is already 
> +			connector->state = kzalloc(sizeof(*connector->state),
> +						   GFP_KERNEL);
>  		}
>  	}
> 
> @@ -13937,6 +13958,8 @@ void intel_modeset_setup_hw_state(struct
> drm_device *dev,
>  				       "[setup_hw_state]");
>  	}
> 
> +	intel_modeset_update_connector_atomic_state(dev);
> +
>  	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
>  		struct intel_shared_dpll *pll = &dev_priv->shared_dplls[i];
> 
> --
> 2.1.0