[Intel-gfx] [PATCH 03/14] drm/i915/ppgtt: Load address space after mi_set_context

[Michel Thierry]

From: Ben Widawsky <benjamin.widawsky at intel.com>

The simple explanation is, the docs say to do this for GEN8. Perhaps we
want to do this for GEN7 too, I am not certain.

PDPs are saved and restored with context. Contexts (without execlists)
only exist on the render ring. The docs say that PDPs are not power
context save/restored.  I've learned that this actually means something
which SW doesn't care about. So pretend the statement doesn't exist.
For non RCS, nothing changes.

All this patch now does is change the ordering of LRI vs MI_SET_CONTEXT
for the initialization of the context. I do this because the docs say to
do it, and frankly, I cannot reason why it is necessary. I've thought
about it a lot, and tried, without success, to get a reason from design.
The answer I got more or less says, "gen7 is different than gen8." I've
given up, and am adding this little bit of code to make it in sync with
the docs.

v2: Completely rewritten commit message that addresses the requests
Ville made for v1
Only load PDPs for initial context load (Ville)

Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 29 +++++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 9ab3dad..cfec178 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -669,6 +669,7 @@ static int do_switch_rcs(struct intel_engine_cs *ring,
 	struct i915_hw_ppgtt *ppgtt = ctx_to_ppgtt(to);
 	u32 hw_flags = 0;
 	bool uninitialized = false;
+	bool needs_pd_load = (INTEL_INFO(ring->dev)->gen < 8) && USES_FULL_PPGTT(ring->dev);
 	int ret;
 
 	if (from != NULL) {
@@ -689,7 +690,10 @@ static int do_switch_rcs(struct intel_engine_cs *ring,
 	 */
 	from = ring->last_context;
 
-	if (USES_FULL_PPGTT(ring->dev)) {
+	if (needs_pd_load) {
+		/* Older GENs still want the load first, "PP_DCLV followed by
+		 * PP_DIR_BASE register through Load Register Immediate commands
+		 * in Ring Buffer before submitting a context."*/
 		ret = ppgtt->switch_mm(ppgtt, ring, false);
 		if (ret)
 			goto unpin_out;
@@ -713,13 +717,34 @@ static int do_switch_rcs(struct intel_engine_cs *ring,
 		vma->bind_vma(vma, to->legacy_hw_ctx.rcs_state->cache_level, GLOBAL_BIND);
 	}
 
-	if (!to->legacy_hw_ctx.initialized || i915_gem_context_is_default(to))
+	if (!to->legacy_hw_ctx.initialized || i915_gem_context_is_default(to)) {
 		hw_flags |= MI_RESTORE_INHIBIT;
+		needs_pd_load = USES_FULL_PPGTT(ring->dev) && IS_GEN8(ring->dev);
+	}
 
 	ret = mi_set_context(ring, to, hw_flags);
 	if (ret)
 		goto unpin_out;
 
+	/* GEN8 does *not* require an explicit reload if the PDPs have been
+	 * setup, and we do not wish to move them.
+	 *
+	 * XXX: If we implemented page directory eviction code, this
+	 * optimization needs to be removed.
+	 */
+	if (needs_pd_load) {
+		ret = ppgtt->switch_mm(ppgtt, ring, false);
+		/* The hardware context switch is emitted, but we haven't
+		 * actually changed the state - so it's probably safe to bail
+		 * here. Still, let the user know something dangerous has
+		 * happened.
+		 */
+		if (ret) {
+			DRM_ERROR("Failed to change address space on context switch\n");
+			goto unpin_out;
+		}
+	}
+
 	remap_l3(ring, to);
 
 	/* The backing object for the context is done after switching to the
-- 
1.9.0