Re: [PATCH v5 13/13] drm/print: Add tracefs support to the drm logging helpers

[Pekka Paalanen <ppaalanen@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4292 bytes --]

On Wed, 10 Jun 2020 09:29:37 -0400
Sean Paul <sean@poorly.run> wrote:

> On Wed, Jun 10, 2020 at 3:57 AM Pekka Paalanen <ppaalanen@gmail.com> wrote:
> >
> > On Mon,  8 Jun 2020 17:05:03 -0400
> > Sean Paul <sean@poorly.run> wrote:
> >  
> > > From: Sean Paul <seanpaul@chromium.org>
> > >
> > > This patch adds a new module parameter called drm.trace which accepts
> > > the same mask as drm.debug. When a debug category is enabled, log
> > > messages will be put in a new tracefs instance called drm for
> > > consumption.
> > >
> > > Using the new tracefs instance will allow distros to enable drm logging
> > > in production without impacting performance or spamming the system
> > > logs.
> > >
> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > Cc: David Airlie <airlied@gmail.com>
> > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > > Cc: Pekka Paalanen <ppaalanen@gmail.com>
> > > Cc: Rob Clark <robdclark@gmail.com>
> > > Cc: Steven Rostedt <rostedt@goodmis.org>
> > > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Steven Rostedt <rostedt@goodmis.org>
> > > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > > Link: https://patchwork.freedesktop.org/patch/msgid/20191010204823.195540-1-sean@poorly.run #v1
> > > Link: https://lists.freedesktop.org/archives/dri-devel/2019-November/243230.html #v2
> > > Link: https://patchwork.freedesktop.org/patch/msgid/20191212203301.142437-1-sean@poorly.run #v3
> > > Link: https://patchwork.freedesktop.org/patch/msgid/20200114172155.215463-1-sean@poorly.run #v4
> > >
> > > Changes in v5:
> > > -Re-write to use trace_array and the tracefs instance support
> > > ---
> > >  Documentation/gpu/drm-uapi.rst |   6 +
> > >  drivers/gpu/drm/drm_drv.c      |   3 +
> > >  drivers/gpu/drm/drm_print.c    | 209 ++++++++++++++++++++++++++++-----
> > >  include/drm/drm_print.h        |  63 ++++++++--
> > >  4 files changed, 241 insertions(+), 40 deletions(-)
> > >

...

> > >  #include <drm/drm.h>
> > >  #include <drm/drm_drv.h>
> > > @@ -43,17 +44,34 @@
> > >  unsigned int __drm_debug_syslog;
> > >  EXPORT_SYMBOL(__drm_debug_syslog);
> > >
> > > -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)");
> > > +/*
> > > + * __drm_debug_trace: Enable debug output in drm tracing instance.
> > > + * Bitmask of DRM_UT_x. See include/drm/drm_print.h for details.
> > > + */
> > > +unsigned int __drm_debug_trace;
> > > +EXPORT_SYMBOL(__drm_debug_trace);  
> >
> > Hi!  
> 
> Hi Pekka,
> Thanks again for the feedback, I'm happy that we seem to be converging!
> 
> >
> > Might distributions perhaps want to set a default value for this via
> > Kconfig? Or could setting it via sysfs happen early enough to diagnose
> > e.g. Plymouth problems?
> >
> > Or maybe there is nothing to see from early boot?
> >
> > The general usefulness of this feature depends on whether people
> > actually run with it enabled.  
> 
> I had assumed that the cmdline argument would be sufficient for
> distros, is Kconfig preferable here? The module parameter has the
> advantage of being runtime configurable and is more in line with
> drm.debug. We can do either in CrOS, so I'm happy to go with crowd
> consensus.

Hi,

I don't know. I did mean only *default* value for the module parameter
via Kconfig. Right now your default value is implicit 0.

But that's just me thinking that making kernel command lines even
longer would be somehow bad. Maybe it's fine?


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]