Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757007AbdCUI02 (ORCPT ); Tue, 21 Mar 2017 04:26:28 -0400 Received: from mga05.intel.com ([192.55.52.43]:4388 "EHLO mga05.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756171AbdCUI00 (ORCPT ); Tue, 21 Mar 2017 04:26:26 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.36,198,1486454400"; d="scan'208";a="836937113" From: Jani Nikula To: Arnd Bergmann , Daniel Vetter , David Airlie Cc: Arnd Bergmann , Mika Kuoppala , Ville =?utf-8?B?U3lyasOkbMOk?= , Chris Wilson , Imre Deak , Ander Conselvan de Oliveira , Robert Bragg , intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] drm/i915: use static const array for PICK macro In-Reply-To: <20170320215713.3086140-1-arnd@arndb.de> Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo References: <20170320215713.3086140-1-arnd@arndb.de> Date: Tue, 21 Mar 2017 10:26:21 +0200 Message-ID: <877f3javde.fsf@intel.com> MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5000 Lines: 109 On Mon, 20 Mar 2017, Arnd Bergmann 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 > Signed-off-by: Arnd Bergmann > --- > 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