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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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@gmail.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@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