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