Re: [PATCH 5/8] drm/i915: add hw state readout/checking for pipe_config

[Jesse Barnes <jbarnes@virtuousgeek.org>]

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

> We need to be able to read out the hw state code for a bunch
> of reasons:
> - Correctly disabling boot-up/resume state.
> - Pure paranoia.
> 
> Since not all of the pipe configuration is e.g. relevant for
> fastboot (or at least we can allow some wiggle room in some
> parameters, like the clocks), we need to add a strict_checking
> parameter to intel_pipe_config_compare for fastboot.
> 
> For now intel_pipe_config_compare should be fully paranoid and
> check everything that the hw state readout code supports. Which
> for this infrastructure code is nothing.
> 
> I've gone a bit overboard with adding 3 get_pipe_config functions:
> The ilk version will differ with the next patch, so it's not too
> onerous.
> 
> v2: Don't check the hw config if the pipe is off, since an enabled,
> but dpms off crtc will obviously have tons of difference with the hw
> state.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |  5 +++
>  drivers/gpu/drm/i915/intel_display.c | 77 +++++++++++++++++++++++++++++++-----
>  2 files changed, 72 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 86777c8..99d7f81 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -285,6 +285,7 @@ struct drm_i915_error_state {
>  };
>  
>  struct intel_crtc_config;
> +struct intel_crtc;
>  
>  struct drm_i915_display_funcs {
>  	bool (*fbc_enabled)(struct drm_device *dev);
> @@ -298,6 +299,10 @@ struct drm_i915_display_funcs {
>  	void (*update_linetime_wm)(struct drm_device *dev, int pipe,
>  				 struct drm_display_mode *mode);
>  	void (*modeset_global_resources)(struct drm_device *dev);
> +	/* Returns the active state of the crtc, and if the crtc is active,
> +	 * fills out the pipe-config with the hw state. */
> +	bool (*get_pipe_config)(struct intel_crtc *,
> +				struct intel_crtc_config *);
>  	int (*crtc_mode_set)(struct drm_crtc *crtc,
>  			     int x, int y,
>  			     struct drm_framebuffer *old_fb);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index e925efe..a5adaa0 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4695,6 +4695,20 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
>  	return ret;
>  }
>  
> +static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
> +				 struct intel_crtc_config *pipe_config)
> +{
> +	struct drm_device *dev = crtc->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	uint32_t tmp;
> +
> +	tmp = I915_READ(PIPECONF(crtc->pipe));
> +	if (!(tmp & PIPECONF_ENABLE))
> +		return false;
> +
> +	return true;
> +}
> +
>  static void ironlake_init_pch_refclk(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -5355,7 +5369,6 @@ static void ironlake_fdi_set_m_n(struct drm_crtc *crtc)
>  		&intel_crtc->config.adjusted_mode;
>  	struct intel_link_m_n m_n = {0};
>  	int target_clock, lane, link_bw;
> -	uint32_t bps;
>  
>  	/* FDI is a binary signal running at ~2.7GHz, encoding
>  	 * each output octet as 10 bits. The actual frequency
> @@ -5611,6 +5624,20 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>  	return fdi_config_ok ? ret : -EINVAL;
>  }
>  
> +static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
> +				     struct intel_crtc_config *pipe_config)
> +{
> +	struct drm_device *dev = crtc->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	uint32_t tmp;
> +
> +	tmp = I915_READ(PIPECONF(crtc->pipe));
> +	if (!(tmp & PIPECONF_ENABLE))
> +		return false;
> +
> +	return true;
> +}
> +
>  static void haswell_modeset_global_resources(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -5725,6 +5752,20 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
>  	return ret;
>  }
>  
> +static bool haswell_get_pipe_config(struct intel_crtc *crtc,
> +				    struct intel_crtc_config *pipe_config)
> +{
> +	struct drm_device *dev = crtc->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	uint32_t tmp;
> +
> +	tmp = I915_READ(PIPECONF(crtc->cpu_transcoder));
> +	if (!(tmp & PIPECONF_ENABLE))
> +		return false;
> +
> +	return true;
> +}
> +
>  static int intel_crtc_mode_set(struct drm_crtc *crtc,
>  			       int x, int y,
>  			       struct drm_framebuffer *fb)
> @@ -7647,12 +7688,21 @@ intel_modeset_update_state(struct drm_device *dev, unsigned prepare_pipes)
>  			    base.head) \
>  		if (mask & (1 <<(intel_crtc)->pipe)) \
>  
> +static bool
> +intel_pipe_config_compare(struct intel_crtc_config *current_config,
> +			  struct intel_crtc_config *pipe_config)
> +{
> +	return true;
> +}
> +
>  void
>  intel_modeset_check_state(struct drm_device *dev)
>  {
> +	drm_i915_private_t *dev_priv = dev->dev_private;
>  	struct intel_crtc *crtc;
>  	struct intel_encoder *encoder;
>  	struct intel_connector *connector;
> +	struct intel_crtc_config pipe_config;
>  
>  	list_for_each_entry(connector, &dev->mode_config.connector_list,
>  			    base.head) {
> @@ -7741,7 +7791,15 @@ 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);
>  
> -		assert_pipe(dev->dev_private, crtc->pipe, crtc->active);
> +		active = dev_priv->display.get_pipe_config(crtc,
> +							   &pipe_config);
> +		WARN(crtc->active != active,
> +		     "crtc active state doesn't match with hw state "
> +		     "(expected %i, found %i)\n", crtc->active, active);
> +
> +		WARN(active &&
> +		     !intel_pipe_config_compare(&crtc->config, &pipe_config),
> +		     "pipe state doesn't match!\n");
>  	}
>  }
>  
> @@ -8549,18 +8607,21 @@ static void intel_init_display(struct drm_device *dev)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
>  	if (HAS_DDI(dev)) {
> +		dev_priv->display.get_pipe_config = haswell_get_pipe_config;
>  		dev_priv->display.crtc_mode_set = haswell_crtc_mode_set;
>  		dev_priv->display.crtc_enable = haswell_crtc_enable;
>  		dev_priv->display.crtc_disable = haswell_crtc_disable;
>  		dev_priv->display.off = haswell_crtc_off;
>  		dev_priv->display.update_plane = ironlake_update_plane;
>  	} else if (HAS_PCH_SPLIT(dev)) {
> +		dev_priv->display.get_pipe_config = ironlake_get_pipe_config;
>  		dev_priv->display.crtc_mode_set = ironlake_crtc_mode_set;
>  		dev_priv->display.crtc_enable = ironlake_crtc_enable;
>  		dev_priv->display.crtc_disable = ironlake_crtc_disable;
>  		dev_priv->display.off = ironlake_crtc_off;
>  		dev_priv->display.update_plane = ironlake_update_plane;
>  	} else {
> +		dev_priv->display.get_pipe_config = i9xx_get_pipe_config;
>  		dev_priv->display.crtc_mode_set = i9xx_crtc_mode_set;
>  		dev_priv->display.crtc_enable = i9xx_crtc_enable;
>  		dev_priv->display.crtc_disable = i9xx_crtc_disable;
> @@ -9092,14 +9153,10 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
>  	}
>  
>  setup_pipes:
> -	for_each_pipe(pipe) {
> -		crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
> -
> -		tmp = I915_READ(PIPECONF(crtc->cpu_transcoder));
> -		if (tmp & PIPECONF_ENABLE)
> -			crtc->active = true;
> -		else
> -			crtc->active = false;
> +	list_for_each_entry(crtc, &dev->mode_config.crtc_list,
> +			    base.head) {
> +		crtc->active = dev_priv->display.get_pipe_config(crtc,
> +								 &crtc->config);
>  
>  		crtc->base.enabled = crtc->active;
>  

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

-- 
Jesse Barnes, Intel Open Source Technology Center