[PATCH] drm/i915: Park before resetting the submission backend

[PATCH] drm/i915: Park before resetting the submission backend

[Chris Wilson]

As different backends may have different park/unpark callbacks, we
should only ever switch backends (reset_default_submission on wedge
recovery, or on enabling the guc) while parked.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c             | 11 +++++++++++
 drivers/gpu/drm/i915/intel_engine_cs.c      |  3 +++
 drivers/gpu/drm/i915/intel_guc_submission.c |  1 +
 3 files changed, 15 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index e148db310ea6..e2880de2fc7e 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3380,6 +3380,17 @@ bool i915_gem_unset_wedged(struct drm_i915_private *i915)
 	i915_retire_requests(i915);
 	GEM_BUG_ON(i915->gt.active_requests);
 
+	/*
+	 * Park before disengaging the old submit mechanism as different
+	 * backends may have different park/unpack callbacks.
+	 *
+	 * We are idle; the idle-worker will be queued, but we need to run
+	 * it now. As we already hold the struct mutex, we can get park
+	 * the GPU right away, letting the lazy worker see that we are
+	 * already active again by the time it acquires the mutex.
+	 */
+	i915_gem_park(i915);
+
 	/*
 	 * Undo nop_submit_request. We prevent all new i915 requests from
 	 * being queued (by disallowing execbuf whilst wedged) so having
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index 12486d8f534b..b4ea77a2896c 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -1651,6 +1651,9 @@ void intel_engines_reset_default_submission(struct drm_i915_private *i915)
 	struct intel_engine_cs *engine;
 	enum intel_engine_id id;
 
+	/* Must be parked first! */
+	GEM_BUG_ON(i915->gt.awake);
+
 	for_each_engine(engine, i915, id)
 		engine->set_default_submission(engine);
 }
diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c b/drivers/gpu/drm/i915/intel_guc_submission.c
index 97121230656c..225fa3927a02 100644
--- a/drivers/gpu/drm/i915/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/intel_guc_submission.c
@@ -1243,6 +1243,7 @@ int intel_guc_submission_enable(struct intel_guc *guc)
 	/* Take over from manual control of ELSP (execlists) */
 	guc_interrupts_capture(dev_priv);
 
+	GEM_BUG_ON(dev_priv->gt.awake); /* Must be idle switching park/unpark */
 	for_each_engine(engine, dev_priv, id) {
 		struct intel_engine_execlists * const execlists =
 			&engine->execlists;
-- 
2.16.3

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

Re: [PATCH] drm/i915: Park before resetting the submission backend

[Chris Wilson]

Quoting Chris Wilson (2018-04-05 11:57:22)
> As different backends may have different park/unpark callbacks, we
> should only ever switch backends (reset_default_submission on wedge
> recovery, or on enabling the guc) while parked.

Last in series, missing the series... Sorry for the noise,
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for drm/i915: Park before resetting the submission backend

[Patchwork]

== Series Details ==

Series: drm/i915: Park before resetting the submission backend
URL   : https://patchwork.freedesktop.org/series/41202/
State : failure

== Summary ==

CHK     include/config/kernel.release
  CHK     include/generated/uapi/linux/version.h
  CHK     include/generated/utsrelease.h
  CHK     include/generated/bounds.h
  CHK     include/generated/timeconst.h
  CHK     include/generated/asm-offsets.h
  CALL    scripts/checksyscalls.sh
  DESCEND  objtool
  CHK     scripts/mod/devicetable-offsets.h
  CHK     include/generated/compile.h
  CHK     kernel/config_data.h
  CC [M]  drivers/gpu/drm/i915/i915_gem.o
drivers/gpu/drm/i915/i915_gem.c: In function ‘i915_gem_unset_wedged’:
drivers/gpu/drm/i915/i915_gem.c:3356:2: error: implicit declaration of function ‘i915_gem_park’ [-Werror=implicit-function-declaration]
  i915_gem_park(i915);
  ^~~~~~~~~~~~~
cc1: all warnings being treated as errors
scripts/Makefile.build:324: recipe for target 'drivers/gpu/drm/i915/i915_gem.o' failed
make[4]: *** [drivers/gpu/drm/i915/i915_gem.o] Error 1
scripts/Makefile.build:583: recipe for target 'drivers/gpu/drm/i915' failed
make[3]: *** [drivers/gpu/drm/i915] Error 2
scripts/Makefile.build:583: recipe for target 'drivers/gpu/drm' failed
make[2]: *** [drivers/gpu/drm] Error 2
scripts/Makefile.build:583: recipe for target 'drivers/gpu' failed
make[1]: *** [drivers/gpu] Error 2
Makefile:1060: recipe for target 'drivers' failed
make: *** [drivers] Error 2

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

[PATCH] drm/i915: Park before resetting the submission backend

[Chris Wilson]

As different backends may have different park/unpark callbacks, we
should only ever switch backends (reset_default_submission on wedge
recovery, or on enabling the guc) while parked.

v2: Remove the assert from the guc code, as we are currently trying to
modify the engine vfuncs pointer on a live system after reset (not just
wedging). We will just have to hope that the system is balanced.
v3: Rebase onto __i915_gem_park and improve grammar.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c        | 15 ++++++++++++---
 drivers/gpu/drm/i915/intel_engine_cs.c |  3 +++
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 28ab0beff86c..dd3e292ba243 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -144,8 +144,6 @@ static u32 __i915_gem_park(struct drm_i915_private *i915)
 	if (!i915->gt.awake)
 		return I915_EPOCH_INVALID;
 
-	GEM_BUG_ON(i915->gt.epoch == I915_EPOCH_INVALID);
-
 	/*
 	 * Be paranoid and flush a concurrent interrupt to make sure
 	 * we don't reactivate any irq tasklets after parking.
@@ -173,6 +171,7 @@ static u32 __i915_gem_park(struct drm_i915_private *i915)
 
 	intel_runtime_pm_put(i915);
 
+	GEM_BUG_ON(i915->gt.epoch == I915_EPOCH_INVALID);
 	return i915->gt.epoch;
 }
 
@@ -3435,7 +3434,17 @@ bool i915_gem_unset_wedged(struct drm_i915_private *i915)
 		}
 	}
 	i915_retire_requests(i915);
-	GEM_BUG_ON(i915->gt.active_requests);
+
+	/*
+	 * Park before disengaging the old submit mechanism as different
+	 * backends may have different park/unpack callbacks.
+	 *
+	 * We are idle; the idle-worker will be queued, but we need to run
+	 * it now. As we already hold the struct mutex, we can park the GPU
+	 * right away, letting the lazy worker see that we are already active
+	 * again by the time it acquires the mutex.
+	 */
+	__i915_gem_park(i915);
 
 	/*
 	 * Undo nop_submit_request. We prevent all new i915 requests from
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index 12486d8f534b..b4ea77a2896c 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -1651,6 +1651,9 @@ void intel_engines_reset_default_submission(struct drm_i915_private *i915)
 	struct intel_engine_cs *engine;
 	enum intel_engine_id id;
 
+	/* Must be parked first! */
+	GEM_BUG_ON(i915->gt.awake);
+
 	for_each_engine(engine, i915, id)
 		engine->set_default_submission(engine);
 }
-- 
2.17.0

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

Re: [PATCH] drm/i915: Park before resetting the submission backend

[Sagar Arun Kamble]

On 4/9/2018 3:48 PM, Chris Wilson wrote:
> As different backends may have different park/unpark callbacks, we
> should only ever switch backends (reset_default_submission on wedge
> recovery, or on enabling the guc) while parked.
>
> v2: Remove the assert from the guc code, as we are currently trying to
> modify the engine vfuncs pointer on a live system after reset (not just
> wedging). We will just have to hope that the system is balanced.
> v3: Rebase onto __i915_gem_park and improve grammar.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem.c        | 15 ++++++++++++---
>   drivers/gpu/drm/i915/intel_engine_cs.c |  3 +++
>   2 files changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 28ab0beff86c..dd3e292ba243 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -144,8 +144,6 @@ static u32 __i915_gem_park(struct drm_i915_private *i915)
>   	if (!i915->gt.awake)
>   		return I915_EPOCH_INVALID;
>   
> -	GEM_BUG_ON(i915->gt.epoch == I915_EPOCH_INVALID);
> -
>   	/*
>   	 * Be paranoid and flush a concurrent interrupt to make sure
>   	 * we don't reactivate any irq tasklets after parking.
> @@ -173,6 +171,7 @@ static u32 __i915_gem_park(struct drm_i915_private *i915)
>   
>   	intel_runtime_pm_put(i915);
>   
> +	GEM_BUG_ON(i915->gt.epoch == I915_EPOCH_INVALID);
>   	return i915->gt.epoch;
>   }
>   
> @@ -3435,7 +3434,17 @@ bool i915_gem_unset_wedged(struct drm_i915_private *i915)
>   		}
>   	}
>   	i915_retire_requests(i915);
> -	GEM_BUG_ON(i915->gt.active_requests);
> +
> +	/*
> +	 * Park before disengaging the old submit mechanism as different
> +	 * backends may have different park/unpack callbacks.
> +	 *
> +	 * We are idle; the idle-worker will be queued, but we need to run
> +	 * it now. As we already hold the struct mutex, we can park the GPU
> +	 * right away, letting the lazy worker see that we are already active
> +	 * again by the time it acquires the mutex.
> +	 */
> +	__i915_gem_park(i915);
>   
>   	/*
>   	 * Undo nop_submit_request. We prevent all new i915 requests from
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index 12486d8f534b..b4ea77a2896c 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -1651,6 +1651,9 @@ void intel_engines_reset_default_submission(struct drm_i915_private *i915)
>   	struct intel_engine_cs *engine;
>   	enum intel_engine_id id;
>   
> +	/* Must be parked first! */
> +	GEM_BUG_ON(i915->gt.awake);
> +
>   	for_each_engine(engine, i915, id)
>   		engine->set_default_submission(engine);
>   }

-- 
Thanks,
Sagar

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

Re: [PATCH] drm/i915: Park before resetting the submission backend

[Chris Wilson]

Quoting Sagar Arun Kamble (2018-04-09 11:38:34)
> 
> 
> On 4/9/2018 3:48 PM, Chris Wilson wrote:
> > As different backends may have different park/unpark callbacks, we
> > should only ever switch backends (reset_default_submission on wedge
> > recovery, or on enabling the guc) while parked.
> >
> > v2: Remove the assert from the guc code, as we are currently trying to
> > modify the engine vfuncs pointer on a live system after reset (not just
> > wedging). We will just have to hope that the system is balanced.
> > v3: Rebase onto __i915_gem_park and improve grammar.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>

Michal, do you want to take this and merge it into your series?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Park before resetting the submission backend

[Michal Wajdeczko]

On Mon, 09 Apr 2018 12:41:22 +0200, Chris Wilson  
<chris@chris-wilson.co.uk> wrote:

> Quoting Sagar Arun Kamble (2018-04-09 11:38:34)
>>
>>
>> On 4/9/2018 3:48 PM, Chris Wilson wrote:
>> > As different backends may have different park/unpark callbacks, we
>> > should only ever switch backends (reset_default_submission on wedge
>> > recovery, or on enabling the guc) while parked.
>> >
>> > v2: Remove the assert from the guc code, as we are currently trying to
>> > modify the engine vfuncs pointer on a live system after reset (not  
>> just
>> > wedging). We will just have to hope that the system is balanced.
>> > v3: Rebase onto __i915_gem_park and improve grammar.
>> >
>> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> > Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>> Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>
> Michal, do you want to take this and merge it into your series?

Yes, I can take it.
Then we will see results from CI.IGT with GuC enabled.

/Michal

ps. this patch is also

Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx