[PATCH 0/4] Enable FBC on SKL

[PATCH 0/4] Enable FBC on SKL

[Paulo Zanoni]

Hi

Here's a patch series to workaround the current SKL FBC problems, then try to
enable it by default.

Between patch 2/4 and the DDX patch, we only need one of them for things to
work. We'll also be fine if we merge both, but we need at least one.

No IGT changes are needed due to the automatic workaround disable during
dirtyfb/sw_finish.

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 SKL too

 drivers/gpu/drm/i915/i915_drv.h          |  9 +++++
 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 | 31 +++++++++++++++++
 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.

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

[RFC xf86-video-intel] sna: Call dirtyfb for all non-tear-free cases

[Paulo Zanoni]

The sna_mode_wants_tear_free() function tries to detect FBC and PSR
based on Kernel module parameters. Currently it fails to detect FBC
due to the default enable_fbc value being -1. While this can easily be
fixed in the Kernel, I had a conversation with Daniel and he expressed
unhappiness with that solution, claiming that yet another different
code path just for a feature that should be transparent is not a good
way to go, and that we should do proper frontbuffer rendering.

So with this patch, we'll have the DDX issuing dirtyfb calls even if
TearFree is not enabled, fixing FBC when i915.enable_fbc=-1.

This fixes a bug that happens on SKL with FBC enabled: if you run
lightdm, your login/password won't appear as you type on your
keyboard. You have to move the mouse over the input box for them to be
displayed.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 src/sna/sna_driver.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/sna/sna_driver.c b/src/sna/sna_driver.c
index b245594..84e8e55 100644
--- a/src/sna/sna_driver.c
+++ b/src/sna/sna_driver.c
@@ -654,7 +654,7 @@ static Bool sna_pre_init(ScrnInfoPtr scrn, int probe)
 	}
 	scrn->currentMode = scrn->modes;
 
-	if (!setup_tear_free(sna) && sna_mode_wants_tear_free(sna))
+	if (!setup_tear_free(sna))
 		sna->kgem.needs_dirtyfb = sna->kgem.has_dirtyfb;
 
 	xf86SetGamma(scrn, zeros);
-- 
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.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h          |  9 +++++++++
 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 | 31 ++++++++++++++++++++++++++++++
 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 efca534..45e31d2 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -873,6 +873,13 @@ enum fb_op_origin {
 	ORIGIN_DIRTYFB,
 };
 
+enum fb_mmap_wa_flags {
+	FB_MMAP_WA_CPU =	1 << 0,
+	FB_MMAP_WA_GTT =	1 << 1,
+	FB_MMAP_WA_DISABLE = 	1 << 2,
+	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. */
@@ -910,6 +917,7 @@ struct intel_fbc {
 			unsigned int stride;
 			int fence_reg;
 			unsigned int tiling_mode;
+			unsigned int mmap_wa_flags;
 		} fb;
 	} state_cache;
 
@@ -2143,6 +2151,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 8588c83..d44f27e 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 28ead66..6e7aa9e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14705,6 +14705,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 ba45245..bbea7c4 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1082,6 +1082,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,
@@ -1371,6 +1372,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..8d9b327 100644
--- a/drivers/gpu/drm/i915/intel_frontbuffer.c
+++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
@@ -241,3 +241,34 @@ 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
+ *
+ * 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_GEN9(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

[PATCH 4/4] drm/i915/fbc: enable FBC on SKL 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.

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

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..67f8810 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) ||
+				  IS_SKYLAKE(dev_priv);
 		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

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 18690 bytes --]

Hi Paulo,

[auto build test WARNING on drm-intel/for-linux-next]
[also build test WARNING on next-20160321]
[cannot apply to v4.5]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Paulo-Zanoni/Enable-FBC-on-SKL/20160322-045829
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
reproduce: make htmldocs

All warnings (new ones prefixed by >>):

   include/drm/drm_crtc.h:776: warning: No description found for parameter 'name'
   include/drm/drm_crtc.h:1235: warning: No description found for parameter 'connector_id'
   include/drm/drm_crtc.h:1235: warning: No description found for parameter 'tile_blob_ptr'
   include/drm/drm_crtc.h:1274: warning: No description found for parameter 'rotation'
   include/drm/drm_crtc.h:1536: warning: No description found for parameter 'name'
   include/drm/drm_crtc.h:1536: warning: No description found for parameter 'mutex'
   include/drm/drm_crtc.h:1536: warning: No description found for parameter 'helper_private'
   include/drm/drm_crtc.h:2146: warning: No description found for parameter 'tile_idr'
   include/drm/drm_crtc.h:2146: warning: No description found for parameter 'connector_ida'
   include/drm/drm_crtc.h:2146: warning: No description found for parameter 'delayed_event'
   include/drm/drm_crtc.h:2146: warning: No description found for parameter 'edid_property'
   include/drm/drm_crtc.h:2146: warning: No description found for parameter 'dpms_property'
   include/drm/drm_crtc.h:2146: warning: No description found for parameter 'path_property'
   include/drm/drm_crtc.h:2146: warning: No description found for parameter 'tile_property'
   include/drm/drm_crtc.h:2146: warning: No description found for parameter 'plane_type_property'
   include/drm/drm_crtc.h:2146: warning: No description found for parameter 'rotation_property'
   include/drm/drm_crtc.h:2146: warning: No description found for parameter 'prop_src_x'
   include/drm/drm_crtc.h:2146: warning: No description found for parameter 'prop_src_y'
   include/drm/drm_crtc.h:2146: warning: No description found for parameter 'prop_src_w'
   include/drm/drm_crtc.h:2146: warning: No description found for parameter 'prop_src_h'
   include/drm/drm_crtc.h:2146: warning: No description found for parameter 'prop_crtc_x'
   include/drm/drm_crtc.h:2146: warning: No description found for parameter 'prop_crtc_y'
   include/drm/drm_crtc.h:2146: warning: No description found for parameter 'prop_crtc_w'
   include/drm/drm_crtc.h:2146: warning: No description found for parameter 'prop_crtc_h'
   include/drm/drm_crtc.h:2146: warning: No description found for parameter 'prop_fb_id'
   include/drm/drm_crtc.h:2146: warning: No description found for parameter 'prop_crtc_id'
   include/drm/drm_crtc.h:2146: warning: No description found for parameter 'prop_active'
   include/drm/drm_crtc.h:2146: warning: No description found for parameter 'prop_mode_id'
   include/drm/drm_crtc.h:2146: warning: No description found for parameter 'dvi_i_subconnector_property'
   include/drm/drm_crtc.h:2146: warning: No description found for parameter 'dvi_i_select_subconnector_property'
   include/drm/drm_crtc.h:2146: warning: No description found for parameter 'tv_subconnector_property'
   include/drm/drm_crtc.h:2146: warning: No description found for parameter 'tv_select_subconnector_property'
   include/drm/drm_crtc.h:2146: warning: No description found for parameter 'tv_mode_property'
   include/drm/drm_crtc.h:2146: warning: No description found for parameter 'tv_left_margin_property'
   include/drm/drm_crtc.h:2146: warning: No description found for parameter 'tv_right_margin_property'
   include/drm/drm_crtc.h:2146: warning: No description found for parameter 'tv_top_margin_property'
   include/drm/drm_crtc.h:2146: warning: No description found for parameter 'tv_bottom_margin_property'
   include/drm/drm_crtc.h:2146: warning: No description found for parameter 'tv_brightness_property'
   include/drm/drm_crtc.h:2146: warning: No description found for parameter 'tv_contrast_property'
   include/drm/drm_crtc.h:2146: warning: No description found for parameter 'tv_flicker_reduction_property'
   include/drm/drm_crtc.h:2146: warning: No description found for parameter 'tv_overscan_property'
   include/drm/drm_crtc.h:2146: warning: No description found for parameter 'tv_saturation_property'
   include/drm/drm_crtc.h:2146: warning: No description found for parameter 'tv_hue_property'
   include/drm/drm_crtc.h:2146: warning: No description found for parameter 'scaling_mode_property'
   include/drm/drm_crtc.h:2146: warning: No description found for parameter 'aspect_ratio_property'
   include/drm/drm_crtc.h:2146: warning: No description found for parameter 'dirty_info_property'
   include/drm/drm_crtc.h:2146: warning: No description found for parameter 'suggested_x_property'
   include/drm/drm_crtc.h:2146: warning: No description found for parameter 'suggested_y_property'
   include/drm/drm_crtc.h:2146: warning: No description found for parameter 'allow_fb_modifiers'
   include/drm/drm_dp_helper.h:749: warning: No description found for parameter 'i2c_nack_count'
   include/drm/drm_dp_helper.h:749: warning: No description found for parameter 'i2c_defer_count'
   drivers/gpu/drm/drm_dp_mst_topology.c:2356: warning: No description found for parameter 'connector'
   include/drm/drm_dp_mst_helper.h:92: warning: No description found for parameter 'cached_edid'
   include/drm/drm_dp_mst_helper.h:92: warning: No description found for parameter 'has_audio'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'max_dpcd_transaction_bytes'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'sink_count'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'total_slots'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'avail_slots'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'total_pbn'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'qlock'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'tx_msg_downq'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'tx_down_in_progress'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'payload_lock'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'proposed_vcpis'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'payloads'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'payload_mask'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'vcpi_mask'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'tx_waitq'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'work'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'tx_work'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'destroy_connector_list'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'destroy_connector_lock'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'destroy_connector_work'
   drivers/gpu/drm/drm_dp_mst_topology.c:2356: warning: No description found for parameter 'connector'
   drivers/gpu/drm/drm_irq.c:176: warning: No description found for parameter 'flags'
   include/drm/drmP.h:168: warning: No description found for parameter 'fmt'
   include/drm/drmP.h:184: warning: No description found for parameter 'fmt'
   include/drm/drmP.h:202: warning: No description found for parameter 'fmt'
   include/drm/drmP.h:247: warning: No description found for parameter 'dev'
   include/drm/drmP.h:247: warning: No description found for parameter 'data'
   include/drm/drmP.h:247: warning: No description found for parameter 'file_priv'
   include/drm/drmP.h:280: warning: No description found for parameter 'ioctl'
   include/drm/drmP.h:280: warning: No description found for parameter '_func'
   include/drm/drmP.h:280: warning: No description found for parameter '_flags'
   include/drm/drmP.h:362: warning: cannot understand function prototype: 'struct drm_lock_data '
   include/drm/drmP.h:415: warning: cannot understand function prototype: 'struct drm_driver '
   include/drm/drmP.h:672: warning: cannot understand function prototype: 'struct drm_info_list '
   include/drm/drmP.h:682: warning: cannot understand function prototype: 'struct drm_info_node '
   include/drm/drmP.h:692: warning: cannot understand function prototype: 'struct drm_minor '
   include/drm/drmP.h:740: warning: cannot understand function prototype: 'struct drm_device '
   drivers/gpu/drm/i915/intel_runtime_pm.c:2286: warning: No description found for parameter 'resume'
   drivers/gpu/drm/i915/intel_runtime_pm.c:2286: warning: No description found for parameter 'resume'
   drivers/gpu/drm/i915/i915_irq.c:2665: warning: No description found for parameter 'wedged'
   drivers/gpu/drm/i915/i915_irq.c:2665: warning: No description found for parameter 'fmt'
   drivers/gpu/drm/i915/i915_irq.c:2665: warning: No description found for parameter 'wedged'
   drivers/gpu/drm/i915/i915_irq.c:2665: warning: No description found for parameter 'fmt'
   drivers/gpu/drm/i915/i915_irq.c:2665: warning: No description found for parameter 'wedged'
   drivers/gpu/drm/i915/i915_irq.c:2665: warning: No description found for parameter 'fmt'
   drivers/gpu/drm/i915/i915_irq.c:2665: warning: No description found for parameter 'wedged'
   drivers/gpu/drm/i915/i915_irq.c:2665: warning: No description found for parameter 'fmt'
>> drivers/gpu/drm/i915/intel_frontbuffer.c:261: warning: No description found for parameter 'flags'
>> drivers/gpu/drm/i915/intel_frontbuffer.c:261: warning: No description found for parameter 'flags'
   drivers/gpu/drm/i915/i915_gem.c:421: warning: No description found for parameter 'dev'
   drivers/gpu/drm/i915/i915_gem.c:421: warning: No description found for parameter 'data'
   drivers/gpu/drm/i915/i915_gem.c:421: warning: No description found for parameter 'file'
   drivers/gpu/drm/i915/i915_gem.c:686: warning: No description found for parameter 'dev'
   drivers/gpu/drm/i915/i915_gem.c:686: warning: No description found for parameter 'data'
   drivers/gpu/drm/i915/i915_gem.c:686: warning: No description found for parameter 'file'
   drivers/gpu/drm/i915/i915_gem.c:767: warning: No description found for parameter 'dev'
   drivers/gpu/drm/i915/i915_gem.c:767: warning: No description found for parameter 'obj'
   drivers/gpu/drm/i915/i915_gem.c:767: warning: No description found for parameter 'args'
   drivers/gpu/drm/i915/i915_gem.c:767: warning: No description found for parameter 'file'
   drivers/gpu/drm/i915/i915_gem.c:1029: warning: No description found for parameter 'dev'
   drivers/gpu/drm/i915/i915_gem.c:1029: warning: No description found for parameter 'data'
   drivers/gpu/drm/i915/i915_gem.c:1029: warning: No description found for parameter 'file'
   drivers/gpu/drm/i915/i915_gem.c:1245: warning: No description found for parameter 'rps'
   drivers/gpu/drm/i915/i915_gem.c:1461: warning: No description found for parameter 'req'
   drivers/gpu/drm/i915/i915_gem.c:1496: warning: No description found for parameter 'obj'
   drivers/gpu/drm/i915/i915_gem.c:1496: warning: No description found for parameter 'readonly'
   drivers/gpu/drm/i915/i915_gem.c:1619: warning: No description found for parameter 'dev'
   drivers/gpu/drm/i915/i915_gem.c:1619: warning: No description found for parameter 'data'
   drivers/gpu/drm/i915/i915_gem.c:1619: warning: No description found for parameter 'file'
   drivers/gpu/drm/i915/i915_gem.c:1682: warning: No description found for parameter 'dev'
   drivers/gpu/drm/i915/i915_gem.c:1682: warning: No description found for parameter 'data'
   drivers/gpu/drm/i915/i915_gem.c:1682: warning: No description found for parameter 'file'
   drivers/gpu/drm/i915/i915_gem.c:1729: warning: No description found for parameter 'dev'
   drivers/gpu/drm/i915/i915_gem.c:1729: warning: No description found for parameter 'data'
   drivers/gpu/drm/i915/i915_gem.c:1729: warning: No description found for parameter 'file'
   drivers/gpu/drm/i915/i915_gem.c:2019: warning: No description found for parameter 'dev'
   drivers/gpu/drm/i915/i915_gem.c:2019: warning: No description found for parameter 'size'
   drivers/gpu/drm/i915/i915_gem.c:2019: warning: No description found for parameter 'tiling_mode'
   drivers/gpu/drm/i915/i915_gem.c:2019: warning: No description found for parameter 'fenced'
   drivers/gpu/drm/i915/i915_gem.c:2019: warning: Excess function parameter 'obj' description in 'i915_gem_get_gtt_alignment'
   drivers/gpu/drm/i915/i915_gem.c:2919: warning: No description found for parameter 'ring'
   drivers/gpu/drm/i915/i915_gem.c:3050: warning: No description found for parameter 'obj'
   drivers/gpu/drm/i915/i915_gem.c:3100: warning: No description found for parameter 'dev'
   drivers/gpu/drm/i915/i915_gem.c:3100: warning: No description found for parameter 'data'
   drivers/gpu/drm/i915/i915_gem.c:3100: warning: No description found for parameter 'file'
   drivers/gpu/drm/i915/i915_gem.c:3100: warning: Excess function parameter 'DRM_IOCTL_ARGS' description in 'i915_gem_wait_ioctl'
   drivers/gpu/drm/i915/i915_gem.c:3472: warning: No description found for parameter 'obj'
   drivers/gpu/drm/i915/i915_gem.c:3472: warning: No description found for parameter 'vm'
   drivers/gpu/drm/i915/i915_gem.c:3472: warning: No description found for parameter 'ggtt_view'
   drivers/gpu/drm/i915/i915_gem.c:3472: warning: No description found for parameter 'alignment'
   drivers/gpu/drm/i915/i915_gem.c:3472: warning: No description found for parameter 'flags'
   drivers/gpu/drm/i915/i915_gem.c:3727: warning: No description found for parameter 'obj'
   drivers/gpu/drm/i915/i915_gem.c:3727: warning: No description found for parameter 'write'
   drivers/gpu/drm/i915/i915_gem.c:3802: warning: No description found for parameter 'obj'
   drivers/gpu/drm/i915/i915_gem.c:3802: warning: No description found for parameter 'cache_level'
   drivers/gpu/drm/i915/i915_gem.c:4076: warning: No description found for parameter 'obj'
   drivers/gpu/drm/i915/i915_gem.c:4076: warning: No description found for parameter 'write'
   drivers/gpu/drm/i915/intel_lrc.c:936: warning: No description found for parameter 'params'
   drivers/gpu/drm/i915/intel_lrc.c:936: warning: Excess function parameter 'dev' description in 'intel_execlists_submission'
   drivers/gpu/drm/i915/intel_lrc.c:936: warning: Excess function parameter 'file' description in 'intel_execlists_submission'
   drivers/gpu/drm/i915/intel_lrc.c:936: warning: Excess function parameter 'ring' description in 'intel_execlists_submission'
   drivers/gpu/drm/i915/intel_lrc.c:936: warning: Excess function parameter 'ctx' description in 'intel_execlists_submission'
   drivers/gpu/drm/i915/intel_lrc.c:936: warning: Excess function parameter 'batch_obj' description in 'intel_execlists_submission'
   drivers/gpu/drm/i915/intel_lrc.c:936: warning: Excess function parameter 'exec_start' description in 'intel_execlists_submission'
   drivers/gpu/drm/i915/intel_lrc.c:936: warning: Excess function parameter 'dispatch_flags' description in 'intel_execlists_submission'
   drivers/gpu/drm/i915/intel_lrc.c:936: warning: No description found for parameter 'params'
   drivers/gpu/drm/i915/intel_lrc.c:936: warning: Excess function parameter 'dev' description in 'intel_execlists_submission'
   drivers/gpu/drm/i915/intel_lrc.c:936: warning: Excess function parameter 'file' description in 'intel_execlists_submission'
   drivers/gpu/drm/i915/intel_lrc.c:936: warning: Excess function parameter 'ring' description in 'intel_execlists_submission'
   drivers/gpu/drm/i915/intel_lrc.c:936: warning: Excess function parameter 'ctx' description in 'intel_execlists_submission'
   drivers/gpu/drm/i915/intel_lrc.c:936: warning: Excess function parameter 'batch_obj' description in 'intel_execlists_submission'
   drivers/gpu/drm/i915/intel_lrc.c:936: warning: Excess function parameter 'exec_start' description in 'intel_execlists_submission'
   drivers/gpu/drm/i915/intel_lrc.c:936: warning: Excess function parameter 'dispatch_flags' description in 'intel_execlists_submission'
   Warning: didn't use docs for i915_hotplug_interrupt_update
   Warning: didn't use docs for ilk_update_display_irq
   Warning: didn't use docs for ilk_update_gt_irq
   Warning: didn't use docs for snb_update_pm_irq
   Warning: didn't use docs for bdw_update_port_irq
   Warning: didn't use docs for bdw_update_pipe_irq
   Warning: didn't use docs for ibx_display_interrupt_update
   Warning: didn't use docs for i915_enable_asle_pipestat
   Warning: didn't use docs for ivybridge_parity_work
   Warning: didn't use docs for i915_reset_and_wakeup
   Warning: didn't use docs for i915_handle_error
   Warning: didn't use docs for intel_irq_install
   Warning: didn't use docs for intel_irq_uninstall

vim +/flags +261 drivers/gpu/drm/i915/intel_frontbuffer.c

   245	/**
   246	 * intel_fb_obj_mmap_wa - notifies about objects being mmapped
   247	 * @obj: GEM object being mmapped
   248	 *
   249	 * This function gets called whenever an object gets mmapped. Not every user
   250	 * space application follows the protocol assumed by the frontbuffer tracking
   251	 * subsystem when it was created, so this mmap notify callback can be used to
   252	 * completely disable frontbuffer features such as FBC and PSR. Even if at some
   253	 * point we fix ever user space application, there's still the possibility that
   254	 * the user may have a new Kernel with the old user space.
   255	 *
   256	 * Also notice that there's no munmap API because user space calls munmap()
   257	 * directly. Even if we had, it probably wouldn't help since munmap() calls are
   258	 * not common.
   259	 */
   260	void intel_fb_obj_mmap_wa(struct drm_i915_gem_object *obj, unsigned int flags)
 > 261	{
   262		struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
   263	
   264		if (!IS_GEN9(dev_priv))
   265			return;
   266	
   267		obj->fb_mmap_wa_flags |= flags;
   268	
   269		if (!obj->frontbuffer_bits)

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 6240 bytes --]

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
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

[Jani Nikula]

On Mon, 21 Mar 2016, Paulo Zanoni <paulo.r.zanoni@intel.com> wrote:
> +enum fb_mmap_wa_flags {
> +	FB_MMAP_WA_CPU =	1 << 0,
> +	FB_MMAP_WA_GTT =	1 << 1,
> +	FB_MMAP_WA_DISABLE = 	1 << 2,
> +	FB_MMAP_WA_FLAG_COUNT =	3,
> +};

Drive-by review, adding bit flags as enums doesn't feel like what enums
should be used for. I'd go for macros instead.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

On Mon, Mar 21, 2016 at 04:26:54PM -0300, Paulo Zanoni wrote:
> 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.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Is this covered by your nasty igt test suite? Kernel side looks good, so
with appropriate Testcase: tag added:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

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

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

[Daniel Vetter]

On Tue, Mar 22, 2016 at 12:28:20PM +0200, Jani Nikula wrote:
> On Mon, 21 Mar 2016, Paulo Zanoni <paulo.r.zanoni@intel.com> wrote:
> > +enum fb_mmap_wa_flags {
> > +	FB_MMAP_WA_CPU =	1 << 0,
> > +	FB_MMAP_WA_GTT =	1 << 1,
> > +	FB_MMAP_WA_DISABLE = 	1 << 2,
> > +	FB_MMAP_WA_FLAG_COUNT =	3,
> > +};
> 
> Drive-by review, adding bit flags as enums doesn't feel like what enums
> should be used for. I'd go for macros instead.

Pretty established convention, some like it since it allows gdb&friends to
decode your bitflags for you. So abusing enums as flag set is imo ok.
-Daniel
-- 
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 4/4] drm/i915/fbc: enable FBC on SKL too

[Daniel Vetter]

On Mon, Mar 21, 2016 at 04:26:58PM -0300, Paulo Zanoni wrote:
> 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.
> 
> 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
> 
> 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..67f8810 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) ||
> +				  IS_SKYLAKE(dev_priv);

Can we just future-proof this and enable on everything gen8+ where we have
fbc? Apparently bsw/bxt simply lack this ...
-Daniel

>  		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

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

[Daniel Vetter]

On Mon, Mar 21, 2016 at 04:26:57PM -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.
> 
> 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.
> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h          |  9 +++++++++
>  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 | 31 ++++++++++++++++++++++++++++++
>  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 efca534..45e31d2 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -873,6 +873,13 @@ enum fb_op_origin {
>  	ORIGIN_DIRTYFB,
>  };
>  
> +enum fb_mmap_wa_flags {
> +	FB_MMAP_WA_CPU =	1 << 0,
> +	FB_MMAP_WA_GTT =	1 << 1,
> +	FB_MMAP_WA_DISABLE = 	1 << 2,
> +	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. */
> @@ -910,6 +917,7 @@ struct intel_fbc {
>  			unsigned int stride;
>  			int fence_reg;
>  			unsigned int tiling_mode;
> +			unsigned int mmap_wa_flags;
>  		} fb;
>  	} state_cache;
>  
> @@ -2143,6 +2151,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 8588c83..d44f27e 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 28ead66..6e7aa9e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14705,6 +14705,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 ba45245..bbea7c4 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1082,6 +1082,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,
> @@ -1371,6 +1372,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..8d9b327 100644
> --- a/drivers/gpu/drm/i915/intel_frontbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
> @@ -241,3 +241,34 @@ 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
> + *
> + * 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_GEN9(dev_priv))

Please only IS_SKYLAKE, I don't want to extend this to either bxt or kbl,
and both are still under prelim_hw_support and not yet shipping, i.e. we
can break them ;-)

Wrt the implementation: I think it would be great to get PSR on board to
avoid duplicating the logic. One option that might would be to create an
invlidate on skl with ORIGIN_MMAP_WA, and as long as that's in effect
(until the first sw_finish or dirty_fb) the frontbuffer tracking code
itself would filter all flushes. fbc could then use ORIGIN_MMAP_WA
invalidate to permanently disable (well, not re-enable after flip) until
it sees a flush. And PSR would Just Work (I hope).

Cheers, Daniel

> +		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
> 

-- 
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: [RFC xf86-video-intel] sna: Call dirtyfb for all non-tear-free cases

[Daniel Vetter]

On Mon, Mar 21, 2016 at 04:26:55PM -0300, Paulo Zanoni wrote:
> The sna_mode_wants_tear_free() function tries to detect FBC and PSR
> based on Kernel module parameters. Currently it fails to detect FBC
> due to the default enable_fbc value being -1. While this can easily be
> fixed in the Kernel, I had a conversation with Daniel and he expressed
> unhappiness with that solution, claiming that yet another different
> code path just for a feature that should be transparent is not a good
> way to go, and that we should do proper frontbuffer rendering.
> 
> So with this patch, we'll have the DDX issuing dirtyfb calls even if
> TearFree is not enabled, fixing FBC when i915.enable_fbc=-1.
> 
> This fixes a bug that happens on SKL with FBC enabled: if you run
> lightdm, your login/password won't appear as you type on your
> keyboard. You have to move the mouse over the input box for them to be
> displayed.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

I thought we need this anyway to get the kernel to allow fbc, since SNA
ends up mmap some of the drm_framebuffer. Even when they're not
frontbuffers.
-Daniel

> ---
>  src/sna/sna_driver.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/sna/sna_driver.c b/src/sna/sna_driver.c
> index b245594..84e8e55 100644
> --- a/src/sna/sna_driver.c
> +++ b/src/sna/sna_driver.c
> @@ -654,7 +654,7 @@ static Bool sna_pre_init(ScrnInfoPtr scrn, int probe)
>  	}
>  	scrn->currentMode = scrn->modes;
>  
> -	if (!setup_tear_free(sna) && sna_mode_wants_tear_free(sna))
> +	if (!setup_tear_free(sna))
>  		sna->kgem.needs_dirtyfb = sna->kgem.has_dirtyfb;
>  
>  	xf86SetGamma(scrn, zeros);
> -- 
> 2.7.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
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: [RFC xf86-video-intel] sna: Call dirtyfb for all non-tear-free cases

[Chris Wilson]

On Tue, Mar 22, 2016 at 12:31:09PM +0100, Daniel Vetter wrote:
> On Mon, Mar 21, 2016 at 04:26:55PM -0300, Paulo Zanoni wrote:
> > The sna_mode_wants_tear_free() function tries to detect FBC and PSR
> > based on Kernel module parameters. Currently it fails to detect FBC
> > due to the default enable_fbc value being -1. While this can easily be
> > fixed in the Kernel, I had a conversation with Daniel and he expressed
> > unhappiness with that solution, claiming that yet another different
> > code path just for a feature that should be transparent is not a good
> > way to go, and that we should do proper frontbuffer rendering.
> > 
> > So with this patch, we'll have the DDX issuing dirtyfb calls even if
> > TearFree is not enabled, fixing FBC when i915.enable_fbc=-1.
> > 
> > This fixes a bug that happens on SKL with FBC enabled: if you run
> > lightdm, your login/password won't appear as you type on your
> > keyboard. You have to move the mouse over the input box for them to be
> > displayed.
> > 
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> I thought we need this anyway to get the kernel to allow fbc, since SNA
> ends up mmap some of the drm_framebuffer. Even when they're not
> frontbuffers.

Nope. FBC => sna_mode_wants_tear_free() is true.
-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

[Jani Nikula]

On Tue, 22 Mar 2016, Daniel Vetter <daniel@ffwll.ch> wrote:
> [ text/plain ]
> On Tue, Mar 22, 2016 at 12:28:20PM +0200, Jani Nikula wrote:
>> On Mon, 21 Mar 2016, Paulo Zanoni <paulo.r.zanoni@intel.com> wrote:
>> > +enum fb_mmap_wa_flags {
>> > +	FB_MMAP_WA_CPU =	1 << 0,
>> > +	FB_MMAP_WA_GTT =	1 << 1,
>> > +	FB_MMAP_WA_DISABLE = 	1 << 2,
>> > +	FB_MMAP_WA_FLAG_COUNT =	3,
>> > +};
>> 
>> Drive-by review, adding bit flags as enums doesn't feel like what enums
>> should be used for. I'd go for macros instead.
>
> Pretty established convention, some like it since it allows gdb&friends to
> decode your bitflags for you. So abusing enums as flag set is imo ok.

Bah, prevents GCC and friends from giving you sensible warnings on
switch cases etc. :p

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Zanoni, Paulo R]

Em Ter, 2016-03-22 às 12:13 +0100, Daniel Vetter escreveu:
> On Mon, Mar 21, 2016 at 04:26:54PM -0300, Paulo Zanoni wrote:
> > 
> > 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.
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Is this covered by your nasty igt test suite? Kernel side looks good,
> so
> with appropriate Testcase: tag added:

I mentioned the tests in the middle of the commit message, but forgot
the Testcase tags. I'll add them.

> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Thanks!

> 
> > 
> > ---
> >  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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC xf86-video-intel] sna: Call dirtyfb for all non-tear-free cases

[Zanoni, Paulo R]

Em Ter, 2016-03-22 às 12:31 +0100, Daniel Vetter escreveu:
> On Mon, Mar 21, 2016 at 04:26:55PM -0300, Paulo Zanoni wrote:
> > 
> > The sna_mode_wants_tear_free() function tries to detect FBC and PSR
> > based on Kernel module parameters. Currently it fails to detect FBC
> > due to the default enable_fbc value being -1. While this can easily
> > be
> > fixed in the Kernel, I had a conversation with Daniel and he
> > expressed
> > unhappiness with that solution, claiming that yet another different
> > code path just for a feature that should be transparent is not a
> > good
> > way to go, and that we should do proper frontbuffer rendering.
> > 
> > So with this patch, we'll have the DDX issuing dirtyfb calls even
> > if
> > TearFree is not enabled, fixing FBC when i915.enable_fbc=-1.
> > 
> > This fixes a bug that happens on SKL with FBC enabled: if you run
> > lightdm, your login/password won't appear as you type on your
> > keyboard. You have to move the mouse over the input box for them to
> > be
> > displayed.
> > 
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> I thought we need this anyway to get the kernel to allow fbc, since
> SNA
> ends up mmap some of the drm_framebuffer. Even when they're not
> frontbuffers.

If we merge patch 2/4, we won't need this one since TearFree will be in
use, and it seems TearFree doesn't touch frontbuffers, so we'll always
get the flush calls during page flips.

> -Daniel
> 
> > 
> > ---
> >  src/sna/sna_driver.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/src/sna/sna_driver.c b/src/sna/sna_driver.c
> > index b245594..84e8e55 100644
> > --- a/src/sna/sna_driver.c
> > +++ b/src/sna/sna_driver.c
> > @@ -654,7 +654,7 @@ static Bool sna_pre_init(ScrnInfoPtr scrn, int
> > probe)
> >  	}
> >  	scrn->currentMode = scrn->modes;
> >  
> > -	if (!setup_tear_free(sna) &&
> > sna_mode_wants_tear_free(sna))
> > +	if (!setup_tear_free(sna))
> >  		sna->kgem.needs_dirtyfb = sna->kgem.has_dirtyfb;
> >  
> >  	xf86SetGamma(scrn, zeros);
> > -- 
> > 2.7.0
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
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 Ter, 2016-03-22 às 12:29 +0100, Daniel Vetter escreveu:
> On Mon, Mar 21, 2016 at 04:26:57PM -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.
> > 
> > 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.
> > 
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h          |  9 +++++++++
> >  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 | 31
> > ++++++++++++++++++++++++++++++
> >  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 efca534..45e31d2 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -873,6 +873,13 @@ enum fb_op_origin {
> >  	ORIGIN_DIRTYFB,
> >  };
> >  
> > +enum fb_mmap_wa_flags {
> > +	FB_MMAP_WA_CPU =	1 << 0,
> > +	FB_MMAP_WA_GTT =	1 << 1,
> > +	FB_MMAP_WA_DISABLE = 	1 << 2,
> > +	FB_MMAP_WA_FLAG_COUNT =	3,
> > +};
> > +

I'll do the change to macros you/Jani mentioned in the other emails.

> >  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.
> > */
> > @@ -910,6 +917,7 @@ struct intel_fbc {
> >  			unsigned int stride;
> >  			int fence_reg;
> >  			unsigned int tiling_mode;
> > +			unsigned int mmap_wa_flags;
> >  		} fb;
> >  	} state_cache;
> >  
> > @@ -2143,6 +2151,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 8588c83..d44f27e 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 28ead66..6e7aa9e 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -14705,6 +14705,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 ba45245..bbea7c4 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1082,6 +1082,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,
> > @@ -1371,6 +1372,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..8d9b327 100644
> > --- a/drivers/gpu/drm/i915/intel_frontbuffer.c
> > +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
> > @@ -241,3 +241,34 @@ 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
> > + *
> > + * 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_GEN9(dev_priv))
> Please only IS_SKYLAKE, I don't want to extend this to either bxt or
> kbl,
> and both are still under prelim_hw_support and not yet shipping, i.e.
> we
> can break them ;-)

Mmmkay.

> 
> Wrt the implementation: I think it would be great to get PSR on board
> to
> avoid duplicating the logic.

The goal of this patch going through intel_frontbuffer.c instead of
directly to intel_fbc.c is exactly to allow for PSR to do something
similar.


>  One option that might would be to create an
> invlidate on skl with ORIGIN_MMAP_WA, and as long as that's in effect
> (until the first sw_finish or dirty_fb) the frontbuffer tracking code
> itself would filter all flushes.

That's an non-trivial distortion of the trivial invalidate/flush API. I
prefer having a special codepath for this special case instead of
adding second meanings to flush/invalidate.

Besides, even with the invalidate/flush API being simple, look at how
many times we already tweaked/broke/fixed those functions, both for PSR
and FBC.


>  fbc could then use ORIGIN_MMAP_WA
> invalidate to permanently disable (well, not re-enable after flip)
> until
> it sees a flush. And PSR would Just Work (I hope).

I think any PSR fixes related to this should be separate patches. I
have to confess I've just been studying the FBC part of the problem, so
I was waiting for Rodrigo's opinions here before any conclusions.
Anyway, we can always change this code (in this patch or later) to
better suit PSR.

> 
> Cheers, Daniel
> 
> > 
> > +		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 4/4] drm/i915/fbc: enable FBC on SKL too

[Zanoni, Paulo R]

Em Ter, 2016-03-22 às 12:16 +0100, Daniel Vetter escreveu:
> On Mon, Mar 21, 2016 at 04:26:58PM -0300, Paulo Zanoni wrote:
> > 
> > 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.
> > 
> > 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
> > 
> > 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..67f8810 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) ||
> > +				  IS_SKYLAKE(dev_priv);
> Can we just future-proof this and enable on everything gen8+ where we
> have
> fbc? Apparently bsw/bxt simply lack this ...

This can be done, but I'm not sure if it's a good idea, given FBC's
never-ending history of platform-specific workarounds. We'd force
people to have to have FBC working right from the start. Hmmm, that
could actually be a good thing, enforcing people to make features work.

> -Daniel
> 
> > 
> >  		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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC xf86-video-intel] sna: Call dirtyfb for all non-tear-free cases

[Daniel Vetter]

On Tue, Mar 22, 2016 at 09:35:36PM +0000, Zanoni, Paulo R wrote:
> Em Ter, 2016-03-22 às 12:31 +0100, Daniel Vetter escreveu:
> > On Mon, Mar 21, 2016 at 04:26:55PM -0300, Paulo Zanoni wrote:
> > > 
> > > The sna_mode_wants_tear_free() function tries to detect FBC and PSR
> > > based on Kernel module parameters. Currently it fails to detect FBC
> > > due to the default enable_fbc value being -1. While this can easily
> > > be
> > > fixed in the Kernel, I had a conversation with Daniel and he
> > > expressed
> > > unhappiness with that solution, claiming that yet another different
> > > code path just for a feature that should be transparent is not a
> > > good
> > > way to go, and that we should do proper frontbuffer rendering.
> > > 
> > > So with this patch, we'll have the DDX issuing dirtyfb calls even
> > > if
> > > TearFree is not enabled, fixing FBC when i915.enable_fbc=-1.
> > > 
> > > This fixes a bug that happens on SKL with FBC enabled: if you run
> > > lightdm, your login/password won't appear as you type on your
> > > keyboard. You have to move the mouse over the input box for them to
> > > be
> > > displayed.
> > > 
> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > I thought we need this anyway to get the kernel to allow fbc, since
> > SNA
> > ends up mmap some of the drm_framebuffer. Even when they're not
> > frontbuffers.
> 
> If we merge patch 2/4, we won't need this one since TearFree will be in
> use, and it seems TearFree doesn't touch frontbuffers, so we'll always
> get the flush calls during page flips.

I'd thought that TearFree would still do rendering with the cpu sometimes,
but only always to the back buffer. So we'd still have cpu mmaps, and
hence the kernel w/a would still potentially kick in and prevent fbc. So
that's not the case then, and we get fbc reliably with the kernel patch,
even without this patch?
-Daniel
-- 
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

[Daniel Vetter]

On Tue, Mar 22, 2016 at 09:48:00PM +0000, Zanoni, Paulo R wrote:
> Em Ter, 2016-03-22 às 12:29 +0100, Daniel Vetter escreveu:
> > On Mon, Mar 21, 2016 at 04:26:57PM -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.
> > > 
> > > 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.
> > > 
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.h          |  9 +++++++++
> > >  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 | 31
> > > ++++++++++++++++++++++++++++++
> > >  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 efca534..45e31d2 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -873,6 +873,13 @@ enum fb_op_origin {
> > >  	ORIGIN_DIRTYFB,
> > >  };
> > >  
> > > +enum fb_mmap_wa_flags {
> > > +	FB_MMAP_WA_CPU =	1 << 0,
> > > +	FB_MMAP_WA_GTT =	1 << 1,
> > > +	FB_MMAP_WA_DISABLE = 	1 << 2,
> > > +	FB_MMAP_WA_FLAG_COUNT =	3,
> > > +};
> > > +
> 
> I'll do the change to macros you/Jani mentioned in the other emails.
> 
> > >  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.
> > > */
> > > @@ -910,6 +917,7 @@ struct intel_fbc {
> > >  			unsigned int stride;
> > >  			int fence_reg;
> > >  			unsigned int tiling_mode;
> > > +			unsigned int mmap_wa_flags;
> > >  		} fb;
> > >  	} state_cache;
> > >  
> > > @@ -2143,6 +2151,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 8588c83..d44f27e 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 28ead66..6e7aa9e 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -14705,6 +14705,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 ba45245..bbea7c4 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -1082,6 +1082,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,
> > > @@ -1371,6 +1372,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..8d9b327 100644
> > > --- a/drivers/gpu/drm/i915/intel_frontbuffer.c
> > > +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
> > > @@ -241,3 +241,34 @@ 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
> > > + *
> > > + * 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_GEN9(dev_priv))
> > Please only IS_SKYLAKE, I don't want to extend this to either bxt or
> > kbl,
> > and both are still under prelim_hw_support and not yet shipping, i.e.
> > we
> > can break them ;-)
> 
> Mmmkay.
> 
> > 
> > Wrt the implementation: I think it would be great to get PSR on board
> > to
> > avoid duplicating the logic.
> 
> The goal of this patch going through intel_frontbuffer.c instead of
> directly to intel_fbc.c is exactly to allow for PSR to do something
> similar.
> 
> 
> >  One option that might would be to create an
> > invlidate on skl with ORIGIN_MMAP_WA, and as long as that's in effect
> > (until the first sw_finish or dirty_fb) the frontbuffer tracking code
> > itself would filter all flushes.
> 
> That's an non-trivial distortion of the trivial invalidate/flush API. I
> prefer having a special codepath for this special case instead of
> adding second meanings to flush/invalidate.
> 
> Besides, even with the invalidate/flush API being simple, look at how
> many times we already tweaked/broke/fixed those functions, both for PSR
> and FBC.

Hm good point. It would indeed not neatly fit into the existing
invalidate/flush scheme (which assumes that there's only ever one thing
that can access a buffer, not a permanent mmap that can always be used).
Agreed that having a special case makes sense.

> >  fbc could then use ORIGIN_MMAP_WA
> > invalidate to permanently disable (well, not re-enable after flip)
> > until
> > it sees a flush. And PSR would Just Work (I hope).
> 
> I think any PSR fixes related to this should be separate patches. I
> have to confess I've just been studying the FBC part of the problem, so
> I was waiting for Rodrigo's opinions here before any conclusions.
> Anyway, we can always change this code (in this patch or later) to
> better suit PSR.

I'd like to at least have an ack from Rrodigo, even better if he can hack
up something quickly. But yeah it's code, we can change it again ;-)
-Daniel
-- 
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 4/4] drm/i915/fbc: enable FBC on SKL too

[Daniel Vetter]

On Tue, Mar 22, 2016 at 09:51:34PM +0000, Zanoni, Paulo R wrote:
> Em Ter, 2016-03-22 às 12:16 +0100, Daniel Vetter escreveu:
> > On Mon, Mar 21, 2016 at 04:26:58PM -0300, Paulo Zanoni wrote:
> > > 
> > > 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.
> > > 
> > > 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
> > > 
> > > 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..67f8810 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) ||
> > > +				  IS_SKYLAKE(dev_priv);
> > Can we just future-proof this and enable on everything gen8+ where we
> > have
> > fbc? Apparently bsw/bxt simply lack this ...
> 
> This can be done, but I'm not sure if it's a good idea, given FBC's
> never-ending history of platform-specific workarounds. We'd force
> people to have to have FBC working right from the start. Hmmm, that
> could actually be a good thing, enforcing people to make features work.

Well we generally do the same in all other places too - we just enable
everything and smash all the patches in. Then testing/power-on happens,
and more fixes pile on top.

We can still disable something if it's completely broken. But I'd really
prefer if this is a concious decision that requires explicit action.
Otherwise I fear we'll "forget" FBC again :( And e.g. modesetting or
rendering also require tons of special cases on each new platform, we
don't disable them either. We just have one overall flag for the entire
driver, which I think should be good enough.
-Daniel
-- 
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: [RFC xf86-video-intel] sna: Call dirtyfb for all non-tear-free cases

[Zanoni, Paulo R]

Em Qua, 2016-03-23 às 09:50 +0100, Daniel Vetter escreveu:
> On Tue, Mar 22, 2016 at 09:35:36PM +0000, Zanoni, Paulo R wrote:
> > 
> > Em Ter, 2016-03-22 às 12:31 +0100, Daniel Vetter escreveu:
> > > 
> > > On Mon, Mar 21, 2016 at 04:26:55PM -0300, Paulo Zanoni wrote:
> > > > 
> > > > 
> > > > The sna_mode_wants_tear_free() function tries to detect FBC and
> > > > PSR
> > > > based on Kernel module parameters. Currently it fails to detect
> > > > FBC
> > > > due to the default enable_fbc value being -1. While this can
> > > > easily
> > > > be
> > > > fixed in the Kernel, I had a conversation with Daniel and he
> > > > expressed
> > > > unhappiness with that solution, claiming that yet another
> > > > different
> > > > code path just for a feature that should be transparent is not
> > > > a
> > > > good
> > > > way to go, and that we should do proper frontbuffer rendering.
> > > > 
> > > > So with this patch, we'll have the DDX issuing dirtyfb calls
> > > > even
> > > > if
> > > > TearFree is not enabled, fixing FBC when i915.enable_fbc=-1.
> > > > 
> > > > This fixes a bug that happens on SKL with FBC enabled: if you
> > > > run
> > > > lightdm, your login/password won't appear as you type on your
> > > > keyboard. You have to move the mouse over the input box for
> > > > them to
> > > > be
> > > > displayed.
> > > > 
> > > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > I thought we need this anyway to get the kernel to allow fbc,
> > > since
> > > SNA
> > > ends up mmap some of the drm_framebuffer. Even when they're not
> > > frontbuffers.
> > If we merge patch 2/4, we won't need this one since TearFree will
> > be in
> > use, and it seems TearFree doesn't touch frontbuffers, so we'll
> > always
> > get the flush calls during page flips.
> I'd thought that TearFree would still do rendering with the cpu
> sometimes,
> but only always to the back buffer. So we'd still have cpu mmaps, and
> hence the kernel w/a would still potentially kick in and prevent fbc.
> So
> that's not the case then, and we get fbc reliably with the kernel
> patch,
> even without this patch?

Oh, you're right. But at least we won't have bugs, and we'll have FBC
on things like xf86-video-modesetting, which some distros seem to
already be using for SKL.

Well, now that I think a little more about this, maybe we could add
little hack to DDX so it does a single sw_finish/dirty_fb call to
TearFree framebuffers upon creation so the workaround gets disabled...
Hmm..

> -Daniel
_______________________________________________
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

[Vivi, Rodrigo]

On Wed, 2016-03-23 at 09:53 +0100, Daniel Vetter wrote:
> On Tue, Mar 22, 2016 at 09:48:00PM +0000, Zanoni, Paulo R wrote:
> > Em Ter, 2016-03-22 às 12:29 +0100, Daniel Vetter escreveu:
> > > On Mon, Mar 21, 2016 at 04:26:57PM -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.
> > > > 
> > > > 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.
> > > > 
> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_drv.h          |  9 +++++++++
> > > >  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 | 31
> > > > ++++++++++++++++++++++++++++++
> > > >  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 efca534..45e31d2 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > @@ -873,6 +873,13 @@ enum fb_op_origin {
> > > >  	ORIGIN_DIRTYFB,
> > > >  };
> > > >  
> > > > +enum fb_mmap_wa_flags {
> > > > +	FB_MMAP_WA_CPU =	1 << 0,
> > > > +	FB_MMAP_WA_GTT =	1 << 1,
> > > > +	FB_MMAP_WA_DISABLE = 	1 << 2,
> > > > +	FB_MMAP_WA_FLAG_COUNT =	3,
> > > > +};
> > > > +
> > 
> > I'll do the change to macros you/Jani mentioned in the other
> > emails.
> > 
> > > >  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.
> > > > */
> > > > @@ -910,6 +917,7 @@ struct intel_fbc {
> > > >  			unsigned int stride;
> > > >  			int fence_reg;
> > > >  			unsigned int tiling_mode;
> > > > +			unsigned int mmap_wa_flags;
> > > >  		} fb;
> > > >  	} state_cache;
> > > >  
> > > > @@ -2143,6 +2151,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 8588c83..d44f27e 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 28ead66..6e7aa9e 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > @@ -14705,6 +14705,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 ba45245..bbea7c4 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -1082,6 +1082,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,
> > > > @@ -1371,6 +1372,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..8d9b327 100644
> > > > --- a/drivers/gpu/drm/i915/intel_frontbuffer.c
> > > > +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
> > > > @@ -241,3 +241,34 @@ 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
> > > > + *
> > > > + * 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_GEN9(dev_priv))
> > > Please only IS_SKYLAKE, I don't want to extend this to either bxt
> > > or
> > > kbl,
> > > and both are still under prelim_hw_support and not yet shipping,
> > > i.e.
> > > we
> > > can break them ;-)
> > 
> > Mmmkay.

I vote for INTEL_INFO(dev)->gen < 9...  Since we only have the proper
api in use for gen8 and older...


> > 
> > > 
> > > Wrt the implementation: I think it would be great to get PSR on
> > > board
> > > to
> > > avoid duplicating the logic.
> > 
> > The goal of this patch going through intel_frontbuffer.c instead of
> > directly to intel_fbc.c is exactly to allow for PSR to do something
> > similar.
> > 
> > 
> > >  One option that might would be to create an
> > > invlidate on skl with ORIGIN_MMAP_WA, and as long as that's in
> > > effect
> > > (until the first sw_finish or dirty_fb) the frontbuffer tracking
> > > code
> > > itself would filter all flushes.
> > 
> > That's an non-trivial distortion of the trivial invalidate/flush
> > API. I
> > prefer having a special codepath for this special case instead of
> > adding second meanings to flush/invalidate.
> > 
> > Besides, even with the invalidate/flush API being simple, look at
> > how
> > many times we already tweaked/broke/fixed those functions, both for
> > PSR
> > and FBC.
> 
> Hm good point. It would indeed not neatly fit into the existing
> invalidate/flush scheme (which assumes that there's only ever one
> thing
> that can access a buffer, not a permanent mmap that can always be
> used).
> Agreed that having a special case makes sense.

Hm.... I believe I like the ORIGIN_MMAP_WA idea more.

I'm afraid that I'm already changing the original invalidate/flush API
here on my side for PSR on VLV. I'm planing to introduce the
ORIGIN_VBLANK to be able to trigger psr exit on vblank waits in vlv...
and only flush those bits when we have no vblank waiting.

Another total different way would be to add a runtime psr domains with
vblank domain, but it would be difficult to sync the psr busy bits...
so PSR would need to control the origin.

So, the same distortion (update?) of the frontbuffer tracking bits
could be used for this case here so I believe the amount of period
power savings feature keeps disabled would be reduced, improving the
power savings.

> 
> > >  fbc could then use ORIGIN_MMAP_WA
> > > invalidate to permanently disable (well, not re-enable after
> > > flip)
> > > until
> > > it sees a flush. And PSR would Just Work (I hope).
> > 
> > I think any PSR fixes related to this should be separate patches. I
> > have to confess I've just been studying the FBC part of the
> > problem, so
> > I was waiting for Rodrigo's opinions here before any conclusions.
> > Anyway, we can always change this code (in this patch or later) to
> > better suit PSR.

I believe for PSR is exactly the same here... I was waiting your
conclusion so I could replicate that for PSR somehow.

> 
> I'd like to at least have an ack from Rrodigo, even better if he can
> hack
> up something quickly. But yeah it's code, we can change it again ;-)

I prefer the solution on the frontbuffer tracking since this is not an
fbc issue only, but I don't want to block here. I agree it we can
always change it.

Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com>


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