diff mbox

drm/i915: Share the common work of disabling active FBC before updating

Message ID 1310073258-19976-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive)
State New, archived
Headers show

Commit Message

Chris Wilson July 7, 2011, 9:14 p.m. UTC
Upon review, all path share the same dependencies for updating the
registers and so we can benefit from sharing the code and checking
early.

This removes the unsightly intel_wait_for_vblank() from the lowlevel
functions and upon further analysis the only path that will require a
wait is if we are performing an instantaneous transition between two
valid FBC configurations. The page-flip path itself will have disabled
FBC registers and will have waited for at least one vblank before
finishing the flip and attempting to re-enable FBC.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---

Respun without the blocking wait. Noting instead that since we delay
enabling FBC by 50ms we can assume that a vblank passes after disabling
FBC and before modifying the control registers.
-Chris

---
 drivers/gpu/drm/i915/i915_drv.h      |    6 +-
 drivers/gpu/drm/i915/intel_display.c |  122 +++++++++++++++++----------------
 2 files changed, 65 insertions(+), 63 deletions(-)

Comments

Keith Packard July 7, 2011, 9:26 p.m. UTC | #1
On Thu,  7 Jul 2011 22:14:18 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Upon review, all path share the same dependencies for updating the
> registers and so we can benefit from sharing the code and checking
> early.
> 
> This removes the unsightly intel_wait_for_vblank() from the lowlevel
> functions and upon further analysis the only path that will require a
> wait is if we are performing an instantaneous transition between two
> valid FBC configurations. The page-flip path itself will have disabled
> FBC registers and will have waited for at least one vblank before
> finishing the flip and attempting to re-enable FBC.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
> 
> Respun without the blocking wait. Noting instead that since we delay
> enabling FBC by 50ms we can assume that a vblank passes after disabling
> FBC and before modifying the control registers.
> -Chris

Looks good to me.

Reviewed-by: Keith Packard <keithp@keithp.com>
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 2b6b3da..ad73a2b 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -329,10 +329,8 @@  typedef struct drm_i915_private {
 	uint32_t last_instdone1;
 
 	unsigned long cfb_size;
-	unsigned long cfb_pitch;
-	unsigned long cfb_offset;
-	int cfb_fence;
-	int cfb_plane;
+	unsigned int cfb_fb;
+	enum plane cfb_plane;
 	int cfb_y;
 	struct intel_fbc_work *fbc_work;
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 315191c..cbea4e6 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1410,27 +1410,17 @@  static void i8xx_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
 	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
 	struct drm_i915_gem_object *obj = intel_fb->obj;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	int cfb_pitch;
 	int plane, i;
 	u32 fbc_ctl, fbc_ctl2;
 
-	if (fb->pitch == dev_priv->cfb_pitch &&
-	    obj->fence_reg == dev_priv->cfb_fence &&
-	    intel_crtc->plane == dev_priv->cfb_plane &&
-	    I915_READ(FBC_CONTROL) & FBC_CTL_EN)
-		return;
-
-	i8xx_disable_fbc(dev);
-
-	dev_priv->cfb_pitch = dev_priv->cfb_size / FBC_LL_SIZE;
-
-	if (fb->pitch < dev_priv->cfb_pitch)
-		dev_priv->cfb_pitch = fb->pitch;
+	cfb_pitch = dev_priv->cfb_size / FBC_LL_SIZE;
+	if (fb->pitch < cfb_pitch)
+		cfb_pitch = fb->pitch;
 
 	/* FBC_CTL wants 64B units */
-	dev_priv->cfb_pitch = (dev_priv->cfb_pitch / 64) - 1;
-	dev_priv->cfb_fence = obj->fence_reg;
-	dev_priv->cfb_plane = intel_crtc->plane;
-	plane = dev_priv->cfb_plane == 0 ? FBC_CTL_PLANEA : FBC_CTL_PLANEB;
+	cfb_pitch = (cfb_pitch / 64) - 1;
+	plane = intel_crtc->plane == 0 ? FBC_CTL_PLANEA : FBC_CTL_PLANEB;
 
 	/* Clear old tags */
 	for (i = 0; i < (FBC_LL_SIZE / 32) + 1; i++)
@@ -1438,8 +1428,7 @@  static void i8xx_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
 
 	/* Set it up... */
 	fbc_ctl2 = FBC_CTL_FENCE_DBL | FBC_CTL_IDLE_IMM | plane;
-	if (obj->tiling_mode != I915_TILING_NONE)
-		fbc_ctl2 |= FBC_CTL_CPU_FENCE;
+	fbc_ctl2 |= FBC_CTL_CPU_FENCE;
 	I915_WRITE(FBC_CONTROL2, fbc_ctl2);
 	I915_WRITE(FBC_FENCE_OFF, crtc->y);
 
@@ -1447,14 +1436,13 @@  static void i8xx_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
 	fbc_ctl = FBC_CTL_EN | FBC_CTL_PERIODIC;
 	if (IS_I945GM(dev))
 		fbc_ctl |= FBC_CTL_C3_IDLE; /* 945 needs special SR handling */
-	fbc_ctl |= (dev_priv->cfb_pitch & 0xff) << FBC_CTL_STRIDE_SHIFT;
+	fbc_ctl |= (cfb_pitch & 0xff) << FBC_CTL_STRIDE_SHIFT;
 	fbc_ctl |= (interval & 0x2fff) << FBC_CTL_INTERVAL_SHIFT;
-	if (obj->tiling_mode != I915_TILING_NONE)
-		fbc_ctl |= dev_priv->cfb_fence;
+	fbc_ctl |= obj->fence_reg;
 	I915_WRITE(FBC_CONTROL, fbc_ctl);
 
-	DRM_DEBUG_KMS("enabled FBC, pitch %ld, yoff %d, plane %d, ",
-		      dev_priv->cfb_pitch, crtc->y, dev_priv->cfb_plane);
+	DRM_DEBUG_KMS("enabled FBC, pitch %d, yoff %d, plane %d, ",
+		      cfb_pitch, crtc->y, intel_crtc->plane);
 }
 
 static bool i8xx_fbc_enabled(struct drm_device *dev)
@@ -1476,23 +1464,8 @@  static void g4x_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
 	unsigned long stall_watermark = 200;
 	u32 dpfc_ctl;
 
-	dpfc_ctl = I915_READ(DPFC_CONTROL);
-	if (dpfc_ctl & DPFC_CTL_EN) {
-		if (dev_priv->cfb_fence == obj->fence_reg &&
-		    dev_priv->cfb_plane == intel_crtc->plane &&
-		    dev_priv->cfb_y == crtc->y)
-			return;
-
-		I915_WRITE(DPFC_CONTROL, dpfc_ctl & ~DPFC_CTL_EN);
-		intel_wait_for_vblank(dev, intel_crtc->pipe);
-	}
-
-	dev_priv->cfb_fence = obj->fence_reg;
-	dev_priv->cfb_plane = intel_crtc->plane;
-	dev_priv->cfb_y = crtc->y;
-
 	dpfc_ctl = plane | DPFC_SR_EN | DPFC_CTL_LIMIT_1X;
-	dpfc_ctl |= DPFC_CTL_FENCE_EN | dev_priv->cfb_fence;
+	dpfc_ctl |= DPFC_CTL_FENCE_EN | obj->fence_reg;
 	I915_WRITE(DPFC_CHICKEN, DPFC_HT_MODIFY);
 
 	I915_WRITE(DPFC_RECOMP_CTL, DPFC_RECOMP_STALL_EN |
@@ -1561,27 +1534,11 @@  static void ironlake_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
 	u32 dpfc_ctl;
 
 	dpfc_ctl = I915_READ(ILK_DPFC_CONTROL);
-	if (dpfc_ctl & DPFC_CTL_EN) {
-		if (dev_priv->cfb_fence == obj->fence_reg &&
-		    dev_priv->cfb_plane == intel_crtc->plane &&
-		    dev_priv->cfb_offset == obj->gtt_offset &&
-		    dev_priv->cfb_y == crtc->y)
-			return;
-
-		I915_WRITE(ILK_DPFC_CONTROL, dpfc_ctl & ~DPFC_CTL_EN);
-		intel_wait_for_vblank(dev, intel_crtc->pipe);
-	}
-
-	dev_priv->cfb_fence = obj->fence_reg;
-	dev_priv->cfb_plane = intel_crtc->plane;
-	dev_priv->cfb_offset = obj->gtt_offset;
-	dev_priv->cfb_y = crtc->y;
-
 	dpfc_ctl &= DPFC_RESERVED;
 	dpfc_ctl |= (plane | DPFC_CTL_LIMIT_1X);
 	/* Set persistent mode for front-buffer rendering, ala X. */
 	dpfc_ctl |= DPFC_CTL_PERSISTENT_MODE;
-	dpfc_ctl |= (DPFC_CTL_FENCE_EN | dev_priv->cfb_fence);
+	dpfc_ctl |= (DPFC_CTL_FENCE_EN | obj->fence_reg);
 	I915_WRITE(ILK_DPFC_CHICKEN, DPFC_HT_MODIFY);
 
 	I915_WRITE(ILK_DPFC_RECOMP_CTL, DPFC_RECOMP_STALL_EN |
@@ -1594,7 +1551,7 @@  static void ironlake_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
 
 	if (IS_GEN6(dev)) {
 		I915_WRITE(SNB_DPFC_CTL_SA,
-			   SNB_CPU_FENCE_ENABLE | dev_priv->cfb_fence);
+			   SNB_CPU_FENCE_ENABLE | obj->fence_reg);
 		I915_WRITE(DPFC_CPU_FENCE_OFFSET, crtc->y);
 		sandybridge_blit_fbc_update(dev);
 	}
@@ -1647,10 +1604,15 @@  static void intel_fbc_work_fn(struct work_struct *__work)
 		/* Double check that we haven't switched fb without cancelling
 		 * the prior work.
 		 */
-		if (work->crtc->fb == work->fb)
+		if (work->crtc->fb == work->fb) {
 			dev_priv->display.enable_fbc(work->crtc,
 						     work->interval);
 
+			dev_priv->cfb_plane = to_intel_crtc(work->crtc)->plane;
+			dev_priv->cfb_fb = work->crtc->fb->base.id;
+			dev_priv->cfb_y = work->crtc->y;
+		}
+
 		dev_priv->fbc_work = NULL;
 	}
 	mutex_unlock(&dev->struct_mutex);
@@ -1708,7 +1670,10 @@  static void intel_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
 	DRM_DEBUG_KMS("scheduling delayed FBC enable\n");
 
 	/* Delay the actual enabling to let pageflipping cease and the
-	 * display to settle before starting the compression.
+	 * display to settle before starting the compression. Note that
+	 * this delay also serves a second purpose: it allows for a
+	 * vblank to pass after disabling the FBC before we attempt
+	 * to modify the control registers.
 	 *
 	 * A more complicated solution would involve tracking vblanks
 	 * following the termination of the page-flipping sequence
@@ -1728,6 +1693,7 @@  void intel_disable_fbc(struct drm_device *dev)
 		return;
 
 	dev_priv->display.disable_fbc(dev);
+	dev_priv->cfb_plane = -1;
 }
 
 /**
@@ -1836,6 +1802,44 @@  static void intel_update_fbc(struct drm_device *dev)
 	if (in_dbg_master())
 		goto out_disable;
 
+	/* If the scanout has not changed, don't modify the FBC settings.
+	 * Note that we make the fundamental assumption that the fb->obj
+	 * cannot be unpinned (and have its GTT offset and fence revoked)
+	 * without first being decoupled from the scanout and FBC disabled.
+	 */
+	if (dev_priv->cfb_plane == intel_crtc->plane &&
+	    dev_priv->cfb_fb == fb->base.id &&
+	    dev_priv->cfb_y == crtc->y)
+		return;
+
+	if (intel_fbc_enabled(dev)) {
+		/* We update FBC along two paths, after changing fb/crtc
+		 * configuration (modeswitching) and after page-flipping
+		 * finishes. For the latter, we know that not only did
+		 * we disable the FBC at the start of the page-flip
+		 * sequence, but also more than one vblank has passed.
+		 *
+		 * For the former case of modeswitching, it is possible
+		 * to switch between two FBC valid configurations
+		 * instantaneously so we do need to disable the FBC
+		 * before we can modify its control registers. We also
+		 * have to wait for the next vblank for that to take
+		 * effect. However, since we delay enabling FBC we can
+		 * assume that a vblank has passed since disabling and
+		 * that we can safely alter the registers in the deferred
+		 * callback.
+		 *
+		 * In the scenario that we go from a valid to invalid
+		 * and then back to valid FBC configuration we have
+		 * no strict enforcement that a vblank occurred since
+		 * disabling the FBC. However, along all current pipe
+		 * disabling paths we do need to wait for a vblank at
+		 * some point. And we wait before enabling FBC anyway.
+		 */
+		DRM_DEBUG_KMS("disabling active FBC for update\n");
+		intel_disable_fbc(dev);
+	}
+
 	intel_enable_fbc(crtc, 500);
 	return;