From: "Zanoni, Paulo R" <paulo.r.zanoni@intel.com>
To: "ville.syrjala@linux.intel.com" <ville.syrjala@linux.intel.com>
Cc: "intel-gfx@lists.freedesktop.org" <intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 02/26] drm/i915: don't stop+start FBC at every flip
Date: Wed, 28 Oct 2015 16:56:39 +0000 [thread overview]
Message-ID: <1446051398.2750.3.camel@intel.com> (raw)
In-Reply-To: <20151027183231.GN4437@intel.com>
Em Ter, 2015-10-27 às 20:32 +0200, Ville Syrjälä escreveu:
> On Tue, Oct 27, 2015 at 02:50:04PM -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.
>
> Yeah, I did this in my patches too, and IIRC it seemed to work just
> fine
> back then.
>
> >
> > v2: Add comment at flush() (Chris).
> >
> > 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 | 103
> > ++++++++++++++++++++++++++++++-
> > drivers/gpu/drm/i915/intel_frontbuffer.c | 1 +
> > 6 files changed, 108 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index ee14962..77da500 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -928,6 +928,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 8942532..3d598a2 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
>
> It's only three bits on gen2. But bit 3 is MBZ and there are only 8
> fence registers on those platforms, so this is OK.
>
> > #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 bc1907e..d9378c3 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -11502,7 +11502,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 08847d0..9065a8f 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1305,6 +1305,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..62f158b 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);
> > +}
>
> IIRC I just called the enable function to reconstruct the entire
> register contents to avoid code duplication. But maybe that's not
> possible without reowrking the fbc state handling entirely
> (which I had done).
> > +
> > 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;
> > @@ -1021,13 +1078,48 @@ void intel_fbc_flush(struct
> > drm_i915_private *dev_priv,
> > if (origin == ORIGIN_GTT)
> > return;
> >
> > + /* Hardware tracking already recompresses the CFB (nuke)
> > for us if FBC
> > + * is enabled and we do a page flip, so we can safely
> > ignore it here.
> > + * FBC may be disabled in case we got an invalidate()
> > before the
> > + * flush(), so we'll still have to check that case below.
> > */
> > + 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);
>
> primary->frontbuffer_bit would seem better.
Why?
>
> > + if (fbc_bits & frontbuffer_bits)
> > + dev_priv->fbc.flip_prepare(dev_priv);
>
> You would still have to disable+reenable if you need to eg.
> reallocate
> the compressed buffer, of if the new fb isn't suitable for fbc, or
> maybe
> if you need to change anything else in the control register.
Yes, but as far as I understand the frontbuffer tracking, this case
won't happen here. I'm assuming we'll always go through
intel_fbc_update() on these cases.
>
> What I had was:
> "
> static bool intel_fbc_need_reinit(struct intel_crtc *crtc)
> {
> struct drm_i915_private *dev_priv = crtc->base.dev-
> >dev_private;
> struct drm_framebuffer *fb = crtc->base.primary->fb;
> struct drm_i915_gem_object *obj = to_intel_framebuffer(fb)-
> >obj;
>
> return crtc != dev_priv->fbc.crtc ||
> obj->base.size > dev_priv->fbc.size ||
> drm_format_plane_cpp(fb->pixel_format, 0) !=
> drm_format_plane_cpp(dev_priv->fbc.pixel_format, 0) ||
> fb->pitches[0] != dev_priv->fbc.pitch;
> }
> intel_fbc_pre_page_flip()
> {
> ...
> intel_fbc_update_pending_score(crtc);
>
> /*
> * If fbc was already possible we can update immediately,
> * otherwise we will wait until the flip is finished.
> */
> if (crtc->fbc.score != 0)
> crtc->fbc.score = crtc->fbc.pending_score;
>
> /*
> * Disable fbc if we're not (yet) capable, or if
> * we just need a full disable+enable reinit.
> */
> if (crtc->fbc.score == 0 || intel_fbc_need_reinit(crtc))
> __intel_fbc_disable(crtc);
> ...
> }
> "
>
> > + } 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 +1155,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);
> > }
> >
> > /**
> > --
> > 2.6.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2015-10-28 16:56 UTC|newest]
Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-10-27 16:50 [PATCH 00/26] Yet another FBC series, v2 Paulo Zanoni
2015-10-27 16:50 ` [PATCH 01/26] drm/i915: change no_fbc_reason from enum to string Paulo Zanoni
2015-10-27 16:50 ` [PATCH 02/26] drm/i915: don't stop+start FBC at every flip Paulo Zanoni
2015-10-27 18:32 ` Ville Syrjälä
2015-10-28 16:56 ` Zanoni, Paulo R [this message]
2015-10-28 17:20 ` Ville Syrjälä
2015-10-29 12:05 ` Maarten Lankhorst
2015-10-29 17:30 ` Ville Syrjälä
2015-10-29 17:52 ` Zanoni, Paulo R
2015-10-29 18:14 ` Ville Syrjälä
2015-10-27 19:50 ` Chris Wilson
2015-10-28 16:58 ` Zanoni, Paulo R
2015-10-27 16:50 ` [PATCH 03/26] drm/i915: rename intel_fbc_nuke to intel_fbc_recompress Paulo Zanoni
2015-10-27 16:50 ` [PATCH 04/26] drm/i915: only nuke FBC when a drawing operation triggers a flush Paulo Zanoni
2015-10-27 16:50 ` [PATCH 05/26] drm/i915: extract fbc_on_pipe_a_only() Paulo Zanoni
2015-10-29 12:05 ` Maarten Lankhorst
2015-10-29 15:55 ` Zanoni, Paulo R
2015-10-27 16:50 ` [PATCH 06/26] drm/i915: remove unnecessary check for crtc->primary->fb Paulo Zanoni
2015-10-27 16:50 ` [PATCH 07/26] drm/i915: extract crtc_is_valid() on the FBC code Paulo Zanoni
2015-10-27 16:50 ` [PATCH 08/26] drm/i915: set dev_priv->fbc.crtc before scheduling the enable work Paulo Zanoni
2015-10-27 16:50 ` [PATCH 09/26] drm/i915: use struct intel_crtc *crtc at __intel_fbc_update() Paulo Zanoni
2015-10-27 16:50 ` [PATCH 10/26] drm/i915: fix the __intel_fbc_update() comments Paulo Zanoni
2015-10-27 16:50 ` [PATCH 11/26] drm/i915: pass the crtc as an argument to intel_fbc_update() Paulo Zanoni
2015-10-27 16:50 ` [PATCH 12/26] drm/i915: don't disable_fbc() if FBC is already disabled Paulo Zanoni
2015-10-27 16:50 ` [PATCH 13/26] drm/i915: introduce is_active/activate/deactivate to the FBC terminology Paulo Zanoni
2015-10-27 16:50 ` [PATCH 14/26] drm/i915: refactor FBC deactivation at init Paulo Zanoni
2015-10-27 16:50 ` [PATCH 15/26] drm/i915: introduce intel_fbc_{enable, disable} Paulo Zanoni
2015-10-27 16:50 ` [PATCH 16/26] drm/i915: remove too-frequent FBC debug message Paulo Zanoni
2015-10-27 16:50 ` [PATCH 17/26] drm/i915: fix the CFB size check Paulo Zanoni
2015-10-27 16:50 ` [PATCH 18/26] drm/i915: alloc/free the FBC CFB during enable/disable Paulo Zanoni
2015-10-27 16:50 ` [PATCH 19/26] drm/i915: move adjusted_mode checks from fbc_update to fbc_enable Paulo Zanoni
2015-10-29 12:59 ` Maarten Lankhorst
2015-10-29 17:58 ` Zanoni, Paulo R
2015-11-02 8:53 ` Maarten Lankhorst
2015-10-27 16:50 ` [PATCH 20/26] drm/i915: move clock frequency " Paulo Zanoni
2015-10-27 16:50 ` [PATCH 21/26] drm/i915: check for FBC planes in the same place as the pipes Paulo Zanoni
2015-10-27 16:50 ` [PATCH 22/26] drm/i915: clarify that checking the FB stride for CFB is intentional Paulo Zanoni
2015-10-27 16:50 ` [PATCH 23/26] drm/i915: use a single intel_fbc_work struct Paulo Zanoni
2015-10-27 20:29 ` Chris Wilson
2015-10-28 17:24 ` Zanoni, Paulo R
2015-10-28 17:40 ` chris
2015-10-27 16:50 ` [PATCH 24/26] drm/i915: wait for a vblank instead of 50ms when enabling FBC Paulo Zanoni
2015-10-27 16:50 ` [PATCH 25/26] drm/i915: remove in_dbg_master check from intel_fbc.c Paulo Zanoni
2015-10-27 16:50 ` [PATCH 26/26] drm/i915: kill fbc.uncompressed_size Paulo Zanoni
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1446051398.2750.3.camel@intel.com \
--to=paulo.r.zanoni@intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=ville.syrjala@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox