Hi Jani. On Tue, Dec 10, 2019 at 02:30:43PM +0200, Jani Nikula wrote: > Add new struct drm_device based logging macros modeled after the core > kernel device based logging macros. These would be preferred over the > drm printk and struct device based macros in drm code, where possible. > > We have existing drm specific struct device based logging functions, but > they are too verbose to use for two main reasons: > > * The names are unnecessarily long, for example DRM_DEV_DEBUG_KMS(). > > * The use of struct device over struct drm_device is too generic for > most users, leading to an extra dereference. > > For example: > > DRM_DEV_DEBUG_KMS(drm->dev, "Hello, world\n"); > > vs. > > drm_dbg_kms(drm, "Hello, world\n"); > > It's a matter of taste, but the SHOUTING UPPERCASE has been argued to be > less readable than lowercase. > > Some names are changed from old DRM names to be based on the core kernel > logging functions. For example, NOTE -> notice, ERROR -> err, DEBUG -> > dbg. > > Due to the conflation of DRM_DEBUG and DRM_DEBUG_DRIVER macro use > (DRM_DEBUG is used widely in drivers though it's supposed to be a core > debugging category), they are named as drm_dbg_core and drm_dbg, > respectively. > > The drm_err and _once/_ratelimited variants no longer include the > function name in order to be able to use the core device based logging > macros. Arguably this is not a significant change; error messages should > not be so common to be only distinguishable by the function name. > > Ratelimited debug logging macros are to be added later. > > Cc: Sam Ravnborg <sam@xxxxxxxxxxxx> > Acked-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> > Acked-by: Sean Paul <sean@xxxxxxxxxx> > Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx> To my sensitive eyes the lower case variants are much preferable. As a follow-up it could be nice to clean up drm_print.h: - Make it obvious that the old variants are deprecated - Let the old variants use the new variants - to make it obvious they are obsolete wrappers. - Add some intro that explains for newbies when to use what variant And then add a todo item - so we can get some janitorials to help with the conversion to the new varaints. For logging we have three cases: - We have a drm_device pointer - nicely covered by this patchset - We have a device * - what do we do here? - We have no pointers to device nor drm_device - what do we do here? Would it be OK to consider drm variants for all the above - so we get consistent prefix on logging? Idea: drm_<level>[_system] - example: drm_info(drm_device *, ..) or drm_info_core(drm_device *, ..) drm_dev_<level>[_system] - example: drm_dev_info(device *, ..) drm_pr_<level>[_system] - example: drm_pr_info(..) level could be info, info_once, info_ratelimited and so on for dbg, err, notice, warn With the above I can see we can make a clean shift to drm based logging. And we do not need to mix different ways to log stuf. The preferred: drm_info() drm_dev_info() drm_pr_info() versus: drm_info() dev_info() pr_info() The patch is OK without the suggested change, but see this as suggestions for improvements. So patch as is has my: Acked-by: Sam Ravnborg <sam@xxxxxxxxxxxx> Sam > > --- > > With something like this, I think i915 could start migrating to > drm_device based logging. I have a hard time convincing myself or anyone > about migrating to the DRM_DEV_* variants. > --- > include/drm/drm_print.h | 65 +++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 65 insertions(+) > > diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h > index 085a9685270c..8f99d389792d 100644 > --- a/include/drm/drm_print.h > +++ b/include/drm/drm_print.h > @@ -322,6 +322,8 @@ static inline bool drm_debug_enabled(enum drm_debug_category category) > > /* > * struct device based logging > + * > + * Prefer drm_device based logging over device or prink based logging. > */ > > __printf(3, 4) > @@ -417,8 +419,71 @@ void drm_dev_dbg(const struct device *dev, enum drm_debug_category category, > _DRM_DEV_DEFINE_DEBUG_RATELIMITED(dev, DRM_UT_PRIME, \ > fmt, ##__VA_ARGS__) > > +/* > + * struct drm_device based logging > + * > + * Prefer drm_device based logging over device or prink based logging. > + */ > + > +/* Helper for struct drm_device based logging. */ > +#define __drm_printk(drm, level, type, fmt, ...) \ > + dev_##level##type((drm)->dev, "[drm] " fmt, ##__VA_ARGS__) > + > + > +#define drm_info(drm, fmt, ...) \ > + __drm_printk((drm), info,, fmt, ##__VA_ARGS__) > + > +#define drm_notice(drm, fmt, ...) \ > + __drm_printk((drm), notice,, fmt, ##__VA_ARGS__) > + > +#define drm_warn(drm, fmt, ...) \ > + __drm_printk((drm), warn,, fmt, ##__VA_ARGS__) > + > +#define drm_err(drm, fmt, ...) \ > + __drm_printk((drm), err,, "*ERROR* " fmt, ##__VA_ARGS__) > + > + > +#define drm_info_once(drm, fmt, ...) \ > + __drm_printk((drm), info, _once, fmt, ##__VA_ARGS__) > + > +#define drm_notice_once(drm, fmt, ...) \ > + __drm_printk((drm), notice, _once, fmt, ##__VA_ARGS__) > + > +#define drm_warn_once(drm, fmt, ...) \ > + __drm_printk((drm), warn, _once, fmt, ##__VA_ARGS__) > + > +#define drm_err_once(drm, fmt, ...) \ > + __drm_printk((drm), err, _once, "*ERROR* " fmt, ##__VA_ARGS__) > + > + > +#define drm_err_ratelimited(drm, fmt, ...) \ > + __drm_printk((drm), err, _ratelimited, "*ERROR* " fmt, ##__VA_ARGS__) > + > + > +#define drm_dbg_core(drm, fmt, ...) \ > + drm_dev_dbg((drm)->dev, DRM_UT_CORE, fmt, ##__VA_ARGS__) > +#define drm_dbg(drm, fmt, ...) \ > + drm_dev_dbg((drm)->dev, DRM_UT_DRIVER, fmt, ##__VA_ARGS__) > +#define drm_dbg_kms(drm, fmt, ...) \ > + drm_dev_dbg((drm)->dev, DRM_UT_KMS, fmt, ##__VA_ARGS__) > +#define drm_dbg_prime(drm, fmt, ...) \ > + drm_dev_dbg((drm)->dev, DRM_UT_PRIME, fmt, ##__VA_ARGS__) > +#define drm_dbg_atomic(drm, fmt, ...) \ > + drm_dev_dbg((drm)->dev, DRM_UT_ATOMIC, fmt, ##__VA_ARGS__) > +#define drm_dbg_vbl(drm, fmt, ...) \ > + drm_dev_dbg((drm)->dev, DRM_UT_VBL, fmt, ##__VA_ARGS__) > +#define drm_dbg_state(drm, fmt, ...) \ > + drm_dev_dbg((drm)->dev, DRM_UT_STATE, fmt, ##__VA_ARGS__) > +#define drm_dbg_lease(drm, fmt, ...) \ > + drm_dev_dbg((drm)->dev, DRM_UT_LEASE, fmt, ##__VA_ARGS__) > +#define drm_dbg_dp(drm, fmt, ...) \ > + drm_dev_dbg((drm)->dev, DRM_UT_DP, fmt, ##__VA_ARGS__) > + > + > /* > * printk based logging > + * > + * Prefer drm_device based logging over device or prink based logging. > */ > > __printf(2, 3) > -- > 2.20.1 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
