| Message ID | 559AD93D.8050503@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jul 07, 2015 at 03:38:37AM +0800, Zhi Wang wrote: > 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? No, if we are just targetting execlists, then disabling it in cleanup_ringbuffers as before is the cleanest (as that is the opposite stage to where we enable them). The reset in i915_driver_unload() is preferred to replace all the resets required during unload. It is safe to move the context reset here as we do not disturb the GTT state between unpining the context and here. Making it conditional on gen>=5 is probably a good first step. -Chris
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); +
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? 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 >