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

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

[Daniel Vetter]

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);
+
+	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);
+	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);
+
 #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

[PATCH 1/2] drm/i915: Use pipe_config state to disable ilk+ pfit

[Daniel Vetter]

No more need to guard the write with a power well check on Haswell now
that we have proper pfit state readout: We can simply only clear the
pfit if it's actually on.

This removes some duplication of knowledge between the haswell pfit
disable and pfit state readout code about.

While at it extract a little helper for this.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 58395e3..d7f2eaa 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3399,6 +3399,21 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
 	intel_wait_for_vblank(dev, intel_crtc->pipe);
 }
 
+static void ironlake_pfit_disable(struct intel_crtc *crtc)
+{
+	struct drm_device *dev = crtc->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	int pipe = crtc->pipe;
+
+	/* To avoid upsetting the power well on haswell only disable the pfit if
+	 * it's in use. The hw state code will make sure we get this right. */
+	if (crtc->config.pch_pfit.size) {
+		I915_WRITE(PF_CTL(pipe), 0);
+		I915_WRITE(PF_WIN_POS(pipe), 0);
+		I915_WRITE(PF_WIN_SZ(pipe), 0);
+	}
+}
+
 static void ironlake_crtc_disable(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
@@ -3428,9 +3443,7 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
 	intel_set_pch_fifo_underrun_reporting(dev, pipe, false);
 	intel_disable_pipe(dev_priv, pipe);
 
-	/* Disable PF */
-	I915_WRITE(PF_CTL(pipe), 0);
-	I915_WRITE(PF_WIN_SZ(pipe), 0);
+	ironlake_pfit_disable(intel_crtc);
 
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		if (encoder->post_disable)
@@ -3512,14 +3525,7 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
 
 	intel_ddi_disable_transcoder_func(dev_priv, cpu_transcoder);
 
-	/* XXX: Once we have proper panel fitter state tracking implemented with
-	 * hardware state read/check support we should switch to only disable
-	 * the panel fitter when we know it's used. */
-	if (intel_display_power_enabled(dev,
-					POWER_DOMAIN_PIPE_PANEL_FITTER(pipe))) {
-		I915_WRITE(PF_CTL(pipe), 0);
-		I915_WRITE(PF_WIN_SZ(pipe), 0);
-	}
+	ironlake_pfit_disable(intel_crtc);
 
 	intel_ddi_disable_pipe_clock(intel_crtc);
 
-- 
1.7.11.7

[PATCH 2/2] drm/i915: Use pipe config state to control gmch pfit enable/disable

[Daniel Vetter]

Allows us to rip out a few fragile checks (which are duplicated in the
hw state readout now, too). Also prepares us a bit for more than one
panel/pfit.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index d7f2eaa..bc3cbe7 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3612,8 +3612,7 @@ static void i9xx_pfit_enable(struct intel_crtc *crtc)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc_config *pipe_config = &crtc->config;
 
-	if (!(intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_EDP) ||
-	      intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_LVDS)))
+	if (!crtc->config.gmch_pfit.control)
 		return;
 
 	WARN_ON(I915_READ(PFIT_CONTROL) & PFIT_ENABLE);
@@ -3732,20 +3731,15 @@ static void i9xx_pfit_disable(struct intel_crtc *crtc)
 {
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	enum pipe pipe;
-	uint32_t pctl = I915_READ(PFIT_CONTROL);
 
-	assert_pipe_disabled(dev_priv, crtc->pipe);
+	if (!crtc->config.gmch_pfit.control)
+		return;
 
-	if (INTEL_INFO(dev)->gen >= 4)
-		pipe = (pctl & PFIT_PIPE_MASK) >> PFIT_PIPE_SHIFT;
-	else
-		pipe = PIPE_B;
+	assert_pipe_disabled(dev_priv, crtc->pipe);
 
-	if (pipe == crtc->pipe) {
-		DRM_DEBUG_DRIVER("disabling pfit, current: 0x%08x\n", pctl);
-		I915_WRITE(PFIT_CONTROL, 0);
-	}
+	DRM_DEBUG_DRIVER("disabling pfit, current: 0x%08x\n",
+			 I915_READ(PFIT_CONTROL));
+	I915_WRITE(PFIT_CONTROL, 0);
 }
 
 static void i9xx_crtc_disable(struct drm_crtc *crtc)
-- 
1.7.11.7

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

[Mika Kuoppala]

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

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

[Daniel Vetter]

On Fri, May 10, 2013 at 02:37:59PM +0300, Mika Kuoppala wrote:
> 
> 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.

Originally I've had that, but then thought it'd look more symmetric if I
read out all the registers anew once we're sure that we're looking at the
right panel fitter. I can do the cse again if you want.

> > +	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?

ilk pfit control state isn't precomputed (we just use size != 0 as a
signal to set it to a specific value). It's also always the same value, so
I don't think we need to jump through loops here.

That will change of course once we bother to enable the higher upscaling
modes (if we ever do that), since those have global sharing constraints.
But for now I don't see a need.
-Daniel

> 
> >  #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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Mika Kuoppala]

Daniel Vetter <daniel@ffwll.ch> writes:

> On Fri, May 10, 2013 at 02:37:59PM +0300, Mika Kuoppala wrote:
>> 
>> 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.
>
> Originally I've had that, but then thought it'd look more symmetric if I
> read out all the registers anew once we're sure that we're looking at the
> right panel fitter. I can do the cse again if you want.

Matter of taste bikeshedding it was. No need to change.

>> > +	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?
>
> ilk pfit control state isn't precomputed (we just use size != 0 as a
> signal to set it to a specific value). It's also always the same value, so
> I don't think we need to jump through loops here.
>
> That will change of course once we bother to enable the higher upscaling
> modes (if we ever do that), since those have global sharing constraints.
> But for now I don't see a need.

As discussed in irc, we might come across a case where panel fitter
is not aligned to the same pipe in ilk+. Perhaps warn if we
encounter such a weird setup?

With or without warning added,

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

> -Daniel
>
>> 
>> >  #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
>
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/2] drm/i915: Use pipe_config state to disable ilk+ pfit

[Mika Kuoppala]

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

> No more need to guard the write with a power well check on Haswell now
> that we have proper pfit state readout: We can simply only clear the
> pfit if it's actually on.
>
> This removes some duplication of knowledge between the haswell pfit
> disable and pfit state readout code about.
>
> While at it extract a little helper for this.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

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

[Daniel Vetter]

On Fri, May 10, 2013 at 3:07 PM, Mika Kuoppala
<mika.kuoppala@linux.intel.com> wrote:
> As discussed in irc, we might come across a case where panel fitter
> is not aligned to the same pipe in ilk+. Perhaps warn if we
> encounter such a weird setup?

Yeah, our current code doesn't support different pfit->pipe
associations. They're used since not all pipes have a high-quality 7x5
or 5x5 upscale mode. I think a WARN here for ivb/hsw is justified,
even when the old code wouldn't have supported this. I'll update the
patch
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/2] drm/i915: Use pipe config state to control gmch pfit enable/disable

[Mika Kuoppala]

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

> Allows us to rip out a few fragile checks (which are duplicated in the
> hw state readout now, too). Also prepares us a bit for more than one
> panel/pfit.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

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

[Daniel Vetter]

On Fri, May 10, 2013 at 03:22:17PM +0200, Daniel Vetter wrote:
> On Fri, May 10, 2013 at 3:07 PM, Mika Kuoppala
> <mika.kuoppala@linux.intel.com> wrote:
> > As discussed in irc, we might come across a case where panel fitter
> > is not aligned to the same pipe in ilk+. Perhaps warn if we
> > encounter such a weird setup?
>
> Yeah, our current code doesn't support different pfit->pipe
> associations. They're used since not all pipes have a high-quality 7x5
> or 5x5 upscale mode. I think a WARN here for ivb/hsw is justified,
> even when the old code wouldn't have supported this. I'll update the
> patch

Jesse poked me to merge these so that he can use them right away for his
fastboot. So I've gone ahead, merged the patches as-is and will do the
WARN in a follow up. Cheap excuses while travelling ;-)

Thanks for reviewing the patch.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch