Re: [PATCH 6/7] drm/i915: add frontbuffer tracking to FBC

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Mar 04, 2015 at 08:54:21PM +0000, Vivi, Rodrigo wrote:
> On Wed, 2015-03-04 at 15:03 -0300, Paulo Zanoni wrote:
> > 2015-03-03 21:57 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@gmail.com>:
> > > On Fri, Feb 13, 2015 at 11:23 AM, Paulo Zanoni <przanoni@gmail.com> wrote:
> > >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > >>
> > >> Kill the blt/render tracking we currently have and use the frontbuffer
> > >> tracking infrastructure.
> > >>
> > >> Don't enable things by default yet.
> > >>
> > >> v2: (Rodrigo) Fix small conflict on rebase and typo at subject.
> > >> v3: (Paulo) Rebase on RENDER_CS change.
> > >> v4: (Paulo) Rebase.
> > >> v5: (Paulo) Simplify: flushes don't have origin (Daniel).
> > >>             Also rebase due to patch order changes.
> > >>
> > >> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > >> ---
> > >>  drivers/gpu/drm/i915/i915_drv.h          | 10 +---
> > >>  drivers/gpu/drm/i915/intel_drv.h         |  6 ++-
> > >>  drivers/gpu/drm/i915/intel_fbc.c         | 91 +++++++++++++++++++-------------
> > >>  drivers/gpu/drm/i915/intel_frontbuffer.c | 14 +----
> > >>  drivers/gpu/drm/i915/intel_ringbuffer.c  | 41 +-------------
> > >>  drivers/gpu/drm/i915/intel_ringbuffer.h  |  1 -
> > >>  6 files changed, 65 insertions(+), 98 deletions(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > >> index 30aaa8e..7680ca0 100644
> > >> --- a/drivers/gpu/drm/i915/i915_drv.h
> > >> +++ b/drivers/gpu/drm/i915/i915_drv.h
> > >> @@ -783,6 +783,8 @@ struct i915_fbc {
> > >>         unsigned long uncompressed_size;
> > >>         unsigned threshold;
> > >>         unsigned int fb_id;
> > >> +       unsigned int possible_framebuffer_bits;
> > >> +       unsigned int busy_bits;
> > >
> > > I'm not sure if I understood why you keep 2 variables here?
> > 
> > After Daniel's last suggestion, the possible_framebuffer_bits could
> > probably be removed and left as a local variable at intel_fbc_validate
> > now: it's just a matter of coding style. Some platforms support FBC on
> > more than just pipe A, so this variable is used to simplify checks
> > related to this.
> > 
> > The busy_bits was also part of Daniel's last request: we need to store
> > which bits triggered intel_fbc_invalidate calls, and then check them
> > when intel_fbc_flush is called.
> 
> Got it.
> Thanks for explaining it.
> 
> > 
> > > Is it because we keep enabling/disabling fbc with updates all over the code?
> > > In this case it is ok we just need to kill it when we kill update_fbc...
> > 
> > I don't understand what you mean here. Please clarify.
> 
> I had missunderstood the reason of possible_frontbuffer bits, sorry.
> But about killing update_fbc I believe that after frontbuffer rework is
> in place we don't need to use that update_fbc function that disable and
> enable fbc on the cases we knew it could fail. With frontbuffer bits we
> could let it always enabled and kicking with frontbuffer bits.
> 
> > 
> > >
> > >>         struct intel_crtc *crtc;
> > >>         int y;
> > >>
> > >> @@ -795,14 +797,6 @@ struct i915_fbc {
> > >>          * possible. */
> > >>         bool enabled;
> > >>
> > >> -       /* On gen8 some rings cannont perform fbc clean operation so for now
> > >> -        * we are doing this on SW with mmio.
> > >> -        * This variable works in the opposite information direction
> > >> -        * of ring->fbc_dirty telling software on frontbuffer tracking
> > >> -        * to perform the cache clean on sw side.
> > >> -        */
> > >> -       bool need_sw_cache_clean;
> > >> -
> > >>         struct intel_fbc_work {
> > >>                 struct delayed_work work;
> > >>                 struct drm_crtc *crtc;
> > >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > >> index 05d0a43f..571174f 100644
> > >> --- a/drivers/gpu/drm/i915/intel_drv.h
> > >> +++ b/drivers/gpu/drm/i915/intel_drv.h
> > >> @@ -1091,7 +1091,11 @@ bool intel_fbc_enabled(struct drm_device *dev);
> > >>  void intel_fbc_update(struct drm_device *dev);
> > >>  void intel_fbc_init(struct drm_i915_private *dev_priv);
> > >>  void intel_fbc_disable(struct drm_device *dev);
> > >> -void bdw_fbc_sw_flush(struct drm_device *dev, u32 value);
> > >> +void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
> > >> +                         unsigned int frontbuffer_bits,
> > >> +                         enum fb_op_origin origin);
> > >> +void intel_fbc_flush(struct drm_i915_private *dev_priv,
> > >> +                    unsigned int frontbuffer_bits);
> > >>
> > >>  /* intel_hdmi.c */
> > >>  void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port);
> > >> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> > >> index 618f7bd..9fcf446 100644
> > >> --- a/drivers/gpu/drm/i915/intel_fbc.c
> > >> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > >> @@ -174,29 +174,10 @@ static bool g4x_fbc_enabled(struct drm_device *dev)
> > >>         return I915_READ(DPFC_CONTROL) & DPFC_CTL_EN;
> > >>  }
> > >>
> > >> -static void snb_fbc_blit_update(struct drm_device *dev)
> > >> +static void intel_fbc_nuke(struct drm_i915_private *dev_priv)
> > >>  {
> > >> -       struct drm_i915_private *dev_priv = dev->dev_private;
> > >> -       u32 blt_ecoskpd;
> > >> -
> > >> -       /* Make sure blitter notifies FBC of writes */
> > >> -
> > >> -       /* Blitter is part of Media powerwell on VLV. No impact of
> > >> -        * his param in other platforms for now */
> > >> -       intel_uncore_forcewake_get(dev_priv, FORCEWAKE_MEDIA);
> > >> -
> > >> -       blt_ecoskpd = I915_READ(GEN6_BLITTER_ECOSKPD);
> > >> -       blt_ecoskpd |= GEN6_BLITTER_FBC_NOTIFY <<
> > >> -               GEN6_BLITTER_LOCK_SHIFT;
> > >> -       I915_WRITE(GEN6_BLITTER_ECOSKPD, blt_ecoskpd);
> > >> -       blt_ecoskpd |= GEN6_BLITTER_FBC_NOTIFY;
> > >> -       I915_WRITE(GEN6_BLITTER_ECOSKPD, blt_ecoskpd);
> > >> -       blt_ecoskpd &= ~(GEN6_BLITTER_FBC_NOTIFY <<
> > >> -                        GEN6_BLITTER_LOCK_SHIFT);
> > >> -       I915_WRITE(GEN6_BLITTER_ECOSKPD, blt_ecoskpd);
> > >> -       POSTING_READ(GEN6_BLITTER_ECOSKPD);
> > >> -
> > >> -       intel_uncore_forcewake_put(dev_priv, FORCEWAKE_MEDIA);
> > >
> > > Are you sure this continue working on snb? or should we fully remove
> > > fbc support for snb and older?
> > >
> > > Anyway I believe this could be in a different patch.
> > 
> > Well, the whole idea of using the sofware-based frontbuffer tracking
> > mechanism is to get rid of all the hardware-based stuff.
> 
> Hm, I still have a concern here. We would need to test on those
> platforms to be certain. The reason for that is that HW tracking still
> works so it can let the buffer in a stage that compression is unable to
> start... same reason that we still need both lines below on BDW.

Afaik the fence based tracking only invalidates specific lines. Everything
else shouldn't be required any more with this patch.

> Anyway this wouldn't break anything and it this rework was a great
> improvement so feel free to use
> 
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx