Re: [PATCH v5 01/16] drm/i915/display: remove enum macro magic in intel_display_wa()

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 05 Mar 2026, Luca Coelho <luciano.coelho@intel.com> wrote:
> There's not much use in passing a number to the macro and let it
> convert that into the enum and a string.  It just hides the symbols.

There was a point, though. Passing a random number to intel_display_wa()
would fail the build. Now you'll only find out runtime. I'll concede
there's value, perhaps more so, in having the symbols not hidden.

> Remove the number to enum conversion magic in intel_display_wa().
>
> This has the side-effect of changing the print in the drm_WARN() that
> is issued when the number is not implemented, but that is moot anyway
> and can be changed later to something cleaner if needed.

I actually wonder if having the names there is really worth it. It's
just a bunch of rodata bloat for not much benefit. It should be easy
enough to find which enumerator isn't handled.

Maybe check the objdump bloat reduction with the intel_display_wa()
wrapper macro and the const char *name parameter removed?

BR,
Jani.

>
> Reviewed-by: Suraj Kandpal <suraj.kandpal@intel.com>
> Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_cdclk.c              | 5 +++--
>  drivers/gpu/drm/i915/display/intel_display.c            | 2 +-
>  drivers/gpu/drm/i915/display/intel_display_power_well.c | 4 ++--
>  drivers/gpu/drm/i915/display/intel_display_wa.c         | 2 +-
>  drivers/gpu/drm/i915/display/intel_display_wa.h         | 2 +-
>  drivers/gpu/drm/i915/display/intel_fbc.c                | 8 ++++----
>  drivers/gpu/drm/i915/display/intel_gmbus.c              | 6 +++---
>  drivers/gpu/drm/i915/display/skl_scaler.c               | 2 +-
>  8 files changed, 16 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> index 38331e899519..7767f8c198da 100644
> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> @@ -1870,7 +1870,7 @@ static void icl_cdclk_pll_disable(struct intel_display *display)
>  	 *      after the PLL is enabled (which is already done as part of the
>  	 *      normal flow of _bxt_set_cdclk()).
>  	 */
> -	if (intel_display_wa(display, 13012396614))
> +	if (intel_display_wa(display, INTEL_DISPLAY_WA_13012396614))
>  		intel_de_rmw(display, CDCLK_CTL, MDCLK_SOURCE_SEL_MASK, MDCLK_SOURCE_SEL_CD2XCLK);
>  
>  	intel_de_rmw(display, BXT_DE_PLL_ENABLE,
> @@ -2186,7 +2186,8 @@ static u32 bxt_cdclk_ctl(struct intel_display *display,
>  		 * icl_cdclk_pll_disable().  Here we are just making sure
>  		 * we keep the expected value.
>  		 */
> -		if (intel_display_wa(display, 13012396614) && vco == 0)
> +		if (intel_display_wa(display, INTEL_DISPLAY_WA_13012396614) &&
> +		    vco == 0)
>  			val |= MDCLK_SOURCE_SEL_CD2XCLK;
>  		else
>  			val |= xe2lpd_mdclk_source_sel(display);
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 138ee7dd1977..15edf609fff4 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -1070,7 +1070,7 @@ static void intel_post_plane_update(struct intel_atomic_state *state,
>  	if (audio_enabling(old_crtc_state, new_crtc_state))
>  		intel_encoders_audio_enable(state, crtc);
>  
> -	if (intel_display_wa(display, 14011503117)) {
> +	if (intel_display_wa(display, INTEL_DISPLAY_WA_14011503117)) {
>  		if (old_crtc_state->pch_pfit.enabled != new_crtc_state->pch_pfit.enabled)
>  			adl_scaler_ecc_unmask(new_crtc_state);
>  	}
> diff --git a/drivers/gpu/drm/i915/display/intel_display_power_well.c b/drivers/gpu/drm/i915/display/intel_display_power_well.c
> index 9c8d29839caf..1e03187dbd38 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power_well.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_power_well.c
> @@ -249,7 +249,7 @@ static void hsw_power_well_post_enable(struct intel_display *display,
>  	if (irq_pipe_mask) {
>  		gen8_irq_power_well_post_enable(display, irq_pipe_mask);
>  
> -		if (intel_display_wa(display, 22021048059))
> +		if (intel_display_wa(display, INTEL_DISPLAY_WA_22021048059))
>  			dss_pipe_gating_enable_disable(display, irq_pipe_mask, false);
>  	}
>  }
> @@ -258,7 +258,7 @@ static void hsw_power_well_pre_disable(struct intel_display *display,
>  				       u8 irq_pipe_mask)
>  {
>  	if (irq_pipe_mask) {
> -		if (intel_display_wa(display, 22021048059))
> +		if (intel_display_wa(display, INTEL_DISPLAY_WA_22021048059))
>  			dss_pipe_gating_enable_disable(display, irq_pipe_mask, true);
>  
>  		gen8_irq_power_well_pre_disable(display, irq_pipe_mask);
> diff --git a/drivers/gpu/drm/i915/display/intel_display_wa.c b/drivers/gpu/drm/i915/display/intel_display_wa.c
> index c2ccdca2c2f3..1d8340b36c01 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_wa.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_wa.c
> @@ -87,7 +87,7 @@ bool __intel_display_wa(struct intel_display *display, enum intel_display_wa wa,
>  	case INTEL_DISPLAY_WA_22021048059:
>  		return IS_DISPLAY_VER(display, 14, 35);
>  	default:
> -		drm_WARN(display->drm, 1, "Missing Wa number: %s\n", name);
> +		drm_WARN(display->drm, 1, "Missing Wa: %s\n", name);
>  		break;
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_display_wa.h b/drivers/gpu/drm/i915/display/intel_display_wa.h
> index 767420d5f406..06c1f62c0f6d 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_wa.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_wa.h
> @@ -40,6 +40,6 @@ enum intel_display_wa {
>  bool __intel_display_wa(struct intel_display *display, enum intel_display_wa wa, const char *name);
>  
>  #define intel_display_wa(__display, __wa) \
> -	__intel_display_wa((__display), INTEL_DISPLAY_WA_##__wa, __stringify(__wa))
> +	__intel_display_wa((__display), __wa, __stringify(__wa))
>  
>  #endif
> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
> index 3e9b3e532499..5d0d7c1027f5 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> @@ -957,7 +957,7 @@ static void intel_fbc_program_workarounds(struct intel_fbc *fbc)
>  	 * Fixes: Screen flicker with FBC and Package C state enabled
>  	 * Workaround: Forced SLB invalidation before start of new frame.
>  	 */
> -	if (intel_display_wa(display, 22014263786))
> +	if (intel_display_wa(display, INTEL_DISPLAY_WA_22014263786))
>  		intel_de_rmw(display, ILK_DPFC_CHICKEN(fbc->id),
>  			     0, DPFC_CHICKEN_FORCE_SLB_INVALIDATION);
>  
> @@ -979,7 +979,7 @@ static void fbc_sys_cache_update_config(struct intel_display *display, u32 reg,
>  	 * Fixes: SoC hardware issue in read caching
>  	 * Workaround: disable cache read setting which is enabled by default.
>  	 */
> -	if (!intel_display_wa(display, 14025769978))
> +	if (!intel_display_wa(display, INTEL_DISPLAY_WA_14025769978))
>  		/* Cache read enable is set by default */
>  		reg |= FBC_SYS_CACHE_READ_ENABLE;
>  
> @@ -1612,7 +1612,7 @@ static int intel_fbc_check_plane(struct intel_atomic_state *state,
>  		return 0;
>  	}
>  
> -	if (intel_display_wa(display, 16023588340)) {
> +	if (intel_display_wa(display, INTEL_DISPLAY_WA_16023588340)) {
>  		plane_state->no_fbc_reason = "Wa_16023588340";
>  		return 0;
>  	}
> @@ -1622,7 +1622,7 @@ static int intel_fbc_check_plane(struct intel_atomic_state *state,
>  	 * Fixes: Underrun during media decode
>  	 * Workaround: Do not enable FBC
>  	 */
> -	if (intel_display_wa(display, 15018326506)) {
> +	if (intel_display_wa(display, INTEL_DISPLAY_WA_15018326506)) {
>  		plane_state->no_fbc_reason = "Wa_15018326506";
>  		return 0;
>  	}
> diff --git a/drivers/gpu/drm/i915/display/intel_gmbus.c b/drivers/gpu/drm/i915/display/intel_gmbus.c
> index 38706017c0c6..df48f27f1cc1 100644
> --- a/drivers/gpu/drm/i915/display/intel_gmbus.c
> +++ b/drivers/gpu/drm/i915/display/intel_gmbus.c
> @@ -250,7 +250,7 @@ static u32 get_reserved(struct intel_gmbus *bus)
>  	preserve_bits |= GPIO_DATA_PULLUP_DISABLE | GPIO_CLOCK_PULLUP_DISABLE;
>  
>  	/* Wa_16025573575: the masks bits need to be preserved through out */
> -	if (intel_display_wa(display, 16025573575))
> +	if (intel_display_wa(display, INTEL_DISPLAY_WA_16025573575))
>  		preserve_bits |= GPIO_CLOCK_DIR_MASK | GPIO_CLOCK_VAL_MASK |
>  				 GPIO_DATA_DIR_MASK | GPIO_DATA_VAL_MASK;
>  
> @@ -342,7 +342,7 @@ intel_gpio_pre_xfer(struct i2c_adapter *adapter)
>  	if (display->platform.pineview)
>  		pnv_gmbus_clock_gating(display, false);
>  
> -	if (intel_display_wa(display, 16025573575))
> +	if (intel_display_wa(display, INTEL_DISPLAY_WA_16025573575))
>  		ptl_handle_mask_bits(bus, true);
>  
>  	set_data(bus, 1);
> @@ -363,7 +363,7 @@ intel_gpio_post_xfer(struct i2c_adapter *adapter)
>  	if (display->platform.pineview)
>  		pnv_gmbus_clock_gating(display, true);
>  
> -	if (intel_display_wa(display, 16025573575))
> +	if (intel_display_wa(display, INTEL_DISPLAY_WA_16025573575))
>  		ptl_handle_mask_bits(bus, false);
>  }
>  
> diff --git a/drivers/gpu/drm/i915/display/skl_scaler.c b/drivers/gpu/drm/i915/display/skl_scaler.c
> index 4c4deac7f9c8..7c5cb188ebf0 100644
> --- a/drivers/gpu/drm/i915/display/skl_scaler.c
> +++ b/drivers/gpu/drm/i915/display/skl_scaler.c
> @@ -823,7 +823,7 @@ void skl_pfit_enable(const struct intel_crtc_state *crtc_state)
>  			crtc_state->scaler_state.scaler_id < 0))
>  		return;
>  
> -	if (intel_display_wa(display, 14011503117))
> +	if (intel_display_wa(display, INTEL_DISPLAY_WA_14011503117))
>  		adl_scaler_ecc_mask(crtc_state);
>  
>  	drm_rect_init(&src, 0, 0,

-- 
Jani Nikula, Intel