[PATCH 1/2] drm/i915: Expose retry count to per gen reset logic

[PATCH 1/2] drm/i915: Expose retry count to per gen reset logic

[Mika Kuoppala]

There is a possibility for per gen reset logic to
be more nasty if the softer approach on resetting does
not bear fruit.

Expose retry count to per gen reset logic if it
wants to take such tough measures.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_uncore.c | 40 +++++++++++++++++++----------
 1 file changed, 26 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index c2fcb51fc58a..027d14574bfa 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -1739,7 +1739,7 @@ static void gen3_stop_engine(struct intel_engine_cs *engine)
 }
 
 static void i915_stop_engines(struct drm_i915_private *dev_priv,
-			      unsigned engine_mask)
+			      unsigned int engine_mask)
 {
 	struct intel_engine_cs *engine;
 	enum intel_engine_id id;
@@ -1759,7 +1759,9 @@ static bool i915_in_reset(struct pci_dev *pdev)
 	return gdrst & GRDOM_RESET_STATUS;
 }
 
-static int i915_do_reset(struct drm_i915_private *dev_priv, unsigned engine_mask)
+static int i915_do_reset(struct drm_i915_private *dev_priv,
+			 unsigned int engine_mask,
+			 unsigned int retry)
 {
 	struct pci_dev *pdev = dev_priv->drm.pdev;
 	int err;
@@ -1786,7 +1788,9 @@ static bool g4x_reset_complete(struct pci_dev *pdev)
 	return (gdrst & GRDOM_RESET_ENABLE) == 0;
 }
 
-static int g33_do_reset(struct drm_i915_private *dev_priv, unsigned engine_mask)
+static int g33_do_reset(struct drm_i915_private *dev_priv,
+			unsigned int engine_mask,
+			unsigned int retry)
 {
 	struct pci_dev *pdev = dev_priv->drm.pdev;
 
@@ -1794,7 +1798,9 @@ static int g33_do_reset(struct drm_i915_private *dev_priv, unsigned engine_mask)
 	return wait_for(g4x_reset_complete(pdev), 500);
 }
 
-static int g4x_do_reset(struct drm_i915_private *dev_priv, unsigned engine_mask)
+static int g4x_do_reset(struct drm_i915_private *dev_priv,
+			unsigned int engine_mask,
+			unsigned int retry)
 {
 	struct pci_dev *pdev = dev_priv->drm.pdev;
 	int ret;
@@ -1831,7 +1837,8 @@ static int g4x_do_reset(struct drm_i915_private *dev_priv, unsigned engine_mask)
 }
 
 static int ironlake_do_reset(struct drm_i915_private *dev_priv,
-			     unsigned engine_mask)
+			     unsigned int engine_mask,
+			     unsigned int retry)
 {
 	int ret;
 
@@ -1887,6 +1894,7 @@ static int gen6_hw_domain_reset(struct drm_i915_private *dev_priv,
  * gen6_reset_engines - reset individual engines
  * @dev_priv: i915 device
  * @engine_mask: mask of intel_ring_flag() engines or ALL_ENGINES for full reset
+ * @retry: the count of of previous attempts to reset.
  *
  * This function will reset the individual engines that are set in engine_mask.
  * If you provide ALL_ENGINES as mask, full global domain reset will be issued.
@@ -1897,7 +1905,8 @@ static int gen6_hw_domain_reset(struct drm_i915_private *dev_priv,
  * Returns 0 on success, nonzero on error.
  */
 static int gen6_reset_engines(struct drm_i915_private *dev_priv,
-			      unsigned engine_mask)
+			      unsigned int engine_mask,
+			      unsigned int retry)
 {
 	struct intel_engine_cs *engine;
 	const u32 hw_engine_mask[I915_NUM_ENGINES] = {
@@ -1936,7 +1945,7 @@ static int gen6_reset_engines(struct drm_i915_private *dev_priv,
  * Returns 0 on success, nonzero on error.
  */
 static int gen11_reset_engines(struct drm_i915_private *dev_priv,
-			       unsigned engine_mask)
+			       unsigned int engine_mask)
 {
 	struct intel_engine_cs *engine;
 	const u32 hw_engine_mask[I915_NUM_ENGINES] = {
@@ -2105,7 +2114,8 @@ static void gen8_reset_engine_cancel(struct intel_engine_cs *engine)
 }
 
 static int gen8_reset_engines(struct drm_i915_private *dev_priv,
-			      unsigned engine_mask)
+			      unsigned int engine_mask,
+			      unsigned int retry)
 {
 	struct intel_engine_cs *engine;
 	unsigned int tmp;
@@ -2121,7 +2131,7 @@ static int gen8_reset_engines(struct drm_i915_private *dev_priv,
 	if (INTEL_GEN(dev_priv) >= 11)
 		ret = gen11_reset_engines(dev_priv, engine_mask);
 	else
-		ret = gen6_reset_engines(dev_priv, engine_mask);
+		ret = gen6_reset_engines(dev_priv, engine_mask, retry);
 
 not_ready:
 	for_each_engine_masked(engine, dev_priv, engine_mask, tmp)
@@ -2130,7 +2140,8 @@ static int gen8_reset_engines(struct drm_i915_private *dev_priv,
 	return ret;
 }
 
-typedef int (*reset_func)(struct drm_i915_private *, unsigned engine_mask);
+typedef int (*reset_func)(struct drm_i915_private *,
+			  unsigned int engine_mask, unsigned int retry);
 
 static reset_func intel_get_gpu_reset(struct drm_i915_private *dev_priv)
 {
@@ -2153,10 +2164,10 @@ static reset_func intel_get_gpu_reset(struct drm_i915_private *dev_priv)
 		return NULL;
 }
 
-int intel_gpu_reset(struct drm_i915_private *dev_priv, unsigned engine_mask)
+int intel_gpu_reset(struct drm_i915_private *dev_priv, unsigned int engine_mask)
 {
 	reset_func reset = intel_get_gpu_reset(dev_priv);
-	int retry;
+	unsigned int retry;
 	int ret;
 
 	/*
@@ -2200,8 +2211,9 @@ int intel_gpu_reset(struct drm_i915_private *dev_priv, unsigned engine_mask)
 
 		ret = -ENODEV;
 		if (reset) {
-			GEM_TRACE("engine_mask=%x\n", engine_mask);
-			ret = reset(dev_priv, engine_mask);
+			ret = reset(dev_priv, engine_mask, retry);
+			GEM_TRACE("engine_mask=%x, ret=%d, retry=%d\n",
+				  engine_mask, ret, retry);
 		}
 		if (ret != -ETIMEDOUT || engine_mask != ALL_ENGINES)
 			break;
-- 
2.17.1

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

[PATCH 2/2] drm/i915: Force reset on unready engine

[Mika Kuoppala]

If engine reports that it is not ready for reset, we
give up. Evidence shows that forcing a per engine reset
on an engine which is not reporting to be ready for reset,
can bring it back into a working order. There is risk that
we corrupt the context image currently executing on that
engine. But that is a risk worth taking as if we unblock
the engine, we prevent a whole device wedging in a case
of full gpu reset.

Reset individual engine even if it reports that it is not
prepared for reset, but only if we aim for full gpu reset
and not on first reset attempt.

v2: force reset only on later attempts, readability (Chris)

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_uncore.c | 49 +++++++++++++++++++++++------
 1 file changed, 39 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 027d14574bfa..d24026839b17 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -2099,9 +2099,6 @@ static int gen8_reset_engine_start(struct intel_engine_cs *engine)
 					   RESET_CTL_READY_TO_RESET,
 					   700, 0,
 					   NULL);
-	if (ret)
-		DRM_ERROR("%s: reset request timeout\n", engine->name);
-
 	return ret;
 }
 
@@ -2113,6 +2110,42 @@ static void gen8_reset_engine_cancel(struct intel_engine_cs *engine)
 		      _MASKED_BIT_DISABLE(RESET_CTL_REQUEST_RESET));
 }
 
+static int reset_engines(struct drm_i915_private *i915,
+			 unsigned int engine_mask,
+			 unsigned int retry)
+{
+	if (INTEL_GEN(i915) >= 11)
+		return gen11_reset_engines(i915, engine_mask);
+	else
+		return gen6_reset_engines(i915, engine_mask, retry);
+}
+
+static int gen8_prepare_engine_for_reset(struct intel_engine_cs *engine,
+					 unsigned int retry)
+{
+	const bool force_reset_on_non_ready = retry >= 1;
+	int ret;
+
+	ret = gen8_reset_engine_start(engine);
+
+	if (ret && force_reset_on_non_ready) {
+		/*
+		 * Try to unblock a single non-ready engine by risking
+		 * context corruption.
+		 */
+		ret = reset_engines(engine->i915,
+				    intel_engine_flag(engine),
+				    retry);
+		if (!ret)
+			ret = gen8_reset_engine_start(engine);
+
+		DRM_ERROR("%s: reset request timeout, forcing reset (%d)\n",
+			  engine->name, ret);
+	}
+
+	return ret;
+}
+
 static int gen8_reset_engines(struct drm_i915_private *dev_priv,
 			      unsigned int engine_mask,
 			      unsigned int retry)
@@ -2122,16 +2155,12 @@ static int gen8_reset_engines(struct drm_i915_private *dev_priv,
 	int ret;
 
 	for_each_engine_masked(engine, dev_priv, engine_mask, tmp) {
-		if (gen8_reset_engine_start(engine)) {
-			ret = -EIO;
+		ret = gen8_prepare_engine_for_reset(engine, retry);
+		if (ret)
 			goto not_ready;
-		}
 	}
 
-	if (INTEL_GEN(dev_priv) >= 11)
-		ret = gen11_reset_engines(dev_priv, engine_mask);
-	else
-		ret = gen6_reset_engines(dev_priv, engine_mask, retry);
+	ret = reset_engines(dev_priv, engine_mask, retry);
 
 not_ready:
 	for_each_engine_masked(engine, dev_priv, engine_mask, tmp)
-- 
2.17.1

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

Re: [PATCH 2/2] drm/i915: Force reset on unready engine

[Chris Wilson]

Quoting Mika Kuoppala (2018-08-10 15:00:36)
> If engine reports that it is not ready for reset, we
> give up. Evidence shows that forcing a per engine reset
> on an engine which is not reporting to be ready for reset,
> can bring it back into a working order. There is risk that
> we corrupt the context image currently executing on that
> engine. But that is a risk worth taking as if we unblock
> the engine, we prevent a whole device wedging in a case
> of full gpu reset.
> 
> Reset individual engine even if it reports that it is not
> prepared for reset, but only if we aim for full gpu reset
> and not on first reset attempt.
> 
> v2: force reset only on later attempts, readability (Chris)
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_uncore.c | 49 +++++++++++++++++++++++------
>  1 file changed, 39 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 027d14574bfa..d24026839b17 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -2099,9 +2099,6 @@ static int gen8_reset_engine_start(struct intel_engine_cs *engine)
>                                            RESET_CTL_READY_TO_RESET,
>                                            700, 0,
>                                            NULL);
> -       if (ret)
> -               DRM_ERROR("%s: reset request timeout\n", engine->name);
> -
>         return ret;
>  }
>  
> @@ -2113,6 +2110,42 @@ static void gen8_reset_engine_cancel(struct intel_engine_cs *engine)
>                       _MASKED_BIT_DISABLE(RESET_CTL_REQUEST_RESET));
>  }
>  
> +static int reset_engines(struct drm_i915_private *i915,
> +                        unsigned int engine_mask,
> +                        unsigned int retry)
> +{
> +       if (INTEL_GEN(i915) >= 11)
> +               return gen11_reset_engines(i915, engine_mask);
> +       else
> +               return gen6_reset_engines(i915, engine_mask, retry);
> +}
> +
> +static int gen8_prepare_engine_for_reset(struct intel_engine_cs *engine,
> +                                        unsigned int retry)
> +{
> +       const bool force_reset_on_non_ready = retry >= 1;
> +       int ret;
> +
> +       ret = gen8_reset_engine_start(engine);
> +
> +       if (ret && force_reset_on_non_ready) {
> +               /*
> +                * Try to unblock a single non-ready engine by risking
> +                * context corruption.
> +                */
> +               ret = reset_engines(engine->i915,
> +                                   intel_engine_flag(engine),
> +                                   retry);
> +               if (!ret)
> +                       ret = gen8_reset_engine_start(engine);
> +
> +               DRM_ERROR("%s: reset request timeout, forcing reset (%d)\n",
> +                         engine->name, ret);

This looks dubious now ;)

After the force you then do a reset in the caller. Twice the reset for
twice the unpreparedness.

> +       }
> +
> +       return ret;
> +}
> +
>  static int gen8_reset_engines(struct drm_i915_private *dev_priv,
>                               unsigned int engine_mask,
>                               unsigned int retry)
> @@ -2122,16 +2155,12 @@ static int gen8_reset_engines(struct drm_i915_private *dev_priv,
>         int ret;
>  
>         for_each_engine_masked(engine, dev_priv, engine_mask, tmp) {
> -               if (gen8_reset_engine_start(engine)) {
> -                       ret = -EIO;
> +               ret = gen8_prepare_engine_for_reset(engine, retry);
> +               if (ret)

if (ret && !force_reset_unread)

>                         goto not_ready;
> -               }
>         }
>  
> -       if (INTEL_GEN(dev_priv) >= 11)
> -               ret = gen11_reset_engines(dev_priv, engine_mask);
> -       else
> -               ret = gen6_reset_engines(dev_priv, engine_mask, retry);
> +       ret = reset_engines(dev_priv, engine_mask, retry);
>  
>  not_ready:
>         for_each_engine_masked(engine, dev_priv, engine_mask, tmp)
> -- 
> 2.17.1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Expose retry count to per gen reset logic

[Chris Wilson]

Quoting Mika Kuoppala (2018-08-10 15:00:35)
> There is a possibility for per gen reset logic to
> be more nasty if the softer approach on resetting does
> not bear fruit.
> 
> Expose retry count to per gen reset logic if it
> wants to take such tough measures.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915: Expose retry count to per gen reset logic

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Expose retry count to per gen reset logic
URL   : https://patchwork.freedesktop.org/series/48019/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4643 -> Patchwork_9921 =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_9921 need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_9921, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/48019/revisions/1/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_9921:

  === IGT changes ===

    ==== Possible regressions ====

    igt@gem_exec_suspend@basic-s3:
      {fi-kbl-soraka}:    NOTRUN -> INCOMPLETE

    
    ==== Warnings ====

    igt@pm_rpm@basic-pci-d3-state:
      fi-glk-j4005:       PASS -> SKIP

    
== Known issues ==

  Here are the changes found in Patchwork_9921 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_selftest@live_requests:
      fi-skl-guc:         PASS -> DMESG-FAIL (fdo#106685)
      {fi-bsw-kefka}:     PASS -> INCOMPLETE (fdo#105876)

    igt@drv_selftest@live_workarounds:
      {fi-cfl-8109u}:     PASS -> DMESG-FAIL (fdo#107292)
      fi-cnl-psr:         PASS -> DMESG-FAIL (fdo#107292)

    
    ==== Possible fixes ====

    igt@drv_module_reload@basic-reload:
      fi-glk-j4005:       DMESG-WARN (fdo#106725, fdo#106248) -> PASS

    igt@kms_flip@basic-flip-vs-wf_vblank:
      fi-glk-j4005:       FAIL (fdo#100368) -> PASS

    
    ==== Warnings ====

    {igt@kms_psr@primary_page_flip}:
      fi-cnl-psr:         DMESG-FAIL (fdo#107372) -> DMESG-WARN (fdo#107372)

    
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#105876 https://bugs.freedesktop.org/show_bug.cgi?id=105876
  fdo#106248 https://bugs.freedesktop.org/show_bug.cgi?id=106248
  fdo#106685 https://bugs.freedesktop.org/show_bug.cgi?id=106685
  fdo#106725 https://bugs.freedesktop.org/show_bug.cgi?id=106725
  fdo#107292 https://bugs.freedesktop.org/show_bug.cgi?id=107292
  fdo#107372 https://bugs.freedesktop.org/show_bug.cgi?id=107372


== Participating hosts (53 -> 49) ==

  Additional (1): fi-kbl-soraka 
  Missing    (5): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-hsw-4200u 


== Build changes ==

    * Linux: CI_DRM_4643 -> Patchwork_9921

  CI_DRM_4643: 2fab0affe5a9f87309773bc4cbef828f4dde7acd @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4592: fce9638b2e60afce872b3056c19a729b1b3708be @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9921: 8d198b5b328dcebb6f57255fe4089dc739d51ff7 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

8d198b5b328d drm/i915: Force reset on unready engine
1848f1c440b6 drm/i915: Expose retry count to per gen reset logic

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9921/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for series starting with [1/2] drm/i915: Expose retry count to per gen reset logic

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Expose retry count to per gen reset logic
URL   : https://patchwork.freedesktop.org/series/48019/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4643_full -> Patchwork_9921_full =

== Summary - SUCCESS ==

  No regressions found.

  

== Known issues ==

  Here are the changes found in Patchwork_9921_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_suspend@fence-restore-untiled:
      shard-glk:          PASS -> FAIL (fdo#103375)

    
    ==== Possible fixes ====

    igt@gem_ppgtt@blt-vs-render-ctxn:
      shard-kbl:          INCOMPLETE (fdo#106023, fdo#103665) -> PASS

    igt@kms_mmap_write_crc:
      shard-hsw:          DMESG-WARN (fdo#102614) -> PASS

    igt@kms_setmode@basic:
      shard-apl:          FAIL (fdo#99912) -> PASS

    igt@perf@polling:
      shard-hsw:          FAIL (fdo#102252) -> PASS

    
  fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
  fdo#102614 https://bugs.freedesktop.org/show_bug.cgi?id=102614
  fdo#103375 https://bugs.freedesktop.org/show_bug.cgi?id=103375
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#106023 https://bugs.freedesktop.org/show_bug.cgi?id=106023
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912


== Participating hosts (5 -> 5) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4643 -> Patchwork_9921

  CI_DRM_4643: 2fab0affe5a9f87309773bc4cbef828f4dde7acd @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4592: fce9638b2e60afce872b3056c19a729b1b3708be @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9921: 8d198b5b328dcebb6f57255fe4089dc739d51ff7 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9921/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Force reset on unready engine

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> Quoting Mika Kuoppala (2018-08-10 15:00:36)
>> If engine reports that it is not ready for reset, we
>> give up. Evidence shows that forcing a per engine reset
>> on an engine which is not reporting to be ready for reset,
>> can bring it back into a working order. There is risk that
>> we corrupt the context image currently executing on that
>> engine. But that is a risk worth taking as if we unblock
>> the engine, we prevent a whole device wedging in a case
>> of full gpu reset.
>> 
>> Reset individual engine even if it reports that it is not
>> prepared for reset, but only if we aim for full gpu reset
>> and not on first reset attempt.
>> 
>> v2: force reset only on later attempts, readability (Chris)
>> 
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_uncore.c | 49 +++++++++++++++++++++++------
>>  1 file changed, 39 insertions(+), 10 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
>> index 027d14574bfa..d24026839b17 100644
>> --- a/drivers/gpu/drm/i915/intel_uncore.c
>> +++ b/drivers/gpu/drm/i915/intel_uncore.c
>> @@ -2099,9 +2099,6 @@ static int gen8_reset_engine_start(struct intel_engine_cs *engine)
>>                                            RESET_CTL_READY_TO_RESET,
>>                                            700, 0,
>>                                            NULL);
>> -       if (ret)
>> -               DRM_ERROR("%s: reset request timeout\n", engine->name);
>> -
>>         return ret;
>>  }
>>  
>> @@ -2113,6 +2110,42 @@ static void gen8_reset_engine_cancel(struct intel_engine_cs *engine)
>>                       _MASKED_BIT_DISABLE(RESET_CTL_REQUEST_RESET));
>>  }
>>  
>> +static int reset_engines(struct drm_i915_private *i915,
>> +                        unsigned int engine_mask,
>> +                        unsigned int retry)
>> +{
>> +       if (INTEL_GEN(i915) >= 11)
>> +               return gen11_reset_engines(i915, engine_mask);
>> +       else
>> +               return gen6_reset_engines(i915, engine_mask, retry);
>> +}
>> +
>> +static int gen8_prepare_engine_for_reset(struct intel_engine_cs *engine,
>> +                                        unsigned int retry)
>> +{
>> +       const bool force_reset_on_non_ready = retry >= 1;
>> +       int ret;
>> +
>> +       ret = gen8_reset_engine_start(engine);
>> +
>> +       if (ret && force_reset_on_non_ready) {
>> +               /*
>> +                * Try to unblock a single non-ready engine by risking
>> +                * context corruption.
>> +                */
>> +               ret = reset_engines(engine->i915,
>> +                                   intel_engine_flag(engine),
>> +                                   retry);
>> +               if (!ret)
>> +                       ret = gen8_reset_engine_start(engine);
>> +
>> +               DRM_ERROR("%s: reset request timeout, forcing reset (%d)\n",
>> +                         engine->name, ret);
>
> This looks dubious now ;)
>
> After the force you then do a reset in the caller. Twice the reset for
> twice the unpreparedness.

It is intentional. First we make the engine unstuck and then do
a full cycle with ready to reset involved. I don't know if
it really matters tho. It could be that the engine is already
in pristine condition after first.

I considered the engine reset here to only be the hammer,
and then the reset afterwards to be the validation.

-Mika

>
>> +       }
>> +
>> +       return ret;
>> +}
>> +
>>  static int gen8_reset_engines(struct drm_i915_private *dev_priv,
>>                               unsigned int engine_mask,
>>                               unsigned int retry)
>> @@ -2122,16 +2155,12 @@ static int gen8_reset_engines(struct drm_i915_private *dev_priv,
>>         int ret;
>>  
>>         for_each_engine_masked(engine, dev_priv, engine_mask, tmp) {
>> -               if (gen8_reset_engine_start(engine)) {
>> -                       ret = -EIO;
>> +               ret = gen8_prepare_engine_for_reset(engine, retry);
>> +               if (ret)
>
> if (ret && !force_reset_unread)
>
>>                         goto not_ready;
>> -               }
>>         }
>>  
>> -       if (INTEL_GEN(dev_priv) >= 11)
>> -               ret = gen11_reset_engines(dev_priv, engine_mask);
>> -       else
>> -               ret = gen6_reset_engines(dev_priv, engine_mask, retry);
>> +       ret = reset_engines(dev_priv, engine_mask, retry);
>>  
>>  not_ready:
>>         for_each_engine_masked(engine, dev_priv, engine_mask, tmp)
>> -- 
>> 2.17.1
>> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Force reset on unready engine

[Chris Wilson]

Quoting Mika Kuoppala (2018-08-13 09:18:07)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > Quoting Mika Kuoppala (2018-08-10 15:00:36)
> >> If engine reports that it is not ready for reset, we
> >> give up. Evidence shows that forcing a per engine reset
> >> on an engine which is not reporting to be ready for reset,
> >> can bring it back into a working order. There is risk that
> >> we corrupt the context image currently executing on that
> >> engine. But that is a risk worth taking as if we unblock
> >> the engine, we prevent a whole device wedging in a case
> >> of full gpu reset.
> >> 
> >> Reset individual engine even if it reports that it is not
> >> prepared for reset, but only if we aim for full gpu reset
> >> and not on first reset attempt.
> >> 
> >> v2: force reset only on later attempts, readability (Chris)
> >> 
> >> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >> Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_uncore.c | 49 +++++++++++++++++++++++------
> >>  1 file changed, 39 insertions(+), 10 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> >> index 027d14574bfa..d24026839b17 100644
> >> --- a/drivers/gpu/drm/i915/intel_uncore.c
> >> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> >> @@ -2099,9 +2099,6 @@ static int gen8_reset_engine_start(struct intel_engine_cs *engine)
> >>                                            RESET_CTL_READY_TO_RESET,
> >>                                            700, 0,
> >>                                            NULL);
> >> -       if (ret)
> >> -               DRM_ERROR("%s: reset request timeout\n", engine->name);
> >> -
> >>         return ret;
> >>  }
> >>  
> >> @@ -2113,6 +2110,42 @@ static void gen8_reset_engine_cancel(struct intel_engine_cs *engine)
> >>                       _MASKED_BIT_DISABLE(RESET_CTL_REQUEST_RESET));
> >>  }
> >>  
> >> +static int reset_engines(struct drm_i915_private *i915,
> >> +                        unsigned int engine_mask,
> >> +                        unsigned int retry)
> >> +{
> >> +       if (INTEL_GEN(i915) >= 11)
> >> +               return gen11_reset_engines(i915, engine_mask);
> >> +       else
> >> +               return gen6_reset_engines(i915, engine_mask, retry);
> >> +}
> >> +
> >> +static int gen8_prepare_engine_for_reset(struct intel_engine_cs *engine,
> >> +                                        unsigned int retry)
> >> +{
> >> +       const bool force_reset_on_non_ready = retry >= 1;
> >> +       int ret;
> >> +
> >> +       ret = gen8_reset_engine_start(engine);
> >> +
> >> +       if (ret && force_reset_on_non_ready) {
> >> +               /*
> >> +                * Try to unblock a single non-ready engine by risking
> >> +                * context corruption.
> >> +                */
> >> +               ret = reset_engines(engine->i915,
> >> +                                   intel_engine_flag(engine),
> >> +                                   retry);
> >> +               if (!ret)
> >> +                       ret = gen8_reset_engine_start(engine);
> >> +
> >> +               DRM_ERROR("%s: reset request timeout, forcing reset (%d)\n",
> >> +                         engine->name, ret);
> >
> > This looks dubious now ;)
> >
> > After the force you then do a reset in the caller. Twice the reset for
> > twice the unpreparedness.
> 
> It is intentional. First we make the engine unstuck and then do
> a full cycle with ready to reset involved. I don't know if
> it really matters tho. It could be that the engine is already
> in pristine condition after first.

Looks extremely weird way of going about it. If you want to do a double
reset after the first try fails, try

diff --git a/drivers/gpu/drm/i915/i915_reset.c b/drivers/gpu/drm/i915/i915_reset.c
index 4e5826045cbf..6fe137d7d455 100644
--- a/drivers/gpu/drm/i915/i915_reset.c
+++ b/drivers/gpu/drm/i915/i915_reset.c
@@ -450,6 +450,8 @@ int intel_gpu_reset(struct drm_i915_private *i915, unsigned int engine_mask)
                        GEM_TRACE("engine_mask=%x\n", engine_mask);
                        preempt_disable();
                        ret = reset(i915, engine_mask);
+                       if (retry > 0 && !ret) /* Double check reset worked */
+                               ret = reset(i915, engine_mask);
                        preempt_enable();
                }
                if (ret != -ETIMEDOUT || engine_mask != ALL_ENGINES)

as a separate patch.

P.S. you really need to review the i915_reset.c patch ;)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Force reset on unready engine

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> Quoting Mika Kuoppala (2018-08-13 09:18:07)
>> Chris Wilson <chris@chris-wilson.co.uk> writes:
>> 
>> > Quoting Mika Kuoppala (2018-08-10 15:00:36)
>> >> If engine reports that it is not ready for reset, we
>> >> give up. Evidence shows that forcing a per engine reset
>> >> on an engine which is not reporting to be ready for reset,
>> >> can bring it back into a working order. There is risk that
>> >> we corrupt the context image currently executing on that
>> >> engine. But that is a risk worth taking as if we unblock
>> >> the engine, we prevent a whole device wedging in a case
>> >> of full gpu reset.
>> >> 
>> >> Reset individual engine even if it reports that it is not
>> >> prepared for reset, but only if we aim for full gpu reset
>> >> and not on first reset attempt.
>> >> 
>> >> v2: force reset only on later attempts, readability (Chris)
>> >> 
>> >> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> >> Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>> >> ---
>> >>  drivers/gpu/drm/i915/intel_uncore.c | 49 +++++++++++++++++++++++------
>> >>  1 file changed, 39 insertions(+), 10 deletions(-)
>> >> 
>> >> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
>> >> index 027d14574bfa..d24026839b17 100644
>> >> --- a/drivers/gpu/drm/i915/intel_uncore.c
>> >> +++ b/drivers/gpu/drm/i915/intel_uncore.c
>> >> @@ -2099,9 +2099,6 @@ static int gen8_reset_engine_start(struct intel_engine_cs *engine)
>> >>                                            RESET_CTL_READY_TO_RESET,
>> >>                                            700, 0,
>> >>                                            NULL);
>> >> -       if (ret)
>> >> -               DRM_ERROR("%s: reset request timeout\n", engine->name);
>> >> -
>> >>         return ret;
>> >>  }
>> >>  
>> >> @@ -2113,6 +2110,42 @@ static void gen8_reset_engine_cancel(struct intel_engine_cs *engine)
>> >>                       _MASKED_BIT_DISABLE(RESET_CTL_REQUEST_RESET));
>> >>  }
>> >>  
>> >> +static int reset_engines(struct drm_i915_private *i915,
>> >> +                        unsigned int engine_mask,
>> >> +                        unsigned int retry)
>> >> +{
>> >> +       if (INTEL_GEN(i915) >= 11)
>> >> +               return gen11_reset_engines(i915, engine_mask);
>> >> +       else
>> >> +               return gen6_reset_engines(i915, engine_mask, retry);
>> >> +}
>> >> +
>> >> +static int gen8_prepare_engine_for_reset(struct intel_engine_cs *engine,
>> >> +                                        unsigned int retry)
>> >> +{
>> >> +       const bool force_reset_on_non_ready = retry >= 1;
>> >> +       int ret;
>> >> +
>> >> +       ret = gen8_reset_engine_start(engine);
>> >> +
>> >> +       if (ret && force_reset_on_non_ready) {
>> >> +               /*
>> >> +                * Try to unblock a single non-ready engine by risking
>> >> +                * context corruption.
>> >> +                */
>> >> +               ret = reset_engines(engine->i915,
>> >> +                                   intel_engine_flag(engine),
>> >> +                                   retry);
>> >> +               if (!ret)
>> >> +                       ret = gen8_reset_engine_start(engine);
>> >> +
>> >> +               DRM_ERROR("%s: reset request timeout, forcing reset (%d)\n",
>> >> +                         engine->name, ret);
>> >
>> > This looks dubious now ;)
>> >
>> > After the force you then do a reset in the caller. Twice the reset for
>> > twice the unpreparedness.
>> 
>> It is intentional. First we make the engine unstuck and then do
>> a full cycle with ready to reset involved. I don't know if
>> it really matters tho. It could be that the engine is already
>> in pristine condition after first.
>
> Looks extremely weird way of going about it. If you want to do a double
> reset after the first try fails, try

The crux is: failing to reset or failing to preparing to reset.
It is the ready for reset dance not working that we are trying to
by doing extra per engine reset. So trying in the higher lever,
again with the same preconditions would not help.

-Mika

>
> diff --git a/drivers/gpu/drm/i915/i915_reset.c b/drivers/gpu/drm/i915/i915_reset.c
> index 4e5826045cbf..6fe137d7d455 100644
> --- a/drivers/gpu/drm/i915/i915_reset.c
> +++ b/drivers/gpu/drm/i915/i915_reset.c
> @@ -450,6 +450,8 @@ int intel_gpu_reset(struct drm_i915_private *i915, unsigned int engine_mask)
>                         GEM_TRACE("engine_mask=%x\n", engine_mask);
>                         preempt_disable();
>                         ret = reset(i915, engine_mask);
> +                       if (retry > 0 && !ret) /* Double check reset worked */
> +                               ret = reset(i915, engine_mask);
>                         preempt_enable();
>                 }
>                 if (ret != -ETIMEDOUT || engine_mask != ALL_ENGINES)
>
> as a separate patch.
>
> P.S. you really need to review the i915_reset.c patch ;)
> -Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Force reset on unready engine

[Chris Wilson]

Quoting Mika Kuoppala (2018-08-13 10:58:10)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > Quoting Mika Kuoppala (2018-08-13 09:18:07)
> >> Chris Wilson <chris@chris-wilson.co.uk> writes:
> >> 
> >> > Quoting Mika Kuoppala (2018-08-10 15:00:36)
> >> >> If engine reports that it is not ready for reset, we
> >> >> give up. Evidence shows that forcing a per engine reset
> >> >> on an engine which is not reporting to be ready for reset,
> >> >> can bring it back into a working order. There is risk that
> >> >> we corrupt the context image currently executing on that
> >> >> engine. But that is a risk worth taking as if we unblock
> >> >> the engine, we prevent a whole device wedging in a case
> >> >> of full gpu reset.
> >> >> 
> >> >> Reset individual engine even if it reports that it is not
> >> >> prepared for reset, but only if we aim for full gpu reset
> >> >> and not on first reset attempt.
> >> >> 
> >> >> v2: force reset only on later attempts, readability (Chris)
> >> >> 
> >> >> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >> >> Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> >> >> ---
> >> >>  drivers/gpu/drm/i915/intel_uncore.c | 49 +++++++++++++++++++++++------
> >> >>  1 file changed, 39 insertions(+), 10 deletions(-)
> >> >> 
> >> >> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> >> >> index 027d14574bfa..d24026839b17 100644
> >> >> --- a/drivers/gpu/drm/i915/intel_uncore.c
> >> >> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> >> >> @@ -2099,9 +2099,6 @@ static int gen8_reset_engine_start(struct intel_engine_cs *engine)
> >> >>                                            RESET_CTL_READY_TO_RESET,
> >> >>                                            700, 0,
> >> >>                                            NULL);
> >> >> -       if (ret)
> >> >> -               DRM_ERROR("%s: reset request timeout\n", engine->name);
> >> >> -
> >> >>         return ret;
> >> >>  }
> >> >>  
> >> >> @@ -2113,6 +2110,42 @@ static void gen8_reset_engine_cancel(struct intel_engine_cs *engine)
> >> >>                       _MASKED_BIT_DISABLE(RESET_CTL_REQUEST_RESET));
> >> >>  }
> >> >>  
> >> >> +static int reset_engines(struct drm_i915_private *i915,
> >> >> +                        unsigned int engine_mask,
> >> >> +                        unsigned int retry)
> >> >> +{
> >> >> +       if (INTEL_GEN(i915) >= 11)
> >> >> +               return gen11_reset_engines(i915, engine_mask);
> >> >> +       else
> >> >> +               return gen6_reset_engines(i915, engine_mask, retry);
> >> >> +}
> >> >> +
> >> >> +static int gen8_prepare_engine_for_reset(struct intel_engine_cs *engine,
> >> >> +                                        unsigned int retry)
> >> >> +{
> >> >> +       const bool force_reset_on_non_ready = retry >= 1;
> >> >> +       int ret;
> >> >> +
> >> >> +       ret = gen8_reset_engine_start(engine);
> >> >> +
> >> >> +       if (ret && force_reset_on_non_ready) {
> >> >> +               /*
> >> >> +                * Try to unblock a single non-ready engine by risking
> >> >> +                * context corruption.
> >> >> +                */
> >> >> +               ret = reset_engines(engine->i915,
> >> >> +                                   intel_engine_flag(engine),
> >> >> +                                   retry);
> >> >> +               if (!ret)
> >> >> +                       ret = gen8_reset_engine_start(engine);
> >> >> +
> >> >> +               DRM_ERROR("%s: reset request timeout, forcing reset (%d)\n",
> >> >> +                         engine->name, ret);
> >> >
> >> > This looks dubious now ;)
> >> >
> >> > After the force you then do a reset in the caller. Twice the reset for
> >> > twice the unpreparedness.
> >> 
> >> It is intentional. First we make the engine unstuck and then do
> >> a full cycle with ready to reset involved. I don't know if
> >> it really matters tho. It could be that the engine is already
> >> in pristine condition after first.
> >
> > Looks extremely weird way of going about it. If you want to do a double
> > reset after the first try fails, try
> 
> The crux is: failing to reset or failing to preparing to reset.
> It is the ready for reset dance not working that we are trying to
> by doing extra per engine reset. So trying in the higher lever,
> again with the same preconditions would not help.

No, you made it into two.

The first is that we don't want to skip the reset even if we fail to
prepare (as a last resort).

The second is that you added the double reset.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/2] drm/i915: Force reset on unready engine

[Mika Kuoppala]

If engine reports that it is not ready for reset, we
give up. Evidence shows that forcing a per engine reset
on an engine which is not reporting to be ready for reset,
can bring it back into a working order. There is risk that
we corrupt the context image currently executing on that
engine. But that is a risk worth taking as if we unblock
the engine, we prevent a whole device wedging in a case
of full gpu reset.

Reset individual engine even if it reports that it is not
prepared for reset, but only if we aim for full gpu reset
and not on first reset attempt.

v2: force reset only on later attempts, readability (Chris)
v3: simplify with adequate caffeine levels (Chris)

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_uncore.c | 36 ++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 027d14574bfa..7cba14ae82a1 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -2085,7 +2085,7 @@ int __intel_wait_for_register(struct drm_i915_private *dev_priv,
 	return ret;
 }
 
-static int gen8_reset_engine_start(struct intel_engine_cs *engine)
+static int gen8_engine_reset_prepare(struct intel_engine_cs *engine)
 {
 	struct drm_i915_private *dev_priv = engine->i915;
 	int ret;
@@ -2105,7 +2105,7 @@ static int gen8_reset_engine_start(struct intel_engine_cs *engine)
 	return ret;
 }
 
-static void gen8_reset_engine_cancel(struct intel_engine_cs *engine)
+static void gen8_engine_reset_cancel(struct intel_engine_cs *engine)
 {
 	struct drm_i915_private *dev_priv = engine->i915;
 
@@ -2113,29 +2113,36 @@ static void gen8_reset_engine_cancel(struct intel_engine_cs *engine)
 		      _MASKED_BIT_DISABLE(RESET_CTL_REQUEST_RESET));
 }
 
+static int reset_engines(struct drm_i915_private *i915,
+			 unsigned int engine_mask,
+			 unsigned int retry)
+{
+	if (INTEL_GEN(i915) >= 11)
+		return gen11_reset_engines(i915, engine_mask);
+	else
+		return gen6_reset_engines(i915, engine_mask, retry);
+}
+
 static int gen8_reset_engines(struct drm_i915_private *dev_priv,
 			      unsigned int engine_mask,
 			      unsigned int retry)
 {
 	struct intel_engine_cs *engine;
+	const bool reset_non_ready = retry >= 1;
 	unsigned int tmp;
 	int ret;
 
 	for_each_engine_masked(engine, dev_priv, engine_mask, tmp) {
-		if (gen8_reset_engine_start(engine)) {
-			ret = -EIO;
-			goto not_ready;
-		}
+		ret = gen8_engine_reset_prepare(engine);
+		if (ret && !reset_non_ready)
+			goto skip_reset;
 	}
 
-	if (INTEL_GEN(dev_priv) >= 11)
-		ret = gen11_reset_engines(dev_priv, engine_mask);
-	else
-		ret = gen6_reset_engines(dev_priv, engine_mask, retry);
+	ret = reset_engines(dev_priv, engine_mask, retry);
 
-not_ready:
+skip_reset:
 	for_each_engine_masked(engine, dev_priv, engine_mask, tmp)
-		gen8_reset_engine_cancel(engine);
+		gen8_engine_reset_cancel(engine);
 
 	return ret;
 }
@@ -2164,12 +2171,15 @@ static reset_func intel_get_gpu_reset(struct drm_i915_private *dev_priv)
 		return NULL;
 }
 
-int intel_gpu_reset(struct drm_i915_private *dev_priv, unsigned int engine_mask)
+int intel_gpu_reset(struct drm_i915_private *dev_priv,
+		    const unsigned int engine_mask)
 {
 	reset_func reset = intel_get_gpu_reset(dev_priv);
 	unsigned int retry;
 	int ret;
 
+	GEM_BUG_ON(!engine_mask);
+
 	/*
 	 * We want to perform per-engine reset from atomic context (e.g.
 	 * softirq), which imposes the constraint that we cannot sleep.
-- 
2.17.1

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

Re: [PATCH 2/2] drm/i915: Force reset on unready engine

[Chris Wilson]

Quoting Mika Kuoppala (2018-08-13 11:42:42)
> If engine reports that it is not ready for reset, we
> give up. Evidence shows that forcing a per engine reset
> on an engine which is not reporting to be ready for reset,
> can bring it back into a working order. There is risk that
> we corrupt the context image currently executing on that
> engine. But that is a risk worth taking as if we unblock
> the engine, we prevent a whole device wedging in a case
> of full gpu reset.
> 
> Reset individual engine even if it reports that it is not
> prepared for reset, but only if we aim for full gpu reset
> and not on first reset attempt.
> 
> v2: force reset only on later attempts, readability (Chris)
> v3: simplify with adequate caffeine levels (Chris)
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

One last thing, you said you recalled one of the reasons for its
existence was to prevent machine lockups on kbl. Is the recollection
true? Do we want to leave a comment in case of fire?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Force reset on unready engine

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> Quoting Mika Kuoppala (2018-08-13 11:42:42)
>> If engine reports that it is not ready for reset, we
>> give up. Evidence shows that forcing a per engine reset
>> on an engine which is not reporting to be ready for reset,
>> can bring it back into a working order. There is risk that
>> we corrupt the context image currently executing on that
>> engine. But that is a risk worth taking as if we unblock
>> the engine, we prevent a whole device wedging in a case
>> of full gpu reset.
>> 
>> Reset individual engine even if it reports that it is not
>> prepared for reset, but only if we aim for full gpu reset
>> and not on first reset attempt.
>> 
>> v2: force reset only on later attempts, readability (Chris)
>> v3: simplify with adequate caffeine levels (Chris)
>> 
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>
> One last thing, you said you recalled one of the reasons for its
> existence was to prevent machine lockups on kbl. Is the recollection
> true? Do we want to leave a comment in case of fire?

We got machine lockups if we did reset a non stopped, active
engine inside a batchbuffer. i915_stop_engines() arise from that
and we have a comment in intel_gpu_reset explaining it. That lockup
did apparently happen regardless of ready-to-reset ack.

How I read it is that we got ready-to-reset acks on
active engines, which then died if we proceed. So this
patch should not make things worse as i915_stop_engines
have hold water.

-Mika *knocks on wood*

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

Re: [PATCH 2/2] drm/i915: Force reset on unready engine

[Chris Wilson]

Quoting Mika Kuoppala (2018-08-13 12:02:51)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > Quoting Mika Kuoppala (2018-08-13 11:42:42)
> >> If engine reports that it is not ready for reset, we
> >> give up. Evidence shows that forcing a per engine reset
> >> on an engine which is not reporting to be ready for reset,
> >> can bring it back into a working order. There is risk that
> >> we corrupt the context image currently executing on that
> >> engine. But that is a risk worth taking as if we unblock
> >> the engine, we prevent a whole device wedging in a case
> >> of full gpu reset.
> >> 
> >> Reset individual engine even if it reports that it is not
> >> prepared for reset, but only if we aim for full gpu reset
> >> and not on first reset attempt.
> >> 
> >> v2: force reset only on later attempts, readability (Chris)
> >> v3: simplify with adequate caffeine levels (Chris)
> >> 
> >> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >> Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> >
> > One last thing, you said you recalled one of the reasons for its
> > existence was to prevent machine lockups on kbl. Is the recollection
> > true? Do we want to leave a comment in case of fire?
> 
> We got machine lockups if we did reset a non stopped, active
> engine inside a batchbuffer. i915_stop_engines() arise from that
> and we have a comment in intel_gpu_reset explaining it. That lockup
> did apparently happen regardless of ready-to-reset ack.
> 
> How I read it is that we got ready-to-reset acks on
> active engines, which then died if we proceed. So this
> patch should not make things worse as i915_stop_engines
> have hold water.

Ok, I still think a comment about the danger for last-chance with a
reference back to i915_stop_engines will be useful for future
archaeology.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915: Expose retry count to per gen reset logic (rev3)

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Expose retry count to per gen reset logic (rev3)
URL   : https://patchwork.freedesktop.org/series/48019/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4660 -> Patchwork_9928 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/48019/revisions/3/mbox/

== Known issues ==

  Here are the changes found in Patchwork_9928 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_selftest@live_hangcheck:
      fi-kbl-7560u:       PASS -> DMESG-FAIL (fdo#106947, fdo#106560)

    igt@drv_selftest@live_workarounds:
      {fi-cfl-8109u}:     PASS -> DMESG-FAIL (fdo#107292)
      fi-cfl-s3:          PASS -> DMESG-FAIL (fdo#107292)
      fi-cnl-psr:         PASS -> DMESG-FAIL (fdo#107292)
      fi-kbl-7560u:       PASS -> DMESG-FAIL (fdo#107292)

    
    ==== Possible fixes ====

    igt@drv_selftest@live_hangcheck:
      fi-skl-guc:         DMESG-FAIL (fdo#107174) -> PASS

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a:
      {fi-byt-clapper}:   FAIL (fdo#107362) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
      fi-skl-guc:         FAIL (fdo#103191) -> PASS

    {igt@kms_psr@primary_mmap_gtt}:
      fi-cnl-psr:         DMESG-WARN (fdo#107372) -> PASS

    
    ==== Warnings ====

    {igt@kms_psr@primary_page_flip}:
      fi-cnl-psr:         DMESG-FAIL (fdo#107372) -> DMESG-WARN (fdo#107372)

    
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#106560 https://bugs.freedesktop.org/show_bug.cgi?id=106560
  fdo#106947 https://bugs.freedesktop.org/show_bug.cgi?id=106947
  fdo#107174 https://bugs.freedesktop.org/show_bug.cgi?id=107174
  fdo#107292 https://bugs.freedesktop.org/show_bug.cgi?id=107292
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
  fdo#107372 https://bugs.freedesktop.org/show_bug.cgi?id=107372


== Participating hosts (54 -> 49) ==

  Missing    (5): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-hsw-4200u 


== Build changes ==

    * Linux: CI_DRM_4660 -> Patchwork_9928

  CI_DRM_4660: 9cd882754c1017e68ed9d2c8d57dc326530522fe @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4592: fce9638b2e60afce872b3056c19a729b1b3708be @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9928: 43b30337b551eda6a92a54cd873bcd4256da34cd @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

43b30337b551 drm/i915: Force reset on unready engine
b6a14da11f65 drm/i915: Expose retry count to per gen reset logic

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9928/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for series starting with [1/2] drm/i915: Expose retry count to per gen reset logic (rev3)

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Expose retry count to per gen reset logic (rev3)
URL   : https://patchwork.freedesktop.org/series/48019/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4660_full -> Patchwork_9928_full =

== Summary - SUCCESS ==

  No regressions found.

  

== Known issues ==

  Here are the changes found in Patchwork_9928_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_suspend@fence-restore-untiled:
      shard-glk:          PASS -> FAIL (fdo#103375)

    igt@drv_suspend@shrink:
      shard-snb:          PASS -> FAIL (fdo#106886)
      shard-glk:          PASS -> FAIL (fdo#106886)

    igt@kms_setmode@basic:
      shard-apl:          PASS -> FAIL (fdo#99912)

    igt@perf@polling:
      shard-hsw:          PASS -> FAIL (fdo#102252)

    
    ==== Possible fixes ====

    igt@gem_eio@reset-stress:
      shard-apl:          FAIL -> PASS

    
  fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
  fdo#103375 https://bugs.freedesktop.org/show_bug.cgi?id=103375
  fdo#106886 https://bugs.freedesktop.org/show_bug.cgi?id=106886
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912


== Participating hosts (5 -> 5) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4660 -> Patchwork_9928

  CI_DRM_4660: 9cd882754c1017e68ed9d2c8d57dc326530522fe @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4592: fce9638b2e60afce872b3056c19a729b1b3708be @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9928: 43b30337b551eda6a92a54cd873bcd4256da34cd @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9928/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/2] drm/i915: Force reset on unready engine

[Mika Kuoppala]

If engine reports that it is not ready for reset, we
give up. Evidence shows that forcing a per engine reset
on an engine which is not reporting to be ready for reset,
can bring it back into a working order. There is risk that
we corrupt the context image currently executing on that
engine. But that is a risk worth taking as if we unblock
the engine, we prevent a whole device wedging in a case
of full gpu reset.

Reset individual engine even if it reports that it is not
prepared for reset, but only if we aim for full gpu reset
and not on first reset attempt.

v2: force reset only on later attempts, readability (Chris)
v3: simplify with adequate caffeine levels (Chris)
v4: comment about risks and migitations (Chris)

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_uncore.c | 50 +++++++++++++++++++++--------
 1 file changed, 37 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 027d14574bfa..20f2f5ad9c3f 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -2085,7 +2085,7 @@ int __intel_wait_for_register(struct drm_i915_private *dev_priv,
 	return ret;
 }
 
-static int gen8_reset_engine_start(struct intel_engine_cs *engine)
+static int gen8_engine_reset_prepare(struct intel_engine_cs *engine)
 {
 	struct drm_i915_private *dev_priv = engine->i915;
 	int ret;
@@ -2105,7 +2105,7 @@ static int gen8_reset_engine_start(struct intel_engine_cs *engine)
 	return ret;
 }
 
-static void gen8_reset_engine_cancel(struct intel_engine_cs *engine)
+static void gen8_engine_reset_cancel(struct intel_engine_cs *engine)
 {
 	struct drm_i915_private *dev_priv = engine->i915;
 
@@ -2113,29 +2113,50 @@ static void gen8_reset_engine_cancel(struct intel_engine_cs *engine)
 		      _MASKED_BIT_DISABLE(RESET_CTL_REQUEST_RESET));
 }
 
+static int reset_engines(struct drm_i915_private *i915,
+			 unsigned int engine_mask,
+			 unsigned int retry)
+{
+	if (INTEL_GEN(i915) >= 11)
+		return gen11_reset_engines(i915, engine_mask);
+	else
+		return gen6_reset_engines(i915, engine_mask, retry);
+}
+
 static int gen8_reset_engines(struct drm_i915_private *dev_priv,
 			      unsigned int engine_mask,
 			      unsigned int retry)
 {
 	struct intel_engine_cs *engine;
+	const bool reset_non_ready = retry >= 1;
 	unsigned int tmp;
 	int ret;
 
 	for_each_engine_masked(engine, dev_priv, engine_mask, tmp) {
-		if (gen8_reset_engine_start(engine)) {
-			ret = -EIO;
-			goto not_ready;
-		}
+		ret = gen8_engine_reset_prepare(engine);
+		if (ret && !reset_non_ready)
+			goto skip_reset;
+
+		/*
+		 * If this is not the first failed attempt to prepare,
+		 * we decide to proceed anyway.
+		 *
+		 * By doing so we risk context corruption and with
+		 * some gens (kbl), possible system hang if reset
+		 * happens during active bb execution.
+		 *
+		 * We rather take context corruption instead of
+		 * failed reset with a wedged driver/gpu. And
+		 * active bb execution case should be covered by
+		 * i915_stop_engines we have before the reset.
+		 */
 	}
 
-	if (INTEL_GEN(dev_priv) >= 11)
-		ret = gen11_reset_engines(dev_priv, engine_mask);
-	else
-		ret = gen6_reset_engines(dev_priv, engine_mask, retry);
+	ret = reset_engines(dev_priv, engine_mask, retry);
 
-not_ready:
+skip_reset:
 	for_each_engine_masked(engine, dev_priv, engine_mask, tmp)
-		gen8_reset_engine_cancel(engine);
+		gen8_engine_reset_cancel(engine);
 
 	return ret;
 }
@@ -2164,12 +2185,15 @@ static reset_func intel_get_gpu_reset(struct drm_i915_private *dev_priv)
 		return NULL;
 }
 
-int intel_gpu_reset(struct drm_i915_private *dev_priv, unsigned int engine_mask)
+int intel_gpu_reset(struct drm_i915_private *dev_priv,
+		    const unsigned int engine_mask)
 {
 	reset_func reset = intel_get_gpu_reset(dev_priv);
 	unsigned int retry;
 	int ret;
 
+	GEM_BUG_ON(!engine_mask);
+
 	/*
 	 * We want to perform per-engine reset from atomic context (e.g.
 	 * softirq), which imposes the constraint that we cannot sleep.
-- 
2.17.1

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

✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915: Expose retry count to per gen reset logic (rev4)

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Expose retry count to per gen reset logic (rev4)
URL   : https://patchwork.freedesktop.org/series/48019/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4662 -> Patchwork_9929 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/48019/revisions/4/mbox/

== Known issues ==

  Here are the changes found in Patchwork_9929 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    {igt@amdgpu/amd_prime@amd-to-i915}:
      {fi-kbl-8809g}:     NOTRUN -> FAIL (fdo#107341)

    igt@drv_selftest@live_hangcheck:
      fi-skl-guc:         PASS -> DMESG-FAIL (fdo#107174)

    igt@drv_selftest@live_requests:
      fi-bsw-n3050:       PASS -> INCOMPLETE (fdo#105876)

    igt@drv_selftest@live_workarounds:
      fi-bsw-n3050:       PASS -> DMESG-FAIL (fdo#107292)

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a:
      {fi-byt-clapper}:   PASS -> FAIL (fdo#107362)

    
    ==== Possible fixes ====

    {igt@amdgpu/amd_basic@userptr}:
      {fi-kbl-8809g}:     INCOMPLETE (fdo#107402) -> PASS

    igt@kms_frontbuffer_tracking@basic:
      {fi-byt-clapper}:   FAIL (fdo#103167) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
      fi-bxt-dsi:         INCOMPLETE (fdo#103927) -> PASS

    
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#105876 https://bugs.freedesktop.org/show_bug.cgi?id=105876
  fdo#107174 https://bugs.freedesktop.org/show_bug.cgi?id=107174
  fdo#107292 https://bugs.freedesktop.org/show_bug.cgi?id=107292
  fdo#107341 https://bugs.freedesktop.org/show_bug.cgi?id=107341
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
  fdo#107402 https://bugs.freedesktop.org/show_bug.cgi?id=107402


== Participating hosts (54 -> 48) ==

  Missing    (6): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 


== Build changes ==

    * Linux: CI_DRM_4662 -> Patchwork_9929

  CI_DRM_4662: 3010d040760de7eb87c6eb54985ba6220447bbba @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4593: c88e219c6e890d89b7836c5e248ffedf334d55a2 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9929: 871b39c1a7e41c61b09fe26c6dd445a59ba4477b @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

871b39c1a7e4 drm/i915: Force reset on unready engine
b0f40a9d4041 drm/i915: Expose retry count to per gen reset logic

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9929/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Expose retry count to per gen reset logic

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> Quoting Mika Kuoppala (2018-08-10 15:00:35)
>> There is a possibility for per gen reset logic to
>> be more nasty if the softer approach on resetting does
>> not bear fruit.
>> 
>> Expose retry count to per gen reset logic if it
>> wants to take such tough measures.
>> 
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Pushed patches 1&2, thanks for review!
-Mika
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for series starting with [1/2] drm/i915: Expose retry count to per gen reset logic (rev4)

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Expose retry count to per gen reset logic (rev4)
URL   : https://patchwork.freedesktop.org/series/48019/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4662_full -> Patchwork_9929_full =

== Summary - SUCCESS ==

  No regressions found.

  

== Known issues ==

  Here are the changes found in Patchwork_9929_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_flip@2x-flip-vs-expired-vblank:
      shard-glk:          PASS -> FAIL (fdo#105363)

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-render:
      shard-snb:          NOTRUN -> INCOMPLETE (fdo#105411)

    igt@kms_rotation_crc@sprite-rotation-180:
      shard-hsw:          PASS -> FAIL (fdo#103925)

    
    ==== Possible fixes ====

    igt@kms_flip@flip-vs-expired-vblank:
      shard-kbl:          FAIL (fdo#102887, fdo#105363) -> PASS

    igt@kms_setmode@basic:
      shard-kbl:          FAIL (fdo#99912) -> PASS

    igt@perf_pmu@busy-accuracy-50-bcs0:
      shard-snb:          INCOMPLETE (fdo#105411) -> SKIP

    
  fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
  fdo#103925 https://bugs.freedesktop.org/show_bug.cgi?id=103925
  fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363
  fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912


== Participating hosts (5 -> 5) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4662 -> Patchwork_9929

  CI_DRM_4662: 3010d040760de7eb87c6eb54985ba6220447bbba @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4593: c88e219c6e890d89b7836c5e248ffedf334d55a2 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9929: 871b39c1a7e41c61b09fe26c6dd445a59ba4477b @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9929/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx