From 8553c18ea696827f26d3ba5652d3a2fa9ce8e874 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Ville=20Syrj=C3=A4l=C3=A4?= Date: Thu, 5 Dec 2013 15:51:39 +0200 Subject: [PATCH] drm/i915: Try to fix the messy IVB sprite scaling workaround MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit We now have a very clear method of disabling LP1+ wartermarks, and we can actually detect if we actually did disable them, or if they were already disabled. Use that to clean up the WaCxSRDisabledForSpriteScaling:ivb handling. I was hoping to apply the workaround in a way that wouldn't require a blocking wait, but sadly IVB really does appear to require LP1+ watermarks to be off for an entire frame before enabling sprite scaling. Simply disabling LP1+ watermarks during the previous frame is not enough, no matter how early in the frame we do it :( Signed-off-by: Ville Syrjälä Reviewed-by: Imre Deak Signed-off-by: Daniel Vetter --- drivers/gpu/drm/i915/i915_drv.h | 1 - drivers/gpu/drm/i915/intel_pm.c | 58 ++++++++++++++++++++++++++++--------- drivers/gpu/drm/i915/intel_sprite.c | 27 +---------------- 3 files changed, 46 insertions(+), 40 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index b0a6e76..50da8a5 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1401,7 +1401,6 @@ typedef struct drm_i915_private { /* overlay */ struct intel_overlay *overlay; - unsigned int sprite_scaling_enabled; /* backlight registers and fields in struct intel_panel */ spinlock_t backlight_lock; diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 7e02834..ecdb64e 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -2426,33 +2426,26 @@ static unsigned int ilk_compute_wm_dirty(struct drm_device *dev, return dirty; } -/* - * The spec says we shouldn't write when we don't need, because every write - * causes WMs to be re-evaluated, expending some power. - */ -static void hsw_write_wm_values(struct drm_i915_private *dev_priv, - struct hsw_wm_values *results) +static bool _ilk_disable_lp_wm(struct drm_i915_private *dev_priv, + unsigned int dirty) { - struct drm_device *dev = dev_priv->dev; struct hsw_wm_values *previous = &dev_priv->wm.hw; - unsigned int dirty; - uint32_t val; - - dirty = ilk_compute_wm_dirty(dev_priv->dev, previous, results); - if (!dirty) - return; + bool changed = false; if (dirty & WM_DIRTY_LP(3) && previous->wm_lp[2] & WM1_LP_SR_EN) { previous->wm_lp[2] &= ~WM1_LP_SR_EN; I915_WRITE(WM3_LP_ILK, previous->wm_lp[2]); + changed = true; } if (dirty & WM_DIRTY_LP(2) && previous->wm_lp[1] & WM1_LP_SR_EN) { previous->wm_lp[1] &= ~WM1_LP_SR_EN; I915_WRITE(WM2_LP_ILK, previous->wm_lp[1]); + changed = true; } if (dirty & WM_DIRTY_LP(1) && previous->wm_lp[0] & WM1_LP_SR_EN) { previous->wm_lp[0] &= ~WM1_LP_SR_EN; I915_WRITE(WM1_LP_ILK, previous->wm_lp[0]); + changed = true; } /* @@ -2460,6 +2453,27 @@ static void hsw_write_wm_values(struct drm_i915_private *dev_priv, * Doing so could cause underruns. */ + return changed; +} + +/* + * The spec says we shouldn't write when we don't need, because every write + * causes WMs to be re-evaluated, expending some power. + */ +static void hsw_write_wm_values(struct drm_i915_private *dev_priv, + struct hsw_wm_values *results) +{ + struct drm_device *dev = dev_priv->dev; + struct hsw_wm_values *previous = &dev_priv->wm.hw; + unsigned int dirty; + uint32_t val; + + dirty = ilk_compute_wm_dirty(dev, previous, results); + if (!dirty) + return; + + _ilk_disable_lp_wm(dev_priv, dirty); + if (dirty & WM_DIRTY_PIPE(PIPE_A)) I915_WRITE(WM0_PIPEA_ILK, results->wm_pipe[0]); if (dirty & WM_DIRTY_PIPE(PIPE_B)) @@ -2523,6 +2537,13 @@ static void hsw_write_wm_values(struct drm_i915_private *dev_priv, dev_priv->wm.hw = *results; } +static bool ilk_disable_lp_wm(struct drm_device *dev) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + + return _ilk_disable_lp_wm(dev_priv, WM_DIRTY_LP_ALL); +} + static void haswell_update_wm(struct drm_crtc *crtc) { struct intel_crtc *intel_crtc = to_intel_crtc(crtc); @@ -2572,6 +2593,7 @@ static void haswell_update_sprite_wm(struct drm_plane *plane, uint32_t sprite_width, int pixel_size, bool enabled, bool scaled) { + struct drm_device *dev = plane->dev; struct intel_plane *intel_plane = to_intel_plane(plane); intel_plane->wm.enabled = enabled; @@ -2579,6 +2601,16 @@ static void haswell_update_sprite_wm(struct drm_plane *plane, intel_plane->wm.horiz_pixels = sprite_width; intel_plane->wm.bytes_per_pixel = pixel_size; + /* + * IVB workaround: must disable low power watermarks for at least + * one frame before enabling scaling. LP watermarks can be re-enabled + * when scaling is disabled. + * + * WaCxSRDisabledForSpriteScaling:ivb + */ + if (IS_IVYBRIDGE(dev) && scaled && ilk_disable_lp_wm(dev)) + intel_wait_for_vblank(dev, intel_plane->pipe); + haswell_update_wm(crtc); } diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index 90a3f6d..0bba103 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -230,7 +230,6 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, u32 sprctl, sprscale = 0; unsigned long sprsurf_offset, linear_offset; int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0); - bool scaling_was_enabled = dev_priv->sprite_scaling_enabled; sprctl = I915_READ(SPRCTL(pipe)); @@ -291,21 +290,8 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, crtc_w--; crtc_h--; - /* - * IVB workaround: must disable low power watermarks for at least - * one frame before enabling scaling. LP watermarks can be re-enabled - * when scaling is disabled. - */ - if (crtc_w != src_w || crtc_h != src_h) { - dev_priv->sprite_scaling_enabled |= 1 << pipe; - - if (!scaling_was_enabled) { - intel_update_watermarks(crtc); - intel_wait_for_vblank(dev, pipe); - } + if (crtc_w != src_w || crtc_h != src_h) sprscale = SPRITE_SCALE_ENABLE | (src_w << 16) | src_h; - } else - dev_priv->sprite_scaling_enabled &= ~(1 << pipe); I915_WRITE(SPRSTRIDE(pipe), fb->pitches[0]); I915_WRITE(SPRPOS(pipe), (crtc_y << 16) | crtc_x); @@ -332,10 +318,6 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, I915_MODIFY_DISPBASE(SPRSURF(pipe), i915_gem_obj_ggtt_offset(obj) + sprsurf_offset); POSTING_READ(SPRSURF(pipe)); - - /* potentially re-enable LP watermarks */ - if (scaling_was_enabled && !dev_priv->sprite_scaling_enabled) - intel_update_watermarks(crtc); } static void @@ -345,7 +327,6 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc) struct drm_i915_private *dev_priv = dev->dev_private; struct intel_plane *intel_plane = to_intel_plane(plane); int pipe = intel_plane->pipe; - bool scaling_was_enabled = dev_priv->sprite_scaling_enabled; I915_WRITE(SPRCTL(pipe), I915_READ(SPRCTL(pipe)) & ~SPRITE_ENABLE); /* Can't leave the scaler enabled... */ @@ -355,13 +336,7 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc) I915_MODIFY_DISPBASE(SPRSURF(pipe), 0); POSTING_READ(SPRSURF(pipe)); - dev_priv->sprite_scaling_enabled &= ~(1 << pipe); - intel_update_sprite_watermarks(plane, crtc, 0, 0, false, false); - - /* potentially re-enable LP watermarks */ - if (scaling_was_enabled && !dev_priv->sprite_scaling_enabled) - intel_update_watermarks(crtc); } static int -- 2.7.4