Re: [PATCH 06/10] drm: Remove usage of deprecated DRM_DEBUG_DRIVER

[jim . cromie]

On Wed, Dec 21, 2022 at 3:14 AM Thomas Zimmermann <tzimmermann@xxxxxxx> wrote:
>
> Hi
>
> Am 21.12.22 um 11:00 schrieb Jani Nikula:
> > On Wed, 21 Dec 2022, Siddh Raman Pant <code@xxxxxxxx> wrote:
> >> drm_print.h says DRM_DEBUG_DRIVER is deprecated.
> >> Thus, use newer drm_dbg_driver().
> >>
> >> Also fix the deprecation comment in drm_print.h which
> >> mentions drm_dbg() instead of drm_dbg_driver().
> >>
> >> Signed-off-by: Siddh Raman Pant <code@xxxxxxxx>
> >> ---
> >>   drivers/gpu/drm/drm_mipi_dbi.c | 10 +++++-----
> >>   include/drm/drm_print.h        |  2 +-
> >>   2 files changed, 6 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c
> >> index 24af507bb687..6ad399f6ab03 100644
> >> --- a/drivers/gpu/drm/drm_mipi_dbi.c
> >> +++ b/drivers/gpu/drm/drm_mipi_dbi.c
> >> @@ -69,11 +69,11 @@
> >>   #define MIPI_DBI_DEBUG_COMMAND(cmd, data, len) \
> >>   ({ \
> >>      if (!len) \
> >> -            DRM_DEBUG_DRIVER("cmd=%02x\n", cmd); \
> >> +            drm_dbg_driver(NULL, "cmd=%02x\n", cmd); \
> >>      else if (len <= 32) \
> >> -            DRM_DEBUG_DRIVER("cmd=%02x, par=%*ph\n", cmd, (int)len, data);\
> >> +            drm_dbg_driver(NULL, "cmd=%02x, par=%*ph\n", cmd, (int)len, data);\
> >>      else \
> >> -            DRM_DEBUG_DRIVER("cmd=%02x, len=%zu\n", cmd, len); \
> >> +            drm_dbg_driver(NULL, "cmd=%02x, len=%zu\n", cmd, len); \
> >>   })
> >>
> >>   static const u8 mipi_dbi_dcs_read_commands[] = {
> >> @@ -632,7 +632,7 @@ bool mipi_dbi_display_is_on(struct mipi_dbi *dbi)
> >>          DCS_POWER_MODE_DISPLAY_NORMAL_MODE | DCS_POWER_MODE_SLEEP_MODE))
> >>              return false;
> >>
> >> -    DRM_DEBUG_DRIVER("Display is ON\n");
> >> +    drm_dbg_driver(NULL, "Display is ON\n");
> >>
> >>      return true;
> >>   }
> >> @@ -1168,7 +1168,7 @@ int mipi_dbi_spi_init(struct spi_device *spi, struct mipi_dbi *dbi,
> >>
> >>      mutex_init(&dbi->cmdlock);
> >>
> >> -    DRM_DEBUG_DRIVER("SPI speed: %uMHz\n", spi->max_speed_hz / 1000000);
> >> +    drm_dbg_driver(NULL, "SPI speed: %uMHz\n", spi->max_speed_hz / 1000000);
> >>
> >>      return 0;
> >>   }
> >> diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
> >> index 53702d830291..10261faec8b6 100644
> >> --- a/include/drm/drm_print.h
> >> +++ b/include/drm/drm_print.h
> >> @@ -614,7 +614,7 @@ void __drm_err(const char *format, ...);
> >>   #define DRM_DEBUG(fmt, ...)                                                \
> >>      __drm_dbg(DRM_UT_CORE, fmt, ##__VA_ARGS__)
> >>
> >> -/* NOTE: this is deprecated in favor of drm_dbg(NULL, ...). */
> >> +/* NOTE: this is deprecated in favor of drm_dbg_driver(NULL, ...). */
> >
> > Way back the idea was to make the shorter drm_dbg() the driver debug,
> > and drm_dbg_core() the drm core debug, because the former vastly
> > outnumbers the the latter.
> >
> > I don't know if that changed with the dyndbg stuff.

well, there was:

commit 95a77b6331c2d2313aa843fa77ec91cd092ab0e4
Author: Jim Cromie <jim.cromie@xxxxxxxxx>
Date:   Sun Sep 11 23:28:49 2022 -0600

    drm-print: add drm_dbg_driver to improve namespace symmetry

    drm_print defines all of these:
        drm_dbg_{core,kms,prime,atomic,vbl,lease,_dp,_drmres}

    but not drm_dbg_driver itself, since it was the original drm_dbg.

    To improve namespace symmetry, change the drm_dbg defn to
    drm_dbg_driver, and redef grandfathered name to symmetric one.

>
> I've recently grepped for these macros and nothing uses drm_dbg_driver()
> directly.

I have a use-case for it, in an unready branch thats trying to adapt nouveau
to use dyndbg.  Its presence allows macro constructs to paste 'driver'
onto the end, and have it resolve.  (see ##cat below)


--- a/drivers/gpu/drm/nouveau/nouveau_drv.h
+++ b/drivers/gpu/drm/nouveau/nouveau_drv.h
@@ -39,6 +39,7 @@
  */

 #include <linux/notifier.h>
+#include <linux/dynamic_debug.h>

 #include <nvif/client.h>
 #include <nvif/device.h>
@@ -263,13 +264,16 @@ void nouveau_drm_device_remove(struct drm_device *dev);
 #define NV_WARN(drm,f,a...) NV_PRINTK(warn, &(drm)->client, f, ##a)
 #define NV_INFO(drm,f,a...) NV_PRINTK(info, &(drm)->client, f, ##a)

-#define NV_DEBUG(drm,f,a...) do {
         \
-       if (drm_debug_enabled(DRM_UT_DRIVER))                                  \
-               NV_PRINTK(info, &(drm)->client, f, ##a);                       \
+#define NV_DRMDBG(cat,c,f,a...) do {                           \
+       struct nouveau_cli *_cli = (c);                         \
+       drm_dbg_##cat(_cli->drm->dev, "%s: "f, _cli->name, ##a); \
 } while(0)
-#define NV_ATOMIC(drm,f,a...) do {
         \
-       if (drm_debug_enabled(DRM_UT_ATOMIC))                                  \
-               NV_PRINTK(info, &(drm)->client, f, ##a);                       \
+
+#define NV_DEBUG(drm,f,a...) do {                                      \
+       NV_DRMDBG(driver, &(drm)->client, f, ##a);                      \
+} while(0)
+#define NV_ATOMIC(drm,f,a...) do {                                     \
+       NV_DRMDBG(atomic, &(drm)->client, f, ##a);                      \
 } while(0)


That could probably be special-cased to avoid pasting "driver",
but it would be harder to read.

>
> I also wondered whether the driver debug macro makes much sense. For
> example, if a driver implements its own atomic helpers, it's much more
> useful to use drm_dbg_kms() within those functions. If enabled, their
> output would then blend into the overall KMS-related debugging.
> drm_dbg/drm_dbg_driver() appears to be mostly useful for init and status
> reporting.

This seems a larger, separate topic:
How to rationalize the broad sub-system-wide use of drm.debug ?
if we can veer onto that topic:

dyndbg has optional decorations, controlled by flags:
"mfl" ->   module.func.lineno

DRM has its own conventions, which I wont try to summarize or enumerate
beyond noting driver name can appear 2x
DRM_INFO  prefixes "[drm]", others too Im sure.

can they (the debug  anyway) be made optional - flag controlled ?
extending flags or even redefining them seems practical.

I have WIP  dyndbg-to-trace interface,
with a T flag to enable each drm-dbg callsite to write to tracefs
how would DRM / DRI best use this ?

Currently, drm -> tracefs works, but messages include trailing \n,
and 2nd is added by trace, giving blank lines.
Stripping it at runtime is possible, but cost >0,

stripping it in src from fmt strings, and restoring it in the underlying printk
is practical, but sorta flag-day.  Is it distasteful ?

is there any pr_cont() style use of drm.debug ?
that would interfere with \n stripping.

should drm.debug --> dyndbg --> tracefs messages
include the decorations ?
or a different form of them ?


Separately (while Im here), what tree does patchwork apply to ?
I seem to always guess wrong.


>
> Best regards
> Thomas
>
> >
> >
> > BR,
> > Jani.
> >
> >
> >>   #define DRM_DEBUG_DRIVER(fmt, ...)                                 \
> >>      __drm_dbg(DRM_UT_DRIVER, fmt, ##__VA_ARGS__)
> >
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Ivo Totev