[PATCH] drm/i915: Add support for FBC on Ivybridge.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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>


[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux