Re: [PATCH] drm/i915: panel fitter hw state readout&check support

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

Hi Daniel,

Daniel Vetter <daniel.vetter@ffwll.ch> writes:

> Pfit state readout is a bit ugly on gen2/3 due to the intermingling
> with the lvds state, but alas.
>
> Also note that since state is always cleared to zero we can
> unconditonally compare all the state and completely neglect the actual
> platform we're running on.
>
> v2: Properly check for the pfit power domain on haswell.
>
> v3: Don't check pgm_ratios on gen4+, they're auto-computed by the hw.
>
> v4: Properly clear the lvds border bits, upset the state checker a
> bit.
>
> v5: Unconditionally read out panel dither settings on gen2/3.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 67 ++++++++++++++++++++++++++++++++++--
>  drivers/gpu/drm/i915/intel_lvds.c    |  1 +
>  2 files changed, 66 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index bfbb484..58395e3 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4967,6 +4967,36 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
>  	return ret;
>  }
>  
> +static void i9xx_get_pfit_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(PFIT_CONTROL);

You could do
const uint32_t ctl = I915_READ(PFIT_CONTROL);
and...

> +	if (INTEL_INFO(dev)->gen < 4) {
> +		if (crtc->pipe != PIPE_B)
> +			return;
> +
> +		/* gen2/3 store dither state in pfit control, needs to match */
> +		pipe_config->gmch_pfit.control = tmp & PANEL_8TO6_DITHER_ENABLE;
> +	} else {
> +		if ((tmp & PFIT_PIPE_MASK) != (crtc->pipe << PFIT_PIPE_SHIFT))
> +			return;
> +	}
> +
> +	if (!(tmp & PFIT_ENABLE))
> +		return;
> +
> +	pipe_config->gmch_pfit.control = I915_READ(PFIT_CONTROL);

and use ctl in here to avoid extra read.

> +	pipe_config->gmch_pfit.pgm_ratios = I915_READ(PFIT_PGM_RATIOS);
> +	if (INTEL_INFO(dev)->gen < 5)
> +		pipe_config->gmch_pfit.lvds_border_bits =
> +			I915_READ(LVDS) & LVDS_BORDER_ENABLE;
> +}
> +
>  static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
>  				 struct intel_crtc_config *pipe_config)
>  {
> @@ -4980,6 +5010,8 @@ static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
>  
>  	intel_get_pipe_timings(crtc, pipe_config);
>  
> +	i9xx_get_pfit_config(crtc, pipe_config);
> +
>  	return true;
>  }
>  
> @@ -5815,6 +5847,21 @@ static void ironlake_get_fdi_m_n_config(struct intel_crtc *crtc,
>  				   & TU_SIZE_MASK) >> TU_SIZE_SHIFT) + 1;
>  }
>  
> +static void ironlake_get_pfit_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(PF_CTL(crtc->pipe));
> +
> +	if (tmp & PF_ENABLE) {
> +		pipe_config->pch_pfit.pos = I915_READ(PF_WIN_POS(crtc->pipe));
> +		pipe_config->pch_pfit.size = I915_READ(PF_WIN_SZ(crtc->pipe));
> +	}
> +}
> +
>  static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
>  				     struct intel_crtc_config *pipe_config)
>  {
> @@ -5838,6 +5885,8 @@ static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
>  
>  	intel_get_pipe_timings(crtc, pipe_config);
>  
> +	ironlake_get_pfit_config(crtc, pipe_config);
> +
>  	return true;
>  }
>  
> @@ -5957,6 +6006,7 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>  	struct drm_device *dev = crtc->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	enum transcoder cpu_transcoder = crtc->config.cpu_transcoder;
> +	enum intel_display_power_domain pfit_domain;
>  	uint32_t tmp;
>  
>  	if (!intel_display_power_enabled(dev,
> @@ -5986,6 +6036,10 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>  
>  	intel_get_pipe_timings(crtc, pipe_config);
>  
> +	pfit_domain = POWER_DOMAIN_PIPE_PANEL_FITTER(crtc->pipe);
> +	if (intel_display_power_enabled(dev, pfit_domain))
> +		ironlake_get_pfit_config(crtc, pipe_config);
> +
>  	return true;
>  }
>  
> @@ -7961,7 +8015,8 @@ intel_modeset_update_state(struct drm_device *dev, unsigned prepare_pipes)
>  		if (mask & (1 <<(intel_crtc)->pipe))
>  
>  static bool
> -intel_pipe_config_compare(struct intel_crtc_config *current_config,
> +intel_pipe_config_compare(struct drm_device *dev,
> +			  struct intel_crtc_config *current_config,
>  			  struct intel_crtc_config *pipe_config)
>  {
>  #define PIPE_CONF_CHECK_I(name)	\
> @@ -8010,6 +8065,14 @@ intel_pipe_config_compare(struct intel_crtc_config *current_config,
>  	PIPE_CONF_CHECK_I(requested_mode.hdisplay);
>  	PIPE_CONF_CHECK_I(requested_mode.vdisplay);
>  
> +	PIPE_CONF_CHECK_I(gmch_pfit.control);
> +	/* pfit ratios are autocomputed by the hw on gen4+ */
> +	if (INTEL_INFO(dev)->gen < 4)
> +		PIPE_CONF_CHECK_I(gmch_pfit.pgm_ratios);
> +	PIPE_CONF_CHECK_I(gmch_pfit.lvds_border_bits);
> +	PIPE_CONF_CHECK_I(pch_pfit.pos);
> +	PIPE_CONF_CHECK_I(pch_pfit.size);
> +

For ironlake+ we don't get conf checking for the control register part.
Should we?

>  #undef PIPE_CONF_CHECK_I
>  #undef PIPE_CONF_CHECK_FLAGS
>  
> @@ -8121,7 +8184,7 @@ intel_modeset_check_state(struct drm_device *dev)
>  		     "(expected %i, found %i)\n", crtc->active, active);
>  
>  		WARN(active &&
> -		     !intel_pipe_config_compare(&crtc->config, &pipe_config),
> +		     !intel_pipe_config_compare(dev, &crtc->config, &pipe_config),
>  		     "pipe state doesn't match!\n");
>  	}
>  }
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index d256fe4..3294efd 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -116,6 +116,7 @@ static void intel_pre_pll_enable_lvds(struct intel_encoder *encoder)
>  	}
>  
>  	/* set the corresponsding LVDS_BORDER bit */
> +	temp &= ~LVDS_BORDER_ENABLE;
>  	temp |= intel_crtc->config.gmch_pfit.lvds_border_bits;
>  	/* Set the B0-B3 data pairs corresponding to whether we're going to
>  	 * set the DPLLs for dual-channel mode or not.
> -- 
> 1.7.11.7
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx