| Message ID | 20210831202133.2165222-4-jim.cromie@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | use DYNAMIC_DEBUG to implement DRM.debug | expand |
On 31/08/2021 21:21, Jim Cromie wrote: > The gvt component of this driver has ~120 pr_debugs, in 9 categories > quite similar to those in DRM. Following the interface model of > drm.debug, add a parameter to map bits to these categorizations. > > DEFINE_DYNAMIC_DEBUG_CATEGORIES(debug_gvt, __gvt_debug, > "dyndbg bitmap desc", > { "gvt:cmd: ", "command processing" }, > { "gvt:core: ", "core help" }, > { "gvt:dpy: ", "display help" }, > { "gvt:el: ", "help" }, > { "gvt:irq: ", "help" }, > { "gvt:mm: ", "help" }, > { "gvt:mmio: ", "help" }, > { "gvt:render: ", "help" }, > { "gvt:sched: " "help" }); > > The actual patch has a few details different, cmd_help() macro emits > the initialization construct. > > if CONFIG_DRM_USE_DYNAMIC_DEBUG, then -DDYNAMIC_DEBUG_MODULE is added > cflags, by gvt/Makefile. > > Signed-off-by: Jim Cromie <jim.cromie@gmail.com> > --- > v5: > . static decl of vector of bit->class descriptors - Emil.V > . relocate gvt-makefile chunk from elsewhere > v7: > . move ccflags addition up to i915/Makefile from i915/gvt > --- > drivers/gpu/drm/i915/Makefile | 4 ++++ > drivers/gpu/drm/i915/i915_params.c | 35 ++++++++++++++++++++++++++++++ Can this work if put under gvt/ or at least intel_gvt.h|c? > 2 files changed, 39 insertions(+) > > diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile > index 4f22cac1c49b..5a4e371a3ec2 100644 > --- a/drivers/gpu/drm/i915/Makefile > +++ b/drivers/gpu/drm/i915/Makefile > @@ -30,6 +30,10 @@ CFLAGS_display/intel_fbdev.o = $(call cc-disable-warning, override-init) > > subdir-ccflags-y += -I$(srctree)/$(src) > > +#ifdef CONFIG_DRM_USE_DYNAMIC_DEBUG > +ccflags-y += -DDYNAMIC_DEBUG_MODULE > +#endif Ignores whether CONFIG_DRM_I915_GVT is enabled or not? > + > # Please keep these build lists sorted! > > # core driver code > diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c > index e07f4cfea63a..e645e149485e 100644 > --- a/drivers/gpu/drm/i915/i915_params.c > +++ b/drivers/gpu/drm/i915/i915_params.c > @@ -265,3 +265,38 @@ void i915_params_free(struct i915_params *params) > I915_PARAMS_FOR_EACH(FREE); > #undef FREE > } > + > +#ifdef CONFIG_DRM_USE_DYNAMIC_DEBUG > +/* todo: needs DYNAMIC_DEBUG_MODULE in some cases */ > + > +unsigned long __gvt_debug; > +EXPORT_SYMBOL(__gvt_debug); > + > +#define _help(key) "\t \"" key "\"\t: help for " key "\n" > + > +#define I915_GVT_CATEGORIES(name) \ > + " Enable debug output via /sys/module/i915/parameters/" #name \ > + ", where each bit enables a debug category.\n" \ > + _help("gvt:cmd:") \ > + _help("gvt:core:") \ > + _help("gvt:dpy:") \ > + _help("gvt:el:") \ > + _help("gvt:irq:") \ > + _help("gvt:mm:") \ > + _help("gvt:mmio:") \ > + _help("gvt:render:") \ > + _help("gvt:sched:") > + > +DEFINE_DYNAMIC_DEBUG_CATEGORIES(debug_gvt, __gvt_debug, > + I915_GVT_CATEGORIES(debug_gvt), > + _DD_cat_("gvt:cmd:"), > + _DD_cat_("gvt:core:"), > + _DD_cat_("gvt:dpy:"), > + _DD_cat_("gvt:el:"), > + _DD_cat_("gvt:irq:"), > + _DD_cat_("gvt:mm:"), > + _DD_cat_("gvt:mmio:"), > + _DD_cat_("gvt:render:"), > + _DD_cat_("gvt:sched:")); > + > +#endif So just the foundation - no actual use sites I mean? How would these be used from the code? Regards, Tvrtko
On Fri, Sep 3, 2021 at 5:07 AM Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: > > > On 31/08/2021 21:21, Jim Cromie wrote: > > The gvt component of this driver has ~120 pr_debugs, in 9 categories > > quite similar to those in DRM. Following the interface model of > > drm.debug, add a parameter to map bits to these categorizations. > > > > DEFINE_DYNAMIC_DEBUG_CATEGORIES(debug_gvt, __gvt_debug, > > "dyndbg bitmap desc", > > { "gvt:cmd: ", "command processing" }, > > { "gvt:core: ", "core help" }, > > { "gvt:dpy: ", "display help" }, > > { "gvt:el: ", "help" }, > > { "gvt:irq: ", "help" }, > > { "gvt:mm: ", "help" }, > > { "gvt:mmio: ", "help" }, > > { "gvt:render: ", "help" }, > > { "gvt:sched: " "help" }); > > BTW, Ive dropped the help field, its already handled, dont need to clutter. > > The actual patch has a few details different, cmd_help() macro emits > > the initialization construct. > > > > if CONFIG_DRM_USE_DYNAMIC_DEBUG, then -DDYNAMIC_DEBUG_MODULE is added > > cflags, by gvt/Makefile. > > > > Signed-off-by: Jim Cromie <jim.cromie@gmail.com> > > --- > > v5: > > . static decl of vector of bit->class descriptors - Emil.V > > . relocate gvt-makefile chunk from elsewhere > > v7: > > . move ccflags addition up to i915/Makefile from i915/gvt > > --- > > drivers/gpu/drm/i915/Makefile | 4 ++++ > > drivers/gpu/drm/i915/i915_params.c | 35 ++++++++++++++++++++++++++++++ > > Can this work if put under gvt/ or at least intel_gvt.h|c? > I thought it belonged here more, at least according to the name of the config.var CONFIG_DRM_USE_DYNAMIC_DEBUG. I suppose its not a great name, its narrow purpose is to swap drm-debug api to use dyndbg. drm-evrything already "uses" dyndbg if CONFIG_DYNAMIC_DEBUG=y, those gvt/pr_debugs in particular. Theres also CONFIG_DYNAMIC_DEBUG_CORE=y, which drm basically ignores currently. So with the name CONFIG_DRM_USE_DYNAMIC_DEBUG it seemed proper to arrange for that to be true on DD-CORE=y builds, by adding -DDYNAMIC_DEBUG_MODULE Does that make some sense ? How to best resolve the frictions ? new CONFIG names ? > > 2 files changed, 39 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile > > index 4f22cac1c49b..5a4e371a3ec2 100644 > > --- a/drivers/gpu/drm/i915/Makefile > > +++ b/drivers/gpu/drm/i915/Makefile > > @@ -30,6 +30,10 @@ CFLAGS_display/intel_fbdev.o = $(call cc-disable-warning, override-init) > > > > subdir-ccflags-y += -I$(srctree)/$(src) > > > > +#ifdef CONFIG_DRM_USE_DYNAMIC_DEBUG > > +ccflags-y += -DDYNAMIC_DEBUG_MODULE > > +#endif > > Ignores whether CONFIG_DRM_I915_GVT is enabled or not? > not intentionally. I think theres 2 things youre noting: 1 - make frag into gvt/Makefile I had it there earlier, not sure why I moved it up. maybe some confusion on proper scope of the flag. 2 - move new declaration code in i915-param.c inside the gvt ifdef Im good with that. I'll probably copy the ifdef wrapper down rather than move the decl up. ie: #if __and(IS_ENABLED(CONFIG_DRM_I915_GVT), IS_ENABLED(CONFIG_DRM_USE_DYNAMIC_DEBUG)) unsigned long __gvt_debug; EXPORT_SYMBOL(__gvt_debug); > > + > > # Please keep these build lists sorted! > > > > # core driver code > > diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c > > index e07f4cfea63a..e645e149485e 100644 > > --- a/drivers/gpu/drm/i915/i915_params.c > > +++ b/drivers/gpu/drm/i915/i915_params.c > > @@ -265,3 +265,38 @@ void i915_params_free(struct i915_params *params) > > I915_PARAMS_FOR_EACH(FREE); > > #undef FREE > > } > > + > > +#ifdef CONFIG_DRM_USE_DYNAMIC_DEBUG > > +/* todo: needs DYNAMIC_DEBUG_MODULE in some cases */ > > + > > +unsigned long __gvt_debug; > > +EXPORT_SYMBOL(__gvt_debug); > > + > > +#define _help(key) "\t \"" key "\"\t: help for " key "\n" > > + > > +#define I915_GVT_CATEGORIES(name) \ > > + " Enable debug output via /sys/module/i915/parameters/" #name \ > > + ", where each bit enables a debug category.\n" \ > > + _help("gvt:cmd:") \ > > + _help("gvt:core:") \ > > + _help("gvt:dpy:") \ > > + _help("gvt:el:") \ > > + _help("gvt:irq:") \ > > + _help("gvt:mm:") \ > > + _help("gvt:mmio:") \ > > + _help("gvt:render:") \ > > + _help("gvt:sched:") > > + > > +DEFINE_DYNAMIC_DEBUG_CATEGORIES(debug_gvt, __gvt_debug, > > + I915_GVT_CATEGORIES(debug_gvt), > > + _DD_cat_("gvt:cmd:"), > > + _DD_cat_("gvt:core:"), > > + _DD_cat_("gvt:dpy:"), > > + _DD_cat_("gvt:el:"), > > + _DD_cat_("gvt:irq:"), > > + _DD_cat_("gvt:mm:"), > > + _DD_cat_("gvt:mmio:"), > > + _DD_cat_("gvt:render:"), > > + _DD_cat_("gvt:sched:")); > > + > > +#endif > > So just the foundation - no actual use sites I mean? How would these be > used from the code? > there are 120 pr_debug "users" :-) no users per se, but anyone using drm.debug /sys/module/drm/parameters/debug might use this too. its a bit easier than composing queries for >/proc/dyamic_debug/control > Regards, > > Tvrtko
On 03/09/2021 20:22, jim.cromie@gmail.com wrote: > On Fri, Sep 3, 2021 at 5:07 AM Tvrtko Ursulin > <tvrtko.ursulin@linux.intel.com> wrote: >> >> >> On 31/08/2021 21:21, Jim Cromie wrote: >>> The gvt component of this driver has ~120 pr_debugs, in 9 categories >>> quite similar to those in DRM. Following the interface model of >>> drm.debug, add a parameter to map bits to these categorizations. >>> >>> DEFINE_DYNAMIC_DEBUG_CATEGORIES(debug_gvt, __gvt_debug, >>> "dyndbg bitmap desc", >>> { "gvt:cmd: ", "command processing" }, >>> { "gvt:core: ", "core help" }, >>> { "gvt:dpy: ", "display help" }, >>> { "gvt:el: ", "help" }, >>> { "gvt:irq: ", "help" }, >>> { "gvt:mm: ", "help" }, >>> { "gvt:mmio: ", "help" }, >>> { "gvt:render: ", "help" }, >>> { "gvt:sched: " "help" }); >>> > > BTW, Ive dropped the help field, its already handled, dont need to clutter. > > >>> The actual patch has a few details different, cmd_help() macro emits >>> the initialization construct. >>> >>> if CONFIG_DRM_USE_DYNAMIC_DEBUG, then -DDYNAMIC_DEBUG_MODULE is added >>> cflags, by gvt/Makefile. >>> >>> Signed-off-by: Jim Cromie <jim.cromie@gmail.com> >>> --- >>> v5: >>> . static decl of vector of bit->class descriptors - Emil.V >>> . relocate gvt-makefile chunk from elsewhere >>> v7: >>> . move ccflags addition up to i915/Makefile from i915/gvt >>> --- >>> drivers/gpu/drm/i915/Makefile | 4 ++++ >>> drivers/gpu/drm/i915/i915_params.c | 35 ++++++++++++++++++++++++++++++ >> >> Can this work if put under gvt/ or at least intel_gvt.h|c? >> > > I thought it belonged here more, at least according to the name of the > config.var Hmm bear with me please - the categories this patch creates are intended to be used explicitly from the GVT "sub-module", or they somehow even get automatically used with no further intervention to callers required? > CONFIG_DRM_USE_DYNAMIC_DEBUG. > > I suppose its not a great name, its narrow purpose is to swap > drm-debug api to use dyndbg. drm-evrything already "uses" > dyndbg if CONFIG_DYNAMIC_DEBUG=y, those gvt/pr_debugs in particular. > > Theres also CONFIG_DYNAMIC_DEBUG_CORE=y, > which drm basically ignores currently. > > So with the name CONFIG_DRM_USE_DYNAMIC_DEBUG > it seemed proper to arrange for that to be true on DD-CORE=y builds, > by adding -DDYNAMIC_DEBUG_MODULE > > Does that make some sense ? > How to best resolve the frictions ? > new CONFIG names ? > >>> 2 files changed, 39 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile >>> index 4f22cac1c49b..5a4e371a3ec2 100644 >>> --- a/drivers/gpu/drm/i915/Makefile >>> +++ b/drivers/gpu/drm/i915/Makefile >>> @@ -30,6 +30,10 @@ CFLAGS_display/intel_fbdev.o = $(call cc-disable-warning, override-init) >>> >>> subdir-ccflags-y += -I$(srctree)/$(src) >>> >>> +#ifdef CONFIG_DRM_USE_DYNAMIC_DEBUG >>> +ccflags-y += -DDYNAMIC_DEBUG_MODULE >>> +#endif >> >> Ignores whether CONFIG_DRM_I915_GVT is enabled or not? >> > > not intentionally. > I think theres 2 things youre noting: > > 1 - make frag into gvt/Makefile > I had it there earlier, not sure why I moved it up. > maybe some confusion on proper scope of the flag. > > > 2 - move new declaration code in i915-param.c inside the gvt ifdef > > Im good with that. > I'll probably copy the ifdef wrapper down rather than move the decl up. > ie: > > #if __and(IS_ENABLED(CONFIG_DRM_I915_GVT), > IS_ENABLED(CONFIG_DRM_USE_DYNAMIC_DEBUG)) > > unsigned long __gvt_debug; > EXPORT_SYMBOL(__gvt_debug); > > >>> + >>> # Please keep these build lists sorted! >>> >>> # core driver code >>> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c >>> index e07f4cfea63a..e645e149485e 100644 >>> --- a/drivers/gpu/drm/i915/i915_params.c >>> +++ b/drivers/gpu/drm/i915/i915_params.c >>> @@ -265,3 +265,38 @@ void i915_params_free(struct i915_params *params) >>> I915_PARAMS_FOR_EACH(FREE); >>> #undef FREE >>> } >>> + >>> +#ifdef CONFIG_DRM_USE_DYNAMIC_DEBUG >>> +/* todo: needs DYNAMIC_DEBUG_MODULE in some cases */ >>> + >>> +unsigned long __gvt_debug; >>> +EXPORT_SYMBOL(__gvt_debug); >>> + >>> +#define _help(key) "\t \"" key "\"\t: help for " key "\n" >>> + >>> +#define I915_GVT_CATEGORIES(name) \ >>> + " Enable debug output via /sys/module/i915/parameters/" #name \ >>> + ", where each bit enables a debug category.\n" \ >>> + _help("gvt:cmd:") \ >>> + _help("gvt:core:") \ >>> + _help("gvt:dpy:") \ >>> + _help("gvt:el:") \ >>> + _help("gvt:irq:") \ >>> + _help("gvt:mm:") \ >>> + _help("gvt:mmio:") \ >>> + _help("gvt:render:") \ >>> + _help("gvt:sched:") >>> + >>> +DEFINE_DYNAMIC_DEBUG_CATEGORIES(debug_gvt, __gvt_debug, >>> + I915_GVT_CATEGORIES(debug_gvt), >>> + _DD_cat_("gvt:cmd:"), >>> + _DD_cat_("gvt:core:"), >>> + _DD_cat_("gvt:dpy:"), >>> + _DD_cat_("gvt:el:"), >>> + _DD_cat_("gvt:irq:"), >>> + _DD_cat_("gvt:mm:"), >>> + _DD_cat_("gvt:mmio:"), >>> + _DD_cat_("gvt:render:"), >>> + _DD_cat_("gvt:sched:")); >>> + >>> +#endif >> >> So just the foundation - no actual use sites I mean? How would these be >> used from the code? >> > > there are 120 pr_debug "users" :-) > > no users per se, but anyone using drm.debug > /sys/module/drm/parameters/debug > might use this too. > its a bit easier than composing queries for >/proc/dyamic_debug/control Same as my previous question, perhaps I am not up to speed with this yet.. Even if pr_debug is used inside GVT - are the categories and debug_gvt global as of this patch (or series)? Regards, Tvrtko
On Mon, Sep 6, 2021 at 6:26 AM Tvrtko Ursulin < tvrtko.ursulin@linux.intel.com> wrote: > > > On 03/09/2021 20:22, jim.cromie@gmail.com wrote: > > On Fri, Sep 3, 2021 at 5:07 AM Tvrtko Ursulin > > <tvrtko.ursulin@linux.intel.com> wrote: > >> > >> > >> On 31/08/2021 21:21, Jim Cromie wrote: > >>> The gvt component of this driver has ~120 pr_debugs, in 9 categories > >>> quite similar to those in DRM. Following the interface model of > >>> drm.debug, add a parameter to map bits to these categorizations. > >>> > >>> DEFINE_DYNAMIC_DEBUG_CATEGORIES(debug_gvt, __gvt_debug, > >>> "dyndbg bitmap desc", > >>> { "gvt:cmd: ", "command processing" }, > >>> v7: > >>> . move ccflags addition up to i915/Makefile from i915/gvt > >>> --- > >>> drivers/gpu/drm/i915/Makefile | 4 ++++ > >>> drivers/gpu/drm/i915/i915_params.c | 35 ++++++++++++++++++++++++++++++ > >> > >> Can this work if put under gvt/ or at least intel_gvt.h|c? I tried this. I moved the code block into gvt/debug.c (new file) added it to Makefile GVT_SOURCES dunno why it wont make. frustratig basic err, Im not seeing. It does seem proper placement, will resolve... > >> > > > > I thought it belonged here more, at least according to the name of the > > config.var > > Hmm bear with me please - the categories this patch creates are intended > to be used explicitly from the GVT "sub-module", or they somehow even > get automatically used with no further intervention to callers required? > 2009 - v5.9.0 the only users were admins reading/echoing /proc/dynamic_debug/control presumably cuz they wanted more info in the logs, episodically. v5.9.0 exported dynamic_debug_exec_queries for in-kernel use, reusing the stringy: echo $query_command > control idiom. My intention was to let in-kernel users roll their own drm.debug type interface, or whatever else they needed. nobodys using it yet. patch 1/8 implements that drm.debug interface. 5/8 is the primary use case 3/8 (this patch) & 4/8 are patches of opportunity, test cases, proof of function/utility. its value as such is easier control of those pr-debugs than given by echo > control Sean Paul seanpaul@chromium.org worked up a patchset to do runtime steering of drm-debug stream, in particular watching for drm:atomic:fail: type activity (a subcategory which doesnt exist yet). 5/8 conflicts with his patchset, I have an rfc approach to that, so his concerns are mine too. note: if this patchset goes in, we dont *really* need the export anymore, since the main use case is covered. We could un-export, and re-add later if its needed for a different use case. Further, it seems likely that the callbacks (refactored) would be a better basis for new in-kernel users. If not that, then full exposure of struct ddebug_query to in-kernel use not quite sure how we got 2 chunks, but theres 1 more q below. On Mon, Sep 6, 2021 at 6:26 AM Tvrtko Ursulin < tvrtko.ursulin@linux.intel.com> wrote: > > On 03/09/2021 20:22, jim.cromie@gmail.com wrote: > > On Fri, Sep 3, 2021 at 5:07 AM Tvrtko Ursulin > > <tvrtko.ursulin@linux.intel.com> wrote: > >> > >> > >> On 31/08/2021 21:21, Jim Cromie wrote: > >>> The gvt component of this driver has ~120 pr_debugs, in 9 categories > >>> quite similar to those in DRM. Following the interface model of > >>> drm.debug, add a parameter to map bits to these categorizations. > >>> > >>> DEFINE_DYNAMIC_DEBUG_CATEGORIES(debug_gvt, __gvt_debug, > >>> "dyndbg bitmap desc", > >>> { "gvt:cmd: ", "command processing" }, > >>> { "gvt:core: ", "core help" }, > >>> { "gvt:dpy: ", "display help" }, > >>> { "gvt:el: ", "help" }, > >>> { "gvt:irq: ", "help" }, > >>> { "gvt:mm: ", "help" }, > >>> { "gvt:mmio: ", "help" }, > >>> { "gvt:render: ", "help" }, > >>> { "gvt:sched: " "help" }); > >>> > > > > BTW, Ive dropped the help field, its already handled, dont need to > clutter. > > > > > >>> The actual patch has a few details different, cmd_help() macro emits > >>> the initialization construct. > >>> > >>> if CONFIG_DRM_USE_DYNAMIC_DEBUG, then -DDYNAMIC_DEBUG_MODULE is added > >>> cflags, by gvt/Makefile. > >>> > >>> Signed-off-by: Jim Cromie <jim.cromie@gmail.com> > >>> --- > >>> v5: > >>> . static decl of vector of bit->class descriptors - Emil.V > >>> . relocate gvt-makefile chunk from elsewhere > >>> v7: > >>> . move ccflags addition up to i915/Makefile from i915/gvt > >>> --- > >>> drivers/gpu/drm/i915/Makefile | 4 ++++ > >>> drivers/gpu/drm/i915/i915_params.c | 35 > ++++++++++++++++++++++++++++++ > >> > >> Can this work if put under gvt/ or at least intel_gvt.h|c? > >> > > > > I thought it belonged here more, at least according to the name of the > > config.var > > Hmm bear with me please - the categories this patch creates are intended > to be used explicitly from the GVT "sub-module", or they somehow even > get automatically used with no further intervention to callers required? > > > CONFIG_DRM_USE_DYNAMIC_DEBUG. > > > > I suppose its not a great name, its narrow purpose is to swap > > drm-debug api to use dyndbg. drm-evrything already "uses" > > dyndbg if CONFIG_DYNAMIC_DEBUG=y, those gvt/pr_debugs in particular. > > > > Theres also CONFIG_DYNAMIC_DEBUG_CORE=y, > > which drm basically ignores currently. > > > > So with the name CONFIG_DRM_USE_DYNAMIC_DEBUG > > it seemed proper to arrange for that to be true on DD-CORE=y builds, > > by adding -DDYNAMIC_DEBUG_MODULE > > > > Does that make some sense ? > > How to best resolve the frictions ? > > new CONFIG names ? > > > >>> 2 files changed, 39 insertions(+) > >>> > >>> diff --git a/drivers/gpu/drm/i915/Makefile > b/drivers/gpu/drm/i915/Makefile > >>> index 4f22cac1c49b..5a4e371a3ec2 100644 > >>> --- a/drivers/gpu/drm/i915/Makefile > >>> +++ b/drivers/gpu/drm/i915/Makefile > >>> @@ -30,6 +30,10 @@ CFLAGS_display/intel_fbdev.o = $(call > cc-disable-warning, override-init) > >>> > >>> subdir-ccflags-y += -I$(srctree)/$(src) > >>> > >>> +#ifdef CONFIG_DRM_USE_DYNAMIC_DEBUG > >>> +ccflags-y += -DDYNAMIC_DEBUG_MODULE > >>> +#endif > >> > >> Ignores whether CONFIG_DRM_I915_GVT is enabled or not? > >> > > > > not intentionally. > > I think theres 2 things youre noting: > > > > 1 - make frag into gvt/Makefile > > I had it there earlier, not sure why I moved it up. > > maybe some confusion on proper scope of the flag. > > > > > > 2 - move new declaration code in i915-param.c inside the gvt ifdef > > > > Im good with that. > > I'll probably copy the ifdef wrapper down rather than move the decl up. > > ie: > > > > #if __and(IS_ENABLED(CONFIG_DRM_I915_GVT), > > IS_ENABLED(CONFIG_DRM_USE_DYNAMIC_DEBUG)) > > > > unsigned long __gvt_debug; > > EXPORT_SYMBOL(__gvt_debug); > > > > > >>> + > >>> # Please keep these build lists sorted! > >>> > >>> # core driver code > >>> diff --git a/drivers/gpu/drm/i915/i915_params.c > b/drivers/gpu/drm/i915/i915_params.c > >>> index e07f4cfea63a..e645e149485e 100644 > >>> --- a/drivers/gpu/drm/i915/i915_params.c > >>> +++ b/drivers/gpu/drm/i915/i915_params.c > >>> @@ -265,3 +265,38 @@ void i915_params_free(struct i915_params *params) > >>> + _DD_cat_("gvt:mmio:"), > >>> + _DD_cat_("gvt:render:"), > >>> + _DD_cat_("gvt:sched:")); > >>> + > >>> +#endif > >> > >> So just the foundation - no actual use sites I mean? How would these be > >> used from the code? > >> > > > > there are 120 pr_debug "users" :-) > > > > no users per se, but anyone using drm.debug > > /sys/module/drm/parameters/debug > > might use this too. > > its a bit easier than composing queries for >/proc/dyamic_debug/control > > Same as my previous question, perhaps I am not up to speed with this > yet.. Even if pr_debug is used inside GVT - are the categories and > debug_gvt global as of this patch (or series)? > > they are already global in the sense that if kernel is built with DYNAMIC_DEBUG, the admin can turn those pr_debugs on and off, and change their decorations in the log (mod,func.line). Nor are modules protected from each other; drm-core could use dd-exec-queries to enable/disable pr-debugs in i915 etc This patch just adds a gvt-debug knob like drm-debug. using the existing format prefixes to categorize them. Whether those prefixes should be bent towards consistency with the rest of drm-debug or adapted towards some gvt community need I couldnt say. Its no save-the-world feature, but its pretty cheap. Id expect the same users as those who play with drm.debug, for similar reasons. does this clarify ? > Regards, > > Tvrtko > thanks, Jim
On 06/09/2021 18:41, jim.cromie@gmail.com wrote: > On Mon, Sep 6, 2021 at 6:26 AM Tvrtko Ursulin > <tvrtko.ursulin@linux.intel.com <mailto:tvrtko.ursulin@linux.intel.com>> > wrote: > > > > > > On 03/09/2021 20:22, jim.cromie@gmail.com > <mailto:jim.cromie@gmail.com> wrote: > > > On Fri, Sep 3, 2021 at 5:07 AM Tvrtko Ursulin > > > <tvrtko.ursulin@linux.intel.com > <mailto:tvrtko.ursulin@linux.intel.com>> wrote: > > >> > > >> > > >> On 31/08/2021 21:21, Jim Cromie wrote: > > >>> The gvt component of this driver has ~120 pr_debugs, in 9 categories > > >>> quite similar to those in DRM. Following the interface model of > > >>> drm.debug, add a parameter to map bits to these categorizations. > > >>> > > >>> DEFINE_DYNAMIC_DEBUG_CATEGORIES(debug_gvt, __gvt_debug, > > >>> "dyndbg bitmap desc", > > >>> { "gvt:cmd: ", "command processing" }, > > > >>> v7: > > >>> . move ccflags addition up to i915/Makefile from i915/gvt > > >>> --- > > >>> drivers/gpu/drm/i915/Makefile | 4 ++++ > > >>> drivers/gpu/drm/i915/i915_params.c | 35 > ++++++++++++++++++++++++++++++ > > >> > > >> Can this work if put under gvt/ or at least intel_gvt.h|c? > > I tried this. > I moved the code block into gvt/debug.c (new file) > added it to Makefile GVT_SOURCES > dunno why it wont make. > frustratig basic err, Im not seeing. > It does seem proper placement, will resolve... > > > > >> > > > > > > I thought it belonged here more, at least according to the name of the > > > config.var > > > > Hmm bear with me please - the categories this patch creates are intended > > to be used explicitly from the GVT "sub-module", or they somehow even > > get automatically used with no further intervention to callers required? > > > > 2009 - v5.9.0 the only users were admins reading/echoing > /proc/dynamic_debug/control > presumably cuz they wanted more info in the logs, episodically. > v5.9.0 exported dynamic_debug_exec_queries for in-kernel use, > reusing the stringy: echo $query_command > control idiom. > My intention was to let in-kernel users roll their own drm.debug type > interface, > or whatever else they needed. nobodys using it yet. What is 2009 referring to? I am still not quite following. In case of the GVT categories you added, in order for them to be used, would you or not also need to use some new logging macros? > patch 1/8 implements that drm.debug interface. > 5/8 is the primary use case > 3/8 (this patch) & 4/8 are patches of opportunity, test cases, proof of > function/utility. > its value as such is easier control of those pr-debugs than given by > echo > control > > Sean Paul seanpaul@chromium.org <mailto:seanpaul@chromium.org> worked up > a patchset to do runtime steering of drm-debug stream, > in particular watching for drm:atomic:fail: type activity (a subcategory > which doesnt exist yet). > 5/8 conflicts with his patchset, I have an rfc approach to that, so his > concerns are mine too. What kind of runtime steering is that - would you happen to have a link? One idea we in the GEM team have mentioned a few time is the ability of making the debug log stream per DRM client. That means opening "something" (socket, fd, etc), enabling debug, which would only show debug logs belonging to one client. That can sometimes be useful (and more secure) than enabling a lot of debug for the system as a whole. But of course there isn't much overlap with your dyndbg work. So just mentioning this since the word "runtime steering" reminded me of it. <snip> > > unsigned long __gvt_debug; > > EXPORT_SYMBOL(__gvt_debug); > > > > > >>> + > >>> # Please keep these build lists sorted! > >>> > >>> # core driver code > >>> diff --git a/drivers/gpu/drm/i915/i915_params.c > b/drivers/gpu/drm/i915/i915_params.c > >>> index e07f4cfea63a..e645e149485e 100644 > >>> --- a/drivers/gpu/drm/i915/i915_params.c > >>> +++ b/drivers/gpu/drm/i915/i915_params.c > >>> @@ -265,3 +265,38 @@ void i915_params_free(struct i915_params > *params) > >>> + _DD_cat_("gvt:mmio:"), > >>> + _DD_cat_("gvt:render:"), > >>> + _DD_cat_("gvt:sched:")); > >>> + > >>> +#endif > >> > >> So just the foundation - no actual use sites I mean? How would > these be > >> used from the code? > >> > > > > there are 120 pr_debug "users" :-) > > > > no users per se, but anyone using drm.debug > > /sys/module/drm/parameters/debug > > might use this too. > > its a bit easier than composing queries for > >/proc/dyamic_debug/control > > Same as my previous question, perhaps I am not up to speed with this > yet.. Even if pr_debug is used inside GVT - are the categories and > debug_gvt global as of this patch (or series)? > > > they are already global in the sense that if kernel is built with > DYNAMIC_DEBUG, > the admin can turn those pr_debugs on and off, and change their > decorations in the log (mod,func.line). > Nor are modules protected from each other; drm-core could use > dd-exec-queries to enable/disable > pr-debugs in i915 etc > > This patch just adds a gvt-debug knob like drm-debug. using the existing > format prefixes to categorize them. > Whether those prefixes should be bent towards consistency with the rest > of drm-debug > or adapted towards some gvt community need I couldnt say. > > Its no save-the-world feature, but its pretty cheap. > > Id expect the same users as those who play with drm.debug, for similar > reasons. > > does this clarify ? Not yet I'm afraid. :) When you say "using the existing format prefixes", but it is not using __drm_debug but __gvt_debug (which isn't a modparam). So I am lost as to what is __gvt_debug for and how does it tie to existing DRM categories. Regards, Tvrtko
On Tue, Sep 7, 2021 at 9:14 AM Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: > > > On 06/09/2021 18:41, jim.cromie@gmail.com wrote: > > On Mon, Sep 6, 2021 at 6:26 AM Tvrtko Ursulin > > <tvrtko.ursulin@linux.intel.com <mailto:tvrtko.ursulin@linux.intel.com>> > > wrote: > > > > > > > > > On 03/09/2021 20:22, jim.cromie@gmail.com > > <mailto:jim.cromie@gmail.com> wrote: > > > > On Fri, Sep 3, 2021 at 5:07 AM Tvrtko Ursulin > > > > <tvrtko.ursulin@linux.intel.com > > <mailto:tvrtko.ursulin@linux.intel.com>> wrote: > > > >> > > > >> > > > >> On 31/08/2021 21:21, Jim Cromie wrote: > > > >>> The gvt component of this driver has ~120 pr_debugs, in 9 categories > > > >>> quite similar to those in DRM. Following the interface model of > > > >>> drm.debug, add a parameter to map bits to these categorizations. > > > >>> > > > >>> DEFINE_DYNAMIC_DEBUG_CATEGORIES(debug_gvt, __gvt_debug, > > > >>> "dyndbg bitmap desc", > > > >>> { "gvt:cmd: ", "command processing" }, > > > > > >>> v7: > > > >>> . move ccflags addition up to i915/Makefile from i915/gvt > > > >>> --- > > > >>> drivers/gpu/drm/i915/Makefile | 4 ++++ > > > >>> drivers/gpu/drm/i915/i915_params.c | 35 > > ++++++++++++++++++++++++++++++ > > > >> > > > >> Can this work if put under gvt/ or at least intel_gvt.h|c? > > > > I tried this. > > I moved the code block into gvt/debug.c (new file) > > added it to Makefile GVT_SOURCES > > dunno why it wont make. > > frustratig basic err, Im not seeing. > > It does seem proper placement, will resolve... > > > > > > > >> > > > > > > > > I thought it belonged here more, at least according to the name of the > > > > config.var > > > > > > Hmm bear with me please - the categories this patch creates are intended > > > to be used explicitly from the GVT "sub-module", or they somehow even > > > get automatically used with no further intervention to callers required? > > > > > > > 2009 - v5.9.0 the only users were admins reading/echoing > > /proc/dynamic_debug/control > > presumably cuz they wanted more info in the logs, episodically. > > v5.9.0 exported dynamic_debug_exec_queries for in-kernel use, > > reusing the stringy: echo $query_command > control idiom. > > My intention was to let in-kernel users roll their own drm.debug type > > interface, > > or whatever else they needed. nobodys using it yet. > > What is 2009 referring to? > > I am still not quite following. In case of the GVT categories you added, > in order for them to be used, would you or not also need to use some new > logging macros? > > > patch 1/8 implements that drm.debug interface. > > 5/8 is the primary use case > > 3/8 (this patch) & 4/8 are patches of opportunity, test cases, proof of > > function/utility. > > its value as such is easier control of those pr-debugs than given by > > echo > control > > > > Sean Paul seanpaul@chromium.org <mailto:seanpaul@chromium.org> worked up > > a patchset to do runtime steering of drm-debug stream, > > in particular watching for drm:atomic:fail: type activity (a subcategory > > which doesnt exist yet). > > 5/8 conflicts with his patchset, I have an rfc approach to that, so his > > concerns are mine too. > > What kind of runtime steering is that - would you happen to have a link? Sean's patches https://patchwork.freedesktop.org/series/78133/ what I think might work better https://lore.kernel.org/dri-devel/20210822222009.2035788-11-jim.cromie@gmail.com/ > One idea we in the GEM team have mentioned a few time is the ability of > making the debug log stream per DRM client. That means opening > "something" (socket, fd, etc), enabling debug, which would only show > debug logs belonging to one client. That can sometimes be useful (and > more secure) than enabling a lot of debug for the system as a whole. But > of course there isn't much overlap with your dyndbg work. So just > mentioning this since the word "runtime steering" reminded me of it. > my rfc patch above might help. it adds register_dyndbg_tracer ( selector_query, handler_callback) I think you could write a single handler to further select / steer the debug stream according to your pr_debug arguments. > > > > unsigned long __gvt_debug; > > > EXPORT_SYMBOL(__gvt_debug); > > > > > > > > >>> + > > >>> # Please keep these build lists sorted! > > >>> > > >>> # core driver code > > >>> diff --git a/drivers/gpu/drm/i915/i915_params.c > > b/drivers/gpu/drm/i915/i915_params.c > > >>> index e07f4cfea63a..e645e149485e 100644 > > >>> --- a/drivers/gpu/drm/i915/i915_params.c > > >>> +++ b/drivers/gpu/drm/i915/i915_params.c > > >>> @@ -265,3 +265,38 @@ void i915_params_free(struct i915_params > > *params) > > >>> + _DD_cat_("gvt:mmio:"), > > >>> + _DD_cat_("gvt:render:"), > > >>> + _DD_cat_("gvt:sched:")); > > >>> + > > >>> +#endif > > >> > > >> So just the foundation - no actual use sites I mean? How would > > these be > > >> used from the code? > > >> > > > > > > there are 120 pr_debug "users" :-) > > > > > > no users per se, but anyone using drm.debug > > > /sys/module/drm/parameters/debug > > > might use this too. > > > its a bit easier than composing queries for > > >/proc/dyamic_debug/control > > > > Same as my previous question, perhaps I am not up to speed with this > > yet.. Even if pr_debug is used inside GVT - are the categories and > > debug_gvt global as of this patch (or series)? > > > > > > they are already global in the sense that if kernel is built with > > DYNAMIC_DEBUG, > > the admin can turn those pr_debugs on and off, and change their > > decorations in the log (mod,func.line). > > Nor are modules protected from each other; drm-core could use > > dd-exec-queries to enable/disable > > pr-debugs in i915 etc > > > > This patch just adds a gvt-debug knob like drm-debug. using the existing > > format prefixes to categorize them. > > Whether those prefixes should be bent towards consistency with the rest > > of drm-debug > > or adapted towards some gvt community need I couldnt say. > > > > Its no save-the-world feature, but its pretty cheap. > > > > Id expect the same users as those who play with drm.debug, for similar > > reasons. > > > > does this clarify ? > > Not yet I'm afraid. :) heh - 2 blind dudes describing their side of the elephant ! When you say "using the existing format > prefixes", but it is not using __drm_debug but __gvt_debug (which isn't > a modparam). So I am lost as to what is __gvt_debug for and how does it > tie to existing DRM categories. > we have 2 kinds of "categories": 1- proper drm categories - one of 10 2- ad-hoc categories - these are systematized - using small set of format-prefixes i915 has 120 of these in GVT, with 9 different prefixes, touched in patches 2,3 i915 also has ~1500 uses of drm-debug API (with proper drm category enums) amdgpu also has lots of both kinds of debug; 13 kinds of prefixes. Ive probably created some confusion by stealing the "category" name, it could have been "class", but I thought we didnt need new vocabulary with subtle and ambiguous differences from the original term. Long term, maybe those ad-hoc prefixes can be aligned better with proper drm categories, but thats a separate discussion. But we can control them now, using a well oiled idiom, a drm.debug "style" bitmap. Its like drm.debug's little sisters, __gvt_debug & __debug_dc. not identical, but related. Anyway, patches 2,3,4 work the ad-hoc prefix categorizations so theyre controllable. 5/8 adapts DRM-debug itself - obsoleting enum category for most of drm-debug api this is where dyndbg's data table gets bigger - core & drivers! have many drm-debug uses, rivaling all builtin prdebugs in size. Then, we have a unified foundation on dyndbg, using glorified prefix strings for both formal DRM_DBG_CAT_* categories, and for similar existing uses. Then we could evolve / extend / bikeshed the categories (spelling, separator char '.' is nice too) Sean has already suggested "drm:atomic:fail:" sub-category. I agree - it is using the new stringy flexibility to significant expressive benefit. dyndbg makes new categories actionable. istm "*:fail:" is maybe a meta-sub-category (dont read that too closely;) maybe "*:warn:" "*:err:" ( what about warning, error ? here lies bikeshed madness !!) > Regards, > > Tvrtko thanks JIm
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index 4f22cac1c49b..5a4e371a3ec2 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -30,6 +30,10 @@ CFLAGS_display/intel_fbdev.o = $(call cc-disable-warning, override-init) subdir-ccflags-y += -I$(srctree)/$(src) +#ifdef CONFIG_DRM_USE_DYNAMIC_DEBUG +ccflags-y += -DDYNAMIC_DEBUG_MODULE +#endif + # Please keep these build lists sorted! # core driver code diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c index e07f4cfea63a..e645e149485e 100644 --- a/drivers/gpu/drm/i915/i915_params.c +++ b/drivers/gpu/drm/i915/i915_params.c @@ -265,3 +265,38 @@ void i915_params_free(struct i915_params *params) I915_PARAMS_FOR_EACH(FREE); #undef FREE } + +#ifdef CONFIG_DRM_USE_DYNAMIC_DEBUG +/* todo: needs DYNAMIC_DEBUG_MODULE in some cases */ + +unsigned long __gvt_debug; +EXPORT_SYMBOL(__gvt_debug); + +#define _help(key) "\t \"" key "\"\t: help for " key "\n" + +#define I915_GVT_CATEGORIES(name) \ + " Enable debug output via /sys/module/i915/parameters/" #name \ + ", where each bit enables a debug category.\n" \ + _help("gvt:cmd:") \ + _help("gvt:core:") \ + _help("gvt:dpy:") \ + _help("gvt:el:") \ + _help("gvt:irq:") \ + _help("gvt:mm:") \ + _help("gvt:mmio:") \ + _help("gvt:render:") \ + _help("gvt:sched:") + +DEFINE_DYNAMIC_DEBUG_CATEGORIES(debug_gvt, __gvt_debug, + I915_GVT_CATEGORIES(debug_gvt), + _DD_cat_("gvt:cmd:"), + _DD_cat_("gvt:core:"), + _DD_cat_("gvt:dpy:"), + _DD_cat_("gvt:el:"), + _DD_cat_("gvt:irq:"), + _DD_cat_("gvt:mm:"), + _DD_cat_("gvt:mmio:"), + _DD_cat_("gvt:render:"), + _DD_cat_("gvt:sched:")); + +#endif
The gvt component of this driver has ~120 pr_debugs, in 9 categories quite similar to those in DRM. Following the interface model of drm.debug, add a parameter to map bits to these categorizations. DEFINE_DYNAMIC_DEBUG_CATEGORIES(debug_gvt, __gvt_debug, "dyndbg bitmap desc", { "gvt:cmd: ", "command processing" }, { "gvt:core: ", "core help" }, { "gvt:dpy: ", "display help" }, { "gvt:el: ", "help" }, { "gvt:irq: ", "help" }, { "gvt:mm: ", "help" }, { "gvt:mmio: ", "help" }, { "gvt:render: ", "help" }, { "gvt:sched: " "help" }); The actual patch has a few details different, cmd_help() macro emits the initialization construct. if CONFIG_DRM_USE_DYNAMIC_DEBUG, then -DDYNAMIC_DEBUG_MODULE is added cflags, by gvt/Makefile. Signed-off-by: Jim Cromie <jim.cromie@gmail.com> --- v5: . static decl of vector of bit->class descriptors - Emil.V . relocate gvt-makefile chunk from elsewhere v7: . move ccflags addition up to i915/Makefile from i915/gvt --- drivers/gpu/drm/i915/Makefile | 4 ++++ drivers/gpu/drm/i915/i915_params.c | 35 ++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+)