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

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

[bing.niu]

From: "Niu,Bing" <bing.niu@intel.com>

It is found that i915 will not reset gpu under execlist mode when
unload module. that will lead to some issues when unload/load module
with different submission mode. e.g. from execlist mode to ring
buffer mode via loading/unloading i915. Because HW is not in a reset
state and registers are not clean under such condition.

Signed-off-by: Niu,Bing <bing.niu@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_context.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 133afcf..551478c 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -372,13 +372,13 @@ void i915_gem_context_fini(struct drm_device *dev)
 	struct intel_context *dctx = dev_priv->ring[RCS].default_context;
 	int i;
 
-	if (dctx->legacy_hw_ctx.rcs_state) {
-		/* 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);
+    /* 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);
 
-		/* When default context is created and switched to, base object refcount
+	if (dctx->legacy_hw_ctx.rcs_state) {
+	    /* When default context is created and switched to, base object refcount
 		 * will be 2 (+1 from object creation and +1 from do_switch()).
 		 * i915_gem_context_fini() will be called after gpu_idle() has switched
 		 * to default context. So we need to unreference the base object once
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

On Thu, Jun 25, 2015 at 10:40:15PM +0800, bing.niu@intel.com wrote:
> From: "Niu,Bing" <bing.niu@intel.com>
> 
> It is found that i915 will not reset gpu under execlist mode when
> unload module. that will lead to some issues when unload/load module
> with different submission mode. e.g. from execlist mode to ring
> buffer mode via loading/unloading i915. Because HW is not in a reset
> state and registers are not clean under such condition.

Now the reason we have the reset there is to disable writes to the power
context after the memory is released. Your complaint above can easily be
read as the driver is unable to correctly initialisae itself from a
random GPU state, which is a fault in driver initialisation.

Have you observed stray memory writes from the GPU after the module is
unloaded? If not, this would seem to be just a band-aid.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Wang, Zhi A]

Hi Chris:
    Thanks for the comments that extends our knowledge about this modification. Really appreciate the help. But is it possible that we still need an ordinary gpu reset here? Consider that we may modprobe i915 with i915.enable_execlists=1 then unload it, then modprobe i915.enable_execlists=0, according to bspec, changing execlist mode to ringbuffer mode, a hardware reset is needed?

Thanks,
Zhi.
> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
> Chris Wilson
> Sent: Thursday, June 25, 2015 4:13 PM
> To: Niu, Bing
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Also perform gpu reset under execlist
> mode.
> 
> On Thu, Jun 25, 2015 at 10:40:15PM +0800, bing.niu@intel.com wrote:
> > From: "Niu,Bing" <bing.niu@intel.com>
> >
> > It is found that i915 will not reset gpu under execlist mode when
> > unload module. that will lead to some issues when unload/load module
> > with different submission mode. e.g. from execlist mode to ring buffer
> > mode via loading/unloading i915. Because HW is not in a reset state
> > and registers are not clean under such condition.
> 
> Now the reason we have the reset there is to disable writes to the power
> context after the memory is released. Your complaint above can easily be read
> as the driver is unable to correctly initialisae itself from a random GPU state,
> which is a fault in driver initialisation.
> 
> Have you observed stray memory writes from the GPU after the module is
> unloaded? If not, this would seem to be just a band-aid.
> -Chris
> 
> --
> Chris Wilson, Intel Open Source Technology Centre
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

On Thu, Jun 25, 2015 at 12:24:59PM +0000, Wang, Zhi A wrote:
> Hi Chris:
>     Thanks for the comments that extends our knowledge about this modification. Really appreciate the help. But is it possible that we still need an ordinary gpu reset here? Consider that we may modprobe i915 with i915.enable_execlists=1 then unload it, then modprobe i915.enable_execlists=0, according to bspec, changing execlist mode to ringbuffer mode, a hardware reset is needed?

Sounds credible that execlist -> execbuffer requires a reset. That reset
should be called in execlist shutdown and documented appropriately.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Zhi Wang]

Hi Chris:
     Thanks for the reply! I just dig the code. It looks there is no 
special code path for execlist shutdown. It has init_rings(), but 
doesn't have cleanup_rings(), only clean_ring(), which are called for 
each ring one by one when i915_gem_cleanup_ringbuffer() gets called.

Do you have any advice? :)

Thanks,
Zhi.

于 06/25/15 20:37, Chris Wilson 写道:
> On Thu, Jun 25, 2015 at 12:24:59PM +0000, Wang, Zhi A wrote:
>> Hi Chris:
>>      Thanks for the comments that extends our knowledge about this modification. Really appreciate the help. But is it possible that we still need an ordinary gpu reset here? Consider that we may modprobe i915 with i915.enable_execlists=1 then unload it, then modprobe i915.enable_execlists=0, according to bspec, changing execlist mode to ringbuffer mode, a hardware reset is needed?
>
> Sounds credible that execlist -> execbuffer requires a reset. That reset
> should be called in execlist shutdown and documented appropriately.
> -Chris
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

On Thu, Jun 25, 2015 at 05:56:58AM +0800, Zhi Wang wrote:
> Hi Chris:
>     Thanks for the reply! I just dig the code. It looks there is no
> special code path for execlist shutdown. It has init_rings(), but
> doesn't have cleanup_rings(), only clean_ring(), which are called
> for each ring one by one when i915_gem_cleanup_ringbuffer() gets
> called.
> 
> Do you have any advice? :)

The quick and dirty version would be:

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 111c5cb2aa99..254c8e28963c 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -5096,6 +5096,14 @@ i915_gem_cleanup_ringbuffer(struct drm_device *dev)
 
        for_each_ring(ring, dev_priv, i)
                dev_priv->gt.cleanup_ring(ring);
+
+       if (i915.enable_execlists)
+               /*
+                * Neither the BIOS, ourselves or any other kernel
+                * expects the system to be in execlists mode on startup,
+                * so we need to reset the GPU back to legacy mode.
+                */
+               intel_gpu_reset(dev);
 }
 
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Zhi Wang]

Thanks! :D

Thanks,
Zhi.

于 06/25/15 21:42, Chris Wilson 写道:
> On Thu, Jun 25, 2015 at 05:56:58AM +0800, Zhi Wang wrote:
>> Hi Chris:
>>      Thanks for the reply! I just dig the code. It looks there is no
>> special code path for execlist shutdown. It has init_rings(), but
>> doesn't have cleanup_rings(), only clean_ring(), which are called
>> for each ring one by one when i915_gem_cleanup_ringbuffer() gets
>> called.
>>
>> Do you have any advice? :)
>
> The quick and dirty version would be:
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 111c5cb2aa99..254c8e28963c 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -5096,6 +5096,14 @@ i915_gem_cleanup_ringbuffer(struct drm_device *dev)
>
>          for_each_ring(ring, dev_priv, i)
>                  dev_priv->gt.cleanup_ring(ring);
> +
> +       if (i915.enable_execlists)
> +               /*
> +                * Neither the BIOS, ourselves or any other kernel
> +                * expects the system to be in execlists mode on startup,
> +                * so we need to reset the GPU back to legacy mode.
> +                */
> +               intel_gpu_reset(dev);
>   }
>
> -Chris
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

On Thu, Jun 25, 2015 at 09:13:13AM +0100, Chris Wilson wrote:
> On Thu, Jun 25, 2015 at 10:40:15PM +0800, bing.niu@intel.com wrote:
> > From: "Niu,Bing" <bing.niu@intel.com>
> > 
> > It is found that i915 will not reset gpu under execlist mode when
> > unload module. that will lead to some issues when unload/load module
> > with different submission mode. e.g. from execlist mode to ring
> > buffer mode via loading/unloading i915. Because HW is not in a reset
> > state and registers are not clean under such condition.
> 
> Now the reason we have the reset there is to disable writes to the power
> context after the memory is released. Your complaint above can easily be
> read as the driver is unable to correctly initialisae itself from a
> random GPU state, which is a fault in driver initialisation.
> 
> Have you observed stray memory writes from the GPU after the module is
> unloaded? If not, this would seem to be just a band-aid.

I think as long as we don't have any bios out there enabling execlist
already it's better to try to leave the gpu behind in a state consistent
what we're seeing from real firmware. Otherwise we just have complexity
for no reason.

Similar example is that we kill all outputs since our pte setup doesn't
math the bios one (and yeah I think there too it'd be good to fill out the
gtt with stolen ptes). Hence this patch makes sense for me.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx