[PATCH 0/2] Low hanging fruit to reduce the driver size

[PATCH 0/2] Low hanging fruit to reduce the driver size

[Damien Lespiau]

Being able to target a single platform to reduce the driver size has been
voiced a few times. These patches provide a Kconfig option to provide the
opportunity.

Let's start small, and, along side the generic "Multi-platform" option, only
present Haswell and Broadwell in the list of platforms to choose from. When
selecting any of those those two platforms, (S)DVO and MIPI DSI code is not
compiled.

The driver .text size is reduced by 67Kb (8.8%) when selecting HSW/BDW.

If the approach convinces, I'll do more.

-- 
Damien

Damien Lespiau (2):
  drm/i915: Provide a config option to select a target platform
  drm/i915: Don't compile the MIPI DSI code on HSW/BDW

 drivers/gpu/drm/i915/Kconfig     | 29 +++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/Makefile    | 26 +++++++++++++-------------
 drivers/gpu/drm/i915/intel_drv.h | 16 ++++++++++++++++
 3 files changed, 58 insertions(+), 13 deletions(-)

-- 
1.8.3.1

[PATCH 1/2] drm/i915: Provide a config option to select a target platform

[Damien Lespiau]

It'd be nice to be able to target a single platform to reduce the size
for the i915 driver.

Start low-fi and coarse grained: it's easy to split the (S)DVO code out,
the API surface is reduced to two init functions.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/Kconfig     | 25 +++++++++++++++++++++++++
 drivers/gpu/drm/i915/Makefile    | 18 +++++++++---------
 drivers/gpu/drm/i915/intel_drv.h | 12 ++++++++++++
 3 files changed, 46 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
index 437e182..b92a999 100644
--- a/drivers/gpu/drm/i915/Kconfig
+++ b/drivers/gpu/drm/i915/Kconfig
@@ -34,6 +34,31 @@ config DRM_I915
 	  i810 driver instead, and the Atom z5xx series has an entirely
 	  different implementation.
 
+config DRM_I915_DVO
+	bool
+
+choice
+	prompt "Platform support"
+	default DRM_I915_PLATFORM_ALL
+	help
+	  The i915 driver can be compiled for a single chip to reduce its .text
+	  size. The driver can only be compiled that way for a subset of the
+	  supported platforms.
+
+	  If in doubt, choose "Multi-platform".
+
+config DRM_I915_PLATFORM_ALL
+	bool "Multi-platform"
+	select DRM_I915_DVO
+
+config DRM_I915_PLATFORM_HASWELL
+	bool "Haswell"
+
+config DRM_I915_PLATFORM_BROADWELL
+	bool "Broadwell"
+
+endchoice
+
 config DRM_I915_KMS
 	bool "Enable modesetting on intel by default"
 	depends on DRM_I915
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index cad1683..346c3c1 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -50,25 +50,25 @@ i915-$(CONFIG_ACPI)		+= intel_acpi.o intel_opregion.o
 i915-$(CONFIG_DRM_I915_FBDEV)	+= intel_fbdev.o
 
 # modesetting output/encoder code
-i915-y += dvo_ch7017.o \
-	  dvo_ch7xxx.o \
-	  dvo_ivch.o \
-	  dvo_ns2501.o \
-	  dvo_sil164.o \
-	  dvo_tfp410.o \
-	  intel_crt.o \
+i915-$(CONFIG_DRM_I915_DVO) += dvo_ch7017.o \
+			       dvo_ch7xxx.o \
+			       dvo_ivch.o \
+			       dvo_ns2501.o \
+			       dvo_sil164.o \
+			       dvo_tfp410.o \
+			       intel_dvo.o \
+			       intel_sdvo.o
+i915-y += intel_crt.o \
 	  intel_ddi.o \
 	  intel_dp.o \
 	  intel_dsi_cmd.o \
 	  intel_dsi.o \
 	  intel_dsi_pll.o \
 	  intel_dsi_panel_vbt.o \
