[PATCH 0/4] Enable FBC on SKL, v2

[PATCH 0/4] Enable FBC on SKL, v2

[Paulo Zanoni]

Hi

The Kernel patches are mostly the same, but with the review
suggestions implemented. The DDX patch is new and should really help
things now.

Thanks,
Paulo

Paulo Zanoni (4):
  drm/i915/fbc: update busy_bits even for GTT and flip flushes
  drm/i915/fbc: sanitize i915.enable_fbc during FBC init
  drm/i915: opt-out CPU and WC mmaps from FBC
  drm/i915/fbc: enable FBC on gen 9+ too

 drivers/gpu/drm/i915/i915_drv.h          |  8 +++++
 drivers/gpu/drm/i915/i915_gem.c          | 19 ++++++----
 drivers/gpu/drm/i915/intel_display.c     |  1 +
 drivers/gpu/drm/i915/intel_drv.h         |  3 ++
 drivers/gpu/drm/i915/intel_fbc.c         | 60 ++++++++++++++++++++++++++------
 drivers/gpu/drm/i915/intel_frontbuffer.c | 32 +++++++++++++++++
 6 files changed, 105 insertions(+), 18 deletions(-)

-- 
2.7.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/4] drm/i915/fbc: update busy_bits even for GTT and flip flushes

[Paulo Zanoni]

We ignore ORIGIN_GTT because the hardware tracking can recognize GTT
writes and take care of them. We also ignore ORIGIN_FLIP because we
deal with flips without relying on the frontbuffer tracking
infrastructure. On the other hand, a flush is a flush and means we're
good to go, so we need to update busy_bits in order to reflect that,
even if we're not going to do anything else about it.

How to reproduce the bug fixed by this patch:
 - boot SKL up to the desktop environment
 - stop the display manager
 - run any of the igt/kms_frontbuffer_tracking/*fbc*onoff* subtests
 - the tests will fail

The steps above will create the right conditions for us to lose track
of busy_bits. If you, for example, run the full set of FBC tests, the
onoff subtests will succeed.

Also notice that the "bug" is that we'll just keep FBC disabled on
cases where it could be enabled, so it's not something the users can
perceive, it just affects power consumption numbers on properly
configured machines.

Testcase: igt/kms_frontbuffer_tracking/*fbc*onoff* (see above)
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_fbc.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 2e571f5..b8ba79c 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -996,13 +996,13 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv,
 	if (!fbc_supported(dev_priv))
 		return;
 
-	if (origin == ORIGIN_GTT || origin == ORIGIN_FLIP)
-		return;
-
 	mutex_lock(&fbc->lock);
 
 	fbc->busy_bits &= ~frontbuffer_bits;
 
+	if (origin == ORIGIN_GTT || origin == ORIGIN_FLIP)
+		goto out;
+
 	if (!fbc->busy_bits && fbc->enabled &&
 	    (frontbuffer_bits & intel_fbc_get_frontbuffer_bit(fbc))) {
 		if (fbc->active)
@@ -1011,6 +1011,7 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv,
 			__intel_fbc_post_update(fbc->crtc);
 	}
 
+out:
 	mutex_unlock(&fbc->lock);
 }
 
-- 
2.7.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH xf86-video-intel] sna: Opt-out of the Kernel mmap workaround

[Paulo Zanoni]

... by issuing dirtyfb calls upon framebuffer creation. Since the
non-working SNA versions just never ever issued dirtyfb or sw_finish
calls, the Kernel workaround disables itself in case it sees one of
those calls. The problem is that we've fixed the bug by forcing
TearFree, so we're still not sending dirtyfb or sw_finish calls, so
the Kernel keeps the workaround enabled since it doesn't know we're
properly behaving. So issue one dirtyfb call each time we create a
framebuffer, making sure the Kernel sets the FB_MMAP_WA_DISABLE flag.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 src/sna/kgem.c             | 14 ++++++++++++++
 src/sna/kgem.h             |  1 +
 src/sna/sna_display.c      |  1 +
 src/sna/sna_video_sprite.c |  2 ++
 4 files changed, 18 insertions(+)

diff --git a/src/sna/kgem.c b/src/sna/kgem.c
index 06c1684..a5969ae 100644
--- a/src/sna/kgem.c
+++ b/src/sna/kgem.c
@@ -5257,6 +5257,18 @@ __kgem_bo_create_as_display(struct kgem *kgem, int size, int tiling, int pitch)
 	return bo;
 }
 
+void kgem_mmap_wa_opt_out(struct kgem *kgem, uint32_t fb_id)
+{
+	struct drm_mode_fb_dirty_cmd cmd;
+
+	if (kgem->gen != 0110 || !kgem->has_dirtyfb)
+		return;
+
+	memset(&cmd, 0, sizeof(cmd));
+	cmd.fb_id = fb_id;
+	(void)drmIoctl(kgem->fd, DRM_IOCTL_MODE_DIRTYFB, &cmd);
+}
+
 static void __kgem_bo_make_scanout(struct kgem *kgem,
 				   struct kgem_bo *bo,
 				   int width, int height)
@@ -5306,6 +5318,7 @@ static void __kgem_bo_make_scanout(struct kgem *kgem,
 		DBG(("%s: attached fb=%d to handle=%d\n",
 		     __FUNCTION__, arg.fb_id, arg.handle));
 		bo->delta = arg.fb_id;
+		kgem_mmap_wa_opt_out(kgem, bo->delta);
 	}
 }
 
@@ -5479,6 +5492,7 @@ struct kgem_bo *kgem_create_2d(struct kgem *kgem,
 
 				bo->delta = arg.fb_id;
 				bo->unique_id = kgem_get_unique_id(kgem);
+				kgem_mmap_wa_opt_out(kgem, bo->delta);
 
 				DBG(("  2:from scanout: pitch=%d, tiling=%d, handle=%d, id=%d\n",
 				     bo->pitch, bo->tiling, bo->handle, bo->unique_id));
diff --git a/src/sna/kgem.h b/src/sna/kgem.h
index 3630b5c..02521ae 100644
--- a/src/sna/kgem.h
+++ b/src/sna/kgem.h
@@ -326,6 +326,7 @@ bool kgem_bo_convert_to_gpu(struct kgem *kgem,
 			    struct kgem_bo *bo,
 			    unsigned flags);
 
+void kgem_mmap_wa_opt_out(struct kgem *kgem, uint32_t fb_id);
 bool kgem_bo_is_fenced(struct kgem *kgem, struct kgem_bo *bo);
 uint32_t kgem_bo_get_binding(struct kgem_bo *bo, uint32_t format);
 void kgem_bo_set_binding(struct kgem_bo *bo, uint32_t format, uint16_t offset);
diff --git a/src/sna/sna_display.c b/src/sna/sna_display.c
index 7d2478c..68eea42 100644
--- a/src/sna/sna_display.c
+++ b/src/sna/sna_display.c
@@ -636,6 +636,7 @@ fail:
 	}
 	assert(arg.fb_id != 0);
 	bo->delta = arg.fb_id;
+	kgem_mmap_wa_opt_out(&sna->kgem, bo->delta);
 	DBG(("%s: attached fb=%d to handle=%d\n",
 	     __FUNCTION__, bo->delta, arg.handle));
 
diff --git a/src/sna/sna_video_sprite.c b/src/sna/sna_video_sprite.c
index ae08ef7..d95abaa 100644
--- a/src/sna/sna_video_sprite.c
+++ b/src/sna/sna_video_sprite.c
@@ -325,6 +325,8 @@ sna_video_sprite_show(struct sna *sna,
 		frame->bo->scanout = true;
 		/* Don't allow the scanout to be cached if not suitable for front */
 		frame->bo->purged = purged;
+
+		kgem_mmap_wa_opt_out(&sna->kgem, frame->bo->delta);
 	}
 
 	assert(frame->bo->scanout);
-- 
2.7.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/4] drm/i915/fbc: sanitize i915.enable_fbc during FBC init

[Paulo Zanoni]

The DDX driver changes its behavior depending on the value it reads
from i915.enable_fbc, so sanitize the value in order to allow it to
know what's going on. It uses this in order to choose the defaults for
the TearFree option. Before this patch, it will read -1 and always
assume that FBC is disabled, so it won't force TearFree.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_fbc.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index b8ba79c..7101880 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -823,21 +823,14 @@ static bool intel_fbc_can_choose(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
 	struct intel_fbc *fbc = &dev_priv->fbc;
-	bool enable_by_default = IS_HASWELL(dev_priv) ||
-				 IS_BROADWELL(dev_priv);
 
 	if (intel_vgpu_active(dev_priv->dev)) {
 		fbc->no_fbc_reason = "VGPU is active";
 		return false;
 	}
 
-	if (i915.enable_fbc < 0 && !enable_by_default) {
-		fbc->no_fbc_reason = "disabled per chip default";
-		return false;
-	}
-
 	if (!i915.enable_fbc) {
-		fbc->no_fbc_reason = "disabled per module param";
+		fbc->no_fbc_reason = "disabled per module param or by default";
 		return false;
 	}
 
@@ -1239,6 +1232,16 @@ void intel_fbc_init(struct drm_i915_private *dev_priv)
 	fbc->active = false;
 	fbc->work.scheduled = false;
 
+	/* The DDX driver changes its behavior depending on the value it reads
+	 * from i915.enable_fbc, so sanitize the value in order to allow it to
+	 * know what's going on. */
+	if (i915.enable_fbc < 0) {
+		i915.enable_fbc = IS_HASWELL(dev_priv) ||
+				  IS_BROADWELL(dev_priv);
+		DRM_DEBUG_KMS("Sanitized enable_fbc value: %d\n",
+			      i915.enable_fbc);
+	}
+
 	if (!HAS_FBC(dev_priv)) {
 		fbc->no_fbc_reason = "unsupported by this chipset";
 		return;
-- 
2.7.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/4] drm/i915: opt-out CPU and WC mmaps from FBC

[Paulo Zanoni]

FBC and the frontbuffer tracking infrastructure were designed assuming
that user space applications would follow a specific set of rules
regarding frontbuffer management and mmapping. I recently discovered
that current user space is not exactly following these rules: my
investigation led me to the conclusion that the generic backend from
SNA - used by SKL and the other new platforms without a specific
backend - is not issuing sw_finish/dirty_fb IOCTLs when using the CPU
and WC mmaps. I discovered this when running lightdm: I would type the
password and nothing would appear on the screen unless I moved the
mouse over the place where the letters were supposed to appear.

Since we can't detect whether the current DRM master is a well-behaved
application or not, let's use the sledgehammer approach and assume
that every user space client on every gen is bad by disabling FBC when
the frontbuffer is CPU/WC mmapped.  This will allow us to enable FBC
on SKL, moving its FBC-related power savings from "always 0%" to "> 0%
in some cases".

There's also some additional code to disable the workaround for
frontbuffers that ever received a sw_finish or dirty_fb IOCTL, since
the current bad user space never issues those calls. This should help
for some specific cases and our IGT tests, but won't be enough for a
new xf86-video-intel using TearFree.

Notice that we may need an equivalent commit for PSR too. We also need
to investigate whether PSR needs to be disabled on GTT mmaps: if
that's the case, we'll have problems since we seem to always have GTT
mmaps on our current user space, so we would end up keeping PSR
disabled forever.

v2:
  - Rename some things.
  - Disable the WA on sw_finish/dirty_fb (Daniel).
  - Fix NULL obj checking.
  - Restric to Gen 9.
v3:
  - Add missing parameter documentation (kbuild test robot).
  - Convert flags from enum to defines (Jani).
  - Make it SKL-only (Daniel).

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com> (v2)
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h          |  8 ++++++++
 drivers/gpu/drm/i915/i915_gem.c          | 19 +++++++++++-------
 drivers/gpu/drm/i915/intel_display.c     |  1 +
 drivers/gpu/drm/i915/intel_drv.h         |  3 +++
 drivers/gpu/drm/i915/intel_fbc.c         | 33 ++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_frontbuffer.c | 32 +++++++++++++++++++++++++++++++
 6 files changed, 89 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b93ef70..cfb8074 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -863,6 +863,12 @@ enum fb_op_origin {
 	ORIGIN_DIRTYFB,
 };
 
+/* Flags for the frontbuffer workaround for old user space. */
+#define FB_MMAP_WA_CPU		(1 << 0)
+#define FB_MMAP_WA_GTT		(1 << 1)
+#define FB_MMAP_WA_DISABLE	(1 << 2)
+#define FB_MMAP_WA_FLAG_COUNT	3
+
 struct intel_fbc {
 	/* This is always the inner lock when overlapping with struct_mutex and
 	 * it's the outer lock when overlapping with stolen_lock. */
@@ -900,6 +906,7 @@ struct intel_fbc {
 			unsigned int stride;
 			int fence_reg;
 			unsigned int tiling_mode;
+			unsigned int mmap_wa_flags;
 		} fb;
 	} state_cache;
 
@@ -2147,6 +2154,7 @@ struct drm_i915_gem_object {
 	unsigned int cache_dirty:1;
 
 	unsigned int frontbuffer_bits:INTEL_FRONTBUFFER_BITS;
+	unsigned int fb_mmap_wa_flags:FB_MMAP_WA_FLAG_COUNT;
 
 	unsigned int pin_display;
 
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index c7a997a..9204054 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1692,6 +1692,8 @@ i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data,
 		goto unlock;
 	}
 
+	intel_fb_obj_mmap_wa(obj, FB_MMAP_WA_DISABLE);
+
 	/* Pinned buffers may be scanout, so flush the cache */
 	if (obj->pin_display)
 		i915_gem_object_flush_cpu_write_domain(obj);
@@ -1724,7 +1726,7 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data,
 		    struct drm_file *file)
 {
 	struct drm_i915_gem_mmap *args = data;
-	struct drm_gem_object *obj;
+	struct drm_i915_gem_object *obj;
 	unsigned long addr;
 
 	if (args->flags & ~(I915_MMAP_WC))
@@ -1733,19 +1735,19 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data,
 	if (args->flags & I915_MMAP_WC && !cpu_has_pat)
 		return -ENODEV;
 
-	obj = drm_gem_object_lookup(dev, file, args->handle);
-	if (obj == NULL)
+	obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle));
+	if (&obj->base == NULL)
 		return -ENOENT;
 
 	/* prime objects have no backing filp to GEM mmap
 	 * pages from.
 	 */
-	if (!obj->filp) {
-		drm_gem_object_unreference_unlocked(obj);
+	if (!obj->base.filp) {
+		drm_gem_object_unreference_unlocked(&obj->base);
 		return -EINVAL;
 	}
 
-	addr = vm_mmap(obj->filp, 0, args->size,
+	addr = vm_mmap(obj->base.filp, 0, args->size,
 		       PROT_READ | PROT_WRITE, MAP_SHARED,
 		       args->offset);
 	if (args->flags & I915_MMAP_WC) {
@@ -1761,10 +1763,12 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data,
 			addr = -ENOMEM;
 		up_write(&mm->mmap_sem);
 	}
-	drm_gem_object_unreference_unlocked(obj);
+	drm_gem_object_unreference_unlocked(&obj->base);
 	if (IS_ERR((void *)addr))
 		return addr;
 
+	intel_fb_obj_mmap_wa(obj, FB_MMAP_WA_CPU);
+
 	args->addr_ptr = (uint64_t) addr;
 
 	return 0;
@@ -2099,6 +2103,7 @@ i915_gem_mmap_gtt(struct drm_file *file,
 		goto out;
 
 	*offset = drm_vma_node_offset_addr(&obj->base.vma_node);
+	intel_fb_obj_mmap_wa(obj, FB_MMAP_WA_GTT);
 
 out:
 	drm_gem_object_unreference(&obj->base);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 29aa64b..da77826 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14597,6 +14597,7 @@ static int intel_user_framebuffer_dirty(struct drm_framebuffer *fb,
 	struct drm_i915_gem_object *obj = intel_fb->obj;
 
 	mutex_lock(&dev->struct_mutex);
+	intel_fb_obj_mmap_wa(obj, FB_MMAP_WA_DISABLE);
 	intel_fb_obj_flush(obj, false, ORIGIN_DIRTYFB);
 	mutex_unlock(&dev->struct_mutex);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index c87b450..a0b49ec 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1085,6 +1085,7 @@ void intel_frontbuffer_flip_complete(struct drm_device *dev,
 				     unsigned frontbuffer_bits);
 void intel_frontbuffer_flip(struct drm_device *dev,
 			    unsigned frontbuffer_bits);
+void intel_fb_obj_mmap_wa(struct drm_i915_gem_object *obj, unsigned int flags);
 unsigned int intel_fb_align_height(struct drm_device *dev,
 				   unsigned int height,
 				   uint32_t pixel_format,
@@ -1377,6 +1378,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_mmap_wa(struct drm_i915_private *dev_priv,
+		       unsigned int frontbuffer_bits, unsigned int flags);
 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 7101880..718ac38 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -745,6 +745,14 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc)
 	cache->fb.stride = fb->pitches[0];
 	cache->fb.fence_reg = obj->fence_reg;
 	cache->fb.tiling_mode = obj->tiling_mode;
+	cache->fb.mmap_wa_flags = obj->fb_mmap_wa_flags;
+}
+
+static bool need_mmap_disable_workaround(struct intel_fbc *fbc)
+{
+	unsigned int flags = fbc->state_cache.fb.mmap_wa_flags;
+
+	return (flags & FB_MMAP_WA_CPU) && !(flags & FB_MMAP_WA_DISABLE);
 }
 
 static bool intel_fbc_can_activate(struct intel_crtc *crtc)
@@ -816,6 +824,11 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc)
 		return false;
 	}
 
+	if (need_mmap_disable_workaround(fbc)) {
+		fbc->no_fbc_reason = "FB is CPU or WC mmapped";
+		return false;
+	}
+
 	return true;
 }
 
@@ -1008,6 +1021,26 @@ out:
 	mutex_unlock(&fbc->lock);
 }
 
+void intel_fbc_mmap_wa(struct drm_i915_private *dev_priv,
+		       unsigned int frontbuffer_bits, unsigned int flags)
+{
+	struct intel_fbc *fbc = &dev_priv->fbc;
+
+	if (!fbc_supported(dev_priv))
+		return;
+
+	mutex_lock(&fbc->lock);
+
+	if (fbc->enabled &&
+	    (intel_fbc_get_frontbuffer_bit(fbc) & frontbuffer_bits)) {
+		fbc->state_cache.fb.mmap_wa_flags = flags;
+		if (need_mmap_disable_workaround(fbc))
+			intel_fbc_deactivate(dev_priv);
+	}
+
+	mutex_unlock(&fbc->lock);
+}
+
 /**
  * intel_fbc_choose_crtc - select a CRTC to enable FBC on
  * @dev_priv: i915 device instance
diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
index ac85357..01483e7 100644
--- a/drivers/gpu/drm/i915/intel_frontbuffer.c
+++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
@@ -241,3 +241,35 @@ void intel_frontbuffer_flip(struct drm_device *dev,
 
 	intel_frontbuffer_flush(dev, frontbuffer_bits, ORIGIN_FLIP);
 }
+
+/**
+ * intel_fb_obj_mmap_wa - notifies about objects being mmapped
+ * @obj: GEM object being mmapped
+ * @flags: new flags to be set to @obj
+ *
+ * This function gets called whenever an object gets mmapped. Not every user
+ * space application follows the protocol assumed by the frontbuffer tracking
+ * subsystem when it was created, so this mmap notify callback can be used to
+ * completely disable frontbuffer features such as FBC and PSR. Even if at some
+ * point we fix ever user space application, there's still the possibility that
+ * the user may have a new Kernel with the old user space.
+ *
+ * Also notice that there's no munmap API because user space calls munmap()
+ * directly. Even if we had, it probably wouldn't help since munmap() calls are
+ * not common.
+ */
+void intel_fb_obj_mmap_wa(struct drm_i915_gem_object *obj, unsigned int flags)
+{
+	struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
+
+	if (!IS_SKYLAKE(dev_priv))
+		return;
+
+	obj->fb_mmap_wa_flags |= flags;
+
+	if (!obj->frontbuffer_bits)
+		return;
+
+	intel_fbc_mmap_wa(dev_priv, obj->frontbuffer_bits,
+			  obj->fb_mmap_wa_flags);
+}
-- 
2.7.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/i915: opt-out CPU and WC mmaps from FBC

[Chris Wilson]

On Thu, Mar 24, 2016 at 04:16:11PM -0300, Paulo Zanoni wrote:
> FBC and the frontbuffer tracking infrastructure were designed assuming
> that user space applications would follow a specific set of rules
> regarding frontbuffer management and mmapping. I recently discovered
> that current user space is not exactly following these rules: my
> investigation led me to the conclusion that the generic backend from
> SNA - used by SKL and the other new platforms without a specific
> backend - is not issuing sw_finish/dirty_fb IOCTLs when using the CPU
> and WC mmaps. I discovered this when running lightdm: I would type the
> password and nothing would appear on the screen unless I moved the
> mouse over the place where the letters were supposed to appear.

Yes, that is a kernel bug. The protocol we said the kernel would follow
is to disable FBC/WC when userspace marks the object for writing by the
CPU and would only reestablish FBC/WC upon dirtyfb.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/i915: opt-out CPU and WC mmaps from FBC

[Zanoni, Paulo R]

Em Qui, 2016-03-24 às 19:31 +0000, Chris Wilson escreveu:
> On Thu, Mar 24, 2016 at 04:16:11PM -0300, Paulo Zanoni wrote:
> > 
> > FBC and the frontbuffer tracking infrastructure were designed
> > assuming
> > that user space applications would follow a specific set of rules
> > regarding frontbuffer management and mmapping. I recently
> > discovered
> > that current user space is not exactly following these rules: my
> > investigation led me to the conclusion that the generic backend
> > from
> > SNA - used by SKL and the other new platforms without a specific
> > backend - is not issuing sw_finish/dirty_fb IOCTLs when using the
> > CPU
> > and WC mmaps. I discovered this when running lightdm: I would type
> > the
> > password and nothing would appear on the screen unless I moved the
> > mouse over the place where the letters were supposed to appear.
> Yes, that is a kernel bug. The protocol we said the kernel would
> follow
> is to disable FBC/WC when userspace marks the object for writing by
> the
> CPU and would only reestablish FBC/WC upon dirtyfb.

But on WC mmaps we mark the object for writing by the GTT instead of
the CPU, and while the tracking engine is able to see "normal" GTT mmap
writes, it's not able to see WC mmap writes, so we established that
we'd call dirtyfb after frontbuffer drawing through WC mmaps, which is
something that the DDX never implemented. This was discussed on #intel-
gfx on Nov 5 2014, and also possibly other places, but I can't find the
logs. Daniel also confirmed this to me again on private IRC on Jun 16
2015. So I still don't understand why this is a Kernel bug instead of a
DDX bug.

> -Chris
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/i915: opt-out CPU and WC mmaps from FBC

[chris]

On Thu, Mar 24, 2016 at 08:53:21PM +0000, Zanoni, Paulo R wrote:
> Em Qui, 2016-03-24 às 19:31 +0000, Chris Wilson escreveu:
> > On Thu, Mar 24, 2016 at 04:16:11PM -0300, Paulo Zanoni wrote:
> > > 
> > > FBC and the frontbuffer tracking infrastructure were designed
> > > assuming
> > > that user space applications would follow a specific set of rules
> > > regarding frontbuffer management and mmapping. I recently
> > > discovered
> > > that current user space is not exactly following these rules: my
> > > investigation led me to the conclusion that the generic backend
> > > from
> > > SNA - used by SKL and the other new platforms without a specific
> > > backend - is not issuing sw_finish/dirty_fb IOCTLs when using the
> > > CPU
> > > and WC mmaps. I discovered this when running lightdm: I would type
> > > the
> > > password and nothing would appear on the screen unless I moved the
> > > mouse over the place where the letters were supposed to appear.
> > Yes, that is a kernel bug. The protocol we said the kernel would
> > follow
> > is to disable FBC/WC when userspace marks the object for writing by
> > the
> > CPU and would only reestablish FBC/WC upon dirtyfb.
> 
> But on WC mmaps we mark the object for writing by the GTT instead of
> the CPU, and while the tracking engine is able to see "normal" GTT mmap
> writes, it's not able to see WC mmap writes, so we established that
> we'd call dirtyfb after frontbuffer drawing through WC mmaps, which is
> something that the DDX never implemented. This was discussed on #intel-
> gfx on Nov 5 2014, and also possibly other places, but I can't find the
> logs. Daniel also confirmed this to me again on private IRC on Jun 16
> 2015. So I still don't understand why this is a Kernel bug instead of a
> DDX bug.

Because we said that once invalidated, it would not be restored until
dirtyfb. The kernel is not doing that. Your patch does not do that. To
be even close, you should be setting the origin flag based on the existence
of wc mmaping for the object inside set-to-gtt-domain. Otherwise, you
are not implementing even close to the protocol you say you are. That is
invalidate on set-domain, flush on dirtyfb.

The kernel's bug is that is not cancelling FBC. Userspace's bug is not
signalling when to reenable it.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/i915: opt-out CPU and WC mmaps from FBC

[chris]

On Thu, Mar 24, 2016 at 09:03:59PM +0000, chris@chris-wilson.co.uk wrote:
> On Thu, Mar 24, 2016 at 08:53:21PM +0000, Zanoni, Paulo R wrote:
> > Em Qui, 2016-03-24 às 19:31 +0000, Chris Wilson escreveu:
> > > On Thu, Mar 24, 2016 at 04:16:11PM -0300, Paulo Zanoni wrote:
> > > > 
> > > > FBC and the frontbuffer tracking infrastructure were designed
> > > > assuming
> > > > that user space applications would follow a specific set of rules
> > > > regarding frontbuffer management and mmapping. I recently
> > > > discovered
> > > > that current user space is not exactly following these rules: my
> > > > investigation led me to the conclusion that the generic backend
> > > > from
> > > > SNA - used by SKL and the other new platforms without a specific
> > > > backend - is not issuing sw_finish/dirty_fb IOCTLs when using the
> > > > CPU
> > > > and WC mmaps. I discovered this when running lightdm: I would type
> > > > the
> > > > password and nothing would appear on the screen unless I moved the
> > > > mouse over the place where the letters were supposed to appear.
> > > Yes, that is a kernel bug. The protocol we said the kernel would
> > > follow
> > > is to disable FBC/WC when userspace marks the object for writing by
> > > the
> > > CPU and would only reestablish FBC/WC upon dirtyfb.
> > 
> > But on WC mmaps we mark the object for writing by the GTT instead of
> > the CPU, and while the tracking engine is able to see "normal" GTT mmap
> > writes, it's not able to see WC mmap writes, so we established that
> > we'd call dirtyfb after frontbuffer drawing through WC mmaps, which is
> > something that the DDX never implemented. This was discussed on #intel-
> > gfx on Nov 5 2014, and also possibly other places, but I can't find the
> > logs. Daniel also confirmed this to me again on private IRC on Jun 16
> > 2015. So I still don't understand why this is a Kernel bug instead of a
> > DDX bug.
> 
> Because we said that once invalidated, it would not be restored until
> dirtyfb. The kernel is not doing that. Your patch does not do that. To
> be even close, you should be setting the origin flag based on the existence
> of wc mmaping for the object inside set-to-gtt-domain. Otherwise, you
> are not implementing even close to the protocol you say you are. That is
> invalidate on set-domain, flush on dirtyfb.
> 
> The kernel's bug is that is not cancelling FBC. Userspace's bug is not
> signalling when to reenable it.

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 8dec2e8..0314346 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2145,6 +2145,7 @@ struct drm_i915_gem_object {
        unsigned int cache_dirty:1;
 
        unsigned int frontbuffer_bits:INTEL_FRONTBUFFER_BITS;
+       unsigned int has_wc_mmap:1;
 
        /** Count of VMA actually bound by this object */
        unsigned int bind_count;
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 05ae706..29ca96d 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1310,6 +1310,13 @@ unlock:
        return ret;
 }
 
+static enum fb_op_origin
+write_origin(struct drm_i915_gem_object *obj, unsigned domain)
+{
+       return domain == I915_GEM_DOMAIN_GTT && !obj->has_wc_mmap ?
+             ORIGIN_GTT : ORIGIN_CPU;
+}
+
 /**
  * Called when user space prepares to use an object with the CPU, either
  * through the mmap ioctl's mapping or a GTT mapping.
@@ -1363,9 +1370,7 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data,
                ret = i915_gem_object_set_to_cpu_domain(obj, write_domain != 0);
 
        if (write_domain != 0)
-               intel_fb_obj_invalidate(obj,
-                                       write_domain == I915_GEM_DOMAIN_GTT ?
-                                       ORIGIN_GTT : ORIGIN_CPU);
+               intel_fb_obj_invalidate(obj, write_origin(obj, write_domain));
 
 unref:
        drm_gem_object_unreference(&obj->base);
@@ -1466,6 +1471,9 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data,
                else
                        addr = -ENOMEM;
                up_write(&mm->mmap_sem);
+
+               /* This may race, but that's ok, it only gets set */
+               to_intel_bo(obj)->has_wc_mmap = true;
        }

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/i915: opt-out CPU and WC mmaps from FBC

[Zanoni, Paulo R]

Em Qui, 2016-03-24 às 21:20 +0000, chris@chris-wilson.co.uk escreveu:
> On Thu, Mar 24, 2016 at 09:03:59PM +0000, chris@chris-wilson.co.uk
> wrote:
> > 
> > On Thu, Mar 24, 2016 at 08:53:21PM +0000, Zanoni, Paulo R wrote:
> > > 
> > > Em Qui, 2016-03-24 às 19:31 +0000, Chris Wilson escreveu:
> > > > 
> > > > On Thu, Mar 24, 2016 at 04:16:11PM -0300, Paulo Zanoni wrote:
> > > > > 
> > > > > 
> > > > > FBC and the frontbuffer tracking infrastructure were designed
> > > > > assuming
> > > > > that user space applications would follow a specific set of
> > > > > rules
> > > > > regarding frontbuffer management and mmapping. I recently
> > > > > discovered
> > > > > that current user space is not exactly following these rules:
> > > > > my
> > > > > investigation led me to the conclusion that the generic
> > > > > backend
> > > > > from
> > > > > SNA - used by SKL and the other new platforms without a
> > > > > specific
> > > > > backend - is not issuing sw_finish/dirty_fb IOCTLs when using
> > > > > the
> > > > > CPU
> > > > > and WC mmaps. I discovered this when running lightdm: I would
> > > > > type
> > > > > the
> > > > > password and nothing would appear on the screen unless I
> > > > > moved the
> > > > > mouse over the place where the letters were supposed to
> > > > > appear.
> > > > Yes, that is a kernel bug. The protocol we said the kernel
> > > > would
> > > > follow
> > > > is to disable FBC/WC when userspace marks the object for
> > > > writing by
> > > > the
> > > > CPU and would only reestablish FBC/WC upon dirtyfb.
> > > But on WC mmaps we mark the object for writing by the GTT instead
> > > of
> > > the CPU, and while the tracking engine is able to see "normal"
> > > GTT mmap
> > > writes, it's not able to see WC mmap writes, so we established
> > > that
> > > we'd call dirtyfb after frontbuffer drawing through WC mmaps,
> > > which is
> > > something that the DDX never implemented. This was discussed on
> > > #intel-
> > > gfx on Nov 5 2014, and also possibly other places, but I can't
> > > find the
> > > logs. Daniel also confirmed this to me again on private IRC on
> > > Jun 16
> > > 2015. So I still don't understand why this is a Kernel bug
> > > instead of a
> > > DDX bug.
> > Because we said that once invalidated, it would not be restored
> > until
> > dirtyfb. The kernel is not doing that. Your patch does not do that.
> > To
> > be even close, you should be setting the origin flag based on the
> > existence
> > of wc mmaping for the object inside set-to-gtt-domain. Otherwise,
> > you
> > are not implementing even close to the protocol you say you are.
> > That is
> > invalidate on set-domain, flush on dirtyfb.
> > 
> > The kernel's bug is that is not cancelling FBC. Userspace's bug is
> > not
> > signalling when to reenable it.
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index 8dec2e8..0314346 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2145,6 +2145,7 @@ struct drm_i915_gem_object {
>         unsigned int cache_dirty:1;
>  
>         unsigned int frontbuffer_bits:INTEL_FRONTBUFFER_BITS;
> +       unsigned int has_wc_mmap:1;
>  
>         /** Count of VMA actually bound by this object */
>         unsigned int bind_count;
> diff --git a/drivers/gpu/drm/i915/i915_gem.c
> b/drivers/gpu/drm/i915/i915_gem.c
> index 05ae706..29ca96d 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1310,6 +1310,13 @@ unlock:
>         return ret;
>  }
>  
> +static enum fb_op_origin
> +write_origin(struct drm_i915_gem_object *obj, unsigned domain)
> +{
> +       return domain == I915_GEM_DOMAIN_GTT && !obj->has_wc_mmap ?
> +             ORIGIN_GTT : ORIGIN_CPU;

What if I have both a WC mmap and a GTT mmap, and I'm actually using
the GTT mmap now? My set_domain call will be treated as WC mmap usage,
while in fact it should be treated as GTT usage. Is there a way to
differentiate between them with the current set_domain API?


> +}
> +
>  /**
>   * Called when user space prepares to use an object with the CPU,
> either
>   * through the mmap ioctl's mapping or a GTT mapping.
> @@ -1363,9 +1370,7 @@ i915_gem_set_domain_ioctl(struct drm_device
> *dev, void *data,
>                 ret = i915_gem_object_set_to_cpu_domain(obj,
> write_domain != 0);
>  
>         if (write_domain != 0)
> -               intel_fb_obj_invalidate(obj,
> -                                       write_domain ==
> I915_GEM_DOMAIN_GTT ?
> -                                       ORIGIN_GTT : ORIGIN_CPU);
> +               intel_fb_obj_invalidate(obj, write_origin(obj,
> write_domain));
>  
>  unref:
>         drm_gem_object_unreference(&obj->base);
> @@ -1466,6 +1471,9 @@ i915_gem_mmap_ioctl(struct drm_device *dev,
> void *data,
>                 else
>                         addr = -ENOMEM;
>                 up_write(&mm->mmap_sem);
> +
> +               /* This may race, but that's ok, it only gets set */
> +               to_intel_bo(obj)->has_wc_mmap = true;
>         }
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/i915: opt-out CPU and WC mmaps from FBC

[chris]

On Fri, Mar 25, 2016 at 02:05:42PM +0000, Zanoni, Paulo R wrote:
> What if I have both a WC mmap and a GTT mmap, and I'm actually using
> the GTT mmap now? My set_domain call will be treated as WC mmap usage,
> while in fact it should be treated as GTT usage. Is there a way to
> differentiate between them with the current set_domain API?

No, there is not. As far as the cache domain is regarded WC/GTT appear to
be identical, i.e. GTT is just WC. That the GTT is not as coherent as
previously regarded is yet another bug.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/i915: opt-out CPU and WC mmaps from FBC

[Daniel Vetter]

On Fri, Mar 25, 2016 at 02:05:42PM +0000, Zanoni, Paulo R wrote:
> Em Qui, 2016-03-24 às 21:20 +0000, chris@chris-wilson.co.uk escreveu:
> > On Thu, Mar 24, 2016 at 09:03:59PM +0000, chris@chris-wilson.co.uk
> > wrote:
> > > 
> > > On Thu, Mar 24, 2016 at 08:53:21PM +0000, Zanoni, Paulo R wrote:
> > > > 
> > > > Em Qui, 2016-03-24 às 19:31 +0000, Chris Wilson escreveu:
> > > > > 
> > > > > On Thu, Mar 24, 2016 at 04:16:11PM -0300, Paulo Zanoni wrote:
> > > > > > 
> > > > > > 
> > > > > > FBC and the frontbuffer tracking infrastructure were designed
> > > > > > assuming
> > > > > > that user space applications would follow a specific set of
> > > > > > rules
> > > > > > regarding frontbuffer management and mmapping. I recently
> > > > > > discovered
> > > > > > that current user space is not exactly following these rules:
> > > > > > my
> > > > > > investigation led me to the conclusion that the generic
> > > > > > backend
> > > > > > from
> > > > > > SNA - used by SKL and the other new platforms without a
> > > > > > specific
> > > > > > backend - is not issuing sw_finish/dirty_fb IOCTLs when using
> > > > > > the
> > > > > > CPU
> > > > > > and WC mmaps. I discovered this when running lightdm: I would
> > > > > > type
> > > > > > the
> > > > > > password and nothing would appear on the screen unless I
> > > > > > moved the
> > > > > > mouse over the place where the letters were supposed to
> > > > > > appear.
> > > > > Yes, that is a kernel bug. The protocol we said the kernel
> > > > > would
> > > > > follow
> > > > > is to disable FBC/WC when userspace marks the object for
> > > > > writing by
> > > > > the
> > > > > CPU and would only reestablish FBC/WC upon dirtyfb.
> > > > But on WC mmaps we mark the object for writing by the GTT instead
> > > > of
> > > > the CPU, and while the tracking engine is able to see "normal"
> > > > GTT mmap
> > > > writes, it's not able to see WC mmap writes, so we established
> > > > that
> > > > we'd call dirtyfb after frontbuffer drawing through WC mmaps,
> > > > which is
> > > > something that the DDX never implemented. This was discussed on
> > > > #intel-
> > > > gfx on Nov 5 2014, and also possibly other places, but I can't
> > > > find the
> > > > logs. Daniel also confirmed this to me again on private IRC on
> > > > Jun 16
> > > > 2015. So I still don't understand why this is a Kernel bug
> > > > instead of a
> > > > DDX bug.
> > > Because we said that once invalidated, it would not be restored
> > > until
> > > dirtyfb. The kernel is not doing that. Your patch does not do that.
> > > To
> > > be even close, you should be setting the origin flag based on the
> > > existence
> > > of wc mmaping for the object inside set-to-gtt-domain. Otherwise,
> > > you
> > > are not implementing even close to the protocol you say you are.
> > > That is
> > > invalidate on set-domain, flush on dirtyfb.
> > > 
> > > The kernel's bug is that is not cancelling FBC. Userspace's bug is
> > > not
> > > signalling when to reenable it.
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 8dec2e8..0314346 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2145,6 +2145,7 @@ struct drm_i915_gem_object {
> >         unsigned int cache_dirty:1;
> >  
> >         unsigned int frontbuffer_bits:INTEL_FRONTBUFFER_BITS;
> > +       unsigned int has_wc_mmap:1;
> >  
> >         /** Count of VMA actually bound by this object */
> >         unsigned int bind_count;
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c
> > b/drivers/gpu/drm/i915/i915_gem.c
> > index 05ae706..29ca96d 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -1310,6 +1310,13 @@ unlock:
> >         return ret;
> >  }
> >  
> > +static enum fb_op_origin
> > +write_origin(struct drm_i915_gem_object *obj, unsigned domain)
> > +{
> > +       return domain == I915_GEM_DOMAIN_GTT && !obj->has_wc_mmap ?
> > +             ORIGIN_GTT : ORIGIN_CPU;
> 
> What if I have both a WC mmap and a GTT mmap, and I'm actually using
> the GTT mmap now? My set_domain call will be treated as WC mmap usage,
> while in fact it should be treated as GTT usage. Is there a way to
> differentiate between them with the current set_domain API?

*Blonk* or whatever the sound for suddenly realization is. Totally forgot
that we're reuseding set_domain(GTT) for wc mmaps because this it's a nice
trick.

Otoh, is that trick the reason why wc mmaps aren't coherent enough? One
possible difference is that this won't do the magic chipset flush in
intel-gtt.c that we have on gen3-5. Let's pretend wbinv doesn't exist on
gen2 ;-) But that's just an aside really ...

Anyway, now that I can see again, ack on the trick to decide on ORIGIN_GTT
vs. ORIGIN_CPU.
-Daniel

> 
> 
> > +}
> > +
> >  /**
> >   * Called when user space prepares to use an object with the CPU,
> > either
> >   * through the mmap ioctl's mapping or a GTT mapping.
> > @@ -1363,9 +1370,7 @@ i915_gem_set_domain_ioctl(struct drm_device
> > *dev, void *data,
> >                 ret = i915_gem_object_set_to_cpu_domain(obj,
> > write_domain != 0);
> >  
> >         if (write_domain != 0)
> > -               intel_fb_obj_invalidate(obj,
> > -                                       write_domain ==
> > I915_GEM_DOMAIN_GTT ?
> > -                                       ORIGIN_GTT : ORIGIN_CPU);
> > +               intel_fb_obj_invalidate(obj, write_origin(obj,
> > write_domain));
> >  
> >  unref:
> >         drm_gem_object_unreference(&obj->base);
> > @@ -1466,6 +1471,9 @@ i915_gem_mmap_ioctl(struct drm_device *dev,
> > void *data,
> >                 else
> >                         addr = -ENOMEM;
> >                 up_write(&mm->mmap_sem);
> > +
> > +               /* This may race, but that's ok, it only gets set */
> > +               to_intel_bo(obj)->has_wc_mmap = true;
> >         }
> > 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/i915: opt-out CPU and WC mmaps from FBC

[chris]

On Tue, Mar 29, 2016 at 01:55:02PM +0200, Daniel Vetter wrote:
> *Blonk* or whatever the sound for suddenly realization is. Totally forgot
> that we're reuseding set_domain(GTT) for wc mmaps because this it's a nice
> trick.
> 
> Otoh, is that trick the reason why wc mmaps aren't coherent enough? One
> possible difference is that this won't do the magic chipset flush in
> intel-gtt.c that we have on gen3-5. Let's pretend wbinv doesn't exist on
> gen2 ;-) But that's just an aside really ...

No, writes through the *GTT* are not coherent.

igt/gem_mmap_wc/coherency -> PASS
igt/gem_mmap_gtt/coherency -> FAIL on byt/bsw

Also see patch on list to insert a magic delay that fixes the issue
exposed in the kernel (which causes various hangs and EINVALs).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/i915: opt-out CPU and WC mmaps from FBC

[Zanoni, Paulo R]

Em Qui, 2016-03-24 às 21:03 +0000, chris@chris-wilson.co.uk escreveu:
> On Thu, Mar 24, 2016 at 08:53:21PM +0000, Zanoni, Paulo R wrote:
> > 
> > Em Qui, 2016-03-24 às 19:31 +0000, Chris Wilson escreveu:
> > > 
> > > On Thu, Mar 24, 2016 at 04:16:11PM -0300, Paulo Zanoni wrote:
> > > > 
> > > > 
> > > > FBC and the frontbuffer tracking infrastructure were designed
> > > > assuming
> > > > that user space applications would follow a specific set of
> > > > rules
> > > > regarding frontbuffer management and mmapping. I recently
> > > > discovered
> > > > that current user space is not exactly following these rules:
> > > > my
> > > > investigation led me to the conclusion that the generic backend
> > > > from
> > > > SNA - used by SKL and the other new platforms without a
> > > > specific
> > > > backend - is not issuing sw_finish/dirty_fb IOCTLs when using
> > > > the
> > > > CPU
> > > > and WC mmaps. I discovered this when running lightdm: I would
> > > > type
> > > > the
> > > > password and nothing would appear on the screen unless I moved
> > > > the
> > > > mouse over the place where the letters were supposed to appear.
> > > Yes, that is a kernel bug. The protocol we said the kernel would
> > > follow
> > > is to disable FBC/WC when userspace marks the object for writing
> > > by
> > > the
> > > CPU and would only reestablish FBC/WC upon dirtyfb.
> > But on WC mmaps we mark the object for writing by the GTT instead
> > of
> > the CPU, and while the tracking engine is able to see "normal" GTT
> > mmap
> > writes, it's not able to see WC mmap writes, so we established that
> > we'd call dirtyfb after frontbuffer drawing through WC mmaps, which
> > is
> > something that the DDX never implemented. This was discussed on
> > #intel-
> > gfx on Nov 5 2014, and also possibly other places, but I can't find
> > the
> > logs. Daniel also confirmed this to me again on private IRC on Jun
> > 16
> > 2015. So I still don't understand why this is a Kernel bug instead
> > of a
> > DDX bug.
> Because we said that once invalidated, it would not be restored until
> dirtyfb. The kernel is not doing that. Your patch does not do that.
> To
> be even close, you should be setting the origin flag based on the
> existence
> of wc mmaping for the object inside set-to-gtt-domain. Otherwise, you
> are not implementing even close to the protocol you say you are. That
> is
> invalidate on set-domain, flush on dirtyfb.

I don't recall this being said in the earlier conversations, but now
that you point it, it can be done. Also, we recently pinged/emailed you
many times about this problem, so I wonder why it took you so long to
point this...

> 
> The kernel's bug is that is not cancelling FBC. Userspace's bug is
> not
> signalling when to reenable it.

So at least you agree user space was missing something :)

> -Chris
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/i915: opt-out CPU and WC mmaps from FBC

[chris]

On Thu, Mar 24, 2016 at 09:21:49PM +0000, Zanoni, Paulo R wrote:
> Em Qui, 2016-03-24 às 21:03 +0000, chris@chris-wilson.co.uk escreveu:
> > On Thu, Mar 24, 2016 at 08:53:21PM +0000, Zanoni, Paulo R wrote:
> > > 
> > > Em Qui, 2016-03-24 às 19:31 +0000, Chris Wilson escreveu:
> > > > 
> > > > On Thu, Mar 24, 2016 at 04:16:11PM -0300, Paulo Zanoni wrote:
> > > > > 
> > > > > 
> > > > > FBC and the frontbuffer tracking infrastructure were designed
> > > > > assuming
> > > > > that user space applications would follow a specific set of
> > > > > rules
> > > > > regarding frontbuffer management and mmapping. I recently
> > > > > discovered
> > > > > that current user space is not exactly following these rules:
> > > > > my
> > > > > investigation led me to the conclusion that the generic backend
> > > > > from
> > > > > SNA - used by SKL and the other new platforms without a
> > > > > specific
> > > > > backend - is not issuing sw_finish/dirty_fb IOCTLs when using
> > > > > the
> > > > > CPU
> > > > > and WC mmaps. I discovered this when running lightdm: I would
> > > > > type
> > > > > the
> > > > > password and nothing would appear on the screen unless I moved
> > > > > the
> > > > > mouse over the place where the letters were supposed to appear.
> > > > Yes, that is a kernel bug. The protocol we said the kernel would
> > > > follow
> > > > is to disable FBC/WC when userspace marks the object for writing
> > > > by
> > > > the
> > > > CPU and would only reestablish FBC/WC upon dirtyfb.
> > > But on WC mmaps we mark the object for writing by the GTT instead
> > > of
> > > the CPU, and while the tracking engine is able to see "normal" GTT
> > > mmap
> > > writes, it's not able to see WC mmap writes, so we established that
> > > we'd call dirtyfb after frontbuffer drawing through WC mmaps, which
> > > is
> > > something that the DDX never implemented. This was discussed on
> > > #intel-
> > > gfx on Nov 5 2014, and also possibly other places, but I can't find
> > > the
> > > logs. Daniel also confirmed this to me again on private IRC on Jun
> > > 16
> > > 2015. So I still don't understand why this is a Kernel bug instead
> > > of a
> > > DDX bug.
> > Because we said that once invalidated, it would not be restored until
> > dirtyfb. The kernel is not doing that. Your patch does not do that.
> > To
> > be even close, you should be setting the origin flag based on the
> > existence
> > of wc mmaping for the object inside set-to-gtt-domain. Otherwise, you
> > are not implementing even close to the protocol you say you are. That
> > is
> > invalidate on set-domain, flush on dirtyfb.
> 
> I don't recall this being said in the earlier conversations, but now
> that you point it, it can be done. Also, we recently pinged/emailed you
> many times about this problem, so I wonder why it took you so long to
> point this...

All that needed to be said had been, I felt I would have been repeating
myself and earlier discussions.

> > The kernel's bug is that is not cancelling FBC. Userspace's bug is
> > not
> > signalling when to reenable it.
> 
> So at least you agree user space was missing something :)

Yes. It has sat there waiting for the missing piece to be added for 3
years. (Almost 3 years, 2 years 9 months.)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/4] drm/i915/fbc: enable FBC on gen 9+ too

[Paulo Zanoni]

Now that we're more protected against user space doing frontbuffer
mmap rendering, the last - how many times did I say this before? -
SKL problem seems to be solved. So let's give it a try.

And since the other newer platforms are still under preliminary
support, let's enable FBC on them too to make sure we fix FBC on them.
Also, it seems KBL is passing the tests.

If you reached this commit through git bisect or if you just want more
information about FBC, please see:
    commit a98ee79317b4091cafb502b4ffdbbbe1335e298c
    Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
    Date:   Tue Feb 16 18:47:21 2016 -0200
        drm/i915/fbc: enable FBC by default on HSW and BDW

v2: Enable not only on SKL, but for everything new (Daniel).

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_fbc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 718ac38..614f9e5 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -1270,7 +1270,8 @@ void intel_fbc_init(struct drm_i915_private *dev_priv)
 	 * know what's going on. */
 	if (i915.enable_fbc < 0) {
 		i915.enable_fbc = IS_HASWELL(dev_priv) ||
-				  IS_BROADWELL(dev_priv);
+				  IS_BROADWELL(dev_priv) ||
+				  INTEL_INFO(dev_priv)->gen >= 9;
 		DRM_DEBUG_KMS("Sanitized enable_fbc value: %d\n",
 			      i915.enable_fbc);
 	}
-- 
2.7.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for sna: Opt-out of the Kernel mmap workaround

[Patchwork]

== Series Details ==

Series: sna: Opt-out of the Kernel mmap workaround
URL   : https://patchwork.freedesktop.org/series/4879/
State : failure

== Summary ==

Series 4879v1 sna: Opt-out of the Kernel mmap workaround
http://patchwork.freedesktop.org/api/1.0/series/4879/revisions/1/mbox/

Test gem_exec_whisper:
        Subgroup basic:
                pass       -> FAIL       (ivb-t430s)
Test kms_frontbuffer_tracking:
        Subgroup basic:
                pass       -> FAIL       (hsw-gt2)
Test pm_rpm:
        Subgroup basic-pci-d3-state:
                pass       -> DMESG-WARN (bsw-nuc-2)
        Subgroup basic-rte:
                dmesg-warn -> PASS       (byt-nuc) UNSTABLE

bdw-nuci7        total:192  pass:179  dwarn:0   dfail:0   fail:1   skip:12 
bdw-ultra        total:192  pass:170  dwarn:0   dfail:0   fail:1   skip:21 
bsw-nuc-2        total:192  pass:154  dwarn:1   dfail:0   fail:0   skip:37 
byt-nuc          total:192  pass:157  dwarn:0   dfail:0   fail:0   skip:35 
hsw-brixbox      total:192  pass:170  dwarn:0   dfail:0   fail:0   skip:22 
hsw-gt2          total:192  pass:174  dwarn:0   dfail:0   fail:1   skip:17 
ivb-t430s        total:192  pass:166  dwarn:0   dfail:0   fail:1   skip:25 
skl-i7k-2        total:192  pass:169  dwarn:0   dfail:0   fail:0   skip:23 
snb-dellxps      total:192  pass:158  dwarn:0   dfail:0   fail:0   skip:34 
snb-x220t        total:192  pass:158  dwarn:0   dfail:0   fail:1   skip:33 

Results at /archive/results/CI_IGT_test/Patchwork_1718/

f5d413cccefa1f93d64c34f357151d42add63a84 drm-intel-nightly: 2016y-03m-24d-14h-34m-29s UTC integration manifest
45b98318dcf57f54457c65caa0bad4485fd587df drm/i915/fbc: enable FBC on gen 9+ too
f19ee52a69b04e3bd7d861abf47b5a702aaf090b drm/i915: opt-out CPU and WC mmaps from FBC
8769c5983b6e33978226b0d84316f4f697c9ccff drm/i915/fbc: sanitize i915.enable_fbc during FBC init
417a12f1c1e47742fb21c15455fd5e27de0fdb8a drm/i915/fbc: update busy_bits even for GTT and flip flushes

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx