[PATCH] drm/i915: Rename __force_wake_get to __force_wake_auto

[PATCH] drm/i915: Rename __force_wake_get to __force_wake_auto

[Chris Wilson]

__force_wake_get() only acquire a temporary wakeref on forcewake that is
automatically releases when a timer expires. When reading the code
again, I confused __intel_uncore_forcewake_get for __force_wake_get and
to my shame thought I found a bug in an unbalanced wake_count handling.

I claim that if the function had been called __force_wake_auto instead I
would not have embarrassed myself.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_uncore.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 96799392c2c7..165ebce5b638 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -716,8 +716,8 @@ __gen2_read(64)
 	trace_i915_reg_rw(false, reg, val, sizeof(val), trace); \
 	return val
 
-static inline void __force_wake_get(struct drm_i915_private *dev_priv,
-				    enum forcewake_domains fw_domains)
+static inline void __force_wake_auto(struct drm_i915_private *dev_priv,
+				     enum forcewake_domains fw_domains)
 {
 	struct intel_uncore_forcewake_domain *domain;
 	enum forcewake_domain_id id;
@@ -745,7 +745,7 @@ static u##x \
 gen6_read##x(struct drm_i915_private *dev_priv, i915_reg_t reg, bool trace) { \
 	GEN6_READ_HEADER(x); \
 	if (NEEDS_FORCE_WAKE(offset)) \
-		__force_wake_get(dev_priv, FORCEWAKE_RENDER); \
+		__force_wake_auto(dev_priv, FORCEWAKE_RENDER); \
 	val = __raw_i915_read##x(dev_priv, reg); \
 	GEN6_READ_FOOTER; \
 }
@@ -762,7 +762,7 @@ vlv_read##x(struct drm_i915_private *dev_priv, i915_reg_t reg, bool trace) { \
 	else if (FORCEWAKE_VLV_MEDIA_RANGE_OFFSET(offset)) \
 		fw_engine = FORCEWAKE_MEDIA; \
 	if (fw_engine) \
-		__force_wake_get(dev_priv, fw_engine); \
+		__force_wake_auto(dev_priv, fw_engine); \
 	val = __raw_i915_read##x(dev_priv, reg); \
 	GEN6_READ_FOOTER; \
 }
@@ -781,7 +781,7 @@ chv_read##x(struct drm_i915_private *dev_priv, i915_reg_t reg, bool trace) { \
 	else if (FORCEWAKE_CHV_COMMON_RANGE_OFFSET(offset)) \
 		fw_engine = FORCEWAKE_RENDER | FORCEWAKE_MEDIA; \
 	if (fw_engine) \
-		__force_wake_get(dev_priv, fw_engine); \
+		__force_wake_auto(dev_priv, fw_engine); \
 	val = __raw_i915_read##x(dev_priv, reg); \
 	GEN6_READ_FOOTER; \
 }
@@ -805,7 +805,7 @@ gen9_read##x(struct drm_i915_private *dev_priv, i915_reg_t reg, bool trace) { \
 	else \
 		fw_engine = FORCEWAKE_BLITTER; \
 	if (fw_engine) \
-		__force_wake_get(dev_priv, fw_engine); \
+		__force_wake_auto(dev_priv, fw_engine); \
 	val = __raw_i915_read##x(dev_priv, reg); \
 	GEN6_READ_FOOTER; \
 }
@@ -969,7 +969,7 @@ static void \
 gen8_write##x(struct drm_i915_private *dev_priv, i915_reg_t reg, u##x val, bool trace) { \
 	GEN6_WRITE_HEADER; \
 	if (NEEDS_FORCE_WAKE(offset) && !is_gen8_shadowed(dev_priv, reg)) \
-		__force_wake_get(dev_priv, FORCEWAKE_RENDER); \
+		__force_wake_auto(dev_priv, FORCEWAKE_RENDER); \
 	__raw_i915_write##x(dev_priv, reg, val); \
 	GEN6_WRITE_FOOTER; \
 }
@@ -989,7 +989,7 @@ chv_write##x(struct drm_i915_private *dev_priv, i915_reg_t reg, u##x val, bool t
 	else if (FORCEWAKE_CHV_COMMON_RANGE_OFFSET(offset)) \
 		fw_engine = FORCEWAKE_RENDER | FORCEWAKE_MEDIA; \
 	if (fw_engine) \
-		__force_wake_get(dev_priv, fw_engine); \
+		__force_wake_auto(dev_priv, fw_engine); \
 	__raw_i915_write##x(dev_priv, reg, val); \
 	GEN6_WRITE_FOOTER; \
 }
@@ -1036,7 +1036,7 @@ gen9_write##x(struct drm_i915_private *dev_priv, i915_reg_t reg, u##x val, \
 	else \
 		fw_engine = FORCEWAKE_BLITTER; \
 	if (fw_engine) \
-		__force_wake_get(dev_priv, fw_engine); \
+		__force_wake_auto(dev_priv, fw_engine); \
 	__raw_i915_write##x(dev_priv, reg, val); \
 	GEN6_WRITE_FOOTER; \
 }
-- 
2.8.0.rc3

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

Re: [PATCH] drm/i915: Rename __force_wake_get to __force_wake_auto

[Dave Gordon]

On 24/03/16 14:31, Chris Wilson wrote:
> __force_wake_get() only acquire a temporary wakeref on forcewake that is
> automatically releases when a timer expires. When reading the code
> again, I confused __intel_uncore_forcewake_get for __force_wake_get and
> to my shame thought I found a bug in an unbalanced wake_count handling.
>
> I claim that if the function had been called __force_wake_auto instead I
> would not have embarrassed myself.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/intel_uncore.c | 18 +++++++++---------
>   1 file changed, 9 insertions(+), 9 deletions(-)

But how does this fit with arming the timer from the put() elsewhere? 
For consistency, should we not also arm it during the put() stage of 
these combined get-access-put functions? In other words, put it into the 
GEN6_{READ,WRITE}_FOOTER macros? And could they not be structured to use 
the same underlying set of functions, i.e. get -> inc ref, write 
register if previously zero, put->if ref == 1, arm timer, else dec ref?

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

Re: [PATCH] drm/i915: Rename __force_wake_get to __force_wake_auto

[Tvrtko Ursulin]

On 24/03/16 14:31, Chris Wilson wrote:
> __force_wake_get() only acquire a temporary wakeref on forcewake that is
> automatically releases when a timer expires. When reading the code
> again, I confused __intel_uncore_forcewake_get for __force_wake_get and
> to my shame thought I found a bug in an unbalanced wake_count handling.
>
> I claim that if the function had been called __force_wake_auto instead I
> would not have embarrassed myself.

Confused myself more than once so on the basis on that I think it is a 
good naming distinction to make.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/intel_uncore.c | 18 +++++++++---------
>   1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 96799392c2c7..165ebce5b638 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -716,8 +716,8 @@ __gen2_read(64)
>   	trace_i915_reg_rw(false, reg, val, sizeof(val), trace); \
>   	return val
>
> -static inline void __force_wake_get(struct drm_i915_private *dev_priv,
> -				    enum forcewake_domains fw_domains)
> +static inline void __force_wake_auto(struct drm_i915_private *dev_priv,
> +				     enum forcewake_domains fw_domains)
>   {
>   	struct intel_uncore_forcewake_domain *domain;
>   	enum forcewake_domain_id id;
> @@ -745,7 +745,7 @@ static u##x \
>   gen6_read##x(struct drm_i915_private *dev_priv, i915_reg_t reg, bool trace) { \
>   	GEN6_READ_HEADER(x); \
>   	if (NEEDS_FORCE_WAKE(offset)) \
> -		__force_wake_get(dev_priv, FORCEWAKE_RENDER); \
> +		__force_wake_auto(dev_priv, FORCEWAKE_RENDER); \
>   	val = __raw_i915_read##x(dev_priv, reg); \
>   	GEN6_READ_FOOTER; \
>   }
> @@ -762,7 +762,7 @@ vlv_read##x(struct drm_i915_private *dev_priv, i915_reg_t reg, bool trace) { \
>   	else if (FORCEWAKE_VLV_MEDIA_RANGE_OFFSET(offset)) \
>   		fw_engine = FORCEWAKE_MEDIA; \
>   	if (fw_engine) \
> -		__force_wake_get(dev_priv, fw_engine); \
> +		__force_wake_auto(dev_priv, fw_engine); \
>   	val = __raw_i915_read##x(dev_priv, reg); \
>   	GEN6_READ_FOOTER; \
>   }
> @@ -781,7 +781,7 @@ chv_read##x(struct drm_i915_private *dev_priv, i915_reg_t reg, bool trace) { \
>   	else if (FORCEWAKE_CHV_COMMON_RANGE_OFFSET(offset)) \
>   		fw_engine = FORCEWAKE_RENDER | FORCEWAKE_MEDIA; \
>   	if (fw_engine) \
> -		__force_wake_get(dev_priv, fw_engine); \
> +		__force_wake_auto(dev_priv, fw_engine); \
>   	val = __raw_i915_read##x(dev_priv, reg); \
>   	GEN6_READ_FOOTER; \
>   }
> @@ -805,7 +805,7 @@ gen9_read##x(struct drm_i915_private *dev_priv, i915_reg_t reg, bool trace) { \
>   	else \
>   		fw_engine = FORCEWAKE_BLITTER; \
>   	if (fw_engine) \
> -		__force_wake_get(dev_priv, fw_engine); \
> +		__force_wake_auto(dev_priv, fw_engine); \
>   	val = __raw_i915_read##x(dev_priv, reg); \
>   	GEN6_READ_FOOTER; \
>   }
> @@ -969,7 +969,7 @@ static void \
>   gen8_write##x(struct drm_i915_private *dev_priv, i915_reg_t reg, u##x val, bool trace) { \
>   	GEN6_WRITE_HEADER; \
>   	if (NEEDS_FORCE_WAKE(offset) && !is_gen8_shadowed(dev_priv, reg)) \
> -		__force_wake_get(dev_priv, FORCEWAKE_RENDER); \
> +		__force_wake_auto(dev_priv, FORCEWAKE_RENDER); \
>   	__raw_i915_write##x(dev_priv, reg, val); \
>   	GEN6_WRITE_FOOTER; \
>   }
> @@ -989,7 +989,7 @@ chv_write##x(struct drm_i915_private *dev_priv, i915_reg_t reg, u##x val, bool t
>   	else if (FORCEWAKE_CHV_COMMON_RANGE_OFFSET(offset)) \
>   		fw_engine = FORCEWAKE_RENDER | FORCEWAKE_MEDIA; \
>   	if (fw_engine) \
> -		__force_wake_get(dev_priv, fw_engine); \
> +		__force_wake_auto(dev_priv, fw_engine); \
>   	__raw_i915_write##x(dev_priv, reg, val); \
>   	GEN6_WRITE_FOOTER; \
>   }
> @@ -1036,7 +1036,7 @@ gen9_write##x(struct drm_i915_private *dev_priv, i915_reg_t reg, u##x val, \
>   	else \
>   		fw_engine = FORCEWAKE_BLITTER; \
>   	if (fw_engine) \
> -		__force_wake_get(dev_priv, fw_engine); \
> +		__force_wake_auto(dev_priv, fw_engine); \
>   	__raw_i915_write##x(dev_priv, reg, val); \
>   	GEN6_WRITE_FOOTER; \
>   }
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Rename __force_wake_get to __force_wake_auto

[Chris Wilson]

On Thu, Mar 24, 2016 at 03:12:02PM +0000, Dave Gordon wrote:
> On 24/03/16 14:31, Chris Wilson wrote:
> >__force_wake_get() only acquire a temporary wakeref on forcewake that is
> >automatically releases when a timer expires. When reading the code
> >again, I confused __intel_uncore_forcewake_get for __force_wake_get and
> >to my shame thought I found a bug in an unbalanced wake_count handling.
> >
> >I claim that if the function had been called __force_wake_auto instead I
> >would not have embarrassed myself.
> >
> >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> >Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> >---
> >  drivers/gpu/drm/i915/intel_uncore.c | 18 +++++++++---------
> >  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> But how does this fit with arming the timer from the put()
> elsewhere? For consistency, should we not also arm it during the
> put() stage of these combined get-access-put functions? In other
> words, put it into the GEN6_{READ,WRITE}_FOOTER macros? And could
> they not be structured to use the same underlying set of functions,
> i.e. get -> inc ref, write register if previously zero, put->if ref
> == 1, arm timer, else dec ref?

We need to acquire the fw in the HEADER for obvious reasons. So the
question is whether we want to do it in the one step or two and add a
corresponding put to the FOOTER. Is the complexity of having a combined
function *and* the individual get/put worth saving a for_each_fw_domain()
loop per register access?

Yes, imo.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: warning for drm/i915: Rename __force_wake_get to __force_wake_auto

[Patchwork]

== Series Details ==

Series: drm/i915: Rename __force_wake_get to __force_wake_auto
URL   : https://patchwork.freedesktop.org/series/4864/
State : warning

== Summary ==

Series 4864v1 drm/i915: Rename __force_wake_get to __force_wake_auto
http://patchwork.freedesktop.org/api/1.0/series/4864/revisions/1/mbox/

Test gem_exec_suspend:
        Subgroup basic-s3:
                pass       -> DMESG-WARN (bsw-nuc-2)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-c:
                pass       -> SKIP       (hsw-gt2)
Test pm_rpm:
        Subgroup basic-rte:
                pass       -> DMESG-WARN (bsw-nuc-2)
                dmesg-warn -> PASS       (byt-nuc) UNSTABLE

bdw-nuci7        total:192  pass:179  dwarn:0   dfail:0   fail:1   skip:12 
bdw-ultra        total:192  pass:170  dwarn:0   dfail:0   fail:1   skip:21 
bsw-nuc-2        total:192  pass:153  dwarn:2   dfail:0   fail:0   skip:37 
byt-nuc          total:192  pass:157  dwarn:0   dfail:0   fail:0   skip:35 
hsw-brixbox      total:192  pass:170  dwarn:0   dfail:0   fail:0   skip:22 
hsw-gt2          total:192  pass:174  dwarn:0   dfail:0   fail:0   skip:18 
ivb-t430s        total:192  pass:167  dwarn:0   dfail:0   fail:0   skip:25 
skl-i7k-2        total:192  pass:169  dwarn:0   dfail:0   fail:0   skip:23 
skl-nuci5        total:192  pass:181  dwarn:0   dfail:0   fail:0   skip:11 
snb-dellxps      total:192  pass:158  dwarn:0   dfail:0   fail:0   skip:34 

Results at /archive/results/CI_IGT_test/Patchwork_1710/

f5d413cccefa1f93d64c34f357151d42add63a84 drm-intel-nightly: 2016y-03m-24d-14h-34m-29s UTC integration manifest
c2a5605a3ca807381aefbcbc7b4097391b55a268 drm/i915: Rename __force_wake_get to __force_wake_auto

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

Re: ✗ Fi.CI.BAT: warning for drm/i915: Rename __force_wake_get to __force_wake_auto

[Mika Kuoppala]

Patchwork <patchwork@emeril.freedesktop.org> writes:

> [ text/plain ]
> == Series Details ==
>
> Series: drm/i915: Rename __force_wake_get to __force_wake_auto
> URL   : https://patchwork.freedesktop.org/series/4864/
> State : warning
>
> == Summary ==
>
> Series 4864v1 drm/i915: Rename __force_wake_get to __force_wake_auto
> http://patchwork.freedesktop.org/api/1.0/series/4864/revisions/1/mbox/
>
> Test gem_exec_suspend:
>         Subgroup basic-s3:
>                 pass       -> DMESG-WARN (bsw-nuc-2)

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

> Test kms_pipe_crc_basic:
>         Subgroup suspend-read-crc-pipe-c:
>                 pass       -> SKIP       (hsw-gt2)
> Test pm_rpm:
>         Subgroup basic-rte:
>                 pass       -> DMESG-WARN (bsw-nuc-2)
>                 dmesg-warn -> PASS       (byt-nuc) UNSTABLE

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

>
> bdw-nuci7        total:192  pass:179  dwarn:0   dfail:0   fail:1   skip:12 
> bdw-ultra        total:192  pass:170  dwarn:0   dfail:0   fail:1   skip:21 
> bsw-nuc-2        total:192  pass:153  dwarn:2   dfail:0   fail:0   skip:37 
> byt-nuc          total:192  pass:157  dwarn:0   dfail:0   fail:0   skip:35 
> hsw-brixbox      total:192  pass:170  dwarn:0   dfail:0   fail:0   skip:22 
> hsw-gt2          total:192  pass:174  dwarn:0   dfail:0   fail:0   skip:18 
> ivb-t430s        total:192  pass:167  dwarn:0   dfail:0   fail:0   skip:25 
> skl-i7k-2        total:192  pass:169  dwarn:0   dfail:0   fail:0   skip:23 
> skl-nuci5        total:192  pass:181  dwarn:0   dfail:0   fail:0   skip:11 
> snb-dellxps      total:192  pass:158  dwarn:0   dfail:0   fail:0   skip:34 
>
> Results at /archive/results/CI_IGT_test/Patchwork_1710/
>
> f5d413cccefa1f93d64c34f357151d42add63a84 drm-intel-nightly: 2016y-03m-24d-14h-34m-29s UTC integration manifest
> c2a5605a3ca807381aefbcbc7b4097391b55a268 drm/i915: Rename __force_wake_get to __force_wake_auto
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for drm/i915: Rename __force_wake_get to __force_wake_auto

[Patchwork]

== Series Details ==

Series: drm/i915: Rename __force_wake_get to __force_wake_auto
URL   : https://patchwork.freedesktop.org/series/4864/
State : failure

== Summary ==

Series 4864v1 drm/i915: Rename __force_wake_get to __force_wake_auto
2016-03-30T10:43:14.185826 http://patchwork.freedesktop.org/api/1.0/series/4864/revisions/1/mbox/
Applying: drm/i915: Rename __force_wake_get to __force_wake_auto
Patch failed at 0001 drm/i915: Rename __force_wake_get to __force_wake_auto

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