[Intel-gfx] [PATCH 20/23] drm/i915: Use atomic state in pipe_has_enabled_pch()

[Daniel Vetter]

On Tue, Mar 03, 2015 at 03:22:14PM +0200, Ander Conselvan de Oliveira wrote:
> This function is called indirectly by intel_crtc_compute_config(),
> which needs to be converted to work only with an atomic state.
> 
> ---
> 
> I'm not sure what are the implications of ignoring intel_crtc->active in
> pipe_has_enabled_pch(). If we allow a config because the third pipe is
> enabled but not active, wouldn't we run into trouble when we tried to
> activate the crtc?

Once we have more of atomic ready and switched to the helpers (or
something equivalent) for enabling/disabling display pipes we need to
remove our usage of intel_crtc->active. Maybe keep it as a debug check.

The new rules are:
- crtc_state->enable controls resource allocation, i.e. this is the bit we
  need to look at for checking shared resources and stuff like that.
- crtc_state->active (and also intel_crtc->active) only control/track the
  physical active state of the pipe (controlled by dpms or the new atomic
  active property). Changing crtc_state->active may never fail.

I think your change actually fixes a bug with the fdi config check code:
If the other pipe is in dpms off we might steal the fdi lanes and then the
subsequent dpms on could fail.

Perhaps split out the removal of intel_crtc->active from the
has_enabled_pch as a bugfix? Would at least help with review.

It should be possible to reproduce this on an ivb with the following
sequence:

1. Enable big mode (uses more than 2 lanes) on pipe B.
2. dpms off pipe B
3. Enable big mode (uses more than 2 lanes) on pipe C.
4. dpms on pipe B should go boom now on one of the FDI checks.

Maybe we need another modeset on pipe C to hit them though or something
like that, we don't have full checks in the dpms patch.

Thanks, Daniel

> 
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira at intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 22 +++++++++++++++-------
>  1 file changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 64751b6..518903e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3150,10 +3150,9 @@ static void intel_fdi_normal_train(struct drm_crtc *crtc)
>  			   FDI_FE_ERRC_ENABLE);
>  }
>  
> -static bool pipe_has_enabled_pch(struct intel_crtc *crtc)
> +static bool pipe_has_enabled_pch(struct intel_crtc_state *crtc_state)
>  {
> -	return crtc->base.state->enable && crtc->active &&
> -		crtc->config->has_pch_encoder;
> +	return crtc_state->base.enable && crtc_state->has_pch_encoder;
>  }
>  
>  static void ivb_modeset_global_resources(struct drm_atomic_state *state)
> @@ -3164,15 +3163,21 @@ static void ivb_modeset_global_resources(struct drm_atomic_state *state)
>  		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_B]);
>  	struct intel_crtc *pipe_C_crtc =
>  		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_C]);
> +	struct intel_crtc_state *pipe_B_crtc_state, *pipe_C_crtc_state;
>  	uint32_t temp;
>  
> +	pipe_B_crtc_state = intel_atomic_get_crtc_state(state, pipe_B_crtc);
> +	pipe_C_crtc_state = intel_atomic_get_crtc_state(state, pipe_C_crtc);
> +	if (WARN_ON(IS_ERR(pipe_B_crtc_state) || IS_ERR(pipe_C_crtc_state)))
> +		return;
> +
>  	/*
>  	 * When everything is off disable fdi C so that we could enable fdi B
>  	 * with all lanes. Note that we don't care about enabled pipes without
>  	 * an enabled pch encoder.
>  	 */
> -	if (!pipe_has_enabled_pch(pipe_B_crtc) &&
> -	    !pipe_has_enabled_pch(pipe_C_crtc)) {
> +	if (!pipe_has_enabled_pch(pipe_B_crtc_state) &&
> +	    !pipe_has_enabled_pch(pipe_C_crtc_state)) {
>  		WARN_ON(I915_READ(FDI_RX_CTL(PIPE_B)) & FDI_RX_ENABLE);
>  		WARN_ON(I915_READ(FDI_RX_CTL(PIPE_C)) & FDI_RX_ENABLE);
>  
> @@ -5528,6 +5533,9 @@ static bool ironlake_check_fdi_lanes(struct drm_device *dev, enum pipe pipe,
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *pipe_B_crtc =
>  		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_B]);
> +	struct intel_crtc_state *pipe_B_crtc_state =
> +		intel_atomic_get_crtc_state(pipe_config->base.state,
> +					    pipe_B_crtc);
>  
>  	DRM_DEBUG_KMS("checking fdi config on pipe %c, lanes %i\n",
>  		      pipe_name(pipe), pipe_config->fdi_lanes);
> @@ -5563,8 +5571,8 @@ static bool ironlake_check_fdi_lanes(struct drm_device *dev, enum pipe pipe,
>  		}
>  		return true;
>  	case PIPE_C:
> -		if (!pipe_has_enabled_pch(pipe_B_crtc) ||
> -		    pipe_B_crtc->config->fdi_lanes <= 2) {
> +		if (!pipe_has_enabled_pch(pipe_B_crtc_state) ||
> +		    pipe_B_crtc_state->fdi_lanes <= 2) {
>  			if (pipe_config->fdi_lanes > 2) {
>  				DRM_DEBUG_KMS("invalid shared fdi lane config on pipe %c: %i lanes\n",
>  					      pipe_name(pipe), pipe_config->fdi_lanes);
> -- 
> 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
+41 (0) 79 365 57 48 - http://blog.ffwll.ch