Re: [PATCH 1/3] drm/i915: implement IPS feature

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, May 23, 2013 at 06:26:28PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Intermediate Pixel Storage is a feature that should reduce the number
> of times the display engine wakes up memory to read pixels, so it
> should allow deeper PC states. IPS can only be enabled on ULT pipe A
> with 8:8:8 pipe pixel formats.
> 
> With eDP 1920x1080 and correct watermarks but without FBC this moves
> my PC7 residency from 2.5% to around 38%.
> 
> v2: - It's tied to pipe A, not port A
>     - Add pipe_config support (Chris)
>     - Add some assertions (Chris)
>     - Rebase against latest dinq
> v3: - Don't ever set ips_enabled to false (Daniel)
>     - Only check for ips_enabled at hsw_disable_ips (Daniel)
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h      | 11 ++++++
>  drivers/gpu/drm/i915/intel_display.c | 73 +++++++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_drv.h     |  2 +
>  3 files changed, 84 insertions(+), 2 deletions(-)
> 
> 
> The workaround we implement is called WANoName. What's the correct way to
> proceed for WANoName?
> 
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 58230ea..5db20dc 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -977,6 +977,8 @@
>  /* Framebuffer compression for Ivybridge */
>  #define IVB_FBC_RT_BASE			0x7020
>  
> +#define IPS_CTL		0x43408
> +#define   IPS_ENABLE	(1 << 31)
>  
>  #define _HSW_PIPE_SLICE_CHICKEN_1_A	0x420B0
>  #define _HSW_PIPE_SLICE_CHICKEN_1_B	0x420B4
> @@ -3620,6 +3622,15 @@
>  #define _LGC_PALETTE_B           0x4a800
>  #define LGC_PALETTE(pipe) _PIPE(pipe, _LGC_PALETTE_A, _LGC_PALETTE_B)
>  
> +#define _GAMMA_MODE_A		0x4a480
> +#define _GAMMA_MODE_B		0x4ac80
> +#define GAMMA_MODE(pipe) _PIPE(pipe, _GAMMA_MODE_A, _GAMMA_MODE_B)
> +#define GAMMA_MODE_MODE_MASK	(3 << 0)
> +#define GAMMA_MODE_MODE_8bit	(0 << 0)
> +#define GAMMA_MODE_MODE_10bit	(1 << 0)
> +#define GAMMA_MODE_MODE_12bit	(2 << 0)
> +#define GAMMA_MODE_MODE_SPLIT	(3 << 0)
> +
>  /* interrupts */
>  #define DE_MASTER_IRQ_CONTROL   (1 << 31)
>  #define DE_SPRITEB_FLIP_DONE    (1 << 29)
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 1c0d6d3..bc99183 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3340,6 +3340,51 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
>  	intel_wait_for_vblank(dev, intel_crtc->pipe);
>  }
>  
> +/* IPS only exists on ULT machines and is tied to pipe A. */
> +static bool hsw_crtc_supports_ips(struct intel_crtc *crtc)
> +{
> +	return (IS_ULT(crtc->base.dev) && crtc->pipe == PIPE_A);
> +}
> +
> +static void hsw_enable_ips(struct intel_crtc *crtc)
> +{
> +	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> +
> +	if (!hsw_crtc_supports_ips(crtc))
> +		return;
> +
> +	if (crtc->config.pipe_bpp != 24)
> +		return;
> +
> +	DRM_DEBUG_KMS("Enabling IPS\n");
> +
> +	crtc->config.ips_enabled = true;

Oops, I've found another one.

The general rule (there are some violations due to in-progress conversion)
is that the pipe_config should _never_ be changed in the ->mode_set and
->enable hooks. Instead it should be completely pre-calculated in the
compute_config stage. So can you please move the above code into a
haswell_compute_ips function and call it with a IS_HASWELL check from
ironlake_fdi_compute_config (maybe at the end where we compute the fdi
config)?

You can also dump the debug output and move that into dump_pipe_config.

The reason for that strict separation is that in the end I want to use the
pipe_config to decide about the different ways we can do modesets like:
- full enable/disable sequence
- fastboot for special cases + some fixups
- just an fb update
If not everything is pre-computed we'll have to deal with even more
special cases for this.

Cheers, Daniel

> +
> +	/* We can only enable IPS after we enable a plane and wait for a vblank.
> +	 * We guarantee that the plane is enabled by calling intel_enable_ips
> +	 * only after intel_enable_plane. And intel_enable_plane already waits
> +	 * for a vblank, so all we need to do here is to enable the IPS bit. */
> +	assert_plane_enabled(dev_priv, crtc->plane);
> +	I915_WRITE(IPS_CTL, IPS_ENABLE);
> +}
> +
> +static void hsw_disable_ips(struct intel_crtc *crtc)
> +{
> +	struct drm_device *dev = crtc->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	if (!crtc->config.ips_enabled)
> +		return;
> +
> +	DRM_DEBUG_KMS("Disabling IPS\n");
> +
> +	assert_plane_enabled(dev_priv, crtc->plane);
> +	I915_WRITE(IPS_CTL, 0);
> +
> +	/* We need to wait for a vblank before we can disable the plane. */
> +	intel_wait_for_vblank(dev, crtc->pipe);
> +}
> +
>  static void haswell_crtc_enable(struct drm_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->dev;
> @@ -3387,6 +3432,8 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>  			  intel_crtc->config.has_pch_encoder);
>  	intel_enable_plane(dev_priv, plane, pipe);
>  
> +	hsw_enable_ips(intel_crtc);
> +
>  	if (intel_crtc->config.has_pch_encoder)
>  		lpt_pch_enable(crtc);
>  
> @@ -3529,6 +3576,8 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
>  	if (dev_priv->cfb_plane == plane)
>  		intel_disable_fbc(dev);
>  
> +	hsw_disable_ips(intel_crtc);
> +
>  	intel_disable_plane(dev_priv, plane, pipe);
>  
>  	if (intel_crtc->config.has_pch_encoder)
> @@ -6051,6 +6100,9 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>  	if (intel_display_power_enabled(dev, pfit_domain))
>  		ironlake_get_pfit_config(crtc, pipe_config);
>  
> +	pipe_config->ips_enabled = hsw_crtc_supports_ips(crtc) &&
> +				   (I915_READ(IPS_CTL) & IPS_ENABLE);
> +
>  	return true;
>  }
>  
> @@ -6355,8 +6407,10 @@ void intel_crtc_load_lut(struct drm_crtc *crtc)
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	int palreg = PALETTE(intel_crtc->pipe);
> +	enum pipe pipe = intel_crtc->pipe;
> +	int palreg = PALETTE(pipe);
>  	int i;
> +	bool reenable_ips = false;
>  
>  	/* The clocks have to be on to load the palette. */
>  	if (!crtc->enabled || !intel_crtc->active)
> @@ -6364,7 +6418,17 @@ void intel_crtc_load_lut(struct drm_crtc *crtc)
>  
>  	/* use legacy palette for Ironlake */
>  	if (HAS_PCH_SPLIT(dev))
> -		palreg = LGC_PALETTE(intel_crtc->pipe);
> +		palreg = LGC_PALETTE(pipe);
> +
> +	/* Workaround : Do not read or write the pipe palette/gamma data while
> +	 * GAMMA_MODE is configured for split gamma and IPS_CTL has IPS enabled.
> +	 */
> +	if (intel_crtc->config.ips_enabled &&
> +	    ((I915_READ(GAMMA_MODE(pipe)) & GAMMA_MODE_MODE_MASK) ==
> +	     GAMMA_MODE_MODE_SPLIT)) {
> +		hsw_disable_ips(intel_crtc);
> +		reenable_ips = true;
> +	}
>  
>  	for (i = 0; i < 256; i++) {
>  		I915_WRITE(palreg + 4 * i,
> @@ -6372,6 +6436,9 @@ void intel_crtc_load_lut(struct drm_crtc *crtc)
>  			   (intel_crtc->lut_g[i] << 8) |
>  			   intel_crtc->lut_b[i]);
>  	}
> +
> +	if (reenable_ips)
> +		hsw_enable_ips(intel_crtc);
>  }
>  
>  static void i845_update_cursor(struct drm_crtc *crtc, u32 base)
> @@ -8083,6 +8150,8 @@ intel_pipe_config_compare(struct drm_device *dev,
>  	PIPE_CONF_CHECK_I(pch_pfit.pos);
>  	PIPE_CONF_CHECK_I(pch_pfit.size);
>  
> +	PIPE_CONF_CHECK_I(ips_enabled);
> +
>  #undef PIPE_CONF_CHECK_I
>  #undef PIPE_CONF_CHECK_FLAGS
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 57de0c1..2045974 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -268,6 +268,8 @@ struct intel_crtc_config {
>  	/* FDI configuration, only valid if has_pch_encoder is set. */
>  	int fdi_lanes;
>  	struct intel_link_m_n fdi_m_n;
> +
> +	bool ips_enabled;
>  };
>  
>  struct intel_crtc {
> -- 
> 1.8.1.2
> 
> _______________________________________________
> 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