-	  intel_dvo.o \
 	  intel_hdmi.o \
 	  intel_i2c.o \
 	  intel_lvds.o \
 	  intel_panel.o \
-	  intel_sdvo.o \
 	  intel_tv.o
 
 # legacy horrors
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 81451e9..dd38095 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -871,7 +871,11 @@ void intel_dsi_init(struct drm_device *dev);
 
 
 /* intel_dvo.c */
+#ifdef CONFIG_DRM_I915_DVO
 void intel_dvo_init(struct drm_device *dev);
+#else
+static inline void intel_dvo_init(struct drm_device *dev) {}
+#endif
 
 
 /* legacy fbdev emulation in intel_fbdev.c */
@@ -1010,7 +1014,15 @@ void ilk_wm_get_hw_state(struct drm_device *dev);
 
 
 /* intel_sdvo.c */
+#ifdef CONFIG_DRM_I915_DVO
 bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob);
+#else
+static inline bool
+intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob)
+{
+	return false;
+}
+#endif
 
 
 /* intel_sprite.c */
-- 
1.8.3.1

[PATCH 2/2] drm/i915: Don't compile the MIPI DSI code on HSW/BDW

[Damien Lespiau]

We don't need MIPI DSI support on those platforms, so only select
DRM_I915_MIPI when compiling the driver with multi-plaform support.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/Kconfig     | 4 ++++
 drivers/gpu/drm/i915/Makefile    | 8 ++++----
 drivers/gpu/drm/i915/intel_drv.h | 4 ++++
 3 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
index b92a999..52ec05c 100644
--- a/drivers/gpu/drm/i915/Kconfig
+++ b/drivers/gpu/drm/i915/Kconfig
@@ -37,6 +37,9 @@ config DRM_I915
 config DRM_I915_DVO
 	bool
 
+config DRM_I915_MIPI
+	bool
+
 choice
 	prompt "Platform support"
 	default DRM_I915_PLATFORM_ALL
@@ -50,6 +53,7 @@ choice
 config DRM_I915_PLATFORM_ALL
 	bool "Multi-platform"
 	select DRM_I915_DVO
+	select DRM_I915_MIPI
 
 config DRM_I915_PLATFORM_HASWELL
 	bool "Haswell"
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 346c3c1..22f6a64 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -58,13 +58,13 @@ i915-$(CONFIG_DRM_I915_DVO) += dvo_ch7017.o \
 			       dvo_tfp410.o \
 			       intel_dvo.o \
 			       intel_sdvo.o
+i915-$(CONFIG_DRM_I915_MIPI) += intel_dsi_cmd.o \
+				intel_dsi.o \
+				intel_dsi_pll.o \
+				intel_dsi_panel_vbt.o
 i915-y += intel_crt.o \
 	  intel_ddi.o \
 	  intel_dp.o \
-	  intel_dsi_cmd.o \
-	  intel_dsi.o \
-	  intel_dsi_pll.o \
-	  intel_dsi_panel_vbt.o \
 	  intel_hdmi.o \
 	  intel_i2c.o \
 	  intel_lvds.o \
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index dd38095..e3f8d6e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -867,7 +867,11 @@ void intel_edp_psr_exit(struct drm_device *dev);
 void intel_edp_psr_init(struct drm_device *dev);
 
 /* intel_dsi.c */
+#ifdef CONFIG_DRM_I915_MIPI
 void intel_dsi_init(struct drm_device *dev);
+#else
+static inline void intel_dsi_init(struct drm_device *dev) {}
+#endif
 
 
 /* intel_dvo.c */
-- 
1.8.3.1

Re: [PATCH 0/2] Low hanging fruit to reduce the driver size

[Damien Lespiau]

On Fri, Jul 11, 2014 at 06:34:12PM +0100, Damien Lespiau wrote:
> If the approach convinces, I'll do more.

That sounds presumptuous, I more likely need to do more to convince.

