| Message ID | 1309549723-13144-1-git-send-email-ben@bwidawsk.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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
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
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); }
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(-)