Re: [PATCH v2] drm/i915: Also perform gpu reset under execlist mode.

Zhi Wang
2015-07-07T11:11:27Z

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