From patchwork Thu Mar 15 08:01:14 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Lucas De Marchi X-Patchwork-Id: 10284011 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 5CC87602C2 for ; Thu, 15 Mar 2018 08:01:19 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 4AD72287B9 for ; Thu, 15 Mar 2018 08:01:19 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 3E432288E4; Thu, 15 Mar 2018 08:01:19 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.2 required=2.0 tests=BAYES_00, RCVD_IN_DNSWL_MED autolearn=ham version=3.3.1 Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 09926287B9 for ; Thu, 15 Mar 2018 08:01:17 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 24A75898A8; Thu, 15 Mar 2018 08:01:17 +0000 (UTC) X-Original-To: intel-gfx@lists.freedesktop.org Delivered-To: intel-gfx@lists.freedesktop.org Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTPS id 86520898A8 for ; Thu, 15 Mar 2018 08:01:15 +0000 (UTC) X-Amp-Result: UNSCANNABLE X-Amp-File-Uploaded: False Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 15 Mar 2018 01:01:14 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.48,308,1517904000"; d="scan'208";a="42213542" Received: from goelabhi-mobl.amr.corp.intel.com (HELO ldmartin-desk.amr.corp.intel.com) ([10.254.177.7]) by orsmga002.jf.intel.com with ESMTP; 15 Mar 2018 01:01:14 -0700 Date: Thu, 15 Mar 2018 01:01:14 -0700 From: Lucas De Marchi To: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Message-ID: <20180315080114.GA6215@ldmartin-desk.amr.corp.intel.com> References: <20180314163132.4130-1-lucas.demarchi@intel.com> <20180314171918.GM5453@intel.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20180314171918.GM5453@intel.com> User-Agent: Mutt/1.9.2 (2017-12-15) Subject: Re: [Intel-gfx] [PATCH] drm/i915: Remove hole and padding from intel_shared_dpll X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: intel-gfx@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" X-Virus-Scanned: ClamAV using ClamSMTP On Wed, Mar 14, 2018 at 07:19:18PM +0200, Ville Syrjälä wrote: > On Wed, Mar 14, 2018 at 09:31:32AM -0700, Lucas De Marchi wrote: > > Reorder fields so we save 8 bytes per instance: this removes a 4-bytes > > hole after enum intel_dpll_id and a 4-bytes padding. > > > > Signed-off-by: Lucas De Marchi > > --- > > > > Is this something desirable? I happened to be looking at > > intel_shared_dpll and noticed the hole. I haven't checked any other struct > > yet, but there are probably more and more important ones. This one saves > > only 8 * I915_NUM_PLLS. > > > > drivers/gpu/drm/i915/intel_dpll_mgr.h | 10 +++++----- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h b/drivers/gpu/drm/i915/intel_dpll_mgr.h > > index f24ccf443d25..9635522dcb32 100644 > > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.h > > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h > > @@ -238,11 +238,6 @@ struct intel_shared_dpll { > > */ > > enum intel_dpll_id id; > > > > - /** > > - * @funcs: platform specific hooks > > - */ > > - struct intel_shared_dpll_funcs funcs; > > - > > #define INTEL_DPLL_ALWAYS_ON (1 << 0) > > /** > > * @flags: > > @@ -252,6 +247,11 @@ struct intel_shared_dpll { > > * not in use by any CRTC. > > */ > > uint32_t flags; > > + > > + /** > > + * @funcs: platform specific hooks > > + */ > > + struct intel_shared_dpll_funcs funcs; > > Why do we need to copy the entire thing here anyway? Can't we just > make this a pointer? We don't *need*, but probably because it was smaller and grew over time? Not checking its history now, just going straight to what's better. Looking at it I thought we wouldn't have much advantage because we would trade a few pointers, but increase .text to handle the indirections. Reality proves me wrong though. Here some measurements: drm_i915_private sizes: original size: 32104, cachelines: 502, members: 135 no-hole size: 32056, cachelines: 501, members: 135 funcs-ptr size: 31912, cachelines: 499, members: 135 and module sizes: text data bss dec hex filename 1767159 69516 5316 1841991 1c1b47 drivers/gpu/drm/i915/i915.ko.original 1767153 69516 5316 1841985 1c1b41 drivers/gpu/drm/i915/i915.ko.no-hole 1767095 69516 5316 1841927 1c1b07 drivers/gpu/drm/i915/i915.ko In the end it's just ~100 bytes from text and 100 from heap, but if the same approach is applied to other parts we can save a little bit more. Worth it? We can even (or alternatively) make dpll_info part of intel_shared_dpll. Below is the diff to make funcs a pointer on top of previous patch. Lucas De Marchi > > > }; > > > > #define SKL_DPLL0 0 > > -- > > 2.14.3 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel OTC Reviewed-by: Ville Syrjälä diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index e3ebb8ffa99e..a864b626650b 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -8768,8 +8768,8 @@ static bool ironlake_get_pipe_config(struct intel_crtc *crtc, intel_get_shared_dpll_by_id(dev_priv, pll_id); pll = pipe_config->shared_dpll; - WARN_ON(!pll->funcs.get_hw_state(dev_priv, pll, - &pipe_config->dpll_hw_state)); + WARN_ON(!pll->funcs->get_hw_state(dev_priv, pll, + &pipe_config->dpll_hw_state)); tmp = pipe_config->dpll_hw_state.dpll; pipe_config->pixel_multiplier = @@ -9245,8 +9245,8 @@ static void haswell_get_ddi_port_state(struct intel_crtc *crtc, pll = pipe_config->shared_dpll; if (pll) { - WARN_ON(!pll->funcs.get_hw_state(dev_priv, pll, - &pipe_config->dpll_hw_state)); + WARN_ON(!pll->funcs->get_hw_state(dev_priv, pll, + &pipe_config->dpll_hw_state)); } /* @@ -11654,7 +11654,7 @@ verify_single_dpll_state(struct drm_i915_private *dev_priv, DRM_DEBUG_KMS("%s\n", pll->name); - active = pll->funcs.get_hw_state(dev_priv, pll, &dpll_hw_state); + active = pll->funcs->get_hw_state(dev_priv, pll, &dpll_hw_state); if (!(pll->flags & INTEL_DPLL_ALWAYS_ON)) { I915_STATE_WARN(!pll->on && pll->active_mask, @@ -15123,8 +15123,8 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) for (i = 0; i < dev_priv->num_shared_dpll; i++) { struct intel_shared_dpll *pll = &dev_priv->shared_dplls[i]; - pll->on = pll->funcs.get_hw_state(dev_priv, pll, - &pll->state.hw_state); + pll->on = pll->funcs->get_hw_state(dev_priv, pll, + &pll->state.hw_state); pll->state.crtc_mask = 0; for_each_intel_crtc(dev, crtc) { struct intel_crtc_state *crtc_state = @@ -15313,7 +15313,7 @@ intel_modeset_setup_hw_state(struct drm_device *dev, DRM_DEBUG_KMS("%s enabled but not in use, disabling\n", pll->name); - pll->funcs.disable(dev_priv, pll); + pll->funcs->disable(dev_priv, pll); pll->on = false; } diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c index 51c5ae4e9116..46413797fbf1 100644 --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c @@ -118,7 +118,7 @@ void assert_shared_dpll(struct drm_i915_private *dev_priv, if (WARN(!pll, "asserting DPLL %s with no DPLL\n", onoff(state))) return; - cur_state = pll->funcs.get_hw_state(dev_priv, pll, &hw_state); + cur_state = pll->funcs->get_hw_state(dev_priv, pll, &hw_state); I915_STATE_WARN(cur_state != state, "%s assertion failure (expected %s, current %s)\n", pll->name, onoff(state), onoff(cur_state)); @@ -147,7 +147,7 @@ void intel_prepare_shared_dpll(struct intel_crtc *crtc) WARN_ON(pll->on); assert_shared_dpll_disabled(dev_priv, pll); - pll->funcs.prepare(dev_priv, pll); + pll->funcs->prepare(dev_priv, pll); } mutex_unlock(&dev_priv->dpll_lock); } @@ -190,7 +190,7 @@ void intel_enable_shared_dpll(struct intel_crtc *crtc) WARN_ON(pll->on); DRM_DEBUG_KMS("enabling %s\n", pll->name); - pll->funcs.enable(dev_priv, pll); + pll->funcs->enable(dev_priv, pll); pll->on = true; out: @@ -232,7 +232,7 @@ void intel_disable_shared_dpll(struct intel_crtc *crtc) goto out; DRM_DEBUG_KMS("disabling %s\n", pll->name); - pll->funcs.disable(dev_priv, pll); + pll->funcs->disable(dev_priv, pll); pll->on = false; out: @@ -2420,7 +2420,7 @@ void intel_shared_dpll_init(struct drm_device *dev) dev_priv->shared_dplls[i].id = dpll_info[i].id; dev_priv->shared_dplls[i].name = dpll_info[i].name; - dev_priv->shared_dplls[i].funcs = *dpll_info[i].funcs; + dev_priv->shared_dplls[i].funcs = dpll_info[i].funcs; dev_priv->shared_dplls[i].flags = dpll_info[i].flags; } diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h b/drivers/gpu/drm/i915/intel_dpll_mgr.h index 9635522dcb32..2b6c2a7d53fa 100644 --- a/drivers/gpu/drm/i915/intel_dpll_mgr.h +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h @@ -251,7 +251,7 @@ struct intel_shared_dpll { /** * @funcs: platform specific hooks */ - struct intel_shared_dpll_funcs funcs; + const struct intel_shared_dpll_funcs *funcs; }; #define SKL_DPLL0 0