[Intel-gfx] [PATCH 1/3] drm/i915: implement IPS feature
Daniel Vetter
daniel at ffwll.ch
Tue May 21 10:41:22 CEST 2013
On Thu, May 16, 2013 at 04:54:04PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni at 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
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
Three small comments below.
> ---
> drivers/gpu/drm/i915/i915_reg.h | 11 +++++
> drivers/gpu/drm/i915/intel_display.c | 79 +++++++++++++++++++++++++++++++++++-
> drivers/gpu/drm/i915/intel_drv.h | 2 +
> 3 files changed, 90 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 32eb97c..65339a7 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
> @@ -3621,6 +3623,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 d588ff6..5b41cf3 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3340,6 +3340,53 @@ 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;
> +
> + /* 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 (!hsw_crtc_supports_ips(crtc))
> + return;
Like Chris suggested I think it's better to use the pipe_config state here
to decide whether to disable ips or not. Otherwise we could leak out an
enabled IPS config if the BIOS sets it up, but the module param is not
set.
> +
> + DRM_DEBUG_KMS("Disabling IPS\n");
> +
> + crtc->config.ips_enabled = false;
> +
> + 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 +3434,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 +3578,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)
> @@ -5021,6 +5072,8 @@ static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
>
> i9xx_get_pfit_config(crtc, pipe_config);
>
> + pipe_config->ips_enabled = false;
No need to ever clear a pipe_config since they are _always_ cleared to
zero. If we'd start doing that we'd very soon have a giant mess of
read-out code which clears things totally unrelated to the platform at
hand.
> +
> return true;
> }
>
> @@ -5890,6 +5943,8 @@ static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
>
> ironlake_get_pfit_config(crtc, pipe_config);
>
> + pipe_config->ips_enabled = false;
> +
> return true;
> }
>
> @@ -6041,6 +6096,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;
> }
>
> @@ -6345,8 +6403,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)
> @@ -6354,7 +6414,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.
> + */
Does this w/a have an official name from the db? If so can you please add
the tag in Damien's new layout?
Cheers, Daniel
> + 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,
> @@ -6362,6 +6432,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)
> @@ -8064,6 +8137,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 ba05cc7..5e4844a 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -264,6 +264,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 at 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
More information about the Intel-gfx
mailing list