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