[PATCH v3 0/7] Venus dynamic debug

[PATCH v3 0/7] Venus dynamic debug

[Stanimir Varbanov]

Hello,

Here is the third version of dynamic debug improvements in Venus
driver.  As has been suggested on previous version by Joe [1] I've
made the relevant changes in dynamic debug core to handle leveling
as more generic way and not open-code/workaround it in the driver.

About changes:
 - added change in the dynamic_debug and in documentation
 - added respective pr_debug_level and dev_dbg_level

regards,
Stan

[1] https://lkml.org/lkml/2020/5/21/668

Stanimir Varbanov (7):
  Documentation: dynamic-debug: Add description of level bitmask
  dynamic_debug: Group debug messages by level bitmask
  dev_printk: Add dev_dbg_level macro over dynamic one
  printk: Add pr_debug_level macro over dynamic one
  venus: Add debugfs interface to set firmware log level
  venus: Make debug infrastructure more flexible
  venus: Add a debugfs file for SSR trigger

 .../admin-guide/dynamic-debug-howto.rst       | 10 +++
 drivers/media/platform/qcom/venus/Makefile    |  2 +-
 drivers/media/platform/qcom/venus/core.c      |  5 ++
 drivers/media/platform/qcom/venus/core.h      |  8 +++
 drivers/media/platform/qcom/venus/dbgfs.c     | 57 +++++++++++++++++
 drivers/media/platform/qcom/venus/dbgfs.h     | 12 ++++
 drivers/media/platform/qcom/venus/helpers.c   |  2 +-
 drivers/media/platform/qcom/venus/hfi_msgs.c  | 30 ++++-----
 drivers/media/platform/qcom/venus/hfi_venus.c | 27 ++++++--
 .../media/platform/qcom/venus/pm_helpers.c    |  3 +-
 drivers/media/platform/qcom/venus/vdec.c      | 63 +++++++++++++++++--
 drivers/media/platform/qcom/venus/venc.c      |  4 ++
 fs/btrfs/ctree.h                              | 12 ++--
 include/linux/acpi.h                          |  3 +-
 include/linux/dev_printk.h                    | 12 +++-
 include/linux/dynamic_debug.h                 | 55 +++++++++++-----
 include/linux/net.h                           |  3 +-
 include/linux/printk.h                        |  9 ++-
 lib/dynamic_debug.c                           | 30 +++++++++
 19 files changed, 289 insertions(+), 58 deletions(-)
 create mode 100644 drivers/media/platform/qcom/venus/dbgfs.c
 create mode 100644 drivers/media/platform/qcom/venus/dbgfs.h

-- 
2.17.1

[PATCH v3 1/7] Documentation: dynamic-debug: Add description of level bitmask

[Stanimir Varbanov]

This adds description of the level bitmask feature.

Cc: Jonathan Corbet <corbet@lwn.net> (maintainer:DOCUMENTATION)

Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
---
 Documentation/admin-guide/dynamic-debug-howto.rst | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/admin-guide/dynamic-debug-howto.rst b/Documentation/admin-guide/dynamic-debug-howto.rst
index 0dc2eb8e44e5..c2b751fc8a17 100644
--- a/Documentation/admin-guide/dynamic-debug-howto.rst
+++ b/Documentation/admin-guide/dynamic-debug-howto.rst
@@ -208,6 +208,12 @@ line
 	line -1605          // the 1605 lines from line 1 to line 1605
 	line 1600-          // all lines from line 1600 to the end of the file
 
+level
+    The given level will be a bitmask ANDed with the level of the each ``pr_debug()``
+    callsite. This will allow to group debug messages and show only those of the
+    same level.  The -p flag takes precedence over the given level. Note that we can
+    have up to five groups of debug messages.
+
 The flags specification comprises a change operation followed
 by one or more flag characters.  The change operation is one
 of the characters::
@@ -346,6 +352,10 @@ Examples
   // add module, function to all enabled messages
   nullarbor:~ # echo -n '+mf' > <debugfs>/dynamic_debug/control
 
+  // enable all messages in file with 0x01 level bitmask
+  nullarbor:~ # echo -n 'file foo.c level 0x01 +p' >
+                                <debugfs>/dynamic_debug/control
+
   // boot-args example, with newlines and comments for readability
   Kernel command line: ...
     // see whats going on in dyndbg=value processing
-- 
2.17.1

Re: [PATCH v3 1/7] Documentation: dynamic-debug: Add description of level bitmask

[Matthew Wilcox]

On Tue, Jun 09, 2020 at 01:45:58PM +0300, Stanimir Varbanov wrote:
> +level
> +    The given level will be a bitmask ANDed with the level of the each ``pr_debug()``
> +    callsite. This will allow to group debug messages and show only those of the
> +    same level.  The -p flag takes precedence over the given level. Note that we can
> +    have up to five groups of debug messages.

That doesn't sound like a "level".  printk has levels.  If you ask for
"level 3" messages, you get messages from levels 0, 1, 2, and 3.  These
seem like "types" or "groups" or something.

> +  // enable all messages in file with 0x01 level bitmask
> +  nullarbor:~ # echo -n 'file foo.c level 0x01 +p' >
> +                                <debugfs>/dynamic_debug/control

Re: [PATCH v3 1/7] Documentation: dynamic-debug: Add description of level bitmask

[Greg Kroah-Hartman]

On Tue, Jun 09, 2020 at 01:45:58PM +0300, Stanimir Varbanov wrote:
> This adds description of the level bitmask feature.
> 
> Cc: Jonathan Corbet <corbet@lwn.net> (maintainer:DOCUMENTATION)
> 
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> ---
>  Documentation/admin-guide/dynamic-debug-howto.rst | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/Documentation/admin-guide/dynamic-debug-howto.rst b/Documentation/admin-guide/dynamic-debug-howto.rst
> index 0dc2eb8e44e5..c2b751fc8a17 100644
> --- a/Documentation/admin-guide/dynamic-debug-howto.rst
> +++ b/Documentation/admin-guide/dynamic-debug-howto.rst
> @@ -208,6 +208,12 @@ line
>  	line -1605          // the 1605 lines from line 1 to line 1605
>  	line 1600-          // all lines from line 1600 to the end of the file
>  
> +level
> +    The given level will be a bitmask ANDed with the level of the each ``pr_debug()``
> +    callsite. This will allow to group debug messages and show only those of the
> +    same level.  The -p flag takes precedence over the given level. Note that we can
> +    have up to five groups of debug messages.

As was pointed out, this isn't a "level", it's some arbitrary type of
"grouping".

But step back, why?  What is wrong with the existing control of dynamic
debug messages that you want to add another type of arbitrary grouping
to it?  And who defines that grouping?  Will it be
driver/subsystem/arch/author specific?  Or kernel-wide?

This feels like it could easily get out of hand really quickly.

Why not just use tracepoints if you really want to be fine-grained?

thanks,

greg k-h

Re: [PATCH v3 1/7] Documentation: dynamic-debug: Add description of level bitmask

[Joe Perches]

On Tue, 2020-06-09 at 13:16 +0200, Greg Kroah-Hartman wrote:
> What is wrong with the existing control of dynamic
> debug messages that you want to add another type of arbitrary grouping
> to it? 

There is no existing grouping mechanism.

Many drivers and some subsystems used an internal one
before dynamic debug.

$ git grep "MODULE_PARM.*\bdebug\b"|wc -l
501

This is an attempt to unify those homebrew mechanisms.

Stanimir attempted to add one for his driver via a
driver specific standardized format substring for level.

> And who defines that grouping?

Individual driver authors

> Will it be driver/subsystem/arch/author specific?  Or kernel-wide?

driver specific

> This feels like it could easily get out of hand really quickly.

Likely not.  A question might be how useful all these
old debugging printks are today and if it's reasonable
to just delete them.

> Why not just use tracepoints if you really want to be fine-grained?

Weight and lack of class/group capability

Re: [PATCH v3 1/7] Documentation: dynamic-debug: Add description of level bitmask

[Edward Cree]

On 09/06/2020 17:58, Joe Perches wrote:
> On Tue, 2020-06-09 at 13:16 +0200, Greg Kroah-Hartman wrote:
>> What is wrong with the existing control of dynamic
>> debug messages that you want to add another type of arbitrary grouping
>> to it? 
> There is no existing grouping mechanism.
>
> Many drivers and some subsystems used an internal one
> before dynamic debug.
>
> $ git grep "MODULE_PARM.*\bdebug\b"|wc -l
> 501
>
> This is an attempt to unify those homebrew mechanisms.
In network drivers, this is probablyusing the existing groupings
 defined by netif_level() - see NETIF_MSG_DRV and friends.  Note
 that those groups are orthogonal to the level, i.e. they control
 netif_err() etc. as well, not just debug messages.
Certainly in the case of sfc, and I'd imagine for many other net
 drivers too, the 'debug' modparam is setting the default for
 net_dev->msg_enable, which can be changed after probe with
 ethtool.
It doesn't look like the proposed mechanism subsumes that (we have
 rather more than 5 groups, and it's not clear how you'd connect
 it to the existing msg_enable (which uapi must be maintained); if
 you don't have a way to do this, better exclude drivers/net/ from
 your grep|wc because you won't be unifying those - in my tree
 that's 119 hits.

-ed

Re: [PATCH v3 1/7] Documentation: dynamic-debug: Add description of level bitmask

[Joe Perches]

On Tue, 2020-06-09 at 18:42 +0100, Edward Cree wrote:
> On 09/06/2020 17:58, Joe Perches wrote:
> > On Tue, 2020-06-09 at 13:16 +0200, Greg Kroah-Hartman wrote:
> > > What is wrong with the existing control of dynamic
> > > debug messages that you want to add another type of arbitrary grouping
> > > to it? 
> > There is no existing grouping mechanism.
> > 
> > Many drivers and some subsystems used an internal one
> > before dynamic debug.
> > 
> > $ git grep "MODULE_PARM.*\bdebug\b"|wc -l
> > 501
> > 
> > This is an attempt to unify those homebrew mechanisms.
> In network drivers, this is probablyusing the existing groupings
>  defined by netif_level() - see NETIF_MSG_DRV and friends.  Note
>  that those groups are orthogonal to the level, i.e. they control
>  netif_err() etc. as well, not just debug messages.

These are _not_ netif_<level> control flags.  Some are though.

For instance:

$ git grep "MODULE_PARM.*\bdebug\b" drivers/net | head -10
drivers/net/ethernet/3com/3c509.c:MODULE_PARM_DESC(debug, "debug level (0-6)");
drivers/net/ethernet/3com/3c515.c:MODULE_PARM_DESC(debug, "3c515 debug level (0-6)");
drivers/net/ethernet/3com/3c59x.c:MODULE_PARM_DESC(debug, "3c59x debug level (0-6)");
drivers/net/ethernet/adaptec/starfire.c:MODULE_PARM_DESC(debug, "Debug level (0-6)");
drivers/net/ethernet/allwinner/sun4i-emac.c:MODULE_PARM_DESC(debug, "debug message flags");
drivers/net/ethernet/altera/altera_tse_main.c:MODULE_PARM_DESC(debug, "Message Level (-1: default, 0: no output, 16: all)");
drivers/net/ethernet/amazon/ena/ena_netdev.c:MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)");
drivers/net/ethernet/amd/atarilance.c:MODULE_PARM_DESC(lance_debug, "atarilance debug level (0-3)");
drivers/net/ethernet/amd/lance.c:MODULE_PARM_DESC(lance_debug, "LANCE/PCnet debug level (0-7)");
drivers/net/ethernet/amd/pcnet32.c:MODULE_PARM_DESC(debug, DRV_NAME " debug level");

These are all level/class output controls.

> Certainly in the case of sfc, and I'd imagine for many other net
>  drivers too, the 'debug' modparam is setting the default for
>  net_dev->msg_enable, which can be changed after probe with
>  ethtool.

True.

> It doesn't look like the proposed mechanism subsumes that (we have
>  rather more than 5 groups, and it's not clear how you'd connect
>  it to the existing msg_enable (which uapi must be maintained); if
>  you don't have a way to do this, better exclude drivers/net/ from
>  your grep|wc because you won't be unifying those - in my tree
>  that's 119 hits.

Likely not.

I agree it'd be useful to attach the modparam control flag
to the dynamic debug use somehow.

cheers, Joe

Re: [PATCH v3 1/7] Documentation: dynamic-debug: Add description of level bitmask

[Edward Cree]

On 09/06/2020 18:56, Joe Perches wrote:
> These are _not_ netif_<level> control flags. Some are though.
> For instance:
>
> $ git grep "MODULE_PARM.*\bdebug\b" drivers/net | head -10
> [...]
>
> These are all level/class output controls.
TIL, thanks!  I should have looked deeperrather than assuming
 they were all like ours.

Though judging just by that grep output, it also looks like
 quite a lot of those won't fit into 5 groups either, so some
 rethink may still be needed...

-ed

Re: [PATCH v3 1/7] Documentation: dynamic-debug: Add description of level bitmask

[Greg Kroah-Hartman]

On Tue, Jun 09, 2020 at 09:58:07AM -0700, Joe Perches wrote:
> On Tue, 2020-06-09 at 13:16 +0200, Greg Kroah-Hartman wrote:
> > What is wrong with the existing control of dynamic
> > debug messages that you want to add another type of arbitrary grouping
> > to it? 
> 
> There is no existing grouping mechanism.

info/warn/err/dbg is what I am referring to.

> Many drivers and some subsystems used an internal one
> before dynamic debug.
> 
> $ git grep "MODULE_PARM.*\bdebug\b"|wc -l
> 501

Yes, and it's horrid and needs to be cleaned up, not added to.

In the beginning, yes, adding loads of different types of debugging
options to a driver is needed by the author, but by the time it is added
to the kernel, all of that should be able to be removed and only a
single "enable debug" should be all that is needed.

We do not need each individual driver thinking it needs to have some
sort of special classification of each type of debug message.  Just use
the framework that we have, you can enable/disable them on a
line-by-line basis as needed.

> This is an attempt to unify those homebrew mechanisms.

All of those should just be removed.

> Stanimir attempted to add one for his driver via a
> driver specific standardized format substring for level.
> 
> > And who defines that grouping?
> 
> Individual driver authors

That way lies madness, let's try to fix all of that up.

greg k-h

Re: [PATCH v3 1/7] Documentation: dynamic-debug: Add description of level bitmask

[Joe Perches]

On Wed, 2020-06-10 at 08:31 +0200, Greg Kroah-Hartman wrote:
> On Tue, Jun 09, 2020 at 09:58:07AM -0700, Joe Perches wrote:
> > On Tue, 2020-06-09 at 13:16 +0200, Greg Kroah-Hartman wrote:
> > > What is wrong with the existing control of dynamic
> > > debug messages that you want to add another type of arbitrary grouping
> > > to it? 
> > 
> > There is no existing grouping mechanism.
> 
> info/warn/err/dbg is what I am referring to.
> 
> > Many drivers and some subsystems used an internal one
> > before dynamic debug.
> > 
> > $ git grep "MODULE_PARM.*\bdebug\b"|wc -l
> > 501
> 
> Yes, and it's horrid and needs to be cleaned up, not added to.

Or unified so driver authors have a standardized mechanism
rather than reinventing or doing things differently.

> In the beginning, yes, adding loads of different types of debugging
> options to a driver is needed by the author, but by the time it is added
> to the kernel, all of that should be able to be removed and only a
> single "enable debug" should be all that is needed.

No one does that.

Re: [PATCH v3 1/7] Documentation: dynamic-debug: Add description of level bitmask

[Greg Kroah-Hartman]

On Tue, Jun 09, 2020 at 11:35:31PM -0700, Joe Perches wrote:
> On Wed, 2020-06-10 at 08:31 +0200, Greg Kroah-Hartman wrote:
> > On Tue, Jun 09, 2020 at 09:58:07AM -0700, Joe Perches wrote:
> > > On Tue, 2020-06-09 at 13:16 +0200, Greg Kroah-Hartman wrote:
> > > > What is wrong with the existing control of dynamic
> > > > debug messages that you want to add another type of arbitrary grouping
> > > > to it? 
> > > 
> > > There is no existing grouping mechanism.
> > 
> > info/warn/err/dbg is what I am referring to.
> > 
> > > Many drivers and some subsystems used an internal one
> > > before dynamic debug.
> > > 
> > > $ git grep "MODULE_PARM.*\bdebug\b"|wc -l
> > > 501
> > 
> > Yes, and it's horrid and needs to be cleaned up, not added to.
> 
> Or unified so driver authors have a standardized mechanism
> rather than reinventing or doing things differently.

But each "level" you all come up with will be intrepreted differently
per driver, causing total confusion (like we have today.)  Try to make
it better by just removing that mess.

> > In the beginning, yes, adding loads of different types of debugging
> > options to a driver is needed by the author, but by the time it is added
> > to the kernel, all of that should be able to be removed and only a
> > single "enable debug" should be all that is needed.
> 
> No one does that.

We did that for USB drivers a decade ago, it can be done.

greg k-h

Re: [PATCH v3 1/7] Documentation: dynamic-debug: Add description of level bitmask

[Joe Perches]

On Wed, 2020-06-10 at 09:09 +0200, Greg Kroah-Hartman wrote:
> On Tue, Jun 09, 2020 at 11:35:31PM -0700, Joe Perches wrote:
> > On Wed, 2020-06-10 at 08:31 +0200, Greg Kroah-Hartman wrote:
> > > On Tue, Jun 09, 2020 at 09:58:07AM -0700, Joe Perches wrote:
> > > > On Tue, 2020-06-09 at 13:16 +0200, Greg Kroah-Hartman wrote:
> > > > > What is wrong with the existing control of dynamic
> > > > > debug messages that you want to add another type of arbitrary grouping
> > > > > to it? 
> > > > 
> > > > There is no existing grouping mechanism.
> > > 
> > > info/warn/err/dbg is what I am referring to.

This is specifically about dbg so that's not relevant is it.
> But each "level" you all come up with will be intrepreted differently
> per driver, causing total confusion (like we have today.)  Try to make
> it better by just removing that mess.

Or add value as it allows the developer to do what's
necessary for their development.

> > > In the beginning, yes, adding loads of different types of debugging
> > > options to a driver is needed by the author, but by the time it is added
> > > to the kernel, all of that should be able to be removed and only a
> > > single "enable debug" should be all that is needed.
> > 
> > No one does that.
> 
> We did that for USB drivers a decade ago, it can be done.

And nearly no one does it.

btw: look up usbip_debug_flag and usbip_dbg_<foo> or the uhci driver

Re: [PATCH v3 1/7] Documentation: dynamic-debug: Add description of level bitmask

[Stanimir Varbanov]

Hi Greg,

On 6/9/20 2:16 PM, Greg Kroah-Hartman wrote:
> On Tue, Jun 09, 2020 at 01:45:58PM +0300, Stanimir Varbanov wrote:
>> This adds description of the level bitmask feature.
>>
>> Cc: Jonathan Corbet <corbet@lwn.net> (maintainer:DOCUMENTATION)
>>
>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
>> ---
>>  Documentation/admin-guide/dynamic-debug-howto.rst | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/Documentation/admin-guide/dynamic-debug-howto.rst b/Documentation/admin-guide/dynamic-debug-howto.rst
>> index 0dc2eb8e44e5..c2b751fc8a17 100644
>> --- a/Documentation/admin-guide/dynamic-debug-howto.rst
>> +++ b/Documentation/admin-guide/dynamic-debug-howto.rst
>> @@ -208,6 +208,12 @@ line
>>  	line -1605          // the 1605 lines from line 1 to line 1605
>>  	line 1600-          // all lines from line 1600 to the end of the file
>>  
>> +level
>> +    The given level will be a bitmask ANDed with the level of the each ``pr_debug()``
>> +    callsite. This will allow to group debug messages and show only those of the
>> +    same level.  The -p flag takes precedence over the given level. Note that we can
>> +    have up to five groups of debug messages.
> 
> As was pointed out, this isn't a "level", it's some arbitrary type of
> "grouping".

Yes, it is grouping of KERN_DEBUG level messages by importance (my
fault, I put incorrect name).  What is important is driver author
decision.  Usually when the driver is huge and has a lot of debug
messages it is not practical to enable all of them to chasing a
particular bug or issue.  You know that debugging (printk) add delays
which could hide or rise additional issue(s) which would complicate
debug and waste time.

For the Venus driver I have defined three groups of KERN_DEBUG - low,
medium and high (again the driver author(s) will decide what the
importance is depending on his past experience).

There is another point where the debugging is made by person who is not
familiar with the driver code. In that case he/she cannot enable lines
or range of lines because he don't know the details. Here the grouping
by importance could help.

> 
> But step back, why?  What is wrong with the existing control of dynamic
> debug messages that you want to add another type of arbitrary grouping
> to it?  And who defines that grouping?  Will it be
> driver/subsystem/arch/author specific?  Or kernel-wide?
> 
> This feels like it could easily get out of hand really quickly.
> 
> Why not just use tracepoints if you really want to be fine-grained?
> 
> thanks,
> 
> greg k-h
> 

-- 
regards,
Stan

Re: [PATCH v3 1/7] Documentation: dynamic-debug: Add description of level bitmask

[Greg Kroah-Hartman]

On Wed, Jun 10, 2020 at 01:29:20PM +0300, Stanimir Varbanov wrote:
> Hi Greg,
> 
> On 6/9/20 2:16 PM, Greg Kroah-Hartman wrote:
> > On Tue, Jun 09, 2020 at 01:45:58PM +0300, Stanimir Varbanov wrote:
> >> This adds description of the level bitmask feature.
> >>
> >> Cc: Jonathan Corbet <corbet@lwn.net> (maintainer:DOCUMENTATION)
> >>
> >> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> >> ---
> >>  Documentation/admin-guide/dynamic-debug-howto.rst | 10 ++++++++++
> >>  1 file changed, 10 insertions(+)
> >>
> >> diff --git a/Documentation/admin-guide/dynamic-debug-howto.rst b/Documentation/admin-guide/dynamic-debug-howto.rst
> >> index 0dc2eb8e44e5..c2b751fc8a17 100644
> >> --- a/Documentation/admin-guide/dynamic-debug-howto.rst
> >> +++ b/Documentation/admin-guide/dynamic-debug-howto.rst
> >> @@ -208,6 +208,12 @@ line
> >>  	line -1605          // the 1605 lines from line 1 to line 1605
> >>  	line 1600-          // all lines from line 1600 to the end of the file
> >>  
> >> +level
> >> +    The given level will be a bitmask ANDed with the level of the each ``pr_debug()``
> >> +    callsite. This will allow to group debug messages and show only those of the
> >> +    same level.  The -p flag takes precedence over the given level. Note that we can
> >> +    have up to five groups of debug messages.
> > 
> > As was pointed out, this isn't a "level", it's some arbitrary type of
> > "grouping".
> 
> Yes, it is grouping of KERN_DEBUG level messages by importance (my
> fault, I put incorrect name).  What is important is driver author
> decision.  Usually when the driver is huge and has a lot of debug
> messages it is not practical to enable all of them to chasing a
> particular bug or issue.  You know that debugging (printk) add delays
> which could hide or rise additional issue(s) which would complicate
> debug and waste time.

That is why it is possible to turn on and off debugging messages on a
function/line basis already.  Why not just use that instead?

> For the Venus driver I have defined three groups of KERN_DEBUG - low,
> medium and high (again the driver author(s) will decide what the
> importance is depending on his past experience).
> 
> There is another point where the debugging is made by person who is not
> familiar with the driver code. In that case he/she cannot enable lines
> or range of lines because he don't know the details. Here the grouping
> by importance could help.

And they will really know what "low/medium/high" are?

Anyway, that makes a bit more sense, but the documentation could use a
lot more in order to describe this type of behavior, and what is
expected by both driver authors, and users of the interface.

thanks,

greg k-h

[PATCH v3 2/7] dynamic_debug: Group debug messages by level bitmask

[Stanimir Varbanov]

This will allow dynamic debug users and driver writers to group
debug messages by level bitmask.  The level bitmask should be a
hex number.

Done this functionality by extending dynamic debug metadata with
new level member and propagate it over all the users.  Also
introduce new dynamic_pr_debug_level and dynamic_dev_dbg_level
macros to be used by the drivers.

Cc: Jason Baron <jbaron@akamai.com>
Cc: Chris Mason <clm@fb.com>
Cc: Josef Bacik <josef@toxicpanda.com>
Cc: David Sterba <dsterba@suse.com>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Len Brown <lenb@kernel.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Petr Mladek <pmladek@suse.com>
Cc: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Suggested-by: Joe Perches <joe@perches.com>
Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
---
 fs/btrfs/ctree.h              | 12 +++++---
 include/linux/acpi.h          |  3 +-
 include/linux/dev_printk.h    |  3 +-
 include/linux/dynamic_debug.h | 55 ++++++++++++++++++++++++-----------
 include/linux/net.h           |  3 +-
 include/linux/printk.h        |  3 +-
 lib/dynamic_debug.c           | 30 +++++++++++++++++++
 7 files changed, 84 insertions(+), 25 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 161533040978..f6a778789056 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -3081,16 +3081,20 @@ void btrfs_printk(const struct btrfs_fs_info *fs_info, const char *fmt, ...);
 
 #if defined(CONFIG_DYNAMIC_DEBUG)
 #define btrfs_debug(fs_info, fmt, args...)				\
-	_dynamic_func_call_no_desc(fmt, btrfs_printk,			\
+	_dynamic_func_call_no_desc(fmt, _DPRINTK_LEVEL_DEFAULT,		\
+				   btrfs_printk,			\
 				   fs_info, KERN_DEBUG fmt, ##args)
 #define btrfs_debug_in_rcu(fs_info, fmt, args...)			\
-	_dynamic_func_call_no_desc(fmt, btrfs_printk_in_rcu,		\
+	_dynamic_func_call_no_desc(fmt, _DPRINTK_LEVEL_DEFAULT,		\
+				   btrfs_printk_in_rcu,			\
 				   fs_info, KERN_DEBUG fmt, ##args)
 #define btrfs_debug_rl_in_rcu(fs_info, fmt, args...)			\
-	_dynamic_func_call_no_desc(fmt, btrfs_printk_rl_in_rcu,		\
+	_dynamic_func_call_no_desc(fmt, _DPRINTK_LEVEL_DEFAULT,		\
+				   btrfs_printk_rl_in_rcu,		\
 				   fs_info, KERN_DEBUG fmt, ##args)
 #define btrfs_debug_rl(fs_info, fmt, args...)				\
-	_dynamic_func_call_no_desc(fmt, btrfs_printk_ratelimited,	\
+	_dynamic_func_call_no_desc(fmt, _DPRINTK_LEVEL_DEFAULT,		\
+				   btrfs_printk_ratelimited,		\
 				   fs_info, KERN_DEBUG fmt, ##args)
 #elif defined(DEBUG)
 #define btrfs_debug(fs_info, fmt, args...) \
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index d661cd0ee64d..6e51438f7635 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1043,7 +1043,8 @@ void __acpi_handle_debug(struct _ddebug *descriptor, acpi_handle handle, const c
 #else
 #if defined(CONFIG_DYNAMIC_DEBUG)
 #define acpi_handle_debug(handle, fmt, ...)				\
-	_dynamic_func_call(fmt, __acpi_handle_debug,			\
+	_dynamic_func_call(fmt, _DPRINTK_LEVEL_DEFAULT,			\
+			   __acpi_handle_debug,				\
 			   handle, pr_fmt(fmt), ##__VA_ARGS__)
 #else
 #define acpi_handle_debug(handle, fmt, ...)				\
diff --git a/include/linux/dev_printk.h b/include/linux/dev_printk.h
index 5aad06b4ca7b..7b50551833e1 100644
--- a/include/linux/dev_printk.h
+++ b/include/linux/dev_printk.h
@@ -188,7 +188,8 @@ do {									\
 	static DEFINE_RATELIMIT_STATE(_rs,				\
 				      DEFAULT_RATELIMIT_INTERVAL,	\
 				      DEFAULT_RATELIMIT_BURST);		\
-	DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt);			\
+	DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt,			\
+				      _DPRINTK_LEVEL_DEFAULT);		\
 	if (DYNAMIC_DEBUG_BRANCH(descriptor) &&				\
 	    __ratelimit(&_rs))						\
 		__dynamic_dev_dbg(&descriptor, dev, dev_fmt(fmt),	\
diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index 4cf02ecd67de..95e97260c517 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -38,6 +38,8 @@ struct _ddebug {
 #define _DPRINTK_FLAGS_DEFAULT 0
 #endif
 	unsigned int flags:8;
+#define _DPRINTK_LEVEL_DEFAULT 0
+	unsigned int level:5;
 #ifdef CONFIG_JUMP_LABEL
 	union {
 		struct static_key_true dd_key_true;
@@ -78,7 +80,7 @@ void __dynamic_ibdev_dbg(struct _ddebug *descriptor,
 			 const struct ib_device *ibdev,
 			 const char *fmt, ...);
 
-#define DEFINE_DYNAMIC_DEBUG_METADATA(name, fmt)		\
+#define DEFINE_DYNAMIC_DEBUG_METADATA(name, fmt, lvl)		\
 	static struct _ddebug  __aligned(8)			\
 	__attribute__((section("__verbose"))) name = {		\
 		.modname = KBUILD_MODNAME,			\
@@ -87,6 +89,7 @@ void __dynamic_ibdev_dbg(struct _ddebug *descriptor,
 		.format = (fmt),				\
 		.lineno = __LINE__,				\
 		.flags = _DPRINTK_FLAGS_DEFAULT,		\
+		.level = (lvl),					\
 		_DPRINTK_KEY_INIT				\
 	}
 
@@ -119,16 +122,16 @@ void __dynamic_ibdev_dbg(struct _ddebug *descriptor,
 
 #endif
 
-#define __dynamic_func_call(id, fmt, func, ...) do {	\
-	DEFINE_DYNAMIC_DEBUG_METADATA(id, fmt);		\
-	if (DYNAMIC_DEBUG_BRANCH(id))			\
-		func(&id, ##__VA_ARGS__);		\
+#define __dynamic_func_call(id, fmt, level, func, ...) do {	\
+	DEFINE_DYNAMIC_DEBUG_METADATA(id, fmt, level);		\
+	if (DYNAMIC_DEBUG_BRANCH(id))				\
+		func(&id, ##__VA_ARGS__);			\
 } while (0)
 
-#define __dynamic_func_call_no_desc(id, fmt, func, ...) do {	\
-	DEFINE_DYNAMIC_DEBUG_METADATA(id, fmt);			\
-	if (DYNAMIC_DEBUG_BRANCH(id))				\
-		func(__VA_ARGS__);				\
+#define __dynamic_func_call_no_desc(id, fmt, level, func, ...) do {	\
+	DEFINE_DYNAMIC_DEBUG_METADATA(id, fmt, level);			\
+	if (DYNAMIC_DEBUG_BRANCH(id))					\
+		func(__VA_ARGS__);					\
 } while (0)
 
 /*
@@ -139,35 +142,49 @@ void __dynamic_ibdev_dbg(struct _ddebug *descriptor,
  * the varargs. Note that fmt is repeated in invocations of this
  * macro.
  */
-#define _dynamic_func_call(fmt, func, ...)				\
-	__dynamic_func_call(__UNIQUE_ID(ddebug), fmt, func, ##__VA_ARGS__)
+#define _dynamic_func_call(fmt, lvl, func, ...)				\
+	__dynamic_func_call(__UNIQUE_ID(ddebug), fmt, lvl, func, ##__VA_ARGS__)
 /*
  * A variant that does the same, except that the descriptor is not
  * passed as the first argument to the function; it is only called
  * with precisely the macro's varargs.
  */
-#define _dynamic_func_call_no_desc(fmt, func, ...)	\
-	__dynamic_func_call_no_desc(__UNIQUE_ID(ddebug), fmt, func, ##__VA_ARGS__)
+#define _dynamic_func_call_no_desc(fmt, lvl, func, ...)			\
+	__dynamic_func_call_no_desc(__UNIQUE_ID(ddebug), fmt, lvl,	\
+				    func, ##__VA_ARGS__)
 
 #define dynamic_pr_debug(fmt, ...)				\
-	_dynamic_func_call(fmt,	__dynamic_pr_debug,		\
+	_dynamic_func_call(fmt, _DPRINTK_LEVEL_DEFAULT,		\
+			   __dynamic_pr_debug,			\
+			   pr_fmt(fmt), ##__VA_ARGS__)
+
+#define dynamic_pr_debug_level(lvl, fmt, ...)			\
+	_dynamic_func_call(fmt, lvl, __dynamic_pr_debug,	\
 			   pr_fmt(fmt), ##__VA_ARGS__)
 
 #define dynamic_dev_dbg(dev, fmt, ...)				\
-	_dynamic_func_call(fmt,__dynamic_dev_dbg, 		\
+	_dynamic_func_call(fmt, _DPRINTK_LEVEL_DEFAULT,		\
+			   __dynamic_dev_dbg,			\
+			   dev, fmt, ##__VA_ARGS__)
+
+#define dynamic_dev_dbg_level(dev, lvl, fmt, ...)		\
+	_dynamic_func_call(fmt, lvl, __dynamic_dev_dbg,		\
 			   dev, fmt, ##__VA_ARGS__)
 
 #define dynamic_netdev_dbg(dev, fmt, ...)			\
-	_dynamic_func_call(fmt, __dynamic_netdev_dbg,		\
+	_dynamic_func_call(fmt, _DPRINTK_LEVEL_DEFAULT,		\
+			   __dynamic_netdev_dbg,		\
 			   dev, fmt, ##__VA_ARGS__)
 
 #define dynamic_ibdev_dbg(dev, fmt, ...)			\
-	_dynamic_func_call(fmt, __dynamic_ibdev_dbg,		\
+	_dynamic_func_call(fmt, _DPRINTK_LEVEL_DEFAULT,		\
+			   __dynamic_ibdev_dbg,			\
 			   dev, fmt, ##__VA_ARGS__)
 
 #define dynamic_hex_dump(prefix_str, prefix_type, rowsize,		\
 			 groupsize, buf, len, ascii)			\
 	_dynamic_func_call_no_desc(__builtin_constant_p(prefix_str) ? prefix_str : "hexdump", \
+				   _DPRINTK_LEVEL_DEFAULT,		\
 				   print_hex_dump,			\
 				   KERN_DEBUG, prefix_str, prefix_type,	\
 				   rowsize, groupsize, buf, len, ascii)
@@ -202,8 +219,12 @@ static inline int ddebug_dyndbg_module_param_cb(char *param, char *val,
 
 #define dynamic_pr_debug(fmt, ...)					\
 	do { if (0) printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__); } while (0)
+#define dynamic_pr_debug_level(lvl, fmt, ...)				\
+	do { if (0) printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__); } while (0)
 #define dynamic_dev_dbg(dev, fmt, ...)					\
 	do { if (0) dev_printk(KERN_DEBUG, dev, fmt, ##__VA_ARGS__); } while (0)
+#define dynamic_dev_dbg_level(dev, lvl, fmt, ...)			\
+	do { if (0) dev_printk(KERN_DEBUG, dev, fmt, ##__VA_ARGS__); } while (0)
 #define dynamic_hex_dump(prefix_str, prefix_type, rowsize,		\
 			 groupsize, buf, len, ascii)			\
 	do { if (0)							\
diff --git a/include/linux/net.h b/include/linux/net.h
index e10f378194a5..bcf6f010bc67 100644
--- a/include/linux/net.h
+++ b/include/linux/net.h
@@ -267,7 +267,8 @@ do {								\
 #if defined(CONFIG_DYNAMIC_DEBUG)
 #define net_dbg_ratelimited(fmt, ...)					\
 do {									\
-	DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt);			\
+	DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt,			\
+				      _DPRINTK_LEVEL_DEFAULT);		\
 	if (DYNAMIC_DEBUG_BRANCH(descriptor) &&				\
 	    net_ratelimit())						\
 		__dynamic_pr_debug(&descriptor, pr_fmt(fmt),		\
diff --git a/include/linux/printk.h b/include/linux/printk.h
index 3cc2f178bf06..ceea84aa705b 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -542,7 +542,8 @@ do {									\
 	static DEFINE_RATELIMIT_STATE(_rs,				\
 				      DEFAULT_RATELIMIT_INTERVAL,	\
 				      DEFAULT_RATELIMIT_BURST);		\
-	DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, pr_fmt(fmt));		\
+	DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, pr_fmt(fmt),		\
+				      _DPRINTK_LEVEL_DEFAULT);		\
 	if (DYNAMIC_DEBUG_BRANCH(descriptor) &&				\
 	    __ratelimit(&_rs))						\
 		__dynamic_pr_debug(&descriptor, pr_fmt(fmt), ##__VA_ARGS__);	\
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 8f199f403ab5..5d28d388f6dd 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -55,6 +55,7 @@ struct ddebug_query {
 	const char *function;
 	const char *format;
 	unsigned int first_lineno, last_lineno;
+	unsigned int level;
 };
 
 struct ddebug_iter {
@@ -187,6 +188,18 @@ static int ddebug_change(const struct ddebug_query *query,
 
 			nfound++;
 
+#ifdef CONFIG_JUMP_LABEL
+			if (query->level && query->level & dp->level) {
+				if (flags & _DPRINTK_FLAGS_PRINT)
+					static_branch_enable(&dp->key.dd_key_true);
+				else
+					static_branch_disable(&dp->key.dd_key_true);
+			} else if (query->level &&
+				   flags & _DPRINTK_FLAGS_PRINT) {
+				static_branch_disable(&dp->key.dd_key_true);
+				continue;
+			}
+#endif
 			newflags = (dp->flags & mask) | flags;
 			if (newflags == dp->flags)
 				continue;
@@ -289,6 +302,20 @@ static inline int parse_lineno(const char *str, unsigned int *val)
 	return 0;
 }
 
+static inline int parse_level(const char *str, unsigned int *val)
+{
+	WARN_ON(str == NULL);
+	if (*str == '\0') {
+		*val = 0;
+		return 0;
+	}
+	if (kstrtouint(str, 0, val) < 0) {
+		pr_err("bad level-number: %s\n", str);
+		return -EINVAL;
+	}
+	return 0;
+}
+
 static int check_set(const char **dest, char *src, char *name)
 {
 	int rc = 0;
@@ -375,6 +402,9 @@ static int ddebug_parse_query(char *words[], int nwords,
 			} else {
 				query->last_lineno = query->first_lineno;
 			}
+		} else if (!strcmp(words[i], "level")) {
+			if (parse_level(words[i+1], &query->level) < 0)
+				return -EINVAL;
 		} else {
 			pr_err("unknown keyword \"%s\"\n", words[i]);
 			return -EINVAL;
-- 
2.17.1

Re: [PATCH v3 2/7] dynamic_debug: Group debug messages by level bitmask

[Petr Mladek]

On Tue 2020-06-09 13:45:59, Stanimir Varbanov wrote:
> This will allow dynamic debug users and driver writers to group
> debug messages by level bitmask.  The level bitmask should be a
> hex number.
> 
> Done this functionality by extending dynamic debug metadata with
> new level member and propagate it over all the users.  Also
> introduce new dynamic_pr_debug_level and dynamic_dev_dbg_level
> macros to be used by the drivers.

Could you please provide more details?

What is the use case?
What is the exact meaning of the level value?
How the levels will get defined?

Dynamic debug is used for messages with KERN_DEBUG log level.
Is this another dimension of the message leveling?

Given that the filter is defined by bits, it is rather grouping
by context or so.


> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> index 8f199f403ab5..5d28d388f6dd 100644
> --- a/lib/dynamic_debug.c
> +++ b/lib/dynamic_debug.c
> @@ -55,6 +55,7 @@ struct ddebug_query {
>  	const char *function;
>  	const char *format;
>  	unsigned int first_lineno, last_lineno;
> +	unsigned int level;
>  };
>  
>  struct ddebug_iter {
> @@ -187,6 +188,18 @@ static int ddebug_change(const struct ddebug_query *query,
>  
>  			nfound++;
>  
> +#ifdef CONFIG_JUMP_LABEL
> +			if (query->level && query->level & dp->level) {
> +				if (flags & _DPRINTK_FLAGS_PRINT)
> +					static_branch_enable(&dp->key.dd_key_true);
> +				else
> +					static_branch_disable(&dp->key.dd_key_true);
> +			} else if (query->level &&
> +				   flags & _DPRINTK_FLAGS_PRINT) {
> +				static_branch_disable(&dp->key.dd_key_true);
> +				continue;
> +			}
> +#endif

This looks like a hack in the existing code:

  + It is suspicious that "continue" is only in one branch. It means
    that static_branch_enable/disable() might get called 2nd time
    by the code below. Or newflags are not stored when there is a change.

  + It changes the behavior and the below vpr_info("changed ...")
    is not called.

Or do I miss anything?

>			newflags = (dp->flags & mask) | flags;
>  			if (newflags == dp->flags)
>  				continue;

Best Regards,
Petr

[PATCH v3 3/7] dev_printk: Add dev_dbg_level macro over dynamic one

[Stanimir Varbanov]

Add dev_dbg_level macro wrapper over dynamic debug one for
dev_dbg variants.

Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
---
 include/linux/dev_printk.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/include/linux/dev_printk.h b/include/linux/dev_printk.h
index 7b50551833e1..d639dc60d84d 100644
--- a/include/linux/dev_printk.h
+++ b/include/linux/dev_printk.h
@@ -112,15 +112,24 @@ void _dev_info(const struct device *dev, const char *fmt, ...)
 #if defined(CONFIG_DYNAMIC_DEBUG)
 #define dev_dbg(dev, fmt, ...)						\
 	dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
+#define dev_dbg_level(dev, lvl, fmt, ...)				\
+	dynamic_dev_dbg_level(dev, lvl, dev_fmt(fmt), ##__VA_ARGS__)
 #elif defined(DEBUG)
 #define dev_dbg(dev, fmt, ...)						\
 	dev_printk(KERN_DEBUG, dev, dev_fmt(fmt), ##__VA_ARGS__)
+#define dev_dbg_level(dev, lvl, fmt, ...)				\
+	dev_printk(KERN_DEBUG, dev, dev_fmt(fmt), ##__VA_ARGS__)
 #else
 #define dev_dbg(dev, fmt, ...)						\
 ({									\
 	if (0)								\
 		dev_printk(KERN_DEBUG, dev, dev_fmt(fmt), ##__VA_ARGS__); \
 })
+#define dev_dbg_level(dev, lvl, fmt, ...)				\
+({									\
+	if (0)								\
+		dev_printk(KERN_DEBUG, dev, dev_fmt(fmt), ##__VA_ARGS__); \
+})
 #endif
 
 #ifdef CONFIG_PRINTK
-- 
2.17.1

[PATCH v3 4/7] printk: Add pr_debug_level macro over dynamic one

[Stanimir Varbanov]

Introduce new pr_debug_level macro over dynamic_debug level one
to allow dynamic debugging to show only important messages.

Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
---
 include/linux/printk.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/linux/printk.h b/include/linux/printk.h
index ceea84aa705b..2a6eca56010f 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -416,12 +416,18 @@ extern int kptr_restrict;
  */
 #define pr_debug(fmt, ...)			\
 	dynamic_pr_debug(fmt, ##__VA_ARGS__)
+#define pr_debug_level(lvl, fmt, ...) \
+	dynamic_pr_debug_level(lvl, fmt, ##__VA_ARGS__)
 #elif defined(DEBUG)
 #define pr_debug(fmt, ...) \
 	printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
+#define pr_debug_level(lvl, fmt, ...) \
+	printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
 #else
 #define pr_debug(fmt, ...) \
 	no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
+#define pr_debug_level(lvl, fmt, ...) \
+	no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
 #endif
 
 /*
-- 
2.17.1

Re: [PATCH v3 4/7] printk: Add pr_debug_level macro over dynamic one

[Greg Kroah-Hartman]

On Tue, Jun 09, 2020 at 01:46:01PM +0300, Stanimir Varbanov wrote:
> Introduce new pr_debug_level macro over dynamic_debug level one
> to allow dynamic debugging to show only important messages.

What does "important messages" mean???

[PATCH v3 5/7] venus: Add debugfs interface to set firmware log level

[Stanimir Varbanov]

This will be useful when debugging specific issues related to
firmware HFI interface.

Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
---
 drivers/media/platform/qcom/venus/Makefile    |  2 +-
 drivers/media/platform/qcom/venus/core.c      |  5 ++++
 drivers/media/platform/qcom/venus/core.h      |  3 +++
 drivers/media/platform/qcom/venus/dbgfs.c     | 26 +++++++++++++++++++
 drivers/media/platform/qcom/venus/dbgfs.h     | 12 +++++++++
 drivers/media/platform/qcom/venus/hfi_venus.c |  7 ++++-
 6 files changed, 53 insertions(+), 2 deletions(-)
 create mode 100644 drivers/media/platform/qcom/venus/dbgfs.c
 create mode 100644 drivers/media/platform/qcom/venus/dbgfs.h

diff --git a/drivers/media/platform/qcom/venus/Makefile b/drivers/media/platform/qcom/venus/Makefile
index 64af0bc1edae..dfc636865709 100644
--- a/drivers/media/platform/qcom/venus/Makefile
+++ b/drivers/media/platform/qcom/venus/Makefile
@@ -3,7 +3,7 @@
 
 venus-core-objs += core.o helpers.o firmware.o \
 		   hfi_venus.o hfi_msgs.o hfi_cmds.o hfi.o \
-		   hfi_parser.o pm_helpers.o
+		   hfi_parser.o pm_helpers.o dbgfs.o
 
 venus-dec-objs += vdec.o vdec_ctrls.o
 venus-enc-objs += venc.o venc_ctrls.o
diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index 203c6538044f..bbb394ca4175 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -290,6 +290,10 @@ static int venus_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_dev_unregister;
 
+	ret = venus_dbgfs_init(core);
+	if (ret)
+		goto err_dev_unregister;
+
 	return 0;
 
 err_dev_unregister:
@@ -337,6 +341,7 @@ static int venus_remove(struct platform_device *pdev)
 	v4l2_device_unregister(&core->v4l2_dev);
 	mutex_destroy(&core->pm_lock);
 	mutex_destroy(&core->lock);
+	venus_dbgfs_deinit(core);
 
 	return ret;
 }
diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
index 7118612673c9..b48782f9aa95 100644
--- a/drivers/media/platform/qcom/venus/core.h
+++ b/drivers/media/platform/qcom/venus/core.h
@@ -12,6 +12,7 @@
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
 
+#include "dbgfs.h"
 #include "hfi.h"
 
 #define VIDC_CLKS_NUM_MAX		4
@@ -136,6 +137,7 @@ struct venus_caps {
  * @priv:	a private filed for HFI operations
  * @ops:		the core HFI operations
  * @work:	a delayed work for handling system fatal error
+ * @root:	debugfs root directory
  */
 struct venus_core {
 	void __iomem *base;
@@ -185,6 +187,7 @@ struct venus_core {
 	unsigned int codecs_count;
 	unsigned int core0_usage_count;
 	unsigned int core1_usage_count;
+	struct dentry *root;
 };
 
 struct vdec_controls {
diff --git a/drivers/media/platform/qcom/venus/dbgfs.c b/drivers/media/platform/qcom/venus/dbgfs.c
new file mode 100644
index 000000000000..a2465fe8e20b
--- /dev/null
+++ b/drivers/media/platform/qcom/venus/dbgfs.c
@@ -0,0 +1,26 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2020 Linaro Ltd.
+ */
+
+#include <linux/debugfs.h>
+
+#include "core.h"
+
+extern int venus_fw_debug;
+
+int venus_dbgfs_init(struct venus_core *core)
+{
+	core->root = debugfs_create_dir("venus", NULL);
+	if (IS_ERR(core->root))
+		return IS_ERR(core->root);
+
+	debugfs_create_x32("fw_level", 0644, core->root, &venus_fw_debug);
+
+	return 0;
+}
+
+void venus_dbgfs_deinit(struct venus_core *core)
+{
+	debugfs_remove_recursive(core->root);
+}
diff --git a/drivers/media/platform/qcom/venus/dbgfs.h b/drivers/media/platform/qcom/venus/dbgfs.h
new file mode 100644
index 000000000000..4e35bd7db15f
--- /dev/null
+++ b/drivers/media/platform/qcom/venus/dbgfs.h
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (C) 2020 Linaro Ltd. */
+
+#ifndef __VENUS_DBGFS_H__
+#define __VENUS_DBGFS_H__
+
+struct venus_core;
+
+int venus_dbgfs_init(struct venus_core *core);
+void venus_dbgfs_deinit(struct venus_core *core);
+
+#endif
diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
index 0d8855014ab3..3a04b08ab85a 100644
--- a/drivers/media/platform/qcom/venus/hfi_venus.c
+++ b/drivers/media/platform/qcom/venus/hfi_venus.c
@@ -130,7 +130,7 @@ struct venus_hfi_device {
 };
 
 static bool venus_pkt_debug;
-static int venus_fw_debug = HFI_DEBUG_MSG_ERROR | HFI_DEBUG_MSG_FATAL;
+int venus_fw_debug = HFI_DEBUG_MSG_ERROR | HFI_DEBUG_MSG_FATAL;
 static bool venus_sys_idle_indicator;
 static bool venus_fw_low_power_mode = true;
 static int venus_hw_rsp_timeout = 1000;
@@ -1130,9 +1130,14 @@ static int venus_session_init(struct venus_inst *inst, u32 session_type,
 			      u32 codec)
 {
 	struct venus_hfi_device *hdev = to_hfi_priv(inst->core);
+	struct device *dev = hdev->core->dev;
 	struct hfi_session_init_pkt pkt;
 	int ret;
 
+	ret = venus_sys_set_debug(hdev, venus_fw_debug);
+	if (ret)
+		dev_warn(dev, "setting fw debug msg ON failed (%d)\n", ret);
+
 	ret = pkt_session_init(&pkt, inst, session_type, codec);
 	if (ret)
 		goto err;
-- 
2.17.1

Re: [PATCH v3 5/7] venus: Add debugfs interface to set firmware log level

[Greg Kroah-Hartman]

On Tue, Jun 09, 2020 at 01:46:02PM +0300, Stanimir Varbanov wrote:
> +int venus_dbgfs_init(struct venus_core *core)
> +{
> +	core->root = debugfs_create_dir("venus", NULL);
> +	if (IS_ERR(core->root))
> +		return IS_ERR(core->root);

You really do not care, and obviously did not test this on a system with
CONFIG_DEBUG_FS disabled :)

Just make the call to debugfs, and move on, feed it into other debugfs
calls, all is good.

This function should just return 'void', no need to care about this at
all.

> +	ret = venus_sys_set_debug(hdev, venus_fw_debug);
> +	if (ret)
> +		dev_warn(dev, "setting fw debug msg ON failed (%d)\n", ret);

Why do you care about this "error"?

thanks,

greg k-h

Re: [PATCH v3 5/7] venus: Add debugfs interface to set firmware log level

[Stanimir Varbanov]

Hi Greg,

Thanks for the comments!

On 6/9/20 2:12 PM, Greg Kroah-Hartman wrote:
> On Tue, Jun 09, 2020 at 01:46:02PM +0300, Stanimir Varbanov wrote:
>> +int venus_dbgfs_init(struct venus_core *core)
>> +{
>> +	core->root = debugfs_create_dir("venus", NULL);
>> +	if (IS_ERR(core->root))
>> +		return IS_ERR(core->root);
> 
> You really do not care, and obviously did not test this on a system with
> CONFIG_DEBUG_FS disabled :)

Probably not :(

> 
> Just make the call to debugfs, and move on, feed it into other debugfs
> calls, all is good.
> 
> This function should just return 'void', no need to care about this at
> all.
> 
>> +	ret = venus_sys_set_debug(hdev, venus_fw_debug);
>> +	if (ret)
>> +		dev_warn(dev, "setting fw debug msg ON failed (%d)\n", ret);
> 
> Why do you care about this "error"?

I don't care so much, that's why it is dev_warn. But if I enable debug
messages from the firmware and I don't see them this warn will give me
an idea why.


-- 
regards,
Stan

[PATCH v3 6/7] venus: Make debug infrastructure more flexible

[Stanimir Varbanov]

Here we introduce few debug macros with levels (low, medium and
high) and debug macro for firmware. Enabling the particular level
will be done by dynamic debug with levels.

For example to enable debug messages with low level:
echo 'module venus_dec level 0x01 +p' > debugfs/dynamic_debug/control

If you want to enable all levels:
echo 'module venus_dec level 0x07 +p' > debugfs/dynamic_debug/control

All the features which dynamic debugging provide are preserved.

And finaly all dev_dbg are translated to VDBGX with appropriate
debug levels.

Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
---
 drivers/media/platform/qcom/venus/core.h      |  5 ++
 drivers/media/platform/qcom/venus/helpers.c   |  2 +-
 drivers/media/platform/qcom/venus/hfi_msgs.c  | 30 ++++-----
 drivers/media/platform/qcom/venus/hfi_venus.c | 20 ++++--
 .../media/platform/qcom/venus/pm_helpers.c    |  3 +-
 drivers/media/platform/qcom/venus/vdec.c      | 63 +++++++++++++++++--
 drivers/media/platform/qcom/venus/venc.c      |  4 ++
 7 files changed, 96 insertions(+), 31 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
index b48782f9aa95..63eabf5ff96d 100644
--- a/drivers/media/platform/qcom/venus/core.h
+++ b/drivers/media/platform/qcom/venus/core.h
@@ -15,6 +15,11 @@
 #include "dbgfs.h"
 #include "hfi.h"
 
+#define VDBGL(fmt, args...)	pr_debug_level(0x01, fmt, ##args)
+#define VDBGM(fmt, args...)	pr_debug_level(0x02, fmt, ##args)
+#define VDBGH(fmt, args...)	pr_debug_level(0x04, fmt, ##args)
+#define VDBGFW(fmt, args...)	pr_debug_level(0x08, fmt, ##args)
+
 #define VIDC_CLKS_NUM_MAX		4
 #define VIDC_VCODEC_CLKS_NUM_MAX	2
 #define VIDC_PMDOMAINS_NUM_MAX		3
diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c
index 0143af7822b2..115a9a2af1d6 100644
--- a/drivers/media/platform/qcom/venus/helpers.c
+++ b/drivers/media/platform/qcom/venus/helpers.c
@@ -396,7 +396,7 @@ put_ts_metadata(struct venus_inst *inst, struct vb2_v4l2_buffer *vbuf)
 	}
 
 	if (slot == -1) {
-		dev_dbg(inst->core->dev, "%s: no free slot\n", __func__);
+		VDBGH("no free slot for timestamp\n");
 		return;
 	}
 
diff --git a/drivers/media/platform/qcom/venus/hfi_msgs.c b/drivers/media/platform/qcom/venus/hfi_msgs.c
index 279a9d6fe737..36986d402c96 100644
--- a/drivers/media/platform/qcom/venus/hfi_msgs.c
+++ b/drivers/media/platform/qcom/venus/hfi_msgs.c
@@ -138,10 +138,9 @@ static void event_sys_error(struct venus_core *core, u32 event,
 			    struct hfi_msg_event_notify_pkt *pkt)
 {
 	if (pkt)
-		dev_dbg(core->dev,
-			"sys error (session id:%x, data1:%x, data2:%x)\n",
-			pkt->shdr.session_id, pkt->event_data1,
-			pkt->event_data2);
+		VDBGH("sys error (session id: %x, data1: %x, data2: %x)\n",
+		      pkt->shdr.session_id, pkt->event_data1,
+		      pkt->event_data2);
 
 	core->core_ops->event_notify(core, event);
 }
@@ -152,8 +151,8 @@ event_session_error(struct venus_core *core, struct venus_inst *inst,
 {
 	struct device *dev = core->dev;
 
-	dev_dbg(dev, "session error: event id:%x, session id:%x\n",
-		pkt->event_data1, pkt->shdr.session_id);
+	VDBGH("session error: event id: %x, session id: %x\n",
+	      pkt->event_data1, pkt->shdr.session_id);
 
 	if (!inst)
 		return;
@@ -236,8 +235,7 @@ static void hfi_sys_init_done(struct venus_core *core, struct venus_inst *inst,
 }
 
 static void
-sys_get_prop_image_version(struct device *dev,
-			   struct hfi_msg_sys_property_info_pkt *pkt)
+sys_get_prop_image_version(struct hfi_msg_sys_property_info_pkt *pkt)
 {
 	int req_bytes;
 
@@ -247,26 +245,25 @@ sys_get_prop_image_version(struct device *dev,
 		/* bad packet */
 		return;
 
-	dev_dbg(dev, "F/W version: %s\n", (u8 *)&pkt->data[1]);
+	VDBGL("F/W version: %s\n", (u8 *)&pkt->data[1]);
 }
 
 static void hfi_sys_property_info(struct venus_core *core,
 				  struct venus_inst *inst, void *packet)
 {
 	struct hfi_msg_sys_property_info_pkt *pkt = packet;
-	struct device *dev = core->dev;
 
 	if (!pkt->num_properties) {
-		dev_dbg(dev, "%s: no properties\n", __func__);
+		VDBGM("no properties\n");
 		return;
 	}
 
 	switch (pkt->data[0]) {
 	case HFI_PROPERTY_SYS_IMAGE_VERSION:
-		sys_get_prop_image_version(dev, pkt);
+		sys_get_prop_image_version(pkt);
 		break;
 	default:
-		dev_dbg(dev, "%s: unknown property data\n", __func__);
+		VDBGM("unknown property data\n");
 		break;
 	}
 }
@@ -297,7 +294,7 @@ static void hfi_sys_ping_done(struct venus_core *core, struct venus_inst *inst,
 static void hfi_sys_idle_done(struct venus_core *core, struct venus_inst *inst,
 			      void *packet)
 {
-	dev_dbg(core->dev, "sys idle\n");
+	VDBGL("sys idle\n");
 }
 
 static void hfi_sys_pc_prepare_done(struct venus_core *core,
@@ -305,7 +302,7 @@ static void hfi_sys_pc_prepare_done(struct venus_core *core,
 {
 	struct hfi_msg_sys_pc_prep_done_pkt *pkt = packet;
 
-	dev_dbg(core->dev, "pc prepare done (error %x)\n", pkt->error_type);
+	VDBGL("pc prepare done (error %x)\n", pkt->error_type);
 }
 
 static unsigned int
@@ -387,8 +384,7 @@ static void hfi_session_prop_info(struct venus_core *core,
 	case HFI_PROPERTY_CONFIG_VDEC_ENTROPY:
 		break;
 	default:
-		dev_dbg(dev, "%s: unknown property id:%x\n", __func__,
-			pkt->data[0]);
+		VDBGH("unknown property id: %x\n", pkt->data[0]);
 		return;
 	}
 
diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
index 3a04b08ab85a..9aef62f9b59a 100644
--- a/drivers/media/platform/qcom/venus/hfi_venus.c
+++ b/drivers/media/platform/qcom/venus/hfi_venus.c
@@ -467,7 +467,6 @@ static int venus_boot_core(struct venus_hfi_device *hdev)
 
 static u32 venus_hwversion(struct venus_hfi_device *hdev)
 {
-	struct device *dev = hdev->core->dev;
 	u32 ver = venus_readl(hdev, WRAPPER_HW_VERSION);
 	u32 major, minor, step;
 
@@ -477,7 +476,7 @@ static u32 venus_hwversion(struct venus_hfi_device *hdev)
 	minor = minor >> WRAPPER_HW_VERSION_MINOR_VERSION_SHIFT;
 	step = ver & WRAPPER_HW_VERSION_STEP_VERSION_MASK;
 
-	dev_dbg(dev, "venus hw version %x.%x.%x\n", major, minor, step);
+	VDBGL("venus hw version %x.%x.%x\n", major, minor, step);
 
 	return major;
 }
@@ -897,7 +896,6 @@ static int venus_session_cmd(struct venus_inst *inst, u32 pkt_type)
 
 static void venus_flush_debug_queue(struct venus_hfi_device *hdev)
 {
-	struct device *dev = hdev->core->dev;
 	void *packet = hdev->dbg_buf;
 
 	while (!venus_iface_dbgq_read(hdev, packet)) {
@@ -906,7 +904,7 @@ static void venus_flush_debug_queue(struct venus_hfi_device *hdev)
 		if (pkt->hdr.pkt_type != HFI_MSG_SYS_COV) {
 			struct hfi_msg_sys_debug_pkt *pkt = packet;
 
-			dev_dbg(dev, "%s", pkt->msg_data);
+			VDBGFW("%s", pkt->msg_data);
 		}
 	}
 }
@@ -1230,6 +1228,11 @@ static int venus_session_etb(struct venus_inst *inst,
 		ret = -EINVAL;
 	}
 
+	VDBGM("etb: %s: itag: %u, flen: %u, addr: %x\n",
+	      session_type == VIDC_SESSION_TYPE_DEC ? "dec" : "enc",
+	      in_frame->clnt_data, in_frame->filled_len,
+	      in_frame->device_addr);
+
 	return ret;
 }
 
@@ -1244,7 +1247,14 @@ static int venus_session_ftb(struct venus_inst *inst,
 	if (ret)
 		return ret;
 
-	return venus_iface_cmdq_write(hdev, &pkt);
+	ret = venus_iface_cmdq_write(hdev, &pkt);
+
+	VDBGM("ftb: %s: otag: %u, flen: %u, addr: %x\n",
+	      inst->session_type == VIDC_SESSION_TYPE_DEC ? "dec" : "enc",
+	      out_frame->clnt_data, out_frame->filled_len,
+	      out_frame->device_addr);
+
+	return ret;
 }
 
 static int venus_session_set_buffers(struct venus_inst *inst,
diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
index abf93158857b..ec7394615ef8 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -212,8 +212,7 @@ static int load_scale_bw(struct venus_core *core)
 	}
 	mutex_unlock(&core->lock);
 
-	dev_dbg(core->dev, "total: avg_bw: %u, peak_bw: %u\n",
-		total_avg, total_peak);
+	VDBGL("total: avg_bw: %u, peak_bw: %u\n", total_avg, total_peak);
 
 	return icc_set_bw(core->video_path, total_avg, total_peak);
 }
diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
index 7c4c483d5438..7959e452fbf3 100644
--- a/drivers/media/platform/qcom/venus/vdec.c
+++ b/drivers/media/platform/qcom/venus/vdec.c
@@ -225,7 +225,7 @@ static int vdec_check_src_change(struct venus_inst *inst)
 
 	if (!(inst->codec_state == VENUS_DEC_STATE_CAPTURE_SETUP) ||
 	    !inst->reconfig)
-		dev_dbg(inst->core->dev, "%s: wrong state\n", __func__);
+		VDBGM("wrong codec state %u\n", inst->codec_state);
 
 done:
 	return 0;
@@ -790,6 +790,10 @@ static int vdec_queue_setup(struct vb2_queue *q,
 	unsigned int in_num, out_num;
 	int ret = 0;
 
+	VDBGM("vb2: queue_setup: %s: begin (codec_state: %u)\n",
+	      V4L2_TYPE_IS_OUTPUT(q->type) ? "out" : "cap",
+	      inst->codec_state);
+
 	if (*num_planes) {
 		unsigned int output_buf_size = venus_helper_get_opb_size(inst);
 
@@ -859,6 +863,10 @@ static int vdec_queue_setup(struct vb2_queue *q,
 		break;
 	}
 
+	VDBGM("vb2: queue_setup: %s: end (codec_state: %u, ret: %d)\n",
+	      V4L2_TYPE_IS_OUTPUT(q->type) ? "out" : "cap",
+	      inst->codec_state, ret);
+
 	return ret;
 
 put_power:
@@ -897,6 +905,8 @@ static int vdec_start_capture(struct venus_inst *inst)
 {
 	int ret;
 
+	VDBGM("on: cap: begin (codec_state: %u)\n", inst->codec_state);
+
 	if (!inst->streamon_out)
 		return 0;
 
@@ -955,11 +965,16 @@ static int vdec_start_capture(struct venus_inst *inst)
 	inst->sequence_cap = 0;
 	inst->reconfig = false;
 
+	VDBGM("on: cap: end (codec_state: %u)\n", inst->codec_state);
+
 	return 0;
 
 free_dpb_bufs:
 	venus_helper_free_dpb_bufs(inst);
 err:
+	VDBGM("on: cap: end (codec_state: %u, ret: %d)\n",
+	      inst->codec_state, ret);
+
 	return ret;
 }
 
@@ -967,6 +982,8 @@ static int vdec_start_output(struct venus_inst *inst)
 {
 	int ret;
 
+	VDBGM("on: out: begin (codec_state: %u)\n", inst->codec_state);
+
 	if (inst->codec_state == VENUS_DEC_STATE_SEEK) {
 		ret = venus_helper_process_initial_out_bufs(inst);
 		inst->codec_state = VENUS_DEC_STATE_DECODING;
@@ -1015,6 +1032,10 @@ static int vdec_start_output(struct venus_inst *inst)
 
 done:
 	inst->streamon_out = 1;
+
+	VDBGM("on: out: end (codec_state: %u, ret: %d)\n",
+	      inst->codec_state, ret);
+
 	return ret;
 }
 
@@ -1069,6 +1090,8 @@ static int vdec_stop_capture(struct venus_inst *inst)
 {
 	int ret = 0;
 
+	VDBGM("off: cap: begin (codec_state: %u)\n", inst->codec_state);
+
 	switch (inst->codec_state) {
 	case VENUS_DEC_STATE_DECODING:
 		ret = hfi_session_flush(inst, HFI_FLUSH_ALL, true);
@@ -1090,6 +1113,9 @@ static int vdec_stop_capture(struct venus_inst *inst)
 
 	INIT_LIST_HEAD(&inst->registeredbufs);
 
+	VDBGM("off: cap: end (codec_state: %u, ret: %d)\n",
+	      inst->codec_state, ret);
+
 	return ret;
 }
 
@@ -1097,6 +1123,8 @@ static int vdec_stop_output(struct venus_inst *inst)
 {
 	int ret = 0;
 
+	VDBGM("off: out: begin (codec_state: %u)\n", inst->codec_state);
+
 	switch (inst->codec_state) {
 	case VENUS_DEC_STATE_DECODING:
 	case VENUS_DEC_STATE_DRAIN:
@@ -1112,6 +1140,9 @@ static int vdec_stop_output(struct venus_inst *inst)
 		break;
 	}
 
+	VDBGM("off: out: end (codec_state: %u, ret %d)\n",
+	      inst->codec_state, ret);
+
 	return ret;
 }
 
@@ -1146,6 +1177,8 @@ static void vdec_session_release(struct venus_inst *inst)
 	struct venus_core *core = inst->core;
 	int ret, abort = 0;
 
+	VDBGM("rel: begin (codec_state: %u)\n", inst->codec_state);
+
 	vdec_pm_get(inst);
 
 	mutex_lock(&inst->lock);
@@ -1175,15 +1208,23 @@ static void vdec_session_release(struct venus_inst *inst)
 
 	venus_pm_release_core(inst);
 	vdec_pm_put(inst, false);
+
+	VDBGM("rel: end (codec_state: %u)\n", inst->codec_state);
 }
 
 static int vdec_buf_init(struct vb2_buffer *vb)
 {
 	struct venus_inst *inst = vb2_get_drv_priv(vb->vb2_queue);
+	int ret;
 
 	inst->buf_count++;
 
-	return venus_helper_vb2_buf_init(vb);
+	ret = venus_helper_vb2_buf_init(vb);
+
+	VDBGM("vb2: buf_init: %s: done (codec_state: %u)\n",
+	      V4L2_TYPE_IS_OUTPUT(vb->type) ? "out" : "cap", inst->codec_state);
+
+	return ret;
 }
 
 static void vdec_buf_cleanup(struct vb2_buffer *vb)
@@ -1193,6 +1234,9 @@ static void vdec_buf_cleanup(struct vb2_buffer *vb)
 	inst->buf_count--;
 	if (!inst->buf_count)
 		vdec_session_release(inst);
+
+	VDBGM("vb2: buf_cleanup: %s: done (codec_state: %u)\n",
+	      V4L2_TYPE_IS_OUTPUT(vb->type) ? "out" : "cap", inst->codec_state);
 }
 
 static void vdec_vb2_buf_queue(struct vb2_buffer *vb)
@@ -1281,6 +1325,10 @@ static void vdec_buf_done(struct venus_inst *inst, unsigned int buf_type,
 	}
 
 	v4l2_m2m_buf_done(vbuf, state);
+
+	VDBGH("done: %s, idx: %02u, flen: %08u, flags: hfi: %08x, v4l2: %08x\n",
+	      V4L2_TYPE_IS_OUTPUT(type) ? "out" : "cap",
+	      vbuf->vb2_buf.index, bytesused, hfi_flags, vbuf->flags);
 }
 
 static void vdec_event_change(struct venus_inst *inst,
@@ -1289,7 +1337,6 @@ static void vdec_event_change(struct venus_inst *inst,
 	static const struct v4l2_event ev = {
 		.type = V4L2_EVENT_SOURCE_CHANGE,
 		.u.src_change.changes = V4L2_EVENT_SRC_CH_RESOLUTION };
-	struct device *dev = inst->core->dev_dec;
 	struct v4l2_format format = {};
 
 	mutex_lock(&inst->lock);
@@ -1310,8 +1357,12 @@ static void vdec_event_change(struct venus_inst *inst,
 	if (inst->bit_depth != ev_data->bit_depth)
 		inst->bit_depth = ev_data->bit_depth;
 
-	dev_dbg(dev, "event %s sufficient resources (%ux%u)\n",
-		sufficient ? "" : "not", ev_data->width, ev_data->height);
+	VDBGH("event: %s sufficient resources (%ux%u)\n",
+	      sufficient ? "" : "not", ev_data->width, ev_data->height);
+
+	if (ev_data->buf_count)
+		VDBGH("event: buf_count: %u, old: %u\n",
+		      ev_data->buf_count, inst->num_output_bufs);
 
 	if (sufficient) {
 		hfi_session_continue(inst);
@@ -1344,7 +1395,7 @@ static void vdec_event_change(struct venus_inst *inst,
 
 		ret = hfi_session_flush(inst, HFI_FLUSH_OUTPUT, false);
 		if (ret)
-			dev_dbg(dev, "flush output error %d\n", ret);
+			VDBGH("flush output error (%d)\n", ret);
 	}
 
 	inst->reconfig = true;
diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
index feed648550d1..c591d00ee0a7 100644
--- a/drivers/media/platform/qcom/venus/venc.c
+++ b/drivers/media/platform/qcom/venus/venc.c
@@ -1074,6 +1074,10 @@ static void venc_buf_done(struct venus_inst *inst, unsigned int buf_type,
 	}
 
 	v4l2_m2m_buf_done(vbuf, VB2_BUF_STATE_DONE);
+
+	VDBGH("done: %s, idx: %02u, flen: %08u, flags: hfi: %08x, v4l2: %08x\n",
+	      V4L2_TYPE_IS_OUTPUT(type) ? "out" : "cap",
+	      vbuf->vb2_buf.index, bytesused, hfi_flags, vbuf->flags);
 }
 
 static void venc_event_notify(struct venus_inst *inst, u32 event,
-- 
2.17.1

Re: [PATCH v3 6/7] venus: Make debug infrastructure more flexible

[Greg Kroah-Hartman]

On Tue, Jun 09, 2020 at 01:46:03PM +0300, Stanimir Varbanov wrote:
> Here we introduce few debug macros with levels (low, medium and
> high) and debug macro for firmware. Enabling the particular level
> will be done by dynamic debug with levels.
> 
> For example to enable debug messages with low level:
> echo 'module venus_dec level 0x01 +p' > debugfs/dynamic_debug/control
> 
> If you want to enable all levels:
> echo 'module venus_dec level 0x07 +p' > debugfs/dynamic_debug/control
> 
> All the features which dynamic debugging provide are preserved.
> 
> And finaly all dev_dbg are translated to VDBGX with appropriate
> debug levels.
> 
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> ---
>  drivers/media/platform/qcom/venus/core.h      |  5 ++
>  drivers/media/platform/qcom/venus/helpers.c   |  2 +-
>  drivers/media/platform/qcom/venus/hfi_msgs.c  | 30 ++++-----
>  drivers/media/platform/qcom/venus/hfi_venus.c | 20 ++++--
>  .../media/platform/qcom/venus/pm_helpers.c    |  3 +-
>  drivers/media/platform/qcom/venus/vdec.c      | 63 +++++++++++++++++--
>  drivers/media/platform/qcom/venus/venc.c      |  4 ++
>  7 files changed, 96 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
> index b48782f9aa95..63eabf5ff96d 100644
> --- a/drivers/media/platform/qcom/venus/core.h
> +++ b/drivers/media/platform/qcom/venus/core.h
> @@ -15,6 +15,11 @@
>  #include "dbgfs.h"
>  #include "hfi.h"
>  
> +#define VDBGL(fmt, args...)	pr_debug_level(0x01, fmt, ##args)
> +#define VDBGM(fmt, args...)	pr_debug_level(0x02, fmt, ##args)
> +#define VDBGH(fmt, args...)	pr_debug_level(0x04, fmt, ##args)
> +#define VDBGFW(fmt, args...)	pr_debug_level(0x08, fmt, ##args)
> +
>  #define VIDC_CLKS_NUM_MAX		4
>  #define VIDC_VCODEC_CLKS_NUM_MAX	2
>  #define VIDC_PMDOMAINS_NUM_MAX		3
> diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c
> index 0143af7822b2..115a9a2af1d6 100644
> --- a/drivers/media/platform/qcom/venus/helpers.c
> +++ b/drivers/media/platform/qcom/venus/helpers.c
> @@ -396,7 +396,7 @@ put_ts_metadata(struct venus_inst *inst, struct vb2_v4l2_buffer *vbuf)
>  	}
>  
>  	if (slot == -1) {
> -		dev_dbg(inst->core->dev, "%s: no free slot\n", __func__);
> +		VDBGH("no free slot for timestamp\n");

So you just lost the information that dev_dbg() gave you with regards to
the device/driver/instance creating that message?

Ick, no, don't do that.

And why is this driver somehow "special" compared to all the rest of
the kernel?  Why is the current dev_dbg() control not sufficient that
you need to change the core for just this tiny thing?

thanks,

greg k-h

Re: [PATCH v3 6/7] venus: Make debug infrastructure more flexible

[Stanimir Varbanov]

On 6/9/20 2:14 PM, Greg Kroah-Hartman wrote:
> On Tue, Jun 09, 2020 at 01:46:03PM +0300, Stanimir Varbanov wrote:
>> Here we introduce few debug macros with levels (low, medium and
>> high) and debug macro for firmware. Enabling the particular level
>> will be done by dynamic debug with levels.
>>
>> For example to enable debug messages with low level:
>> echo 'module venus_dec level 0x01 +p' > debugfs/dynamic_debug/control
>>
>> If you want to enable all levels:
>> echo 'module venus_dec level 0x07 +p' > debugfs/dynamic_debug/control
>>
>> All the features which dynamic debugging provide are preserved.
>>
>> And finaly all dev_dbg are translated to VDBGX with appropriate
>> debug levels.
>>
>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
>> ---
>>  drivers/media/platform/qcom/venus/core.h      |  5 ++
>>  drivers/media/platform/qcom/venus/helpers.c   |  2 +-
>>  drivers/media/platform/qcom/venus/hfi_msgs.c  | 30 ++++-----
>>  drivers/media/platform/qcom/venus/hfi_venus.c | 20 ++++--
>>  .../media/platform/qcom/venus/pm_helpers.c    |  3 +-
>>  drivers/media/platform/qcom/venus/vdec.c      | 63 +++++++++++++++++--
>>  drivers/media/platform/qcom/venus/venc.c      |  4 ++
>>  7 files changed, 96 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
>> index b48782f9aa95..63eabf5ff96d 100644
>> --- a/drivers/media/platform/qcom/venus/core.h
>> +++ b/drivers/media/platform/qcom/venus/core.h
>> @@ -15,6 +15,11 @@
>>  #include "dbgfs.h"
>>  #include "hfi.h"
>>  
>> +#define VDBGL(fmt, args...)	pr_debug_level(0x01, fmt, ##args)
>> +#define VDBGM(fmt, args...)	pr_debug_level(0x02, fmt, ##args)
>> +#define VDBGH(fmt, args...)	pr_debug_level(0x04, fmt, ##args)
>> +#define VDBGFW(fmt, args...)	pr_debug_level(0x08, fmt, ##args)
>> +
>>  #define VIDC_CLKS_NUM_MAX		4
>>  #define VIDC_VCODEC_CLKS_NUM_MAX	2
>>  #define VIDC_PMDOMAINS_NUM_MAX		3
>> diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c
>> index 0143af7822b2..115a9a2af1d6 100644
>> --- a/drivers/media/platform/qcom/venus/helpers.c
>> +++ b/drivers/media/platform/qcom/venus/helpers.c
>> @@ -396,7 +396,7 @@ put_ts_metadata(struct venus_inst *inst, struct vb2_v4l2_buffer *vbuf)
>>  	}
>>  
>>  	if (slot == -1) {
>> -		dev_dbg(inst->core->dev, "%s: no free slot\n", __func__);
>> +		VDBGH("no free slot for timestamp\n");
> 
> So you just lost the information that dev_dbg() gave you with regards to
> the device/driver/instance creating that message?

No, I don't lose anything.  When I do debug I know that all debug
messages comes from my driver.  dev_dbg will give me few device
identifiers which I don't care so much. IMO, the device information
makes more sense to dev_err/warn/err variants.  On the other side we
will have dev_dbg_level(group) if still someone needs the device
information.

> 
> Ick, no, don't do that.
> 
> And why is this driver somehow "special" compared to all the rest of

Of course it is special ... to me ;-)

> the kernel?  Why is the current dev_dbg() control not sufficient that
> you need to change the core for just this tiny thing?
> 
> thanks,
> 
> greg k-h
> 

-- 
regards,
Stan

Re: [PATCH v3 6/7] venus: Make debug infrastructure more flexible

[Greg Kroah-Hartman]

On Wed, Jun 10, 2020 at 04:29:27PM +0300, Stanimir Varbanov wrote:
> 
> 
> On 6/9/20 2:14 PM, Greg Kroah-Hartman wrote:
> > On Tue, Jun 09, 2020 at 01:46:03PM +0300, Stanimir Varbanov wrote:
> >> Here we introduce few debug macros with levels (low, medium and
> >> high) and debug macro for firmware. Enabling the particular level
> >> will be done by dynamic debug with levels.
> >>
> >> For example to enable debug messages with low level:
> >> echo 'module venus_dec level 0x01 +p' > debugfs/dynamic_debug/control
> >>
> >> If you want to enable all levels:
> >> echo 'module venus_dec level 0x07 +p' > debugfs/dynamic_debug/control
> >>
> >> All the features which dynamic debugging provide are preserved.
> >>
> >> And finaly all dev_dbg are translated to VDBGX with appropriate
> >> debug levels.
> >>
> >> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> >> ---
> >>  drivers/media/platform/qcom/venus/core.h      |  5 ++
> >>  drivers/media/platform/qcom/venus/helpers.c   |  2 +-
> >>  drivers/media/platform/qcom/venus/hfi_msgs.c  | 30 ++++-----
> >>  drivers/media/platform/qcom/venus/hfi_venus.c | 20 ++++--
> >>  .../media/platform/qcom/venus/pm_helpers.c    |  3 +-
> >>  drivers/media/platform/qcom/venus/vdec.c      | 63 +++++++++++++++++--
> >>  drivers/media/platform/qcom/venus/venc.c      |  4 ++
> >>  7 files changed, 96 insertions(+), 31 deletions(-)
> >>
> >> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
> >> index b48782f9aa95..63eabf5ff96d 100644
> >> --- a/drivers/media/platform/qcom/venus/core.h
> >> +++ b/drivers/media/platform/qcom/venus/core.h
> >> @@ -15,6 +15,11 @@
> >>  #include "dbgfs.h"
> >>  #include "hfi.h"
> >>  
> >> +#define VDBGL(fmt, args...)	pr_debug_level(0x01, fmt, ##args)
> >> +#define VDBGM(fmt, args...)	pr_debug_level(0x02, fmt, ##args)
> >> +#define VDBGH(fmt, args...)	pr_debug_level(0x04, fmt, ##args)
> >> +#define VDBGFW(fmt, args...)	pr_debug_level(0x08, fmt, ##args)
> >> +
> >>  #define VIDC_CLKS_NUM_MAX		4
> >>  #define VIDC_VCODEC_CLKS_NUM_MAX	2
> >>  #define VIDC_PMDOMAINS_NUM_MAX		3
> >> diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c
> >> index 0143af7822b2..115a9a2af1d6 100644
> >> --- a/drivers/media/platform/qcom/venus/helpers.c
> >> +++ b/drivers/media/platform/qcom/venus/helpers.c
> >> @@ -396,7 +396,7 @@ put_ts_metadata(struct venus_inst *inst, struct vb2_v4l2_buffer *vbuf)
> >>  	}
> >>  
> >>  	if (slot == -1) {
> >> -		dev_dbg(inst->core->dev, "%s: no free slot\n", __func__);
> >> +		VDBGH("no free slot for timestamp\n");
> > 
> > So you just lost the information that dev_dbg() gave you with regards to
> > the device/driver/instance creating that message?
> 
> No, I don't lose anything.  When I do debug I know that all debug
> messages comes from my driver.  dev_dbg will give me few device
> identifiers which I don't care so much.

No, you need/want that, trust me.

> IMO, the device information makes more sense to dev_err/warn/err
> variants.  On the other side we will have dev_dbg_level(group) if
> still someone needs the device information.

You really want those "gerneric identifiers" as tools today are built to
properly parse and handle them to be able to match and filter on what
device/driver is causing what issue.

Please do not try to create driver-specific prefixes instead, use the
standard the rest of the kernel uses, your driver is not "special" in
this case at all.

> > Ick, no, don't do that.
> > 
> > And why is this driver somehow "special" compared to all the rest of
> 
> Of course it is special ... to me ;-)

Yes, "special and unique" like all other drivers in the kernel :)

Please work with the infrastructure we have, we have spent a lot of time
and effort to make it uniform to make it easier for users and
developers.  Don't regress and try to make driver-specific ways of doing
things, that way lies madness...

thanks,

greg k-h

Re: [PATCH v3 6/7] venus: Make debug infrastructure more flexible

[Joe Perches]

On Wed, 2020-06-10 at 15:37 +0200, Greg Kroah-Hartman wrote:
> Please work with the infrastructure we have, we have spent a lot of time
> and effort to make it uniform to make it easier for users and
> developers.

Not quite.

This lack of debug grouping by type has been a
_long_ standing issue with drivers.

> Don't regress and try to make driver-specific ways of doing
> things, that way lies madness...

It's not driver specific, it allows driver developers to
better isolate various debug states instead of keeping
lists of specific debug messages and enabling them
individually.

Re: [PATCH v3 6/7] venus: Make debug infrastructure more flexible

[Joe Perches]

On Wed, 2020-06-10 at 12:49 -0700, Joe Perches wrote:
> On Wed, 2020-06-10 at 15:37 +0200, Greg Kroah-Hartman wrote:
> > Please work with the infrastructure we have, we have spent a lot of time
> > and effort to make it uniform to make it easier for users and
> > developers.
> 
> Not quite.
> 
> This lack of debug grouping by type has been a
> _long_ standing issue with drivers.
> 
> > Don't regress and try to make driver-specific ways of doing
> > things, that way lies madness...
> 
> It's not driver specific, it allows driver developers to
> better isolate various debug states instead of keeping
> lists of specific debug messages and enabling them
> individually.

For instance, look at the homebrew content in
drivers/gpu/drm/drm_print.c that does _not_ use
dynamic_debug.

MODULE_PARM_DESC(debug, "Enable debug output, where each bit enables a debug category.\n"
"\t\tBit 0 (0x01)  will enable CORE messages (drm core code)\n"
"\t\tBit 1 (0x02)  will enable DRIVER messages (drm controller code)\n"
"\t\tBit 2 (0x04)  will enable KMS messages (modesetting code)\n"
"\t\tBit 3 (0x08)  will enable PRIME messages (prime code)\n"
"\t\tBit 4 (0x10)  will enable ATOMIC messages (atomic code)\n"
"\t\tBit 5 (0x20)  will enable VBL messages (vblank code)\n"
"\t\tBit 7 (0x80)  will enable LEASE messages (leasing code)\n"
"\t\tBit 8 (0x100) will enable DP messages (displayport code)");
module_param_named(debug, __drm_debug, int, 0600);

void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
		 const char *format, ...)
{
	struct va_format vaf;
	va_list args;

	if (!drm_debug_enabled(category))
		return;

Re: [PATCH v3 6/7] venus: Make debug infrastructure more flexible

[Greg Kroah-Hartman]

On Wed, Jun 10, 2020 at 01:23:56PM -0700, Joe Perches wrote:
> On Wed, 2020-06-10 at 12:49 -0700, Joe Perches wrote:
> > On Wed, 2020-06-10 at 15:37 +0200, Greg Kroah-Hartman wrote:
> > > Please work with the infrastructure we have, we have spent a lot of time
> > > and effort to make it uniform to make it easier for users and
> > > developers.
> > 
> > Not quite.
> > 
> > This lack of debug grouping by type has been a
> > _long_ standing issue with drivers.
> > 
> > > Don't regress and try to make driver-specific ways of doing
> > > things, that way lies madness...
> > 
> > It's not driver specific, it allows driver developers to
> > better isolate various debug states instead of keeping
> > lists of specific debug messages and enabling them
> > individually.
> 
> For instance, look at the homebrew content in
> drivers/gpu/drm/drm_print.c that does _not_ use
> dynamic_debug.
> 
> MODULE_PARM_DESC(debug, "Enable debug output, where each bit enables a debug category.\n"
> "\t\tBit 0 (0x01)  will enable CORE messages (drm core code)\n"
> "\t\tBit 1 (0x02)  will enable DRIVER messages (drm controller code)\n"
> "\t\tBit 2 (0x04)  will enable KMS messages (modesetting code)\n"
> "\t\tBit 3 (0x08)  will enable PRIME messages (prime code)\n"
> "\t\tBit 4 (0x10)  will enable ATOMIC messages (atomic code)\n"
> "\t\tBit 5 (0x20)  will enable VBL messages (vblank code)\n"
> "\t\tBit 7 (0x80)  will enable LEASE messages (leasing code)\n"
> "\t\tBit 8 (0x100) will enable DP messages (displayport code)");
> module_param_named(debug, __drm_debug, int, 0600);
> 
> void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
> 		 const char *format, ...)
> {
> 	struct va_format vaf;
> 	va_list args;
> 
> 	if (!drm_debug_enabled(category))
> 		return;
> 
> 

Ok, and will this proposal be able to handle stuff like this?  If not,
then it is yet another way for driver authors to think that they need to
come up with something unique to themselves. :(

greg k-h

Re: [PATCH v3 6/7] venus: Make debug infrastructure more flexible

[Joe Perches]

On Thu, 2020-06-11 at 08:26 +0200, Greg Kroah-Hartman wrote:
> On Wed, Jun 10, 2020 at 01:23:56PM -0700, Joe Perches wrote:
> > On Wed, 2020-06-10 at 12:49 -0700, Joe Perches wrote:
> > > On Wed, 2020-06-10 at 15:37 +0200, Greg Kroah-Hartman wrote:
> > > > Please work with the infrastructure we have, we have spent a lot of time
> > > > and effort to make it uniform to make it easier for users and
> > > > developers.
> > > 
> > > Not quite.
> > > 
> > > This lack of debug grouping by type has been a
> > > _long_ standing issue with drivers.
> > > 
> > > > Don't regress and try to make driver-specific ways of doing
> > > > things, that way lies madness...
> > > 
> > > It's not driver specific, it allows driver developers to
> > > better isolate various debug states instead of keeping
> > > lists of specific debug messages and enabling them
> > > individually.
> > 
> > For instance, look at the homebrew content in
> > drivers/gpu/drm/drm_print.c that does _not_ use
> > dynamic_debug.
> > 
> > MODULE_PARM_DESC(debug, "Enable debug output, where each bit enables a debug category.\n"
> > "\t\tBit 0 (0x01)  will enable CORE messages (drm core code)\n"
> > "\t\tBit 1 (0x02)  will enable DRIVER messages (drm controller code)\n"
> > "\t\tBit 2 (0x04)  will enable KMS messages (modesetting code)\n"
> > "\t\tBit 3 (0x08)  will enable PRIME messages (prime code)\n"
> > "\t\tBit 4 (0x10)  will enable ATOMIC messages (atomic code)\n"
> > "\t\tBit 5 (0x20)  will enable VBL messages (vblank code)\n"
> > "\t\tBit 7 (0x80)  will enable LEASE messages (leasing code)\n"
> > "\t\tBit 8 (0x100) will enable DP messages (displayport code)");
> > module_param_named(debug, __drm_debug, int, 0600);
> > 
> > void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
> > 		 const char *format, ...)
> > {
> > 	struct va_format vaf;
> > 	va_list args;
> > 
> > 	if (!drm_debug_enabled(category))
> > 		return;
> 
> Ok, and will this proposal be able to handle stuff like this?

Yes, that's the entire point.

If it doesn't have the capability to handle stuff like this,
then no, it wouldn't be a good or useful change.

That includes the ability to work without dynamic debug and
perhaps still use a MODULE_PARM_DESC. 

Re: [PATCH v3 6/7] venus: Make debug infrastructure more flexible

[Daniel Thompson]

On Wed, Jun 10, 2020 at 11:42:43PM -0700, Joe Perches wrote:
> On Thu, 2020-06-11 at 08:26 +0200, Greg Kroah-Hartman wrote:
> > On Wed, Jun 10, 2020 at 01:23:56PM -0700, Joe Perches wrote:
> > > On Wed, 2020-06-10 at 12:49 -0700, Joe Perches wrote:
> > > > On Wed, 2020-06-10 at 15:37 +0200, Greg Kroah-Hartman wrote:
> > > > > Please work with the infrastructure we have, we have spent a lot of time
> > > > > and effort to make it uniform to make it easier for users and
> > > > > developers.
> > > > 
> > > > Not quite.
> > > > 
> > > > This lack of debug grouping by type has been a
> > > > _long_ standing issue with drivers.
> > > > 
> > > > > Don't regress and try to make driver-specific ways of doing
> > > > > things, that way lies madness...
> > > > 
> > > > It's not driver specific, it allows driver developers to
> > > > better isolate various debug states instead of keeping
> > > > lists of specific debug messages and enabling them
> > > > individually.
> > > 
> > > For instance, look at the homebrew content in
> > > drivers/gpu/drm/drm_print.c that does _not_ use
> > > dynamic_debug.
> > > 
> > > MODULE_PARM_DESC(debug, "Enable debug output, where each bit enables a debug category.\n"
> > > "\t\tBit 0 (0x01)  will enable CORE messages (drm core code)\n"
> > > "\t\tBit 1 (0x02)  will enable DRIVER messages (drm controller code)\n"
> > > "\t\tBit 2 (0x04)  will enable KMS messages (modesetting code)\n"
> > > "\t\tBit 3 (0x08)  will enable PRIME messages (prime code)\n"
> > > "\t\tBit 4 (0x10)  will enable ATOMIC messages (atomic code)\n"
> > > "\t\tBit 5 (0x20)  will enable VBL messages (vblank code)\n"
> > > "\t\tBit 7 (0x80)  will enable LEASE messages (leasing code)\n"
> > > "\t\tBit 8 (0x100) will enable DP messages (displayport code)");
> > > module_param_named(debug, __drm_debug, int, 0600);
> > > 
> > > void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
> > > 		 const char *format, ...)
> > > {
> > > 	struct va_format vaf;
> > > 	va_list args;
> > > 
> > > 	if (!drm_debug_enabled(category))
> > > 		return;
> > 
> > Ok, and will this proposal be able to handle stuff like this?
> 
> Yes, that's the entire point.

Currently I think there not enough "levels" to map something like
drm.debug to the new dyn dbg feature. I don't think it is intrinsic
but I couldn't find the bit of the code where the 5-bit level in struct
_ddebug is converted from a mask to a bit number and vice-versa.


Daniel.

Re: [PATCH v3 6/7] venus: Make debug infrastructure more flexible

[Stanimir Varbanov]

On 6/11/20 1:52 PM, Daniel Thompson wrote:
> On Wed, Jun 10, 2020 at 11:42:43PM -0700, Joe Perches wrote:
>> On Thu, 2020-06-11 at 08:26 +0200, Greg Kroah-Hartman wrote:
>>> On Wed, Jun 10, 2020 at 01:23:56PM -0700, Joe Perches wrote:
>>>> On Wed, 2020-06-10 at 12:49 -0700, Joe Perches wrote:
>>>>> On Wed, 2020-06-10 at 15:37 +0200, Greg Kroah-Hartman wrote:
>>>>>> Please work with the infrastructure we have, we have spent a lot of time
>>>>>> and effort to make it uniform to make it easier for users and
>>>>>> developers.
>>>>>
>>>>> Not quite.
>>>>>
>>>>> This lack of debug grouping by type has been a
>>>>> _long_ standing issue with drivers.
>>>>>
>>>>>> Don't regress and try to make driver-specific ways of doing
>>>>>> things, that way lies madness...
>>>>>
>>>>> It's not driver specific, it allows driver developers to
>>>>> better isolate various debug states instead of keeping
>>>>> lists of specific debug messages and enabling them
>>>>> individually.
>>>>
>>>> For instance, look at the homebrew content in
>>>> drivers/gpu/drm/drm_print.c that does _not_ use
>>>> dynamic_debug.
>>>>
>>>> MODULE_PARM_DESC(debug, "Enable debug output, where each bit enables a debug category.\n"
>>>> "\t\tBit 0 (0x01)  will enable CORE messages (drm core code)\n"
>>>> "\t\tBit 1 (0x02)  will enable DRIVER messages (drm controller code)\n"
>>>> "\t\tBit 2 (0x04)  will enable KMS messages (modesetting code)\n"
>>>> "\t\tBit 3 (0x08)  will enable PRIME messages (prime code)\n"
>>>> "\t\tBit 4 (0x10)  will enable ATOMIC messages (atomic code)\n"
>>>> "\t\tBit 5 (0x20)  will enable VBL messages (vblank code)\n"
>>>> "\t\tBit 7 (0x80)  will enable LEASE messages (leasing code)\n"
>>>> "\t\tBit 8 (0x100) will enable DP messages (displayport code)");
>>>> module_param_named(debug, __drm_debug, int, 0600);
>>>>
>>>> void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
>>>> 		 const char *format, ...)
>>>> {
>>>> 	struct va_format vaf;
>>>> 	va_list args;
>>>>
>>>> 	if (!drm_debug_enabled(category))
>>>> 		return;
>>>
>>> Ok, and will this proposal be able to handle stuff like this?
>>
>> Yes, that's the entire point.
> 
> Currently I think there not enough "levels" to map something like
> drm.debug to the new dyn dbg feature. I don't think it is intrinsic
> but I couldn't find the bit of the code where the 5-bit level in struct
> _ddebug is converted from a mask to a bit number and vice-versa.

Here [1] is Joe's initial suggestion. But I decided that bitmask is a
good start for the discussion.

I guess we can add new member uint "level" in struct _ddebug so that we
can cover more "levels" (types, groups).

-- 
regards,
Stan

[1] https://lkml.org/lkml/2020/5/21/915

Re: [PATCH v3 6/7] venus: Make debug infrastructure more flexible

[Daniel Thompson]

On Thu, Jun 11, 2020 at 02:31:07PM +0300, Stanimir Varbanov wrote:
> On 6/11/20 1:52 PM, Daniel Thompson wrote:
> > On Wed, Jun 10, 2020 at 11:42:43PM -0700, Joe Perches wrote:
> >> On Thu, 2020-06-11 at 08:26 +0200, Greg Kroah-Hartman wrote:
> >>> On Wed, Jun 10, 2020 at 01:23:56PM -0700, Joe Perches wrote:
> >>>> On Wed, 2020-06-10 at 12:49 -0700, Joe Perches wrote:
> >>>>> On Wed, 2020-06-10 at 15:37 +0200, Greg Kroah-Hartman wrote:
> >>>>>> Please work with the infrastructure we have, we have spent a lot of time
> >>>>>> and effort to make it uniform to make it easier for users and
> >>>>>> developers.
> >>>>>
> >>>>> Not quite.
> >>>>>
> >>>>> This lack of debug grouping by type has been a
> >>>>> _long_ standing issue with drivers.
> >>>>>
> >>>>>> Don't regress and try to make driver-specific ways of doing
> >>>>>> things, that way lies madness...
> >>>>>
> >>>>> It's not driver specific, it allows driver developers to
> >>>>> better isolate various debug states instead of keeping
> >>>>> lists of specific debug messages and enabling them
> >>>>> individually.
> >>>>
> >>>> For instance, look at the homebrew content in
> >>>> drivers/gpu/drm/drm_print.c that does _not_ use
> >>>> dynamic_debug.
> >>>>
> >>>> MODULE_PARM_DESC(debug, "Enable debug output, where each bit enables a debug category.\n"
> >>>> "\t\tBit 0 (0x01)  will enable CORE messages (drm core code)\n"
> >>>> "\t\tBit 1 (0x02)  will enable DRIVER messages (drm controller code)\n"
> >>>> "\t\tBit 2 (0x04)  will enable KMS messages (modesetting code)\n"
> >>>> "\t\tBit 3 (0x08)  will enable PRIME messages (prime code)\n"
> >>>> "\t\tBit 4 (0x10)  will enable ATOMIC messages (atomic code)\n"
> >>>> "\t\tBit 5 (0x20)  will enable VBL messages (vblank code)\n"
> >>>> "\t\tBit 7 (0x80)  will enable LEASE messages (leasing code)\n"
> >>>> "\t\tBit 8 (0x100) will enable DP messages (displayport code)");
> >>>> module_param_named(debug, __drm_debug, int, 0600);
> >>>>
> >>>> void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
> >>>> 		 const char *format, ...)
> >>>> {
> >>>> 	struct va_format vaf;
> >>>> 	va_list args;
> >>>>
> >>>> 	if (!drm_debug_enabled(category))
> >>>> 		return;
> >>>
> >>> Ok, and will this proposal be able to handle stuff like this?
> >>
> >> Yes, that's the entire point.
> > 
> > Currently I think there not enough "levels" to map something like
> > drm.debug to the new dyn dbg feature. I don't think it is intrinsic
> > but I couldn't find the bit of the code where the 5-bit level in struct
> > _ddebug is converted from a mask to a bit number and vice-versa.
> 
> Here [1] is Joe's initial suggestion. But I decided that bitmask is a
> good start for the discussion.
> 
> I guess we can add new member uint "level" in struct _ddebug so that we
> can cover more "levels" (types, groups).

I don't think it is allocating only 5 bits that is the problem!

The problem is that those 5 bits need not be encoded as a bitmask by
dyndbg, that can simply be the category code for the message. They only
need be converted into a mask when we compare them to the mask provided
by the user.


Daniel.

Re: [PATCH v3 6/7] venus: Make debug infrastructure more flexible

[jim.cromie]

trimmed..

> > > Currently I think there not enough "levels" to map something like
> > > drm.debug to the new dyn dbg feature. I don't think it is intrinsic
> > > but I couldn't find the bit of the code where the 5-bit level in struct
> > > _ddebug is converted from a mask to a bit number and vice-versa.
> >
> > Here [1] is Joe's initial suggestion. But I decided that bitmask is a
> > good start for the discussion.
> >
> > I guess we can add new member uint "level" in struct _ddebug so that we
> > can cover more "levels" (types, groups).
>
> I don't think it is allocating only 5 bits that is the problem!
>
> The problem is that those 5 bits need not be encoded as a bitmask by
> dyndbg, that can simply be the category code for the message. They only
> need be converted into a mask when we compare them to the mask provided
> by the user.
>
>
> Daniel.


heres what I have in mind.  whats described here is working.
I'll send it out soon

commit 20298ec88cc2ed64269c8be7b287a24e60a5347e
Author: Jim Cromie <jim.cromie@gmail.com>
Date:   Wed Jun 10 12:55:08 2020 -0600

    dyndbg: WIP towards module->debugflags based callsite controls

    There are *lots* of ad-hoc debug printing solutions in kernel,
    this is a 1st attempt at providing a common mechanism for many of them.

    Basically, there are 2 styles of debug printing:
    - levels, with increasing verbosity, 1-10 forex
    - bits/flags, independently controlling separate groups of dprints

    This patch does bits/flags (with no distinction made yet between 2)

    API:

    - change pr_debug(...)  -->  pr_debug_typed(type_id=0, ...)
    - all existing uses have type_id=0
    - developer creates exclusive types of log messages with type_id>0
      1, 2, 3 are disjoint groups, for example: hi, mid, low

    - !!type_id is just an additional callsite selection criterion

      Qfoo() { echo module foo $* >/proc/dynamic_debug/control }
      Qfoo +p               # all groups, including default 0
      Qfoo mflags 1 +p      # only group 1
      Qfoo mflags 12 +p     # TBD[1]: groups 1 or 2
      Qfoo mflags 0 +p      # ignored atm TBD[2]
      Qfoo mflags af +p     # TBD[3]: groups a or f (10 or 15)

    so patch does:

    - add u32 debugflags to struct module. Each bit is a separate print-class.

    - add unsigned int mflags into struct ddebug_query
      mflags matched in ddebug_change

    - add unsigned int type_id:5 to struct _ddebug
      picks a single debugflag bit.  No subclass or multitype nonsense.
      nice and dense, packs with other members.
      we will have a lot of struct _ddebugs.

    - in ddebug_change()
      filter on !! module->debugflags,
      IFF query->module is given, and matches dt->mod_name
      and query->mflags is given, and bitmatches module->debugflags

    - in parse_query()
      accept new query term: mflags $arg
      populate query->mflags
      arg-type needs some attention, but basic plumbing is there

    WIP: not included:

    - pr_debug_typed( bitpos=0, ....)
      aka: pr_debug_class() or pr_debug_id()
      the bitpos is 1<<shift, allowing a single type. no ISA relations.
      this covers OP's high,mid,low case, many others

    - no way to exersize new code in ddebug_change
      need pr_debug_typed() to make a (not-null) typed callsite.
      also no way to set module->debugflags

    Im relying on:
    cdf6d00696 dynamic_debug: don't duplicate modname in ddebug_add_module

    which copies the ptr-val from module->name to dt->mod_name, which
    allowed == tests instead of strcmp.

    That equivalence and a (void*) cast in use of container_of() seem to
    do the trick to get the module, then module->debugflags.

    Notes:

    1- A query ANDs all its query terms together, so Qfoo() above
    requires both "module foo" AND all additional query terms given in $*

    But since callsite type_id creates disjoint groups, "mflags 12" is
    nonsense if it means groups 1 AND 2.  Here, 1 OR 2 is meaningful, if
    its not judged to be too confusing.

    2- im not sure what this does atm, or should do
       Qfoo mflags 0 +p      # select only untyped ? or no flags check at all ?

    3- since modflags has 32 bits, [1-9a-v] could select any single type_id
       it is succinct, but arcane.
       its a crude alphanumeric map for developers to make flags mnemonic
       maybe query keyword should be mbits

Re: [PATCH v3 6/7] venus: Make debug infrastructure more flexible

[Jason Baron]

On 6/11/20 5:19 PM, jim.cromie@gmail.com wrote:
> trimmed..
> 
>>>> Currently I think there not enough "levels" to map something like
>>>> drm.debug to the new dyn dbg feature. I don't think it is intrinsic
>>>> but I couldn't find the bit of the code where the 5-bit level in struct
>>>> _ddebug is converted from a mask to a bit number and vice-versa.
>>>
>>> Here [1] is Joe's initial suggestion. But I decided that bitmask is a
>>> good start for the discussion.
>>>
>>> I guess we can add new member uint "level" in struct _ddebug so that we
>>> can cover more "levels" (types, groups).
>>
>> I don't think it is allocating only 5 bits that is the problem!
>>
>> The problem is that those 5 bits need not be encoded as a bitmask by
>> dyndbg, that can simply be the category code for the message. They only
>> need be converted into a mask when we compare them to the mask provided
>> by the user.
>>
>>
>> Daniel.
> 
> 
> heres what I have in mind.  whats described here is working.
> I'll send it out soon

Cool. thanks for working on this!

> 
> commit 20298ec88cc2ed64269c8be7b287a24e60a5347e
> Author: Jim Cromie <jim.cromie@gmail.com>
> Date:   Wed Jun 10 12:55:08 2020 -0600
> 
>     dyndbg: WIP towards module->debugflags based callsite controls
> 
>     There are *lots* of ad-hoc debug printing solutions in kernel,
>     this is a 1st attempt at providing a common mechanism for many of them.
> 
>     Basically, there are 2 styles of debug printing:
>     - levels, with increasing verbosity, 1-10 forex
>     - bits/flags, independently controlling separate groups of dprints
> 
>     This patch does bits/flags (with no distinction made yet between 2)


I think it might be nice to have this proposal to integrate level too
so we see how it fits in. Maybe level is just about we enable things?
So you still mark callsites with pr_debug_typed(), but we can eanble
them via something like, 'echo module foo level N +p  >
/proc/dynamic_debug/control'. And that and 'p' to all callsites <= N.
So anybody using pr_debug_typed() can use either style to enable/disable.


> 
>     API:
> 
>     - change pr_debug(...)  -->  pr_debug_typed(type_id=0, ...)
>     - all existing uses have type_id=0
>     - developer creates exclusive types of log messages with type_id>0
>       1, 2, 3 are disjoint groups, for example: hi, mid, low
> 
>     - !!type_id is just an additional callsite selection criterion
> 
>       Qfoo() { echo module foo $* >/proc/dynamic_debug/control }
>       Qfoo +p               # all groups, including default 0
>       Qfoo mflags 1 +p      # only group 1
>       Qfoo mflags 12 +p     # TBD[1]: groups 1 or 2

So I thought this meant select group twelve. Aren't there 32 groups?

>       Qfoo mflags 0 +p      # ignored atm TBD[2]
>       Qfoo mflags af +p     # TBD[3]: groups a or f (10 or 15)
> 
>     so patch does:
> 
>     - add u32 debugflags to struct module. Each bit is a separate print-class.
> 

what is this for again?

>     - add unsigned int mflags into struct ddebug_query
>       mflags matched in ddebug_change
> 
>     - add unsigned int type_id:5 to struct _ddebug
>       picks a single debugflag bit.  No subclass or multitype nonsense.
>       nice and dense, packs with other members.
>       we will have a lot of struct _ddebugs.
> 
>     - in ddebug_change()
>       filter on !! module->debugflags,
>       IFF query->module is given, and matches dt->mod_name
>       and query->mflags is given, and bitmatches module->debugflags
> 
>     - in parse_query()
>       accept new query term: mflags $arg
>       populate query->mflags
>       arg-type needs some attention, but basic plumbing is there
> 
>     WIP: not included:
> 
>     - pr_debug_typed( bitpos=0, ....)
>       aka: pr_debug_class() or pr_debug_id()
>       the bitpos is 1<<shift, allowing a single type. no ISA relations.
>       this covers OP's high,mid,low case, many others
> 
>     - no way to exersize new code in ddebug_change
>       need pr_debug_typed() to make a (not-null) typed callsite.
>       also no way to set module->debugflags
> 
>     Im relying on:
>     cdf6d00696 dynamic_debug: don't duplicate modname in ddebug_add_module
> 
>     which copies the ptr-val from module->name to dt->mod_name, which
>     allowed == tests instead of strcmp.
> 
>     That equivalence and a (void*) cast in use of container_of() seem to
>     do the trick to get the module, then module->debugflags.
> 
>     Notes:
> 
>     1- A query ANDs all its query terms together, so Qfoo() above
>     requires both "module foo" AND all additional query terms given in $*
> 
>     But since callsite type_id creates disjoint groups, "mflags 12" is
>     nonsense if it means groups 1 AND 2.  Here, 1 OR 2 is meaningful, if
>     its not judged to be too confusing.
> 
>     2- im not sure what this does atm, or should do
>        Qfoo mflags 0 +p      # select only untyped ? or no flags check at all ?

seems like it should group 0 which you're calling untyped. So you can write group
0 call sites as pr_debug(); or pr_debug_typed(0, ...); I'm not sure why we should
treat it differently other than it can be written without the pr_debug_typed().

> 
>     3- since modflags has 32 bits, [1-9a-v] could select any single type_id
>        it is succinct, but arcane.
>        its a crude alphanumeric map for developers to make flags mnemonic
>        maybe query keyword should be mbits
> 

Also wondering if the control file should display the type (if non-zero). I think
that would be nice.

Thanks,

-Jason

Re: [PATCH v3 6/7] venus: Make debug infrastructure more flexible

[Joe Perches]

On Thu, 2020-06-11 at 17:59 -0400, Jason Baron wrote:
> 
> On 6/11/20 5:19 PM, jim.cromie@gmail.com wrote:
> > trimmed..
> > 
> > > > > Currently I think there not enough "levels" to map something like
> > > > > drm.debug to the new dyn dbg feature. I don't think it is intrinsic
> > > > > but I couldn't find the bit of the code where the 5-bit level in struct
> > > > > _ddebug is converted from a mask to a bit number and vice-versa.
> > > > 
> > > > Here [1] is Joe's initial suggestion. But I decided that bitmask is a
> > > > good start for the discussion.
> > > > 
> > > > I guess we can add new member uint "level" in struct _ddebug so that we
> > > > can cover more "levels" (types, groups).
> > > 
> > > I don't think it is allocating only 5 bits that is the problem!

There were 6 unused bits in struct _ddebug;

The original idea was to avoid expanding the already somewhat
large struct _ddebug uses and the __verbose/__dyndbg section
that can have quite a lot of these structs.

I imagine adding another int or long wouldn't be too bad.

> > > The problem is that those 5 bits need not be encoded as a bitmask by
> > > dyndbg, that can simply be the category code for the message. They only
> > > need be converted into a mask when we compare them to the mask provided
> > > by the user.
> > > 

I also suggested adding a pointer to whatever is provided
by the developer so the address of something like
MODULE_PARM_DESC(variable, ...) can be also be used.

> > heres what I have in mind.  whats described here is working.
> > I'll send it out soon
> 
> Cool. thanks for working on this!

Truly, thank you both Jim and Stanimir.

Please remember that dynamic_debug is not required and
pr_debug should still work.

> >     API:
> > 
> >     - change pr_debug(...)  -->  pr_debug_typed(type_id=0, ...)
> >     - all existing uses have type_id=0
> >     - developer creates exclusive types of log messages with type_id>0
> >       1, 2, 3 are disjoint groups, for example: hi, mid, low

You could have a u8 for type if there are to be 3 classes

	bitmask
	level
	group by value

though I believe group by value might as well just be bitmask
and bool is_bitmask is enough (!is_bitmask would be level)

cheers, Joe

Re: [PATCH v3 6/7] venus: Make debug infrastructure more flexible

[jim.cromie]

calling out some thinkos

On Thu, Jun 11, 2020 at 3:19 PM <jim.cromie@gmail.com> wrote:
>
> heres what I have in mind.  whats described here is working.
> I'll send it out soon
>
> commit 20298ec88cc2ed64269c8be7b287a24e60a5347e
> Author: Jim Cromie <jim.cromie@gmail.com>
> Date:   Wed Jun 10 12:55:08 2020 -0600
>
>     dyndbg: WIP towards module->debugflags based callsite controls
>
>     There are *lots* of ad-hoc debug printing solutions in kernel,
>     this is a 1st attempt at providing a common mechanism for many of them.
>
>     Basically, there are 2 styles of debug printing:
>     - levels, with increasing verbosity, 1-10 forex
>     - bits/flags, independently controlling separate groups of dprints
>
>     This patch does bits/flags (with no distinction made yet between 2)
>
>     API:
>
>     - change pr_debug(...)  -->  pr_debug_typed(type_id=0, ...)

pr_debug, pr_debug_n now in printk.h

_?_?dynamic_.+_cl  adaptations in dynamic_debug.h

>     - all existing uses have type_id=0
>     - developer creates exclusive types of log messages with type_id>0
>       1, 2, 3 are disjoint groups, for example: hi, mid, low
>
>     - !!type_id is just an additional callsite selection criterion
>
>       Qfoo() { echo module foo $* >/proc/dynamic_debug/control }
>       Qfoo +p               # all groups, including default 0
>       Qfoo mflags 1 +p      # only group 1
>       Qfoo mflags 12 +p     # TBD[1]: groups 1 or 2
>       Qfoo mflags 0 +p      # ignored atm TBD[2]
>       Qfoo mflags af +p     # TBD[3]: groups a or f (10 or 15)
>
>     so patch does:
>
>     - add u32 debugflags to struct module. Each bit is a separate print-class.

this is feeling wrong now.
setting these bits would have to trigger an update via ddebug_exec_query
kinda like setting a bit would trigger
       echo module $foo mflags $bitpos +p > control

its possible, but not 1st, or 2nd perhaps.
In general Im quite leery of rigging up some callback to do it.

its prudent to effect all debug changes via >control

>     - in ddebug_change()
>       filter on !! module->debugflags,
>       IFF query->module is given, and matches dt->mod_name
>       and query->mflags is given, and bitmatches module->debugflags

wrong, ddebug_change cannot respond to changes of debugflags,
most it could do is consult it on queries


>     - in parse_query()
>       accept new query term: mflags $arg
>       populate query->mflags
>       arg-type needs some attention, but basic plumbing is there
>
>     WIP: not included:
>
>     - pr_debug_typed( bitpos=0, ....)'

now done, as pr_debug_n, pr_debug in printk.h

Ive adapted the macros with a "_cl(cl, " insertion,

also added trailing prcls to control output

>
>     - no way to exersize new code in ddebug_change
>       need pr_debug_typed() to make a (not-null) typed callsite.
>       also no way to set module->debugflags

close enough to see the thinkos

WIP generic module->debug_flags and dynamic_debug

[jim.cromie]

so Ive got a WIP / broken / partial approach to giving all modules a
u32 flagset,
and enabling pr_debug based upon it.  I leave out the "pr_debug_typed(
bitpos )" for now.  For Stanimir, bits 1,2,3 could be high, middle,
low.

ATM its broken on my lack of container_of() skills.

Im trying to use it to get a struct module* using its name value thats
been copied
into a ddebug_table member.

Im relying on
cdf6d006968  dynamic_debug: don't duplicate modname in ddebug_add_module
to have the same value in both structs

but Im clearly missing a few things
besides the likely future trouble with .rodata builtin modules
(after compile prob solved)

It seems container_of wants me to use struct ddebug_table instead,
but I dont want a *ddebug_table.
Ive blindly guessed at adding & and * to 1st param, w/o understanding.

can anyone diagnose my problem ?


[jimc@frodo build-virtme]$ git diff
diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index a5d76f8f6b40..2bfd1aa083b3 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -20,6 +20,7 @@ struct _ddebug {
        const char *function;
        const char *filename;
        const char *format;
+       u32 reqd_flags; /*misleading name todo, probly should hold
just a single type-bit */
        unsigned int lineno:18;
        /*
         * The flags field controls the behaviour at the callsite.
diff --git a/include/linux/module.h b/include/linux/module.h
index 1ad393e62bef..06eeef438fd3 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -429,6 +429,7 @@ struct module {
        struct mod_arch_specific arch;

        unsigned long taints;   /* same bits as kernel:taint_flags */
+       u32 debugflags;

 #ifdef CONFIG_GENERIC_BUG
        /* Support for BUG */
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 63ae6f77a0e4..965ee96630b5 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -202,6 +202,20 @@ static int ddebug_change(const struct ddebug_query *query,
                        if ((~dp->flags & filter->mask) != filter->mask)
                                continue;

+                       /* screen on module->debugflags */
+                       if (query->module) {
+                               /* dt->modname is known == module->name */
+                               struct module *m =
+                                       container_of((&(dt->mod_name)),
+                                                    struct module, name);
+                               if (m->debugflags &&
+                                   ((m->debugflags & dp->reqd_flags)
+                                    != dp->reqd_flags))
+                                       continue;
+                               vpr_info("%s module-debugflags: 0x%x\n",
+                                        dt->mod_name, dp->reqd_flags);
+                       }
+
                        nfound++;

                        newflags = (dp->flags & mods->mask) | mods->flags;
(END)

Re: WIP generic module->debug_flags and dynamic_debug

[Joe Perches]

On Wed, 2020-06-10 at 12:32 -0600, jim.cromie@gmail.com wrote:
> so Ive got a WIP / broken / partial approach to giving all modules a
> u32 flagset,
> and enabling pr_debug based upon it.  I leave out the "pr_debug_typed(
> bitpos )" for now.  For Stanimir, bits 1,2,3 could be high, middle,
> low.

Can you change this please to be a pointer to an unsigned long/int?

That way, existing non-dynamic debug uses like

MODULE_PARM_DESC(debug, "module specific debug control");

maybe could use the same mechanism to enable/disable by class.

Re: WIP generic module->debug_flags and dynamic_debug

[Rasmus Villemoes]

On 10/06/2020 20.32, jim.cromie@gmail.com wrote:
> so Ive got a WIP / broken / partial approach to giving all modules a
> u32 flagset,
> and enabling pr_debug based upon it.  I leave out the "pr_debug_typed(
> bitpos )" for now.  For Stanimir, bits 1,2,3 could be high, middle,
> low.
> 
> ATM its broken on my lack of container_of() skills.
> 
> Im trying to use it to get a struct module* using its name value thats
> been copied
> into a ddebug_table member.
> 
> Im relying on
> cdf6d006968  dynamic_debug: don't duplicate modname in ddebug_add_module
> to have the same value in both structs
> 
> but Im clearly missing a few things
> besides the likely future trouble with .rodata builtin modules
> (after compile prob solved)
> 
> It seems container_of wants me to use struct ddebug_table instead,
> but I dont want a *ddebug_table.
> Ive blindly guessed at adding & and * to 1st param, w/o understanding.
> 
> can anyone diagnose my problem ?

Sorry, I have not the faintest idea of what you're trying to achieve.
Can you spell that out?

Rasmus

Re: WIP generic module->debug_flags and dynamic_debug

[jim.cromie]

> > can anyone diagnose my problem ?
>
> Sorry, I have not the faintest idea of what you're trying to achieve.
> Can you spell that out?
>
> Rasmus

hey, sorry for the lack of context.
Ive solved my problem, by adding (void*) to my container_of usage.
WIP code now "works", but still incomplete.
will reroll and resubmit

[PATCH v3 7/7] venus: Add a debugfs file for SSR trigger

[Stanimir Varbanov]

The SSR (SubSystem Restart) is used to simulate an error on FW
side of Venus. We support following type of triggers - fatal error,
div by zero and watchdog IRQ.

Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
---
 drivers/media/platform/qcom/venus/dbgfs.c | 31 +++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/media/platform/qcom/venus/dbgfs.c b/drivers/media/platform/qcom/venus/dbgfs.c
index a2465fe8e20b..59d52e5af64a 100644
--- a/drivers/media/platform/qcom/venus/dbgfs.c
+++ b/drivers/media/platform/qcom/venus/dbgfs.c
@@ -9,6 +9,35 @@
 
 extern int venus_fw_debug;
 
+static int trigger_ssr_open(struct inode *inode, struct file *file)
+{
+	file->private_data = inode->i_private;
+	return 0;
+}
+
+static ssize_t trigger_ssr_write(struct file *filp, const char __user *buf,
+				 size_t count, loff_t *ppos)
+{
+	struct venus_core *core = filp->private_data;
+	u32 ssr_type;
+	int ret;
+
+	ret = kstrtou32_from_user(buf, count, 4, &ssr_type);
+	if (ret)
+		return ret;
+
+	ret = hfi_core_trigger_ssr(core, ssr_type);
+	if (ret < 0)
+		return ret;
+
+	return count;
+}
+
+static const struct file_operations ssr_fops = {
+	.open = trigger_ssr_open,
+	.write = trigger_ssr_write,
+};
+
 int venus_dbgfs_init(struct venus_core *core)
 {
 	core->root = debugfs_create_dir("venus", NULL);
@@ -17,6 +46,8 @@ int venus_dbgfs_init(struct venus_core *core)
 
 	debugfs_create_x32("fw_level", 0644, core->root, &venus_fw_debug);
 
+	debugfs_create_file("trigger_ssr", 0200, core->root, core, &ssr_fops);
+
 	return 0;
 }
 
-- 
2.17.1

Re: [PATCH v3 0/7] Venus dynamic debug

[Matthew Wilcox]

On Tue, Jun 09, 2020 at 01:45:57PM +0300, Stanimir Varbanov wrote:
> Here is the third version of dynamic debug improvements in Venus
> driver.  As has been suggested on previous version by Joe [1] I've
> made the relevant changes in dynamic debug core to handle leveling
> as more generic way and not open-code/workaround it in the driver.
> 
> About changes:
>  - added change in the dynamic_debug and in documentation
>  - added respective pr_debug_level and dev_dbg_level

Honestly, this seems like you want to use tracepoints, not dynamic debug.

Re: [PATCH v3 0/7] Venus dynamic debug

[Randy Dunlap]

On 6/9/20 4:13 AM, Matthew Wilcox wrote:
> On Tue, Jun 09, 2020 at 01:45:57PM +0300, Stanimir Varbanov wrote:
>> Here is the third version of dynamic debug improvements in Venus
>> driver.  As has been suggested on previous version by Joe [1] I've
>> made the relevant changes in dynamic debug core to handle leveling
>> as more generic way and not open-code/workaround it in the driver.
>>
>> About changes:
>>  - added change in the dynamic_debug and in documentation
>>  - added respective pr_debug_level and dev_dbg_level
> 
> Honestly, this seems like you want to use tracepoints, not dynamic debug.
> 

Also see this patch series:
https://lore.kernel.org/lkml/20200605162645.289174-1-jim.cromie@gmail.com/
[PATCH 00/16] dynamic_debug: cleanups, 2 features

It adds/expands dynamic debug flags quite a bit.

-- 
~Randy

Re: [PATCH v3 0/7] Venus dynamic debug

[Joe Perches]

(adding Jim Cromie and comments)

On Tue, 2020-06-09 at 09:03 -0700, Randy Dunlap wrote:
> On 6/9/20 4:13 AM, Matthew Wilcox wrote:
> > On Tue, Jun 09, 2020 at 01:45:57PM +0300, Stanimir Varbanov wrote:
> > > Here is the third version of dynamic debug improvements in Venus
> > > driver.  As has been suggested on previous version by Joe [1] I've
> > > made the relevant changes in dynamic debug core to handle leveling
> > > as more generic way and not open-code/workaround it in the driver.
> > > 
> > > About changes:
> > >  - added change in the dynamic_debug and in documentation
> > >  - added respective pr_debug_level and dev_dbg_level
> > 
> > Honestly, this seems like you want to use tracepoints, not dynamic debug.

Tracepoints are a bit heavy and do not have any class
or grouping mechanism.

debug_class is likely a better name than debug_level

> Also see this patch series:
> https://lore.kernel.org/lkml/20200605162645.289174-1-jim.cromie@gmail.com/
> [PATCH 00/16] dynamic_debug: cleanups, 2 features
> 
> It adds/expands dynamic debug flags quite a bit.

Yes, and thanks Randy and Jim and Stanimir

I haven't gone through Jim's proposal enough yet.
It's unfortunate these patches series conflict.

And for Jim, a link to Stanimir's patch series:
https://lore.kernel.org/lkml/20200609104604.1594-1-stanimir.varbanov@linaro.org/

Re: [PATCH v3 0/7] Venus dynamic debug

[jim.cromie]

On Tue, Jun 9, 2020 at 10:49 AM Joe Perches <joe@perches.com> wrote:
>
> (adding Jim Cromie and comments)
>

thanks for bringing me in...


> On Tue, 2020-06-09 at 09:03 -0700, Randy Dunlap wrote:
> > On 6/9/20 4:13 AM, Matthew Wilcox wrote:
> > > On Tue, Jun 09, 2020 at 01:45:57PM +0300, Stanimir Varbanov wrote:
> > > > Here is the third version of dynamic debug improvements in Venus
> > > > driver.  As has been suggested on previous version by Joe [1] I've
> > > > made the relevant changes in dynamic debug core to handle leveling
> > > > as more generic way and not open-code/workaround it in the driver.
> > > >
> > > > About changes:
> > > >  - added change in the dynamic_debug and in documentation
> > > >  - added respective pr_debug_level and dev_dbg_level
> > >
> > > Honestly, this seems like you want to use tracepoints, not dynamic debug.
>
> Tracepoints are a bit heavy and do not have any class
> or grouping mechanism.
>
> debug_class is likely a better name than debug_level
>
> > Also see this patch series:
> > https://lore.kernel.org/lkml/20200605162645.289174-1-jim.cromie@gmail.com/
> > [PATCH 00/16] dynamic_debug: cleanups, 2 features
> >
> > It adds/expands dynamic debug flags quite a bit.
>
> Yes, and thanks Randy and Jim and Stanimir
>
> I haven't gone through Jim's proposal enough yet.
> It's unfortunate these patches series conflict.
>
> And for Jim, a link to Stanimir's patch series:
> https://lore.kernel.org/lkml/20200609104604.1594-1-stanimir.varbanov@linaro.org/
>
>


As Joe noted, there is a lot of ad-hockery to possibly clean up,
but I dont grok how these levels should be distinguished from
KERN_(WARN|INFO|DEBUG) constants.
Those constants are used by coders, partly to convey how bad things are
As a user, Id be reluctant to disable an EMERG callsite.

are you trying to add a User Bit ? or maybe 7-9 of them ?

I have a patchset which adds a 'u' flag, for user.
An earlier version had x,y,z flags for 3 different user purposes.
I simplified, since only 1 was needed to mark up arbitrary sets of callsites.
Another patchset feature lets u select on that flag.

 #> echo u+p > control

Joe suggested class, I certainly find level confusing.

Is what you want user-flags u[1-7], or driver-flags d]1-7]   ?
and let me distinguish,
your flags are set in code, not modifiable by user, only for filtering
on flag/bit state ?
so theyd be different than [pfmltu_] flags, which are user changed.

my patchset also adds filtering on flag-state,
so that "echo u+p > control " could work.

if you had
     echo 'module venus 1+p; 2+p; 9+p' > control
how far would you get ?

if it covers your needs, then we could add
numerical flags (aka U1, U9) can be distinguished from  [pfmltu_PFMLTU]
and excluded from the mod-flags changes

from there, it shouldnt be hard to add some macro help

DECLARE_DYNDBG_FLAG ( 1, 'x' )
DECLARE_DYNDBG_FLAG ( 2, 'y' )
DECLARE_DYNDBG_FLAG ( 3, 'z' )

DECLARE_DYNDBG_FLAG_INFO ( 4, 'q', "unquiet a programmer defined debug
callsite set" )

also, since Im here, and this is pretty much on-topic,
I perused https://lkml.org/lkml/2020/5/21/399

I see 3 things;
- s / dev_dbg / VDBGL /
- add a bunch of VDBGM calls
- sys_get_prop_image_version  signature change.   (this doesnt really
belong here)

ISTM most of the selection of dyndbg callsites in that part of venus
could be selected by format.

   echo "module venus format error +p" > control

if so, refining your messages will define the logical sets for you ?


thanks
JimC (one of them anyway)

Re: [PATCH v3 0/7] Venus dynamic debug

[Joe Perches]

On Tue, 2020-06-09 at 15:21 -0600, jim.cromie@gmail.com wrote:
> On Tue, Jun 9, 2020 at 10:49 AM Joe Perches <joe@perches.com> wrote:
> > (adding Jim Cromie and comments)
> > On Tue, 2020-06-09 at 09:03 -0700, Randy Dunlap wrote:
> > > On 6/9/20 4:13 AM, Matthew Wilcox wrote:
> > > > On Tue, Jun 09, 2020 at 01:45:57PM +0300, Stanimir Varbanov wrote:
> > > > > Here is the third version of dynamic debug improvements in Venus
> > > > > driver.  As has been suggested on previous version by Joe [1] I've
> > > > > made the relevant changes in dynamic debug core to handle leveling
> > > > > as more generic way and not open-code/workaround it in the driver.
> > > > > 
> > > > > About changes:
> > > > >  - added change in the dynamic_debug and in documentation
> > > > >  - added respective pr_debug_level and dev_dbg_level
> > > > 
> > > > Honestly, this seems like you want to use tracepoints, not dynamic debug.
> > 
> > Tracepoints are a bit heavy and do not have any class
> > or grouping mechanism.
> > 
> > debug_class is likely a better name than debug_level
> > 
> > > Also see this patch series:
> > > https://lore.kernel.org/lkml/20200605162645.289174-1-jim.cromie@gmail.com/
> > > [PATCH 00/16] dynamic_debug: cleanups, 2 features
> > > 
> > > It adds/expands dynamic debug flags quite a bit.
> > 
> > Yes, and thanks Randy and Jim and Stanimir
> > 
> > I haven't gone through Jim's proposal enough yet.
> > It's unfortunate these patches series conflict.
> > 
> > And for Jim, a link to Stanimir's patch series:
> > https://lore.kernel.org/lkml/20200609104604.1594-1-stanimir.varbanov@linaro.org/
> > 
> > 
> 
> As Joe noted, there is a lot of ad-hockery to possibly clean up,
> but I dont grok how these levels should be distinguished from
> KERN_(WARN|INFO|DEBUG) constants.

These are not KERN_<LEVEL> at all, all are emitted at KERN_DEBUG

These are just driver developer mechanisms to enable/disable
groups of formats via some test for < level or | bitmap

> Those constants are used by coders, partly to convey how bad things are
> As a user, Id be reluctant to disable an EMERG callsite.

Not possible.

> are you trying to add a User Bit ? or maybe 7-9 of them ?

Or maybe a u32/ulong worth as most as I think the largest
current use is 32 bits of bitmask.

> I have a patchset which adds a 'u' flag, for user.

Adapting that with an external bool for bitmask or class
would work fine.

	if (is_bitmask)
		enable/disable(value|flag)
	else
		enable/disable(value < flag)

So the equivalent of logical sets would work just fine.

Re: [PATCH v3 0/7] Venus dynamic debug

[Joe Perches]

On Tue, 2020-06-09 at 15:23 -0700, Joe Perches wrote:
> These are just driver developer mechanisms to enable/disable
> groups of formats via some test for < level or | bitmap

duh: & bitmask

> 	if (is_bitmask)
> 		enable/disable(value|flag)

obviously
		enable/disable(value & flag)

Re: [PATCH v3 0/7] Venus dynamic debug

[jim.cromie]

On Tue, Jun 9, 2020 at 4:23 PM Joe Perches <joe@perches.com> wrote:
>
> On Tue, 2020-06-09 at 15:21 -0600, jim.cromie@gmail.com wrote:
> >
> > As Joe noted, there is a lot of ad-hockery to possibly clean up,
> > but I dont grok how these levels should be distinguished from
> > KERN_(WARN|INFO|DEBUG) constants.
>
> These are not KERN_<LEVEL> at all, all are emitted at KERN_DEBUG

yes indeed.  but they are chosen by programmer, fixed by compiler.  not dynamic.
<pmladek@suse.com> also noted the conceptual adjacency (ambiguity),
and referenced KERN_<lvl>



If we need this extra query-term, lets call it   mbits / mflags /
module_flags / module_bits
it needs to be module specific, so also requiring "module foo" search
term in the query.
( "modflags" is no good, cuz "mod" also means "modified" - just mflags
is better )

Already, we have function, file, module, all of which convey semantic
structure of the code,
and they also match wildcards, so " function foo_*_* " is an effective grouping.
Id think this would cover most cases.

Finally, all "module venus +p " callsites could be explicitly
specified individually in
universe=`grep venus control | wc -l`
lines, likely a small set.
Using the semantic structure exposed by `grep venus control`, it would
likely be far less.

Re: [PATCH v3 0/7] Venus dynamic debug

[Joe Perches]

On Tue, 2020-06-09 at 13:45 +0300, Stanimir Varbanov wrote:
> Hello,
> 
> Here is the third version of dynamic debug improvements in Venus
> driver.  As has been suggested on previous version by Joe [1] I've
> made the relevant changes in dynamic debug core to handle leveling
> as more generic way and not open-code/workaround it in the driver.
> 
> About changes:
>  - added change in the dynamic_debug and in documentation
>  - added respective pr_debug_level and dev_dbg_level
> 
> regards,
> Stan
> 
> [1] https://lkml.org/lkml/2020/5/21/668

This capability is already clumsily used by many
drivers that use a module level "debug" flag.

$ git grep -P "MODULE_PARM.*\bdebug\b"|wc -l
501

That's a _lot_ of homebrewed mechanisms.

Making dynamic debug have this as a feature would
help consolidate and standardize the capability.

ftrace is definitely useful, but not quite as
lightweight and doesn't have the typical uses.