I meant, I still need to have a look at the harder problem, providing a
static device_info structure and hardcoding various HAS_FOO() defines
for the selected platform.

-- 
Damien

Re: [PATCH 0/2] Low hanging fruit to reduce the driver size

[Daniel Vetter]

On Fri, Jul 11, 2014 at 06:34:12PM +0100, Damien Lespiau wrote:
> Being able to target a single platform to reduce the driver size has been
> voiced a few times. These patches provide a Kconfig option to provide the
> opportunity.
> 
> Let's start small, and, along side the generic "Multi-platform" option, only
> present Haswell and Broadwell in the list of platforms to choose from. When
> selecting any of those those two platforms, (S)DVO and MIPI DSI code is not
> compiled.
> 
> The driver .text size is reduced by 67Kb (8.8%) when selecting HSW/BDW.

Iirc my poor-man's-lto (include everything into one file) hack with a
similar scenario and hard-coding intel_info plus adding a few hacks to
help along gcc saved around 20-25% percent.

And I'm really not terribly convinced that kb shaving is worth the effort
overall, especially if it comes at a fairly steep maintainance burden of
piles of new config options and #defines.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 0/2] Low hanging fruit to reduce the driver size

[Damien Lespiau]

On Sat, Jul 12, 2014 at 01:01:42PM +0200, Daniel Vetter wrote:
> And I'm really not terribly convinced that kb shaving is worth the effort
> overall, especially if it comes at a fairly steep maintainance burden of
> piles of new config options and #defines.

I wanted to try, I tried, probably not worth it, next!

-- 
Damien

Re: [PATCH 1/2] drm/i915: Provide a config option to select a target platform

[Barbalho, Rafael]

> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf
> Of Damien Lespiau
> Sent: Friday, July 11, 2014 6:34 PM
> To: intel-gfx@lists.freedesktop.org
> Subject: [Intel-gfx] [PATCH 1/2] drm/i915: Provide a config option to select a
> target platform
> 
> It'd be nice to be able to target a single platform to reduce the size
> for the i915 driver.
> 
> Start low-fi and coarse grained: it's easy to split the (S)DVO code out,
> the API surface is reduced to two init functions.
> 
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>

Is this the right way to go?

I think what will happen is that we are just going to have everything enabled anyway because you'd want your kernel to boot on any intel platform. Certainly on the stuff I'm working on I'll just have to enable this stuff all the time because I don't know if the system image is going to be installed on a broadwell or a valleyview. The same comment applies to patch 2.

Thanks,
Rafael

> ---
>  drivers/gpu/drm/i915/Kconfig     | 25 +++++++++++++++++++++++++
>  drivers/gpu/drm/i915/Makefile    | 18 +++++++++---------
>  drivers/gpu/drm/i915/intel_drv.h | 12 ++++++++++++
>  3 files changed, 46 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
> index 437e182..b92a999 100644
> --- a/drivers/gpu/drm/i915/Kconfig
> +++ b/drivers/gpu/drm/i915/Kconfig
> @@ -34,6 +34,31 @@ config DRM_I915
>  	  i810 driver instead, and the Atom z5xx series has an entirely
>  	  different implementation.
> 
> +config DRM_I915_DVO
> +	bool
> +
> +choice
> +	prompt "Platform support"
> +	default DRM_I915_PLATFORM_ALL
> +	help
> +	  The i915 driver can be compiled for a single chip to reduce its .text
> +	  size. The driver can only be compiled that way for a subset of the
> +	  supported platforms.
> +
> +	  If in doubt, choose "Multi-platform".
> +
> +config DRM_I915_PLATFORM_ALL
> +	bool "Multi-platform"
> +	select DRM_I915_DVO
> +
> +config DRM_I915_PLATFORM_HASWELL
> +	bool "Haswell"
> +
> +config DRM_I915_PLATFORM_BROADWELL
> +	bool "Broadwell"
> +
> +endchoice
> +
>  config DRM_I915_KMS
>  	bool "Enable modesetting on intel by default"
>  	depends on DRM_I915
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index cad1683..346c3c1 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -50,25 +50,25 @@ i915-$(CONFIG_ACPI)		+= intel_acpi.o
> intel_opregion.o
>  i915-$(CONFIG_DRM_I915_FBDEV)	+= intel_fbdev.o
> 
>  # modesetting output/encoder code
> -i915-y += dvo_ch7017.o \
> -	  dvo_ch7xxx.o \
> -	  dvo_ivch.o \
> -	  dvo_ns2501.o \
> -	  dvo_sil164.o \
> -	  dvo_tfp410.o \
> -	  intel_crt.o \
> +i915-$(CONFIG_DRM_I915_DVO) += dvo_ch7017.o \
> +			       dvo_ch7xxx.o \
> +			       dvo_ivch.o \
> +			       dvo_ns2501.o \
> +			       dvo_sil164.o \
> +			       dvo_tfp410.o \
> +			       intel_dvo.o \
> +			       intel_sdvo.o
> +i915-y += intel_crt.o \
>  	  intel_ddi.o \
>  	  intel_dp.o \
>  	  intel_dsi_cmd.o \
>  	  intel_dsi.o \
>  	  intel_dsi_pll.o \
>  	  intel_dsi_panel_vbt.o \
> -	  intel_dvo.o \
>  	  intel_hdmi.o \
>  	  intel_i2c.o \
>  	  intel_lvds.o \
>  	  intel_panel.o \
> -	  intel_sdvo.o \
>  	  intel_tv.o
> 
>  # legacy horrors
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 81451e9..dd38095 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -871,7 +871,11 @@ void intel_dsi_init(struct drm_device *dev);
> 
> 
>  /* intel_dvo.c */
> +#ifdef CONFIG_DRM_I915_DVO
>  void intel_dvo_init(struct drm_device *dev);
> +#else
> +static inline void intel_dvo_init(struct drm_device *dev) {}
> +#endif
> 
> 
>  /* legacy fbdev emulation in intel_fbdev.c */
> @@ -1010,7 +1014,15 @@ void ilk_wm_get_hw_state(struct drm_device
> *dev);
> 
> 
>  /* intel_sdvo.c */
> +#ifdef CONFIG_DRM_I915_DVO
>  bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool
> is_sdvob);
> +#else
> +static inline bool
> +intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob)
> +{
> +	return false;
> +}
> +#endif
> 
> 
>  /* intel_sprite.c */
> --
> 1.8.3.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Provide a config option to select a target platform

[Damien Lespiau]

On Mon, Jul 14, 2014 at 11:18:30AM +0100, Barbalho, Rafael wrote:
> 
> 
> > -----Original Message-----
> > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf
> > Of Damien Lespiau
> > Sent: Friday, July 11, 2014 6:34 PM
> > To: intel-gfx@lists.freedesktop.org
> > Subject: [Intel-gfx] [PATCH 1/2] drm/i915: Provide a config option to select a
> > target platform
> > 
> > It'd be nice to be able to target a single platform to reduce the size
> > for the i915 driver.
> > 
> > Start low-fi and coarse grained: it's easy to split the (S)DVO code out,
> > the API surface is reduced to two init functions.
> > 
> > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> 
> Is this the right way to go?
> 
> I think what will happen is that we are just going to have everything
> enabled anyway because you'd want your kernel to boot on any intel
> platform. Certainly on the stuff I'm working on I'll just have to
> enable this stuff all the time because I don't know if the system
> image is going to be installed on a broadwell or a valleyview. The
> same comment applies to patch 2.

The patches are already dropped. I guess it depends what you want to do
really. I can see a specific product, say running on Valleyview, that
could turn that on. It sounds likely that people will at least have a
per-platform config file (if not per-platform kernels), which could have
this tweak.

I still think we could save 300+ Kb if taking this to its limits, but
meh.

-- 
Damien