diff mbox

[02/18] drm/i915: don't stop+start FBC at every flip

Message ID 1445349004-16409-3-git-send-email-paulo.r.zanoni@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Zanoni, Paulo R Oct. 20, 2015, 1:49 p.m. UTC
The hardware already takes care of disabling and recompressing FBC
when we do a page flip, so all we need to do is to update the fence
registers and move on.

One of the important things to notice is that on the pre-gen6
platforms the fence is programmed on the FBC control register and the
documentation says we can't update the control register while FBC is
enabled. This would basically mean we'd have to to disable+enable FBC
at every flip in order to make sure the hardware tracking still works,
which doesn't seem to make too much sense. So I sent an email to the
hardware team requesting some clarification. The information I got was
this:

"I don't think any hardware is latching on to 0x100100, 0x100104, or
the old fence number in FBC_CTL. Instructions against changing on the
fly would be to simplify testing and ensure you don't miss any
invalidation that happened right at that time."

So I guess we're fine for flips. But I can't really say I tested FBC
on these ancient platforms - nor that I'll ever propose enabling FBC
by default on them exactly because of problems like these.

Testcase: igt/kms_frontbuffer_tracking/fbc*-fliptrack
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h          |  1 +
 drivers/gpu/drm/i915/i915_reg.h          |  3 +
 drivers/gpu/drm/i915/intel_display.c     |  1 -
 drivers/gpu/drm/i915/intel_drv.h         |  2 +
 drivers/gpu/drm/i915/intel_fbc.c         | 98 +++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_frontbuffer.c |  1 +
 6 files changed, 103 insertions(+), 3 deletions(-)

Comments

Daniel Vetter Oct. 21, 2015, 7:04 a.m. UTC | #1
On Tue, Oct 20, 2015 at 11:49:48AM -0200, Paulo Zanoni wrote:
> The hardware already takes care of disabling and recompressing FBC
> when we do a page flip, so all we need to do is to update the fence
> registers and move on.
> 
> One of the important things to notice is that on the pre-gen6
> platforms the fence is programmed on the FBC control register and the
> documentation says we can't update the control register while FBC is
> enabled. This would basically mean we'd have to to disable+enable FBC
> at every flip in order to make sure the hardware tracking still works,
> which doesn't seem to make too much sense. So I sent an email to the
> hardware team requesting some clarification. The information I got was
> this:
> 
> "I don't think any hardware is latching on to 0x100100, 0x100104, or
> the old fence number in FBC_CTL. Instructions against changing on the
> fly would be to simplify testing and ensure you don't miss any
> invalidation that happened right at that time."
> 
> So I guess we're fine for flips. But I can't really say I tested FBC
> on these ancient platforms - nor that I'll ever propose enabling FBC
> by default on them exactly because of problems like these.
> 
> Testcase: igt/kms_frontbuffer_tracking/fbc*-fliptrack
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Iirc on pre-g4x flips don't invalidate the fbc compression, and we do
actually have to disable fbc entirely. There's some funky language that if
you want to flip with fbc you have to track the new backbuffer.

So I'm not entirely sure this is correct for the i8x case. Otoh we don't
ever enable it, so we could also just nuke the i8x fbc code. Either way it
doesn't matter to much.

