[PATCH] drm/i915: Wait old forcewake ack to clear on vlv

[PATCH] drm/i915: Wait old forcewake ack to clear on vlv

[Mika Kuoppala]

Don't rush into getting new fw until the clearing
of old one has been acked.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=85684
Tested-by: lu hua <huax.lu@intel.com>
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/intel_uncore.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 9427641..5259b38 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -204,6 +204,10 @@ static void __vlv_force_wake_get(struct drm_i915_private *dev_priv,
 	/* Check for Render Engine */
 	if (FORCEWAKE_RENDER & fw_engine) {
 
+		if (wait_for_atomic((__raw_i915_read32(dev_priv, FORCEWAKE_ACK_VLV) &
+				     FORCEWAKE_KERNEL) == 0, FORCEWAKE_ACK_TIMEOUT_MS))
+			DRM_ERROR("Timed out: waiting for old Render ack to clear.\n");
+
 		__raw_i915_write32(dev_priv, FORCEWAKE_VLV,
 				   _MASKED_BIT_ENABLE(FORCEWAKE_KERNEL));
 
@@ -217,6 +221,11 @@ static void __vlv_force_wake_get(struct drm_i915_private *dev_priv,
 	/* Check for Media Engine */
 	if (FORCEWAKE_MEDIA & fw_engine) {
 
+		if (wait_for_atomic((__raw_i915_read32(dev_priv, FORCEWAKE_ACK_MEDIA_VLV) &
+				     FORCEWAKE_KERNEL) == 0, FORCEWAKE_ACK_TIMEOUT_MS))
+			DRM_ERROR("Timed out: waiting for old media ack to clear.\n");
+
+
 		__raw_i915_write32(dev_priv, FORCEWAKE_MEDIA_VLV,
 				   _MASKED_BIT_ENABLE(FORCEWAKE_KERNEL));
 
-- 
1.9.1

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

Re: [PATCH] drm/i915: Wait old forcewake ack to clear on vlv

[Chris Wilson]

On Wed, Nov 05, 2014 at 10:18:46AM +0200, Mika Kuoppala wrote:
> Don't rush into getting new fw until the clearing
> of old one has been acked.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=85684
> Tested-by: lu hua <huax.lu@intel.com>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>

vlv is backwards wrt to our design for the other snb+ gen. No surprises
there. It currently waits-for-ack after requesting the forcewake, so you
need to explain the bug more clearly or remove the surplus wait.
-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: Wait old forcewake ack to clear on vlv

[Mika Kuoppala]

with

commit 5cb13c07dae73380d8b3ddc792740487b8742938
Author: Deepak S <deepak.s@linux.intel.com>
Date:   Thu Sep 18 18:51:50 2014 +0530

    drm/i915/vlv: Remove check for Old Ack during forcewake

we didn't wait anymore for old ack bit clearing in basis
of workaround WaRsDontPollForAckOnClearingFWBits.
That WA says that driver sw should not wait for ack on clearing
fw, as the clearing is done lazily. Only when all the the fws
are cleared, the acks will clear. When in multibit fw use case.

But as we only use one bit per engine, and the assumption in here
is that engines are independant, our vlv/chv forcewake use case
does not correlate with WaRsDontPollForAckOnClearingFWBits.

So revert into waiting the ack to clear, because there is
only one bit to wait.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=85684
Tested-by: lu hua <huax.lu@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: S, Deepak <deepak.s@intel.com>
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/intel_uncore.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 9427641..e315bce 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -195,15 +195,22 @@ static void __vlv_force_wake_get(struct drm_i915_private *dev_priv,
 						int fw_engine)
 {
 	/*
-	 * WaRsDontPollForAckOnClearingFWBits:vlv
-	 * Hardware clears ack bits lazily (only when all ack
-	 * bits become 0) so don't poll for individiual ack
-	 * bits to be clear here like on other platforms.
+	 * WaRsDontPollForAckOnClearingFWBits is not required
+	 * here as we only do one bit per engine so, the lazy
+	 * clearing of bits is not of our concern.
+	 *
+	 * XXX Even if we access multi-threaded forcewake registers, we 
+	 * use only one bit in it (another bit for userspace). Multibit access
+	 * is broken on multiple gens (IVB, BDW). See WaRSForceSingleThreadFW.
 	 */
 
 	/* Check for Render Engine */
 	if (FORCEWAKE_RENDER & fw_engine) {
 
+		if (wait_for_atomic((__raw_i915_read32(dev_priv, FORCEWAKE_ACK_VLV) &
+				     FORCEWAKE_KERNEL) == 0, FORCEWAKE_ACK_TIMEOUT_MS))
+			DRM_ERROR("Timed out: waiting for old Render ack to clear.\n");
+
 		__raw_i915_write32(dev_priv, FORCEWAKE_VLV,
 				   _MASKED_BIT_ENABLE(FORCEWAKE_KERNEL));
 
@@ -217,6 +224,11 @@ static void __vlv_force_wake_get(struct drm_i915_private *dev_priv,
 	/* Check for Media Engine */
 	if (FORCEWAKE_MEDIA & fw_engine) {
 
+		if (wait_for_atomic((__raw_i915_read32(dev_priv, FORCEWAKE_ACK_MEDIA_VLV) &
+				     FORCEWAKE_KERNEL) == 0, FORCEWAKE_ACK_TIMEOUT_MS))
+			DRM_ERROR("Timed out: waiting for old media ack to clear.\n");
+
+
 		__raw_i915_write32(dev_priv, FORCEWAKE_MEDIA_VLV,
 				   _MASKED_BIT_ENABLE(FORCEWAKE_KERNEL));
 
-- 
1.9.1

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

Re: [PATCH] drm/i915: Wait old forcewake ack to clear on vlv

[Ville Syrjälä]

On Wed, Nov 05, 2014 at 10:18:46AM +0200, Mika Kuoppala wrote:
> Don't rush into getting new fw until the clearing
> of old one has been acked.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=85684
> Tested-by: lu hua <huax.lu@intel.com>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>

This is just a revert of 
 commit 5cb13c07dae73380d8b3ddc792740487b8742938
 Author: Deepak S <deepak.s@linux.intel.com>
 Date:   Thu Sep 18 18:51:50 2014 +0530

    drm/i915/vlv: Remove check for Old Ack during forcewake

except you left the comment behind.

Would be nice to have some kind of real explanation what is going on
here. Deepak, any ideas?

> ---
>  drivers/gpu/drm/i915/intel_uncore.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 9427641..5259b38 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -204,6 +204,10 @@ static void __vlv_force_wake_get(struct drm_i915_private *dev_priv,
>  	/* Check for Render Engine */
>  	if (FORCEWAKE_RENDER & fw_engine) {
>  
> +		if (wait_for_atomic((__raw_i915_read32(dev_priv, FORCEWAKE_ACK_VLV) &
> +				     FORCEWAKE_KERNEL) == 0, FORCEWAKE_ACK_TIMEOUT_MS))
> +			DRM_ERROR("Timed out: waiting for old Render ack to clear.\n");
> +
>  		__raw_i915_write32(dev_priv, FORCEWAKE_VLV,
>  				   _MASKED_BIT_ENABLE(FORCEWAKE_KERNEL));
>  
> @@ -217,6 +221,11 @@ static void __vlv_force_wake_get(struct drm_i915_private *dev_priv,
>  	/* Check for Media Engine */
>  	if (FORCEWAKE_MEDIA & fw_engine) {
>  
> +		if (wait_for_atomic((__raw_i915_read32(dev_priv, FORCEWAKE_ACK_MEDIA_VLV) &
> +				     FORCEWAKE_KERNEL) == 0, FORCEWAKE_ACK_TIMEOUT_MS))
> +			DRM_ERROR("Timed out: waiting for old media ack to clear.\n");
> +
> +
>  		__raw_i915_write32(dev_priv, FORCEWAKE_MEDIA_VLV,
>  				   _MASKED_BIT_ENABLE(FORCEWAKE_KERNEL));
>  
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Wait old forcewake ack to clear on vlv

[S, Deepak]

On 11/5/2014 2:23 PM, Ville Syrjälä wrote:
> On Wed, Nov 05, 2014 at 10:18:46AM +0200, Mika Kuoppala wrote:
>> Don't rush into getting new fw until the clearing
>> of old one has been acked.
>>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=85684
>> Tested-by: lu hua <huax.lu@intel.com>
>> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> This is just a revert of
>   commit 5cb13c07dae73380d8b3ddc792740487b8742938
>   Author: Deepak S <deepak.s@linux.intel.com>
>   Date:   Thu Sep 18 18:51:50 2014 +0530
>
>      drm/i915/vlv: Remove check for Old Ack during forcewake
>
> except you left the comment behind.
>
> Would be nice to have some kind of real explanation what is going on
> here. Deepak, any ideas?
Hi Ville,

This is strange :(.  Expectation from HW team is not to wait for old 
ack. Let me go thought my mail and I will send you the explanation i got 
from HW team.
Sorry, I was working some other task and i will check this issue on my 
system & update the thread.

Thanks
Deepak

>> ---
>>   drivers/gpu/drm/i915/intel_uncore.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
>> index 9427641..5259b38 100644
>> --- a/drivers/gpu/drm/i915/intel_uncore.c
>> +++ b/drivers/gpu/drm/i915/intel_uncore.c
>> @@ -204,6 +204,10 @@ static void __vlv_force_wake_get(struct drm_i915_private *dev_priv,
>>   	/* Check for Render Engine */
>>   	if (FORCEWAKE_RENDER & fw_engine) {
>>   
>> +		if (wait_for_atomic((__raw_i915_read32(dev_priv, FORCEWAKE_ACK_VLV) &
>> +				     FORCEWAKE_KERNEL) == 0, FORCEWAKE_ACK_TIMEOUT_MS))
>> +			DRM_ERROR("Timed out: waiting for old Render ack to clear.\n");
>> +
>>   		__raw_i915_write32(dev_priv, FORCEWAKE_VLV,
>>   				   _MASKED_BIT_ENABLE(FORCEWAKE_KERNEL));
>>   
>> @@ -217,6 +221,11 @@ static void __vlv_force_wake_get(struct drm_i915_private *dev_priv,
>>   	/* Check for Media Engine */
>>   	if (FORCEWAKE_MEDIA & fw_engine) {
>>   
>> +		if (wait_for_atomic((__raw_i915_read32(dev_priv, FORCEWAKE_ACK_MEDIA_VLV) &
>> +				     FORCEWAKE_KERNEL) == 0, FORCEWAKE_ACK_TIMEOUT_MS))
>> +			DRM_ERROR("Timed out: waiting for old media ack to clear.\n");
>> +
>> +
>>   		__raw_i915_write32(dev_priv, FORCEWAKE_MEDIA_VLV,
>>   				   _MASKED_BIT_ENABLE(FORCEWAKE_KERNEL));
>>   
>> -- 
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH] drm/i915: Wait old forcewake ack to clear on vlv

[Mika Kuoppala]

Ville Syrjälä <ville.syrjala@linux.intel.com> writes:

> On Wed, Nov 05, 2014 at 10:18:46AM +0200, Mika Kuoppala wrote:
>> Don't rush into getting new fw until the clearing
>> of old one has been acked.
>> 
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=85684
>> Tested-by: lu hua <huax.lu@intel.com>
>> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
>
> This is just a revert of 
>  commit 5cb13c07dae73380d8b3ddc792740487b8742938
>  Author: Deepak S <deepak.s@linux.intel.com>
>  Date:   Thu Sep 18 18:51:50 2014 +0530
>
>     drm/i915/vlv: Remove check for Old Ack during forcewake
>
> except you left the comment behind.

I failed at archeology. There is even nice comment from Deepak on
top I managed to miss.

But after reading this comment and the workaround description, I
think that this WaRsDontPollForAckOnClearingFWBits is not valid for
our use case.

We use only one bit per engine, so our use case is not multithreaded
in that sense.

-Mika

> Would be nice to have some kind of real explanation what is going on
> here. Deepak, any ideas?
>
>> ---
>>  drivers/gpu/drm/i915/intel_uncore.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
>> index 9427641..5259b38 100644
>> --- a/drivers/gpu/drm/i915/intel_uncore.c
>> +++ b/drivers/gpu/drm/i915/intel_uncore.c
>> @@ -204,6 +204,10 @@ static void __vlv_force_wake_get(struct drm_i915_private *dev_priv,
>>  	/* Check for Render Engine */
>>  	if (FORCEWAKE_RENDER & fw_engine) {
>>  
>> +		if (wait_for_atomic((__raw_i915_read32(dev_priv, FORCEWAKE_ACK_VLV) &
>> +				     FORCEWAKE_KERNEL) == 0, FORCEWAKE_ACK_TIMEOUT_MS))
>> +			DRM_ERROR("Timed out: waiting for old Render ack to clear.\n");
>> +
>>  		__raw_i915_write32(dev_priv, FORCEWAKE_VLV,
>>  				   _MASKED_BIT_ENABLE(FORCEWAKE_KERNEL));
>>  
>> @@ -217,6 +221,11 @@ static void __vlv_force_wake_get(struct drm_i915_private *dev_priv,
>>  	/* Check for Media Engine */
>>  	if (FORCEWAKE_MEDIA & fw_engine) {
>>  
>> +		if (wait_for_atomic((__raw_i915_read32(dev_priv, FORCEWAKE_ACK_MEDIA_VLV) &
>> +				     FORCEWAKE_KERNEL) == 0, FORCEWAKE_ACK_TIMEOUT_MS))
>> +			DRM_ERROR("Timed out: waiting for old media ack to clear.\n");
>> +
>> +
>>  		__raw_i915_write32(dev_priv, FORCEWAKE_MEDIA_VLV,
>>  				   _MASKED_BIT_ENABLE(FORCEWAKE_KERNEL));
>>  
>> -- 
>> 1.9.1
>> 
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> -- 
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Wait old forcewake ack to clear on vlv

[Daniel Vetter]

On Wed, Nov 05, 2014 at 12:13:40PM +0200, Mika Kuoppala wrote:
> Ville Syrjälä <ville.syrjala@linux.intel.com> writes:
> 
> > On Wed, Nov 05, 2014 at 10:18:46AM +0200, Mika Kuoppala wrote:
> >> Don't rush into getting new fw until the clearing
> >> of old one has been acked.
> >> 
> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=85684
> >> Tested-by: lu hua <huax.lu@intel.com>
> >> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> >
> > This is just a revert of 
> >  commit 5cb13c07dae73380d8b3ddc792740487b8742938
> >  Author: Deepak S <deepak.s@linux.intel.com>
> >  Date:   Thu Sep 18 18:51:50 2014 +0530
> >
> >     drm/i915/vlv: Remove check for Old Ack during forcewake
> >
> > except you left the comment behind.
> 
> I failed at archeology. There is even nice comment from Deepak on
> top I managed to miss.
> 
> But after reading this comment and the workaround description, I
> think that this WaRsDontPollForAckOnClearingFWBits is not valid for
> our use case.
> 
> We use only one bit per engine, so our use case is not multithreaded
> in that sense.

Since this is a regression I prefer we apply the revert and then dig
deeper. Mika, can you pls resend as a proper revert? Except ofc when we
have a solution right now, too.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] Revert "drm/i915/vlv: Remove check for Old Ack during forcewake"

[Mika Kuoppala]

This reverts commit 5cb13c07dae73380d8b3ddc792740487b8742938.

While the relevance for WaRsDontPollForAckOnClearingFWBits is under
investigation, revert this as regression.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=85684
Tested-by:   Tested-by: lu hua <huax.lu@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: S, Deepak <deepak.s@intel.com>
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/intel_uncore.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 68e722b..6a0c3fb 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -194,15 +194,13 @@ static void vlv_force_wake_reset(struct drm_i915_private *dev_priv)
 static void __vlv_force_wake_get(struct drm_i915_private *dev_priv,
 						int fw_engine)
 {
-	/*
-	 * WaRsDontPollForAckOnClearingFWBits:vlv
-	 * Hardware clears ack bits lazily (only when all ack
-	 * bits become 0) so don't poll for individiual ack
-	 * bits to be clear here like on other platforms.
-	 */
-
 	/* Check for Render Engine */
 	if (FORCEWAKE_RENDER & fw_engine) {
+		if (wait_for_atomic((__raw_i915_read32(dev_priv,
+						FORCEWAKE_ACK_VLV) &
+						FORCEWAKE_KERNEL) == 0,
+					FORCEWAKE_ACK_TIMEOUT_MS))
+			DRM_ERROR("Timed out: Render forcewake old ack to clear.\n");
 
 		__raw_i915_write32(dev_priv, FORCEWAKE_VLV,
 				   _MASKED_BIT_ENABLE(FORCEWAKE_KERNEL));
@@ -216,6 +214,11 @@ static void __vlv_force_wake_get(struct drm_i915_private *dev_priv,
 
 	/* Check for Media Engine */
 	if (FORCEWAKE_MEDIA & fw_engine) {
+		if (wait_for_atomic((__raw_i915_read32(dev_priv,
+						FORCEWAKE_ACK_MEDIA_VLV) &
+						FORCEWAKE_KERNEL) == 0,
+					FORCEWAKE_ACK_TIMEOUT_MS))
+			DRM_ERROR("Timed out: Media forcewake old ack to clear.\n");
 
 		__raw_i915_write32(dev_priv, FORCEWAKE_MEDIA_VLV,
 				   _MASKED_BIT_ENABLE(FORCEWAKE_KERNEL));
-- 
1.9.1

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

Re: [PATCH] Revert "drm/i915/vlv: Remove check for Old Ack during forcewake"

[Daniel Vetter]

On Wed, Nov 05, 2014 at 05:30:52PM +0200, Mika Kuoppala wrote:
> This reverts commit 5cb13c07dae73380d8b3ddc792740487b8742938.
> 
> While the relevance for WaRsDontPollForAckOnClearingFWBits is under
> investigation, revert this as regression.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=85684
> Tested-by:   Tested-by: lu hua <huax.lu@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: S, Deepak <deepak.s@intel.com>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>

Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] Revert "drm/i915/vlv: Remove check for Old Ack during forcewake"

[Dave Gordon]

On 06/11/14 14:09, Daniel Vetter wrote:
> On Wed, Nov 05, 2014 at 05:30:52PM +0200, Mika Kuoppala wrote:
>> This reverts commit 5cb13c07dae73380d8b3ddc792740487b8742938.
>>
>> While the relevance for WaRsDontPollForAckOnClearingFWBits is under
>> investigation, revert this as regression.
>>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=85684
>> Tested-by:   Tested-by: lu hua <huax.lu@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: S, Deepak <deepak.s@intel.com>
>> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> 
> Queued for -next, thanks for the patch.
> -Daniel
> 

Hi,

while looking at the (semi-generic) force wake code in the function
gen6_gt_force_wake_get(), I noticed that if the platform doesn't provide
a low-level forcewake register-access function, then we omit the PM
get/put calls as well.

> void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv, int fw_engine)
> {
>         unsigned long irqflags;
> 
>         if (!dev_priv->uncore.funcs.force_wake_get)
>                 return;
> 
>         intel_runtime_pm_get(dev_priv);
> 
>         /* Redirect to Gen9 specific routine */
>         if (IS_GEN9(dev_priv->dev))
>                 return gen9_force_wake_get(dev_priv, fw_engine);
> 
>         /* Redirect to VLV specific routine */
>         if (IS_VALLEYVIEW(dev_priv->dev))
>                 return vlv_force_wake_get(dev_priv, fw_engine);
> 
>         spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
>         if (dev_priv->uncore.forcewake_count++ == 0)
>                 dev_priv->uncore.funcs.force_wake_get(dev_priv, FORCEWAKE_ALL);
>         spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> }

Does anyone know whether this is intentional, or just an accident of the
current implementation? Presumably we don't have any platforms where
this applies, but we is it plausible that some future device might want
the runtime PM effects without actually having h/w support
for forcewake?

Alternatively, why do we need the runtime PM calls in here? I can see
Paolo's commit "c8c8fb3 drm/i915: add some runtime PM get/put calls"
added lots of get/put pairs round various sequences of operations in
debugfs etc, so why do we also need them at this inner level? Is it just
to catch any cases where the caller should have (but didn't) deal with
runtime PM already?

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

Re: [PATCH] Revert "drm/i915/vlv: Remove check for Old Ack during forcewake"

[Daniel Vetter]

On Fri, Nov 07, 2014 at 02:58:37PM +0000, Dave Gordon wrote:
> On 06/11/14 14:09, Daniel Vetter wrote:
> > On Wed, Nov 05, 2014 at 05:30:52PM +0200, Mika Kuoppala wrote:
> >> This reverts commit 5cb13c07dae73380d8b3ddc792740487b8742938.
> >>
> >> While the relevance for WaRsDontPollForAckOnClearingFWBits is under
> >> investigation, revert this as regression.
> >>
> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=85684
> >> Tested-by:   Tested-by: lu hua <huax.lu@intel.com>
> >> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> Cc: S, Deepak <deepak.s@intel.com>
> >> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> > 
> > Queued for -next, thanks for the patch.
> > -Daniel
> > 
> 
> Hi,
> 
> while looking at the (semi-generic) force wake code in the function
> gen6_gt_force_wake_get(), I noticed that if the platform doesn't provide
> a low-level forcewake register-access function, then we omit the PM
> get/put calls as well.
> 
> > void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv, int fw_engine)
> > {
> >         unsigned long irqflags;
> > 
> >         if (!dev_priv->uncore.funcs.force_wake_get)
> >                 return;
> > 
> >         intel_runtime_pm_get(dev_priv);
> > 
> >         /* Redirect to Gen9 specific routine */
> >         if (IS_GEN9(dev_priv->dev))
> >                 return gen9_force_wake_get(dev_priv, fw_engine);
> > 
> >         /* Redirect to VLV specific routine */
> >         if (IS_VALLEYVIEW(dev_priv->dev))
> >                 return vlv_force_wake_get(dev_priv, fw_engine);
> > 
> >         spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> >         if (dev_priv->uncore.forcewake_count++ == 0)
> >                 dev_priv->uncore.funcs.force_wake_get(dev_priv, FORCEWAKE_ALL);
> >         spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> > }
> 
> Does anyone know whether this is intentional, or just an accident of the
> current implementation? Presumably we don't have any platforms where
> this applies, but we is it plausible that some future device might want
> the runtime PM effects without actually having h/w support
> for forcewake?
> 
> Alternatively, why do we need the runtime PM calls in here? I can see
> Paolo's commit "c8c8fb3 drm/i915: add some runtime PM get/put calls"
> added lots of get/put pairs round various sequences of operations in
> debugfs etc, so why do we also need them at this inner level? Is it just
> to catch any cases where the caller should have (but didn't) deal with
> runtime PM already?

The idea is that power domains nest, and the inner power domains grab the
reference for the outer domains for you.

All the places paulo sprinkled runtime pm over don't need anything else
than the device out of D3, or at least that was the idea. So if you want
to wake the gt we'll automatically get the device out of D3. And on the
display side where power domains can nest even more it'll wake up the
entire chain. Without this the entire abstraction of mapping generic power
domains onto what the hw actually looks like falls apart. And without that
our driver would be a pretty crazy mess ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Wait old forcewake ack to clear on vlv

[S, Deepak]

On 11/5/2014 3:43 PM, Mika Kuoppala wrote:
> Ville Syrjälä <ville.syrjala@linux.intel.com> writes:
>
>> On Wed, Nov 05, 2014 at 10:18:46AM +0200, Mika Kuoppala wrote:
>>> Don't rush into getting new fw until the clearing
>>> of old one has been acked.
>>>
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=85684
>>> Tested-by: lu hua <huax.lu@intel.com>
>>> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
>> This is just a revert of
>>   commit 5cb13c07dae73380d8b3ddc792740487b8742938
>>   Author: Deepak S <deepak.s@linux.intel.com>
>>   Date:   Thu Sep 18 18:51:50 2014 +0530
>>
>>      drm/i915/vlv: Remove check for Old Ack during forcewake
>>
>> except you left the comment behind.
> I failed at archeology. There is even nice comment from Deepak on
> top I managed to miss.
>
> But after reading this comment and the workaround description, I
> think that this WaRsDontPollForAckOnClearingFWBits is not valid for
> our use case.
>
> We use only one bit per engine, so our use case is not multithreaded
> in that sense.
>
> -Mika

I agree with you, in kernel we use only one bit for FW, but based on my 
understanding of forcewake implementation back to back forcewake should 
not cause any issue.
Don't you think adding old ack might be hiding some other issue? I think 
we need to root cause further.

As of now we can revert the patch & I will start root cause on my 
machine on why the issue is happening.

>> Would be nice to have some kind of real explanation what is going on
>> here. Deepak, any ideas?
>>
>>> ---
>>>   drivers/gpu/drm/i915/intel_uncore.c | 9 +++++++++
>>>   1 file changed, 9 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
>>> index 9427641..5259b38 100644
>>> --- a/drivers/gpu/drm/i915/intel_uncore.c
>>> +++ b/drivers/gpu/drm/i915/intel_uncore.c
>>> @@ -204,6 +204,10 @@ static void __vlv_force_wake_get(struct drm_i915_private *dev_priv,
>>>   	/* Check for Render Engine */
>>>   	if (FORCEWAKE_RENDER & fw_engine) {
>>>   
>>> +		if (wait_for_atomic((__raw_i915_read32(dev_priv, FORCEWAKE_ACK_VLV) &
>>> +				     FORCEWAKE_KERNEL) == 0, FORCEWAKE_ACK_TIMEOUT_MS))
>>> +			DRM_ERROR("Timed out: waiting for old Render ack to clear.\n");
>>> +
>>>   		__raw_i915_write32(dev_priv, FORCEWAKE_VLV,
>>>   				   _MASKED_BIT_ENABLE(FORCEWAKE_KERNEL));
>>>   
>>> @@ -217,6 +221,11 @@ static void __vlv_force_wake_get(struct drm_i915_private *dev_priv,
>>>   	/* Check for Media Engine */
>>>   	if (FORCEWAKE_MEDIA & fw_engine) {
>>>   
>>> +		if (wait_for_atomic((__raw_i915_read32(dev_priv, FORCEWAKE_ACK_MEDIA_VLV) &
>>> +				     FORCEWAKE_KERNEL) == 0, FORCEWAKE_ACK_TIMEOUT_MS))
>>> +			DRM_ERROR("Timed out: waiting for old media ack to clear.\n");
>>> +
>>> +
>>>   		__raw_i915_write32(dev_priv, FORCEWAKE_MEDIA_VLV,
>>>   				   _MASKED_BIT_ENABLE(FORCEWAKE_KERNEL));
>>>   
>>> -- 
>>> 1.9.1
>>>
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> -- 
>> Ville Syrjälä
>> Intel OTC

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