| Message ID | 1382633954-7375-2-git-send-email-benjamin.widawsky@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2013/10/24 Ben Widawsky <benjamin.widawsky@intel.com>: > Production HSW does not need it. I confirmed this with Art. > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> I just hope these things don't start uncovering bugs :) Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > drivers/gpu/drm/i915/i915_reg.h | 3 --- > drivers/gpu/drm/i915/intel_pm.c | 10 ---------- > 2 files changed, 13 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 6c98238..6799d53 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -1110,9 +1110,6 @@ > _HSW_PIPE_SLICE_CHICKEN_1_A, + \ > _HSW_PIPE_SLICE_CHICKEN_1_B) > > -#define HSW_CLKGATE_DISABLE_PART_1 0x46500 > -#define HSW_DPFC_GATING_DISABLE (1<<23) > - > /* > * GPIO regs > */ > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 33ad028..686699c 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -254,12 +254,6 @@ static void ironlake_disable_fbc(struct drm_device *dev) > dpfc_ctl &= ~DPFC_CTL_EN; > I915_WRITE(ILK_DPFC_CONTROL, dpfc_ctl); > > - if (IS_HASWELL(dev)) > - /* WaFbcDisableDpfcClockGating:hsw */ > - I915_WRITE(HSW_CLKGATE_DISABLE_PART_1, > - I915_READ(HSW_CLKGATE_DISABLE_PART_1) & > - ~HSW_DPFC_GATING_DISABLE); > - > DRM_DEBUG_KMS("disabled FBC\n"); > } > } > @@ -293,10 +287,6 @@ static void gen7_enable_fbc(struct drm_crtc *crtc, unsigned long interval) > /* WaFbcAsynchFlipDisableFbcQueue:hsw */ > I915_WRITE(HSW_PIPE_SLICE_CHICKEN_1(intel_crtc->pipe), > HSW_BYPASS_FBC_QUEUE); > - /* WaFbcDisableDpfcClockGating:hsw */ > - I915_WRITE(HSW_CLKGATE_DISABLE_PART_1, > - I915_READ(HSW_CLKGATE_DISABLE_PART_1) | > - HSW_DPFC_GATING_DISABLE); > } > > I915_WRITE(SNB_DPFC_CTL_SA, > -- > 1.8.4.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Oct 25, 2013 at 03:27:50PM -0200, Paulo Zanoni wrote: > 2013/10/24 Ben Widawsky <benjamin.widawsky@intel.com>: > > Production HSW does not need it. I confirmed this with Art. > > > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > > I just hope these things don't start uncovering bugs :) > > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> Merged the first 2 patches of this series. Not sure what to do with the other two, since fbc is essentially disabled on pre-hsw. And no one seems to really work on it :( So I only see minimal reasons to frob with it ... -Daniel
2013/10/27 Daniel Vetter <daniel@ffwll.ch>: > On Fri, Oct 25, 2013 at 03:27:50PM -0200, Paulo Zanoni wrote: >> 2013/10/24 Ben Widawsky <benjamin.widawsky@intel.com>: >> > Production HSW does not need it. I confirmed this with Art. >> > >> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> >> >> I just hope these things don't start uncovering bugs :) >> >> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Merged the first 2 patches of this series. Not sure what to do with the > other two, since fbc is essentially disabled on pre-hsw. And no one seems > to really work on it :( So I only see minimal reasons to frob with it ... IMHO what you said is another reason to actually merge the other two patches, since they make FBC-only WAs be applied only on FBC (e.g., probably never). > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Mon, Oct 28, 2013 at 10:22:31AM -0200, Paulo Zanoni wrote: > 2013/10/27 Daniel Vetter <daniel@ffwll.ch>: > > On Fri, Oct 25, 2013 at 03:27:50PM -0200, Paulo Zanoni wrote: > >> 2013/10/24 Ben Widawsky <benjamin.widawsky@intel.com>: > >> > Production HSW does not need it. I confirmed this with Art. > >> > > >> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > >> > >> I just hope these things don't start uncovering bugs :) > >> > >> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > Merged the first 2 patches of this series. Not sure what to do with the > > other two, since fbc is essentially disabled on pre-hsw. And no one seems > > to really work on it :( So I only see minimal reasons to frob with it ... > > IMHO what you said is another reason to actually merge the other two > patches, since they make FBC-only WAs be applied only on FBC (e.g., > probably never). Another reason would be keeping the codepaths at least somewhat similar. Could make it a bit easier to fix things later. If it would be me who gets to fix the FBC mess at some point, I'd try to fix it for all gens for sure. At some point I posted a patch to attempt a quick FBC fix for SNB: "[PATCH] drm/i915: Attempt to fix FBC render tracking with hardware contexts" In theory that could make FBC work equally well for SNB as it works for IVB+. And I must confess that I have FBC enabled on my IVB ultrabook currently since it appears to save a rather significant amount of power.
On Mon, Oct 28, 2013 at 10:22:31AM -0200, Paulo Zanoni wrote: > 2013/10/27 Daniel Vetter <daniel@ffwll.ch>: > > On Fri, Oct 25, 2013 at 03:27:50PM -0200, Paulo Zanoni wrote: > >> 2013/10/24 Ben Widawsky <benjamin.widawsky@intel.com>: > >> > Production HSW does not need it. I confirmed this with Art. > >> > > >> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > >> > >> I just hope these things don't start uncovering bugs :) > >> > >> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > Merged the first 2 patches of this series. Not sure what to do with the > > other two, since fbc is essentially disabled on pre-hsw. And no one seems > > to really work on it :( So I only see minimal reasons to frob with it ... > > IMHO what you said is another reason to actually merge the other two > patches, since they make FBC-only WAs be applied only on FBC (e.g., > probably never). Oh, that was just a comment on top since the last 2 patches in this series are blocked on review comments anyway. I'll pull them in once that's settled. But maybe it spurs someone to work a bit on fbc ... -Daniel
On Mon, Oct 28, 2013 at 03:05:12PM +0200, Ville Syrjälä wrote: > On Mon, Oct 28, 2013 at 10:22:31AM -0200, Paulo Zanoni wrote: > > 2013/10/27 Daniel Vetter <daniel@ffwll.ch>: > > > On Fri, Oct 25, 2013 at 03:27:50PM -0200, Paulo Zanoni wrote: > > >> 2013/10/24 Ben Widawsky <benjamin.widawsky@intel.com>: > > >> > Production HSW does not need it. I confirmed this with Art. > > >> > > > >> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > > >> > > >> I just hope these things don't start uncovering bugs :) > > >> > > >> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > > > Merged the first 2 patches of this series. Not sure what to do with the > > > other two, since fbc is essentially disabled on pre-hsw. And no one seems > > > to really work on it :( So I only see minimal reasons to frob with it ... > > > > IMHO what you said is another reason to actually merge the other two > > patches, since they make FBC-only WAs be applied only on FBC (e.g., > > probably never). > > Another reason would be keeping the codepaths at least somewhat similar. > Could make it a bit easier to fix things later. If it would be me who > gets to fix the FBC mess at some point, I'd try to fix it for all gens > for sure. > > At some point I posted a patch to attempt a quick FBC fix for SNB: > "[PATCH] drm/i915: Attempt to fix FBC render tracking with hardware contexts" > > In theory that could make FBC work equally well for SNB as it works for > IVB+. And I must confess that I have FBC enabled on my IVB ultrabook > currently since it appears to save a rather significant amount of power. > > -- > Ville Syrjälä > Intel OTC I just looked at your patch, and I should probably comment there, but it's 5 months old :D. Did you actually observe a fix of something with that patch? I feel like the way in which we enable/disable tracking, it shouldn't make a difference. FWIW: I have two other patches I didn't post yet for SNB. Unfortunately, I only know one person with failing fbc on SNB, and given his description, I am not certain it will fix anything. http://cgit.freedesktop.org/~bwidawsk/drm-intel/log/?h=fbc_fixes
On Mon, Oct 28, 2013 at 09:48:55AM -0700, Ben Widawsky wrote: > On Mon, Oct 28, 2013 at 03:05:12PM +0200, Ville Syrjälä wrote: > > On Mon, Oct 28, 2013 at 10:22:31AM -0200, Paulo Zanoni wrote: > > > 2013/10/27 Daniel Vetter <daniel@ffwll.ch>: > > > > On Fri, Oct 25, 2013 at 03:27:50PM -0200, Paulo Zanoni wrote: > > > >> 2013/10/24 Ben Widawsky <benjamin.widawsky@intel.com>: > > > >> > Production HSW does not need it. I confirmed this with Art. > > > >> > > > > >> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > > > >> > > > >> I just hope these things don't start uncovering bugs :) > > > >> > > > >> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > > > > > Merged the first 2 patches of this series. Not sure what to do with the > > > > other two, since fbc is essentially disabled on pre-hsw. And no one seems > > > > to really work on it :( So I only see minimal reasons to frob with it ... > > > > > > IMHO what you said is another reason to actually merge the other two > > > patches, since they make FBC-only WAs be applied only on FBC (e.g., > > > probably never). > > > > Another reason would be keeping the codepaths at least somewhat similar. > > Could make it a bit easier to fix things later. If it would be me who > > gets to fix the FBC mess at some point, I'd try to fix it for all gens > > for sure. > > > > At some point I posted a patch to attempt a quick FBC fix for SNB: > > "[PATCH] drm/i915: Attempt to fix FBC render tracking with hardware contexts" > > > > In theory that could make FBC work equally well for SNB as it works for > > IVB+. And I must confess that I have FBC enabled on my IVB ultrabook > > currently since it appears to save a rather significant amount of power. > > > > -- > > Ville Syrjälä > > Intel OTC > > > I just looked at your patch, and I should probably comment there, but > it's 5 months old :D. Did you actually observe a fix of something with > that patch? I feel like the way in which we enable/disable tracking, it > shouldn't make a difference. I've never even tried to enable FBC on SNB ;) But it should easy to trick it into doing the wrong thing. 1) switch to context A 2) page flip to buf 0 -> FBC RT address will point to buf 0 3) switch to context B 4) page flip to buf 1 -> FBC RT address will point to buf 1 5) switch to context A -> FBC RT addres will be restored to point to buf 0 6) render into buf 1 and observe that FBC doesn't invalidate the compressed data
On Mon, Oct 28, 2013 at 07:43:30PM +0200, Ville Syrjälä wrote: > On Mon, Oct 28, 2013 at 09:48:55AM -0700, Ben Widawsky wrote: > > On Mon, Oct 28, 2013 at 03:05:12PM +0200, Ville Syrjälä wrote: > > > On Mon, Oct 28, 2013 at 10:22:31AM -0200, Paulo Zanoni wrote: > > > > 2013/10/27 Daniel Vetter <daniel@ffwll.ch>: > > > > > On Fri, Oct 25, 2013 at 03:27:50PM -0200, Paulo Zanoni wrote: > > > > >> 2013/10/24 Ben Widawsky <benjamin.widawsky@intel.com>: > > > > >> > Production HSW does not need it. I confirmed this with Art. > > > > >> > > > > > >> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > > > > >> > > > > >> I just hope these things don't start uncovering bugs :) > > > > >> > > > > >> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > > > > > > > Merged the first 2 patches of this series. Not sure what to do with the > > > > > other two, since fbc is essentially disabled on pre-hsw. And no one seems > > > > > to really work on it :( So I only see minimal reasons to frob with it ... > > > > > > > > IMHO what you said is another reason to actually merge the other two > > > > patches, since they make FBC-only WAs be applied only on FBC (e.g., > > > > probably never). > > > > > > Another reason would be keeping the codepaths at least somewhat similar. > > > Could make it a bit easier to fix things later. If it would be me who > > > gets to fix the FBC mess at some point, I'd try to fix it for all gens > > > for sure. > > > > > > At some point I posted a patch to attempt a quick FBC fix for SNB: > > > "[PATCH] drm/i915: Attempt to fix FBC render tracking with hardware contexts" > > > > > > In theory that could make FBC work equally well for SNB as it works for > > > IVB+. And I must confess that I have FBC enabled on my IVB ultrabook > > > currently since it appears to save a rather significant amount of power. > > > > > > -- > > > Ville Syrjälä > > > Intel OTC > > > > > > I just looked at your patch, and I should probably comment there, but > > it's 5 months old :D. Did you actually observe a fix of something with > > that patch? I feel like the way in which we enable/disable tracking, it > > shouldn't make a difference. > > I've never even tried to enable FBC on SNB ;) > > But it should easy to trick it into doing the wrong thing. > > 1) switch to context A > 2) page flip to buf 0 > -> FBC RT address will point to buf 0 > 3) switch to context B > 4) page flip to buf 1 > -> FBC RT address will point to buf 1 > 5) switch to context A > -> FBC RT addres will be restored to point to buf 0 > 6) render into buf 1 and observe that FBC doesn't invalidate > the compressed data > > -- > Ville Syrjälä > Intel OTC I thought this is fine because we set the enable bit concurrently with the base address, and disable when not being used. I really don't know the code well enough though.
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 6c98238..6799d53 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -1110,9 +1110,6 @@ _HSW_PIPE_SLICE_CHICKEN_1_A, + \ _HSW_PIPE_SLICE_CHICKEN_1_B) -#define HSW_CLKGATE_DISABLE_PART_1 0x46500 -#define HSW_DPFC_GATING_DISABLE (1<<23) - /* * GPIO regs */ diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 33ad028..686699c 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -254,12 +254,6 @@ static void ironlake_disable_fbc(struct drm_device *dev) dpfc_ctl &= ~DPFC_CTL_EN; I915_WRITE(ILK_DPFC_CONTROL, dpfc_ctl); - if (IS_HASWELL(dev)) - /* WaFbcDisableDpfcClockGating:hsw */ - I915_WRITE(HSW_CLKGATE_DISABLE_PART_1, - I915_READ(HSW_CLKGATE_DISABLE_PART_1) & - ~HSW_DPFC_GATING_DISABLE); - DRM_DEBUG_KMS("disabled FBC\n"); } } @@ -293,10 +287,6 @@ static void gen7_enable_fbc(struct drm_crtc *crtc, unsigned long interval) /* WaFbcAsynchFlipDisableFbcQueue:hsw */ I915_WRITE(HSW_PIPE_SLICE_CHICKEN_1(intel_crtc->pipe), HSW_BYPASS_FBC_QUEUE); - /* WaFbcDisableDpfcClockGating:hsw */ - I915_WRITE(HSW_CLKGATE_DISABLE_PART_1, - I915_READ(HSW_CLKGATE_DISABLE_PART_1) | - HSW_DPFC_GATING_DISABLE); } I915_WRITE(SNB_DPFC_CTL_SA,
Production HSW does not need it. I confirmed this with Art. Signed-off-by: Ben Widawsky <ben@bwidawsk.net> --- drivers/gpu/drm/i915/i915_reg.h | 3 --- drivers/gpu/drm/i915/intel_pm.c | 10 ---------- 2 files changed, 13 deletions(-)