From: Jani Nikula <jani.nikula@linux.intel.com> To: Arnd Bergmann <arnd@arndb.de>, Daniel Vetter <daniel.vetter@intel.com>, David Airlie <airlied@linux.ie> Cc: "Arnd Bergmann" <arnd@arndb.de>, "Mika Kuoppala" <mika.kuoppala@linux.intel.com>, "Ville Syrjälä" <ville.syrjala@linux.intel.com>, "Chris Wilson" <chris@chris-wilson.co.uk>, "Imre Deak" <imre.deak@intel.com>, "Ander Conselvan de Oliveira" <ander.conselvan.de.oliveira@intel.com>, "Robert Bragg" <robert@sixbynine.org>, 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 Date: Tue, 21 Mar 2017 10:26:21 +0200 [thread overview] Message-ID: <877f3javde.fsf@intel.com> (raw) In-Reply-To: <20170320215713.3086140-1-arnd@arndb.de> On Mon, 20 Mar 2017, Arnd Bergmann <arnd@arndb.de> 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@linux.intel.com> > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > 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
WARNING: multiple messages have this Message-ID (diff)
From: Jani Nikula <jani.nikula@linux.intel.com> To: Daniel Vetter <daniel.vetter@intel.com>, David Airlie <airlied@linux.ie> Cc: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>, Arnd Bergmann <arnd@arndb.de>, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, intel-gfx@lists.freedesktop.org Subject: Re: [PATCH] drm/i915: use static const array for PICK macro Date: Tue, 21 Mar 2017 10:26:21 +0200 [thread overview] Message-ID: <877f3javde.fsf@intel.com> (raw) In-Reply-To: <20170320215713.3086140-1-arnd@arndb.de> On Mon, 20 Mar 2017, Arnd Bergmann <arnd@arndb.de> 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@linux.intel.com> > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2017-03-21 8:26 UTC|newest] Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-03-20 21:56 [PATCH] drm/i915: use static const array for PICK macro Arnd Bergmann 2017-03-20 21:56 ` Arnd Bergmann 2017-03-20 23:28 ` ✗ Fi.CI.BAT: warning for " Patchwork 2017-03-21 8:26 ` Jani Nikula [this message] 2017-03-21 8:26 ` [PATCH] " Jani Nikula 2017-03-21 8:44 ` Arnd Bergmann 2017-03-21 8:44 ` Arnd Bergmann 2017-03-21 10:33 ` [Intel-gfx] " Daniel Vetter 2017-03-21 10:33 ` Daniel Vetter 2017-03-21 11:23 ` [Intel-gfx] " Jani Nikula 2017-03-21 11:23 ` Jani Nikula 2017-03-31 13:37 ` Arnd Bergmann 2017-03-31 13:37 ` Arnd Bergmann 2017-12-11 12:44 ` Arnd Bergmann 2017-12-11 12:44 ` Arnd Bergmann 2017-03-21 13:16 ` ✗ Fi.CI.BAT: warning for " Patchwork
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=877f3javde.fsf@intel.com \ --to=jani.nikula@linux.intel.com \ --cc=airlied@linux.ie \ --cc=ander.conselvan.de.oliveira@intel.com \ --cc=arnd@arndb.de \ --cc=chris@chris-wilson.co.uk \ --cc=daniel.vetter@intel.com \ --cc=dri-devel@lists.freedesktop.org \ --cc=imre.deak@intel.com \ --cc=intel-gfx@lists.freedesktop.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mika.kuoppala@linux.intel.com \ --cc=robert@sixbynine.org \ --cc=ville.syrjala@linux.intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.