Hi Ville, thanks for the careful review.
You are right on most cases, but on of you suggestions made fbc stopping
work here, after a while trying to figure out what it was and testing case
by case I found out we must continue using ILK_FBC_RT_VALID or it doesn't
work on IVB neither on HSW. Maybe because we don't implement that other LRI
WA. So a necessary evil for now.
It saves less power with front target bit set:
// This works:
// I915_WRITE(IVB_FBC_RT_BASE,
// obj->gtt_offset << 12); |
// ILK_FBC_RT_VALID);
// This doesn't work:
// I915_WRITE(IVB_FBC_RT_BASE, obj->gtt_offset << 12 |
SNB_FBC_FRONT_BUFFER);
// This works:
// I915_WRITE(IVB_FBC_RT_BASE, obj->gtt_offset |
// ILK_FBC_RT_VALID | SNB_FBC_FRONT_BUFFER);
// min en 20.1 / av en 20.3 / av dis 20.8
// This works better:
I915_WRITE(IVB_FBC_RT_BASE, obj->gtt_offset | ILK_FBC_RT_VALID);
// min en 19.9 / av en 20.2 / av dis 20.8
new version coming soon...
On Wed, Apr 24, 2013 at 1:47 PM, Ville Syrj?l? <
ville.syrjala at linux.intel.com> wrote:
> On Tue, Apr 23, 2013 at 05:55:04PM -0300, Rodrigo Vivi wrote:
> > This patch introduce Frame Buffer Compression (FBC) support for IVB,
> > without enabling it by default.
> > It adds a new function gen7_enable_fbc to avoid getting
> > ironlake_enable_fbc messed with many IS_IVYBRIDGE checks.
> >
> > v2: Fixes from Ville.
> > * Fix Plane. FBC is tied to primary plane A in HSW
> > * Fix DPFC initial write to avoid let trash on the register.
> > v3: Checking for bad plane on intel_update_fbc() as Chris suggested.
> > v4: Ville pointed out that according to BSpec FBC_CTL bits 0:3 must be 0.
> > v5: Up to v4 this work was entirely focused on Haswell. However Ville
> > noticed I could reuse the FBC work done for HSW and get FBC for free
> > at Ivybridge. So it makes more sense enable FBC for IVB first.
> > FBC for HSW comming on next patches. We are just not enabling it by
> > default on IVB.
> > v6: Fix confused commit name (by Matt Turner).
> >
> > Cc: Matt Turner <mattst88 at gmail.com>
> > Cc: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Ville Syrj?l? <ville.syrjala at linux.intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi at gmail.com>
> > ---
> > drivers/gpu/drm/i915/i915_drv.c | 1 +
> > drivers/gpu/drm/i915/i915_reg.h | 6 ++++++
> > drivers/gpu/drm/i915/intel_pm.c | 35 +++++++++++++++++++++++++++++++++--
> > 3 files changed, 40 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> b/drivers/gpu/drm/i915/i915_drv.c
> > index 9ebe895..a073b4c 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -280,6 +280,7 @@ static const struct intel_device_info
> intel_ivybridge_m_info = {
> > GEN7_FEATURES,
> > .is_ivybridge = 1,
> > .is_mobile = 1,
> > + .has_fbc = 1,
> > };
> >
> > static const struct intel_device_info intel_ivybridge_q_info = {
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> b/drivers/gpu/drm/i915/i915_reg.h
> > index 077d40f..f64f118 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -809,7 +809,9 @@
> > #define DPFC_CTL_EN (1<<31)
> > #define DPFC_CTL_PLANEA (0<<30)
> > #define DPFC_CTL_PLANEB (1<<30)
> > +#define IVB_DPFC_CTL_PLANE_SHIFT (29)
> > #define DPFC_CTL_FENCE_EN (1<<29)
> > +#define IVB_DPFC_CTL_FENCE_EN (1<<28)
> > #define DPFC_CTL_PERSISTENT_MODE (1<<25)
> > #define DPFC_SR_EN (1<<10)
> > #define DPFC_CTL_LIMIT_1X (0<<6)
> > @@ -857,6 +859,10 @@
> > #define SNB_CPU_FENCE_ENABLE (1<<29)
> > #define DPFC_CPU_FENCE_OFFSET 0x100104
> >
> > +/* Framebuffer compression for Ivybridge */
> > +#define IVB_FBC_RT_BASE 0x7020
> > +#define IVB_FBC_RT_BASE_ADDR_SHIFT 12
> > +
> >
> > /*
> > * GPIO regs
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> > index f747cb0..86a941a 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -253,6 +253,32 @@ static bool ironlake_fbc_enabled(struct drm_device
> *dev)
> > return I915_READ(ILK_DPFC_CONTROL) & DPFC_CTL_EN;
> > }
> >
> > +static void gen7_enable_fbc(struct drm_crtc *crtc, unsigned long
> interval)
> > +{
> > + struct drm_device *dev = crtc->dev;
> > + struct drm_i915_private *dev_priv = dev->dev_private;
> > + struct drm_framebuffer *fb = crtc->fb;
> > + 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);
> > +
> > + I915_WRITE(IVB_FBC_RT_BASE,
> > + obj->gtt_offset << IVB_FBC_RT_BASE_ADDR_SHIFT |
>
> gtt_offset is a page aligned byte offset, so the shift shouldn't be
> here.
>
> Also I'm thinking we should probably set the front buffer target bit,
> since we're not using GFDT (yet at least) and our scanout buffers aren't
> cached in LLC. I'm not sure what will happen if we leave the bit unset
> and don't issue GFDT flushes.
>
> > + ILK_FBC_RT_VALID);
>
> BTW Bspec seems to tell me that we shouldn't even enable this render
> tracking stuff, and instead we should using LRIs to nuke the FBC state
> after flushing.
>
> It's also saying something that we shouldn't write this more than once
> per context. That doesn't sound quite right. What if we flip to another
> buffer? Oh and what happens when we switch to another context? Is it
> going to overwrite this register with a stale value? Should we maybe
> reload this register with the latest value after each context switch?
> Maybe this context mess is the reason we're not supposed to use this
> register.
>
> > +
> > + I915_WRITE(ILK_DPFC_CONTROL, DPFC_CTL_EN | DPFC_CTL_LIMIT_1X |
>
> Apparently we should use 2x for 16bpp formats. BTW the spec doesn't
> say anything about 8bpp. Maybe we need to disable fbc w/ 8bpp?
>
> > + IVB_DPFC_CTL_FENCE_EN |
> > + intel_crtc->plane << IVB_DPFC_CTL_PLANE_SHIFT);
> > +
> > + I915_WRITE(SNB_DPFC_CTL_SA,
> > + SNB_CPU_FENCE_ENABLE | obj->fence_reg);
> > + I915_WRITE(DPFC_CPU_FENCE_OFFSET, crtc->y);
> > +
> > + sandybridge_blit_fbc_update(dev);
> > +
> > + DRM_DEBUG_KMS("enabled fbc on plane %d\n", intel_crtc->plane);
> > +}
> > +
> > bool intel_fbc_enabled(struct drm_device *dev)
> > {
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -439,7 +465,7 @@ void intel_update_fbc(struct drm_device *dev)
> > if (enable_fbc < 0) {
> > DRM_DEBUG_KMS("fbc set to per-chip default\n");
> > enable_fbc = 1;
> > - if (INTEL_INFO(dev)->gen <= 6)
> > + if (INTEL_INFO(dev)->gen <= 7)
> > enable_fbc = 0;
> > }
> > if (!enable_fbc) {
> > @@ -4180,7 +4206,12 @@ void intel_init_pm(struct drm_device *dev)
> > if (I915_HAS_FBC(dev)) {
> > if (HAS_PCH_SPLIT(dev)) {
> > dev_priv->display.fbc_enabled =
> ironlake_fbc_enabled;
> > - dev_priv->display.enable_fbc = ironlake_enable_fbc;
> > + if (IS_IVYBRIDGE(dev))
> > + dev_priv->display.enable_fbc =
> > + gen7_enable_fbc;
> > + else
> > + dev_priv->display.enable_fbc =
> > + ironlake_enable_fbc;
> > dev_priv->display.disable_fbc =
> ironlake_disable_fbc;
> > } else if (IS_GM45(dev)) {
> > dev_priv->display.fbc_enabled = g4x_fbc_enabled;
> > --
> > 1.8.1.4
>
> --
> Ville Syrj?l?
> Intel OTC
>
--
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/intel-gfx/attachments/20130425/67593667/attachment.html>