> ---
>  drivers/gpu/drm/i915/i915_drv.h          |  1 +
>  drivers/gpu/drm/i915/i915_reg.h          |  3 +
>  drivers/gpu/drm/i915/intel_display.c     |  1 -
>  drivers/gpu/drm/i915/intel_drv.h         |  2 +
>  drivers/gpu/drm/i915/intel_fbc.c         | 98 +++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_frontbuffer.c |  1 +
>  6 files changed, 103 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index c334525..f04f56f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -930,6 +930,7 @@ struct i915_fbc {
>  	bool (*fbc_enabled)(struct drm_i915_private *dev_priv);
>  	void (*enable_fbc)(struct intel_crtc *crtc);
>  	void (*disable_fbc)(struct drm_i915_private *dev_priv);
> +	void (*flip_prepare)(struct drm_i915_private *dev_priv);
>  };
>  
>  /**
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 9ebf032..3620b59 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -2028,6 +2028,7 @@ enum skl_disp_power_wells {
>  #define   FBC_CTL_C3_IDLE	(1<<13)
>  #define   FBC_CTL_STRIDE_SHIFT	(5)
>  #define   FBC_CTL_FENCENO_SHIFT	(0)
> +#define   FBC_CTL_FENCENO_MASK	0xF
>  #define FBC_COMMAND		0x0320c
>  #define   FBC_CMD_COMPRESS	(1<<0)
>  #define FBC_STATUS		0x03210
> @@ -2064,6 +2065,7 @@ enum skl_disp_power_wells {
>  #define   DPFC_CTL_LIMIT_1X	(0<<6)
>  #define   DPFC_CTL_LIMIT_2X	(1<<6)
>  #define   DPFC_CTL_LIMIT_4X	(2<<6)
> +#define   DPFC_CTL_FENCE_MASK	0xF
>  #define DPFC_RECOMP_CTL		0x320c
>  #define   DPFC_RECOMP_STALL_EN	(1<<27)
>  #define   DPFC_RECOMP_STALL_WM_SHIFT (16)
> @@ -2086,6 +2088,7 @@ enum skl_disp_power_wells {
>  #define   FBC_CTL_FALSE_COLOR	(1<<10)
>  /* The bit 28-8 is reserved */
>  #define   DPFC_RESERVED		(0x1FFFFF00)
> +#define   ILK_DPFC_FENCE_MASK	0xF
>  #define ILK_DPFC_RECOMP_CTL	0x4320c
>  #define ILK_DPFC_STATUS		0x43210
>  #define ILK_DPFC_FENCE_YOFF	0x43218
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 1fc1d24..e83a428 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11470,7 +11470,6 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>  			  to_intel_plane(primary)->frontbuffer_bit);
>  	mutex_unlock(&dev->struct_mutex);
>  
> -	intel_fbc_disable_crtc(intel_crtc);
>  	intel_frontbuffer_flip_prepare(dev,
>  				       to_intel_plane(primary)->frontbuffer_bit);
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index dc55e4c..1e08c8a 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1287,6 +1287,8 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
>  			  enum fb_op_origin origin);
>  void intel_fbc_flush(struct drm_i915_private *dev_priv,
>  		     unsigned int frontbuffer_bits, enum fb_op_origin origin);
> +void intel_fbc_flip_prepare(struct drm_i915_private *dev_priv,
> +			    unsigned int frontbuffer_bits);
>  void intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv);
>  
>  /* intel_hdmi.c */
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index d9d7e54..9477379 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -82,6 +82,22 @@ static void i8xx_fbc_disable(struct drm_i915_private *dev_priv)
>  	DRM_DEBUG_KMS("disabled FBC\n");
>  }
>  
> +static void i8xx_fbc_flip_prepare(struct drm_i915_private *dev_priv)
> +{
> +	struct intel_crtc *crtc = dev_priv->fbc.crtc;
> +	struct drm_framebuffer *fb = crtc->base.primary->fb;
> +	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> +	uint32_t val;
> +
> +	/* Although the documentation suggests we can't change DPFC_CONTROL
> +	 * while compression is enabled, the hardware guys said that updating
> +	 * the fence register bits during a flip is fine. */
> +	val = I915_READ(FBC_CONTROL);
> +	val &= ~FBC_CTL_FENCENO_MASK;
> +	val |= obj->fence_reg;
> +	I915_WRITE(FBC_CONTROL, val);
> +}
> +
>  static void i8xx_fbc_enable(struct intel_crtc *crtc)
>  {
>  	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> @@ -161,6 +177,22 @@ static void g4x_fbc_enable(struct intel_crtc *crtc)
>  	DRM_DEBUG_KMS("enabled fbc on plane %c\n", plane_name(crtc->plane));
>  }
>  
> +static void g4x_fbc_flip_prepare(struct drm_i915_private *dev_priv)
> +{
> +	struct intel_crtc *crtc = dev_priv->fbc.crtc;
> +	struct drm_framebuffer *fb = crtc->base.primary->fb;
> +	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> +	uint32_t val;
> +
> +	/* Although the documentation suggests we can't change DPFC_CONTROL
> +	 * while compression is enabled, the hardware guys said that updating
> +	 * the fence register bits during a flip is fine. */
> +	val = I915_READ(DPFC_CONTROL);
> +	val &= ~DPFC_CTL_FENCE_MASK;
> +	val |= obj->fence_reg;
> +	I915_WRITE(DPFC_CONTROL, val);
> +}
> +
>  static void g4x_fbc_disable(struct drm_i915_private *dev_priv)
>  {
>  	u32 dpfc_ctl;
> @@ -236,6 +268,31 @@ static void ilk_fbc_enable(struct intel_crtc *crtc)
>  	DRM_DEBUG_KMS("enabled fbc on plane %c\n", plane_name(crtc->plane));
>  }
>  
> +static void ilk_fbc_flip_prepare(struct drm_i915_private *dev_priv)
> +{
> +	struct intel_crtc *crtc = dev_priv->fbc.crtc;
> +	struct drm_framebuffer *fb = crtc->base.primary->fb;
> +	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> +	uint32_t val;
> +
> +	/* Although the documentation suggests we can't change DPFC_CONTROL
> +	 * while compression is enabled, the hardware guys said that updating
> +	 * the fence register bits during a flip is fine. */
> +	val = I915_READ(ILK_DPFC_CONTROL);
> +	val &= ~ILK_DPFC_FENCE_MASK;
> +	val |= obj->fence_reg;
> +	I915_WRITE(ILK_DPFC_CONTROL, val);
> +}
> +
> +static void snb_fbc_flip_prepare(struct drm_i915_private *dev_priv)
> +{
> +	struct intel_crtc *crtc = dev_priv->fbc.crtc;
> +	struct drm_framebuffer *fb = crtc->base.primary->fb;
> +	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> +
> +	I915_WRITE(SNB_DPFC_CTL_SA, SNB_CPU_FENCE_ENABLE | obj->fence_reg);
> +}
> +
>  static void ilk_fbc_disable(struct drm_i915_private *dev_priv)
>  {
>  	u32 dpfc_ctl;
> @@ -1020,14 +1077,44 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv,
>  
>  	if (origin == ORIGIN_GTT)
>  		return;
> +	if (origin == ORIGIN_FLIP && dev_priv->fbc.enabled)
> +		return;
>  
>  	mutex_lock(&dev_priv->fbc.lock);
>  
>  	dev_priv->fbc.busy_bits &= ~frontbuffer_bits;
>  
>  	if (!dev_priv->fbc.busy_bits) {
> -		__intel_fbc_disable(dev_priv);
> -		__intel_fbc_update(dev_priv);
> +		if (origin == ORIGIN_FLIP) {
> +			__intel_fbc_update(dev_priv);
> +		} else {
> +			__intel_fbc_disable(dev_priv);
> +			__intel_fbc_update(dev_priv);
> +		}
> +	}
> +
> +	mutex_unlock(&dev_priv->fbc.lock);
> +}
> +
> +void intel_fbc_flip_prepare(struct drm_i915_private *dev_priv,
> +			    unsigned int frontbuffer_bits)
> +{
> +	unsigned int fbc_bits;
> +
> +	if (!fbc_supported(dev_priv))
> +		return;
> +
> +	mutex_lock(&dev_priv->fbc.lock);
> +
> +	if (dev_priv->fbc.enabled) {
> +		fbc_bits = INTEL_FRONTBUFFER_PRIMARY(dev_priv->fbc.crtc->pipe);
> +		if (fbc_bits & frontbuffer_bits)
> +			dev_priv->fbc.flip_prepare(dev_priv);
> +	} else if (dev_priv->fbc.fbc_work) {
> +		fbc_bits = INTEL_FRONTBUFFER_PRIMARY(
> +				dev_priv->fbc.fbc_work->crtc->pipe);
> +		if (fbc_bits & frontbuffer_bits)
> +			__intel_fbc_disable(dev_priv);
>  	}
>  
>  	mutex_unlock(&dev_priv->fbc.lock);
> @@ -1063,18 +1150,25 @@ void intel_fbc_init(struct drm_i915_private *dev_priv)
>  		dev_priv->fbc.fbc_enabled = ilk_fbc_enabled;
>  		dev_priv->fbc.enable_fbc = gen7_fbc_enable;
>  		dev_priv->fbc.disable_fbc = ilk_fbc_disable;
> +		dev_priv->fbc.flip_prepare = snb_fbc_flip_prepare;
>  	} else if (INTEL_INFO(dev_priv)->gen >= 5) {
>  		dev_priv->fbc.fbc_enabled = ilk_fbc_enabled;
>  		dev_priv->fbc.enable_fbc = ilk_fbc_enable;
>  		dev_priv->fbc.disable_fbc = ilk_fbc_disable;
> +		if (INTEL_INFO(dev_priv)->gen == 5)
> +			dev_priv->fbc.flip_prepare = ilk_fbc_flip_prepare;
> +		else
> +			dev_priv->fbc.flip_prepare = snb_fbc_flip_prepare;
>  	} else if (IS_GM45(dev_priv)) {
>  		dev_priv->fbc.fbc_enabled = g4x_fbc_enabled;
>  		dev_priv->fbc.enable_fbc = g4x_fbc_enable;
>  		dev_priv->fbc.disable_fbc = g4x_fbc_disable;
> +		dev_priv->fbc.flip_prepare = g4x_fbc_flip_prepare;
>  	} else {
>  		dev_priv->fbc.fbc_enabled = i8xx_fbc_enabled;
>  		dev_priv->fbc.enable_fbc = i8xx_fbc_enable;
>  		dev_priv->fbc.disable_fbc = i8xx_fbc_disable;
> +		dev_priv->fbc.flip_prepare = i8xx_fbc_flip_prepare;
>  
>  		/* This value was pulled out of someone's hat */
>  		I915_WRITE(FBC_CONTROL, 500 << FBC_CTL_INTERVAL_SHIFT);
> diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
> index ac85357..31a1ad3 100644
> --- a/drivers/gpu/drm/i915/intel_frontbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
> @@ -192,6 +192,7 @@ void intel_frontbuffer_flip_prepare(struct drm_device *dev,
>  	mutex_unlock(&dev_priv->fb_tracking.lock);
>  
>  	intel_psr_single_frame_update(dev, frontbuffer_bits);
> +	intel_fbc_flip_prepare(dev_priv, frontbuffer_bits);

I haven't read ahead, but both psr single frame update and this here
should be converted. Since with this here there's the small (but very
real) chance that rendering the buffer for this flip will take longer than
the next vblank, and at least for the psr case we'd then fail to upload
the changes. For fbc it's less of a problem, since if you page flip then
userspace shouldn't race frontbuffer rendering.

So I think this is ok (but the PSR case should be fixed by moving the
register write itself into the atomic update, which means we need to have
a boolean to decided whether to do that in the intel_crtc_state).
-Daniel


>  }
>  
>  /**
> -- 
> 2.6.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Ville Syrjälä Oct. 21, 2015, 7:12 a.m. UTC | #2
On Wed, Oct 21, 2015 at 09:04:39AM +0200, Daniel Vetter wrote:
> On Tue, Oct 20, 2015 at 11:49:48AM -0200, Paulo Zanoni wrote:
> > The hardware already takes care of disabling and recompressing FBC
> > when we do a page flip, so all we need to do is to update the fence
> > registers and move on.
> > 
> > One of the important things to notice is that on the pre-gen6
> > platforms the fence is programmed on the FBC control register and the
> > documentation says we can't update the control register while FBC is
> > enabled. This would basically mean we'd have to to disable+enable FBC
> > at every flip in order to make sure the hardware tracking still works,
> > which doesn't seem to make too much sense. So I sent an email to the
> > hardware team requesting some clarification. The information I got was
> > this:
> > 
> > "I don't think any hardware is latching on to 0x100100, 0x100104, or
> > the old fence number in FBC_CTL. Instructions against changing on the
> > fly would be to simplify testing and ensure you don't miss any
> > invalidation that happened right at that time."
> > 
> > So I guess we're fine for flips. But I can't really say I tested FBC
> > on these ancient platforms - nor that I'll ever propose enabling FBC
> > by default on them exactly because of problems like these.
> > 
> > Testcase: igt/kms_frontbuffer_tracking/fbc*-fliptrack
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Iirc on pre-g4x flips don't invalidate the fbc compression, and we do
> actually have to disable fbc entirely.

Nope. At least my gen2 was doing fine with hw flip nuke >1 year ago.

> There's some funky language that if
> you want to flip with fbc you have to track the new backbuffer.
> 
> So I'm not entirely sure this is correct for the i8x case. Otoh we don't
> ever enable it, so we could also just nuke the i8x fbc code. Either way it
> doesn't matter to much.
> 
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h          |  1 +
> >  drivers/gpu/drm/i915/i915_reg.h          |  3 +
> >  drivers/gpu/drm/i915/intel_display.c     |  1 -
> >  drivers/gpu/drm/i915/intel_drv.h         |  2 +
> >  drivers/gpu/drm/i915/intel_fbc.c         | 98 +++++++++++++++++++++++++++++++-
> >  drivers/gpu/drm/i915/intel_frontbuffer.c |  1 +
> >  6 files changed, 103 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index c334525..f04f56f 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -930,6 +930,7 @@ struct i915_fbc {
> >  	bool (*fbc_enabled)(struct drm_i915_private *dev_priv);
> >  	void (*enable_fbc)(struct intel_crtc *crtc);
> >  	void (*disable_fbc)(struct drm_i915_private *dev_priv);
> > +	void (*flip_prepare)(struct drm_i915_private *dev_priv);
> >  };
> >  
> >  /**
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 9ebf032..3620b59 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -2028,6 +2028,7 @@ enum skl_disp_power_wells {
> >  #define   FBC_CTL_C3_IDLE	(1<<13)
> >  #define   FBC_CTL_STRIDE_SHIFT	(5)
> >  #define   FBC_CTL_FENCENO_SHIFT	(0)
> > +#define   FBC_CTL_FENCENO_MASK	0xF
> >  #define FBC_COMMAND		0x0320c
> >  #define   FBC_CMD_COMPRESS	(1<<0)
> >  #define FBC_STATUS		0x03210
> > @@ -2064,6 +2065,7 @@ enum skl_disp_power_wells {
> >  #define   DPFC_CTL_LIMIT_1X	(0<<6)
> >  #define   DPFC_CTL_LIMIT_2X	(1<<6)
> >  #define   DPFC_CTL_LIMIT_4X	(2<<6)
> > +#define   DPFC_CTL_FENCE_MASK	0xF
> >  #define DPFC_RECOMP_CTL		0x320c
> >  #define   DPFC_RECOMP_STALL_EN	(1<<27)
> >  #define   DPFC_RECOMP_STALL_WM_SHIFT (16)
> > @@ -2086,6 +2088,7 @@ enum skl_disp_power_wells {
> >  #define   FBC_CTL_FALSE_COLOR	(1<<10)
> >  /* The bit 28-8 is reserved */
> >  #define   DPFC_RESERVED		(0x1FFFFF00)
> > +#define   ILK_DPFC_FENCE_MASK	0xF
> >  #define ILK_DPFC_RECOMP_CTL	0x4320c
> >  #define ILK_DPFC_STATUS		0x43210
> >  #define ILK_DPFC_FENCE_YOFF	0x43218
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 1fc1d24..e83a428 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -11470,7 +11470,6 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
> >  			  to_intel_plane(primary)->frontbuffer_bit);
> >  	mutex_unlock(&dev->struct_mutex);
> >  
> > -	intel_fbc_disable_crtc(intel_crtc);
> >  	intel_frontbuffer_flip_prepare(dev,
> >  				       to_intel_plane(primary)->frontbuffer_bit);
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index dc55e4c..1e08c8a 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1287,6 +1287,8 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
> >  			  enum fb_op_origin origin);
> >  void intel_fbc_flush(struct drm_i915_private *dev_priv,
> >  		     unsigned int frontbuffer_bits, enum fb_op_origin origin);
> > +void intel_fbc_flip_prepare(struct drm_i915_private *dev_priv,
> > +			    unsigned int frontbuffer_bits);
> >  void intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv);
> >  
> >  /* intel_hdmi.c */
> > diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> > index d9d7e54..9477379 100644
> > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > @@ -82,6 +82,22 @@ static void i8xx_fbc_disable(struct drm_i915_private *dev_priv)
> >  	DRM_DEBUG_KMS("disabled FBC\n");
> >  }
> >  
> > +static void i8xx_fbc_flip_prepare(struct drm_i915_private *dev_priv)
> > +{
> > +	struct intel_crtc *crtc = dev_priv->fbc.crtc;
> > +	struct drm_framebuffer *fb = crtc->base.primary->fb;
> > +	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> > +	uint32_t val;
> > +
> > +	/* Although the documentation suggests we can't change DPFC_CONTROL
> > +	 * while compression is enabled, the hardware guys said that updating
> > +	 * the fence register bits during a flip is fine. */
> > +	val = I915_READ(FBC_CONTROL);
> > +	val &= ~FBC_CTL_FENCENO_MASK;
> > +	val |= obj->fence_reg;
> > +	I915_WRITE(FBC_CONTROL, val);
> > +}
> > +
> >  static void i8xx_fbc_enable(struct intel_crtc *crtc)
> >  {
> >  	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> > @@ -161,6 +177,22 @@ static void g4x_fbc_enable(struct intel_crtc *crtc)
> >  	DRM_DEBUG_KMS("enabled fbc on plane %c\n", plane_name(crtc->plane));
> >  }
> >  
> > +static void g4x_fbc_flip_prepare(struct drm_i915_private *dev_priv)
> > +{
> > +	struct intel_crtc *crtc = dev_priv->fbc.crtc;
> > +	struct drm_framebuffer *fb = crtc->base.primary->fb;
> > +	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> > +	uint32_t val;
> > +
> > +	/* Although the documentation suggests we can't change DPFC_CONTROL
> > +	 * while compression is enabled, the hardware guys said that updating
> > +	 * the fence register bits during a flip is fine. */
> > +	val = I915_READ(DPFC_CONTROL);
> > +	val &= ~DPFC_CTL_FENCE_MASK;
> > +	val |= obj->fence_reg;
> > +	I915_WRITE(DPFC_CONTROL, val);
> > +}
> > +
> >  static void g4x_fbc_disable(struct drm_i915_private *dev_priv)
> >  {
> >  	u32 dpfc_ctl;
> > @@ -236,6 +268,31 @@ static void ilk_fbc_enable(struct intel_crtc *crtc)
> >  	DRM_DEBUG_KMS("enabled fbc on plane %c\n", plane_name(crtc->plane));
> >  }
> >  
> > +static void ilk_fbc_flip_prepare(struct drm_i915_private *dev_priv)
> > +{
> > +	struct intel_crtc *crtc = dev_priv->fbc.crtc;
> > +	struct drm_framebuffer *fb = crtc->base.primary->fb;
> > +	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> > +	uint32_t val;
> > +
> > +	/* Although the documentation suggests we can't change DPFC_CONTROL
> > +	 * while compression is enabled, the hardware guys said that updating
> > +	 * the fence register bits during a flip is fine. */
> > +	val = I915_READ(ILK_DPFC_CONTROL);
> > +	val &= ~ILK_DPFC_FENCE_MASK;
> > +	val |= obj->fence_reg;
> > +	I915_WRITE(ILK_DPFC_CONTROL, val);
> > +}
> > +
> > +static void snb_fbc_flip_prepare(struct drm_i915_private *dev_priv)
> > +{
> > +	struct intel_crtc *crtc = dev_priv->fbc.crtc;
> > +	struct drm_framebuffer *fb = crtc->base.primary->fb;
> > +	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> > +
> > +	I915_WRITE(SNB_DPFC_CTL_SA, SNB_CPU_FENCE_ENABLE | obj->fence_reg);
> > +}
> > +
> >  static void ilk_fbc_disable(struct drm_i915_private *dev_priv)
> >  {
> >  	u32 dpfc_ctl;
> > @@ -1020,14 +1077,44 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv,
> >  
> >  	if (origin == ORIGIN_GTT)
> >  		return;
> > +	if (origin == ORIGIN_FLIP && dev_priv->fbc.enabled)
> > +		return;
> >  
> >  	mutex_lock(&dev_priv->fbc.lock);
> >  
> >  	dev_priv->fbc.busy_bits &= ~frontbuffer_bits;
> >  
> >  	if (!dev_priv->fbc.busy_bits) {
> > -		__intel_fbc_disable(dev_priv);
> > -		__intel_fbc_update(dev_priv);
> > +		if (origin == ORIGIN_FLIP) {
> > +			__intel_fbc_update(dev_priv);
> > +		} else {
> > +			__intel_fbc_disable(dev_priv);
> > +			__intel_fbc_update(dev_priv);
> > +		}
> > +	}
> > +
> > +	mutex_unlock(&dev_priv->fbc.lock);
> > +}
> > +
> > +void intel_fbc_flip_prepare(struct drm_i915_private *dev_priv,
> > +			    unsigned int frontbuffer_bits)
> > +{
> > +	unsigned int fbc_bits;
> > +
> > +	if (!fbc_supported(dev_priv))
> > +		return;
> > +
> > +	mutex_lock(&dev_priv->fbc.lock);
> > +
> > +	if (dev_priv->fbc.enabled) {
> > +		fbc_bits = INTEL_FRONTBUFFER_PRIMARY(dev_priv->fbc.crtc->pipe);
> > +		if (fbc_bits & frontbuffer_bits)
> > +			dev_priv->fbc.flip_prepare(dev_priv);
> > +	} else if (dev_priv->fbc.fbc_work) {
> > +		fbc_bits = INTEL_FRONTBUFFER_PRIMARY(
> > +				dev_priv->fbc.fbc_work->crtc->pipe);
> > +		if (fbc_bits & frontbuffer_bits)
> > +			__intel_fbc_disable(dev_priv);
> >  	}
> >  
> >  	mutex_unlock(&dev_priv->fbc.lock);
> > @@ -1063,18 +1150,25 @@ void intel_fbc_init(struct drm_i915_private *dev_priv)
> >  		dev_priv->fbc.fbc_enabled = ilk_fbc_enabled;
> >  		dev_priv->fbc.enable_fbc = gen7_fbc_enable;
> >  		dev_priv->fbc.disable_fbc = ilk_fbc_disable;
> > +		dev_priv->fbc.flip_prepare = snb_fbc_flip_prepare;
> >  	} else if (INTEL_INFO(dev_priv)->gen >= 5) {
> >  		dev_priv->fbc.fbc_enabled = ilk_fbc_enabled;
> >  		dev_priv->fbc.enable_fbc = ilk_fbc_enable;
> >  		dev_priv->fbc.disable_fbc = ilk_fbc_disable;
> > +		if (INTEL_INFO(dev_priv)->gen == 5)
> > +			dev_priv->fbc.flip_prepare = ilk_fbc_flip_prepare;
> > +		else
> > +			dev_priv->fbc.flip_prepare = snb_fbc_flip_prepare;
> >  	} else if (IS_GM45(dev_priv)) {
> >  		dev_priv->fbc.fbc_enabled = g4x_fbc_enabled;
> >  		dev_priv->fbc.enable_fbc = g4x_fbc_enable;
> >  		dev_priv->fbc.disable_fbc = g4x_fbc_disable;
> > +		dev_priv->fbc.flip_prepare = g4x_fbc_flip_prepare;
> >  	} else {
> >  		dev_priv->fbc.fbc_enabled = i8xx_fbc_enabled;
> >  		dev_priv->fbc.enable_fbc = i8xx_fbc_enable;
> >  		dev_priv->fbc.disable_fbc = i8xx_fbc_disable;
> > +		dev_priv->fbc.flip_prepare = i8xx_fbc_flip_prepare;
> >  
> >  		/* This value was pulled out of someone's hat */
> >  		I915_WRITE(FBC_CONTROL, 500 << FBC_CTL_INTERVAL_SHIFT);
> > diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
> > index ac85357..31a1ad3 100644
> > --- a/drivers/gpu/drm/i915/intel_frontbuffer.c
> > +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
> > @@ -192,6 +192,7 @@ void intel_frontbuffer_flip_prepare(struct drm_device *dev,
> >  	mutex_unlock(&dev_priv->fb_tracking.lock);
> >  
> >  	intel_psr_single_frame_update(dev, frontbuffer_bits);
> > +	intel_fbc_flip_prepare(dev_priv, frontbuffer_bits);
> 
> I haven't read ahead, but both psr single frame update and this here
> should be converted. Since with this here there's the small (but very
> real) chance that rendering the buffer for this flip will take longer than
> the next vblank, and at least for the psr case we'd then fail to upload
> the changes. For fbc it's less of a problem, since if you page flip then
> userspace shouldn't race frontbuffer rendering.
> 
> So I think this is ok (but the PSR case should be fixed by moving the
> register write itself into the atomic update, which means we need to have
> a boolean to decided whether to do that in the intel_crtc_state).
> -Daniel
> 
> 
> >  }
> >  
> >  /**
> > -- 
> > 2.6.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Ville Syrjälä Oct. 21, 2015, 7:40 a.m. UTC | #3
On Wed, Oct 21, 2015 at 10:12:50AM +0300, Ville Syrjälä wrote:
> On Wed, Oct 21, 2015 at 09:04:39AM +0200, Daniel Vetter wrote:
> > On Tue, Oct 20, 2015 at 11:49:48AM -0200, Paulo Zanoni wrote:
> > > The hardware already takes care of disabling and recompressing FBC
> > > when we do a page flip, so all we need to do is to update the fence
> > > registers and move on.
> > > 
> > > One of the important things to notice is that on the pre-gen6
> > > platforms the fence is programmed on the FBC control register and the
> > > documentation says we can't update the control register while FBC is
> > > enabled. This would basically mean we'd have to to disable+enable FBC
> > > at every flip in order to make sure the hardware tracking still works,
> > > which doesn't seem to make too much sense. So I sent an email to the
> > > hardware team requesting some clarification. The information I got was
> > > this:
> > > 
> > > "I don't think any hardware is latching on to 0x100100, 0x100104, or
> > > the old fence number in FBC_CTL. Instructions against changing on the
> > > fly would be to simplify testing and ensure you don't miss any
> > > invalidation that happened right at that time."
> > > 
> > > So I guess we're fine for flips. But I can't really say I tested FBC
> > > on these ancient platforms - nor that I'll ever propose enabling FBC
> > > by default on them exactly because of problems like these.
> > > 
> > > Testcase: igt/kms_frontbuffer_tracking/fbc*-fliptrack
> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > Iirc on pre-g4x flips don't invalidate the fbc compression, and we do
> > actually have to disable fbc entirely.
> 
> Nope. At least my gen2 was doing fine with hw flip nuke >1 year ago.

Double checked the spec, and it's contradictory.

In on place it says"
"2. Async Flips and/or panning of Display Plane A is not permitted.
 3. Sync flips of Display Plane A are permitted, given that only the
    source buffer address is changed (and note that it is UNDEFINED 
    to flip to the currently-displayed buffer). Other source display
    buffer parameters must be kept in synch with the FBC control
    parameters (which will require RLE-FBC to first be disabled)."

I have no idea what it thinks is the difference between panning and
sync flips on gen2.

In another place it says:
"All lines will be marked as modified whenever:
- uncompressed source Frame Buffer base address changes (this is only permitted to happen as a result of a
  direct register write – flips of Display Plane A are not allowed when RLE-FBC is enabled)"

But IIRC CS flips worked just fine on my 855.

> 
> > There's some funky language that if
> > you want to flip with fbc you have to track the new backbuffer.
> > 
> > So I'm not entirely sure this is correct for the i8x case. Otoh we don't
> > ever enable it, so we could also just nuke the i8x fbc code. Either way it
> > doesn't matter to much.
> > 
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.h          |  1 +
> > >  drivers/gpu/drm/i915/i915_reg.h          |  3 +
> > >  drivers/gpu/drm/i915/intel_display.c     |  1 -
> > >  drivers/gpu/drm/i915/intel_drv.h         |  2 +
> > >  drivers/gpu/drm/i915/intel_fbc.c         | 98 +++++++++++++++++++++++++++++++-
> > >  drivers/gpu/drm/i915/intel_frontbuffer.c |  1 +
> > >  6 files changed, 103 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index c334525..f04f56f 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -930,6 +930,7 @@ struct i915_fbc {
> > >  	bool (*fbc_enabled)(struct drm_i915_private *dev_priv);
> > >  	void (*enable_fbc)(struct intel_crtc *crtc);
> > >  	void (*disable_fbc)(struct drm_i915_private *dev_priv);
> > > +	void (*flip_prepare)(struct drm_i915_private *dev_priv);
> > >  };
> > >  
> > >  /**
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > index 9ebf032..3620b59 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -2028,6 +2028,7 @@ enum skl_disp_power_wells {
> > >  #define   FBC_CTL_C3_IDLE	(1<<13)
> > >  #define   FBC_CTL_STRIDE_SHIFT	(5)
> > >  #define   FBC_CTL_FENCENO_SHIFT	(0)
> > > +#define   FBC_CTL_FENCENO_MASK	0xF
> > >  #define FBC_COMMAND		0x0320c
> > >  #define   FBC_CMD_COMPRESS	(1<<0)
> > >  #define FBC_STATUS		0x03210
> > > @@ -2064,6 +2065,7 @@ enum skl_disp_power_wells {
> > >  #define   DPFC_CTL_LIMIT_1X	(0<<6)
> > >  #define   DPFC_CTL_LIMIT_2X	(1<<6)
> > >  #define   DPFC_CTL_LIMIT_4X	(2<<6)
> > > +#define   DPFC_CTL_FENCE_MASK	0xF
> > >  #define DPFC_RECOMP_CTL		0x320c
> > >  #define   DPFC_RECOMP_STALL_EN	(1<<27)
> > >  #define   DPFC_RECOMP_STALL_WM_SHIFT (16)
> > > @@ -2086,6 +2088,7 @@ enum skl_disp_power_wells {
> > >  #define   FBC_CTL_FALSE_COLOR	(1<<10)
> > >  /* The bit 28-8 is reserved */
> > >  #define   DPFC_RESERVED		(0x1FFFFF00)
> > > +#define   ILK_DPFC_FENCE_MASK	0xF
> > >  #define ILK_DPFC_RECOMP_CTL	0x4320c
> > >  #define ILK_DPFC_STATUS		0x43210
> > >  #define ILK_DPFC_FENCE_YOFF	0x43218
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 1fc1d24..e83a428 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -11470,7 +11470,6 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
> > >  			  to_intel_plane(primary)->frontbuffer_bit);
> > >  	mutex_unlock(&dev->struct_mutex);
> > >  
> > > -	intel_fbc_disable_crtc(intel_crtc);
> > >  	intel_frontbuffer_flip_prepare(dev,
> > >  				       to_intel_plane(primary)->frontbuffer_bit);
> > >  
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > index dc55e4c..1e08c8a 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -1287,6 +1287,8 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
> > >  			  enum fb_op_origin origin);
> > >  void intel_fbc_flush(struct drm_i915_private *dev_priv,
> > >  		     unsigned int frontbuffer_bits, enum fb_op_origin origin);
> > > +void intel_fbc_flip_prepare(struct drm_i915_private *dev_priv,
> > > +			    unsigned int frontbuffer_bits);
> > >  void intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv);
> > >  
> > >  /* intel_hdmi.c */
> > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> > > index d9d7e54..9477379 100644
> > > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > > @@ -82,6 +82,22 @@ static void i8xx_fbc_disable(struct drm_i915_private *dev_priv)
> > >  	DRM_DEBUG_KMS("disabled FBC\n");
> > >  }
> > >  
> > > +static void i8xx_fbc_flip_prepare(struct drm_i915_private *dev_priv)
> > > +{
> > > +	struct intel_crtc *crtc = dev_priv->fbc.crtc;
> > > +	struct drm_framebuffer *fb = crtc->base.primary->fb;
> > > +	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> > > +	uint32_t val;
> > > +
> > > +	/* Although the documentation suggests we can't change DPFC_CONTROL
> > > +	 * while compression is enabled, the hardware guys said that updating
> > > +	 * the fence register bits during a flip is fine. */
> > > +	val = I915_READ(FBC_CONTROL);
> > > +	val &= ~FBC_CTL_FENCENO_MASK;
> > > +	val |= obj->fence_reg;
> > > +	I915_WRITE(FBC_CONTROL, val);
> > > +}
> > > +
> > >  static void i8xx_fbc_enable(struct intel_crtc *crtc)
> > >  {
> > >  	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> > > @@ -161,6 +177,22 @@ static void g4x_fbc_enable(struct intel_crtc *crtc)
> > >  	DRM_DEBUG_KMS("enabled fbc on plane %c\n", plane_name(crtc->plane));
> > >  }
> > >  
> > > +static void g4x_fbc_flip_prepare(struct drm_i915_private *dev_priv)
> > > +{
> > > +	struct intel_crtc *crtc = dev_priv->fbc.crtc;
> > > +	struct drm_framebuffer *fb = crtc->base.primary->fb;
> > > +	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> > > +	uint32_t val;
> > > +
> > > +	/* Although the documentation suggests we can't change DPFC_CONTROL
> > > +	 * while compression is enabled, the hardware guys said that updating
> > > +	 * the fence register bits during a flip is fine. */
> > > +	val = I915_READ(DPFC_CONTROL);
> > > +	val &= ~DPFC_CTL_FENCE_MASK;
> > > +	val |= obj->fence_reg;
> > > +	I915_WRITE(DPFC_CONTROL, val);
> > > +}
> > > +
> > >  static void g4x_fbc_disable(struct drm_i915_private *dev_priv)
> > >  {
> > >  	u32 dpfc_ctl;
> > > @@ -236,6 +268,31 @@ static void ilk_fbc_enable(struct intel_crtc *crtc)
> > >  	DRM_DEBUG_KMS("enabled fbc on plane %c\n", plane_name(crtc->plane));
> > >  }
> > >  
> > > +static void ilk_fbc_flip_prepare(struct drm_i915_private *dev_priv)
> > > +{
> > > +	struct intel_crtc *crtc = dev_priv->fbc.crtc;
> > > +	struct drm_framebuffer *fb = crtc->base.primary->fb;
> > > +	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> > > +	uint32_t val;
> > > +
> > > +	/* Although the documentation suggests we can't change DPFC_CONTROL
> > > +	 * while compression is enabled, the hardware guys said that updating
> > > +	 * the fence register bits during a flip is fine. */
> > > +	val = I915_READ(ILK_DPFC_CONTROL);
> > > +	val &= ~ILK_DPFC_FENCE_MASK;
> > > +	val |= obj->fence_reg;
> > > +	I915_WRITE(ILK_DPFC_CONTROL, val);
> > > +}
> > > +
> > > +static void snb_fbc_flip_prepare(struct drm_i915_private *dev_priv)
> > > +{
> > > +	struct intel_crtc *crtc = dev_priv->fbc.crtc;
> > > +	struct drm_framebuffer *fb = crtc->base.primary->fb;
> > > +	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> > > +
> > > +	I915_WRITE(SNB_DPFC_CTL_SA, SNB_CPU_FENCE_ENABLE | obj->fence_reg);
> > > +}
> > > +
> > >  static void ilk_fbc_disable(struct drm_i915_private *dev_priv)
> > >  {
> > >  	u32 dpfc_ctl;
> > > @@ -1020,14 +1077,44 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv,
> > >  
> > >  	if (origin == ORIGIN_GTT)
> > >  		return;
> > > +	if (origin == ORIGIN_FLIP && dev_priv->fbc.enabled)
> > > +		return;
> > >  
> > >  	mutex_lock(&dev_priv->fbc.lock);
> > >  
> > >  	dev_priv->fbc.busy_bits &= ~frontbuffer_bits;
> > >  
> > >  	if (!dev_priv->fbc.busy_bits) {
> > > -		__intel_fbc_disable(dev_priv);
> > > -		__intel_fbc_update(dev_priv);
> > > +		if (origin == ORIGIN_FLIP) {
> > > +			__intel_fbc_update(dev_priv);
> > > +		} else {
> > > +			__intel_fbc_disable(dev_priv);
> > > +			__intel_fbc_update(dev_priv);
> > > +		}
> > > +	}
> > > +
> > > +	mutex_unlock(&dev_priv->fbc.lock);
> > > +}
> > > +
> > > +void intel_fbc_flip_prepare(struct drm_i915_private *dev_priv,
> > > +			    unsigned int frontbuffer_bits)
> > > +{
> > > +	unsigned int fbc_bits;
> > > +
> > > +	if (!fbc_supported(dev_priv))
> > > +		return;
> > > +
> > > +	mutex_lock(&dev_priv->fbc.lock);
> > > +
> > > +	if (dev_priv->fbc.enabled) {
> > > +		fbc_bits = INTEL_FRONTBUFFER_PRIMARY(dev_priv->fbc.crtc->pipe);
> > > +		if (fbc_bits & frontbuffer_bits)
> > > +			dev_priv->fbc.flip_prepare(dev_priv);
> > > +	} else if (dev_priv->fbc.fbc_work) {
> > > +		fbc_bits = INTEL_FRONTBUFFER_PRIMARY(
> > > +				dev_priv->fbc.fbc_work->crtc->pipe);
> > > +		if (fbc_bits & frontbuffer_bits)
> > > +			__intel_fbc_disable(dev_priv);
> > >  	}
> > >  
> > >  	mutex_unlock(&dev_priv->fbc.lock);
> > > @@ -1063,18 +1150,25 @@ void intel_fbc_init(struct drm_i915_private *dev_priv)
> > >  		dev_priv->fbc.fbc_enabled = ilk_fbc_enabled;
> > >  		dev_priv->fbc.enable_fbc = gen7_fbc_enable;
> > >  		dev_priv->fbc.disable_fbc = ilk_fbc_disable;
> > > +		dev_priv->fbc.flip_prepare = snb_fbc_flip_prepare;
> > >  	} else if (INTEL_INFO(dev_priv)->gen >= 5) {
> > >  		dev_priv->fbc.fbc_enabled = ilk_fbc_enabled;
> > >  		dev_priv->fbc.enable_fbc = ilk_fbc_enable;
> > >  		dev_priv->fbc.disable_fbc = ilk_fbc_disable;
> > > +		if (INTEL_INFO(dev_priv)->gen == 5)
> > > +			dev_priv->fbc.flip_prepare = ilk_fbc_flip_prepare;
> > > +		else
> > > +			dev_priv->fbc.flip_prepare = snb_fbc_flip_prepare;
> > >  	} else if (IS_GM45(dev_priv)) {
> > >  		dev_priv->fbc.fbc_enabled = g4x_fbc_enabled;
> > >  		dev_priv->fbc.enable_fbc = g4x_fbc_enable;
> > >  		dev_priv->fbc.disable_fbc = g4x_fbc_disable;
> > > +		dev_priv->fbc.flip_prepare = g4x_fbc_flip_prepare;
> > >  	} else {
> > >  		dev_priv->fbc.fbc_enabled = i8xx_fbc_enabled;
> > >  		dev_priv->fbc.enable_fbc = i8xx_fbc_enable;
> > >  		dev_priv->fbc.disable_fbc = i8xx_fbc_disable;
> > > +		dev_priv->fbc.flip_prepare = i8xx_fbc_flip_prepare;
> > >  
> > >  		/* This value was pulled out of someone's hat */
> > >  		I915_WRITE(FBC_CONTROL, 500 << FBC_CTL_INTERVAL_SHIFT);
> > > diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
> > > index ac85357..31a1ad3 100644
> > > --- a/drivers/gpu/drm/i915/intel_frontbuffer.c
> > > +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
> > > @@ -192,6 +192,7 @@ void intel_frontbuffer_flip_prepare(struct drm_device *dev,
> > >  	mutex_unlock(&dev_priv->fb_tracking.lock);
> > >  
> > >  	intel_psr_single_frame_update(dev, frontbuffer_bits);
> > > +	intel_fbc_flip_prepare(dev_priv, frontbuffer_bits);
> > 
> > I haven't read ahead, but both psr single frame update and this here
> > should be converted. Since with this here there's the small (but very
> > real) chance that rendering the buffer for this flip will take longer than
> > the next vblank, and at least for the psr case we'd then fail to upload
> > the changes. For fbc it's less of a problem, since if you page flip then
> > userspace shouldn't race frontbuffer rendering.
> > 
> > So I think this is ok (but the PSR case should be fixed by moving the
> > register write itself into the atomic update, which means we need to have
> > a boolean to decided whether to do that in the intel_crtc_state).
> > -Daniel
> > 
> > 
> > >  }
> > >  
> > >  /**
> > > -- 
> > > 2.6.1
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c334525..f04f56f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -930,6 +930,7 @@  struct i915_fbc {
 	bool (*fbc_enabled)(struct drm_i915_private *dev_priv);
 	void (*enable_fbc)(struct intel_crtc *crtc);
 	void (*disable_fbc)(struct drm_i915_private *dev_priv);
+	void (*flip_prepare)(struct drm_i915_private *dev_priv);
 };
 
 /**
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 9ebf032..3620b59 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -2028,6 +2028,7 @@  enum skl_disp_power_wells {
 #define   FBC_CTL_C3_IDLE	(1<<13)
 #define   FBC_CTL_STRIDE_SHIFT	(5)
 #define   FBC_CTL_FENCENO_SHIFT	(0)
+#define   FBC_CTL_FENCENO_MASK	0xF
 #define FBC_COMMAND		0x0320c
 #define   FBC_CMD_COMPRESS	(1<<0)
 #define FBC_STATUS		0x03210
@@ -2064,6 +2065,7 @@  enum skl_disp_power_wells {
 #define   DPFC_CTL_LIMIT_1X	(0<<6)
 #define   DPFC_CTL_LIMIT_2X	(1<<6)
 #define   DPFC_CTL_LIMIT_4X	(2<<6)
+#define   DPFC_CTL_FENCE_MASK	0xF
 #define DPFC_RECOMP_CTL		0x320c
 #define   DPFC_RECOMP_STALL_EN	(1<<27)
 #define   DPFC_RECOMP_STALL_WM_SHIFT (16)
@@ -2086,6 +2088,7 @@  enum skl_disp_power_wells {
 #define   FBC_CTL_FALSE_COLOR	(1<<10)
 /* The bit 28-8 is reserved */
 #define   DPFC_RESERVED		(0x1FFFFF00)
+#define   ILK_DPFC_FENCE_MASK	0xF
 #define ILK_DPFC_RECOMP_CTL	0x4320c
 #define ILK_DPFC_STATUS		0x43210
 #define ILK_DPFC_FENCE_YOFF	0x43218
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1fc1d24..e83a428 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11470,7 +11470,6 @@  static int intel_crtc_page_flip(struct drm_crtc *crtc,
 			  to_intel_plane(primary)->frontbuffer_bit);
 	mutex_unlock(&dev->struct_mutex);
 
-	intel_fbc_disable_crtc(intel_crtc);
 	intel_frontbuffer_flip_prepare(dev,
 				       to_intel_plane(primary)->frontbuffer_bit);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index dc55e4c..1e08c8a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1287,6 +1287,8 @@  void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
 			  enum fb_op_origin origin);
 void intel_fbc_flush(struct drm_i915_private *dev_priv,
 		     unsigned int frontbuffer_bits, enum fb_op_origin origin);
+void intel_fbc_flip_prepare(struct drm_i915_private *dev_priv,
+			    unsigned int frontbuffer_bits);
 void intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv);
 
 /* intel_hdmi.c */
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index d9d7e54..9477379 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -82,6 +82,22 @@  static void i8xx_fbc_disable(struct drm_i915_private *dev_priv)
 	DRM_DEBUG_KMS("disabled FBC\n");
 }
 
+static void i8xx_fbc_flip_prepare(struct drm_i915_private *dev_priv)
+{
+	struct intel_crtc *crtc = dev_priv->fbc.crtc;
+	struct drm_framebuffer *fb = crtc->base.primary->fb;
+	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
+	uint32_t val;
+
+	/* Although the documentation suggests we can't change DPFC_CONTROL
+	 * while compression is enabled, the hardware guys said that updating
+	 * the fence register bits during a flip is fine. */
+	val = I915_READ(FBC_CONTROL);
+	val &= ~FBC_CTL_FENCENO_MASK;
+	val |= obj->fence_reg;
+	I915_WRITE(FBC_CONTROL, val);
+}
+
 static void i8xx_fbc_enable(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
@@ -161,6 +177,22 @@  static void g4x_fbc_enable(struct intel_crtc *crtc)
 	DRM_DEBUG_KMS("enabled fbc on plane %c\n", plane_name(crtc->plane));
 }
 
+static void g4x_fbc_flip_prepare(struct drm_i915_private *dev_priv)
+{
+	struct intel_crtc *crtc = dev_priv->fbc.crtc;
+	struct drm_framebuffer *fb = crtc->base.primary->fb;
+	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
+	uint32_t val;
+
+	/* Although the documentation suggests we can't change DPFC_CONTROL
+	 * while compression is enabled, the hardware guys said that updating
+	 * the fence register bits during a flip is fine. */
+	val = I915_READ(DPFC_CONTROL);
+	val &= ~DPFC_CTL_FENCE_MASK;
+	val |= obj->fence_reg;
+	I915_WRITE(DPFC_CONTROL, val);
+}
+
 static void g4x_fbc_disable(struct drm_i915_private *dev_priv)
 {
 	u32 dpfc_ctl;
@@ -236,6 +268,31 @@  static void ilk_fbc_enable(struct intel_crtc *crtc)
 	DRM_DEBUG_KMS("enabled fbc on plane %c\n", plane_name(crtc->plane));
 }
 
+static void ilk_fbc_flip_prepare(struct drm_i915_private *dev_priv)
+{
+	struct intel_crtc *crtc = dev_priv->fbc.crtc;
+	struct drm_framebuffer *fb = crtc->base.primary->fb;
+	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
+	uint32_t val;
+
+	/* Although the documentation suggests we can't change DPFC_CONTROL
+	 * while compression is enabled, the hardware guys said that updating
+	 * the fence register bits during a flip is fine. */
+	val = I915_READ(ILK_DPFC_CONTROL);
+	val &= ~ILK_DPFC_FENCE_MASK;
+	val |= obj->fence_reg;
+	I915_WRITE(ILK_DPFC_CONTROL, val);
+}
+
+static void snb_fbc_flip_prepare(struct drm_i915_private *dev_priv)
+{
+	struct intel_crtc *crtc = dev_priv->fbc.crtc;
+	struct drm_framebuffer *fb = crtc->base.primary->fb;
+	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
+
+	I915_WRITE(SNB_DPFC_CTL_SA, SNB_CPU_FENCE_ENABLE | obj->fence_reg);
+}
+
 static void ilk_fbc_disable(struct drm_i915_private *dev_priv)
 {
 	u32 dpfc_ctl;
@@ -1020,14 +1077,44 @@  void intel_fbc_flush(struct drm_i915_private *dev_priv,
 
 	if (origin == ORIGIN_GTT)
 		return;
+	if (origin == ORIGIN_FLIP && dev_priv->fbc.enabled)
+		return;
 
 	mutex_lock(&dev_priv->fbc.lock);
 
 	dev_priv->fbc.busy_bits &= ~frontbuffer_bits;
 
 	if (!dev_priv->fbc.busy_bits) {
-		__intel_fbc_disable(dev_priv);
-		__intel_fbc_update(dev_priv);
+		if (origin == ORIGIN_FLIP) {
+			__intel_fbc_update(dev_priv);
+		} else {
+			__intel_fbc_disable(dev_priv);
+			__intel_fbc_update(dev_priv);
+		}
+	}
+
+	mutex_unlock(&dev_priv->fbc.lock);
+}
+
+void intel_fbc_flip_prepare(struct drm_i915_private *dev_priv,
+			    unsigned int frontbuffer_bits)
+{
+	unsigned int fbc_bits;
+
+	if (!fbc_supported(dev_priv))
+		return;
+
+	mutex_lock(&dev_priv->fbc.lock);
+
+	if (dev_priv->fbc.enabled) {
+		fbc_bits = INTEL_FRONTBUFFER_PRIMARY(dev_priv->fbc.crtc->pipe);
+		if (fbc_bits & frontbuffer_bits)
+			dev_priv->fbc.flip_prepare(dev_priv);
+	} else if (dev_priv->fbc.fbc_work) {
+		fbc_bits = INTEL_FRONTBUFFER_PRIMARY(
+				dev_priv->fbc.fbc_work->crtc->pipe);
+		if (fbc_bits & frontbuffer_bits)
+			__intel_fbc_disable(dev_priv);
 	}
 
 	mutex_unlock(&dev_priv->fbc.lock);
@@ -1063,18 +1150,25 @@  void intel_fbc_init(struct drm_i915_private *dev_priv)
 		dev_priv->fbc.fbc_enabled = ilk_fbc_enabled;
 		dev_priv->fbc.enable_fbc = gen7_fbc_enable;
 		dev_priv->fbc.disable_fbc = ilk_fbc_disable;
+		dev_priv->fbc.flip_prepare = snb_fbc_flip_prepare;
 	} else if (INTEL_INFO(dev_priv)->gen >= 5) {
 		dev_priv->fbc.fbc_enabled = ilk_fbc_enabled;
 		dev_priv->fbc.enable_fbc = ilk_fbc_enable;
 		dev_priv->fbc.disable_fbc = ilk_fbc_disable;
+		if (INTEL_INFO(dev_priv)->gen == 5)
+			dev_priv->fbc.flip_prepare = ilk_fbc_flip_prepare;
+		else
+			dev_priv->fbc.flip_prepare = snb_fbc_flip_prepare;
 	} else if (IS_GM45(dev_priv)) {
 		dev_priv->fbc.fbc_enabled = g4x_fbc_enabled;
 		dev_priv->fbc.enable_fbc = g4x_fbc_enable;
 		dev_priv->fbc.disable_fbc = g4x_fbc_disable;
+		dev_priv->fbc.flip_prepare = g4x_fbc_flip_prepare;
 	} else {
 		dev_priv->fbc.fbc_enabled = i8xx_fbc_enabled;
 		dev_priv->fbc.enable_fbc = i8xx_fbc_enable;
 		dev_priv->fbc.disable_fbc = i8xx_fbc_disable;
+		dev_priv->fbc.flip_prepare = i8xx_fbc_flip_prepare;
 
 		/* This value was pulled out of someone's hat */
 		I915_WRITE(FBC_CONTROL, 500 << FBC_CTL_INTERVAL_SHIFT);
diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
index ac85357..31a1ad3 100644
--- a/drivers/gpu/drm/i915/intel_frontbuffer.c
+++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
@@ -192,6 +192,7 @@  void intel_frontbuffer_flip_prepare(struct drm_device *dev,
 	mutex_unlock(&dev_priv->fb_tracking.lock);
 
 	intel_psr_single_frame_update(dev, frontbuffer_bits);
+	intel_fbc_flip_prepare(dev_priv, frontbuffer_bits);
 }
 
 /**