* [PATCH] drm/i915: Introduce i915_dbg macro
@ 2016-01-22 12:24 akash.goel
2016-01-22 12:43 ` ✗ Fi.CI.BAT: warning for " Patchwork
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: akash.goel @ 2016-01-22 12:24 UTC (permalink / raw)
To: intel-gfx; +Cc: Akash Goel
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)
#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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* ✗ Fi.CI.BAT: warning for drm/i915: Introduce i915_dbg macro
2016-01-22 12:24 [PATCH] drm/i915: Introduce i915_dbg macro akash.goel
@ 2016-01-22 12:43 ` Patchwork
2016-01-22 12:49 ` [PATCH] " Arun Siluvery
2016-01-25 18:17 ` Daniel Vetter
2 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2016-01-22 12:43 UTC (permalink / raw)
To: Akash Goel; +Cc: intel-gfx
== Summary ==
Built on 8fe9e785ae04fa7c37f7935cff12d62e38054b60 drm-intel-nightly: 2016y-01m-21d-11h-02m-42s UTC integration manifest
Test kms_pipe_crc_basic:
Subgroup suspend-read-crc-pipe-b:
pass -> DMESG-WARN (ivb-t430s)
bdw-nuci7 total:140 pass:131 dwarn:0 dfail:0 fail:0 skip:9
bdw-ultra total:143 pass:137 dwarn:0 dfail:0 fail:0 skip:6
bsw-nuc-2 total:143 pass:119 dwarn:0 dfail:0 fail:0 skip:24
byt-nuc total:143 pass:128 dwarn:0 dfail:0 fail:0 skip:15
hsw-brixbox total:143 pass:136 dwarn:0 dfail:0 fail:0 skip:7
hsw-gt2 total:143 pass:139 dwarn:0 dfail:0 fail:0 skip:4
ilk-hp8440p total:143 pass:104 dwarn:1 dfail:0 fail:0 skip:38
ivb-t430s total:143 pass:136 dwarn:1 dfail:0 fail:0 skip:6
skl-i5k-2 total:143 pass:134 dwarn:1 dfail:0 fail:0 skip:8
snb-dellxps total:143 pass:129 dwarn:0 dfail:0 fail:0 skip:14
snb-x220t total:143 pass:129 dwarn:0 dfail:0 fail:1 skip:13
Results at /archive/results/CI_IGT_test/Patchwork_1248/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/i915: Introduce i915_dbg macro
2016-01-22 12:24 [PATCH] drm/i915: Introduce i915_dbg macro akash.goel
2016-01-22 12:43 ` ✗ Fi.CI.BAT: warning for " Patchwork
@ 2016-01-22 12:49 ` Arun Siluvery
2016-01-22 13:14 ` Chris Wilson
2016-01-25 18:17 ` Daniel Vetter
2 siblings, 1 reply; 10+ messages in thread
From: Arun Siluvery @ 2016-01-22 12:49 UTC (permalink / raw)
To: akash.goel, intel-gfx
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)
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/i915: Introduce i915_dbg macro
2016-01-22 12:49 ` [PATCH] " Arun Siluvery
@ 2016-01-22 13:14 ` Chris Wilson
0 siblings, 0 replies; 10+ messages in thread
From: Chris Wilson @ 2016-01-22 13:14 UTC (permalink / raw)
To: Arun Siluvery; +Cc: akash.goel, intel-gfx
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
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/i915: Introduce i915_dbg macro
2016-01-22 12:24 [PATCH] drm/i915: Introduce i915_dbg macro akash.goel
2016-01-22 12:43 ` ✗ Fi.CI.BAT: warning for " Patchwork
2016-01-22 12:49 ` [PATCH] " Arun Siluvery
@ 2016-01-25 18:17 ` Daniel Vetter
2016-01-25 18:57 ` Dave Gordon
2 siblings, 1 reply; 10+ messages in thread
From: Daniel Vetter @ 2016-01-25 18:17 UTC (permalink / raw)
To: akash.goel; +Cc: intel-gfx
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
--
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] 10+ messages in thread
* Re: [PATCH] drm/i915: Introduce i915_dbg macro
2016-01-25 18:17 ` Daniel Vetter
@ 2016-01-25 18:57 ` Dave Gordon
2016-01-26 9:44 ` Joonas Lahtinen
0 siblings, 1 reply; 10+ messages in thread
From: Dave Gordon @ 2016-01-25 18:57 UTC (permalink / raw)
To: akash.goel; +Cc: 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.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/i915: Introduce i915_dbg macro
2016-01-25 18:57 ` Dave Gordon
@ 2016-01-26 9:44 ` Joonas Lahtinen
2016-01-27 17:32 ` Dave Gordon
0 siblings, 1 reply; 10+ messages in thread
From: Joonas Lahtinen @ 2016-01-26 9:44 UTC (permalink / raw)
To: Dave Gordon, akash.goel, Daniel Vetter; +Cc: intel-gfx
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
--
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] 10+ messages in thread
* Re: [PATCH] drm/i915: Introduce i915_dbg macro
2016-01-26 9:44 ` Joonas Lahtinen
@ 2016-01-27 17:32 ` Dave Gordon
2016-01-27 17:50 ` Daniel Vetter
2016-01-28 8:42 ` Joonas Lahtinen
0 siblings, 2 replies; 10+ messages in thread
From: Dave Gordon @ 2016-01-27 17:32 UTC (permalink / raw)
To: Joonas Lahtinen, Goel, Akash, Daniel Vetter; +Cc: 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.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/i915: Introduce i915_dbg macro
2016-01-27 17:32 ` Dave Gordon
@ 2016-01-27 17:50 ` Daniel Vetter
2016-01-28 8:42 ` Joonas Lahtinen
1 sibling, 0 replies; 10+ messages in thread
From: Daniel Vetter @ 2016-01-27 17:50 UTC (permalink / raw)
To: Dave Gordon; +Cc: Goel, Akash, intel-gfx
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/i915: Introduce i915_dbg macro
2016-01-27 17:32 ` Dave Gordon
2016-01-27 17:50 ` Daniel Vetter
@ 2016-01-28 8:42 ` Joonas Lahtinen
1 sibling, 0 replies; 10+ messages in thread
From: Joonas Lahtinen @ 2016-01-28 8:42 UTC (permalink / raw)
To: Dave Gordon; +Cc: Goel, Akash, intel-gfx
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.
--
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] 10+ messages in thread
end of thread, other threads:[~2016-01-28 8:41 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-01-22 12:24 [PATCH] drm/i915: Introduce i915_dbg macro akash.goel
2016-01-22 12:43 ` ✗ Fi.CI.BAT: warning for " Patchwork
2016-01-22 12:49 ` [PATCH] " Arun Siluvery
2016-01-22 13:14 ` Chris Wilson
2016-01-25 18:17 ` Daniel Vetter
2016-01-25 18:57 ` Dave Gordon
2016-01-26 9:44 ` Joonas Lahtinen
2016-01-27 17:32 ` Dave Gordon
2016-01-27 17:50 ` Daniel Vetter
2016-01-28 8:42 ` Joonas Lahtinen
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.