| Message ID | 20180315080114.GA6215@ldmartin-desk.amr.corp.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Mar 15, 2018 at 01:01:14AM -0700, Lucas De Marchi wrote: > 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 <lucas.demarchi@intel.com> > > > --- > > > > > > 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. You mean something like? struct intel_shared_dpll { ... - id; - name; - flags; + const struct dpll_info *info; ... }; That would make sense to me since the info seems to be all read-only data. Oh and then we wouldn't even need the extra 'funcs' pointer. Some extra indirection there but this isn't performance sensitive or anything. Even if it wouldn't make things smaller I'd still like it just for the clarity of having all the read-only data being const. > > Below is the diff to make funcs a pointer on top of previous patch. This one is Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > 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 > > > 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
On Thu, Mar 15, 2018 at 11:54:19AM +0200, Ville Syrjälä wrote: > > We can even (or alternatively) make dpll_info part of intel_shared_dpll. > > You mean something like? > > struct intel_shared_dpll { > ... > - id; > - name; > - flags; > + const struct dpll_info *info; > ... > }; yep, that. > That would make sense to me since the info seems to be all read-only > data. Oh and then we wouldn't even need the extra 'funcs' pointer. > Some extra indirection there but this isn't performance sensitive or > anything. > > Even if it wouldn't make things smaller I'd still like it just for > the clarity of having all the read-only data being const. > > > > > Below is the diff to make funcs a pointer on top of previous patch. > > This one is > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Humn... do you mean the initial patch or the diff below? If I'm going to embed dpll_info inside intel_shared_dpll, this patch would be pointless. Lucas De Marchi
On Thu, Mar 15, 2018 at 11:07:04AM -0700, Lucas De Marchi wrote: > On Thu, Mar 15, 2018 at 11:54:19AM +0200, Ville Syrjälä wrote: > > > We can even (or alternatively) make dpll_info part of intel_shared_dpll. > > > > You mean something like? > > > > struct intel_shared_dpll { > > ... > > - id; > > - name; > > - flags; > > + const struct dpll_info *info; > > ... > > }; > > yep, that. > > > That would make sense to me since the info seems to be all read-only > > data. Oh and then we wouldn't even need the extra 'funcs' pointer. > > Some extra indirection there but this isn't performance sensitive or > > anything. > > > > Even if it wouldn't make things smaller I'd still like it just for > > the clarity of having all the read-only data being const. > > > > > > > > Below is the diff to make funcs a pointer on top of previous patch. > > > > This one is > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Humn... do you mean the initial patch or the diff below? The diff. > If I'm going to > embed dpll_info inside intel_shared_dpll, this patch would be pointless. Yeah. But I gave the r-b anyway in case you were going to stop here.
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