[PATCH] drm/i915/skl: Use correct use counters for force wakes

[PATCH] drm/i915/skl: Use correct use counters for force wakes

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Write and reads following the block changed use engine specific use counters
and unless that is matched here force wake use counting goes bad. Same
force wake is attempted to be taken twice which leads to at least time outs.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 3048d78..4b33767 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -301,7 +301,7 @@ static void execlists_elsp_write(struct intel_engine_cs *ring,
 	 * Instead, we do the runtime_pm_get/put when creating/destroying requests.
 	 */
 	spin_lock_irqsave(&dev_priv->uncore.lock, flags);
-	if (IS_CHERRYVIEW(dev_priv->dev)) {
+	if (IS_CHERRYVIEW(dev_priv->dev) || IS_GEN9(dev_priv->dev)) {
 		if (dev_priv->uncore.fw_rendercount++ == 0)
 			dev_priv->uncore.funcs.force_wake_get(dev_priv,
 							      FORCEWAKE_RENDER);
@@ -326,7 +326,7 @@ static void execlists_elsp_write(struct intel_engine_cs *ring,
 
 	/* Release Force Wakeup (see the big comment above). */
 	spin_lock_irqsave(&dev_priv->uncore.lock, flags);
-	if (IS_CHERRYVIEW(dev_priv->dev)) {
+	if (IS_CHERRYVIEW(dev_priv->dev) || IS_GEN9(dev_priv->dev)) {
 		if (--dev_priv->uncore.fw_rendercount == 0)
 			dev_priv->uncore.funcs.force_wake_put(dev_priv,
 							      FORCEWAKE_RENDER);
-- 
2.1.0

Re: [PATCH] drm/i915/skl: Use correct use counters for force wakes

[Damien Lespiau]

On Thu, Sep 25, 2014 at 11:17:00AM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Write and reads following the block changed use engine specific use counters
> and unless that is matched here force wake use counting goes bad. Same
> force wake is attempted to be taken twice which leads to at least time outs.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Is it worth a v2 to have gen >= 9 here?

-- 
Damien

> ---
>  drivers/gpu/drm/i915/intel_lrc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 3048d78..4b33767 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -301,7 +301,7 @@ static void execlists_elsp_write(struct intel_engine_cs *ring,
>  	 * Instead, we do the runtime_pm_get/put when creating/destroying requests.
>  	 */
>  	spin_lock_irqsave(&dev_priv->uncore.lock, flags);
> -	if (IS_CHERRYVIEW(dev_priv->dev)) {
> +	if (IS_CHERRYVIEW(dev_priv->dev) || IS_GEN9(dev_priv->dev)) {
>  		if (dev_priv->uncore.fw_rendercount++ == 0)
>  			dev_priv->uncore.funcs.force_wake_get(dev_priv,
>  							      FORCEWAKE_RENDER);
> @@ -326,7 +326,7 @@ static void execlists_elsp_write(struct intel_engine_cs *ring,
>  
>  	/* Release Force Wakeup (see the big comment above). */
>  	spin_lock_irqsave(&dev_priv->uncore.lock, flags);
> -	if (IS_CHERRYVIEW(dev_priv->dev)) {
> +	if (IS_CHERRYVIEW(dev_priv->dev) || IS_GEN9(dev_priv->dev)) {
>  		if (--dev_priv->uncore.fw_rendercount == 0)
>  			dev_priv->uncore.funcs.force_wake_put(dev_priv,
>  							      FORCEWAKE_RENDER);
> -- 
> 2.1.0
> 

Re: [PATCH] drm/i915/skl: Use correct use counters for force wakes

[Mika Kuoppala]

Damien Lespiau <damien.lespiau@intel.com> writes:

> On Thu, Sep 25, 2014 at 11:17:00AM +0100, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> 
>> Write and reads following the block changed use engine specific use counters
>> and unless that is matched here force wake use counting goes bad. Same
>> force wake is attempted to be taken twice which leads to at least time outs.
>> 
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Is it worth a v2 to have gen >= 9 here?

I think we should have gen >= 8 here.

Shadowed ELSP's seems not to work on gen8. And the posting read will
need fw anyways.

Assuming the shadowing works on skl and we can get rid of the posting
read, we could run this part without taking forcewake.

-Mika

> -- 
> Damien
>
>> ---
>>  drivers/gpu/drm/i915/intel_lrc.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
>> index 3048d78..4b33767 100644
>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>> @@ -301,7 +301,7 @@ static void execlists_elsp_write(struct intel_engine_cs *ring,
>>  	 * Instead, we do the runtime_pm_get/put when creating/destroying requests.
>>  	 */
>>  	spin_lock_irqsave(&dev_priv->uncore.lock, flags);
>> -	if (IS_CHERRYVIEW(dev_priv->dev)) {
>> +	if (IS_CHERRYVIEW(dev_priv->dev) || IS_GEN9(dev_priv->dev)) {
>>  		if (dev_priv->uncore.fw_rendercount++ == 0)
>>  			dev_priv->uncore.funcs.force_wake_get(dev_priv,
>>  							      FORCEWAKE_RENDER);
>> @@ -326,7 +326,7 @@ static void execlists_elsp_write(struct intel_engine_cs *ring,
>>  
>>  	/* Release Force Wakeup (see the big comment above). */
>>  	spin_lock_irqsave(&dev_priv->uncore.lock, flags);
>> -	if (IS_CHERRYVIEW(dev_priv->dev)) {
>> +	if (IS_CHERRYVIEW(dev_priv->dev) || IS_GEN9(dev_priv->dev)) {
>>  		if (--dev_priv->uncore.fw_rendercount == 0)
>>  			dev_priv->uncore.funcs.force_wake_put(dev_priv,
>>  							      FORCEWAKE_RENDER);
>> -- 
>> 2.1.0
>> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/skl: Use correct use counters for force wakes

[Tvrtko Ursulin]

On 09/25/2014 01:05 PM, Mika Kuoppala wrote:
> Damien Lespiau <damien.lespiau@intel.com> writes:
>
>> On Thu, Sep 25, 2014 at 11:17:00AM +0100, Tvrtko Ursulin wrote:
>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>
>>> Write and reads following the block changed use engine specific use counters
>>> and unless that is matched here force wake use counting goes bad. Same
>>> force wake is attempted to be taken twice which leads to at least time outs.
>>>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Is it worth a v2 to have gen >= 9 here?
>
> I think we should have gen >= 8 here.

But that would not match against the current implementation of GEN8 vs 
CHV read/write functions.

> Shadowed ELSP's seems not to work on gen8. And the posting read will
> need fw anyways.
>
> Assuming the shadowing works on skl and we can get rid of the posting
> read, we could run this part without taking forcewake.

I don't know what criteria would need to be satisfied to get rid of the 
posting read. On GEN9 only you are saying?

2nd part, how to test if shadowing works? Just remove force wakes and 
see what happens?

Thanks,

Tvrtko

Re: [PATCH] drm/i915/skl: Use correct use counters for force wakes

[Damien Lespiau]

On Thu, Sep 25, 2014 at 01:43:31PM +0100, Tvrtko Ursulin wrote:
> 
> On 09/25/2014 01:05 PM, Mika Kuoppala wrote:
> >Damien Lespiau <damien.lespiau@intel.com> writes:
> >
> >>On Thu, Sep 25, 2014 at 11:17:00AM +0100, Tvrtko Ursulin wrote:
> >>>From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>>
> >>>Write and reads following the block changed use engine specific use counters
> >>>and unless that is matched here force wake use counting goes bad. Same
> >>>force wake is attempted to be taken twice which leads to at least time outs.
> >>>
> >>>Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>
> >>Is it worth a v2 to have gen >= 9 here?
> >
> >I think we should have gen >= 8 here.
> 
> But that would not match against the current implementation of GEN8
> vs CHV read/write functions.

The first problem here is that we have two sets of reference counts, one
for FORCEWAKE_ALL (uncore.forcewake_count) and one for each forcewake
engine on CHV/SKL. CHV has 2 fw engines, SKL 3.

So, in that regard, maybe the fw engine abstraction needs work to unify
the reference counts.

The least intrusive change to make SKL not error out would be to have a
separate code path for gen >= 9 with the 3 engines.

The thread leading to that patch is great and already has all the right
questions. We just ignored them.

The most immediate one is, why are we waking up all the fw engines when
writing a specific ring ELSP, but the whole thread is worth reading.

> >Shadowed ELSP's seems not to work on gen8. And the posting read will
> >need fw anyways.
> >
> >Assuming the shadowing works on skl and we can get rid of the posting
> >read, we could run this part without taking forcewake.
> 
> I don't know what criteria would need to be satisfied to get rid of
> the posting read. On GEN9 only you are saying?
> 
> 2nd part, how to test if shadowing works? Just remove force wakes
> and see what happens?

And that's a separate issue, can we use the shadow register mechanism to
ensure the write lands and is that enough? Let's try to ask people
internally about that.

HTH,

-- 
Damien

[PATCH] drm/i915/skl: Use correct use counters for force wakes

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Write and reads following the block changed use engine specific use counters
and unless that is matched here force wake use counting goes bad. Same
force wake is attempted to be taken twice which leads to at least time outs.

NOTE: Depending on feedback from hardware designers it may not be necessary
to grab force wakes on Gen9 here. But for Gen8 it is needed due to a race
between RC6 and ELSP writes.

v2: Added blitter force wake engine and made more future proof.
    Added commit note.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 3048d78..0792d7a 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -276,7 +276,8 @@ static void execlists_elsp_write(struct intel_engine_cs *ring,
 				 struct drm_i915_gem_object *ctx_obj0,
 				 struct drm_i915_gem_object *ctx_obj1)
 {
-	struct drm_i915_private *dev_priv = ring->dev->dev_private;
+	struct drm_device *dev = ring->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	uint64_t temp = 0;
 	uint32_t desc[4];
 	unsigned long flags;
@@ -301,13 +302,18 @@ static void execlists_elsp_write(struct intel_engine_cs *ring,
 	 * Instead, we do the runtime_pm_get/put when creating/destroying requests.
 	 */
 	spin_lock_irqsave(&dev_priv->uncore.lock, flags);
-	if (IS_CHERRYVIEW(dev_priv->dev)) {
+	if (IS_CHERRYVIEW(dev) || INTEL_INFO(dev)->gen >= 9) {
 		if (dev_priv->uncore.fw_rendercount++ == 0)
 			dev_priv->uncore.funcs.force_wake_get(dev_priv,
 							      FORCEWAKE_RENDER);
 		if (dev_priv->uncore.fw_mediacount++ == 0)
 			dev_priv->uncore.funcs.force_wake_get(dev_priv,
 							      FORCEWAKE_MEDIA);
+		if (INTEL_INFO(dev)->gen >= 9) {
+			if (dev_priv->uncore.fw_blittercount++ == 0)
+				dev_priv->uncore.funcs.force_wake_get(dev_priv,
+							FORCEWAKE_BLITTER);
+		}
 	} else {
 		if (dev_priv->uncore.forcewake_count++ == 0)
 			dev_priv->uncore.funcs.force_wake_get(dev_priv,
@@ -326,13 +332,18 @@ static void execlists_elsp_write(struct intel_engine_cs *ring,
 
 	/* Release Force Wakeup (see the big comment above). */
 	spin_lock_irqsave(&dev_priv->uncore.lock, flags);
-	if (IS_CHERRYVIEW(dev_priv->dev)) {
+	if (IS_CHERRYVIEW(dev) || INTEL_INFO(dev)->gen >= 9) {
 		if (--dev_priv->uncore.fw_rendercount == 0)
 			dev_priv->uncore.funcs.force_wake_put(dev_priv,
 							      FORCEWAKE_RENDER);
 		if (--dev_priv->uncore.fw_mediacount == 0)
 			dev_priv->uncore.funcs.force_wake_put(dev_priv,
 							      FORCEWAKE_MEDIA);
+		if (INTEL_INFO(dev)->gen >= 9) {
+			if (--dev_priv->uncore.fw_blittercount == 0)
+				dev_priv->uncore.funcs.force_wake_put(dev_priv,
+							FORCEWAKE_BLITTER);
+		}
 	} else {
 		if (--dev_priv->uncore.forcewake_count == 0)
 			dev_priv->uncore.funcs.force_wake_put(dev_priv,
-- 
2.1.0

Re: [PATCH] drm/i915/skl: Use correct use counters for force wakes

[Damien Lespiau]

On Tue, Sep 30, 2014 at 03:08:02PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Write and reads following the block changed use engine specific use counters
> and unless that is matched here force wake use counting goes bad. Same
> force wake is attempted to be taken twice which leads to at least time outs.
> 
> NOTE: Depending on feedback from hardware designers it may not be necessary
> to grab force wakes on Gen9 here. But for Gen8 it is needed due to a race
> between RC6 and ELSP writes.
> 
> v2: Added blitter force wake engine and made more future proof.
>     Added commit note.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Damien Lespiau <damien.lespiau@intel.com>
> ---

This still has the issue of taking every fw engine, not looking at the
ring we're queuing the work for. Also I'll add the note in a comment
above the whole block. It does solve at least an error in current
kernels so:

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

-- 
Damien

>  drivers/gpu/drm/i915/intel_lrc.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 3048d78..0792d7a 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -276,7 +276,8 @@ static void execlists_elsp_write(struct intel_engine_cs *ring,
>  				 struct drm_i915_gem_object *ctx_obj0,
>  				 struct drm_i915_gem_object *ctx_obj1)
>  {
> -	struct drm_i915_private *dev_priv = ring->dev->dev_private;
> +	struct drm_device *dev = ring->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
>  	uint64_t temp = 0;
>  	uint32_t desc[4];
>  	unsigned long flags;
> @@ -301,13 +302,18 @@ static void execlists_elsp_write(struct intel_engine_cs *ring,
>  	 * Instead, we do the runtime_pm_get/put when creating/destroying requests.
>  	 */
>  	spin_lock_irqsave(&dev_priv->uncore.lock, flags);
> -	if (IS_CHERRYVIEW(dev_priv->dev)) {
> +	if (IS_CHERRYVIEW(dev) || INTEL_INFO(dev)->gen >= 9) {
>  		if (dev_priv->uncore.fw_rendercount++ == 0)
>  			dev_priv->uncore.funcs.force_wake_get(dev_priv,
>  							      FORCEWAKE_RENDER);
>  		if (dev_priv->uncore.fw_mediacount++ == 0)
>  			dev_priv->uncore.funcs.force_wake_get(dev_priv,
>  							      FORCEWAKE_MEDIA);
> +		if (INTEL_INFO(dev)->gen >= 9) {
> +			if (dev_priv->uncore.fw_blittercount++ == 0)
> +				dev_priv->uncore.funcs.force_wake_get(dev_priv,
> +							FORCEWAKE_BLITTER);
> +		}
>  	} else {
>  		if (dev_priv->uncore.forcewake_count++ == 0)
>  			dev_priv->uncore.funcs.force_wake_get(dev_priv,
> @@ -326,13 +332,18 @@ static void execlists_elsp_write(struct intel_engine_cs *ring,
>  
>  	/* Release Force Wakeup (see the big comment above). */
>  	spin_lock_irqsave(&dev_priv->uncore.lock, flags);
> -	if (IS_CHERRYVIEW(dev_priv->dev)) {
> +	if (IS_CHERRYVIEW(dev) || INTEL_INFO(dev)->gen >= 9) {
>  		if (--dev_priv->uncore.fw_rendercount == 0)
>  			dev_priv->uncore.funcs.force_wake_put(dev_priv,
>  							      FORCEWAKE_RENDER);
>  		if (--dev_priv->uncore.fw_mediacount == 0)
>  			dev_priv->uncore.funcs.force_wake_put(dev_priv,
>  							      FORCEWAKE_MEDIA);
> +		if (INTEL_INFO(dev)->gen >= 9) {
> +			if (--dev_priv->uncore.fw_blittercount == 0)
> +				dev_priv->uncore.funcs.force_wake_put(dev_priv,
> +							FORCEWAKE_BLITTER);
> +		}
>  	} else {
>  		if (--dev_priv->uncore.forcewake_count == 0)
>  			dev_priv->uncore.funcs.force_wake_put(dev_priv,
> -- 
> 2.1.0
> 

Re: [PATCH] drm/i915/skl: Use correct use counters for force wakes

[Chris Wilson]

On Tue, Sep 30, 2014 at 03:08:02PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Write and reads following the block changed use engine specific use counters
> and unless that is matched here force wake use counting goes bad. Same
> force wake is attempted to be taken twice which leads to at least time outs.
> 
> NOTE: Depending on feedback from hardware designers it may not be necessary
> to grab force wakes on Gen9 here. But for Gen8 it is needed due to a race
> between RC6 and ELSP writes.
> 
> v2: Added blitter force wake engine and made more future proof.
>     Added commit note.

Speaking of futureproofing, what did you think of my patch to remove the
duplicated counting logic?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915/skl: Use correct use counters for force wakes

[Tvrtko Ursulin]

On 09/30/2014 03:57 PM, Chris Wilson wrote:
> On Tue, Sep 30, 2014 at 03:08:02PM +0100, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Write and reads following the block changed use engine specific use counters
>> and unless that is matched here force wake use counting goes bad. Same
>> force wake is attempted to be taken twice which leads to at least time outs.
>>
>> NOTE: Depending on feedback from hardware designers it may not be necessary
>> to grab force wakes on Gen9 here. But for Gen8 it is needed due to a race
>> between RC6 and ELSP writes.
>>
>> v2: Added blitter force wake engine and made more future proof.
>>      Added commit note.
>
> Speaking of futureproofing, what did you think of my patch to remove the
> duplicated counting logic?

[For reference it is called "[PATCH] drm/i915: Reduce duplicated 
forcewake logic".]

Disclaimer: I don't know this code that well - only had to dig into it a 
few days back when I hit this bug which resulted in my patch.

But from a glance your patch does make it look cleaner and indeed more 
future proof (much better separation between platform specific and 
generic). Does it need a proper review to move it forward?

Regards,

Tvrtko

Re: [PATCH] drm/i915/skl: Use correct use counters for force wakes

[Daniel Vetter]

On Tue, Sep 30, 2014 at 04:58:54PM +0100, Tvrtko Ursulin wrote:
> 
> 
> On 09/30/2014 03:57 PM, Chris Wilson wrote:
> >On Tue, Sep 30, 2014 at 03:08:02PM +0100, Tvrtko Ursulin wrote:
> >>From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>
> >>Write and reads following the block changed use engine specific use counters
> >>and unless that is matched here force wake use counting goes bad. Same
> >>force wake is attempted to be taken twice which leads to at least time outs.
> >>
> >>NOTE: Depending on feedback from hardware designers it may not be necessary
> >>to grab force wakes on Gen9 here. But for Gen8 it is needed due to a race
> >>between RC6 and ELSP writes.
> >>
> >>v2: Added blitter force wake engine and made more future proof.
> >>     Added commit note.
> >
> >Speaking of futureproofing, what did you think of my patch to remove the
> >duplicated counting logic?
> 
> [For reference it is called "[PATCH] drm/i915: Reduce duplicated forcewake
> logic".]
> 
> Disclaimer: I don't know this code that well - only had to dig into it a few
> days back when I hit this bug which resulted in my patch.
> 
> But from a glance your patch does make it look cleaner and indeed more
> future proof (much better separation between platform specific and generic).
> Does it need a proper review to move it forward?

Yes. Volunteered?

And when you review pls reply with a blurb in case the existing commit
message from Chris doesn't fully cover the bug you've stumbled over, so
that lazy me can just copy-paste that while applying ;-)

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch