[PATCH] drm/i915: Request for resets under forcewake

[PATCH] drm/i915: Request for resets under forcewake

[Mika Kuoppala]

We have a timed release of a forcewake when using
I915_READ/WRITE macros. wait_for() macro will go to quite
long sleep if the first read doesn't satisfy the condition for
successful exit. With these two interacting, it is possible that
we lose the forcewake during the wait_for() and the subsequent read
will reaquire forcewake.

Further experiments with skl shows that when we lose forcewake,
we lose the reset request we submitted. So this register
is not power context saved.

Grab forcewakes for all engines before starting to request for
resets so that all requests stay valid for the duration of reset
requisition across all the engines.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92774
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Tested-by: Tomi Sarvela <tomix.p.sarvela@intel.com>
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/intel_uncore.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index f0f97b2..5a6e7f1b 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -1483,6 +1483,8 @@ static int gen8_do_reset(struct drm_device *dev)
 	struct intel_engine_cs *engine;
 	int i;
 
+	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
+
 	for_each_ring(engine, dev_priv, i) {
 		I915_WRITE(RING_RESET_CTL(engine->mmio_base),
 			   _MASKED_BIT_ENABLE(RESET_CTL_REQUEST_RESET));
@@ -1497,6 +1499,8 @@ static int gen8_do_reset(struct drm_device *dev)
 		}
 	}
 
+	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
+
 	return gen6_do_reset(dev);
 
 not_ready:
@@ -1504,6 +1508,8 @@ not_ready:
 		I915_WRITE(RING_RESET_CTL(engine->mmio_base),
 			   _MASKED_BIT_DISABLE(RESET_CTL_REQUEST_RESET));
 
+	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
+
 	return -EIO;
 }
 
-- 
2.5.0

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

Re: [PATCH] drm/i915: Request for resets under forcewake

[Chris Wilson]

On Wed, Nov 04, 2015 at 11:33:04AM +0200, Mika Kuoppala wrote:
> We have a timed release of a forcewake when using
> I915_READ/WRITE macros. wait_for() macro will go to quite
> long sleep if the first read doesn't satisfy the condition for
> successful exit. With these two interacting, it is possible that
> we lose the forcewake during the wait_for() and the subsequent read
> will reaquire forcewake.
> 
> Further experiments with skl shows that when we lose forcewake,
> we lose the reset request we submitted. So this register
> is not power context saved.
> 
> Grab forcewakes for all engines before starting to request for
> resets so that all requests stay valid for the duration of reset
> requisition across all the engines.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92774
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Tested-by: Tomi Sarvela <tomix.p.sarvela@intel.com>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_uncore.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index f0f97b2..5a6e7f1b 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -1483,6 +1483,8 @@ static int gen8_do_reset(struct drm_device *dev)
>  	struct intel_engine_cs *engine;
>  	int i;
>  
        /* If the power well sleeps during the reset, the reset
	 * request may be dropped and never completes (causing -EIO).
	 */
> +	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);

I wonder if this would be more useful in intel_gpu_reset(), it won't
hurt any other platforms and may prevent similar snafu in future?
-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: Request for resets under forcewake

[Mika Kuoppala]

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

> On Wed, Nov 04, 2015 at 11:33:04AM +0200, Mika Kuoppala wrote:
>> We have a timed release of a forcewake when using
>> I915_READ/WRITE macros. wait_for() macro will go to quite
>> long sleep if the first read doesn't satisfy the condition for
>> successful exit. With these two interacting, it is possible that
>> we lose the forcewake during the wait_for() and the subsequent read
>> will reaquire forcewake.
>> 
>> Further experiments with skl shows that when we lose forcewake,
>> we lose the reset request we submitted. So this register
>> is not power context saved.
>> 
>> Grab forcewakes for all engines before starting to request for
>> resets so that all requests stay valid for the duration of reset
>> requisition across all the engines.
>> 
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92774
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Tested-by: Tomi Sarvela <tomix.p.sarvela@intel.com>
>> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_uncore.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
>> index f0f97b2..5a6e7f1b 100644
>> --- a/drivers/gpu/drm/i915/intel_uncore.c
>> +++ b/drivers/gpu/drm/i915/intel_uncore.c
>> @@ -1483,6 +1483,8 @@ static int gen8_do_reset(struct drm_device *dev)
>>  	struct intel_engine_cs *engine;
>>  	int i;
>>  
>         /* If the power well sleeps during the reset, the reset
> 	 * request may be dropped and never completes (causing -EIO).
> 	 */
>> +	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
>
> I wonder if this would be more useful in intel_gpu_reset(), it won't
> hurt any other platforms and may prevent similar snafu in future?

I had similar thoughts but chickened out due to forcewake_reset ending
up inside the call chain on such setup.

Now with a refreshing my memory, the forcewake_reset should handle the 'restore'
case.

-Mika

> -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

[PATCH] drm/i915: Do graphics device reset under forcewake

[Mika Kuoppala]

We have a timed release of a forcewake when using
I915_READ/WRITE macros. wait_for() macro will go to quite
long sleep if the first read doesn't satisfy the condition for
successful exit. With these two interacting, it is possible that
we lose the forcewake during the wait_for() and the subsequent read
will reaquire forcewake.

Further experiments with skl shows that when we lose forcewake,
we lose the reset request we submitted. So reset request register
is not power context saved.

Grab forcewakes for all engines before starting the reset/request
dance so that all requests stay valid for the duration of reset
requisition across all the engines.

v2: Add comment on power well sleeps. Wrap the reset handling
    under forcewake instead of just reset requests (Chris)

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92774
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Tested-by: Tomi Sarvela <tomix.p.sarvela@intel.com> (v1, v2)
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/intel_uncore.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index f0f97b2..5bb269c 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -1530,13 +1530,22 @@ static int (*intel_get_gpu_reset(struct drm_device *dev))(struct drm_device *)
 
 int intel_gpu_reset(struct drm_device *dev)
 {
+	struct drm_i915_private *dev_priv = to_i915(dev);
 	int (*reset)(struct drm_device *);
+	int ret;
 
 	reset = intel_get_gpu_reset(dev);
 	if (reset == NULL)
 		return -ENODEV;
 
-	return reset(dev);
+	/* If the power well sleeps during the reset, the reset
+	 * request may be dropped and never completes (causing -EIO).
+	 */
+	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
+	ret = reset(dev);
+	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
+
+	return ret;
 }
 
 bool intel_has_gpu_reset(struct drm_device *dev)
-- 
2.5.0

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

Re: [PATCH] drm/i915: Do graphics device reset under forcewake

[Chris Wilson]

On Thu, Nov 05, 2015 at 01:11:38PM +0200, Mika Kuoppala wrote:
> We have a timed release of a forcewake when using
> I915_READ/WRITE macros. wait_for() macro will go to quite
> long sleep if the first read doesn't satisfy the condition for
> successful exit. With these two interacting, it is possible that
> we lose the forcewake during the wait_for() and the subsequent read
> will reaquire forcewake.
> 
> Further experiments with skl shows that when we lose forcewake,
> we lose the reset request we submitted. So reset request register
> is not power context saved.
> 
> Grab forcewakes for all engines before starting the reset/request
> dance so that all requests stay valid for the duration of reset
> requisition across all the engines.
> 
> v2: Add comment on power well sleeps. Wrap the reset handling
>     under forcewake instead of just reset requests (Chris)
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92774
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Tested-by: Tomi Sarvela <tomix.p.sarvela@intel.com> (v1, v2)
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-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: Do graphics device reset under forcewake

[Jani Nikula]

On Thu, 05 Nov 2015, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Thu, Nov 05, 2015 at 01:11:38PM +0200, Mika Kuoppala wrote:
>> We have a timed release of a forcewake when using
>> I915_READ/WRITE macros. wait_for() macro will go to quite
>> long sleep if the first read doesn't satisfy the condition for
>> successful exit. With these two interacting, it is possible that
>> we lose the forcewake during the wait_for() and the subsequent read
>> will reaquire forcewake.
>> 
>> Further experiments with skl shows that when we lose forcewake,
>> we lose the reset request we submitted. So reset request register
>> is not power context saved.
>> 
>> Grab forcewakes for all engines before starting the reset/request
>> dance so that all requests stay valid for the duration of reset
>> requisition across all the engines.
>> 
>> v2: Add comment on power well sleeps. Wrap the reset handling
>>     under forcewake instead of just reset requests (Chris)
>> 
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92774
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Tested-by: Tomi Sarvela <tomix.p.sarvela@intel.com> (v1, v2)
>> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Pushed to drm-intel-next-fixes, thanks for the patch and review.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx