Re: [PATCH 08/11] drm/i915: add fronbuffer tracking to FBC

[Rodrigo Vivi <rodrigo.vivi@gmail.com>]

just a note that I sent a reviwed v2: rebased and with typo fix.

On Mon, Dec 8, 2014 at 8:09 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.
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h          |   9 +--
>  drivers/gpu/drm/i915/intel_drv.h         |   6 +-
>  drivers/gpu/drm/i915/intel_fbc.c         | 119 ++++++++++++++++++-------------
>  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, 80 insertions(+), 110 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index ea3cc81..22285c2 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -696,6 +696,7 @@ struct i915_fbc {
>         unsigned long size;
>         unsigned threshold;
>         unsigned int fb_id;
> +       unsigned int possible_framebuffer_bits;
>         struct intel_crtc *crtc;
>         int y;
>
> @@ -708,14 +709,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 633fb9a..3a1c3d82 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1071,7 +1071,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, enum fb_op_origin origin);
>
>  /* 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 88d00d3..03ba43d 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -178,29 +178,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 */
> -       gen6_gt_force_wake_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);
> -
> -       gen6_gt_force_wake_put(dev_priv, FORCEWAKE_MEDIA);
> +       I915_WRITE(MSG_FBC_REND_STATE, FBC_REND_NUKE);
> +       POSTING_READ(MSG_FBC_REND_STATE);
>  }
>
>  static void ilk_fbc_enable(struct drm_crtc *crtc)
> @@ -243,9 +224,10 @@ static void ilk_fbc_enable(struct drm_crtc *crtc)
>                 I915_WRITE(SNB_DPFC_CTL_SA,
>                            SNB_CPU_FENCE_ENABLE | obj->fence_reg);
>                 I915_WRITE(DPFC_CPU_FENCE_OFFSET, crtc->y);
> -               snb_fbc_blit_update(dev);
>         }
>
> +       intel_fbc_nuke(dev_priv);
> +
>         DRM_DEBUG_KMS("enabled fbc on plane %c\n", plane_name(intel_crtc->plane));
>  }
>
> @@ -324,7 +306,7 @@ static void gen7_fbc_enable(struct drm_crtc *crtc)
>                    SNB_CPU_FENCE_ENABLE | obj->fence_reg);
>         I915_WRITE(DPFC_CPU_FENCE_OFFSET, crtc->y);
>
> -       snb_fbc_blit_update(dev);
> +       intel_fbc_nuke(dev_priv);
>
>         DRM_DEBUG_KMS("enabled fbc on plane %c\n", plane_name(intel_crtc->plane));
>  }
> @@ -342,31 +324,6 @@ bool intel_fbc_enabled(struct drm_device *dev)
>         return dev_priv->fbc.enabled;
>  }
>
> -/**
> - * bdw_fbc_sw_flush - FBC Software Flush for Broadwell.
> - * @dev: the drm_device
> - * @value: Value to be set on MSG_FBC_REND_STATE. Possible values are
> - *         FBC_REND_NUKE and FBC_REND_CACHE_CLEAN.
> - *
> - * This function is needed on Broadwell to perform Nuke or Cache clean on
> - * software side over MMIO.
> - * On Broadwell, due a hardware bug, MSG_FBC_REND_STATE stay in a forbidden
> - * address that has a huge risk of causing GPU Hangs if set with LRI on some
> - * command streamers.
> - */
> -void bdw_fbc_sw_flush(struct drm_device *dev, u32 value)
> -{
> -       struct drm_i915_private *dev_priv = dev->dev_private;
> -
> -       if (!IS_GEN8(dev))
> -               return;
> -
> -       if (!intel_fbc_enabled(dev))
> -               return;
> -
> -       I915_WRITE(MSG_FBC_REND_STATE, value);
> -}
> -
>  static void intel_fbc_work_fn(struct work_struct *__work)
>  {
>         struct intel_fbc_work *work =
> @@ -672,6 +629,63 @@ out_disable:
>         i915_gem_stolen_cleanup_compression(dev);
>  }
>
> +void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
> +                         unsigned int frontbuffer_bits,
> +                         enum fb_op_origin origin)
> +{
> +       struct drm_device *dev = dev_priv->dev;
> +       unsigned int fbc_bits;
> +
> +       if (origin == ORIGIN_GTT)
> +               return;
> +
> +       if (dev_priv->fbc.enabled)
> +               fbc_bits = INTEL_FRONTBUFFER_PRIMARY(dev_priv->fbc.crtc->pipe);
> +       else if (dev_priv->fbc.fbc_work)
> +               fbc_bits = INTEL_FRONTBUFFER_PRIMARY(
> +                       to_intel_crtc(dev_priv->fbc.fbc_work->crtc)->pipe);
> +       else
> +               fbc_bits = dev_priv->fbc.possible_framebuffer_bits;
> +
> +       if ((fbc_bits & frontbuffer_bits) == 0)
> +               return;
> +
> +       intel_fbc_disable(dev);
> +}
> +
> +void intel_fbc_flush(struct drm_i915_private *dev_priv,
> +                    unsigned int frontbuffer_bits,
> +                    enum fb_op_origin origin)
> +{
> +       struct drm_device *dev = dev_priv->dev;
> +       unsigned int fbc_bits = 0;
> +       bool fbc_enabled;
> +
> +       if (dev_priv->fbc.enabled)
> +               fbc_bits = INTEL_FRONTBUFFER_PRIMARY(dev_priv->fbc.crtc->pipe);
> +       else
> +               fbc_bits = dev_priv->fbc.possible_framebuffer_bits;
> +
> +       /* These are not the planes you are looking for. */
> +       if ((frontbuffer_bits & fbc_bits) == 0)
> +               return;
> +
> +       fbc_enabled = intel_fbc_enabled(dev);
> +
> +       if ((dev_priv->fb_tracking.busy_bits & frontbuffer_bits &
> +            fbc_bits) == 0) {
> +               if (fbc_enabled) {
> +                       if (origin == ORIGIN_RENDER || origin == ORIGIN_CPU)
> +                               intel_fbc_nuke(dev_priv);
> +               } else {
> +                       intel_fbc_update(dev);
> +               }
> +       } else {
> +               if (fbc_enabled)
> +                       intel_fbc_disable(dev);
> +       }
> +}
> +
>  /**
>   * intel_fbc_init - Initialize FBC
>   * @dev_priv: the i915 device
> @@ -680,12 +694,19 @@ out_disable:
>   */
>  void intel_fbc_init(struct drm_i915_private *dev_priv)
>  {
> +       enum pipe pipe;
> +
>         if (!HAS_FBC(dev_priv)) {
>                 dev_priv->fbc.enabled = false;
>                 dev_priv->fbc.no_fbc_reason = FBC_UNSUPPORTED;
>                 return;
>         }
>
> +       /* TODO: some platforms have FBC tied to a specific plane! */
> +       for_each_pipe(dev_priv, pipe)
> +               dev_priv->fbc.possible_framebuffer_bits |=
> +                               INTEL_FRONTBUFFER_PRIMARY(pipe);
> +
>         if (INTEL_INFO(dev_priv)->gen >= 7) {
>                 dev_priv->display.fbc_enabled = ilk_fbc_enabled;
>                 dev_priv->display.enable_fbc = gen7_fbc_enable;
> diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
> index 7bdac69..47b3409 100644
> --- a/drivers/gpu/drm/i915/intel_frontbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
> @@ -118,8 +118,6 @@ static void intel_mark_fb_busy(struct drm_device *dev,
>                         continue;
>
>                 intel_increase_pllclock(dev, pipe);
> -               if (ring && intel_fbc_enabled(dev))
> -                       ring->fbc_dirty = true;
>         }
>  }
>
> @@ -159,6 +157,7 @@ void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
>         intel_mark_fb_busy(dev, obj->frontbuffer_bits, ring);
>
>         intel_psr_invalidate(dev, obj->frontbuffer_bits);
> +       intel_fbc_invalidate(dev_priv, obj->frontbuffer_bits, origin);
>  }
>
>  /**
> @@ -187,16 +186,7 @@ void intel_frontbuffer_flush(struct drm_device *dev,
>         intel_mark_fb_busy(dev, frontbuffer_bits, NULL);
>
>         intel_psr_flush(dev, frontbuffer_bits);
> -
> -       /*
> -        * FIXME: Unconditional fbc flushing here is a rather gross hack and
> -        * needs to be reworked into a proper frontbuffer tracking scheme like
> -        * psr employs.
> -        */
> -       if (dev_priv->fbc.need_sw_cache_clean) {
> -               dev_priv->fbc.need_sw_cache_clean = false;
> -               bdw_fbc_sw_flush(dev, FBC_REND_CACHE_CLEAN);
> -       }
> +       intel_fbc_flush(dev_priv, frontbuffer_bits, origin);
>  }
>
>  /**
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 83accb7..5982da9 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -317,29 +317,6 @@ gen7_render_ring_cs_stall_wa(struct intel_engine_cs *ring)
>         return 0;
>  }
>
> -static int gen7_ring_fbc_flush(struct intel_engine_cs *ring, u32 value)
> -{
> -       int ret;
> -
> -       if (!ring->fbc_dirty)
> -               return 0;
> -
> -       ret = intel_ring_begin(ring, 6);
> -       if (ret)
> -               return ret;
> -       /* WaFbcNukeOn3DBlt:ivb/hsw */
> -       intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
> -       intel_ring_emit(ring, MSG_FBC_REND_STATE);
> -       intel_ring_emit(ring, value);
> -       intel_ring_emit(ring, MI_STORE_REGISTER_MEM(1) | MI_SRM_LRM_GLOBAL_GTT);
> -       intel_ring_emit(ring, MSG_FBC_REND_STATE);
> -       intel_ring_emit(ring, ring->scratch.gtt_offset + 256);
> -       intel_ring_advance(ring);
> -
> -       ring->fbc_dirty = false;
> -       return 0;
> -}
> -
>  static int
>  gen7_render_ring_flush(struct intel_engine_cs *ring,
>                        u32 invalidate_domains, u32 flush_domains)
> @@ -395,9 +372,6 @@ gen7_render_ring_flush(struct intel_engine_cs *ring,
>         intel_ring_emit(ring, 0);
>         intel_ring_advance(ring);
>
> -       if (!invalidate_domains && flush_domains)
> -               return gen7_ring_fbc_flush(ring, FBC_REND_NUKE);
> -
>         return 0;
>  }
>
> @@ -459,9 +433,6 @@ gen8_render_ring_flush(struct intel_engine_cs *ring,
>         if (ret)
>                 return ret;
>
> -       if (!invalidate_domains && flush_domains)
> -               return gen7_ring_fbc_flush(ring, FBC_REND_NUKE);
> -
>         return 0;
>  }
>
> @@ -2279,7 +2250,6 @@ static int gen6_ring_flush(struct intel_engine_cs *ring,
>                            u32 invalidate, u32 flush)
>  {
>         struct drm_device *dev = ring->dev;
> -       struct drm_i915_private *dev_priv = dev->dev_private;
>         uint32_t cmd;
>         int ret;
>
> @@ -2288,7 +2258,7 @@ static int gen6_ring_flush(struct intel_engine_cs *ring,
>                 return ret;
>
>         cmd = MI_FLUSH_DW;
> -       if (INTEL_INFO(ring->dev)->gen >= 8)
> +       if (INTEL_INFO(dev)->gen >= 8)
>                 cmd += 1;
>         /*
>          * Bspec vol 1c.3 - blitter engine command streamer:
> @@ -2301,7 +2271,7 @@ static int gen6_ring_flush(struct intel_engine_cs *ring,
>                         MI_FLUSH_DW_OP_STOREDW;
>         intel_ring_emit(ring, cmd);
>         intel_ring_emit(ring, I915_GEM_HWS_SCRATCH_ADDR | MI_FLUSH_DW_USE_GTT);
> -       if (INTEL_INFO(ring->dev)->gen >= 8) {
> +       if (INTEL_INFO(dev)->gen >= 8) {
>                 intel_ring_emit(ring, 0); /* upper addr */
>                 intel_ring_emit(ring, 0); /* value */
>         } else  {
> @@ -2310,13 +2280,6 @@ static int gen6_ring_flush(struct intel_engine_cs *ring,
>         }
>         intel_ring_advance(ring);
>
> -       if (!invalidate && flush) {
> -               if (IS_GEN7(dev))
> -                       return gen7_ring_fbc_flush(ring, FBC_REND_CACHE_CLEAN);
> -               else if (IS_BROADWELL(dev))
> -                       dev_priv->fbc.need_sw_cache_clean = true;
> -       }
> -
>         return 0;
>  }
>
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 6dbb6f4..7e31ec8 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -269,7 +269,6 @@ struct  intel_engine_cs {
>          */
>         struct drm_i915_gem_request *outstanding_lazy_request;
>         bool gpu_caches_dirty;
> -       bool fbc_dirty;
>
>         wait_queue_head_t irq_queue;
>
> --
> 2.1.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx