Hi Chris:
Thanks for the comments! I can understand that we're concerned
about regressions, so this is why I think put this reset in module
unload path looks much safer. For safety, maybe we should only reset GPU
perhaps only when GEN >= 6? That looks much easier and safer, also
combine execlist reset and power context reset.
Or we just add this before i915_uncore_fini() inside
i915_driver_unload()? This way looks much safer?
How about this one?
diff --git a/drivers/gpu/drm/i915/i915_dma.c
b/drivers/gpu/drm/i915/i915_dma.c
index c5349fa..81103af 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1134,6 +1134,15 @@ int i915_driver_unload(struct drm_device *dev)
i915_global_gtt_cleanup(dev);
+ /*
+ * Restore HW workload submission mode back to default mode when
shutdown.
+ * It makes i915 module loading/unloading be able to switch between
+ * different workload submission mode on gen8+. And according to
B-spec,
+ * the only way to reset HW workload submission mode to default
mode is GPU reset.
+ */
+ if (i915.enable_execlists)
+ intel_gpu_reset(dev);
+
intel_uncore_fini(dev);
if (dev_priv->regs != NULL)
pci_iounmap(dev->pdev, dev_priv->regs);
于 07/07/15 16:58, Chris Wilson 写道:
On Tue, Jul 07, 2015 at 12:04:10AM +0800, Zhi Wang wrote:
Hi Chris and Mika:
Thanks for the comments. I think that reset HW on module unload
is an good idea. For now I think we should choose a proper position
in the module unload sequence to reset HW. As GPU reset is render
engine reset plus ring imrs(They will become to alll F after full
GPU reset), I think a proper position should be after render and
interrupt shutdown path.
How about this place?
diff --git a/drivers/gpu/drm/i915/i915_dma.c
b/drivers/gpu/drm/i915/i915_dma.c
index c5349fa..aeaf59e 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1133,7 +1133,10 @@ int i915_driver_unload(struct drm_device *dev)
pm_qos_remove_request(&dev_priv->pm_qos);
i915_global_gtt_cleanup(dev);
-
+ /* The only known way to stop the gpu from accessing the hw
context is
+ * to reset it. Do this as the very last operation to avoid
confusing
+ * other code, leading to spurious errors. */
+ intel_gpu_reset(dev);
That feels right. The comment is out-of-place now and needs expansion to
include other side effects for which the gpu reset is meritted.
But this is a riskier patch since we now start doing unconditional
resets for gen3-gen5. Just requires more soak testing, but I would
prefer it as (1) add execlists reset, (2) combine execlists reset +
power context reset into a single unload reset. That way if we do get a
regression in doing the unload reset we can revert back to execlists
easily.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
