[PATCH] drm/i915: Stop touching forcewake following a gen6+ engine reset

[PATCH] drm/i915: Stop touching forcewake following a gen6+ engine reset

[Chris Wilson]

Forcewake is not affected by the engine reset on gen6+. Indeed the
reason why we added intel_uncore_forcewake_reset() to
gen6_reset_engines() was to keep the bookkeeping intact because the
reset did not touch the forcewake bit (yet we cancelled the forcewake
consumers)!  This was done in commit 521198a2e7095:
    Author: Mika Kuoppala <mika.kuoppala@linux.intel.com>
    Date:   Fri Aug 23 16:52:30 2013 +0300

	drm/i915: sanitize forcewake registers on reset

	In reset we try to restore the forcewake state to
	pre reset state, using forcewake_count. The reset
	doesn't seem to clear the forcewake bits so we
	get warn on forcewake ack register not clearing.

That futzing of the forcewake bookkeeping was dropped in commit
0294ae7b44bb ("drm/i915: Consolidate forcewake resetting to a single
function"), but it did not make the realisation that the remaining
intel_uncore_forcewake_reset() was redundant.

The new danger with using intel_uncore_forcewake_reset() with per-engine
resets is that the driver and hw are still in an active state as we
perform the reset. We may be using the forcewake to read protected
registers elsewhere and those results may be clobbered by the concurrent
dropping of forcewake.

Reported-by: Michel Thierry <michel.thierry@intel.com>
Fixes: 142bc7d99bcf ("drm/i915: Modify error handler for per engine hang recovery")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Cc: Michel Thierry <michel.thierry@intel.com>
---
 drivers/gpu/drm/i915/intel_uncore.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index deb4430541cf..1d7b879cc68c 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -1497,7 +1497,6 @@ static int gen6_reset_engines(struct drm_i915_private *dev_priv,
 		[VECS] = GEN6_GRDOM_VECS,
 	};
 	u32 hw_mask;
-	int ret;
 
 	if (engine_mask == ALL_ENGINES) {
 		hw_mask = GEN6_GRDOM_FULL;
@@ -1509,11 +1508,7 @@ static int gen6_reset_engines(struct drm_i915_private *dev_priv,
 			hw_mask |= hw_engine_mask[engine->id];
 	}
 
-	ret = gen6_hw_domain_reset(dev_priv, hw_mask);
-
-	intel_uncore_forcewake_reset(dev_priv, true);
-
-	return ret;
+	return gen6_hw_domain_reset(dev_priv, hw_mask);
 }
 
 /**
-- 
2.14.1

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

Re: [PATCH] drm/i915: Stop touching forcewake following a gen6+ engine reset

[Michel Thierry]

On 17/08/17 10:32, Chris Wilson wrote:
> Forcewake is not affected by the engine reset on gen6+. Indeed the
> reason why we added intel_uncore_forcewake_reset() to
> gen6_reset_engines() was to keep the bookkeeping intact because the
> reset did not touch the forcewake bit (yet we cancelled the forcewake
> consumers)!  This was done in commit 521198a2e7095:
>      Author: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>      Date:   Fri Aug 23 16:52:30 2013 +0300
> 
> 	drm/i915: sanitize forcewake registers on reset
> 
> 	In reset we try to restore the forcewake state to
> 	pre reset state, using forcewake_count. The reset
> 	doesn't seem to clear the forcewake bits so we
> 	get warn on forcewake ack register not clearing.
> 
> That futzing of the forcewake bookkeeping was dropped in commit
> 0294ae7b44bb ("drm/i915: Consolidate forcewake resetting to a single
> function"), but it did not make the realisation that the remaining
> intel_uncore_forcewake_reset() was redundant.
> 
> The new danger with using intel_uncore_forcewake_reset() with per-engine
> resets is that the driver and hw are still in an active state as we
> perform the reset. We may be using the forcewake to read protected
> registers elsewhere and those results may be clobbered by the concurrent
> dropping of forcewake.
> 
> Reported-by: Michel Thierry <michel.thierry@intel.com>
> Fixes: 142bc7d99bcf ("drm/i915: Modify error handler for per engine hang recovery")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Cc: Michel Thierry <michel.thierry@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_uncore.c | 7 +------
>   1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index deb4430541cf..1d7b879cc68c 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -1497,7 +1497,6 @@ static int gen6_reset_engines(struct drm_i915_private *dev_priv,
>   		[VECS] = GEN6_GRDOM_VECS,
>   	};
>   	u32 hw_mask;
> -	int ret;
>   
>   	if (engine_mask == ALL_ENGINES) {
>   		hw_mask = GEN6_GRDOM_FULL;
> @@ -1509,11 +1508,7 @@ static int gen6_reset_engines(struct drm_i915_private *dev_priv,
>   			hw_mask |= hw_engine_mask[engine->id];
>   	}
>   
> -	ret = gen6_hw_domain_reset(dev_priv, hw_mask);
> -
> -	intel_uncore_forcewake_reset(dev_priv, true);
> -
> -	return ret;
> +	return gen6_hw_domain_reset(dev_priv, hw_mask);
>   }
>   
>   /**
> 

Thanks for digging out the commit history.

Reviewed-by Michel Thierry <michel.thierry@intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915: Stop touching forcewake following a gen6+ engine reset

[Patchwork]

== Series Details ==

Series: drm/i915: Stop touching forcewake following a gen6+ engine reset
URL   : https://patchwork.freedesktop.org/series/28936/
State : success

== Summary ==

Series 28936v1 drm/i915: Stop touching forcewake following a gen6+ engine reset
https://patchwork.freedesktop.org/api/1.0/series/28936/revisions/1/mbox/

fi-bdw-5557u     total:279  pass:268  dwarn:0   dfail:0   fail:0   skip:11  time:458s
fi-bdw-gvtdvm    total:279  pass:265  dwarn:0   dfail:0   fail:0   skip:14  time:437s
fi-blb-e6850     total:279  pass:224  dwarn:1   dfail:0   fail:0   skip:54  time:358s
fi-bsw-n3050     total:279  pass:243  dwarn:0   dfail:0   fail:0   skip:36  time:545s
fi-bxt-j4205     total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:523s
fi-byt-j1900     total:279  pass:254  dwarn:1   dfail:0   fail:0   skip:24  time:529s
fi-glk-2a        total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:619s
fi-hsw-4770      total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:445s
fi-hsw-4770r     total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:426s
fi-ilk-650       total:279  pass:229  dwarn:0   dfail:0   fail:0   skip:50  time:421s
fi-ivb-3520m     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:509s
fi-ivb-3770      total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:478s
fi-kbl-7500u     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:475s
fi-kbl-7560u     total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:601s
fi-kbl-r         total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:597s
fi-pnv-d510      total:279  pass:223  dwarn:1   dfail:0   fail:0   skip:55  time:531s
fi-skl-6260u     total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:471s
fi-skl-6700k     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:477s
fi-skl-6770hq    total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:490s
fi-skl-gvtdvm    total:279  pass:266  dwarn:0   dfail:0   fail:0   skip:13  time:444s
fi-skl-x1585l    total:279  pass:268  dwarn:0   dfail:0   fail:0   skip:11  time:484s
fi-snb-2520m     total:279  pass:251  dwarn:0   dfail:0   fail:0   skip:28  time:541s
fi-snb-2600      total:279  pass:250  dwarn:0   dfail:0   fail:0   skip:29  time:409s

e1b18fbd4daecb1c1cf31ca101f64e29a8933bcf drm-tip: 2017y-08m-17d-14h-58m-23s UTC integration manifest
289f21608f26 drm/i915: Stop touching forcewake following a gen6+ engine reset

== Logs ==

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

Re: [PATCH] drm/i915: Stop touching forcewake following a gen6+ engine reset

[Chris Wilson]

Quoting Michel Thierry (2017-08-17 18:41:16)
> On 17/08/17 10:32, Chris Wilson wrote:
> > Forcewake is not affected by the engine reset on gen6+. Indeed the
> > reason why we added intel_uncore_forcewake_reset() to
> > gen6_reset_engines() was to keep the bookkeeping intact because the
> > reset did not touch the forcewake bit (yet we cancelled the forcewake
> > consumers)!  This was done in commit 521198a2e7095:
> >      Author: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> >      Date:   Fri Aug 23 16:52:30 2013 +0300
> > 
> >       drm/i915: sanitize forcewake registers on reset
> > 
> >       In reset we try to restore the forcewake state to
> >       pre reset state, using forcewake_count. The reset
> >       doesn't seem to clear the forcewake bits so we
> >       get warn on forcewake ack register not clearing.
> > 
> > That futzing of the forcewake bookkeeping was dropped in commit
> > 0294ae7b44bb ("drm/i915: Consolidate forcewake resetting to a single
> > function"), but it did not make the realisation that the remaining
> > intel_uncore_forcewake_reset() was redundant.
> > 
> > The new danger with using intel_uncore_forcewake_reset() with per-engine
> > resets is that the driver and hw are still in an active state as we
> > perform the reset. We may be using the forcewake to read protected
> > registers elsewhere and those results may be clobbered by the concurrent
> > dropping of forcewake.
> > 
> > Reported-by: Michel Thierry <michel.thierry@intel.com>
> > Fixes: 142bc7d99bcf ("drm/i915: Modify error handler for per engine hang recovery")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> > Cc: Michel Thierry <michel.thierry@intel.com>

[snip]

> Thanks for digging out the commit history.

As you suspected, applying this commit makes BXT happy with the
per-engine reset scenarios that were failing before. One pass at
least, a few more (another 24hours or so) to be truly convinced :)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Stop touching forcewake following a gen6+ engine reset

[Daniel Vetter]

On Thu, Aug 17, 2017 at 06:32:29PM +0100, Chris Wilson wrote:
> Forcewake is not affected by the engine reset on gen6+. Indeed the
> reason why we added intel_uncore_forcewake_reset() to
> gen6_reset_engines() was to keep the bookkeeping intact because the
> reset did not touch the forcewake bit (yet we cancelled the forcewake
> consumers)!  This was done in commit 521198a2e7095:
>     Author: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>     Date:   Fri Aug 23 16:52:30 2013 +0300
> 
> 	drm/i915: sanitize forcewake registers on reset
> 
> 	In reset we try to restore the forcewake state to
> 	pre reset state, using forcewake_count. The reset
> 	doesn't seem to clear the forcewake bits so we
> 	get warn on forcewake ack register not clearing.
> 
> That futzing of the forcewake bookkeeping was dropped in commit
> 0294ae7b44bb ("drm/i915: Consolidate forcewake resetting to a single
> function"), but it did not make the realisation that the remaining
> intel_uncore_forcewake_reset() was redundant.
> 
> The new danger with using intel_uncore_forcewake_reset() with per-engine
> resets is that the driver and hw are still in an active state as we
> perform the reset. We may be using the forcewake to read protected
> registers elsewhere and those results may be clobbered by the concurrent
> dropping of forcewake.
> 
> Reported-by: Michel Thierry <michel.thierry@intel.com>
> Fixes: 142bc7d99bcf ("drm/i915: Modify error handler for per engine hang recovery")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Cc: Michel Thierry <michel.thierry@intel.com>

Could this explain some of the random unclaimed register failures we're
seeing on hsw shard CI?

https://bugs.freedesktop.org/show_bug.cgi?id=102249

Otoh this is in the display well, but maybe we're just racing and it's
just a stale bit in the unclaimed mmio debugging stuff. Worth a
References: I think.
-Daniel
> ---
>  drivers/gpu/drm/i915/intel_uncore.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index deb4430541cf..1d7b879cc68c 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -1497,7 +1497,6 @@ static int gen6_reset_engines(struct drm_i915_private *dev_priv,
>  		[VECS] = GEN6_GRDOM_VECS,
>  	};
>  	u32 hw_mask;
> -	int ret;
>  
>  	if (engine_mask == ALL_ENGINES) {
>  		hw_mask = GEN6_GRDOM_FULL;
> @@ -1509,11 +1508,7 @@ static int gen6_reset_engines(struct drm_i915_private *dev_priv,
>  			hw_mask |= hw_engine_mask[engine->id];
>  	}
>  
> -	ret = gen6_hw_domain_reset(dev_priv, hw_mask);
> -
> -	intel_uncore_forcewake_reset(dev_priv, true);
> -
> -	return ret;
> +	return gen6_hw_domain_reset(dev_priv, hw_mask);
>  }
>  
>  /**
> -- 
> 2.14.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Stop touching forcewake following a gen6+ engine reset

[Chris Wilson]

Quoting Daniel Vetter (2017-08-18 09:12:12)
> On Thu, Aug 17, 2017 at 06:32:29PM +0100, Chris Wilson wrote:
> > Forcewake is not affected by the engine reset on gen6+. Indeed the
> > reason why we added intel_uncore_forcewake_reset() to
> > gen6_reset_engines() was to keep the bookkeeping intact because the
> > reset did not touch the forcewake bit (yet we cancelled the forcewake
> > consumers)!  This was done in commit 521198a2e7095:
> >     Author: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> >     Date:   Fri Aug 23 16:52:30 2013 +0300
> > 
> >       drm/i915: sanitize forcewake registers on reset
> > 
> >       In reset we try to restore the forcewake state to
> >       pre reset state, using forcewake_count. The reset
> >       doesn't seem to clear the forcewake bits so we
> >       get warn on forcewake ack register not clearing.
> > 
> > That futzing of the forcewake bookkeeping was dropped in commit
> > 0294ae7b44bb ("drm/i915: Consolidate forcewake resetting to a single
> > function"), but it did not make the realisation that the remaining
> > intel_uncore_forcewake_reset() was redundant.
> > 
> > The new danger with using intel_uncore_forcewake_reset() with per-engine
> > resets is that the driver and hw are still in an active state as we
> > perform the reset. We may be using the forcewake to read protected
> > registers elsewhere and those results may be clobbered by the concurrent
> > dropping of forcewake.
> > 
> > Reported-by: Michel Thierry <michel.thierry@intel.com>
> > Fixes: 142bc7d99bcf ("drm/i915: Modify error handler for per engine hang recovery")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> > Cc: Michel Thierry <michel.thierry@intel.com>
> 
> Could this explain some of the random unclaimed register failures we're
> seeing on hsw shard CI?
> 
> https://bugs.freedesktop.org/show_bug.cgi?id=102249
> 
> Otoh this is in the display well, but maybe we're just racing and it's
> just a stale bit in the unclaimed mmio debugging stuff. Worth a
> References: I think.

No. Those registers are outside of the forcewake domain. I was hoping
this might explain the kbl reset failures, but the only trace there I
saw was the GPU reset request timing out i.e. gen6_hw_domain_reset()
which is before the fw reset.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Stop touching forcewake following a gen6+ engine reset

[Chris Wilson]

Quoting Daniel Vetter (2017-08-18 09:12:12)
> On Thu, Aug 17, 2017 at 06:32:29PM +0100, Chris Wilson wrote:
> > Forcewake is not affected by the engine reset on gen6+. Indeed the
> > reason why we added intel_uncore_forcewake_reset() to
> > gen6_reset_engines() was to keep the bookkeeping intact because the
> > reset did not touch the forcewake bit (yet we cancelled the forcewake
> > consumers)!  This was done in commit 521198a2e7095:
> >     Author: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> >     Date:   Fri Aug 23 16:52:30 2013 +0300
> > 
> >       drm/i915: sanitize forcewake registers on reset
> > 
> >       In reset we try to restore the forcewake state to
> >       pre reset state, using forcewake_count. The reset
> >       doesn't seem to clear the forcewake bits so we
> >       get warn on forcewake ack register not clearing.
> > 
> > That futzing of the forcewake bookkeeping was dropped in commit
> > 0294ae7b44bb ("drm/i915: Consolidate forcewake resetting to a single
> > function"), but it did not make the realisation that the remaining
> > intel_uncore_forcewake_reset() was redundant.
> > 
> > The new danger with using intel_uncore_forcewake_reset() with per-engine
> > resets is that the driver and hw are still in an active state as we
> > perform the reset. We may be using the forcewake to read protected
> > registers elsewhere and those results may be clobbered by the concurrent
> > dropping of forcewake.
> > 
> > Reported-by: Michel Thierry <michel.thierry@intel.com>
> > Fixes: 142bc7d99bcf ("drm/i915: Modify error handler for per engine hang recovery")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> > Cc: Michel Thierry <michel.thierry@intel.com>
> 
> Could this explain some of the random unclaimed register failures we're
> seeing on hsw shard CI?
> 
> https://bugs.freedesktop.org/show_bug.cgi?id=102249
> 
> Otoh this is in the display well, but maybe we're just racing and it's
> just a stale bit in the unclaimed mmio debugging stuff. Worth a
> References: I think.

Also that call trace is serialized with the
intel_uncore_forcewake_reset() and cannot be running concurrently.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Stop touching forcewake following a gen6+ engine reset

[Mika Kuoppala]

Michel Thierry <michel.thierry@intel.com> writes:

> On 17/08/17 10:32, Chris Wilson wrote:
>> Forcewake is not affected by the engine reset on gen6+. Indeed the
>> reason why we added intel_uncore_forcewake_reset() to
>> gen6_reset_engines() was to keep the bookkeeping intact because the
>> reset did not touch the forcewake bit (yet we cancelled the forcewake
>> consumers)!  This was done in commit 521198a2e7095:
>>      Author: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>>      Date:   Fri Aug 23 16:52:30 2013 +0300
>> 
>> 	drm/i915: sanitize forcewake registers on reset
>> 
>> 	In reset we try to restore the forcewake state to
>> 	pre reset state, using forcewake_count. The reset
>> 	doesn't seem to clear the forcewake bits so we
>> 	get warn on forcewake ack register not clearing.
>> 
>> That futzing of the forcewake bookkeeping was dropped in commit
>> 0294ae7b44bb ("drm/i915: Consolidate forcewake resetting to a single
>> function"), but it did not make the realisation that the remaining
>> intel_uncore_forcewake_reset() was redundant.
>> 
>> The new danger with using intel_uncore_forcewake_reset() with per-engine
>> resets is that the driver and hw are still in an active state as we
>> perform the reset. We may be using the forcewake to read protected
>> registers elsewhere and those results may be clobbered by the concurrent
>> dropping of forcewake.
>> 
>> Reported-by: Michel Thierry <michel.thierry@intel.com>
>> Fixes: 142bc7d99bcf ("drm/i915: Modify error handler for per engine hang recovery")
>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>> Cc: Michel Thierry <michel.thierry@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_uncore.c | 7 +------
>>   1 file changed, 1 insertion(+), 6 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
>> index deb4430541cf..1d7b879cc68c 100644
>> --- a/drivers/gpu/drm/i915/intel_uncore.c
>> +++ b/drivers/gpu/drm/i915/intel_uncore.c
>> @@ -1497,7 +1497,6 @@ static int gen6_reset_engines(struct drm_i915_private *dev_priv,
>>   		[VECS] = GEN6_GRDOM_VECS,
>>   	};
>>   	u32 hw_mask;
>> -	int ret;
>>   
>>   	if (engine_mask == ALL_ENGINES) {
>>   		hw_mask = GEN6_GRDOM_FULL;
>> @@ -1509,11 +1508,7 @@ static int gen6_reset_engines(struct drm_i915_private *dev_priv,
>>   			hw_mask |= hw_engine_mask[engine->id];
>>   	}
>>   
>> -	ret = gen6_hw_domain_reset(dev_priv, hw_mask);
>> -
>> -	intel_uncore_forcewake_reset(dev_priv, true);
>> -
>> -	return ret;
>> +	return gen6_hw_domain_reset(dev_priv, hw_mask);
>>   }
>>   
>>   /**
>> 
>
> Thanks for digging out the commit history.
>
> Reviewed-by Michel Thierry <michel.thierry@intel.com>

Nice find, possibly preventing quite amount of brain sprain.

Acked-by: Mika Kuoppala <mika.kuoppala@intel.com>

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

Re: [PATCH] drm/i915: Stop touching forcewake following a gen6+ engine reset

[Chris Wilson]

Quoting Mika Kuoppala (2017-08-18 09:56:23)
> Michel Thierry <michel.thierry@intel.com> writes:
> 
> > On 17/08/17 10:32, Chris Wilson wrote:
> >> Forcewake is not affected by the engine reset on gen6+. Indeed the
> >> reason why we added intel_uncore_forcewake_reset() to
> >> gen6_reset_engines() was to keep the bookkeeping intact because the
> >> reset did not touch the forcewake bit (yet we cancelled the forcewake
> >> consumers)!  This was done in commit 521198a2e7095:
> >>      Author: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> >>      Date:   Fri Aug 23 16:52:30 2013 +0300
> >> 
> >>      drm/i915: sanitize forcewake registers on reset
> >> 
> >>      In reset we try to restore the forcewake state to
> >>      pre reset state, using forcewake_count. The reset
> >>      doesn't seem to clear the forcewake bits so we
> >>      get warn on forcewake ack register not clearing.
> >> 
> >> That futzing of the forcewake bookkeeping was dropped in commit
> >> 0294ae7b44bb ("drm/i915: Consolidate forcewake resetting to a single
> >> function"), but it did not make the realisation that the remaining
> >> intel_uncore_forcewake_reset() was redundant.
> >> 
> >> The new danger with using intel_uncore_forcewake_reset() with per-engine
> >> resets is that the driver and hw are still in an active state as we
> >> perform the reset. We may be using the forcewake to read protected
> >> registers elsewhere and those results may be clobbered by the concurrent
> >> dropping of forcewake.
> >> 
> >> Reported-by: Michel Thierry <michel.thierry@intel.com>
> >> Fixes: 142bc7d99bcf ("drm/i915: Modify error handler for per engine hang recovery")
> >> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> >> Cc: Michel Thierry <michel.thierry@intel.com>
> >> ---
> >>   drivers/gpu/drm/i915/intel_uncore.c | 7 +------
> >>   1 file changed, 1 insertion(+), 6 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> >> index deb4430541cf..1d7b879cc68c 100644
> >> --- a/drivers/gpu/drm/i915/intel_uncore.c
> >> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> >> @@ -1497,7 +1497,6 @@ static int gen6_reset_engines(struct drm_i915_private *dev_priv,
> >>              [VECS] = GEN6_GRDOM_VECS,
> >>      };
> >>      u32 hw_mask;
> >> -    int ret;
> >>   
> >>      if (engine_mask == ALL_ENGINES) {
> >>              hw_mask = GEN6_GRDOM_FULL;
> >> @@ -1509,11 +1508,7 @@ static int gen6_reset_engines(struct drm_i915_private *dev_priv,
> >>                      hw_mask |= hw_engine_mask[engine->id];
> >>      }
> >>   
> >> -    ret = gen6_hw_domain_reset(dev_priv, hw_mask);
> >> -
> >> -    intel_uncore_forcewake_reset(dev_priv, true);
> >> -
> >> -    return ret;
> >> +    return gen6_hw_domain_reset(dev_priv, hw_mask);
> >>   }
> >>   
> >>   /**
> >> 
> >
> > Thanks for digging out the commit history.
> >
> > Reviewed-by Michel Thierry <michel.thierry@intel.com>
> 
> Nice find, possibly preventing quite amount of brain sprain.

Indeed it was.
 
> Acked-by: Mika Kuoppala <mika.kuoppala@intel.com>

I've pushed, but without the added references. I added the link to the
bugzilla just in case I am wrong.

Thanks for finding this bug Michel!
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx