[Intel-gfx] [PATCH] drm/i915: Disable full ppgtt by default

[Daniel Vetter]

There are too many oustanding issues:

- Fence handling in the current code is broken. There's a patch series
  from me, but it's blocked on and extended review (which includes
  writing the testcases).

- IOMMU mapping handling is broken, we need to properly refcount it -
  currently it gets destroyed when the first vma is unbound, so way
  too early.

- There's a pending reset issue on snb. Since Mika's reset work and
  full ppgtt have been pulled in in separate branches and ended up
  intermittingly breaking each another it's unclear who's the exact
  culprit here.

- We still have persistent evidince of crazy recursion bugs through
  vma_unbind and ppgtt_relase, e.g.

  https://bugs.freedesktop.org/show_bug.cgi?id=73383

  This issue (and a few others meanwhile resolved) have blocked our
  performance measuring/tuning group since 3 months.

- Secure batch dispatching is broken. This is blocking Brad Volkin's
  command checker work since 3 months.

All these issues are confirmed to only happen when full ppgtt is
enabled, falling back to aliasing ppgtt resolves them. But even
aliasing ppgtt itself still has a regression:

- We currently unconditionally bind objects into the aliasing ppgtt,
  which means all priviledged objects like ringbuffers are visible to
  unpriviledged access again. On top of that this also breaks the
  command checker for aliasing ppgtt, since it can't hide the
  validated batch any more.

Furthermore topic/full-ppgtt has never been reviewed:

- Lifetime rules around vma unbinding/release are unclear, resulting
  into this awesome hack called ppgtt_release. Which seems to take the
  blame for most of the recursion fallout.

- Context/ring init works different on gpu reset than anywhere else.
  Such differeneces have in the past always lead to really hard to
  track down bugs.

- Aliasing ppgtt is treated in a bunch of places as a real address
  space, but it isn't - the real address space is always the global
  gtt in that case. This results in a bit a mess between contexts and
  ppgtt object, further complication the context/ppgtt/vma lifetime
  rules.

- We don't have any docs describing the overall concepts introduced
  with full ppgtt. A short, concise overview describing vmas and some
  of the strange bits around them (like the unbound vmas used by
  execbuf, or the new binding rules) really is needed.

Note that a lot of the post topic/full-ppgtt merge fallout has already
been addressed, this entire list here of 10 issues really only contains
the still outstanding issues.

Finally the 3.15 merge window is approaching and I think we need to
use the remaining time to ensure that our fallback option of using
aliasing ppgtt is in solid shape. Hence I think it's time to throw the
switch. While at it demote the helper from static inline status
because really.

Cc: Ben Widawsky <ben at bwidawsk.net>
Cc: Dave Airlie <airlied at gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter at ffwll.ch>

---

To preempt the inevitable flamewar:

- I'm not really up for another experiment to merge a working feature
  as-is through a branch and then polish it later because
  a) full ppgtt never really worked well enough to begin with and
  b) the polishing (and bugfixing) didn't really happen sufficiently
  quickly to avoid blocking tons of other people's work.
  No I don't have a better idea than our current patch/people crunch
  process to get big feature work in, but feature branches (at least
  like it went down with full ppgtt) is definitely worse.

- Not moving the goalpost here. Most of these issues have been spotted
  in review or caught with tests even before the plan to merge an
  unreviewed full ppgtt topic branch was agreed on. But they all have
  been shrugged off or ducttaped over the failing testcase without
  fixing the underlying issue.

- I've had my fair share of flamewars in the past few months, not up
  for another one just now. I've discussed this a bit with Ben in
  private and he said that as long as I clearly state what's missing
  and as long I only disable the default and don't rip out the code he
  can live with this, though obviously he's not happy.

  So if you disagree with my decision to disable full ppgtt for now or
  disagree with my assessment that the feature branch experiment
  failed and shouldn't be repeated without changes, then please follow
  the established maintainer impeachement procedures and send a pull
  request to Dave Airlie to sign yourself up for this grateful
  position.

-Daniel
---
 drivers/gpu/drm/i915/i915_drv.h     | 22 +---------------------
 drivers/gpu/drm/i915/i915_gem_gtt.c | 23 +++++++++++++++++++++++
 2 files changed, 24 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index dce09fcddc2c..843aaee8d80b 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2377,27 +2377,7 @@ static inline void i915_gem_chipset_flush(struct drm_device *dev)
 		intel_gtt_chipset_flush();
 }
 int i915_gem_init_ppgtt(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt);
-static inline bool intel_enable_ppgtt(struct drm_device *dev, bool full)
-{
-	if (i915.enable_ppgtt == 0 || !HAS_ALIASING_PPGTT(dev))
-		return false;
-
-	if (i915.enable_ppgtt == 1 && full)
-		return false;
-
-#ifdef CONFIG_INTEL_IOMMU
-	/* Disable ppgtt on SNB if VT-d is on. */
-	if (INTEL_INFO(dev)->gen == 6 && intel_iommu_gfx_mapped) {
-		DRM_INFO("Disabling PPGTT because VT-d is on\n");
-		return false;
-	}
-#endif
-
-	if (full)
-		return HAS_PPGTT(dev);
-	else
-		return HAS_ALIASING_PPGTT(dev);
-}
+bool intel_enable_ppgtt(struct drm_device *dev, bool full);
 
 /* i915_gem_stolen.c */
 int i915_gem_init_stolen(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index a616cac0f161..3d8bd62a926c 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -30,6 +30,29 @@
 #include "i915_trace.h"
 #include "intel_drv.h"
 
+bool intel_enable_ppgtt(struct drm_device *dev, bool full)
+{
+	if (i915.enable_ppgtt == 0 || !HAS_ALIASING_PPGTT(dev))
+		return false;
+
+	if (i915.enable_ppgtt == 1 && full)
+		return false;
+
+#ifdef CONFIG_INTEL_IOMMU
+	/* Disable ppgtt on SNB if VT-d is on. */
+	if (INTEL_INFO(dev)->gen == 6 && intel_iommu_gfx_mapped) {
+		DRM_INFO("Disabling PPGTT because VT-d is on\n");
+		return false;
+	}
+#endif
+
+	/* Full ppgtt disabled by default for now due to issues. */
+	if (full)
+		return false; /* HAS_PPGTT(dev) */
+	else
+		return HAS_ALIASING_PPGTT(dev);
+}
+
 #define GEN6_PPGTT_PD_ENTRIES 512
 #define I915_PPGTT_PT_ENTRIES (PAGE_SIZE / sizeof(gen6_gtt_pte_t))
 typedef uint64_t gen8_gtt_pte_t;
-- 
1.8.5.2