| Message ID | 1453465455-10998-1-git-send-email-akash.goel@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 22/01/2016 12:24, 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_dev_dbg may be? is there any reason why we haven't used dev_dbg before in our driver? regards Arun > #define INTEL_INFO(p) (&__I915__(p)->info) > #define INTEL_DEVID(p) (INTEL_INFO(p)->device_id) > #define INTEL_REVID(p) (__I915__(p)->dev->pdev->revision) >
On Fri, Jan 22, 2016 at 12:49:01PM +0000, Arun Siluvery wrote: > On 22/01/2016 12:24, 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_dev_dbg may be? pr_debug dev_dbg i915_dbg > is there any reason why we haven't used dev_dbg before in our driver? It's much newer than DRM_DEBUG. There is a general consensus that we should migrate DRM_DEBUG over to dev_dbg (since dprintk offers more fine control over debug messages), just no plan of action. -Chris
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) Yeah, dynamic debug is awesome, but imo completely useless if it means the usual drm.debug=0xe doesn't work any more. What we imo need to roll this is is to enable this for all the drm debug infrastructure. Maybe with a compile option, or maybe with some hacks to make drm.debug=0xe still work. Yes this is a pretty big project, but I really want a unified way to enable debugging output for both drm and i915. Thanks, Daniel > #define INTEL_INFO(p) (&__I915__(p)->info) > #define INTEL_DEVID(p) (INTEL_INFO(p)->device_id) > #define INTEL_REVID(p) (__I915__(p)->dev->pdev->revision) > -- > 1.9.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
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! .Dave.
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. > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
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: #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.
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
On ke, 2016-01-27 at 17:32 +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: > > > <SNIP> > > > > > > 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: > You missed my whole point, "dev_dbg definition is a macro too, like almost all the printing functions. I'd rather see it as i915_dbg." Probably should have written "as i915_dbg than I915_DBG". No matter how the implementation, the name should be consistent with dev_dbg. > #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. That actually seems to happen; $ git grep dev_dbg | grep "++" | wc -l 16 > 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! > Meh, just improve the existing stuff. Seems like you did a good analysis on the current code, why not make some patches to fix it and other similar code? CONFIG_DYNAMIC_DEBUG code appeared afterwards, that's probably one reason for the current state. While you're at it, #ifdef logic for CONFIG_DYNAMIC_DEBUG, DEBUG and no-DEBUG is having different preference in different places (DEBUG or CONFIG_DYNAMIC_DEBUG to dominate). Regards, Joonas > .Dave.
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) #define INTEL_INFO(p) (&__I915__(p)->info) #define INTEL_DEVID(p) (INTEL_INFO(p)->device_id) #define INTEL_REVID(p) (__I915__(p)->dev->pdev->revision)