Re: [PATCH 6/8] drm/i915: hw readout support for ->has_pch_encoders

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Thu, 28 Mar 2013 10:42:01 +0100
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> Now we can ditch the checks in the Haswell disable code.
> 
> v2: add support for Haswell
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 34 +++++++++++++++++++++++++---------
>  1 file changed, 25 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index a5adaa0..c9e873e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2975,11 +2975,6 @@ static void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc)
>  	mutex_unlock(&dev->struct_mutex);
>  }
>  
> -static bool haswell_crtc_driving_pch(struct drm_crtc *crtc)
> -{
> -	return intel_pipe_has_type(crtc, INTEL_OUTPUT_ANALOG);
> -}
> -
>  /* Program iCLKIP clock to the desired frequency */
>  static void lpt_program_iclkip(struct drm_crtc *crtc)
>  {
> @@ -3562,13 +3557,10 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
>  	int pipe = intel_crtc->pipe;
>  	int plane = intel_crtc->plane;
>  	enum transcoder cpu_transcoder = intel_crtc->cpu_transcoder;
> -	bool is_pch_port;
>  
>  	if (!intel_crtc->active)
>  		return;
>  
> -	is_pch_port = haswell_crtc_driving_pch(crtc);
> -
>  	for_each_encoder_on_crtc(dev, crtc, encoder)
>  		encoder->disable(encoder);
>  
> @@ -3595,7 +3587,7 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
>  		if (encoder->post_disable)
>  			encoder->post_disable(encoder);
>  
> -	if (is_pch_port) {
> +	if (intel_crtc->config.has_pch_encoder) {
>  		lpt_disable_pch_transcoder(dev_priv);
>  		intel_ddi_fdi_disable(crtc);
>  	}
> @@ -5635,6 +5627,9 @@ static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
>  	if (!(tmp & PIPECONF_ENABLE))
>  		return false;
>  
> +	if (I915_READ(TRANSCONF(crtc->pipe)) & TRANS_ENABLE)
> +		pipe_config->has_pch_encoder = true;
> +
>  	return true;
>  }
>  
> @@ -5763,6 +5758,17 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>  	if (!(tmp & PIPECONF_ENABLE))
>  		return false;
>  
> +	/*
> +	 * aswell has only FDI/PCH transcoder A. It is which is connected to
> +	 * DDI E. So just check whether this pipe is wired to DDI E and whether
> +	 * the PCH transcoder is on.
> +	 */
> +	tmp = I915_READ(TRANS_DDI_FUNC_CTL(crtc->pipe));
> +	if ((tmp & TRANS_DDI_PORT_MASK) == TRANS_DDI_SELECT_PORT(PORT_E) &&
> +	    I915_READ(TRANSCONF(PIPE_A)) & TRANS_ENABLE)
> +		pipe_config->has_pch_encoder = true;
> +
> +
>  	return true;
>  }
>  
> @@ -7692,6 +7698,14 @@ static bool
>  intel_pipe_config_compare(struct intel_crtc_config *current_config,
>  			  struct intel_crtc_config *pipe_config)
>  {
> +	if (current_config->has_pch_encoder != pipe_config->has_pch_encoder) {
> +		DRM_ERROR("mismatch in has_pch_encoder "
> +			  "(expected %i, found %i)\n",
> +			  current_config->has_pch_encoder,
> +			  pipe_config->has_pch_encoder);
> +		return false;
> +	}
> +
>  	return true;
>  }
>  
> @@ -7791,6 +7805,7 @@ intel_modeset_check_state(struct drm_device *dev)
>  		     "crtc's computed enabled state doesn't match tracked enabled state "
>  		     "(expected %i, found %i)\n", enabled, crtc->base.enabled);
>  
> +		memset(&pipe_config, 0, sizeof(pipe_config));
>  		active = dev_priv->display.get_pipe_config(crtc,
>  							   &pipe_config);
>  		WARN(crtc->active != active,
> @@ -9155,6 +9170,7 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
>  setup_pipes:
>  	list_for_each_entry(crtc, &dev->mode_config.crtc_list,
>  			    base.head) {
> +		memset(&crtc->config, 0, sizeof(crtc->config));
>  		crtc->active = dev_priv->display.get_pipe_config(crtc,
>  								 &crtc->config);
>  

I think this is correct; I was worried about mode_set, crtc_disable,
crtc_enable.  But in that case, the pipe_config shouldn't be overridden
(that won't happen until the next mode_set).  So I think this is safe.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center