Re: [PATCH 1/8] drm/print: introduce new struct drm_device based logging macros

From: Jani Nikula <jani.nikula@intel.com>
To: Sam Ravnborg <sam@ravnborg.org>
Cc: intel-gfx@lists.freedesktop.org, Sean Paul <sean@poorly.run>,
	dri-devel@lists.freedesktop.org,
	Rodrigo Vivi <rodrigo.vivi@intel.com>
Subject: Re: [PATCH 1/8] drm/print: introduce new struct drm_device based logging macros
Date: Fri, 13 Dec 2019 16:41:39 +0200	[thread overview]
Message-ID: <87tv641e58.fsf@intel.com> (raw)
In-Reply-To: <20191212215303.GA11520@ravnborg.org>

On Thu, 12 Dec 2019, Sam Ravnborg <sam@ravnborg.org> wrote:
> 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@ravnborg.org>
>> Acked-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Acked-by: Sean Paul <sean@poorly.run>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>
> 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(..)

The main concern I have here is that converting the last two will lead
to a lot of churn with no functional change. And instead of three sets
of functions, we'd actually have five, until all of it is converted.

Also it seems to me adding the last two will validate their widespread
use, while personally I think the focus should be on transforming to use
the struct drm_device based functions introduced in this series.

Maybe we should wait until we have enough code converted to the struct
drm_device based functions, so that the rest could perhaps be converted
en masse using cocci?

Other than that, I could easily get behind your proposal.

> 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@ravnborg.org>

Thanks, appreciated!

Still, while I'd really just like to merge this now and get done with
it, I think I'd rather have more acks behind this (at the risk of
soliciting bikeshedding...) so nobody feels like I'm sneaking this in.

BR,
Jani.

>
>
> 	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

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel