drm/i915: Init PPGTT before context enable

commit f48a01651b1758550c4d3ee65ec726dfa0658780 upstream. Commit 82460d972 ("drm/i915: Rework ppgtt init to no require an aliasing ppgtt") introduced a regression on Broadwell, triggering the following IOMMU fault at startup: vgaarb: device changed decodes: PCI:0000:00:02.0,olddecodes=io+mem,decodes=io+mem:owns=io+mem dmar: DRHD: handling fault status reg 2 dmar: DMAR:[DMA Write] Request device [00:02.0] fault addr 880000 DMAR:[fault reason 23] Unknown fbcon: inteldrmfb (fb0) is primary device Further commentary from Daniel: I sugggested this change to David after staring at the offending patch for a while. I have no idea and theory whatsoever why this would upset the gpu less than the other way round. But it seems to work. David promised to chase hw people a bit more to get a more meaningful answer. Wrt the comment that this deletes: I've done some digging and afaict loading context before ppgtt enable was once required before our recent restructuring of the context/ppgtt init code: Before that context sw setup (i.e. allocating the default context) and hw setup was smashed together. Also the setup of the default context was the bit that actually allocated the aliasing ppgtt structures. Which is the reason for the context before ppgtt depency. Or was, since with all the untangling there's no no real depency any more (functional, who knows what the hw is doing), so the comment is just stale. Signed-off-by: David Woodhouse <David.Woodhouse@intel.com> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by: Jani Nikula <jani.nikula@intel.com> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

drm/i915: Init PPGTT before context enable
commit f48a01651b1758550c4d3ee65ec726dfa0658780 upstream. Commit 82460d972 ("drm/i915: Rework ppgtt init to no require an aliasing ppgtt") introduced a regression on Broadwell, triggering the following IOMMU fault at startup: vgaarb: device changed decodes: PCI:0000:00:02.0,olddecodes=io+mem,decodes=io+mem:owns=io+mem dmar: DRHD: handling fault status reg 2 dmar: DMAR:[DMA Write] Request device [00:02.0] fault addr 880000 DMAR:[fault reason 23] Unknown fbcon: inteldrmfb (fb0) is primary device Further commentary from Daniel: I sugggested this change to David after staring at the offending patch for a while. I have no idea and theory whatsoever why this would upset the gpu less than the other way round. But it seems to work. David promised to chase hw people a bit more to get a more meaningful answer. Wrt the comment that this deletes: I've done some digging and afaict loading context before ppgtt enable was once required before our recent restructuring of the context/ppgtt init code: Before that context sw setup (i.e. allocating the default context) and hw setup was smashed together. Also the setup of the default context was the bit that actually allocated the aliasing ppgtt structures. Which is the reason for the context before ppgtt depency. Or was, since with all the untangling there's no no real depency any more (functional, who knows what the hw is doing), so the comment is just stale. Signed-off-by: David Woodhouse <David.Woodhouse@intel.com> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by: Jani Nikula <jani.nikula@intel.com> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
David Woodhouse · Greg Kroah-Hartman
1 parent bcee607f44
Showing 1 changed file with 6 additions and 13 deletions Side-by-side Diff
drivers/gpu/drm/i915/i915_gem.c
@@ -4818,25 +4818,18 @@
 	for (i = 0; i < NUM_L3_SLICES(dev); i++)
 		i915_gem_l3_remap(&dev_priv->ring[RCS], i);
  
-	/*
-	 * XXX: Contexts should only be initialized once. Doing a switch to the
-	 * default context switch however is something we'd like to do after
-	 * reset or thaw (the latter may not actually be necessary for HW, but
-	 * goes with our code better). Context switching requires rings (for
-	 * the do_switch), but before enabling PPGTT. So don't move this.
-	 */
-	ret = i915_gem_context_enable(dev_priv);
+	ret = i915_ppgtt_init_hw(dev);
 	if (ret && ret != -EIO) {
-		DRM_ERROR("Context enable failed %d\n", ret);
+		DRM_ERROR("PPGTT enable failed %d\n", ret);
 		i915_gem_cleanup_ringbuffer(dev);
-
-		return ret;
 	}
  
-	ret = i915_ppgtt_init_hw(dev);
+	ret = i915_gem_context_enable(dev_priv);
 	if (ret && ret != -EIO) {
-		DRM_ERROR("PPGTT enable failed %d\n", ret);
+		DRM_ERROR("Context enable failed %d\n", ret);
 		i915_gem_cleanup_ringbuffer(dev);
+
+		return ret;
 	}
  
 	return ret;
...	...	@@ -4818,25 +4818,18 @@
4818	4818	for (i = 0; i < NUM_L3_SLICES(dev); i++)
4819	4819	i915_gem_l3_remap(&dev_priv->ring[RCS], i);
4820	4820
4821		- /*
4822		- * XXX: Contexts should only be initialized once. Doing a switch to the
4823		- * default context switch however is something we'd like to do after
4824		- * reset or thaw (the latter may not actually be necessary for HW, but
4825		- * goes with our code better). Context switching requires rings (for
4826		- * the do_switch), but before enabling PPGTT. So don't move this.
4827		- */
4828		- ret = i915_gem_context_enable(dev_priv);
	4821	+ ret = i915_ppgtt_init_hw(dev);
4829	4822	if (ret && ret != -EIO) {
4830		- DRM_ERROR("Context enable failed %d\n", ret);
	4823	+ DRM_ERROR("PPGTT enable failed %d\n", ret);
4831	4824	i915_gem_cleanup_ringbuffer(dev);
4832		-
4833		- return ret;
4834	4825	}
4835	4826
4836		- ret = i915_ppgtt_init_hw(dev);
	4827	+ ret = i915_gem_context_enable(dev_priv);
4837	4828	if (ret && ret != -EIO) {
4838		- DRM_ERROR("PPGTT enable failed %d\n", ret);
	4829	+ DRM_ERROR("Context enable failed %d\n", ret);
4839	4830	i915_gem_cleanup_ringbuffer(dev);
	4831	+
	4832	+ return ret;
4840	4833	}
4841	4834
4842	4835	return ret;