Re: [PATCH v7 1/9] drm_print: condense enum drm_debug_category

From: Jani Nikula <jani.nikula@linux.intel.com>
To: Jim Cromie <jim.cromie@gmail.com>,
	jbaron@akamai.com, gregkh@linuxfoundation.org,
	dri-devel@lists.freedesktop.org, amd-gfx@lists.freedesktop.org,
	intel-gvt-dev@lists.freedesktop.org,
	intel-gfx@lists.freedesktop.org, linux-kernel@vger.kernel.org
Cc: daniel.vetter@ffwll.ch, linux@rasmusvillemoes.dk,
	seanpaul@chromium.org, joe@perches.com
Subject: Re: [PATCH v7 1/9] drm_print: condense enum drm_debug_category
Date: Mon, 12 Sep 2022 13:17:34 +0300	[thread overview]
Message-ID: <87sfkw6gn5.fsf@intel.com> (raw)
In-Reply-To: <20220912052852.1123868-2-jim.cromie@gmail.com>

On Sun, 11 Sep 2022, Jim Cromie <jim.cromie@gmail.com> wrote:
> enum drm_debug_category has 10 categories, but is initialized with
> bitmasks which require 10 bits of underlying storage.  By using
> natural enumeration, and moving the BIT(cat) into drm_debug_enabled(),
> the enum fits in 4 bits, allowing the category to be represented
> directly in pr_debug callsites, via the ddebug.class_id field.
>
> While this slightly pessimizes the bit-test in drm_debug_enabled(),
> using dyndbg with JUMP_LABEL will avoid the function entirely.
>
> NOTE: this change forecloses the possibility of doing:
>
>   drm_dbg(DRM_UT_CORE|DRM_UT_KMS, "weird 2-cat experiment")
>
> but thats already strongly implied by the use of the enum itself; its
> not a normal enum if it can be 2 values simultaneously.

The drm.debug module parameter values are, arguably, ABI. There are tons
of people, scripts, test environments, documentation, bug reports, etc,
etc, referring to specific drm.debug module parameter values to enable
specific drm debug logging categories.

AFAICT you're not changing any of the values here, but having an enum
without the hard coded values makes it more likely to accidentally
change the category to bit mapping. At the very least deserves a
comment.

BR,
Jani.

>
> Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
> ---
>  include/drm/drm_print.h | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
> index 22fabdeed297..b3b470440e46 100644
> --- a/include/drm/drm_print.h
> +++ b/include/drm/drm_print.h
> @@ -279,49 +279,49 @@ enum drm_debug_category {
>  	 * @DRM_UT_CORE: Used in the generic drm code: drm_ioctl.c, drm_mm.c,
>  	 * drm_memory.c, ...
>  	 */
> -	DRM_UT_CORE		= 0x01,
> +	DRM_UT_CORE,
>  	/**
>  	 * @DRM_UT_DRIVER: Used in the vendor specific part of the driver: i915,
>  	 * radeon, ... macro.
>  	 */
> -	DRM_UT_DRIVER		= 0x02,
> +	DRM_UT_DRIVER,
>  	/**
>  	 * @DRM_UT_KMS: Used in the modesetting code.
>  	 */
> -	DRM_UT_KMS		= 0x04,
> +	DRM_UT_KMS,
>  	/**
>  	 * @DRM_UT_PRIME: Used in the prime code.
>  	 */
> -	DRM_UT_PRIME		= 0x08,
> +	DRM_UT_PRIME,
>  	/**
>  	 * @DRM_UT_ATOMIC: Used in the atomic code.
>  	 */
> -	DRM_UT_ATOMIC		= 0x10,
> +	DRM_UT_ATOMIC,
>  	/**
>  	 * @DRM_UT_VBL: Used for verbose debug message in the vblank code.
>  	 */
> -	DRM_UT_VBL		= 0x20,
> +	DRM_UT_VBL,
>  	/**
>  	 * @DRM_UT_STATE: Used for verbose atomic state debugging.
>  	 */
> -	DRM_UT_STATE		= 0x40,
> +	DRM_UT_STATE,
>  	/**
>  	 * @DRM_UT_LEASE: Used in the lease code.
>  	 */
> -	DRM_UT_LEASE		= 0x80,
> +	DRM_UT_LEASE,
>  	/**
>  	 * @DRM_UT_DP: Used in the DP code.
>  	 */
> -	DRM_UT_DP		= 0x100,
> +	DRM_UT_DP,
>  	/**
>  	 * @DRM_UT_DRMRES: Used in the drm managed resources code.
>  	 */
> -	DRM_UT_DRMRES		= 0x200,
> +	DRM_UT_DRMRES
>  };
>  
>  static inline bool drm_debug_enabled(enum drm_debug_category category)
>  {
> -	return unlikely(__drm_debug & category);
> +	return unlikely(__drm_debug & BIT(category));
>  }
>  
>  /*

-- 
Jani Nikula, Intel Open Source Graphics Center