[Intel-gfx] [PATCH v7 3/8] i915/gvt: use DEFINE_DYNAMIC_DEBUG_CATEGORIES to create "gvt:core:" etc categories
jim.cromie at gmail.com
jim.cromie at gmail.com
Tue Sep 7 17:26:46 UTC 2021
On Tue, Sep 7, 2021 at 9:14 AM Tvrtko Ursulin
<tvrtko.ursulin at linux.intel.com> wrote:
>
>
> On 06/09/2021 18:41, jim.cromie at gmail.com wrote:
> > On Mon, Sep 6, 2021 at 6:26 AM Tvrtko Ursulin
> > <tvrtko.ursulin at linux.intel.com <mailto:tvrtko.ursulin at linux.intel.com>>
> > wrote:
> > >
> > >
> > > On 03/09/2021 20:22, jim.cromie at gmail.com
> > <mailto:jim.cromie at gmail.com> wrote:
> > > > On Fri, Sep 3, 2021 at 5:07 AM Tvrtko Ursulin
> > > > <tvrtko.ursulin at linux.intel.com
> > <mailto:tvrtko.ursulin at 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 at chromium.org <mailto:seanpaul at 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
More information about the amd-gfx
mailing list