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