LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: jim.cromie@gmail.com
To: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: Sean Paul <seanpaul@chromium.org>,
	Jason Baron <jbaron@akamai.com>,
	Daniel Vetter <daniel.vetter@ffwll.ch>,
	Greg KH <gregkh@linuxfoundation.org>,
	LKML <linux-kernel@vger.kernel.org>,
	dri-devel <dri-devel@lists.freedesktop.org>,
	amd-gfx mailing list <amd-gfx@lists.freedesktop.org>,
	intel-gvt-dev@lists.freedesktop.org,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>
Subject: Re: [Intel-gfx] [PATCH v7 3/8] i915/gvt: use DEFINE_DYNAMIC_DEBUG_CATEGORIES to create "gvt:core:" etc categories
Date: Tue, 7 Sep 2021 11:26:46 -0600	[thread overview]
Message-ID: <CAJfuBxxeG0-bijXvoExRyh6Zv5o8PSz42cWam6m0aVCUKPMZ+Q@mail.gmail.com> (raw)
In-Reply-To: <92c7639b-c8f4-cfa4-f9ca-82c0a06e0337@linux.intel.com>

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

  reply	other threads:[~2021-09-07 17:27 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-31 20:21 [PATCH v7 0/8] use DYNAMIC_DEBUG to implement DRM.debug Jim Cromie
2021-08-31 20:21 ` [PATCH v7 1/8] dyndbg: add DEFINE_DYNAMIC_DEBUG_CATEGORIES and callbacks Jim Cromie
2021-08-31 20:21 ` [PATCH v7 2/8] dyndbg: remove spaces in pr_debug "gvt: core:" etc prefixes Jim Cromie
2021-08-31 20:21 ` [PATCH v7 3/8] i915/gvt: use DEFINE_DYNAMIC_DEBUG_CATEGORIES to create "gvt:core:" etc categories Jim Cromie
2021-09-03 11:07   ` [Intel-gfx] " Tvrtko Ursulin
2021-09-03 19:22     ` jim.cromie
2021-09-06 12:26       ` Tvrtko Ursulin
     [not found]         ` <CAJfuBxymoFx79kQzGw_Gxv1vk7kVaTN-V0Hn694C6kT=kP7u2A@mail.gmail.com>
2021-09-07 15:14           ` Tvrtko Ursulin
2021-09-07 17:26             ` jim.cromie [this message]
2021-08-31 20:21 ` [PATCH v7 4/8] amdgpu: use DEFINE_DYNAMIC_DEBUG_CATEGORIES Jim Cromie
2021-08-31 20:21 ` [PATCH v7 5/8] drm_print: add choice to use dynamic debug in drm-debug Jim Cromie
2021-09-03 11:15   ` [Intel-gfx] " Tvrtko Ursulin
2021-09-03 21:57     ` jim.cromie
2021-09-06 10:20       ` Tvrtko Ursulin
2021-09-06 18:24         ` jim.cromie
2021-08-31 20:21 ` [PATCH v7 6/8] drm_print: instrument drm_debug_enabled Jim Cromie
2021-09-05 18:50   ` jim.cromie
2021-08-31 20:21 ` [PATCH v7 7/8] amdgpu_ucode: reduce number of pr_debug calls Jim Cromie
2021-08-31 20:21 ` [PATCH v7 8/8] nouveau: fold multiple DRM_DEBUG_DRIVERs together Jim Cromie

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAJfuBxxeG0-bijXvoExRyh6Zv5o8PSz42cWam6m0aVCUKPMZ+Q@mail.gmail.com \
    --to=jim.cromie@gmail.com \
    --cc=amd-gfx@lists.freedesktop.org \
    --cc=daniel.vetter@ffwll.ch \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=intel-gvt-dev@lists.freedesktop.org \
    --cc=jbaron@akamai.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=seanpaul@chromium.org \
    --cc=tvrtko.ursulin@linux.intel.com \
    --subject='Re: [Intel-gfx] [PATCH v7 3/8] i915/gvt: use DEFINE_DYNAMIC_DEBUG_CATEGORIES to create "gvt:core:" etc categories' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).