FBC fixes for review and testing

FBC fixes for review and testing

[Chris Wilson]

FBC is currently disabled upstream due a few conflicting requirements
and questionable benefit. It is also buggy...

https://bugs.freedesktop.org/show_bug.cgi?id=33487 is the current open
SNB bug where FBC prevents the screen from being redrawn under a
compositing WM. At last it appears that we have found the cause and a
solution, so please do give these patches some testing - you will need
to pass i915.i915_enable_fbc=1 to override the forced disabling of FBC.

1-2: tidy up some spurious code in the fbc routines. 1 is very confused
indeed.

3: Enables persistent-mode for ILK/SNB which the documentation and
discussion suggests is necessary for correct front-buffer rendering,
such as X. I wonder if this then relates to
https://bugs.freedesktop.org/show_bug.cgi?id=3174

4: Demonstrates the problem encountered with mixed-mode compositors (i.e.
they combine page-flipping with copy-sub-buffer blits). The simple
solution here comes at a cost though - it will halve the vsync'ed update
rate. Note that this problem is universal to our implementation of FBC.

5: Prepares the way for the solution to the performance regression,
this could be done in conjunction with 1/2, and unexports the chipset
specific routines and forces the couple of external callers of disable
FBC through the generic interface which is about to gain some extra
complications...

6: Defers intel_enable_fbc() by 50ms to ensure that page-flipping has
completed before incurring a slow and synchronous restart of FBC.
-Chris

[PATCH 1/6] drm/i915: Remove vestigial pitch from post-gen2 FBC control routines

[Chris Wilson]

The cfb_pitch was only used for 8xx_enable_fbc(), every later routine
was just overwriting the value with itself thanks to a copy'n'paste
error.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_display.c |    8 ++------
 1 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e58627f..577229d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1478,8 +1478,7 @@ static void g4x_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
 
 	dpfc_ctl = I915_READ(DPFC_CONTROL);
 	if (dpfc_ctl & DPFC_CTL_EN) {
-		if (dev_priv->cfb_pitch == dev_priv->cfb_pitch / 64 - 1 &&
-		    dev_priv->cfb_fence == obj->fence_reg &&
+		if (dev_priv->cfb_fence == obj->fence_reg &&
 		    dev_priv->cfb_plane == intel_crtc->plane &&
 		    dev_priv->cfb_y == crtc->y)
 			return;
@@ -1488,7 +1487,6 @@ static void g4x_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
 		intel_wait_for_vblank(dev, intel_crtc->pipe);
 	}
 
-	dev_priv->cfb_pitch = (dev_priv->cfb_pitch / 64) - 1;
 	dev_priv->cfb_fence = obj->fence_reg;
 	dev_priv->cfb_plane = intel_crtc->plane;
 	dev_priv->cfb_y = crtc->y;
@@ -1568,8 +1566,7 @@ static void ironlake_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
 
 	dpfc_ctl = I915_READ(ILK_DPFC_CONTROL);
 	if (dpfc_ctl & DPFC_CTL_EN) {
-		if (dev_priv->cfb_pitch == dev_priv->cfb_pitch / 64 - 1 &&
-		    dev_priv->cfb_fence == obj->fence_reg &&
+		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)
@@ -1579,7 +1576,6 @@ static void ironlake_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
 		intel_wait_for_vblank(dev, intel_crtc->pipe);
 	}
 
-	dev_priv->cfb_pitch = (dev_priv->cfb_pitch / 64) - 1;
 	dev_priv->cfb_fence = obj->fence_reg;
 	dev_priv->cfb_plane = intel_crtc->plane;
 	dev_priv->cfb_offset = obj->gtt_offset;
-- 
1.7.5.4

Re: [PATCH 1/6] drm/i915: Remove vestigial pitch from post-gen2 FBC control routines

[Jesse Barnes]

On Thu,  7 Jul 2011 12:48:06 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> The cfb_pitch was only used for 8xx_enable_fbc(), every later routine
> was just overwriting the value with itself thanks to a copy'n'paste
> error.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---

Yep, looks good.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

[PATCH 2/6] drm/i915: Use of a CPU fence is mandatory to update FBC regions upon CPU writes

[Chris Wilson]

...and this requirement is enforced by intel_update_fbc() so we can
remove the later check from g4x_enable_fbc() and ironlake_enable_fbc().

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_display.c |   16 ++++------------
 1 files changed, 4 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 577229d..0cbae6c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1492,12 +1492,8 @@ static void g4x_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
 	dev_priv->cfb_y = crtc->y;
 
 	dpfc_ctl = plane | DPFC_SR_EN | DPFC_CTL_LIMIT_1X;
-	if (obj->tiling_mode != I915_TILING_NONE) {
-		dpfc_ctl |= DPFC_CTL_FENCE_EN | dev_priv->cfb_fence;
-		I915_WRITE(DPFC_CHICKEN, DPFC_HT_MODIFY);
-	} else {
-		I915_WRITE(DPFC_CHICKEN, ~DPFC_HT_MODIFY);
-	}
+	dpfc_ctl |= DPFC_CTL_FENCE_EN | dev_priv->cfb_fence;
+	I915_WRITE(DPFC_CHICKEN, DPFC_HT_MODIFY);
 
 	I915_WRITE(DPFC_RECOMP_CTL, DPFC_RECOMP_STALL_EN |
 		   (stall_watermark << DPFC_RECOMP_STALL_WM_SHIFT) |
@@ -1583,12 +1579,8 @@ static void ironlake_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
 
 	dpfc_ctl &= DPFC_RESERVED;
 	dpfc_ctl |= (plane | DPFC_CTL_LIMIT_1X);
-	if (obj->tiling_mode != I915_TILING_NONE) {
-		dpfc_ctl |= (DPFC_CTL_FENCE_EN | dev_priv->cfb_fence);
-		I915_WRITE(ILK_DPFC_CHICKEN, DPFC_HT_MODIFY);
-	} else {
-		I915_WRITE(ILK_DPFC_CHICKEN, ~DPFC_HT_MODIFY);
-	}
+	dpfc_ctl |= (DPFC_CTL_FENCE_EN | dev_priv->cfb_fence);
+	I915_WRITE(ILK_DPFC_CHICKEN, DPFC_HT_MODIFY);
 
 	I915_WRITE(ILK_DPFC_RECOMP_CTL, DPFC_RECOMP_STALL_EN |
 		   (stall_watermark << DPFC_RECOMP_STALL_WM_SHIFT) |
-- 
1.7.5.4

Re: [PATCH 2/6] drm/i915: Use of a CPU fence is mandatory to update FBC regions upon CPU writes

[Jesse Barnes]

On Thu,  7 Jul 2011 12:48:07 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> ...and this requirement is enforced by intel_update_fbc() so we can
> remove the later check from g4x_enable_fbc() and ironlake_enable_fbc().
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_display.c |   16 ++++------------
>  1 files changed, 4 insertions(+), 12 deletions(-)
> 

Yeah we probably shouldn't try removing that restriction...

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

[PATCH 3/6] drm/i915: Set persistent-mode for ILK/SNB framebuffer compression

[Chris Wilson]

Persistent mode is intended for use with front-buffer rendering, such as
X, where it is necessary to detect writes to the scanout either by the
GPU or through the CPU's fence, and recompress the dirty regions on the
fly. (By comparison to the back-buffer rendering, the scanout is always
recompressed after a page-flip.)

References: https://bugs.freedesktop.org/show_bug.cgi?id=33487
References: https://bugs.freedesktop.org/show_bug.cgi?id=31742
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_reg.h      |    1 +
 drivers/gpu/drm/i915/intel_display.c |    2 ++
 2 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 5d5def7..86a75bc 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -579,6 +579,7 @@
 #define   DPFC_CTL_PLANEA	(0<<30)
 #define   DPFC_CTL_PLANEB	(1<<30)
 #define   DPFC_CTL_FENCE_EN	(1<<29)
+#define   DPFC_CTL_PERSISTENT_MODE	(1<<25)
 #define   DPFC_SR_EN		(1<<10)
 #define   DPFC_CTL_LIMIT_1X	(0<<6)
 #define   DPFC_CTL_LIMIT_2X	(1<<6)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0cbae6c..8ff0eae 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1579,6 +1579,8 @@ static void ironlake_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
 
 	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);
 	I915_WRITE(ILK_DPFC_CHICKEN, DPFC_HT_MODIFY);
 
-- 
1.7.5.4

Re: [PATCH 3/6] drm/i915: Set persistent-mode for ILK/SNB framebuffer compression

[Jesse Barnes]

On Thu,  7 Jul 2011 12:48:08 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> Persistent mode is intended for use with front-buffer rendering, such as
> X, where it is necessary to detect writes to the scanout either by the
> GPU or through the CPU's fence, and recompress the dirty regions on the
> fly. (By comparison to the back-buffer rendering, the scanout is always
> recompressed after a page-flip.)
> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=33487
> References: https://bugs.freedesktop.org/show_bug.cgi?id=31742
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_reg.h      |    1 +
>  drivers/gpu/drm/i915/intel_display.c |    2 ++
>  2 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 5d5def7..86a75bc 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -579,6 +579,7 @@
>  #define   DPFC_CTL_PLANEA	(0<<30)
>  #define   DPFC_CTL_PLANEB	(1<<30)
>  #define   DPFC_CTL_FENCE_EN	(1<<29)
> +#define   DPFC_CTL_PERSISTENT_MODE	(1<<25)
>  #define   DPFC_SR_EN		(1<<10)
>  #define   DPFC_CTL_LIMIT_1X	(0<<6)
>  #define   DPFC_CTL_LIMIT_2X	(1<<6)
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 0cbae6c..8ff0eae 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1579,6 +1579,8 @@ static void ironlake_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
>  
>  	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);
>  	I915_WRITE(ILK_DPFC_CHICKEN, DPFC_HT_MODIFY);
>  

Yes, extra bits!  Looks like the latest specs don't restrict this bit
to being SNB only or anything, so setting it here looks good.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

[PATCH 4/6] drm/i915: Disable FBC across page-flipping

[Chris Wilson]

Page-flipping updates the scanout address, nukes the FBC compressed
image and so forces an FBC update so that the displayed image remains
consistent. However, page-flipping does not update the FBC registers
themselves, which remain pointing to both the old address and the old
CPU fence. Future updates to the new front-buffer (scanout) are then
undetected!

This first approach to demonstrate the issue and highlight the fix,
simply disables FBC upon page-flip (a recompression will be forced on
every flip so FBC becomes immaterial) and then re-enables FBC in the
page-flip finish work function, so that the FBC registers are now
pointing to the new framebuffer and front-buffer rendering works once
more.

Ideally, we want to only re-enable FBC after page-flipping is complete,
as otherwise we are just wasting cycles (along with an undesirable
wait-for-vblank which will halve the frame-rate of vsync'ed games) and
power (with needless recompression) whilst the page-flipping application
is still running.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=33487
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_display.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8ff0eae..ea085ff 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6131,6 +6131,7 @@ static void intel_unpin_work_fn(struct work_struct *__work)
 	drm_gem_object_unreference(&work->pending_flip_obj->base);
 	drm_gem_object_unreference(&work->old_fb_obj->base);
 
+	intel_update_fbc(work->dev);
 	mutex_unlock(&work->dev->struct_mutex);
 	kfree(work);
 }
@@ -6495,6 +6496,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	if (ret)
 		goto cleanup_pending;
 
+	intel_disable_fbc(dev);
 	mutex_unlock(&dev->struct_mutex);
 
 	trace_i915_flip_request(intel_crtc->plane, obj);
-- 
1.7.5.4

Re: [PATCH 4/6] drm/i915: Disable FBC across page-flipping

[Jesse Barnes]

On Thu,  7 Jul 2011 12:48:09 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> Page-flipping updates the scanout address, nukes the FBC compressed
> image and so forces an FBC update so that the displayed image remains
> consistent. However, page-flipping does not update the FBC registers
> themselves, which remain pointing to both the old address and the old
> CPU fence. Future updates to the new front-buffer (scanout) are then
> undetected!
> 
> This first approach to demonstrate the issue and highlight the fix,
> simply disables FBC upon page-flip (a recompression will be forced on
> every flip so FBC becomes immaterial) and then re-enables FBC in the
> page-flip finish work function, so that the FBC registers are now
> pointing to the new framebuffer and front-buffer rendering works once
> more.
> 
> Ideally, we want to only re-enable FBC after page-flipping is complete,
> as otherwise we are just wasting cycles (along with an undesirable
> wait-for-vblank which will halve the frame-rate of vsync'ed games) and
> power (with needless recompression) whilst the page-flipping application
> is still running.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=33487
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---

Yeah just disabling it is probably better than trying to let the hw
figure out what to recompress since it likely won't be worthwhile.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

[PATCH 5/6] drm/i915: Only export the generic intel_disable_fbc() interface

[Chris Wilson]

As the enable/disable routines will be gain additional complexity in
future patches, it is necessary that all callers do not bypass the
generic interface by calling into the chipset routines directly. to do
this we make the chipset routines static, so there is no choice.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_dma.c      |    4 ++-
 drivers/gpu/drm/i915/i915_drv.h      |    6 +---
 drivers/gpu/drm/i915/i915_suspend.c  |    4 +--
 drivers/gpu/drm/i915/intel_display.c |   50 +++++++++++++++++-----------------
 4 files changed, 30 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 2b79588..108ed3a 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1073,6 +1073,9 @@ static void i915_setup_compression(struct drm_device *dev, int size)
 	unsigned long cfb_base;
 	unsigned long ll_base = 0;
 
+	/* Just in case the BIOS is doing something questionable. */
+	intel_disable_fbc(dev);
+
 	compressed_fb = drm_mm_search_free(&dev_priv->mm.stolen, size, 4096, 0);
 	if (compressed_fb)
 		compressed_fb = drm_mm_get_block(compressed_fb, size, 4096);
@@ -1099,7 +1102,6 @@ static void i915_setup_compression(struct drm_device *dev, int size)
 
 	dev_priv->cfb_size = size;
 
-	intel_disable_fbc(dev);
 	dev_priv->compressed_fb = compressed_fb;
 	if (HAS_PCH_SPLIT(dev))
 		I915_WRITE(ILK_DPFC_CB_BASE, compressed_fb->start);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 01affb63..940279f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1316,12 +1316,8 @@ extern void intel_modeset_init(struct drm_device *dev);
 extern void intel_modeset_gem_init(struct drm_device *dev);
 extern void intel_modeset_cleanup(struct drm_device *dev);
 extern int intel_modeset_vga_set_state(struct drm_device *dev, bool state);
-extern void i8xx_disable_fbc(struct drm_device *dev);
-extern void g4x_disable_fbc(struct drm_device *dev);
-extern void ironlake_disable_fbc(struct drm_device *dev);
-extern void intel_disable_fbc(struct drm_device *dev);
-extern void intel_enable_fbc(struct drm_crtc *crtc, unsigned long interval);
 extern bool intel_fbc_enabled(struct drm_device *dev);
+extern void intel_disable_fbc(struct drm_device *dev);
 extern bool ironlake_set_drps(struct drm_device *dev, u8 val);
 extern void ironlake_enable_rc6(struct drm_device *dev);
 extern void gen6_set_rps(struct drm_device *dev, u8 val);
diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
index e8152d2..cb75db4 100644
--- a/drivers/gpu/drm/i915/i915_suspend.c
+++ b/drivers/gpu/drm/i915/i915_suspend.c
@@ -762,15 +762,13 @@ void i915_restore_display(struct drm_device *dev)
 	/* FIXME: restore TV & SDVO state */
 
 	/* only restore FBC info on the platform that supports FBC*/
+	intel_disable_fbc(dev);
 	if (I915_HAS_FBC(dev)) {
 		if (HAS_PCH_SPLIT(dev)) {
-			ironlake_disable_fbc(dev);
 			I915_WRITE(ILK_DPFC_CB_BASE, dev_priv->saveDPFC_CB_BASE);
 		} else if (IS_GM45(dev)) {
-			g4x_disable_fbc(dev);
 			I915_WRITE(DPFC_CB_BASE, dev_priv->saveDPFC_CB_BASE);
 		} else {
-			i8xx_disable_fbc(dev);
 			I915_WRITE(FBC_CFB_BASE, dev_priv->saveFBC_CFB_BASE);
 			I915_WRITE(FBC_LL_BASE, dev_priv->saveFBC_LL_BASE);
 			I915_WRITE(FBC_CONTROL2, dev_priv->saveFBC_CONTROL2);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ea085ff..f98f0cb 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1380,6 +1380,28 @@ static void intel_disable_pch_ports(struct drm_i915_private *dev_priv,
 	disable_pch_hdmi(dev_priv, pipe, HDMID);
 }
 
+static void i8xx_disable_fbc(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	u32 fbc_ctl;
+
+	/* Disable compression */
+	fbc_ctl = I915_READ(FBC_CONTROL);
+	if ((fbc_ctl & FBC_CTL_EN) == 0)
+		return;
+
+	fbc_ctl &= ~FBC_CTL_EN;
+	I915_WRITE(FBC_CONTROL, fbc_ctl);
+
+	/* Wait for compressing bit to clear */
+	if (wait_for((I915_READ(FBC_STATUS) & FBC_STAT_COMPRESSING) == 0, 10)) {
+		DRM_DEBUG_KMS("FBC idle timed out\n");
+		return;
+	}
+
+	DRM_DEBUG_KMS("disabled FBC\n");
+}
+
 static void i8xx_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
 {
 	struct drm_device *dev = crtc->dev;
@@ -1435,28 +1457,6 @@ static void i8xx_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
 		      dev_priv->cfb_pitch, crtc->y, dev_priv->cfb_plane);
 }
 
-void i8xx_disable_fbc(struct drm_device *dev)
-{
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	u32 fbc_ctl;
-
-	/* Disable compression */
-	fbc_ctl = I915_READ(FBC_CONTROL);
-	if ((fbc_ctl & FBC_CTL_EN) == 0)
-		return;
-
-	fbc_ctl &= ~FBC_CTL_EN;
-	I915_WRITE(FBC_CONTROL, fbc_ctl);
-
-	/* Wait for compressing bit to clear */
-	if (wait_for((I915_READ(FBC_STATUS) & FBC_STAT_COMPRESSING) == 0, 10)) {
-		DRM_DEBUG_KMS("FBC idle timed out\n");
-		return;
-	}
-
-	DRM_DEBUG_KMS("disabled FBC\n");
-}
-
 static bool i8xx_fbc_enabled(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -1506,7 +1506,7 @@ static void g4x_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
 	DRM_DEBUG_KMS("enabled fbc on plane %d\n", intel_crtc->plane);
 }
 
-void g4x_disable_fbc(struct drm_device *dev)
+static void g4x_disable_fbc(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u32 dpfc_ctl;
@@ -1602,7 +1602,7 @@ static void ironlake_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
 	DRM_DEBUG_KMS("enabled fbc on plane %d\n", intel_crtc->plane);
 }
 
-void ironlake_disable_fbc(struct drm_device *dev)
+static void ironlake_disable_fbc(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u32 dpfc_ctl;
@@ -1634,7 +1634,7 @@ bool intel_fbc_enabled(struct drm_device *dev)
 	return dev_priv->display.fbc_enabled(dev);
 }
 
-void intel_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
+static void intel_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
 {
 	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
 
-- 
1.7.5.4

Re: [PATCH 5/6] drm/i915: Only export the generic intel_disable_fbc() interface

[Jesse Barnes]

On Thu,  7 Jul 2011 12:48:10 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> As the enable/disable routines will be gain additional complexity in
> future patches, it is necessary that all callers do not bypass the
> generic interface by calling into the chipset routines directly. to do
> this we make the chipset routines static, so there is no choice.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_dma.c      |    4 ++-
>  drivers/gpu/drm/i915/i915_drv.h      |    6 +---
>  drivers/gpu/drm/i915/i915_suspend.c  |    4 +--
>  drivers/gpu/drm/i915/intel_display.c |   50 +++++++++++++++++-----------------
>  4 files changed, 30 insertions(+), 34 deletions(-)
> 

The code motion made me look twice, but it's a real fix.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

[PATCH] drm/i915: Replace direct calls to vfunc.disable_fbc with intel_disable_fbc()

[Chris Wilson]

...to ensure that any pending FBC enable tasklet is cancelled.

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

Considering the case for module unload, pointed out that I missed a few
cases where the chipset specific disable_fbc vfunc is used directly.
Obviously we need to make sure that any pending enable task is cancelled
or otherwise, the disable won't last.
-Chris

---
 drivers/gpu/drm/i915/intel_display.c |   13 +++++--------
 1 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1d6118f..b6de1e5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2794,9 +2794,8 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
 
 	intel_disable_plane(dev_priv, plane, pipe);
 
-	if (dev_priv->cfb_plane == plane &&
-	    dev_priv->display.disable_fbc)
-		dev_priv->display.disable_fbc(dev);
+	if (dev_priv->cfb_plane == plane)
+		intel_disable_fbc(dev);
 
 	intel_disable_pipe(dev_priv, pipe);
 
@@ -2960,9 +2959,8 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 	intel_crtc_dpms_overlay(intel_crtc, false);
 	intel_crtc_update_cursor(crtc, false);
 
-	if (dev_priv->cfb_plane == plane &&
-	    dev_priv->display.disable_fbc)
-		dev_priv->display.disable_fbc(dev);
+	if (dev_priv->cfb_plane == plane)
+		intel_disable_fbc(dev);
 
 	intel_disable_plane(dev_priv, plane, pipe);
 	intel_disable_pipe(dev_priv, pipe);
@@ -8021,8 +8019,7 @@ void intel_modeset_cleanup(struct drm_device *dev)
 		intel_increase_pllclock(crtc);
 	}
 
-	if (dev_priv->display.disable_fbc)
-		dev_priv->display.disable_fbc(dev);
+	intel_disable_fbc(dev);
 
 	if (IS_IRONLAKE_M(dev))
 		ironlake_disable_drps(dev);
-- 
1.7.5.4

[PATCH 6/6] drm/i915: Perform intel_enable_fbc() from a delayed task

[Chris Wilson]

In order to accommodate the requirements of re-enabling FBC after
page-flipping, but to avoid doing so and incurring the cost of a wait
for vblank in the middle of a page-flip sequence, we defer the actual
enablement by 50ms. If any request to disable FBC arrive within that
interval, the enablement is cancelled and we are saved from blocking on
the wait.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.h      |    2 +
 drivers/gpu/drm/i915/intel_display.c |   80 +++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_drv.h     |    7 +++
 3 files changed, 87 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 940279f..2b6b3da 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -264,6 +264,7 @@ enum intel_pch {
 #define QUIRK_PIPEA_FORCE (1<<0)
 
 struct intel_fbdev;
+struct intel_fbc_work;
 
 typedef struct drm_i915_private {
 	struct drm_device *dev;
@@ -333,6 +334,7 @@ typedef struct drm_i915_private {
 	int cfb_fence;
 	int cfb_plane;
 	int cfb_y;
+	struct intel_fbc_work *fbc_work;
 
 	struct intel_opregion opregion;
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f98f0cb..2565fc0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1634,20 +1634,96 @@ bool intel_fbc_enabled(struct drm_device *dev)
 	return dev_priv->display.fbc_enabled(dev);
 }
 
+static void intel_fbc_work_fn(struct work_struct *__work)
+{
+	struct intel_fbc_work *work =
+		container_of(to_delayed_work(__work),
+			     struct intel_fbc_work, work);
+	struct drm_device *dev = work->crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	mutex_lock(&dev->struct_mutex);
+	if (work == dev_priv->fbc_work) {
+		/* Double check that we haven't switched fb without cancelling
+		 * the prior work.
+		 */
+		if (work->crtc->fb == work->fb)
+			dev_priv->display.enable_fbc(work->crtc,
+						     work->interval);
+
+		dev_priv->fbc_work = NULL;
+	}
+	mutex_unlock(&dev->struct_mutex);
+
+	kfree(work);
+}
+
+static void intel_cancel_fbc_work(struct drm_i915_private *dev_priv)
+{
+	if (dev_priv->fbc_work == NULL)
+		return;
+
+	DRM_DEBUG_KMS("cancelling pending FBC enable\n");
+
+	/* Synchronsiation is provided by struct_mutex and checking of
+	 * dev_priv->fbc_work, so we can perform the cancellation
+	 * entirely asynchronously.
+	 */
+	if (cancel_delayed_work(&dev_priv->fbc_work->work))
+		/* tasklet was killed before being run, clean up */
+		kfree(dev_priv->fbc_work);
+
+	/* Mark the work as no longer wanted so that if it does
+	 * wake-up (because the work was already running and waiting
+	 * for our mutex), it will discover that is no longer
+	 * necessary to run.
+	 */
+	dev_priv->fbc_work = NULL;
+}
+
 static void intel_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
 {
-	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
+	struct intel_fbc_work *work;
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 
 	if (!dev_priv->display.enable_fbc)
 		return;
 
-	dev_priv->display.enable_fbc(crtc, interval);
+	intel_cancel_fbc_work(dev_priv);
+
+	work = kzalloc(sizeof *work, GFP_KERNEL);
+	if (work == NULL) {
+		dev_priv->display.enable_fbc(crtc, interval);
+		return;
+	}
+
+	work->crtc = crtc;
+	work->fb = crtc->fb;
+	work->interval = interval;
+	INIT_DELAYED_WORK(&work->work, intel_fbc_work_fn);
+
+	dev_priv->fbc_work = work;
+
+	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.
+	 *
+	 * A more complicated solution would involve tracking vblanks
+	 * following the termination of the page-flipping sequence
+	 * and indeed performing the enable as a co-routine and not
+	 * waiting synchronously upon the vblank.
+	 */
+	schedule_delayed_work(&work->work, msecs_to_jiffies(50));
 }
 
 void intel_disable_fbc(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	intel_cancel_fbc_work(dev_priv);
+
 	if (!dev_priv->display.disable_fbc)
 		return;
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 9ffa61e..bd72f65 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -233,6 +233,13 @@ struct intel_unpin_work {
 	bool enable_stall_check;
 };
 
+struct intel_fbc_work {
+	struct delayed_work work;
+	struct drm_crtc *crtc;
+	struct drm_framebuffer *fb;
+	int interval;
+};
+
 int intel_ddc_get_modes(struct drm_connector *c, struct i2c_adapter *adapter);
 extern bool intel_ddc_probe(struct intel_encoder *intel_encoder, int ddc_bus);
 
-- 
1.7.5.4

Re: [PATCH 6/6] drm/i915: Perform intel_enable_fbc() from a delayed task

[Jesse Barnes]

On Thu,  7 Jul 2011 12:48:11 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> In order to accommodate the requirements of re-enabling FBC after
> page-flipping, but to avoid doing so and incurring the cost of a wait
> for vblank in the middle of a page-flip sequence, we defer the actual
> enablement by 50ms. If any request to disable FBC arrive within that
> interval, the enablement is cancelled and we are saved from blocking on
> the wait.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---

Assuming we can't get away with what Keith suggested, this looks like a
good change.

Did you test module unload/reload as well to make sure the work
teardown is working correctly?

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

[PATCH] drm/i915: Flush any scheduled tasks during unload

[Chris Wilson]

As we use the global workqueue for delayed tasks, such as enabling FBC,
we need to be sure that they have finished executing before we unload
the module and remove their code pages.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
Right, as Jesse hinted, since we add a task to the global workqueue and
only asynchronously cancel it, we need to be sure that it has indeed
finished executing before we unload the module.

This is a delta patch that can be applied on top of the delayed FBC
enable path.
---
 drivers/gpu/drm/i915/intel_display.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7395e8a..cfa9b61 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8038,6 +8038,9 @@ void intel_modeset_cleanup(struct drm_device *dev)
 	drm_irq_uninstall(dev);
 	cancel_work_sync(&dev_priv->hotplug_work);
 
+	/* flush any delayed tasks or pending work */
+	flush_scheduled_work();
+
 	/* Shut off idle work before the crtcs get freed. */
 	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
 		intel_crtc = to_intel_crtc(crtc);
-- 
1.7.5.4

Re: FBC fixes for review and testing

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 1063 bytes --]

On Thu,  7 Jul 2011 12:48:05 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> FBC is currently disabled upstream due a few conflicting requirements
> and questionable benefit. It is also buggy...
> 
> https://bugs.freedesktop.org/show_bug.cgi?id=33487 is the current open
> SNB bug where FBC prevents the screen from being redrawn under a
> compositing WM. At last it appears that we have found the cause and a
> solution, so please do give these patches some testing - you will need
> to pass i915.i915_enable_fbc=1 to override the forced disabling of
> FBC.

This all looks good to me. I've one question though -- enable_fbc needs
to wait for vblank only to make sure it is disabled before turning it
back on right? If so, it seems like we could simply disable FBC
immediately, and then schedule the enable at the next vblank interval
(or even several vblank intervals in the future). Seems like this would
effectively eliminate waiting with the struct_mutex held, which seems
like a nice feature to me.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

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>
---

So the most important outcome of this is that my concerns over the impact
of the simple fix are groundless. The only advantage that the delayed
enabling of FBC gives us is to prevent us from attempting to re-enable
FBC in the middle of a video/gam or immediately after a mode-change when
the screen is likely to be redrawn frequently as the desktop is jiggled
into its new arrangement.
-Chris

---
 drivers/gpu/drm/i915/i915_drv.h      |    6 +-
 drivers/gpu/drm/i915/intel_display.c |  115 +++++++++++++++++-----------------
 2 files changed, 59 insertions(+), 62 deletions(-)

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..aafaec9 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);
@@ -1728,6 +1690,7 @@ void intel_disable_fbc(struct drm_device *dev)
 		return;
 
 	dev_priv->display.disable_fbc(dev);
+	dev_priv->cfb_plane = -1;
 }
 
 /**
@@ -1836,6 +1799,42 @@ 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.
+		 *
+		 * 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...
+		 */
+		DRM_DEBUG_KMS("disabling active FBC for update\n");
+		intel_disable_fbc(dev);
+		intel_wait_for_vblank(dev, intel_crtc->pipe);
+	}
+
 	intel_enable_fbc(crtc, 500);
 	return;
 
-- 
1.7.5.4

Re: [PATCH] drm/i915: Share the common work of disabling active FBC before updating

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 1692 bytes --]

On Thu,  7 Jul 2011 21:30:19 +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.

yeah, looks good.

> +	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.
> +		 *
> +		 * 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...
> +		 */
> +		DRM_DEBUG_KMS("disabling active FBC for update\n");
> +		intel_disable_fbc(dev);
> +		intel_wait_for_vblank(dev, intel_crtc->pipe);
> +	}
> +
>  	intel_enable_fbc(crtc, 500);

Should this path queue a worker function to re-enable FBC after 'a
while'? Is there any particular reason it needs to be done synchronously
here? That would avoid a 'wait_for_vblank' call with the mutex held.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

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(-)

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;
 
-- 
1.7.5.4

Re: [PATCH] drm/i915: Share the common work of disabling active FBC before updating

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 1037 bytes --]

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>

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx