Re: [PATCH] drm/i915: Introduce i915_dbg macro

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Jan 27, 2016 at 05:32:55PM +0000, Dave Gordon wrote:
> 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@intel.com
> >>>wrote:
> >>>>From: Akash Goel <akash.goel@intel.com>
> >>>>
> >>>>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@chris-wilson.co.uk>
> >>>>Signed-off-by: Akash Goel <akash.goel@intel.com>
> >>>>---
> >>>>   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:

We _are_ upstream, at least in some way. So if the dynamic debug stuff is
somewhat silly like you point out below, please let's just fix it up.

I know that sometimes discussions in a larger scope degenerate to a
positional stalemate, but we shouldn't ever not try this as the first
approach. Hence also why I suggested to at least drag along the drm debug
infrastructure overall in such an effort to improve debug logging tooling.

Thanks, 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