From mboxrd@z Thu Jan 1 00:00:00 1970 From: Rodrigo Vivi Subject: Re: [PATCH 1/3] drm/i915: implement IPS feature Date: Fri, 31 May 2013 13:07:33 -0300 Message-ID: References: <20130521084122.GF12292@phenom.ffwll.local> <1369344388-4330-1-git-send-email-przanoni@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-ve0-f172.google.com (mail-ve0-f172.google.com [209.85.128.172]) by gabe.freedesktop.org (Postfix) with ESMTP id 8B708E60DD for ; Fri, 31 May 2013 09:07:33 -0700 (PDT) Received: by mail-ve0-f172.google.com with SMTP id jz10so1275225veb.31 for ; Fri, 31 May 2013 09:07:33 -0700 (PDT) In-Reply-To: <1369344388-4330-1-git-send-email-przanoni@gmail.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Paulo Zanoni Cc: intel-gfx , Paulo Zanoni List-Id: intel-gfx@lists.freedesktop.org Reviewed-by: Rodrigo Vivi On Thu, May 23, 2013 at 6:26 PM, Paulo Zanoni wrote: > From: Paulo Zanoni > > 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 > --- > 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; > + > + /* 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 -- Rodrigo Vivi Blog: http://blog.vivi.eng.br