On 26/01/16 09:44, Joonas Lahtinen wrote:
On ma, 2016-01-25 at 18:57 +0000, Dave Gordon wrote:
On 25/01/16 18:17, Daniel Vetter wrote:
On Fri, Jan 22, 2016 at 05:54:15PM +0530, akash.goel@xxxxxxxxx
wrote:
From: Akash Goel <akash.goel@xxxxxxxxx>
Added a new macro i915_dbg, which is a wrapper over dev_dbg
macro.
dev_dbg allows use of dynamic debug framework, so offers a number
of advantages over DRM_DEBUG to debug user space startup issues.
Like provides more fine grain control by allowing to
enable/disable
certain debug messages of interest on the fly, also allows
filtering
of debug messages based on pid.
Suggested-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Signed-off-by: Akash Goel <akash.goel@xxxxxxxxx>
---
drivers/gpu/drm/i915/i915_drv.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.h
b/drivers/gpu/drm/i915/i915_drv.h
index bc7164f..749513f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2456,6 +2456,7 @@ struct drm_i915_cmd_table {
BUILD_BUG(); \
__p; \
})
+#define i915_dbg(DEV, args...) dev_dbg(__I915__(DEV)-
dev->dev, ##args)
I915_DBG(...) ?
It's conventional that macros should be UPPERCASE.
Especially when some config options may mean that the code
disappears
entirely, so you have to be sure not to use arguments with side-
effects!
Slight correction here (for future), from Kernel Coding Style
documentation;
"CAPITALIZED macro names are appreciated but macros resembling
functions may be named in lower case."
And looking at "include/linux/device.h", dev_dbg definition is a macro
too, like almost all the printing functions. I'd rather see it as
i915_dbg. Arguments with side effects can be handled nicely as can be
seen.
We really should increase the priority of modernizing the debugging
infrastructure for i915 (and as a dependency for DRM as Daniel hoped).
Regards, Joonas
.Dave.
The fact that the upstream definitions are not great doesn't mean we
should copy the flaws:
#if defined(CONFIG_DYNAMIC_DEBUG)
#define dev_dbg(dev, format, ...) \
do { \
dynamic_dev_dbg(dev, format, ##__VA_ARGS__); \
} while (0)
#elif defined(DEBUG)
#define dev_dbg(dev, format, arg...) \
dev_printk(KERN_DEBUG, dev, format, ##arg)
#else
#define dev_dbg(dev, format, arg...) \
({ \
if (0) \
dev_printk(KERN_DEBUG, dev, format, ##arg); \
})
#endif
So what's wrong with the above?
Firstly, the CONFIG_DYNAMIC_DEBUG version is wrapped in a do-while(0)
but the others aren't; this makes them different syntactically - it's a
statement body, whereas the others are (void) expressions. In either
case, writing
x = dev_dbg(...);
will give an error (different errors, though!). But the following:
x = 1, dev_dbg(...);
compiles if not CONFIG_DYNAMIC_DEBUG. You probably wouldn't write the
above, but it could itself be the result of a macro expansion, and it
would work (x is assigned 1, dev_dbg() is called) ... until you try to
enable dynamic debug.
(IMHO they should all be wrapped, which ensures you can't get away with
using it in any other way than as a statement.)
Secondly, the CONFIG_DYNAMIC_DEBUG version uses the C99 __VA_ARGS__
syntax, whereas the others use the GCC-specific "arg..." method. This
*probably* won't matter but it's an unnecessary inconsistency.
Thirdly, the non-DEBUG version doesn't evaluate its arguments, whereas
the other two obviously do. So code that includes a side-effect inside
the parameters to the call will behave differently; and there'll be no
clue at all that something that looks like a regular function call:
dev_dbg(mydev, "Been here %d times now", ++i);
... may or may not increment i, depending on the compile-time definition
above. This is just laying traps for the developer; calling it DEV_DBG()
might at least make people *notice* that it's a macro not a function!
.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
