Re: [PATCH] drm/i915: use static const array for PICK macro

[Jani Nikula]

On Mon, 20 Mar 2017, Arnd Bergmann <arnd@xxxxxxxx> wrote:
> The varargs macro trick in _PIPE3/_PHY3/_PORT3 was meant as an optimization
> to shrink the i915 kernel module by around 1000 bytes.

Really, I didn't care one bit about the size shrink, I only cared about
making it easier and less error prone to increase the number of args in
a number of places. Maintainability and correctness were the goals. Just
for the record. ;)

Otherwise, this seems like an acceptable approach to me. Would be
interesting to see what happens with this, and f4c3a88e5f04 ("drm/i915:
Tighten mmio arrays for MIPI_PORT") reverted on top.

BR,
Jani.



> However, the
> downside is a size regression with CONFIG_KASAN, as I found from stack size
> warnings with gcc-7.0.1:
>
> before:
> drivers/gpu/drm/i915/intel_dpll_mgr.c: In function 'bxt_ddi_pll_get_hw_state':
> drivers/gpu/drm/i915/intel_dpll_mgr.c:1644:1: error: the frame size of 176 bytes is larger than 100 bytes [-Werror=frame-larger-than=]
> drivers/gpu/drm/i915/intel_dpll_mgr.c: In function 'bxt_ddi_pll_enable':
> drivers/gpu/drm/i915/intel_dpll_mgr.c:1548:1: error: the frame size of 224 bytes is larger than 100 bytes [-Werror=frame-larger-than=]
>
> after:
> drivers/gpu/drm/i915/intel_dpll_mgr.c: In function 'bxt_ddi_pll_get_hw_state':
> drivers/gpu/drm/i915/intel_dpll_mgr.c:1644:1: error: the frame size of 1016 bytes is larger than 1000 bytes [-Werror=frame-larger-than=]
> drivers/gpu/drm/i915/intel_dpll_mgr.c: In function 'bxt_ddi_pll_enable':
> drivers/gpu/drm/i915/intel_dpll_mgr.c:1548:1: error: the frame size of 1960 bytes is larger than 1000 bytes [-Werror=frame-larger-than=]
>
> I also checked the module sizes and got with gcc-7.0.1
>
> original:
>    text	   data	    bss	    dec	    hex	filename
> 2380830	1155436	   4448	3540714	 3606ea	drivers/gpu/drm/i915/i915-kasan.o
> 1298054	 543692	   2884	1844630	 1c2596	drivers/gpu/drm/i915/i915-nokasan.o
>
> after ce64645d86ac:
>    text	   data	    bss	    dec	    hex	filename
> 2389515	1154476	   4448	3548439	 362517	drivers/gpu/drm/i915/i915-kasan.o
> 1299639	 543692	   2884	1846215	 1c2bc7	drivers/gpu/drm/i915/i915-nokasan.o
>
> with this patch:
>    text	   data	    bss	    dec	    hex	filename
> 2381275	1163884	   4448	3549607	 3629a7	drivers/gpu/drm/i915/i915-kasan.o
> 1296038	 543692	   2884	1842614	 1c1db6	drivers/gpu/drm/i915/i915-nokasan.o
>
> Actually showing a code size growth in .text both with and without kasan,
> and my version gets most of it back at the expense of larger .data when
> kasan is enabled.
>
> Fixes: ce64645d86ac ("drm/i915: use variadic macros and arrays to choose port/pipe based registers")
> Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80114
> Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
> Signed-off-by: Arnd Bergmann <arnd@xxxxxxxx>
> ---
>  drivers/gpu/drm/i915/i915_reg.h | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 04c8f69fcc62..39b53878a188 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -48,7 +48,7 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
>  	return !i915_mmio_reg_equal(reg, INVALID_MMIO_REG);
>  }
>  
> -#define _PICK(__index, ...) (((const u32 []){ __VA_ARGS__ })[__index])
> +#define _PICK(__index, ...) ({static const u32 __arr[] = { __VA_ARGS__ }; __arr[__index];})
>  
>  #define _PIPE(pipe, a, b) ((a) + (pipe)*((b)-(a)))
>  #define _MMIO_PIPE(pipe, a, b) _MMIO(_PIPE(pipe, a, b))
> @@ -2657,10 +2657,10 @@ enum skl_disp_power_wells {
>  /*
>   * Clock control & power management
>   */
> -#define _DPLL_A (dev_priv->info.display_mmio_offset + 0x6014)
> -#define _DPLL_B (dev_priv->info.display_mmio_offset + 0x6018)
> -#define _CHV_DPLL_C (dev_priv->info.display_mmio_offset + 0x6030)
> -#define DPLL(pipe) _MMIO_PIPE3((pipe), _DPLL_A, _DPLL_B, _CHV_DPLL_C)
> +#define _DPLL_A			0x6014
> +#define _DPLL_B			0x6018
> +#define _CHV_DPLL_C		0x6030
> +#define DPLL(pipe) _MMIO(dev_priv->info.display_mmio_offset + _PIPE3((pipe), _DPLL_A, _DPLL_B, _CHV_DPLL_C))
>  
>  #define VGA0	_MMIO(0x6000)
>  #define VGA1	_MMIO(0x6004)
> @@ -2756,10 +2756,10 @@ enum skl_disp_power_wells {
>  #define   SDVO_MULTIPLIER_SHIFT_HIRES		4
>  #define   SDVO_MULTIPLIER_SHIFT_VGA		0
>  
> -#define _DPLL_A_MD (dev_priv->info.display_mmio_offset + 0x601c)
> -#define _DPLL_B_MD (dev_priv->info.display_mmio_offset + 0x6020)
> -#define _CHV_DPLL_C_MD (dev_priv->info.display_mmio_offset + 0x603c)
> -#define DPLL_MD(pipe) _MMIO_PIPE3((pipe), _DPLL_A_MD, _DPLL_B_MD, _CHV_DPLL_C_MD)
> +#define _DPLL_A_MD				0x601c
> +#define _DPLL_B_MD				0x6020
> +#define _CHV_DPLL_C_MD				0x603c
> +#define DPLL_MD(pipe) _MMIO(dev_priv->info.display_mmio_offset + _PIPE3((pipe), _DPLL_A_MD, _DPLL_B_MD, _CHV_DPLL_C_MD))
>  
>  /*
>   * UDI pixel divider, controlling how many pixels are stuffed into a packet.

-- 
Jani Nikula, Intel Open Source Technology Center