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

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

 



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@xxxxxxxxx>
> > 
> > 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@xxxxxxxxxxxxxxxxxxxxx
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx




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