[PATCH] drm/i915: set persistent mode for fbc

[PATCH] drm/i915: set persistent mode for fbc

[Ben Widawsky]

This seems to fix my bugs with sna enabled.

We should collect some power numbers, and validate it works on ILK
before upstreaming. (And read more about what it actually does).

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 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 804ac4d..4b94d71 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1607,6 +1607,8 @@ static void ironlake_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
 		I915_WRITE(SNB_DPFC_CTL_SA,
 			   SNB_CPU_FENCE_ENABLE | dev_priv->cfb_fence);
 		I915_WRITE(DPFC_CPU_FENCE_OFFSET, crtc->y);
+		/* Set persistent mode */
+		I915_WRITE(ILK_DPFC_CONTROL, 1 << 25);
 		sandybridge_blit_fbc_update(dev);
 	}
 
-- 
1.7.6

Re: [PATCH] drm/i915: set persistent mode for fbc

[Chris Wilson]

On Fri,  1 Jul 2011 12:48:43 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> This seems to fix my bugs with sna enabled.
> 
> We should collect some power numbers, and validate it works on ILK
> before upstreaming. (And read more about what it actually does).

The hints that I've read indicate that persistent mode is to be used with
front-buffer rendering, ala X. Page-flipping always causes a
recompression, so I'm not sure what the design for the non-persistent mode
is for, but maybe it helps save power in the page-flipping case. Inferring
further, I suspect it is whether the SA monitors the usage of the fence
associated with the scanout for direct modifications by the CPU.

Intrigued to know what the power numbers are, but it appears that we
cannot live without this bit set on SNB. So it is a case of whether
enabling FBC is a power-conservation for us at all... Unless we clear that
bit upon a page-flip and then reset it after the following vblank (with no
intervening flips).

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

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: set persistent mode for fbc

[Chris Wilson]

I think we can make the patch and resulting code a bit more
comprehensible...

On Fri,  1 Jul 2011 12:48:43 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> This seems to fix my bugs with sna enabled.
> 
> We should collect some power numbers, and validate it works on ILK
> before upstreaming. (And read more about what it actually does).
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  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 804ac4d..4b94d71 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1607,6 +1607,8 @@ static void ironlake_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
>  		I915_WRITE(SNB_DPFC_CTL_SA,
>  			   SNB_CPU_FENCE_ENABLE | dev_priv->cfb_fence);
>  		I915_WRITE(DPFC_CPU_FENCE_OFFSET, crtc->y);
> +		/* Set persistent mode */
> +		I915_WRITE(ILK_DPFC_CONTROL, 1 << 25);
/* Set persistent mode for front-buffer rendering and to detect direct
 * writes through the CPU */
I915_WRITE(ILK_DPFC_CONTROL,
           I915_READ(ILK_DPFC_CONTROL) | DPFC_CTL_PERSISTENT_MODE);
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: set persistent mode for fbc

[Ben Widawsky]

On Sun, Jul 03, 2011 at 12:01:47PM +0100, Chris Wilson wrote:
> I think we can make the patch and resulting code a bit more
> comprehensible...
> 
> On Fri,  1 Jul 2011 12:48:43 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> > This seems to fix my bugs with sna enabled.
> > 
> > We should collect some power numbers, and validate it works on ILK
> > before upstreaming. (And read more about what it actually does).
> > 
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > ---
> >  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 804ac4d..4b94d71 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -1607,6 +1607,8 @@ static void ironlake_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
> >  		I915_WRITE(SNB_DPFC_CTL_SA,
> >  			   SNB_CPU_FENCE_ENABLE | dev_priv->cfb_fence);
> >  		I915_WRITE(DPFC_CPU_FENCE_OFFSET, crtc->y);
> > +		/* Set persistent mode */
> > +		I915_WRITE(ILK_DPFC_CONTROL, 1 << 25);
> /* Set persistent mode for front-buffer rendering and to detect direct
>  * writes through the CPU */
> I915_WRITE(ILK_DPFC_CONTROL,
>            I915_READ(ILK_DPFC_CONTROL) | DPFC_CTL_PERSISTENT_MODE);
> -Chris

Looks good to me. I'd like to get some power numbers from Jesse though
before going for -fixes.

Would you care to resend your patch with your description from the
earlier mail as the commit message?

Ben

[PATCH] drm/i915: Set persistent-mode for 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.)

This fixes the missing rendering oft triggered in the past, but easily
reproduced by using mutter with sna, for which the current workaround is
to disable fbc entirely.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=33487
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 |   11 +++++++++--
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 5d5def7..22f50da 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   SNB_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 ec05497..a32a894 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1601,16 +1601,23 @@ static void ironlake_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
 		   (interval << DPFC_RECOMP_TIMER_COUNT_SHIFT));
 	I915_WRITE(ILK_DPFC_FENCE_YOFF, crtc->y);
 	I915_WRITE(ILK_FBC_RT_BASE, obj->gtt_offset | ILK_FBC_RT_VALID);
-	/* enable it... */
-	I915_WRITE(ILK_DPFC_CONTROL, dpfc_ctl | DPFC_CTL_EN);
 
 	if (IS_GEN6(dev)) {
 		I915_WRITE(SNB_DPFC_CTL_SA,
 			   SNB_CPU_FENCE_ENABLE | dev_priv->cfb_fence);
 		I915_WRITE(DPFC_CPU_FENCE_OFFSET, crtc->y);
+
+		/* Set persistent mode for front-buffer rendering and to
+		 * detect direct writes through the CPU.
+		  */
+		dpfc_ctl |= SNB_DPFC_CTL_PERSISTENT_MODE;
+
 		sandybridge_blit_fbc_update(dev);
 	}
 
+	/* and finally enable it... */
+	I915_WRITE(ILK_DPFC_CONTROL, dpfc_ctl | DPFC_CTL_EN);
+
 	DRM_DEBUG_KMS("enabled fbc on plane %d\n", intel_crtc->plane);
 }
 
-- 
1.7.5.4

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

[Chris Wilson]

On Mon,  4 Jul 2011 13:35:57 +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.)
> 
> This fixes the missing rendering oft triggered in the past, but easily
> reproduced by using mutter with sna, for which the current workaround is
> to disable fbc entirely.

Hah. Spoke too soon, actually testing it, it seems the fix was due to the
little buglet in the first patch which had the effect of disabling FBC.

Oh well, it was a nice idea.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Keith Packard]

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

On Mon,  4 Jul 2011 13:35:57 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> This fixes the missing rendering oft triggered in the past, but easily
> reproduced by using mutter with sna, for which the current workaround is
> to disable fbc entirely.

This looks good; I'd like to see a few other people on the bug chime in
with test results so we can mark this one as fixed as the patch gets
merged into the kernel for 3.0

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

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

[Ben Widawsky]

On Mon, Jul 04, 2011 at 07:18:11PM +0100, Chris Wilson wrote:
> On Mon,  4 Jul 2011 13:35:57 +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.)
> > 
> > This fixes the missing rendering oft triggered in the past, but easily
> > reproduced by using mutter with sna, for which the current workaround is
> > to disable fbc entirely.
> 
> Hah. Spoke too soon, actually testing it, it seems the fix was due to the
> little buglet in the first patch which had the effect of disabling FBC.
> 
> Oh well, it was a nice idea.
> -Chris

I was looking at the patch you submitted and it made me realize that
surely something must have been missing from my original patch. I'll
play around with fbc a bit more to see if I can find some other magic
bit.

Ben