diff mbox

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

Message ID 1415175526-24082-1-git-send-email-mika.kuoppala@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Mika Kuoppala Nov. 5, 2014, 8:18 a.m. UTC 
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(+)

Comments

Chris Wilson Nov. 5, 2014, 8:25 a.m. UTC | #1 
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
Ville Syrjälä Nov. 5, 2014, 8:53 a.m. UTC | #2 
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
deepak.s@intel.com Nov. 5, 2014, 9:26 a.m. UTC | #3 
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
Mika Kuoppala Nov. 5, 2014, 10:13 a.m. UTC | #4 
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
Daniel Vetter Nov. 5, 2014, 12:42 p.m. UTC | #5 
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
deepak.s@intel.com Nov. 5, 2014, 12:47 p.m. UTC | #6 
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
diff mbox

Patch

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