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

[Mika Kuoppala]

Chris Wilson <chris at 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 at chris-wilson.co.uk>
>> Tested-by: Tomi Sarvela <tomix.p.sarvela at intel.com>
>> Signed-off-by: Mika Kuoppala <mika.kuoppala at 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