FBC improvements + frontbuffer tracking conversion

From: Rodrigo Vivi <[email protected]>

No functional changes.

v2 (Paulo): Rebase.

Signed-off-by Rodrigo Vivi <[email protected]>

Signed-off-by Paulo Zanoni <[email protected]>

---
 Documentation/DocBook/drm.tmpl   |  5 ++++
 drivers/gpu/drm/i915/intel_fbc.c | 57 ++++++++++++++++++++++++++++++++++------
 2 files changed, 54 insertions(+), 8 deletions(-)

Documentation/DocBook/drm.tmpl +5 -0

index 85287cb..8b780ab 100644
@@ -3926,6 +3926,11 @@ int num_ioctls;</synopsis>
 !Idrivers/gpu/drm/i915/intel_psr.c
       </sect2>
       <sect2>
+	<title>Frame Buffer Compression (FBC)</title>
+!Pdrivers/gpu/drm/i915/intel_fbc.c Frame Buffer Compression (FBC)
+!Idrivers/gpu/drm/i915/intel_fbc.c
+      </sect2>
+      <sect2>
         <title>DPIO</title>
 !Pdrivers/gpu/drm/i915/i915_reg.h DPIO
 	<table id="dpiox2">

drivers/gpu/drm/i915/intel_fbc.c +0 -1

index f1eeb86..7686573 100644
@@ -21,20 +21,31 @@
  * DEALINGS IN THE SOFTWARE.
  */
 
-#include "intel_drv.h"

drivers/gpu/drm/i915/intel_fbc.c +3 -0

  */
 
+#include "intel_drv.h"
+#include "i915_drv.h"
+
 static void i8xx_fbc_disable(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;

drivers/gpu/drm/i915/intel_fbc.c +3 -0

+ */
 bool intel_fbc_enabled(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;

@@ -325,6 +342,18 @@ bool intel_fbc_enabled(struct drm_device *dev)
 	return dev_priv->fbc.enabled;
 }
 
+/**
+ * bdw_fbc_sw_flush - FBC Software Flush for Broadwell.

drivers/gpu/drm/i915/intel_fbc.c +12 -0

 void bdw_fbc_sw_flush(struct drm_device *dev, u32 value)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;

@@ -429,6 +458,12 @@ static void intel_fbc_enable(struct drm_crtc *crtc)
 	schedule_delayed_work(&work->work, msecs_to_jiffies(50));
 }
 
+/**
+ * intel_fbc_disable - disable FBC
+ * @dev: the drm_device
+ *
+ * This function disables FBC.
+ */
 void intel_fbc_disable(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;

@@ -643,6 +678,12 @@ out_disable:
 	i915_gem_stolen_cleanup_compression(dev);
 }
 
+/**
+ * intel_fbc_init - Initialize FBC
+ * @dev_priv: the i915 device
+ *
+ * This function might be called during PM init process.
+ */
 void intel_fbc_init(struct drm_i915_private *dev_priv)
 {
 	if (!HAS_FBC(dev_priv)) {

Signature · 6 lines

2.1.3

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 8 lines · Paulo Zanoni

From: Rodrigo Vivi <[email protected]>

No functional changes.

v2 (Paulo): Rebase.

Signed-off-by Rodrigo Vivi <[email protected]>

Signed-off-by Paulo Zanoni <[email protected]>

Some suggestions to polish the documentation a bit below. I've merged patch 1 right away to avoid further rebase pain.

Thanks, Daniel

Quoted 30 lines

---
 Documentation/DocBook/drm.tmpl   |  5 ++++
 drivers/gpu/drm/i915/intel_fbc.c | 57 ++++++++++++++++++++++++++++++++++------
 2 files changed, 54 insertions(+), 8 deletions(-)

Documentation/DocBook/drm.tmpl +5 -0

index 85287cb..8b780ab 100644
@@ -3926,6 +3926,11 @@ int num_ioctls;</synopsis>
 !Idrivers/gpu/drm/i915/intel_psr.c
       </sect2>
       <sect2>
+     <title>Frame Buffer Compression (FBC)</title>
+!Pdrivers/gpu/drm/i915/intel_fbc.c Frame Buffer Compression (FBC)
+!Idrivers/gpu/drm/i915/intel_fbc.c
+      </sect2>
+      <sect2>
         <title>DPIO</title>
 !Pdrivers/gpu/drm/i915/i915_reg.h DPIO
      <table id="dpiox2">

drivers/gpu/drm/i915/intel_fbc.c +0 -1

index f1eeb86..7686573 100644
@@ -21,20 +21,31 @@
  * DEALINGS IN THE SOFTWARE.
  */
 
-#include "intel_drv.h"

drivers/gpu/drm/i915/intel_fbc.c line 29

Quoted 10 lines

drivers/gpu/drm/i915/intel_fbc.c +5 -4

-#include "i915_drv.h"
-
-/* FBC, or Frame Buffer Compression, is a technique employed to compress the
- * framebuffer contents in-memory, aiming at reducing the required bandwidth
+/**
+ * DOC: Frame Buffer Compression (FBC)
+ *
+ * FBC is a technique employed to compress the framebuffer contents
+ * in-memory, aiming at reducing the required bandwidth
  * during in-memory transfers and, therefore, reduce the power packet.

The above paragraph imo doesn't make sense. And duplicates what's below, so just delete it.

drivers/gpu/drm/i915/intel_fbc.c line 34

Quoted 5 lines

drivers/gpu/drm/i915/intel_fbc.c +4 -0

  *
+ * FBC is primarily a memory power savings technology. That is the major
+ * benefit is to the memory power while displaying the processor graphics
+ * information to the display. FBC works by compressing the amount of memory
+ * used by the display. It means that it is total transparent to user space.

This reads a bit too much like marketing for my taste ;-) What about:

* FBC tries to save memory bandwidth (and so power consumption) by
* compressing the amount of memory used by the display. It is total
* transparent to user space and completely handled in the kernel.

drivers/gpu/drm/i915/intel_fbc.c line 41

Quoted 10 lines

drivers/gpu/drm/i915/intel_fbc.c +5 -3

+ *
  * The benefits of FBC are mostly visible with solid backgrounds and
- * variation-less patterns.
+ * variation-less patterns. It comes from keeping the memory footprint small
+ * and having fewer memory pages opened and accessed for refreshing the display.
  *
- * FBC-related functionality can be enabled by the means of the
- * i915.i915_fbc_enable parameter
+ * i915 is responsible to reserve stolen memory for FBC and configure its
+ * offset on proper register. The hardware takes care of all

^registers

drivers/gpu/drm/i915/intel_fbc.c line 43

Quoted 2 lines

drivers/gpu/drm/i915/intel_fbc.c +2 -0

+ * compress/decompress. However there are many known cases where we have to
+ * forcibly disable it to allow proper screen updates.

Mayb add "... using the frontbuffer tracking infrastructure." At least when that's the case ;-)

Quoted 10 lines

drivers/gpu/drm/i915/intel_fbc.c +3 -0

  */
 
+#include "intel_drv.h"
+#include "i915_drv.h"
+
 static void i8xx_fbc_disable(struct drm_device *dev)
 {
      struct drm_i915_private *dev_priv = dev->dev_private;

drivers/gpu/drm/i915/intel_fbc.c static void gen7_fbc_enable(struct drm_crtc *crtc) · line 336

Quoted 8 lines

drivers/gpu/drm/i915/intel_fbc.c +5 -0

@@ -318,6 +329,12 @@ static void gen7_fbc_enable(struct drm_crtc *crtc)
      DRM_DEBUG_KMS("enabled fbc on plane %c\n", plane_name(intel_crtc->plane));
 }
 
+/**
+ * intel_fbc_enabled - Is FBC enabled?
+ * @dev: the drm_device
+ *
+ * This function is used to verify the current state of FBC.

This needs a FIXME: This should be tracked in the plane config eventually instead of queried at runtime for most callers.

Quoted 10 lines

drivers/gpu/drm/i915/intel_fbc.c +3 -0

+ */
 bool intel_fbc_enabled(struct drm_device *dev)
 {
      struct drm_i915_private *dev_priv = dev->dev_private;

@@ -325,6 +342,18 @@ bool intel_fbc_enabled(struct drm_device *dev)
      return dev_priv->fbc.enabled;
 }
 
+/**
+ * bdw_fbc_sw_flush - FBC Software Flush for Broadwell.

drivers/gpu/drm/i915/intel_fbc.c bool intel_fbc_enabled(struct drm_device *dev) · line 356

Quoted 10 lines

drivers/gpu/drm/i915/intel_fbc.c +10 -0

+ * @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.
+ */

I guess with the frontbuffer tracking we'll just have invalidate/flush entry points and this becomes a static function? In that case I wouldn't bother documenting it - maybe do a comment referencing the wa name if there is one.

Quoted 32 lines

drivers/gpu/drm/i915/intel_fbc.c +12 -1

 void bdw_fbc_sw_flush(struct drm_device *dev, u32 value)
 {
      struct drm_i915_private *dev_priv = dev->dev_private;

@@ -429,6 +458,12 @@ static void intel_fbc_enable(struct drm_crtc *crtc)
      schedule_delayed_work(&work->work, msecs_to_jiffies(50));
 }
 
+/**
+ * intel_fbc_disable - disable FBC
+ * @dev: the drm_device
+ *
+ * This function disables FBC.
+ */
 void intel_fbc_disable(struct drm_device *dev)
 {
      struct drm_i915_private *dev_priv = dev->dev_private;

@@ -643,6 +678,12 @@ out_disable:
      i915_gem_stolen_cleanup_compression(dev);
 }
 
+/**
+ * intel_fbc_init - Initialize FBC
+ * @dev_priv: the i915 device
+ *
+ * This function might be called during PM init process.
+ */
 void intel_fbc_init(struct drm_i915_private *dev_priv)
 {
      if (!HAS_FBC(dev_priv)) {
--

2.1.3

Quoted 4 lines

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 6 lines · Daniel Vetter

Quoted 2 lines · Paulo Zanoni

+ * compress/decompress. However there are many known cases where we have to
+ * forcibly disable it to allow proper screen updates.

Mayb add "... using the frontbuffer tracking infrastructure." At least when that's the case ;-)

I agree, but I believe the right place is on subsequent patch that changes to actually using frontbuffer tracking....

From: Paulo Zanoni <[email protected]>

.. because it would be a waste of time, so move the place where the check is done. Also, with this we won't risk printing "more than one pipe active, disabling compression" or "no output, disabling" when FBC is actually disabled.

This patch also represents a small behavior difference when using i915.powersave=0: it is now exactly the same as i915.enable_fbc=0 on this part of the code.

Signed-off-by Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/intel_fbc.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

drivers/gpu/drm/i915/intel_fbc.c +8 -13

index 7686573..4647d57 100644
@@ -521,10 +521,16 @@ void intel_fbc_update(struct drm_device *dev)
                return;
        }
 
-       if (!i915.powersave) {
+       if (i915.enable_fbc < 0) {
+               if (set_no_fbc_reason(dev_priv, FBC_CHIP_DEFAULT))
+                       DRM_DEBUG_KMS("disabled per chip default\n");
+               goto out_disable;
+       }
+
+       if (!i915.enable_fbc || !i915.powersave) {
                if (set_no_fbc_reason(dev_priv, FBC_MODULE_PARAM))
                        DRM_DEBUG_KMS("fbc disabled per module param\n");
-               return;
+               goto out_disable;
        }
 
        /*

@@ -559,16 +565,6 @@ void intel_fbc_update(struct drm_device *dev)
        obj = intel_fb_obj(fb);
        adjusted_mode = &intel_crtc->config.adjusted_mode;
 
-       if (i915.enable_fbc < 0) {
-               if (set_no_fbc_reason(dev_priv, FBC_CHIP_DEFAULT))
-                       DRM_DEBUG_KMS("disabled per chip default\n");
-               goto out_disable;
-       }
-       if (!i915.enable_fbc) {
-               if (set_no_fbc_reason(dev_priv, FBC_MODULE_PARAM))
-                       DRM_DEBUG_KMS("fbc disabled per module param\n");
-               goto out_disable;
-       }
        if ((adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE) ||
            (adjusted_mode->flags & DRM_MODE_FLAG_DBLSCAN)) {
                if (set_no_fbc_reason(dev_priv, FBC_UNSUPPORTED_MODE))
--

2.1.3

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 10 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>

.. because it would be a waste of time, so move the place where the check is done. Also, with this we won't risk printing "more than one pipe active, disabling compression" or "no output, disabling" when FBC is actually disabled.

This patch also represents a small behavior difference when using i915.powersave=0: it is now exactly the same as i915.enable_fbc=0 on this part of the code.

I always ask myself if we should continue using this i915.powersave here. I vaguely remember someone complaining when I tried to re-org this i915.powersave making it a umbrealla for all powersaving features and the complain was to avoid more than one variable for feature... to avoid superset of parameters, or something like that.

Or if we keep this here shouldn't we also put this to disable psr?

Anyway this can be another patch. Daniel, please let me know what you want with this parameters that I can change that later.

From: Paulo Zanoni <[email protected]>

This may save a few picoseconds on !HAS_FBC platforms. And it also satisfies my OCD.

Signed-off-by Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/intel_fbc.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

drivers/gpu/drm/i915/intel_fbc.c +0 -0

index 4647d57..f3d5764 100644

drivers/gpu/drm/i915/intel_fbc.c +2 -2

+       if (!HAS_FBC(dev))
                return;
-       }
 
        if (i915.enable_fbc < 0) {
                if (set_no_fbc_reason(dev_priv, FBC_CHIP_DEFAULT))

@@ -684,6 +682,7 @@ void intel_fbc_init(struct drm_i915_private *dev_priv)
 {
        if (!HAS_FBC(dev_priv)) {
                dev_priv->fbc.enabled = false;
+               dev_priv->fbc.no_fbc_reason = FBC_UNSUPPORTED;
                return;
        }
 
--

2.1.3

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 18 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>

This may save a few picoseconds on !HAS_FBC platforms. And it also satisfies my OCD.

Signed-off-by Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/intel_fbc.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

drivers/gpu/drm/i915/intel_fbc.c +0 -0

index 4647d57..f3d5764 100644
@@ -516,10 +516,8 @@ void intel_fbc_update(struct drm_device *dev)
        const struct drm_display_mode *adjusted_mode;
        unsigned int max_width, max_height;
 

drivers/gpu/drm/i915/intel_fbc.c void intel_fbc_update(struct drm_device *dev) · line 520

Quoted 2 lines

drivers/gpu/drm/i915/intel_fbc.c +0 -2

-       if (!HAS_FBC(dev)) {
-               set_no_fbc_reason(dev_priv, FBC_UNSUPPORTED);

I really don't like this fbc_reasons... I believe a drm_debug here is enough. but I can try to kill that later.

From: Paulo Zanoni <[email protected]>

Since the mapping from CRTCs to planes is fixed, looking at the CRTC is essentially the same as looking at the plane. Also, the next patches wil start using the frontbuffer_bits macros, and they take the pipe as the parameter instead of the plane, and this could differ on gens 2 and 3.

Another nice thing is that we don't risk accidentally initializing things to PLANE_A if we don't set the value before it is used for the first time. But this shouldn't be a problem with the current code.

Signed-off-by Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/i915_drv.h      | 2 +-
 drivers/gpu/drm/i915/intel_display.c | 5 ++---
 drivers/gpu/drm/i915/intel_fbc.c     | 6 +++---
 drivers/gpu/drm/i915/intel_sprite.c  | 2 +-
 4 files changed, 7 insertions(+), 8 deletions(-)

drivers/gpu/drm/i915/i915_drv.h +1 -1

index b5260bf..9d694f1 100644
@@ -689,7 +689,7 @@ struct i915_fbc {
        unsigned long size;
        unsigned threshold;
        unsigned int fb_id;
-       enum plane plane;
+       struct intel_crtc *crtc;
        int y;
 
        struct drm_mm_node compressed_fb;

drivers/gpu/drm/i915/intel_display.c +2 -3

index e4789f4..88f3652 100644
@@ -4218,11 +4218,10 @@ static void intel_crtc_disable_planes(struct drm_crtc *crtc)
        struct drm_i915_private *dev_priv = dev->dev_private;
        struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
        int pipe = intel_crtc->pipe;
-       int plane = intel_crtc->plane;
 
        intel_crtc_wait_for_pending_flips(crtc);
 
-       if (dev_priv->fbc.plane == plane)
+       if (dev_priv->fbc.crtc == intel_crtc)
                intel_fbc_disable(dev);
 
        hsw_disable_ips(intel_crtc);

@@ -11813,7 +11812,7 @@ intel_commit_primary_plane(struct drm_plane *plane,
                 */
                if (intel_crtc->primary_enabled &&
                    INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) &&
-                   dev_priv->fbc.plane == intel_crtc->plane &&
+                   dev_priv->fbc.crtc == intel_crtc &&
                    intel_plane->rotation != BIT(DRM_ROTATE_0)) {
                        intel_fbc_disable(dev);
                }

drivers/gpu/drm/i915/intel_fbc.c +3 -3

index f3d5764..88d00d3 100644
@@ -383,7 +383,7 @@ static void intel_fbc_work_fn(struct work_struct *__work)
                if (work->crtc->primary->fb == work->fb) {
                        dev_priv->display.enable_fbc(work->crtc);
 
-                       dev_priv->fbc.plane = to_intel_crtc(work->crtc)->plane;
+                       dev_priv->fbc.crtc = to_intel_crtc(work->crtc);
                        dev_priv->fbc.fb_id = work->crtc->primary->fb->base.id;
                        dev_priv->fbc.y = work->crtc->y;
                }

@@ -474,7 +474,7 @@ void intel_fbc_disable(struct drm_device *dev)
                return;
 
        dev_priv->display.disable_fbc(dev);
-       dev_priv->fbc.plane = -1;
+       dev_priv->fbc.crtc = NULL;
 }
 
 static bool set_no_fbc_reason(struct drm_i915_private *dev_priv,

@@ -626,7 +626,7 @@ void intel_fbc_update(struct drm_device *dev)
         * cannot be unpinned (and have its GTT offset and fence revoked)
         * without first being decoupled from the scanout and FBC disabled.
         */
-       if (dev_priv->fbc.plane == intel_crtc->plane &&
+       if (dev_priv->fbc.crtc == intel_crtc &&
            dev_priv->fbc.fb_id == fb->base.id &&
            dev_priv->fbc.y == crtc->y)
                return;

drivers/gpu/drm/i915/intel_sprite.c +1 -2

index c18e57d..942daca 100644
@@ -1016,7 +1016,7 @@ intel_pre_disable_primary(struct drm_crtc *crtc)
        struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 
        mutex_lock(&dev->struct_mutex);
-       if (dev_priv->fbc.plane == intel_crtc->plane)
+       if (dev_priv->fbc.crtc == intel_crtc)
                intel_fbc_disable(dev);
        mutex_unlock(&dev->struct_mutex);
 
--

2.1.3

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

From: Paulo Zanoni <[email protected]>

We want to port FBC to the frontbuffer tracking infrastructure, but for that we need to know what caused the object invalidation/flush so we can react accordingly: CPU mmaps need manual, GTT mmaps and flips don't need handling and ring rendering needs nukes.

Signed-off-by Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/i915_drv.h            |  7 +++++++
 drivers/gpu/drm/i915/i915_gem.c            | 10 +++++-----
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |  2 +-
 drivers/gpu/drm/i915/intel_drv.h           | 11 +++++++----
 drivers/gpu/drm/i915/intel_frontbuffer.c   | 15 ++++++++++-----
 5 files changed, 30 insertions(+), 15 deletions(-)

drivers/gpu/drm/i915/i915_drv.h +0 -0

index 9d694f1..ea3cc81 100644

drivers/gpu/drm/i915/i915_drv.h +3 -0

+	ORIGIN_FLIP,
+};
+
 struct i915_fbc {
 	unsigned long size;
 	unsigned threshold;

drivers/gpu/drm/i915/i915_gem.c +5 -5

index de241eb..7ef12e8 100644
@@ -2321,7 +2321,7 @@ i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj)
 			list_move_tail(&vma->mm_list, &vm->inactive_list);
 	}
 
-	intel_fb_obj_flush(obj, true);
+	intel_fb_obj_flush(obj, true, ORIGIN_RENDER);
 
 	list_del_init(&obj->ring_list);
 

@@ -3665,7 +3665,7 @@ i915_gem_object_flush_gtt_write_domain(struct drm_i915_gem_object *obj)
 	old_write_domain = obj->base.write_domain;
 	obj->base.write_domain = 0;
 
-	intel_fb_obj_flush(obj, false);
+	intel_fb_obj_flush(obj, false, ORIGIN_GTT);
 
 	trace_i915_gem_object_change_domain(obj,
 					    obj->base.read_domains,

@@ -3688,7 +3688,7 @@ i915_gem_object_flush_cpu_write_domain(struct drm_i915_gem_object *obj,
 	old_write_domain = obj->base.write_domain;
 	obj->base.write_domain = 0;
 
-	intel_fb_obj_flush(obj, false);
+	intel_fb_obj_flush(obj, false, ORIGIN_CPU);
 
 	trace_i915_gem_object_change_domain(obj,
 					    obj->base.read_domains,

@@ -3745,7 +3745,7 @@ i915_gem_object_set_to_gtt_domain(struct drm_i915_gem_object *obj, bool write)
 	}
 
 	if (write)
-		intel_fb_obj_invalidate(obj, NULL);
+		intel_fb_obj_invalidate(obj, NULL, ORIGIN_GTT);
 
 	trace_i915_gem_object_change_domain(obj,
 					    old_read_domains,

@@ -4072,7 +4072,7 @@ i915_gem_object_set_to_cpu_domain(struct drm_i915_gem_object *obj, bool write)
 	}
 
 	if (write)
-		intel_fb_obj_invalidate(obj, NULL);
+		intel_fb_obj_invalidate(obj, NULL, ORIGIN_CPU);
 
 	trace_i915_gem_object_change_domain(obj,
 					    old_read_domains,

drivers/gpu/drm/i915/i915_gem_execbuffer.c +1 -1

index 0c25f62..af290e6 100644
@@ -965,7 +965,7 @@ i915_gem_execbuffer_move_to_active(struct list_head *vmas,
 			obj->dirty = 1;
 			i915_gem_request_assign(&obj->last_write_req, req);
 
-			intel_fb_obj_invalidate(obj, ring);
+			intel_fb_obj_invalidate(obj, ring, ORIGIN_RENDER);
 
 			/* update for the implicit flush after a batch */
 			obj->base.write_domain &= ~I915_GEM_GPU_DOMAINS;

drivers/gpu/drm/i915/intel_drv.h +7 -4

index 588b618..633fb9a 100644
@@ -841,13 +841,15 @@ void intel_ddi_set_vc_payload_alloc(struct drm_crtc *crtc, bool state);
 
 /* intel_frontbuffer.c */
 void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
-			     struct intel_engine_cs *ring);
+			     struct intel_engine_cs *ring,
+			     enum fb_op_origin origin);
 void intel_frontbuffer_flip_prepare(struct drm_device *dev,
 				    unsigned frontbuffer_bits);
 void intel_frontbuffer_flip_complete(struct drm_device *dev,
 				     unsigned frontbuffer_bits);
 void intel_frontbuffer_flush(struct drm_device *dev,
-			     unsigned frontbuffer_bits);
+			     unsigned frontbuffer_bits,
+			     enum fb_op_origin origin);
 /**
  * intel_frontbuffer_flip - synchronous frontbuffer flip
  * @dev: DRM device

@@ -863,10 +865,11 @@ static inline
 void intel_frontbuffer_flip(struct drm_device *dev,
 			    unsigned frontbuffer_bits)
 {
-	intel_frontbuffer_flush(dev, frontbuffer_bits);
+	intel_frontbuffer_flush(dev, frontbuffer_bits, ORIGIN_FLIP);
 }
 
-void intel_fb_obj_flush(struct drm_i915_gem_object *obj, bool retire);
+void intel_fb_obj_flush(struct drm_i915_gem_object *obj, bool retire,
+			enum fb_op_origin origin);
 
 
 /* intel_audio.c */

drivers/gpu/drm/i915/intel_frontbuffer.c +10 -5

index 79f6d72..7bdac69 100644
@@ -127,6 +127,7 @@ static void intel_mark_fb_busy(struct drm_device *dev,
  * intel_fb_obj_invalidate - invalidate frontbuffer object
  * @obj: GEM object to invalidate
  * @ring: set for asynchronous rendering
+ * @origin: which operation caused the invalidation
  *
  * This function gets called every time rendering on the given object starts and
  * frontbuffer caching (fbc, low refresh rate for DRRS, panel self refresh) must

@@ -135,7 +136,8 @@ static void intel_mark_fb_busy(struct drm_device *dev,
  * scheduled.
  */
 void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
-			     struct intel_engine_cs *ring)
+			     struct intel_engine_cs *ring,
+			     enum fb_op_origin origin)
 {
 	struct drm_device *dev = obj->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;

@@ -163,6 +165,7 @@ void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
  * intel_frontbuffer_flush - flush frontbuffer
  * @dev: DRM device
  * @frontbuffer_bits: frontbuffer plane tracking bits
+ * @origin: which operation caused the invalidation
  *
  * This function gets called every time rendering on the given planes has
  * completed and frontbuffer caching can be started again. Flushes will get

@@ -171,7 +174,8 @@ void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
  * Can be called without any locks held.
  */
 void intel_frontbuffer_flush(struct drm_device *dev,
-			     unsigned frontbuffer_bits)
+			     unsigned frontbuffer_bits,
+			     enum fb_op_origin origin)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 

@@ -199,13 +203,14 @@ void intel_frontbuffer_flush(struct drm_device *dev,
  * intel_fb_obj_flush - flush frontbuffer object
  * @obj: GEM object to flush
  * @retire: set when retiring asynchronous rendering
+ * @origin: which operation caused the invalidation
  *
  * This function gets called every time rendering on the given object has
  * completed and frontbuffer caching can be started again. If @retire is true
  * then any delayed flushes will be unblocked.
  */
 void intel_fb_obj_flush(struct drm_i915_gem_object *obj,
-			bool retire)
+			bool retire, enum fb_op_origin origin)
 {
 	struct drm_device *dev = obj->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;

@@ -227,7 +232,7 @@ void intel_fb_obj_flush(struct drm_i915_gem_object *obj,
 		mutex_unlock(&dev_priv->fb_tracking.lock);
 	}
 
-	intel_frontbuffer_flush(dev, frontbuffer_bits);
+	intel_frontbuffer_flush(dev, frontbuffer_bits, origin);
 }
 
 /**

@@ -275,5 +280,5 @@ void intel_frontbuffer_flip_complete(struct drm_device *dev,
 	dev_priv->fb_tracking.flip_bits &= ~frontbuffer_bits;
 	mutex_unlock(&dev_priv->fb_tracking.lock);
 
-	intel_frontbuffer_flush(dev, frontbuffer_bits);
+	intel_frontbuffer_flush(dev, frontbuffer_bits, ORIGIN_FLIP);
 }

Signature · 6 lines

2.1.3

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 24 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>

We want to port FBC to the frontbuffer tracking infrastructure, but for that we need to know what caused the object invalidation/flush so we can react accordingly: CPU mmaps need manual, GTT mmaps and flips don't need handling and ring rendering needs nukes.

Signed-off-by Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/i915_drv.h            |  7 +++++++
 drivers/gpu/drm/i915/i915_gem.c            | 10 +++++-----
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |  2 +-
 drivers/gpu/drm/i915/intel_drv.h           | 11 +++++++----
 drivers/gpu/drm/i915/intel_frontbuffer.c   | 15 ++++++++++-----
 5 files changed, 30 insertions(+), 15 deletions(-)

drivers/gpu/drm/i915/i915_drv.h +0 -0

index 9d694f1..ea3cc81 100644
@@ -685,6 +685,13 @@ struct intel_context {
      struct list_head link;
 };
 

drivers/gpu/drm/i915/i915_drv.h struct intel_context { · line 691

Quoted 4 lines

drivers/gpu/drm/i915/i915_drv.h +4 -0

+enum fb_op_origin {
+     ORIGIN_GTT,
+     ORIGIN_CPU,
+     ORIGIN_RENDER,

Maybe GPU instead of RENDER since it includes the blitter? Render typically only means the render ring in gem code. And maybe add an I915_ prefix or so at least to the enum.

Anyway that's it from me with bikesheds, lgtm overall.

-Daniel

Quoted 179 lines

drivers/gpu/drm/i915/i915_drv.h +3 -0

+     ORIGIN_FLIP,
+};
+
 struct i915_fbc {
      unsigned long size;
      unsigned threshold;

drivers/gpu/drm/i915/i915_gem.c +5 -5

index de241eb..7ef12e8 100644
@@ -2321,7 +2321,7 @@ i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj)
                      list_move_tail(&vma->mm_list, &vm->inactive_list);
      }
 
-     intel_fb_obj_flush(obj, true);
+     intel_fb_obj_flush(obj, true, ORIGIN_RENDER);
 
      list_del_init(&obj->ring_list);
 

@@ -3665,7 +3665,7 @@ i915_gem_object_flush_gtt_write_domain(struct drm_i915_gem_object *obj)
      old_write_domain = obj->base.write_domain;
      obj->base.write_domain = 0;
 
-     intel_fb_obj_flush(obj, false);
+     intel_fb_obj_flush(obj, false, ORIGIN_GTT);
 
      trace_i915_gem_object_change_domain(obj,
                                          obj->base.read_domains,

@@ -3688,7 +3688,7 @@ i915_gem_object_flush_cpu_write_domain(struct drm_i915_gem_object *obj,
      old_write_domain = obj->base.write_domain;
      obj->base.write_domain = 0;
 
-     intel_fb_obj_flush(obj, false);
+     intel_fb_obj_flush(obj, false, ORIGIN_CPU);
 
      trace_i915_gem_object_change_domain(obj,
                                          obj->base.read_domains,

@@ -3745,7 +3745,7 @@ i915_gem_object_set_to_gtt_domain(struct drm_i915_gem_object *obj, bool write)
      }
 
      if (write)
-             intel_fb_obj_invalidate(obj, NULL);
+             intel_fb_obj_invalidate(obj, NULL, ORIGIN_GTT);
 
      trace_i915_gem_object_change_domain(obj,
                                          old_read_domains,

@@ -4072,7 +4072,7 @@ i915_gem_object_set_to_cpu_domain(struct drm_i915_gem_object *obj, bool write)
      }
 
      if (write)
-             intel_fb_obj_invalidate(obj, NULL);
+             intel_fb_obj_invalidate(obj, NULL, ORIGIN_CPU);
 
      trace_i915_gem_object_change_domain(obj,
                                          old_read_domains,

drivers/gpu/drm/i915/i915_gem_execbuffer.c +1 -1

index 0c25f62..af290e6 100644
@@ -965,7 +965,7 @@ i915_gem_execbuffer_move_to_active(struct list_head *vmas,
                      obj->dirty = 1;
                      i915_gem_request_assign(&obj->last_write_req, req);
 
-                     intel_fb_obj_invalidate(obj, ring);
+                     intel_fb_obj_invalidate(obj, ring, ORIGIN_RENDER);
 
                      /* update for the implicit flush after a batch */
                      obj->base.write_domain &= ~I915_GEM_GPU_DOMAINS;

drivers/gpu/drm/i915/intel_drv.h +7 -4

index 588b618..633fb9a 100644
@@ -841,13 +841,15 @@ void intel_ddi_set_vc_payload_alloc(struct drm_crtc *crtc, bool state);
 
 /* intel_frontbuffer.c */
 void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
-                          struct intel_engine_cs *ring);
+                          struct intel_engine_cs *ring,
+                          enum fb_op_origin origin);
 void intel_frontbuffer_flip_prepare(struct drm_device *dev,
                                  unsigned frontbuffer_bits);
 void intel_frontbuffer_flip_complete(struct drm_device *dev,
                                   unsigned frontbuffer_bits);
 void intel_frontbuffer_flush(struct drm_device *dev,
-                          unsigned frontbuffer_bits);
+                          unsigned frontbuffer_bits,
+                          enum fb_op_origin origin);
 /**
  * intel_frontbuffer_flip - synchronous frontbuffer flip
  * @dev: DRM device

@@ -863,10 +865,11 @@ static inline
 void intel_frontbuffer_flip(struct drm_device *dev,
                          unsigned frontbuffer_bits)
 {
-     intel_frontbuffer_flush(dev, frontbuffer_bits);
+     intel_frontbuffer_flush(dev, frontbuffer_bits, ORIGIN_FLIP);
 }
 
-void intel_fb_obj_flush(struct drm_i915_gem_object *obj, bool retire);
+void intel_fb_obj_flush(struct drm_i915_gem_object *obj, bool retire,
+                     enum fb_op_origin origin);
 
 
 /* intel_audio.c */

drivers/gpu/drm/i915/intel_frontbuffer.c +10 -6

index 79f6d72..7bdac69 100644
@@ -127,6 +127,7 @@ static void intel_mark_fb_busy(struct drm_device *dev,
  * intel_fb_obj_invalidate - invalidate frontbuffer object
  * @obj: GEM object to invalidate
  * @ring: set for asynchronous rendering
+ * @origin: which operation caused the invalidation
  *
  * This function gets called every time rendering on the given object starts and
  * frontbuffer caching (fbc, low refresh rate for DRRS, panel self refresh) must

@@ -135,7 +136,8 @@ static void intel_mark_fb_busy(struct drm_device *dev,
  * scheduled.
  */
 void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
-                          struct intel_engine_cs *ring)
+                          struct intel_engine_cs *ring,
+                          enum fb_op_origin origin)
 {
      struct drm_device *dev = obj->base.dev;
      struct drm_i915_private *dev_priv = dev->dev_private;

@@ -163,6 +165,7 @@ void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
  * intel_frontbuffer_flush - flush frontbuffer
  * @dev: DRM device
  * @frontbuffer_bits: frontbuffer plane tracking bits
+ * @origin: which operation caused the invalidation
  *
  * This function gets called every time rendering on the given planes has
  * completed and frontbuffer caching can be started again. Flushes will get

@@ -171,7 +174,8 @@ void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
  * Can be called without any locks held.
  */
 void intel_frontbuffer_flush(struct drm_device *dev,
-                          unsigned frontbuffer_bits)
+                          unsigned frontbuffer_bits,
+                          enum fb_op_origin origin)
 {
      struct drm_i915_private *dev_priv = dev->dev_private;
 

@@ -199,13 +203,14 @@ void intel_frontbuffer_flush(struct drm_device *dev,
  * intel_fb_obj_flush - flush frontbuffer object
  * @obj: GEM object to flush
  * @retire: set when retiring asynchronous rendering
+ * @origin: which operation caused the invalidation
  *
  * This function gets called every time rendering on the given object has
  * completed and frontbuffer caching can be started again. If @retire is true
  * then any delayed flushes will be unblocked.
  */
 void intel_fb_obj_flush(struct drm_i915_gem_object *obj,
-                     bool retire)
+                     bool retire, enum fb_op_origin origin)
 {
      struct drm_device *dev = obj->base.dev;
      struct drm_i915_private *dev_priv = dev->dev_private;

@@ -227,7 +232,7 @@ void intel_fb_obj_flush(struct drm_i915_gem_object *obj,
              mutex_unlock(&dev_priv->fb_tracking.lock);
      }
 
-     intel_frontbuffer_flush(dev, frontbuffer_bits);
+     intel_frontbuffer_flush(dev, frontbuffer_bits, origin);
 }
 
 /**

@@ -275,5 +280,5 @@ void intel_frontbuffer_flip_complete(struct drm_device *dev,
      dev_priv->fb_tracking.flip_bits &= ~frontbuffer_bits;
      mutex_unlock(&dev_priv->fb_tracking.lock);
 
-     intel_frontbuffer_flush(dev, frontbuffer_bits);
+     intel_frontbuffer_flush(dev, frontbuffer_bits, ORIGIN_FLIP);
 }
--

2.1.3

Quoted 4 lines

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 25 lines · Daniel Vetter

Quoted 24 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>

We want to port FBC to the frontbuffer tracking infrastructure, but for that we need to know what caused the object invalidation/flush so we can react accordingly: CPU mmaps need manual, GTT mmaps and flips don't need handling and ring rendering needs nukes.

Signed-off-by Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/i915_drv.h            |  7 +++++++
 drivers/gpu/drm/i915/i915_gem.c            | 10 +++++-----
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |  2 +-
 drivers/gpu/drm/i915/intel_drv.h           | 11 +++++++----
 drivers/gpu/drm/i915/intel_frontbuffer.c   | 15 ++++++++++-----
 5 files changed, 30 insertions(+), 15 deletions(-)

drivers/gpu/drm/i915/i915_drv.h +0 -0

index 9d694f1..ea3cc81 100644
@@ -685,6 +685,13 @@ struct intel_context {
      struct list_head link;
 };
 

Quoted 8 lines

drivers/gpu/drm/i915/i915_drv.h struct intel_context { · line 691

Quoted 4 lines

drivers/gpu/drm/i915/i915_drv.h +4 -0

+enum fb_op_origin {
+     ORIGIN_GTT,
+     ORIGIN_CPU,
+     ORIGIN_RENDER,

Maybe GPU instead of RENDER since it includes the blitter? Render typically only means the render ring in gem code. And maybe add an I915_ prefix or so at least to the enum.

I agree with Daniel that RENDER isn't a good name, but also not sure about GPU... Maybe ENGINE_CS or RINGS...

From: Paulo Zanoni <[email protected]>

We need this for FBC, and possibly for PSR too.

Signed-off-by Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/i915_gem.c | 4 ++++
 1 file changed, 4 insertions(+)

drivers/gpu/drm/i915/i915_gem.c +4 -0

index 7ef12e8..b8c906e 100644
@@ -1117,6 +1117,10 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
 			ret = i915_gem_phys_pwrite(obj, args, file);
 		else
 			ret = i915_gem_shmem_pwrite(dev, obj, args, file);
+
+		intel_fb_obj_flush(obj, false, ORIGIN_CPU);
+	} else {
+		intel_fb_obj_flush(obj, false, ORIGIN_GTT);
 	}
 
 out:

Signature · 6 lines

2.1.3

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 5 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>

We need this for FBC, and possibly for PSR too.

Signed-off-by Paulo Zanoni <[email protected]>

Hm, this might fix some cursor update failures if userspace only pwrites and doesn't follow suite with a cursor ioctl update call. Not sure this ever happens, but definitely makes sense.
-Daniel

Quoted 22 lines

---
 drivers/gpu/drm/i915/i915_gem.c | 4 ++++
 1 file changed, 4 insertions(+)

drivers/gpu/drm/i915/i915_gem.c +4 -1

index 7ef12e8..b8c906e 100644
@@ -1117,6 +1117,10 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
                      ret = i915_gem_phys_pwrite(obj, args, file);
              else
                      ret = i915_gem_shmem_pwrite(dev, obj, args, file);
+
+             intel_fb_obj_flush(obj, false, ORIGIN_CPU);
+     } else {
+             intel_fb_obj_flush(obj, false, ORIGIN_GTT);
      }
 
 out:
--

2.1.3

Quoted 4 lines

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

wondering if this fixes: https://bugs.freedesktop.org/show_bug.cgi?id=87143

From: Paulo Zanoni <[email protected]>

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 <[email protected]>

---
 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(-)

drivers/gpu/drm/i915/i915_drv.h +1 -8

index ea3cc81..22285c2 100644
@@ -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;

drivers/gpu/drm/i915/intel_drv.h +5 -1

index 633fb9a..3a1c3d82 100644
@@ -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);

drivers/gpu/drm/i915/intel_fbc.c +70 -49

index 88d00d3..03ba43d 100644
@@ -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;

drivers/gpu/drm/i915/intel_frontbuffer.c +2 -12

index 7bdac69..47b3409 100644
@@ -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);
 }
 
 /**

drivers/gpu/drm/i915/intel_ringbuffer.c +2 -39

index 83accb7..5982da9 100644
@@ -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;
 }
 

drivers/gpu/drm/i915/intel_ringbuffer.h +0 -2

index 6dbb6f4..7e31ec8 100644
@@ -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
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

From: Paulo Zanoni <[email protected]>

I want to make this code a little more complicated, so let's extract the function first.

Signed-off-by Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/intel_fbc.c | 46 +++++++++++++++++++++++++---------------
 1 file changed, 29 insertions(+), 17 deletions(-)

drivers/gpu/drm/i915/intel_fbc.c +29 -18

index 03ba43d..450d0be 100644
@@ -444,6 +444,32 @@ static bool set_no_fbc_reason(struct drm_i915_private *dev_priv,
        return true;
 }
 
+static struct drm_crtc *intel_fbc_find_crtc(struct drm_i915_private *dev_priv)
+{
+       struct drm_device *dev = dev_priv->dev;
+       struct drm_crtc *crtc = NULL, *tmp_crtc;
+
+       for_each_crtc(dev, tmp_crtc) {
+               if (intel_crtc_active(tmp_crtc) &&
+                   to_intel_crtc(tmp_crtc)->primary_enabled) {
+                       if (crtc) {
+                               if (set_no_fbc_reason(dev_priv, FBC_MULTIPLE_PIPES))
+                                       DRM_DEBUG_KMS("more than one pipe active, disabling compression\n");
+                               return NULL;
+                       }
+                       crtc = tmp_crtc;
+               }
+       }
+
+       if (!crtc || crtc->primary->fb == NULL) {
+               if (set_no_fbc_reason(dev_priv, FBC_NO_OUTPUT))
+                       DRM_DEBUG_KMS("no output, disabling\n");
+               return NULL;
+       }
+
+       return crtc;
+}
+
 /**
  * intel_fbc_update - enable/disable FBC as needed
  * @dev: the drm_device

@@ -466,7 +492,7 @@ static bool set_no_fbc_reason(struct drm_i915_private *dev_priv,
 void intel_fbc_update(struct drm_device *dev)
 {
        struct drm_i915_private *dev_priv = dev->dev_private;
-       struct drm_crtc *crtc = NULL, *tmp_crtc;
+       struct drm_crtc *crtc = NULL;
        struct intel_crtc *intel_crtc;
        struct drm_framebuffer *fb;
        struct drm_i915_gem_object *obj;

@@ -497,23 +523,9 @@ void intel_fbc_update(struct drm_device *dev)
         *   - new fb is too large to fit in compressed buffer
         *   - going to an unsupported config (interlace, pixel multiply, etc.)
         */
-       for_each_crtc(dev, tmp_crtc) {
-               if (intel_crtc_active(tmp_crtc) &&
-                   to_intel_crtc(tmp_crtc)->primary_enabled) {
-                       if (crtc) {
-                               if (set_no_fbc_reason(dev_priv, FBC_MULTIPLE_PIPES))
-                                       DRM_DEBUG_KMS("more than one pipe active, disabling compression\n");
-                               goto out_disable;
-                       }
-                       crtc = tmp_crtc;
-               }
-       }
-
-       if (!crtc || crtc->primary->fb == NULL) {
-               if (set_no_fbc_reason(dev_priv, FBC_NO_OUTPUT))
-                       DRM_DEBUG_KMS("no output, disabling\n");
+       crtc = intel_fbc_find_crtc(dev_priv);
+       if (!crtc)
                goto out_disable;
-       }
 
        intel_crtc = to_intel_crtc(crtc);
        fb = crtc->primary->fb;
--

2.1.3

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

From: Paulo Zanoni <[email protected]>

So add code to consider this case.

Signed-off-by Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/intel_fbc.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

drivers/gpu/drm/i915/intel_fbc.c +16 -5

index 450d0be..e8dc1d5 100644
@@ -446,10 +446,16 @@ static bool set_no_fbc_reason(struct drm_i915_private *dev_priv,
 
 static struct drm_crtc *intel_fbc_find_crtc(struct drm_i915_private *dev_priv)
 {
-       struct drm_device *dev = dev_priv->dev;
        struct drm_crtc *crtc = NULL, *tmp_crtc;
+       enum pipe pipe;
+       bool pipe_a_only = false;
+
+       if (IS_HASWELL(dev_priv) || INTEL_INFO(dev_priv)->gen >= 8)
+               pipe_a_only = true;
+
+       for_each_pipe(dev_priv, pipe) {
+               tmp_crtc = dev_priv->pipe_to_crtc_mapping[pipe];
 
-       for_each_crtc(dev, tmp_crtc) {
                if (intel_crtc_active(tmp_crtc) &&
                    to_intel_crtc(tmp_crtc)->primary_enabled) {
                        if (crtc) {

@@ -459,6 +465,9 @@ static struct drm_crtc *intel_fbc_find_crtc(struct drm_i915_private *dev_priv)
                        }
                        crtc = tmp_crtc;
                }
+
+               if (pipe_a_only)
+                       break;
        }
 
        if (!crtc || crtc->primary->fb == NULL) {

@@ -714,11 +723,14 @@ void intel_fbc_init(struct drm_i915_private *dev_priv)
                return;
        }
 
-       /* TODO: some platforms have FBC tied to a specific plane! */
-       for_each_pipe(dev_priv, pipe)
+       for_each_pipe(dev_priv, pipe) {
                dev_priv->fbc.possible_framebuffer_bits |=
                                INTEL_FRONTBUFFER_PRIMARY(pipe);
 
+               if (IS_HASWELL(dev_priv) || INTEL_INFO(dev_priv)->gen >= 8)
+                       break;
+       }
+
        if (INTEL_INFO(dev_priv)->gen >= 7) {
                dev_priv->display.fbc_enabled = ilk_fbc_enabled;
                dev_priv->display.enable_fbc = gen7_fbc_enable;
--

2.1.3

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

I always ask myself if we should just clean the code and remove all platforms before HSW that always had many fbc issues. So we could make it simple and just do for pipe A for all platforms.

From: Paulo Zanoni <[email protected]>

So allow it.

Signed-off-by Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/intel_fbc.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

drivers/gpu/drm/i915/intel_fbc.c +4 -3

index e8dc1d5..1c22922 100644
@@ -448,17 +448,19 @@ static struct drm_crtc *intel_fbc_find_crtc(struct drm_i915_private *dev_priv)
 {
        struct drm_crtc *crtc = NULL, *tmp_crtc;
        enum pipe pipe;
-       bool pipe_a_only = false;
+       bool pipe_a_only = false, one_pipe_only = false;
 
        if (IS_HASWELL(dev_priv) || INTEL_INFO(dev_priv)->gen >= 8)
                pipe_a_only = true;
+       else if (INTEL_INFO(dev_priv)->gen <= 4)
+               one_pipe_only = true;
 
        for_each_pipe(dev_priv, pipe) {
                tmp_crtc = dev_priv->pipe_to_crtc_mapping[pipe];
 
                if (intel_crtc_active(tmp_crtc) &&
                    to_intel_crtc(tmp_crtc)->primary_enabled) {
-                       if (crtc) {
+                       if (one_pipe_only && crtc) {
                                if (set_no_fbc_reason(dev_priv, FBC_MULTIPLE_PIPES))
                                        DRM_DEBUG_KMS("more than one pipe active, disabling compression\n");
                                return NULL;
--

2.1.3

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

From: Paulo Zanoni <[email protected]>

Just a little helper for code that needs to wait for a certain condition to happen. It has the nice advantage that it can survive the signal helper.

Despite the callers added in this patch, there is another that will go in a separate patch, and another in a new IGT test file that I plan to push later.

Signed-off-by Paulo Zanoni <[email protected]>

---
 lib/igt_aux.c  | 18 +-----------------
 lib/igt_aux.h  | 40 ++++++++++++++++++++++++++++++++++++++++
 tests/pm_rpm.c | 28 +++++-----------------------
 3 files changed, 46 insertions(+), 40 deletions(-)

lib/igt_aux.c +1 -17

index 3051d84..180141e 100644
@@ -518,29 +518,13 @@ enum igt_runtime_pm_status igt_get_runtime_pm_status(void)
  * Waits until for the driver to switch to into the desired runtime PM status,
  * with a 10 second timeout.
  *
- * Some subtests call this function while the signal helper is active, so we
- * can't assume each usleep() call will sleep for 100ms.
- *
  * Returns:
  * True if the desired runtime PM status was attained, false if the operation
  * timed out.
  */
 bool igt_wait_for_pm_status(enum igt_runtime_pm_status status)
 {
-	struct timeval start, end, diff;
-
-	igt_assert(gettimeofday(&start, NULL) == 0);
-	do {
-		if (igt_get_runtime_pm_status() == status)
-			return true;
-
-		usleep(100 * 1000);
-
-		igt_assert(gettimeofday(&end, NULL) == 0);
-		timersub(&end, &start, &diff);
-	} while (diff.tv_sec < 10);
-
-	return false;
+	return igt_wait(igt_get_runtime_pm_status() == status, 10000, 100);
 }
 
 /* Functions with prefix kmstest_ independent of cairo library are pulled out

lib/igt_aux.h +40 -0

index 6c83c53..48c495b 100644
@@ -30,6 +30,7 @@
 
 #include <intel_bufmgr.h>
 #include <stdbool.h>
+#include <sys/time.h>
 
 extern drm_intel_bo **trash_bos;
 extern int num_trash_bos;

@@ -90,4 +91,43 @@ void intel_require_memory(uint32_t count, uint32_t size, unsigned mode);
 #define min(a, b) ((a) < (b) ? (a) : (b))
 #define max(a, b) ((a) > (b) ? (a) : (b))
 
+/**
+ * igt_wait:
+ * @COND: condition to wait
+ * @timeout_ms: timeout in milliseconds
+ * @interval_ms: amount of time we try to sleep between COND checks
+ *
+ * Waits until COND evaluates to true or the timeout passes.
+ *
+ * It is safe to call this macro if the signal helper is active. The only
+ * problem is that the usleep() calls will return early, making us evaluate COND
+ * too often, possibly eating valuable CPU cycles.
+ *
+ * Returns:
+ * True of COND evaluated to true, false otherwise.
+ */
+#define igt_wait(COND, timeout_ms, interval_ms) ({			\
+	struct timeval start_, end_, diff_;				\
+	int elapsed_ms_;						\
+	bool ret_ = false;						\
+									\
+	igt_assert(gettimeofday(&start_, NULL) == 0);			\
+	do {								\
+		if (COND) {						\
+			ret_ = true;					\
+			break;						\
+		}							\
+									\
+		usleep(interval_ms * 1000);				\
+									\
+		igt_assert(gettimeofday(&end_, NULL) == 0);		\
+		timersub(&end_, &start_, &diff_);			\
+									\
+		elapsed_ms_ = diff_.tv_sec * 1000 +			\
+			      diff_.tv_usec / 1000;			\
+	} while (elapsed_ms_ < timeout_ms);				\
+									\
+	ret_;								\
+})
+
 #endif /* IGT_AUX_H */

tests/pm_rpm.c +5 -23

index c120d75..b7f9635 100644
@@ -153,24 +153,16 @@ static uint64_t get_residency(uint32_t type)
 
 static bool pc8_plus_residency_changed(unsigned int timeout_sec)
 {
-	unsigned int i;
 	uint64_t res_pc8, res_pc9, res_pc10;
-	int to_sleep = 100 * 1000;
 
 	res_pc8 = get_residency(MSR_PC8_RES);
 	res_pc9 = get_residency(MSR_PC9_RES);
 	res_pc10 = get_residency(MSR_PC10_RES);
 
-	for (i = 0; i < timeout_sec * 1000 * 1000; i += to_sleep) {
-		if (res_pc8 != get_residency(MSR_PC8_RES) ||
-		    res_pc9 != get_residency(MSR_PC9_RES) ||
-		    res_pc10 != get_residency(MSR_PC10_RES)) {
-			return true;
-		}
-		usleep(to_sleep);
-	}
-
-	return false;
+	return igt_wait(res_pc8 != get_residency(MSR_PC8_RES) ||
+			res_pc9 != get_residency(MSR_PC9_RES) ||
+			res_pc10 != get_residency(MSR_PC10_RES),
+			timeout_sec * 1000, 100);
 }
 
 static enum pc8_status get_pc8_status(void)

@@ -191,17 +183,7 @@ static enum pc8_status get_pc8_status(void)
 
 static bool wait_for_pc8_status(enum pc8_status status)
 {
-	int i;
-	int hundred_ms = 100 * 1000, ten_s = 10 * 1000 * 1000;
-
-	for (i = 0; i < ten_s; i += hundred_ms) {
-		if (get_pc8_status() == status)
-			return true;
-
-		usleep(hundred_ms);
-	}
-
-	return false;
+	return igt_wait(get_pc8_status() == status, 10000, 100);
 }
 
 static bool wait_for_suspended(void)

Signature · 6 lines

2.1.3

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx