[PATCH 0/2] Optimize WARN_ON macros

[PATCH 0/2] Optimize WARN_ON macros

[Joonas Lahtinen]

Joonas Lahtinen (2):
  drm/i915: Simplify _STATE_ debug macros
  drm/i915: Compile time concatenate WARN_ON macro strings

 drivers/gpu/drm/i915/i915_drv.h | 23 ++++++-----------------
 1 file changed, 6 insertions(+), 17 deletions(-)

-- 
2.4.3

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

[PATCH 1/2] drm/i915: Simplify _STATE_ debug macros

[Joonas Lahtinen]

Take advantage of WARN return value to simplify the flow.

Cc: Rob Clark <robdclark@gmail.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Reported-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 1d28d90..5a5a3e0 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -87,23 +87,18 @@
  */
 #define I915_STATE_WARN(condition, format...) ({			\
 	int __ret_warn_on = !!(condition);				\
-	if (unlikely(__ret_warn_on)) {					\
-		if (i915.verbose_state_checks)				\
-			WARN(1, format);				\
-		else 							\
+	if (unlikely(__ret_warn_on))					\
+		if (!WARN(i915.verbose_state_checks, format))		\
 			DRM_ERROR(format);				\
-	}								\
 	unlikely(__ret_warn_on);					\
 })
 
 #define I915_STATE_WARN_ON(condition) ({				\
 	int __ret_warn_on = !!(condition);				\
-	if (unlikely(__ret_warn_on)) {					\
-		if (i915.verbose_state_checks)				\
-			WARN(1, "WARN_ON(" #condition ")\n");		\
-		else 							\
+	if (unlikely(__ret_warn_on))					\
+		if (!WARN(i915.verbose_state_checks,			\
+			  "WARN_ON(" #condition ")\n"))			\
 			DRM_ERROR("WARN_ON(" #condition ")\n");		\
-	}								\
 	unlikely(__ret_warn_on);					\
 })
 
-- 
2.4.3

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

[PATCH 2/2] drm/i915: Compile time concatenate WARN_ON macro strings

[Joonas Lahtinen]

Using __stringify(x) instead of #x adds support for macros as
a parameter and reduces runtime overhead.

Slightly increases the .text size but should not matter.

v2:
- Define I915_STATE_WARN_ON though I915_STATE_WARN
  (Bikeshed inspiration by Chris)

Cc: Rob Clark <robdclark@gmail.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 5a5a3e0..1ccd137 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -69,11 +69,11 @@
 		BUILD_BUG_ON(__i915_warn_cond); \
 	WARN(__i915_warn_cond, "WARN_ON(" #x ")"); })
 #else
-#define WARN_ON(x) WARN((x), "WARN_ON(%s)", #x )
+#define WARN_ON(x) WARN((x), "WARN_ON(" __stringify(x) ")")
 #endif
 
 #undef WARN_ON_ONCE
-#define WARN_ON_ONCE(x) WARN_ONCE((x), "WARN_ON_ONCE(%s)", #x )
+#define WARN_ON_ONCE(x) WARN_ONCE((x), "WARN_ON_ONCE(" __stringify(x) ")")
 
 #define MISSING_CASE(x) WARN(1, "Missing switch case (%lu) in %s\n", \
 			     (long) (x), __func__);
@@ -93,14 +93,8 @@
 	unlikely(__ret_warn_on);					\
 })
 
-#define I915_STATE_WARN_ON(condition) ({				\
-	int __ret_warn_on = !!(condition);				\
-	if (unlikely(__ret_warn_on))					\
-		if (!WARN(i915.verbose_state_checks,			\
-			  "WARN_ON(" #condition ")\n"))			\
-			DRM_ERROR("WARN_ON(" #condition ")\n");		\
-	unlikely(__ret_warn_on);					\
-})
+#define I915_STATE_WARN_ON(x)						\
+	I915_STATE_WARN((x), "WARN_ON(" __stringify(x) ")")
 
 static inline const char *yesno(bool v)
 {
-- 
2.4.3

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

Re: [PATCH 2/2] drm/i915: Compile time concatenate WARN_ON macro strings

[Chris Wilson]

On Fri, Dec 18, 2015 at 12:39:17PM +0200, Joonas Lahtinen wrote:
> Using __stringify(x) instead of #x adds support for macros as
> a parameter and reduces runtime overhead.
> 
> Slightly increases the .text size but should not matter.
> 
> v2:
> - Define I915_STATE_WARN_ON though I915_STATE_WARN
>   (Bikeshed inspiration by Chris)
> 
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Neat. Both
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-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

✗ warning: Fi.CI.BAT

[Patchwork]

== Summary ==

Built on 3c71db89fc37b6061ce6b070ce73f89155da5f20 drm-intel-nightly: 2015y-12m-18d-09h-42m-49s UTC integration manifest

Test igt@kms_flip@basic-flip-vs-wf_vblank on ivb-t430s pass -> dmesg-warn
Test igt@kms_flip@basic-flip-vs-wf_vblank on byt-nuc dmesg-warn -> skip
Test igt@kms_flip@basic-flip-vs-wf_vblank on bsw-nuc-2 pass -> dmesg-warn
Test igt@kms_pipe_crc_basic@hang-read-crc-pipe-b on byt-nuc pass -> skip
Test igt@kms_pipe_crc_basic@hang-read-crc-pipe-a on ivb-t430s dmesg-warn -> pass
Test igt@gem_mmap@basic on byt-nuc dmesg-warn -> pass
Test igt@kms_flip@basic-flip-vs-dpms on byt-nuc pass -> skip
Test igt@drv_module_reload_basic on byt-nuc pass -> dmesg-warn
Test igt@kms_flip@basic-plain-flip on bsw-nuc-2 pass -> dmesg-warn
Test igt@gem_mmap_gtt@basic-write on bsw-nuc-2 dmesg-warn -> pass
Test igt@kms_pipe_crc_basic@read-crc-pipe-a on snb-x220t dmesg-warn -> pass
Test igt@kms_pipe_crc_basic@read-crc-pipe-a on byt-nuc dmesg-warn -> pass
Test igt@kms_pipe_crc_basic@read-crc-pipe-c on skl-i5k-2 dmesg-warn -> pass
Test igt@kms_flip@basic-flip-vs-modeset on snb-x220t pass -> dmesg-warn
Test igt@kms_flip@basic-flip-vs-modeset on hsw-brixbox dmesg-warn -> pass
Test igt@kms_flip@basic-flip-vs-modeset on skl-i5k-2 pass -> dmesg-warn

bdw-nuci7        total:135  pass:125  dwarn:1   dfail:0   fail:0   skip:9  
bsw-nuc-2        total:135  pass:113  dwarn:2   dfail:0   fail:0   skip:20 
byt-nuc          total:135  pass:117  dwarn:2   dfail:0   fail:0   skip:16 
hsw-brixbox      total:135  pass:127  dwarn:1   dfail:0   fail:0   skip:7  
hsw-gt2          total:135  pass:130  dwarn:1   dfail:0   fail:0   skip:4  
ilk-hp8440p      total:135  pass:100  dwarn:0   dfail:0   fail:2   skip:33 
ivb-t430s        total:135  pass:128  dwarn:1   dfail:1   fail:1   skip:4  
skl-i5k-2        total:135  pass:122  dwarn:5   dfail:0   fail:0   skip:8  
skl-i7k-2        total:135  pass:123  dwarn:4   dfail:0   fail:0   skip:8  
snb-dellxps      total:135  pass:121  dwarn:2   dfail:0   fail:0   skip:12 
snb-x220t        total:135  pass:121  dwarn:2   dfail:0   fail:1   skip:11 

Results at /archive/results/CI_IGT_test/Patchwork_713/

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

Re: [PATCH 2/2] drm/i915: Compile time concatenate WARN_ON macro strings

[Dave Gordon]

On 18/12/15 10:39, Joonas Lahtinen wrote:
> Using __stringify(x) instead of #x adds support for macros as
> a parameter and reduces runtime overhead.
>
> Slightly increases the .text size but should not matter.
>
> v2:
> - Define I915_STATE_WARN_ON though I915_STATE_WARN
>    (Bikeshed inspiration by Chris)
>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.h | 14 ++++----------
>   1 file changed, 4 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 5a5a3e0..1ccd137 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -69,11 +69,11 @@
>   		BUILD_BUG_ON(__i915_warn_cond); \
>   	WARN(__i915_warn_cond, "WARN_ON(" #x ")"); })
>   #else
> -#define WARN_ON(x) WARN((x), "WARN_ON(%s)", #x )
> +#define WARN_ON(x) WARN((x), "WARN_ON(" __stringify(x) ")")
>   #endif
>
>   #undef WARN_ON_ONCE
> -#define WARN_ON_ONCE(x) WARN_ONCE((x), "WARN_ON_ONCE(%s)", #x )
> +#define WARN_ON_ONCE(x) WARN_ONCE((x), "WARN_ON_ONCE(" __stringify(x) ")")
>
>   #define MISSING_CASE(x) WARN(1, "Missing switch case (%lu) in %s\n", \
>   			     (long) (x), __func__);
> @@ -93,14 +93,8 @@
>   	unlikely(__ret_warn_on);					\
>   })
>
> -#define I915_STATE_WARN_ON(condition) ({				\
> -	int __ret_warn_on = !!(condition);				\
> -	if (unlikely(__ret_warn_on))					\
> -		if (!WARN(i915.verbose_state_checks,			\
> -			  "WARN_ON(" #condition ")\n"))			\
> -			DRM_ERROR("WARN_ON(" #condition ")\n");		\
> -	unlikely(__ret_warn_on);					\
> -})
> +#define I915_STATE_WARN_ON(x)						\
> +	I915_STATE_WARN((x), "WARN_ON(" __stringify(x) ")")
>
>   static inline const char *yesno(bool v)
>   {

NAK.

This will give compile-time warnings for lines such as:

	WARN_ON(x%16 != 0);

because the stringified text of the expression (which in this case 
contains a "%" character) would appear as part of the format string, 
rather than inside an argument. See:

4eee492 drm/i915: fix driver's versions of WARN_ON & WARN_ON_ONCE

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

Re: [PATCH 2/2] drm/i915: Compile time concatenate WARN_ON macro strings

[Joonas Lahtinen]

On pe, 2015-12-18 at 12:03 +0000, Dave Gordon wrote:
> On 18/12/15 10:39, Joonas Lahtinen wrote:
> > Using __stringify(x) instead of #x adds support for macros as
> > a parameter and reduces runtime overhead.
> > 
> > Slightly increases the .text size but should not matter.
> > 
> > v2:
> > - Define I915_STATE_WARN_ON though I915_STATE_WARN
> >    (Bikeshed inspiration by Chris)
> > 
> > Cc: Rob Clark <robdclark@gmail.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_drv.h | 14 ++++----------
> >   1 file changed, 4 insertions(+), 10 deletions(-)
> > 
> > 

<SNIP>

> > 					
> > \
> > -})
> > +#define I915_STATE_WARN_ON(x)					
> > 	\
> > +	I915_STATE_WARN((x), "WARN_ON(" __stringify(x) ")")
> > 
> >   static inline const char *yesno(bool v)
> >   {
> 
> NAK.
> 
> This will give compile-time warnings for lines such as:
> 
> 	WARN_ON(x%16 != 0);
> 
> because the stringified text of the expression (which in this case 
> contains a "%" character) would appear as part of the format string, 
> rather than inside an argument. See:
> 
> 4eee492 drm/i915: fix driver's versions of WARN_ON & WARN_ON_ONCE
> 

Great catch, I'll change it to have preceding "%s". Did not get any
compile-time warning though, this would be something caught by a
(rather advanced?) static analysis.

Regards, Joonas

> .Dave.
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

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

Re: [PATCH 2/2] drm/i915: Compile time concatenate WARN_ON macro strings

[Daniel Vetter]

On Fri, Dec 18, 2015 at 02:11:24PM +0200, Joonas Lahtinen wrote:
> On pe, 2015-12-18 at 12:03 +0000, Dave Gordon wrote:
> > On 18/12/15 10:39, Joonas Lahtinen wrote:
> > > Using __stringify(x) instead of #x adds support for macros as
> > > a parameter and reduces runtime overhead.
> > > 
> > > Slightly increases the .text size but should not matter.
> > > 
> > > v2:
> > > - Define I915_STATE_WARN_ON though I915_STATE_WARN
> > >    (Bikeshed inspiration by Chris)
> > > 
> > > Cc: Rob Clark <robdclark@gmail.com>
> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/i915_drv.h | 14 ++++----------
> > >   1 file changed, 4 insertions(+), 10 deletions(-)
> > > 
> > > 
> 
> <SNIP>
> 
> > > 					
> > > \
> > > -})
> > > +#define I915_STATE_WARN_ON(x)					
> > > 	\
> > > +	I915_STATE_WARN((x), "WARN_ON(" __stringify(x) ")")
> > > 
> > >   static inline const char *yesno(bool v)
> > >   {
> > 
> > NAK.
> > 
> > This will give compile-time warnings for lines such as:
> > 
> > 	WARN_ON(x%16 != 0);
> > 
> > because the stringified text of the expression (which in this case 
> > contains a "%" character) would appear as part of the format string, 
> > rather than inside an argument. See:
> > 
> > 4eee492 drm/i915: fix driver's versions of WARN_ON & WARN_ON_ONCE
> > 
> 
> Great catch, I'll change it to have preceding "%s". Did not get any
> compile-time warning though, this would be something caught by a
> (rather advanced?) static analysis.

New gcc seems to complain about this. At least since a few weeks I've seen
a few patches for kernel-wide changes to address this float about.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx