Re: [PATCH] drm/i915: Remove hole and padding from intel_shared_dpll

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxx>
> > ---
> > 
> > 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.

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@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux