* [PATCH v3 0/2] Optimize WARN_ON macros
@ 2015-12-18 12:27 Joonas Lahtinen
2015-12-18 12:27 ` [PATCH v3 1/2] drm/i915: Simplify _STATE_ debug macros Joonas Lahtinen
` (3 more replies)
0 siblings, 4 replies; 11+ messages in thread
From: Joonas Lahtinen @ 2015-12-18 12:27 UTC (permalink / raw)
To: Intel graphics driver community testing & development
v3:
- Fixed potential OOPS scenario with modulo in condition
(Dave Gordon)
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3 1/2] drm/i915: Simplify _STATE_ debug macros
2015-12-18 12:27 [PATCH v3 0/2] Optimize WARN_ON macros Joonas Lahtinen
@ 2015-12-18 12:27 ` Joonas Lahtinen
2015-12-18 16:18 ` Dave Gordon
2015-12-18 12:27 ` [PATCH v3 2/2] drm/i915: Compile-time concatenate WARN_ON macro strings Joonas Lahtinen
` (2 subsequent siblings)
3 siblings, 1 reply; 11+ messages in thread
From: Joonas Lahtinen @ 2015-12-18 12:27 UTC (permalink / raw)
To: Intel graphics driver community testing & development
Take advantage of WARN return value to simplify the flow.
Cc: Rob Clark <robdclark@gmail.com>
Reviewed-by: 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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v3 2/2] drm/i915: Compile-time concatenate WARN_ON macro strings
2015-12-18 12:27 [PATCH v3 0/2] Optimize WARN_ON macros Joonas Lahtinen
2015-12-18 12:27 ` [PATCH v3 1/2] drm/i915: Simplify _STATE_ debug macros Joonas Lahtinen
@ 2015-12-18 12:27 ` Joonas Lahtinen
2015-12-22 9:03 ` Daniel Vetter
2015-12-18 13:20 ` ✗ warning: Fi.CI.BAT Patchwork
2015-12-18 14:00 ` Patchwork
3 siblings, 1 reply; 11+ messages in thread
From: Joonas Lahtinen @ 2015-12-18 12:27 UTC (permalink / raw)
To: Intel graphics driver community testing & development
Using __stringify(x) instead of #x adds support for macros as
a parameter and compile-time concatenation reduces the 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)
v3:
- More specific commit message
v4:
- Do not directly pass arbitary string as format, instead
guard with "%s" (Dave)
Cc: Rob Clark <robdclark@gmail.com>
Cc: Dave Gordon <david.s.gordon@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> (v3)
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..664ceb4 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), "%s", "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), "%s", "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), "%s", "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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* ✗ warning: Fi.CI.BAT
2015-12-18 12:27 [PATCH v3 0/2] Optimize WARN_ON macros Joonas Lahtinen
2015-12-18 12:27 ` [PATCH v3 1/2] drm/i915: Simplify _STATE_ debug macros Joonas Lahtinen
2015-12-18 12:27 ` [PATCH v3 2/2] drm/i915: Compile-time concatenate WARN_ON macro strings Joonas Lahtinen
@ 2015-12-18 13:20 ` Patchwork
2015-12-18 14:00 ` Patchwork
3 siblings, 0 replies; 11+ messages in thread
From: Patchwork @ 2015-12-18 13:20 UTC (permalink / raw)
To: Joonas Lahtinen; +Cc: intel-gfx
== 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 bsw-nuc-2 pass -> dmesg-warn
Test igt@gem_storedw_loop@basic-render on skl-i7k-2 pass -> dmesg-warn
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@gem_mmap_gtt@basic-write on bsw-nuc-2 dmesg-warn -> pass
Test igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a on snb-x220t pass -> dmesg-warn
Test igt@kms_pipe_crc_basic@read-crc-pipe-a on ilk-hp8440p pass -> dmesg-warn
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 bdw-nuci7 pass -> dmesg-warn
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 hsw-brixbox dmesg-warn -> pass
bdw-nuci7 total:135 pass:124 dwarn:2 dfail:0 fail:0 skip:9
bdw-ultra total:132 pass:124 dwarn:2 dfail:0 fail:0 skip:6
bsw-nuc-2 total:135 pass:114 dwarn:1 dfail:0 fail:0 skip:20
byt-nuc total:135 pass:120 dwarn:2 dfail:0 fail:0 skip:13
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
hsw-xps12 total:132 pass:125 dwarn:3 dfail:0 fail:0 skip:4
ilk-hp8440p total:135 pass:99 dwarn:1 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:123 dwarn:4 dfail:0 fail:0 skip:8
skl-i7k-2 total:135 pass:122 dwarn:5 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_717/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* ✗ warning: Fi.CI.BAT
2015-12-18 12:27 [PATCH v3 0/2] Optimize WARN_ON macros Joonas Lahtinen
` (2 preceding siblings ...)
2015-12-18 13:20 ` ✗ warning: Fi.CI.BAT Patchwork
@ 2015-12-18 14:00 ` Patchwork
3 siblings, 0 replies; 11+ messages in thread
From: Patchwork @ 2015-12-18 14:00 UTC (permalink / raw)
To: Joonas Lahtinen; +Cc: intel-gfx
== Summary ==
Built on 3c71db89fc37b6061ce6b070ce73f89155da5f20 drm-intel-nightly: 2015y-12m-18d-09h-42m-49s UTC integration manifest
Test gem_mmap:
Subgroup basic:
dmesg-warn -> PASS (byt-nuc)
Test gem_mmap_gtt:
Subgroup basic-write:
dmesg-warn -> PASS (bsw-nuc-2)
Test gem_storedw_loop:
Subgroup basic-render:
pass -> DMESG-WARN (skl-i7k-2)
Test kms_flip:
Subgroup basic-flip-vs-modeset:
dmesg-warn -> PASS (hsw-brixbox)
Subgroup basic-flip-vs-wf_vblank:
pass -> DMESG-WARN (bsw-nuc-2)
pass -> DMESG-WARN (ivb-t430s)
Test kms_pipe_crc_basic:
Subgroup hang-read-crc-pipe-a:
dmesg-warn -> PASS (ivb-t430s)
Subgroup read-crc-pipe-a:
dmesg-warn -> PASS (snb-x220t)
pass -> DMESG-WARN (ilk-hp8440p)
dmesg-warn -> PASS (byt-nuc)
Subgroup read-crc-pipe-c:
dmesg-warn -> PASS (skl-i5k-2)
pass -> DMESG-WARN (bdw-nuci7)
Subgroup suspend-read-crc-pipe-a:
pass -> DMESG-WARN (snb-x220t)
bdw-nuci7 total:135 pass:124 dwarn:2 dfail:0 fail:0 skip:9
bdw-ultra total:132 pass:124 dwarn:2 dfail:0 fail:0 skip:6
bsw-nuc-2 total:135 pass:114 dwarn:1 dfail:0 fail:0 skip:20
byt-nuc total:135 pass:120 dwarn:2 dfail:0 fail:0 skip:13
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
hsw-xps12 total:132 pass:125 dwarn:3 dfail:0 fail:0 skip:4
ilk-hp8440p total:135 pass:99 dwarn:1 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:123 dwarn:4 dfail:0 fail:0 skip:8
skl-i7k-2 total:135 pass:122 dwarn:5 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_717/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 1/2] drm/i915: Simplify _STATE_ debug macros
2015-12-18 12:27 ` [PATCH v3 1/2] drm/i915: Simplify _STATE_ debug macros Joonas Lahtinen
@ 2015-12-18 16:18 ` Dave Gordon
2015-12-21 8:11 ` Joonas Lahtinen
0 siblings, 1 reply; 11+ messages in thread
From: Dave Gordon @ 2015-12-18 16:18 UTC (permalink / raw)
To: Joonas Lahtinen,
Intel graphics driver community testing & development
On 18/12/15 12:27, Joonas Lahtinen wrote:
> Take advantage of WARN return value to simplify the flow.
>
> Cc: Rob Clark <robdclark@gmail.com>
> Reviewed-by: 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"); \
These last two lines still have the text of the condition as part of a
format string :(
For compile-testing, you might want to change:
static void lpt_bend_clkout_dp(struct drm_i915_private *dev_priv
...
if (WARN_ON(steps % 5 != 0))
return;
to use I915_STATE_WARN_ON() instead of WARN_ON, then you should get a
compile-time warning if the '%' ends up in the format string.
.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 1/2] drm/i915: Simplify _STATE_ debug macros
2015-12-18 16:18 ` Dave Gordon
@ 2015-12-21 8:11 ` Joonas Lahtinen
2015-12-21 11:53 ` Dave Gordon
0 siblings, 1 reply; 11+ messages in thread
From: Joonas Lahtinen @ 2015-12-21 8:11 UTC (permalink / raw)
To: Dave Gordon, Intel graphics driver community testing & development
On pe, 2015-12-18 at 16:18 +0000, Dave Gordon wrote:
> On 18/12/15 12:27, Joonas Lahtinen wrote:
> > Take advantage of WARN return value to simplify the flow.
> >
> > Cc: Rob Clark <robdclark@gmail.com>
> > Reviewed-by: 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");
> > \
>
> These last two lines still have the text of the condition as part of
> a
> format string :(
>
> For compile-testing, you might want to change:
>
> static void lpt_bend_clkout_dp(struct drm_i915_private *dev_priv
> ...
> if (WARN_ON(steps % 5 != 0))
> return;
>
> to use I915_STATE_WARN_ON() instead of WARN_ON, then you should get a
> compile-time warning if the '%' ends up in the format string.
>
This is just a patch to convert the old macros to different order
before changing them. The way of constructing the strings is intact.
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 1/2] drm/i915: Simplify _STATE_ debug macros
2015-12-21 8:11 ` Joonas Lahtinen
@ 2015-12-21 11:53 ` Dave Gordon
2015-12-21 13:41 ` Daniel Vetter
0 siblings, 1 reply; 11+ messages in thread
From: Dave Gordon @ 2015-12-21 11:53 UTC (permalink / raw)
To: Joonas Lahtinen,
Intel graphics driver community testing & development
On 21/12/15 08:11, Joonas Lahtinen wrote:
> On pe, 2015-12-18 at 16:18 +0000, Dave Gordon wrote:
>> On 18/12/15 12:27, Joonas Lahtinen wrote:
>>> Take advantage of WARN return value to simplify the flow.
>>>
>>> Cc: Rob Clark <robdclark@gmail.com>
>>> Reviewed-by: 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");
>>> \
>>
>> These last two lines still have the text of the condition as part of
>> a
>> format string :(
>>
>> For compile-testing, you might want to change:
>>
>> static void lpt_bend_clkout_dp(struct drm_i915_private *dev_priv
>> ...
>> if (WARN_ON(steps % 5 != 0))
>> return;
>>
>> to use I915_STATE_WARN_ON() instead of WARN_ON, then you should get a
>> compile-time warning if the '%' ends up in the format string.
>>
>
> This is just a patch to convert the old macros to different order
> before changing them. The way of constructing the strings is intact.
>
> Regards, Joonas
Yes, I agree, you didn't break them -- they were already wrong!
.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 1/2] drm/i915: Simplify _STATE_ debug macros
2015-12-21 11:53 ` Dave Gordon
@ 2015-12-21 13:41 ` Daniel Vetter
2015-12-22 8:41 ` Joonas Lahtinen
0 siblings, 1 reply; 11+ messages in thread
From: Daniel Vetter @ 2015-12-21 13:41 UTC (permalink / raw)
To: Dave Gordon; +Cc: Intel graphics driver community testing & development
On Mon, Dec 21, 2015 at 11:53:52AM +0000, Dave Gordon wrote:
> On 21/12/15 08:11, Joonas Lahtinen wrote:
> >On pe, 2015-12-18 at 16:18 +0000, Dave Gordon wrote:
> >>On 18/12/15 12:27, Joonas Lahtinen wrote:
> >>>Take advantage of WARN return value to simplify the flow.
> >>>
> >>>Cc: Rob Clark <robdclark@gmail.com>
> >>>Reviewed-by: 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");
> >>> \
> >>
> >>These last two lines still have the text of the condition as part of
> >>a
> >>format string :(
> >>
> >>For compile-testing, you might want to change:
> >>
> >> static void lpt_bend_clkout_dp(struct drm_i915_private *dev_priv
> >> ...
> >> if (WARN_ON(steps % 5 != 0))
> >> return;
> >>
> >>to use I915_STATE_WARN_ON() instead of WARN_ON, then you should get a
> >>compile-time warning if the '%' ends up in the format string.
> >>
> >
> >This is just a patch to convert the old macros to different order
> >before changing them. The way of constructing the strings is intact.
> >
> >Regards, Joonas
>
> Yes, I agree, you didn't break them -- they were already wrong!
Yeah I think it makes sense to fix that. I'll wait for v4.
-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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 1/2] drm/i915: Simplify _STATE_ debug macros
2015-12-21 13:41 ` Daniel Vetter
@ 2015-12-22 8:41 ` Joonas Lahtinen
0 siblings, 0 replies; 11+ messages in thread
From: Joonas Lahtinen @ 2015-12-22 8:41 UTC (permalink / raw)
To: Daniel Vetter, Dave Gordon
Cc: Intel graphics driver community testing & development
On ma, 2015-12-21 at 14:41 +0100, Daniel Vetter wrote:
> On Mon, Dec 21, 2015 at 11:53:52AM +0000, Dave Gordon wrote:
> > On 21/12/15 08:11, Joonas Lahtinen wrote:
> > > On pe, 2015-12-18 at 16:18 +0000, Dave Gordon wrote:
> > > > On 18/12/15 12:27, Joonas Lahtinen wrote:
> > > > > Take advantage of WARN return value to simplify the flow.
> > > > >
> > > > > Cc: Rob Clark <robdclark@gmail.com>
> > > > > Reviewed-by: 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");
> > > > > \
> > > >
> > > > These last two lines still have the text of the condition as
> > > > part of
> > > > a
> > > > format string :(
> > > >
> > > > For compile-testing, you might want to change:
> > > >
> > > > static void lpt_bend_clkout_dp(struct drm_i915_private
> > > > *dev_priv
> > > > ...
> > > > if (WARN_ON(steps % 5 != 0))
> > > > return;
> > > >
> > > > to use I915_STATE_WARN_ON() instead of WARN_ON, then you should
> > > > get a
> > > > compile-time warning if the '%' ends up in the format string.
> > > >
> > >
> > > This is just a patch to convert the old macros to different order
> > > before changing them. The way of constructing the strings is
> > > intact.
> > >
> > > Regards, Joonas
> >
> > Yes, I agree, you didn't break them -- they were already wrong!
>
> Yeah I think it makes sense to fix that. I'll wait for v4.
Ok, to be perfectly clear; This is patch 1/2, which does as the commit
message specifies, it simplifies the function flow, patch 2/2 of the
series changes the string construction and addresses the mentioned
problem. Sorry for being little bit unclear about that on the first
comment.
Regards, Joonas
> -Daniel
--
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 2/2] drm/i915: Compile-time concatenate WARN_ON macro strings
2015-12-18 12:27 ` [PATCH v3 2/2] drm/i915: Compile-time concatenate WARN_ON macro strings Joonas Lahtinen
@ 2015-12-22 9:03 ` Daniel Vetter
0 siblings, 0 replies; 11+ messages in thread
From: Daniel Vetter @ 2015-12-22 9:03 UTC (permalink / raw)
To: Joonas Lahtinen; +Cc: Intel graphics driver community testing & development
On Fri, Dec 18, 2015 at 02:27:27PM +0200, Joonas Lahtinen wrote:
> Using __stringify(x) instead of #x adds support for macros as
> a parameter and compile-time concatenation reduces the 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)
>
> v3:
> - More specific commit message
>
> v4:
> - Do not directly pass arbitary string as format, instead
> guard with "%s" (Dave)
>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Dave Gordon <david.s.gordon@intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> (v3)
> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Both patches merged to dinq, thanks.
-Daniel
> ---
> 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..664ceb4 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), "%s", "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), "%s", "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), "%s", "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
--
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
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2015-12-22 9:03 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-12-18 12:27 [PATCH v3 0/2] Optimize WARN_ON macros Joonas Lahtinen
2015-12-18 12:27 ` [PATCH v3 1/2] drm/i915: Simplify _STATE_ debug macros Joonas Lahtinen
2015-12-18 16:18 ` Dave Gordon
2015-12-21 8:11 ` Joonas Lahtinen
2015-12-21 11:53 ` Dave Gordon
2015-12-21 13:41 ` Daniel Vetter
2015-12-22 8:41 ` Joonas Lahtinen
2015-12-18 12:27 ` [PATCH v3 2/2] drm/i915: Compile-time concatenate WARN_ON macro strings Joonas Lahtinen
2015-12-22 9:03 ` Daniel Vetter
2015-12-18 13:20 ` ✗ warning: Fi.CI.BAT Patchwork
2015-12-18 14:00 ` Patchwork
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.