diff mbox series

[v2,2/2] drm/print: document DRM_ logging functions

Message ID 20200102221519.31037-3-sam@ravnborg.org (mailing list archive)
State New, archived
Headers show
Series drm: document logging functions | expand

Commit Message

Sam Ravnborg Jan. 2, 2020, 10:15 p.m. UTC 
Document the remaining DRM_ logging functions.
As the logging functions are now all properly
listed drop the few specific kernel-doc markers
so we keep the relevant parts in the documentation.

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Sean Paul <sean@poorly.run>
Cc: Daniel Vetter <daniel@ffwll.ch>
---
 include/drm/drm_print.h | 84 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 80 insertions(+), 4 deletions(-)

Comments

Daniel Vetter Jan. 7, 2020, 4:08 p.m. UTC | #1 
On Thu, Jan 02, 2020 at 11:15:19PM +0100, Sam Ravnborg wrote:
> Document the remaining DRM_ logging functions.
> As the logging functions are now all properly
> listed drop the few specific kernel-doc markers
> so we keep the relevant parts in the documentation.
> 
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Sean Paul <sean@poorly.run>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> ---
>  include/drm/drm_print.h | 84 +++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 80 insertions(+), 4 deletions(-)
> 
> diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
> index 89e75eea65d2..abe247199bf7 100644
> --- a/include/drm/drm_print.h
> +++ b/include/drm/drm_print.h
> @@ -335,6 +335,82 @@ static inline struct drm_printer drm_err_printer(const char *prefix)
>   *
>   * See enum &drm_debug_category for a description of the categories.
>   *
> + * Logging when a &device * is available, but no &drm_device *
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + *
> + * DRM/Drivers can use the following functions for logging when there is a
> + * struct device * available.
> + * The logging functions share the same prototype:

I'd replace the entire block with a "This stuff is deprecated" warning. We
have at least a corresponding todo.rst entry.
-Daniel

> + *
> + * .. code-block:: c
> + *
> + *   void DRM_xxx(struct device *, char * fmt, ...)
> + *
> + * .. code-block:: none
> + *
> + *   # Plain logging
> + *   DRM_DEV_INFO(dev, fmt, ...)
> + *   DRM_DEV_ERROR(dev, fmt, ...)
> + *
> + *   # Log only once
> + *   DRM_DEV_INFO_ONCE(dev, fmt, ...)
> + *
> + *   # Ratelimited - do not flood the logs
> + *   DRM_DEV_DEBUG_RATELIMITED(dev, fmt, ...)
> + *   DRM_DEV_DEBUG_DRIVER_RATELIMITED(dev, fmt, ...)
> + *   DRM_DEV_DEBUG_KMS_RATELIMITED(dev, fmt, ...)
> + *   DRM_DEV_DEBUG_PRIME_RATELIMITED(dev, fmt, ...)
> + *   DRM_DEV_ERROR_RATELIMITED(dev, fmt, ...)
> + *
> + *   # Logging with a specific category
> + *   DRM_DEV_DEBUG(dev, fmt, ...)		# Logged as CORE
> + *   DRM_DEV_DEBUG_DRIVER(dev, fmt, ...)
> + *   DRM_DEV_DEBUG_KMS(dev, fmt, ...)
> + *   DRM_DEV_DEBUG_PRIME(dev, fmt, ...)
> + *   DRM_DEV_DEBUG_ATOMIC(dev, fmt, ...)
> + *   DRM_DEV_DEBUG_VBL(dev, fmt, ...)
> + *   DRM_DEV_DEBUG_DP(dev, fmt, ...)
> + *
> + * Logging when no &device * nor &drm_device * is available
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + *
> + * DRM/Drivers can use the following functions for logging when there is no
> + * extra info associated to the logging.
> + * The logging functions share the same prototype:
> + *
> + * .. code-block:: c
> + *
> + *   void DRM_xxx(char * fmt, ...)
> + *
> + * .. code-block:: none
> + *
> + *   # Plain logging
> + *   DRM_INFO(fmt, ...)
> + *   DRM_NOTE(fmt, ...)
> + *   DRM_WARN(fmt, ...)
> + *   DRM_ERROR(fmt, ...)
> + *
> + *   # Log only once
> + *   DRM_INFO_ONCE(fmt, ...)
> + *   DRM_NOTE_ONCE(fmt, ...)
> + *   DRM_WARN_ONCE(fmt, ...)
> + *
> + *   # Ratelimited - do not flood the logs
> + *   DRM_DEBUG_RATELIMITED(fmt, ...)
> + *   DRM_DEBUG_DRIVER_RATELIMITED(fmt, ...)
> + *   DRM_DEBUG_KMS_RATELIMITED(fmt, ...)
> + *   DRM_DEBUG_PRIME_RATELIMITED(fmt, ...)
> + *   DRM_ERROR_RATELIMITED(fmt, ...)
> + *
> + *   # Logging with a specific category
> + *   DRM_DEBUG(fmt, ...)		# Logged as CORE
> + *   DRM_DEBUG_DRIVER(fmt, ...)
> + *   DRM_DEBUG_KMS(fmt, ...)
> + *   DRM_DEBUG_PRIME(fmt, ...)
> + *   DRM_DEBUG_ATOMIC(fmt, ...)
> + *   DRM_DEBUG_VBL(fmt, ...)
> + *   DRM_DEBUG_LEASE(fmt, ...)
> + *   DRM_DEBUG_DP(fmt, ...)
>   */
>  
>  /**
> @@ -399,7 +475,7 @@ __printf(3, 4)
>  void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
>  		 const char *format, ...);
>  
> -/**
> +/*
>   * Error output.
>   *
>   * @dev: device pointer
> @@ -408,7 +484,7 @@ void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
>  #define DRM_DEV_ERROR(dev, fmt, ...)					\
>  	drm_dev_printk(dev, KERN_ERR, "*ERROR* " fmt, ##__VA_ARGS__)
>  
> -/**
> +/*
>   * Rate limited error output.  Like DRM_ERROR() but won't flood the log.
>   *
>   * @dev: device pointer
> @@ -436,7 +512,7 @@ void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
>  	}								\
>  })
>  
> -/**
> +/*
>   * Debug output.
>   *
>   * @dev: device pointer
> @@ -466,7 +542,7 @@ void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
>  		drm_dev_dbg(dev, category, fmt, ##__VA_ARGS__);		\
>  })
>  
> -/**
> +/*
>   * Rate limited debug output. Like DRM_DEBUG() but won't flood the log.
>   *
>   * @dev: device pointer
> -- 
> 2.20.1
>
Sam Ravnborg Jan. 7, 2020, 6:17 p.m. UTC | #2 
Hi Daniel.

> > + * Logging when a &device * is available, but no &drm_device *
> > + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > + *
> > + * DRM/Drivers can use the following functions for logging when there is a
> > + * struct device * available.
> > + * The logging functions share the same prototype:
> 
> I'd replace the entire block with a "This stuff is deprecated" warning. We
> have at least a corresponding todo.rst entry.

We have many situations where no drm_device is available.
At least when you a buried in drm_panel patches.

So it is either DRM_DEV_ERROR() or drv_err().
Which is why I have pushed for nicer drm_ variants of these...

The todo entry only covers the nice new macros that Jani added
where we have a drm_device.

	Sam



> -Daniel
> 
> > + *
> > + * .. code-block:: c
> > + *
> > + *   void DRM_xxx(struct device *, char * fmt, ...)
> > + *
> > + * .. code-block:: none
> > + *
> > + *   # Plain logging
> > + *   DRM_DEV_INFO(dev, fmt, ...)
> > + *   DRM_DEV_ERROR(dev, fmt, ...)
> > + *
> > + *   # Log only once
> > + *   DRM_DEV_INFO_ONCE(dev, fmt, ...)
> > + *
> > + *   # Ratelimited - do not flood the logs
> > + *   DRM_DEV_DEBUG_RATELIMITED(dev, fmt, ...)
> > + *   DRM_DEV_DEBUG_DRIVER_RATELIMITED(dev, fmt, ...)
> > + *   DRM_DEV_DEBUG_KMS_RATELIMITED(dev, fmt, ...)
> > + *   DRM_DEV_DEBUG_PRIME_RATELIMITED(dev, fmt, ...)
> > + *   DRM_DEV_ERROR_RATELIMITED(dev, fmt, ...)
> > + *
> > + *   # Logging with a specific category
> > + *   DRM_DEV_DEBUG(dev, fmt, ...)		# Logged as CORE
> > + *   DRM_DEV_DEBUG_DRIVER(dev, fmt, ...)
> > + *   DRM_DEV_DEBUG_KMS(dev, fmt, ...)
> > + *   DRM_DEV_DEBUG_PRIME(dev, fmt, ...)
> > + *   DRM_DEV_DEBUG_ATOMIC(dev, fmt, ...)
> > + *   DRM_DEV_DEBUG_VBL(dev, fmt, ...)
> > + *   DRM_DEV_DEBUG_DP(dev, fmt, ...)
> > + *
> > + * Logging when no &device * nor &drm_device * is available
> > + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > + *
> > + * DRM/Drivers can use the following functions for logging when there is no
> > + * extra info associated to the logging.
> > + * The logging functions share the same prototype:
> > + *
> > + * .. code-block:: c
> > + *
> > + *   void DRM_xxx(char * fmt, ...)
> > + *
> > + * .. code-block:: none
> > + *
> > + *   # Plain logging
> > + *   DRM_INFO(fmt, ...)
> > + *   DRM_NOTE(fmt, ...)
> > + *   DRM_WARN(fmt, ...)
> > + *   DRM_ERROR(fmt, ...)
> > + *
> > + *   # Log only once
> > + *   DRM_INFO_ONCE(fmt, ...)
> > + *   DRM_NOTE_ONCE(fmt, ...)
> > + *   DRM_WARN_ONCE(fmt, ...)
> > + *
> > + *   # Ratelimited - do not flood the logs
> > + *   DRM_DEBUG_RATELIMITED(fmt, ...)
> > + *   DRM_DEBUG_DRIVER_RATELIMITED(fmt, ...)
> > + *   DRM_DEBUG_KMS_RATELIMITED(fmt, ...)
> > + *   DRM_DEBUG_PRIME_RATELIMITED(fmt, ...)
> > + *   DRM_ERROR_RATELIMITED(fmt, ...)
> > + *
> > + *   # Logging with a specific category
> > + *   DRM_DEBUG(fmt, ...)		# Logged as CORE
> > + *   DRM_DEBUG_DRIVER(fmt, ...)
> > + *   DRM_DEBUG_KMS(fmt, ...)
> > + *   DRM_DEBUG_PRIME(fmt, ...)
> > + *   DRM_DEBUG_ATOMIC(fmt, ...)
> > + *   DRM_DEBUG_VBL(fmt, ...)
> > + *   DRM_DEBUG_LEASE(fmt, ...)
> > + *   DRM_DEBUG_DP(fmt, ...)
> >   */
> >  
> >  /**
> > @@ -399,7 +475,7 @@ __printf(3, 4)
> >  void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
> >  		 const char *format, ...);
> >  
> > -/**
> > +/*
> >   * Error output.
> >   *
> >   * @dev: device pointer
> > @@ -408,7 +484,7 @@ void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
> >  #define DRM_DEV_ERROR(dev, fmt, ...)					\
> >  	drm_dev_printk(dev, KERN_ERR, "*ERROR* " fmt, ##__VA_ARGS__)
> >  
> > -/**
> > +/*
> >   * Rate limited error output.  Like DRM_ERROR() but won't flood the log.
> >   *
> >   * @dev: device pointer
> > @@ -436,7 +512,7 @@ void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
> >  	}								\
> >  })
> >  
> > -/**
> > +/*
> >   * Debug output.
> >   *
> >   * @dev: device pointer
> > @@ -466,7 +542,7 @@ void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
> >  		drm_dev_dbg(dev, category, fmt, ##__VA_ARGS__);		\
> >  })
> >  
> > -/**
> > +/*
> >   * Rate limited debug output. Like DRM_DEBUG() but won't flood the log.
> >   *
> >   * @dev: device pointer
> > -- 
> > 2.20.1
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
Daniel Vetter Jan. 8, 2020, 6:49 p.m. UTC | #3 
On Tue, Jan 07, 2020 at 07:17:52PM +0100, Sam Ravnborg wrote:
> Hi Daniel.
> 
> > > + * Logging when a &device * is available, but no &drm_device *
> > > + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > + *
> > > + * DRM/Drivers can use the following functions for logging when there is a
> > > + * struct device * available.
> > > + * The logging functions share the same prototype:
> > 
> > I'd replace the entire block with a "This stuff is deprecated" warning. We
> > have at least a corresponding todo.rst entry.
> 
> We have many situations where no drm_device is available.
> At least when you a buried in drm_panel patches.
> 
> So it is either DRM_DEV_ERROR() or drv_err().
> Which is why I have pushed for nicer drm_ variants of these...

Huh, drm_panel indeed has no drm_device. And I guess we don't have a
convenient excuse to add it ...

> The todo entry only covers the nice new macros that Jani added
> where we have a drm_device.

I wonder whether for those cases we shouldn't just directly use the
various dev_* macros?
-Daniel

> 
> 	Sam
> 
> 
> 
> > -Daniel
> > 
> > > + *
> > > + * .. code-block:: c
> > > + *
> > > + *   void DRM_xxx(struct device *, char * fmt, ...)
> > > + *
> > > + * .. code-block:: none
> > > + *
> > > + *   # Plain logging
> > > + *   DRM_DEV_INFO(dev, fmt, ...)
> > > + *   DRM_DEV_ERROR(dev, fmt, ...)
> > > + *
> > > + *   # Log only once
> > > + *   DRM_DEV_INFO_ONCE(dev, fmt, ...)
> > > + *
> > > + *   # Ratelimited - do not flood the logs
> > > + *   DRM_DEV_DEBUG_RATELIMITED(dev, fmt, ...)
> > > + *   DRM_DEV_DEBUG_DRIVER_RATELIMITED(dev, fmt, ...)
> > > + *   DRM_DEV_DEBUG_KMS_RATELIMITED(dev, fmt, ...)
> > > + *   DRM_DEV_DEBUG_PRIME_RATELIMITED(dev, fmt, ...)
> > > + *   DRM_DEV_ERROR_RATELIMITED(dev, fmt, ...)
> > > + *
> > > + *   # Logging with a specific category
> > > + *   DRM_DEV_DEBUG(dev, fmt, ...)		# Logged as CORE
> > > + *   DRM_DEV_DEBUG_DRIVER(dev, fmt, ...)
> > > + *   DRM_DEV_DEBUG_KMS(dev, fmt, ...)
> > > + *   DRM_DEV_DEBUG_PRIME(dev, fmt, ...)
> > > + *   DRM_DEV_DEBUG_ATOMIC(dev, fmt, ...)
> > > + *   DRM_DEV_DEBUG_VBL(dev, fmt, ...)
> > > + *   DRM_DEV_DEBUG_DP(dev, fmt, ...)
> > > + *
> > > + * Logging when no &device * nor &drm_device * is available
> > > + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > + *
> > > + * DRM/Drivers can use the following functions for logging when there is no
> > > + * extra info associated to the logging.
> > > + * The logging functions share the same prototype:
> > > + *
> > > + * .. code-block:: c
> > > + *
> > > + *   void DRM_xxx(char * fmt, ...)
> > > + *
> > > + * .. code-block:: none
> > > + *
> > > + *   # Plain logging
> > > + *   DRM_INFO(fmt, ...)
> > > + *   DRM_NOTE(fmt, ...)
> > > + *   DRM_WARN(fmt, ...)
> > > + *   DRM_ERROR(fmt, ...)
> > > + *
> > > + *   # Log only once
> > > + *   DRM_INFO_ONCE(fmt, ...)
> > > + *   DRM_NOTE_ONCE(fmt, ...)
> > > + *   DRM_WARN_ONCE(fmt, ...)
> > > + *
> > > + *   # Ratelimited - do not flood the logs
> > > + *   DRM_DEBUG_RATELIMITED(fmt, ...)
> > > + *   DRM_DEBUG_DRIVER_RATELIMITED(fmt, ...)
> > > + *   DRM_DEBUG_KMS_RATELIMITED(fmt, ...)
> > > + *   DRM_DEBUG_PRIME_RATELIMITED(fmt, ...)
> > > + *   DRM_ERROR_RATELIMITED(fmt, ...)
> > > + *
> > > + *   # Logging with a specific category
> > > + *   DRM_DEBUG(fmt, ...)		# Logged as CORE
> > > + *   DRM_DEBUG_DRIVER(fmt, ...)
> > > + *   DRM_DEBUG_KMS(fmt, ...)
> > > + *   DRM_DEBUG_PRIME(fmt, ...)
> > > + *   DRM_DEBUG_ATOMIC(fmt, ...)
> > > + *   DRM_DEBUG_VBL(fmt, ...)
> > > + *   DRM_DEBUG_LEASE(fmt, ...)
> > > + *   DRM_DEBUG_DP(fmt, ...)
> > >   */
> > >  
> > >  /**
> > > @@ -399,7 +475,7 @@ __printf(3, 4)
> > >  void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
> > >  		 const char *format, ...);
> > >  
> > > -/**
> > > +/*
> > >   * Error output.
> > >   *
> > >   * @dev: device pointer
> > > @@ -408,7 +484,7 @@ void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
> > >  #define DRM_DEV_ERROR(dev, fmt, ...)					\
> > >  	drm_dev_printk(dev, KERN_ERR, "*ERROR* " fmt, ##__VA_ARGS__)
> > >  
> > > -/**
> > > +/*
> > >   * Rate limited error output.  Like DRM_ERROR() but won't flood the log.
> > >   *
> > >   * @dev: device pointer
> > > @@ -436,7 +512,7 @@ void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
> > >  	}								\
> > >  })
> > >  
> > > -/**
> > > +/*
> > >   * Debug output.
> > >   *
> > >   * @dev: device pointer
> > > @@ -466,7 +542,7 @@ void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
> > >  		drm_dev_dbg(dev, category, fmt, ##__VA_ARGS__);		\
> > >  })
> > >  
> > > -/**
> > > +/*
> > >   * Rate limited debug output. Like DRM_DEBUG() but won't flood the log.
> > >   *
> > >   * @dev: device pointer
> > > -- 
> > > 2.20.1
> > > 
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
Sam Ravnborg Jan. 8, 2020, 8:04 p.m. UTC | #4 
Hi Daniel.
> > > 
> > > I'd replace the entire block with a "This stuff is deprecated" warning. We
> > > have at least a corresponding todo.rst entry.
> > 
> > We have many situations where no drm_device is available.
> > At least when you a buried in drm_panel patches.
> > 
> > So it is either DRM_DEV_ERROR() or drv_err().
> > Which is why I have pushed for nicer drm_ variants of these...
> 
> Huh, drm_panel indeed has no drm_device. And I guess we don't have a
> convenient excuse to add it ...
> 
> > The todo entry only covers the nice new macros that Jani added
> > where we have a drm_device.
> 
> I wonder whether for those cases we shouldn't just directly use the
> various dev_* macros?

We would miss the nice [drm] marker in the logging.
So [drm] will be added by the drivers and the core - but not the panels.
That is the only drawback I see right now.

Which was enough justification for me to add the drm_dev_ variants.
Feel free to convince me that this is not justification to add these
variants.

In drm/panel/* there is no use of DRM_DEBUG* - and there is no
reason to introduce the variants we can filer with drm.debug.

There is a single DRM_DEBUG() user, which does not count here.


We could introduce only:

drm_dev_(err|warn|info|debug) - and not the more specialized variants.

Then we avoid that people make shortcuts and use drm_dev_dbg_kms() when
they are supposed to use drm_dbg_kms().
This was one of the very valid argumest against the patch that
introduced all the drm_dev_* variants.

	Sam
Joe Perches Jan. 8, 2020, 8:50 p.m. UTC | #5 
On Wed, 2020-01-08 at 21:04 +0100, Sam Ravnborg wrote:
> Hi Daniel.
> > > > I'd replace the entire block with a "This stuff is deprecated" warning. We
> > > > have at least a corresponding todo.rst entry.
> > > 
> > > We have many situations where no drm_device is available.
> > > At least when you a buried in drm_panel patches.
> > > 
> > > So it is either DRM_DEV_ERROR() or drv_err().
> > > Which is why I have pushed for nicer drm_ variants of these...
> > 
> > Huh, drm_panel indeed has no drm_device. And I guess we don't have a
> > convenient excuse to add it ...
> > 
> > > The todo entry only covers the nice new macros that Jani added
> > > where we have a drm_device.
> > 
> > I wonder whether for those cases we shouldn't just directly use the
> > various dev_* macros?
> 
> We would miss the nice [drm] marker in the logging.
> So [drm] will be added by the drivers and the core - but not the panels.
> That is the only drawback I see right now.
> 
> Which was enough justification for me to add the drm_dev_ variants.
> Feel free to convince me that this is not justification to add these
> variants.
> 
> In drm/panel/* there is no use of DRM_DEBUG* - and there is no
> reason to introduce the variants we can filer with drm.debug.
> 
> There is a single DRM_DEBUG() user, which does not count here.
> 
> 
> We could introduce only:
> 
> drm_dev_(err|warn|info|debug) - and not the more specialized variants.
> 
> Then we avoid that people make shortcuts and use drm_dev_dbg_kms() when
> they are supposed to use drm_dbg_kms().
> This was one of the very valid argumest against the patch that
> introduced all the drm_dev_* variants.

A few of these defines aren't used at all.

$ git grep -P -oh "DRM_DEV_DEBUG[A-Z_]*" | sort | uniq -c | sort -rn
     98 DRM_DEV_DEBUG_KMS
     38 DRM_DEV_DEBUG_DRIVER
     26 DRM_DEV_DEBUG
      2 DRM_DEV_DEBUG_RATELIMITED
      2 DRM_DEV_DEBUG_PRIME_RATELIMITED
      2 DRM_DEV_DEBUG_KMS_RATELIMITED
      2 DRM_DEV_DEBUG_DRIVER_RATELIMITED
      1 DRM_DEV_DEBUG_VBL
      1 DRM_DEV_DEBUG_PRIME
      1 DRM_DEV_DEBUG_DP
      1 DRM_DEV_DEBUG_ATOMIC

It might be sensible to consolidate these into just 2 calls
and add an appropriate <type> argument.

	DRM_DEV_DEBUG(dev, type, fmt, ...)
	DRM_DEV_DEBUG_RATELIMITED(dev, type, fmt, ...)

or

	drm_dev_dbg(dev, type, fmt, ...)
	drm_dev_dbg_ratelimited(dev, type, fmt, ...)

A treewide sed is trivial.
Daniel Vetter Jan. 12, 2020, 10:48 p.m. UTC | #6 
On Wed, Jan 08, 2020 at 09:04:16PM +0100, Sam Ravnborg wrote:
> Hi Daniel.
> > > > 
> > > > I'd replace the entire block with a "This stuff is deprecated" warning. We
> > > > have at least a corresponding todo.rst entry.
> > > 
> > > We have many situations where no drm_device is available.
> > > At least when you a buried in drm_panel patches.
> > > 
> > > So it is either DRM_DEV_ERROR() or drv_err().
> > > Which is why I have pushed for nicer drm_ variants of these...
> > 
> > Huh, drm_panel indeed has no drm_device. And I guess we don't have a
> > convenient excuse to add it ...
> > 
> > > The todo entry only covers the nice new macros that Jani added
> > > where we have a drm_device.
> > 
> > I wonder whether for those cases we shouldn't just directly use the
> > various dev_* macros?
> 
> We would miss the nice [drm] marker in the logging.
> So [drm] will be added by the drivers and the core - but not the panels.
> That is the only drawback I see right now.
> 
> Which was enough justification for me to add the drm_dev_ variants.
> Feel free to convince me that this is not justification to add these
> variants.

tbh I don't mind, I just don't want to have a massive proliferation of drm
debugging functions, all kinda alike but not the same. If we can settle on
some color choice for this bikeshed and actually reduce the not-longer
favoured version, I'm pretty much ok with anything.

> In drm/panel/* there is no use of DRM_DEBUG* - and there is no
> reason to introduce the variants we can filer with drm.debug.
> 
> There is a single DRM_DEBUG() user, which does not count here.
> 
> 
> We could introduce only:
> 
> drm_dev_(err|warn|info|debug) - and not the more specialized variants.
> 
> Then we avoid that people make shortcuts and use drm_dev_dbg_kms() when
> they are supposed to use drm_dbg_kms().
> This was one of the very valid argumest against the patch that
> introduced all the drm_dev_* variants.

Hm, if you want something for panels, what about a drm_panel_* set of
functions? Plus ofc patches to just roll them out everywhere (it should be
a simple sed/cocci job, drm/panel/* isn't that big). Some churn, but yay!
for consisteny at least.

If we have another set of generic drm debug/logging functions then I think
consistency is going to take a big toll, and we're already bad with this.
-Daniel
diff mbox series

Patch

diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
index 89e75eea65d2..abe247199bf7 100644
--- a/include/drm/drm_print.h
+++ b/include/drm/drm_print.h
@@ -335,6 +335,82 @@  static inline struct drm_printer drm_err_printer(const char *prefix)
  *
  * See enum &drm_debug_category for a description of the categories.
  *
+ * Logging when a &device * is available, but no &drm_device *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ * DRM/Drivers can use the following functions for logging when there is a
+ * struct device * available.
+ * The logging functions share the same prototype:
+ *
+ * .. code-block:: c
+ *
+ *   void DRM_xxx(struct device *, char * fmt, ...)
+ *
+ * .. code-block:: none
+ *
+ *   # Plain logging
+ *   DRM_DEV_INFO(dev, fmt, ...)
+ *   DRM_DEV_ERROR(dev, fmt, ...)
+ *
+ *   # Log only once
+ *   DRM_DEV_INFO_ONCE(dev, fmt, ...)
+ *
+ *   # Ratelimited - do not flood the logs
+ *   DRM_DEV_DEBUG_RATELIMITED(dev, fmt, ...)
+ *   DRM_DEV_DEBUG_DRIVER_RATELIMITED(dev, fmt, ...)
+ *   DRM_DEV_DEBUG_KMS_RATELIMITED(dev, fmt, ...)
+ *   DRM_DEV_DEBUG_PRIME_RATELIMITED(dev, fmt, ...)
+ *   DRM_DEV_ERROR_RATELIMITED(dev, fmt, ...)
+ *
+ *   # Logging with a specific category
+ *   DRM_DEV_DEBUG(dev, fmt, ...)		# Logged as CORE
+ *   DRM_DEV_DEBUG_DRIVER(dev, fmt, ...)
+ *   DRM_DEV_DEBUG_KMS(dev, fmt, ...)
+ *   DRM_DEV_DEBUG_PRIME(dev, fmt, ...)
+ *   DRM_DEV_DEBUG_ATOMIC(dev, fmt, ...)
+ *   DRM_DEV_DEBUG_VBL(dev, fmt, ...)
+ *   DRM_DEV_DEBUG_DP(dev, fmt, ...)
+ *
+ * Logging when no &device * nor &drm_device * is available
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ * DRM/Drivers can use the following functions for logging when there is no
+ * extra info associated to the logging.
+ * The logging functions share the same prototype:
+ *
+ * .. code-block:: c
+ *
+ *   void DRM_xxx(char * fmt, ...)
+ *
+ * .. code-block:: none
+ *
+ *   # Plain logging
+ *   DRM_INFO(fmt, ...)
+ *   DRM_NOTE(fmt, ...)
+ *   DRM_WARN(fmt, ...)
+ *   DRM_ERROR(fmt, ...)
+ *
+ *   # Log only once
+ *   DRM_INFO_ONCE(fmt, ...)
+ *   DRM_NOTE_ONCE(fmt, ...)
+ *   DRM_WARN_ONCE(fmt, ...)
+ *
+ *   # Ratelimited - do not flood the logs
+ *   DRM_DEBUG_RATELIMITED(fmt, ...)
+ *   DRM_DEBUG_DRIVER_RATELIMITED(fmt, ...)
+ *   DRM_DEBUG_KMS_RATELIMITED(fmt, ...)
+ *   DRM_DEBUG_PRIME_RATELIMITED(fmt, ...)
+ *   DRM_ERROR_RATELIMITED(fmt, ...)
+ *
+ *   # Logging with a specific category
+ *   DRM_DEBUG(fmt, ...)		# Logged as CORE
+ *   DRM_DEBUG_DRIVER(fmt, ...)
+ *   DRM_DEBUG_KMS(fmt, ...)
+ *   DRM_DEBUG_PRIME(fmt, ...)
+ *   DRM_DEBUG_ATOMIC(fmt, ...)
+ *   DRM_DEBUG_VBL(fmt, ...)
+ *   DRM_DEBUG_LEASE(fmt, ...)
+ *   DRM_DEBUG_DP(fmt, ...)
  */
 
 /**
@@ -399,7 +475,7 @@  __printf(3, 4)
 void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
 		 const char *format, ...);
 
-/**
+/*
  * Error output.
  *
  * @dev: device pointer
@@ -408,7 +484,7 @@  void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
 #define DRM_DEV_ERROR(dev, fmt, ...)					\
 	drm_dev_printk(dev, KERN_ERR, "*ERROR* " fmt, ##__VA_ARGS__)
 
-/**
+/*
  * Rate limited error output.  Like DRM_ERROR() but won't flood the log.
  *
  * @dev: device pointer
@@ -436,7 +512,7 @@  void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
 	}								\
 })
 
-/**
+/*
  * Debug output.
  *
  * @dev: device pointer
@@ -466,7 +542,7 @@  void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
 		drm_dev_dbg(dev, category, fmt, ##__VA_ARGS__);		\
 })
 
-/**
+/*
  * Rate limited debug output. Like DRM_DEBUG() but won't flood the log.
  *
  * @dev: device pointer