[PATCH] drm/sun4i: fix build failure with CONFIG_DRM_SUN8I_MIXER=m

[PATCH] drm/sun4i: fix build failure with CONFIG_DRM_SUN8I_MIXER=m

[Arnd Bergmann]

Having DRM_SUN4I built-in but DRM_SUN8I_MIXER as a loadable module results in
a link error, as we try to access a symbol from the sun8i_tcon_top.ko module:

ERROR: "sun8i_tcon_top_of_table" [drivers/gpu/drm/sun4i/sun8i-drm-hdmi.ko] undefined!
ERROR: "sun8i_tcon_top_of_table" [drivers/gpu/drm/sun4i/sun4i-drm.ko] undefined!

This solves the problem by making DRM_SUN8I_MIXER a 'bool' symbol, building
the sun8i_tcon_top module the same way as the core sun4i-drm module whenever
DRM_SUN8I_MIXER is enabled, or not building it at all otherwise.

Alternatively, we could always build sun8i_tcon_top.ko along with sun4-drm.ko
and detach it from the mixer module, I could not tell which way is more
appropriate here.

Fixes: 57e23de02f48 ("drm/sun4i: DW HDMI: Expand algorithm for possible crtcs")
Fixes: ef0cf6441fbb ("drm/sun4i: Add support for traversing graph with TCON TOP")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/gpu/drm/sun4i/Kconfig         | 2 +-
 drivers/gpu/drm/sun4i/Makefile        | 4 +++-
 drivers/gpu/drm/sun4i/sun4i_drv.c     | 3 ++-
 drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c | 3 ++-
 4 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/Kconfig b/drivers/gpu/drm/sun4i/Kconfig
index 156a865c3e6d..709461deb1c0 100644
--- a/drivers/gpu/drm/sun4i/Kconfig
+++ b/drivers/gpu/drm/sun4i/Kconfig
@@ -60,7 +60,7 @@ config DRM_SUN8I_DW_HDMI
 	  selected the module will be called sun8i_dw_hdmi.
 
 config DRM_SUN8I_MIXER
-	tristate "Support for Allwinner Display Engine 2.0 Mixer"
+	bool "Support for Allwinner Display Engine 2.0 Mixer"
 	default MACH_SUN8I
 	help
 	  Choose this option if you have an Allwinner SoC with the
diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile
index cd27d02c94e2..11de3cd52dae 100644
--- a/drivers/gpu/drm/sun4i/Makefile
+++ b/drivers/gpu/drm/sun4i/Makefile
@@ -39,4 +39,6 @@ endif
 obj-$(CONFIG_DRM_SUN4I_HDMI)	+= sun4i-drm-hdmi.o
 obj-$(CONFIG_DRM_SUN6I_DSI)	+= sun6i-dsi.o
 obj-$(CONFIG_DRM_SUN8I_DW_HDMI)	+= sun8i-drm-hdmi.o
-obj-$(CONFIG_DRM_SUN8I_MIXER)	+= sun8i-mixer.o sun8i_tcon_top.o
+ifdef CONFIG_DRM_SUN8I_MIXER
+obj-$(CONFIG_DRM_SUN4I)		+= sun8i-mixer.o sun8i_tcon_top.o
+endif
diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c b/drivers/gpu/drm/sun4i/sun4i_drv.c
index 6ddf4eaccb40..7551dcb34c71 100644
--- a/drivers/gpu/drm/sun4i/sun4i_drv.c
+++ b/drivers/gpu/drm/sun4i/sun4i_drv.c
@@ -216,7 +216,8 @@ static bool sun4i_drv_node_is_tcon_with_ch0(struct device_node *node)
 
 static bool sun4i_drv_node_is_tcon_top(struct device_node *node)
 {
-	return !!of_match_node(sun8i_tcon_top_of_table, node);
+	return IS_ENABLED(CONFIG_DRM_SUN8I_MIXER) &&
+		!!of_match_node(sun8i_tcon_top_of_table, node);
 }
 
 static int compare_of(struct device *dev, void *data)
diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
index 3459b9ec56c9..b18c8f175dba 100644
--- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
+++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
@@ -44,7 +44,8 @@ sun8i_dw_hdmi_mode_valid(struct drm_connector *connector,
 
 static bool sun8i_dw_hdmi_node_is_tcon_top(struct device_node *node)
 {
-	return !!of_match_node(sun8i_tcon_top_of_table, node);
+	return IS_ENABLED(CONFIG_DRM_SUN8I_MIXER) &&
+		!!of_match_node(sun8i_tcon_top_of_table, node);
 }
 
 static u32 sun8i_dw_hdmi_find_possible_crtcs(struct drm_device *drm,
-- 
2.9.0

[PATCH] drm/sun4i: fix build failure with CONFIG_DRM_SUN8I_MIXER=m

[Arnd Bergmann]

Having DRM_SUN4I built-in but DRM_SUN8I_MIXER as a loadable module results in
a link error, as we try to access a symbol from the sun8i_tcon_top.ko module:

ERROR: "sun8i_tcon_top_of_table" [drivers/gpu/drm/sun4i/sun8i-drm-hdmi.ko] undefined!
ERROR: "sun8i_tcon_top_of_table" [drivers/gpu/drm/sun4i/sun4i-drm.ko] undefined!

This solves the problem by making DRM_SUN8I_MIXER a 'bool' symbol, building
the sun8i_tcon_top module the same way as the core sun4i-drm module whenever
DRM_SUN8I_MIXER is enabled, or not building it at all otherwise.

Alternatively, we could always build sun8i_tcon_top.ko along with sun4-drm.ko
and detach it from the mixer module, I could not tell which way is more
appropriate here.

Fixes: 57e23de02f48 ("drm/sun4i: DW HDMI: Expand algorithm for possible crtcs")
Fixes: ef0cf6441fbb ("drm/sun4i: Add support for traversing graph with TCON TOP")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/gpu/drm/sun4i/Kconfig         | 2 +-
 drivers/gpu/drm/sun4i/Makefile        | 4 +++-
 drivers/gpu/drm/sun4i/sun4i_drv.c     | 3 ++-
 drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c | 3 ++-
 4 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/Kconfig b/drivers/gpu/drm/sun4i/Kconfig
index 156a865c3e6d..709461deb1c0 100644
--- a/drivers/gpu/drm/sun4i/Kconfig
+++ b/drivers/gpu/drm/sun4i/Kconfig
@@ -60,7 +60,7 @@ config DRM_SUN8I_DW_HDMI
 	  selected the module will be called sun8i_dw_hdmi.
 
 config DRM_SUN8I_MIXER
-	tristate "Support for Allwinner Display Engine 2.0 Mixer"
+	bool "Support for Allwinner Display Engine 2.0 Mixer"
 	default MACH_SUN8I
 	help
 	  Choose this option if you have an Allwinner SoC with the
diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile
index cd27d02c94e2..11de3cd52dae 100644
--- a/drivers/gpu/drm/sun4i/Makefile
+++ b/drivers/gpu/drm/sun4i/Makefile
@@ -39,4 +39,6 @@ endif
 obj-$(CONFIG_DRM_SUN4I_HDMI)	+= sun4i-drm-hdmi.o
 obj-$(CONFIG_DRM_SUN6I_DSI)	+= sun6i-dsi.o
 obj-$(CONFIG_DRM_SUN8I_DW_HDMI)	+= sun8i-drm-hdmi.o
-obj-$(CONFIG_DRM_SUN8I_MIXER)	+= sun8i-mixer.o sun8i_tcon_top.o
+ifdef CONFIG_DRM_SUN8I_MIXER
+obj-$(CONFIG_DRM_SUN4I)		+= sun8i-mixer.o sun8i_tcon_top.o
+endif
diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c b/drivers/gpu/drm/sun4i/sun4i_drv.c
index 6ddf4eaccb40..7551dcb34c71 100644
--- a/drivers/gpu/drm/sun4i/sun4i_drv.c
+++ b/drivers/gpu/drm/sun4i/sun4i_drv.c
@@ -216,7 +216,8 @@ static bool sun4i_drv_node_is_tcon_with_ch0(struct device_node *node)
 
 static bool sun4i_drv_node_is_tcon_top(struct device_node *node)
 {
-	return !!of_match_node(sun8i_tcon_top_of_table, node);
+	return IS_ENABLED(CONFIG_DRM_SUN8I_MIXER) &&
+		!!of_match_node(sun8i_tcon_top_of_table, node);
 }
 
 static int compare_of(struct device *dev, void *data)
diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
index 3459b9ec56c9..b18c8f175dba 100644
--- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
+++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
@@ -44,7 +44,8 @@ sun8i_dw_hdmi_mode_valid(struct drm_connector *connector,
 
 static bool sun8i_dw_hdmi_node_is_tcon_top(struct device_node *node)
 {
-	return !!of_match_node(sun8i_tcon_top_of_table, node);
+	return IS_ENABLED(CONFIG_DRM_SUN8I_MIXER) &&
+		!!of_match_node(sun8i_tcon_top_of_table, node);
 }
 
 static u32 sun8i_dw_hdmi_find_possible_crtcs(struct drm_device *drm,
-- 
2.9.0

[PATCH] drm/sun4i: fix build failure with CONFIG_DRM_SUN8I_MIXER=m

[Arnd Bergmann]

Having DRM_SUN4I built-in but DRM_SUN8I_MIXER as a loadable module results in
a link error, as we try to access a symbol from the sun8i_tcon_top.ko module:

ERROR: "sun8i_tcon_top_of_table" [drivers/gpu/drm/sun4i/sun8i-drm-hdmi.ko] undefined!
ERROR: "sun8i_tcon_top_of_table" [drivers/gpu/drm/sun4i/sun4i-drm.ko] undefined!

This solves the problem by making DRM_SUN8I_MIXER a 'bool' symbol, building
the sun8i_tcon_top module the same way as the core sun4i-drm module whenever
DRM_SUN8I_MIXER is enabled, or not building it at all otherwise.

Alternatively, we could always build sun8i_tcon_top.ko along with sun4-drm.ko
and detach it from the mixer module, I could not tell which way is more
appropriate here.

Fixes: 57e23de02f48 ("drm/sun4i: DW HDMI: Expand algorithm for possible crtcs")
Fixes: ef0cf6441fbb ("drm/sun4i: Add support for traversing graph with TCON TOP")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/gpu/drm/sun4i/Kconfig         | 2 +-
 drivers/gpu/drm/sun4i/Makefile        | 4 +++-
 drivers/gpu/drm/sun4i/sun4i_drv.c     | 3 ++-
 drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c | 3 ++-
 4 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/Kconfig b/drivers/gpu/drm/sun4i/Kconfig
index 156a865c3e6d..709461deb1c0 100644
--- a/drivers/gpu/drm/sun4i/Kconfig
+++ b/drivers/gpu/drm/sun4i/Kconfig
@@ -60,7 +60,7 @@ config DRM_SUN8I_DW_HDMI
 	  selected the module will be called sun8i_dw_hdmi.
 
 config DRM_SUN8I_MIXER
-	tristate "Support for Allwinner Display Engine 2.0 Mixer"
+	bool "Support for Allwinner Display Engine 2.0 Mixer"
 	default MACH_SUN8I
 	help
 	  Choose this option if you have an Allwinner SoC with the
diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile
index cd27d02c94e2..11de3cd52dae 100644
--- a/drivers/gpu/drm/sun4i/Makefile
+++ b/drivers/gpu/drm/sun4i/Makefile
@@ -39,4 +39,6 @@ endif
 obj-$(CONFIG_DRM_SUN4I_HDMI)	+= sun4i-drm-hdmi.o
 obj-$(CONFIG_DRM_SUN6I_DSI)	+= sun6i-dsi.o
 obj-$(CONFIG_DRM_SUN8I_DW_HDMI)	+= sun8i-drm-hdmi.o
-obj-$(CONFIG_DRM_SUN8I_MIXER)	+= sun8i-mixer.o sun8i_tcon_top.o
+ifdef CONFIG_DRM_SUN8I_MIXER
+obj-$(CONFIG_DRM_SUN4I)		+= sun8i-mixer.o sun8i_tcon_top.o
+endif
diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c b/drivers/gpu/drm/sun4i/sun4i_drv.c
index 6ddf4eaccb40..7551dcb34c71 100644
--- a/drivers/gpu/drm/sun4i/sun4i_drv.c
+++ b/drivers/gpu/drm/sun4i/sun4i_drv.c
@@ -216,7 +216,8 @@ static bool sun4i_drv_node_is_tcon_with_ch0(struct device_node *node)
 
 static bool sun4i_drv_node_is_tcon_top(struct device_node *node)
 {
-	return !!of_match_node(sun8i_tcon_top_of_table, node);
+	return IS_ENABLED(CONFIG_DRM_SUN8I_MIXER) &&
+		!!of_match_node(sun8i_tcon_top_of_table, node);
 }
 
 static int compare_of(struct device *dev, void *data)
diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
index 3459b9ec56c9..b18c8f175dba 100644
--- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
+++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
@@ -44,7 +44,8 @@ sun8i_dw_hdmi_mode_valid(struct drm_connector *connector,
 
 static bool sun8i_dw_hdmi_node_is_tcon_top(struct device_node *node)
 {
-	return !!of_match_node(sun8i_tcon_top_of_table, node);
+	return IS_ENABLED(CONFIG_DRM_SUN8I_MIXER) &&
+		!!of_match_node(sun8i_tcon_top_of_table, node);
 }
 
 static u32 sun8i_dw_hdmi_find_possible_crtcs(struct drm_device *drm,
-- 
2.9.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/sun4i: fix build failure with CONFIG_DRM_SUN8I_MIXER=m

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1064 bytes --]

On Fri, Jul 06, 2018 at 02:45:53PM +0200, Arnd Bergmann wrote:
> Having DRM_SUN4I built-in but DRM_SUN8I_MIXER as a loadable module results in
> a link error, as we try to access a symbol from the sun8i_tcon_top.ko module:
> 
> ERROR: "sun8i_tcon_top_of_table" [drivers/gpu/drm/sun4i/sun8i-drm-hdmi.ko] undefined!
> ERROR: "sun8i_tcon_top_of_table" [drivers/gpu/drm/sun4i/sun4i-drm.ko] undefined!
> 
> This solves the problem by making DRM_SUN8I_MIXER a 'bool' symbol, building
> the sun8i_tcon_top module the same way as the core sun4i-drm module whenever
> DRM_SUN8I_MIXER is enabled, or not building it at all otherwise.
> 
> Alternatively, we could always build sun8i_tcon_top.ko along with sun4-drm.ko
> and detach it from the mixer module, I could not tell which way is more
> appropriate here.

If that's easily doable, then yeah, that would be the preferred option
I guess. Jernej? Chen-Yu? Any opinion on this?

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH] drm/sun4i: fix build failure with CONFIG_DRM_SUN8I_MIXER=m

[Maxime Ripard]

On Fri, Jul 06, 2018 at 02:45:53PM +0200, Arnd Bergmann wrote:
> Having DRM_SUN4I built-in but DRM_SUN8I_MIXER as a loadable module results in
> a link error, as we try to access a symbol from the sun8i_tcon_top.ko module:
> 
> ERROR: "sun8i_tcon_top_of_table" [drivers/gpu/drm/sun4i/sun8i-drm-hdmi.ko] undefined!
> ERROR: "sun8i_tcon_top_of_table" [drivers/gpu/drm/sun4i/sun4i-drm.ko] undefined!
> 
> This solves the problem by making DRM_SUN8I_MIXER a 'bool' symbol, building
> the sun8i_tcon_top module the same way as the core sun4i-drm module whenever
> DRM_SUN8I_MIXER is enabled, or not building it at all otherwise.
> 
> Alternatively, we could always build sun8i_tcon_top.ko along with sun4-drm.ko
> and detach it from the mixer module, I could not tell which way is more
> appropriate here.

If that's easily doable, then yeah, that would be the preferred option
I guess. Jernej? Chen-Yu? Any opinion on this?

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180709/1dbcfbfc/attachment.sig>

Re: [PATCH] drm/sun4i: fix build failure with CONFIG_DRM_SUN8I_MIXER=m

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 1064 bytes --]

On Fri, Jul 06, 2018 at 02:45:53PM +0200, Arnd Bergmann wrote:
> Having DRM_SUN4I built-in but DRM_SUN8I_MIXER as a loadable module results in
> a link error, as we try to access a symbol from the sun8i_tcon_top.ko module:
> 
> ERROR: "sun8i_tcon_top_of_table" [drivers/gpu/drm/sun4i/sun8i-drm-hdmi.ko] undefined!
> ERROR: "sun8i_tcon_top_of_table" [drivers/gpu/drm/sun4i/sun4i-drm.ko] undefined!
> 
> This solves the problem by making DRM_SUN8I_MIXER a 'bool' symbol, building
> the sun8i_tcon_top module the same way as the core sun4i-drm module whenever
> DRM_SUN8I_MIXER is enabled, or not building it at all otherwise.
> 
> Alternatively, we could always build sun8i_tcon_top.ko along with sun4-drm.ko
> and detach it from the mixer module, I could not tell which way is more
> appropriate here.

If that's easily doable, then yeah, that would be the preferred option
I guess. Jernej? Chen-Yu? Any opinion on this?

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/sun4i: fix build failure with CONFIG_DRM_SUN8I_MIXER=m

[Jernej Škrabec]

Dne ponedeljek, 09. julij 2018 ob 10:07:24 CEST je Maxime Ripard napisal(a):
> On Fri, Jul 06, 2018 at 02:45:53PM +0200, Arnd Bergmann wrote:
> > Having DRM_SUN4I built-in but DRM_SUN8I_MIXER as a loadable module results
> > in a link error, as we try to access a symbol from the sun8i_tcon_top.ko
> > module:
> > 
> > ERROR: "sun8i_tcon_top_of_table" [drivers/gpu/drm/sun4i/sun8i-drm-hdmi.ko]
> > undefined! ERROR: "sun8i_tcon_top_of_table"
> > [drivers/gpu/drm/sun4i/sun4i-drm.ko] undefined!
> > 
> > This solves the problem by making DRM_SUN8I_MIXER a 'bool' symbol,
> > building
> > the sun8i_tcon_top module the same way as the core sun4i-drm module
> > whenever DRM_SUN8I_MIXER is enabled, or not building it at all otherwise.
> > 
> > Alternatively, we could always build sun8i_tcon_top.ko along with
> > sun4-drm.ko and detach it from the mixer module, I could not tell which
> > way is more appropriate here.
> 
> If that's easily doable, then yeah, that would be the preferred option
> I guess. Jernej? Chen-Yu? Any opinion on this?

I guess that only means building sun8i_tcon_top.o with DRM_SUN4I instead of  
DRM_SUN8I_MIXER.

While this would be simple solution, sun8i_tcon_top would be dead weight if 
DRM_SUN8I_MIXER is disabled. But it is really small module, so I don't see any 
harm.

Additionally, with my follow up R40 HDMI series, there is even more calls from 
DRM_SUN4I enabled drivers to sun8i_tcon_top driver.

So I'm also for sun8i_tcon_top.o being build with DRM_SUN4I, because it is 
simpler, cleaner and it's symbols (including those introduced in R40 HDMI 
follow up series) are used mostly by DRM_SUN4I drivers (only exception being 
sun8i_dw_hdmi).

Best regards,
Jernej

[PATCH] drm/sun4i: fix build failure with CONFIG_DRM_SUN8I_MIXER=m

[Jernej Škrabec]

Dne ponedeljek, 09. julij 2018 ob 10:07:24 CEST je Maxime Ripard napisal(a):
> On Fri, Jul 06, 2018 at 02:45:53PM +0200, Arnd Bergmann wrote:
> > Having DRM_SUN4I built-in but DRM_SUN8I_MIXER as a loadable module results
> > in a link error, as we try to access a symbol from the sun8i_tcon_top.ko
> > module:
> > 
> > ERROR: "sun8i_tcon_top_of_table" [drivers/gpu/drm/sun4i/sun8i-drm-hdmi.ko]
> > undefined! ERROR: "sun8i_tcon_top_of_table"
> > [drivers/gpu/drm/sun4i/sun4i-drm.ko] undefined!
> > 
> > This solves the problem by making DRM_SUN8I_MIXER a 'bool' symbol,
> > building
> > the sun8i_tcon_top module the same way as the core sun4i-drm module
> > whenever DRM_SUN8I_MIXER is enabled, or not building it at all otherwise.
> > 
> > Alternatively, we could always build sun8i_tcon_top.ko along with
> > sun4-drm.ko and detach it from the mixer module, I could not tell which
> > way is more appropriate here.
> 
> If that's easily doable, then yeah, that would be the preferred option
> I guess. Jernej? Chen-Yu? Any opinion on this?

I guess that only means building sun8i_tcon_top.o with DRM_SUN4I instead of  
DRM_SUN8I_MIXER.

While this would be simple solution, sun8i_tcon_top would be dead weight if 
DRM_SUN8I_MIXER is disabled. But it is really small module, so I don't see any 
harm.

Additionally, with my follow up R40 HDMI series, there is even more calls from 
DRM_SUN4I enabled drivers to sun8i_tcon_top driver.

So I'm also for sun8i_tcon_top.o being build with DRM_SUN4I, because it is 
simpler, cleaner and it's symbols (including those introduced in R40 HDMI 
follow up series) are used mostly by DRM_SUN4I drivers (only exception being 
sun8i_dw_hdmi).

Best regards,
Jernej

Re: [PATCH] drm/sun4i: fix build failure with CONFIG_DRM_SUN8I_MIXER=m

[Chen-Yu Tsai]

On Mon, Jul 9, 2018 at 4:07 PM, Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> On Fri, Jul 06, 2018 at 02:45:53PM +0200, Arnd Bergmann wrote:
>> Having DRM_SUN4I built-in but DRM_SUN8I_MIXER as a loadable module results in
>> a link error, as we try to access a symbol from the sun8i_tcon_top.ko module:
>>
>> ERROR: "sun8i_tcon_top_of_table" [drivers/gpu/drm/sun4i/sun8i-drm-hdmi.ko] undefined!
>> ERROR: "sun8i_tcon_top_of_table" [drivers/gpu/drm/sun4i/sun4i-drm.ko] undefined!
>>
>> This solves the problem by making DRM_SUN8I_MIXER a 'bool' symbol, building
>> the sun8i_tcon_top module the same way as the core sun4i-drm module whenever
>> DRM_SUN8I_MIXER is enabled, or not building it at all otherwise.
>>
>> Alternatively, we could always build sun8i_tcon_top.ko along with sun4-drm.ko
>> and detach it from the mixer module, I could not tell which way is more
>> appropriate here.
>
> If that's easily doable, then yeah, that would be the preferred option
> I guess. Jernej? Chen-Yu? Any opinion on this?

Yeah, that definitely works for me. Having TCON TOP being part of the core
sun4i-drm makes more sense. The TCON code will likely call into it later on
when Jernej adds the encoder muxing code.

I wonder if we shouldn't just build the whole display engine code, excluding
downstream encoders (HDMI, DSI, TV) into one module. That would also fix the
issue that DRM_SUN4I_BACKEND has to be built-in if DRM_SUN4I is built-in.
That might be overkill though, given that one day we should be able to get
rid of the frontend check.

Here's a list of the size of various parts of this driver, built for ARMv7:

Common stuff:
   text    data     bss     dec     hex filename
   5021     344       0    5365    14f5 drivers/gpu/drm/sun4i/sun4i_drv.o
   1058       0       0    1058     422 drivers/gpu/drm/sun4i/sun4i_layer.o
    192       4       0     196      c4
drivers/gpu/drm/sun4i/sun4i_framebuffer.o
   1704       0       0    1704     6a8 drivers/gpu/drm/sun4i/sun4i_crtc.o
   1221       0       0    1221     4c5 drivers/gpu/drm/sun4i/sun4i_dotclock.o
   1192      28       0    1220     4c4 drivers/gpu/drm/sun4i/sun4i_lvds.o
   1583      92       0    1675     68b drivers/gpu/drm/sun4i/sun4i_rgb.o
  10088     248       0   10336    2860 drivers/gpu/drm/sun4i/sun4i_tcon.o
   1690     104       0    1794     702 drivers/gpu/drm/sun4i/sun6i_drc.o
  23749     820       0   24569    5ff9 (TOTALS)

DE 1.0:
   text    data     bss     dec     hex filename
   3596     248       0    3844     f04 drivers/gpu/drm/sun4i/sun4i_frontend.o
   8735     248       0    8983    2317 drivers/gpu/drm/sun4i/sun4i_backend.o
  12331     496       0   12827    321b (TOTALS)

DE 2.0:
   text    data     bss     dec     hex filename
   4040     248       0    4288    10c0 drivers/gpu/drm/sun4i/sun8i_mixer.o
   2620      28       0    2648     a58 drivers/gpu/drm/sun4i/sun8i_ui_layer.o
   1500       0       0    1500     5dc drivers/gpu/drm/sun4i/sun8i_ui_scaler.o
   2780      28       0    2808     af8 drivers/gpu/drm/sun4i/sun8i_vi_layer.o
  12612       0       0   12612    3144 drivers/gpu/drm/sun4i/sun8i_vi_scaler.o
    367       0       0     367     16f drivers/gpu/drm/sun4i/sun8i_csc.o
  23919     304       0   24223    5e9f (TOTALS)

TCON TOP:
   text    data     bss     dec     hex filename
   2623     104       0    2727     aa7 drivers/gpu/drm/sun4i/sun8i_tcon_top.o
   2623     104       0    2727     aa7 (TOTALS)

DE 1.0 HDMI:
   text    data     bss     dec     hex filename
    913       0       0     913     391
drivers/gpu/drm/sun4i/sun4i_hdmi_ddc_clk.o
   5543     104       0    5647    160f drivers/gpu/drm/sun4i/sun4i_hdmi_enc.o
   2583       0       0    2583     a17 drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.o
   1326       0       0    1326     52e
drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.o
  10365     104       0   10469    28e5 (TOTALS)

MIPI DSI:
   text    data     bss     dec     hex filename
   1990     144       0    2134     856 drivers/gpu/drm/sun4i/sun6i_mipi_dphy.o
   5921     132       0    6053    17a5 drivers/gpu/drm/sun4i/sun6i_mipi_dsi.o
   7911     276       0    8187    1ffb (TOTALS)

DW HDMI:
   text    data     bss     dec     hex filename
   1774     104       0    1878     756 drivers/gpu/drm/sun4i/sun8i_dw_hdmi.o
   1189       0       0    1189     4a5
drivers/gpu/drm/sun4i/sun8i_hdmi_phy_clk.o
   4877     144       0    5021    139d drivers/gpu/drm/sun4i/sun8i_hdmi_phy.o
   7840     248       0    8088    1f98 (TOTALS)

So it seems better to keep various parts as separate modules.

The "data" column makes me wonder if we've constify-ed all possible data
structures yet.

Regards
ChenYu

[PATCH] drm/sun4i: fix build failure with CONFIG_DRM_SUN8I_MIXER=m

[Chen-Yu Tsai]

On Mon, Jul 9, 2018 at 4:07 PM, Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> On Fri, Jul 06, 2018 at 02:45:53PM +0200, Arnd Bergmann wrote:
>> Having DRM_SUN4I built-in but DRM_SUN8I_MIXER as a loadable module results in
>> a link error, as we try to access a symbol from the sun8i_tcon_top.ko module:
>>
>> ERROR: "sun8i_tcon_top_of_table" [drivers/gpu/drm/sun4i/sun8i-drm-hdmi.ko] undefined!
>> ERROR: "sun8i_tcon_top_of_table" [drivers/gpu/drm/sun4i/sun4i-drm.ko] undefined!
>>
>> This solves the problem by making DRM_SUN8I_MIXER a 'bool' symbol, building
>> the sun8i_tcon_top module the same way as the core sun4i-drm module whenever
>> DRM_SUN8I_MIXER is enabled, or not building it at all otherwise.
>>
>> Alternatively, we could always build sun8i_tcon_top.ko along with sun4-drm.ko
>> and detach it from the mixer module, I could not tell which way is more
>> appropriate here.
>
> If that's easily doable, then yeah, that would be the preferred option
> I guess. Jernej? Chen-Yu? Any opinion on this?

Yeah, that definitely works for me. Having TCON TOP being part of the core
sun4i-drm makes more sense. The TCON code will likely call into it later on
when Jernej adds the encoder muxing code.

I wonder if we shouldn't just build the whole display engine code, excluding
downstream encoders (HDMI, DSI, TV) into one module. That would also fix the
issue that DRM_SUN4I_BACKEND has to be built-in if DRM_SUN4I is built-in.
That might be overkill though, given that one day we should be able to get
rid of the frontend check.

Here's a list of the size of various parts of this driver, built for ARMv7:

Common stuff:
   text    data     bss     dec     hex filename
   5021     344       0    5365    14f5 drivers/gpu/drm/sun4i/sun4i_drv.o
   1058       0       0    1058     422 drivers/gpu/drm/sun4i/sun4i_layer.o
    192       4       0     196      c4
drivers/gpu/drm/sun4i/sun4i_framebuffer.o
   1704       0       0    1704     6a8 drivers/gpu/drm/sun4i/sun4i_crtc.o
   1221       0       0    1221     4c5 drivers/gpu/drm/sun4i/sun4i_dotclock.o
   1192      28       0    1220     4c4 drivers/gpu/drm/sun4i/sun4i_lvds.o
   1583      92       0    1675     68b drivers/gpu/drm/sun4i/sun4i_rgb.o
  10088     248       0   10336    2860 drivers/gpu/drm/sun4i/sun4i_tcon.o
   1690     104       0    1794     702 drivers/gpu/drm/sun4i/sun6i_drc.o
  23749     820       0   24569    5ff9 (TOTALS)

DE 1.0:
   text    data     bss     dec     hex filename
   3596     248       0    3844     f04 drivers/gpu/drm/sun4i/sun4i_frontend.o
   8735     248       0    8983    2317 drivers/gpu/drm/sun4i/sun4i_backend.o
  12331     496       0   12827    321b (TOTALS)

DE 2.0:
   text    data     bss     dec     hex filename
   4040     248       0    4288    10c0 drivers/gpu/drm/sun4i/sun8i_mixer.o
   2620      28       0    2648     a58 drivers/gpu/drm/sun4i/sun8i_ui_layer.o
   1500       0       0    1500     5dc drivers/gpu/drm/sun4i/sun8i_ui_scaler.o
   2780      28       0    2808     af8 drivers/gpu/drm/sun4i/sun8i_vi_layer.o
  12612       0       0   12612    3144 drivers/gpu/drm/sun4i/sun8i_vi_scaler.o
    367       0       0     367     16f drivers/gpu/drm/sun4i/sun8i_csc.o
  23919     304       0   24223    5e9f (TOTALS)

TCON TOP:
   text    data     bss     dec     hex filename
   2623     104       0    2727     aa7 drivers/gpu/drm/sun4i/sun8i_tcon_top.o
   2623     104       0    2727     aa7 (TOTALS)

DE 1.0 HDMI:
   text    data     bss     dec     hex filename
    913       0       0     913     391
drivers/gpu/drm/sun4i/sun4i_hdmi_ddc_clk.o
   5543     104       0    5647    160f drivers/gpu/drm/sun4i/sun4i_hdmi_enc.o
   2583       0       0    2583     a17 drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.o
   1326       0       0    1326     52e
drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.o
  10365     104       0   10469    28e5 (TOTALS)

MIPI DSI:
   text    data     bss     dec     hex filename
   1990     144       0    2134     856 drivers/gpu/drm/sun4i/sun6i_mipi_dphy.o
   5921     132       0    6053    17a5 drivers/gpu/drm/sun4i/sun6i_mipi_dsi.o
   7911     276       0    8187    1ffb (TOTALS)

DW HDMI:
   text    data     bss     dec     hex filename
   1774     104       0    1878     756 drivers/gpu/drm/sun4i/sun8i_dw_hdmi.o
   1189       0       0    1189     4a5
drivers/gpu/drm/sun4i/sun8i_hdmi_phy_clk.o
   4877     144       0    5021    139d drivers/gpu/drm/sun4i/sun8i_hdmi_phy.o
   7840     248       0    8088    1f98 (TOTALS)

So it seems better to keep various parts as separate modules.

The "data" column makes me wonder if we've constify-ed all possible data
structures yet.

Regards
ChenYu

Re: [PATCH] drm/sun4i: fix build failure with CONFIG_DRM_SUN8I_MIXER=m

[Chen-Yu Tsai]

On Mon, Jul 9, 2018 at 4:07 PM, Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> On Fri, Jul 06, 2018 at 02:45:53PM +0200, Arnd Bergmann wrote:
>> Having DRM_SUN4I built-in but DRM_SUN8I_MIXER as a loadable module results in
>> a link error, as we try to access a symbol from the sun8i_tcon_top.ko module:
>>
>> ERROR: "sun8i_tcon_top_of_table" [drivers/gpu/drm/sun4i/sun8i-drm-hdmi.ko] undefined!
>> ERROR: "sun8i_tcon_top_of_table" [drivers/gpu/drm/sun4i/sun4i-drm.ko] undefined!
>>
>> This solves the problem by making DRM_SUN8I_MIXER a 'bool' symbol, building
>> the sun8i_tcon_top module the same way as the core sun4i-drm module whenever
>> DRM_SUN8I_MIXER is enabled, or not building it at all otherwise.
>>
>> Alternatively, we could always build sun8i_tcon_top.ko along with sun4-drm.ko
>> and detach it from the mixer module, I could not tell which way is more
>> appropriate here.
>
> If that's easily doable, then yeah, that would be the preferred option
> I guess. Jernej? Chen-Yu? Any opinion on this?

Yeah, that definitely works for me. Having TCON TOP being part of the core
sun4i-drm makes more sense. The TCON code will likely call into it later on
when Jernej adds the encoder muxing code.

I wonder if we shouldn't just build the whole display engine code, excluding
downstream encoders (HDMI, DSI, TV) into one module. That would also fix the
issue that DRM_SUN4I_BACKEND has to be built-in if DRM_SUN4I is built-in.
That might be overkill though, given that one day we should be able to get
rid of the frontend check.

Here's a list of the size of various parts of this driver, built for ARMv7:

Common stuff:
   text    data     bss     dec     hex filename
   5021     344       0    5365    14f5 drivers/gpu/drm/sun4i/sun4i_drv.o
   1058       0       0    1058     422 drivers/gpu/drm/sun4i/sun4i_layer.o
    192       4       0     196      c4
drivers/gpu/drm/sun4i/sun4i_framebuffer.o
   1704       0       0    1704     6a8 drivers/gpu/drm/sun4i/sun4i_crtc.o
   1221       0       0    1221     4c5 drivers/gpu/drm/sun4i/sun4i_dotclock.o
   1192      28       0    1220     4c4 drivers/gpu/drm/sun4i/sun4i_lvds.o
   1583      92       0    1675     68b drivers/gpu/drm/sun4i/sun4i_rgb.o
  10088     248       0   10336    2860 drivers/gpu/drm/sun4i/sun4i_tcon.o
   1690     104       0    1794     702 drivers/gpu/drm/sun4i/sun6i_drc.o
  23749     820       0   24569    5ff9 (TOTALS)

DE 1.0:
   text    data     bss     dec     hex filename
   3596     248       0    3844     f04 drivers/gpu/drm/sun4i/sun4i_frontend.o
   8735     248       0    8983    2317 drivers/gpu/drm/sun4i/sun4i_backend.o
  12331     496       0   12827    321b (TOTALS)

DE 2.0:
   text    data     bss     dec     hex filename
   4040     248       0    4288    10c0 drivers/gpu/drm/sun4i/sun8i_mixer.o
   2620      28       0    2648     a58 drivers/gpu/drm/sun4i/sun8i_ui_layer.o
   1500       0       0    1500     5dc drivers/gpu/drm/sun4i/sun8i_ui_scaler.o
   2780      28       0    2808     af8 drivers/gpu/drm/sun4i/sun8i_vi_layer.o
  12612       0       0   12612    3144 drivers/gpu/drm/sun4i/sun8i_vi_scaler.o
    367       0       0     367     16f drivers/gpu/drm/sun4i/sun8i_csc.o
  23919     304       0   24223    5e9f (TOTALS)

TCON TOP:
   text    data     bss     dec     hex filename
   2623     104       0    2727     aa7 drivers/gpu/drm/sun4i/sun8i_tcon_top.o
   2623     104       0    2727     aa7 (TOTALS)

DE 1.0 HDMI:
   text    data     bss     dec     hex filename
    913       0       0     913     391
drivers/gpu/drm/sun4i/sun4i_hdmi_ddc_clk.o
   5543     104       0    5647    160f drivers/gpu/drm/sun4i/sun4i_hdmi_enc.o
   2583       0       0    2583     a17 drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.o
   1326       0       0    1326     52e
drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.o
  10365     104       0   10469    28e5 (TOTALS)

MIPI DSI:
   text    data     bss     dec     hex filename
   1990     144       0    2134     856 drivers/gpu/drm/sun4i/sun6i_mipi_dphy.o
   5921     132       0    6053    17a5 drivers/gpu/drm/sun4i/sun6i_mipi_dsi.o
   7911     276       0    8187    1ffb (TOTALS)

DW HDMI:
   text    data     bss     dec     hex filename
   1774     104       0    1878     756 drivers/gpu/drm/sun4i/sun8i_dw_hdmi.o
   1189       0       0    1189     4a5
drivers/gpu/drm/sun4i/sun8i_hdmi_phy_clk.o
   4877     144       0    5021    139d drivers/gpu/drm/sun4i/sun8i_hdmi_phy.o
   7840     248       0    8088    1f98 (TOTALS)

So it seems better to keep various parts as separate modules.

The "data" column makes me wonder if we've constify-ed all possible data
structures yet.

Regards
ChenYu
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/sun4i: fix build failure with CONFIG_DRM_SUN8I_MIXER=m

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2021 bytes --]

On Mon, Jul 09, 2018 at 04:58:48PM +0800, Chen-Yu Tsai wrote:
> On Mon, Jul 9, 2018 at 4:07 PM, Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > On Fri, Jul 06, 2018 at 02:45:53PM +0200, Arnd Bergmann wrote:
> >> Having DRM_SUN4I built-in but DRM_SUN8I_MIXER as a loadable module results in
> >> a link error, as we try to access a symbol from the sun8i_tcon_top.ko module:
> >>
> >> ERROR: "sun8i_tcon_top_of_table" [drivers/gpu/drm/sun4i/sun8i-drm-hdmi.ko] undefined!
> >> ERROR: "sun8i_tcon_top_of_table" [drivers/gpu/drm/sun4i/sun4i-drm.ko] undefined!
> >>
> >> This solves the problem by making DRM_SUN8I_MIXER a 'bool' symbol, building
> >> the sun8i_tcon_top module the same way as the core sun4i-drm module whenever
> >> DRM_SUN8I_MIXER is enabled, or not building it at all otherwise.
> >>
> >> Alternatively, we could always build sun8i_tcon_top.ko along with sun4-drm.ko
> >> and detach it from the mixer module, I could not tell which way is more
> >> appropriate here.
> >
> > If that's easily doable, then yeah, that would be the preferred option
> > I guess. Jernej? Chen-Yu? Any opinion on this?
> 
> Yeah, that definitely works for me. Having TCON TOP being part of the core
> sun4i-drm makes more sense. The TCON code will likely call into it later on
> when Jernej adds the encoder muxing code.
> 
> I wonder if we shouldn't just build the whole display engine code, excluding
> downstream encoders (HDMI, DSI, TV) into one module. That would also fix the
> issue that DRM_SUN4I_BACKEND has to be built-in if DRM_SUN4I is built-in.
> That might be overkill though, given that one day we should be able to get
> rid of the frontend check.

We can't really do that (or at least without rewriting a significant
part of the driver), since we can have only one
module_init/module_exit function per module, and we have one
per-hardware block.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH] drm/sun4i: fix build failure with CONFIG_DRM_SUN8I_MIXER=m

[Maxime Ripard]

On Mon, Jul 09, 2018 at 04:58:48PM +0800, Chen-Yu Tsai wrote:
> On Mon, Jul 9, 2018 at 4:07 PM, Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > On Fri, Jul 06, 2018 at 02:45:53PM +0200, Arnd Bergmann wrote:
> >> Having DRM_SUN4I built-in but DRM_SUN8I_MIXER as a loadable module results in
> >> a link error, as we try to access a symbol from the sun8i_tcon_top.ko module:
> >>
> >> ERROR: "sun8i_tcon_top_of_table" [drivers/gpu/drm/sun4i/sun8i-drm-hdmi.ko] undefined!
> >> ERROR: "sun8i_tcon_top_of_table" [drivers/gpu/drm/sun4i/sun4i-drm.ko] undefined!
> >>
> >> This solves the problem by making DRM_SUN8I_MIXER a 'bool' symbol, building
> >> the sun8i_tcon_top module the same way as the core sun4i-drm module whenever
> >> DRM_SUN8I_MIXER is enabled, or not building it at all otherwise.
> >>
> >> Alternatively, we could always build sun8i_tcon_top.ko along with sun4-drm.ko
> >> and detach it from the mixer module, I could not tell which way is more
> >> appropriate here.
> >
> > If that's easily doable, then yeah, that would be the preferred option
> > I guess. Jernej? Chen-Yu? Any opinion on this?
> 
> Yeah, that definitely works for me. Having TCON TOP being part of the core
> sun4i-drm makes more sense. The TCON code will likely call into it later on
> when Jernej adds the encoder muxing code.
> 
> I wonder if we shouldn't just build the whole display engine code, excluding
> downstream encoders (HDMI, DSI, TV) into one module. That would also fix the
> issue that DRM_SUN4I_BACKEND has to be built-in if DRM_SUN4I is built-in.
> That might be overkill though, given that one day we should be able to get
> rid of the frontend check.

We can't really do that (or at least without rewriting a significant
part of the driver), since we can have only one
module_init/module_exit function per module, and we have one
per-hardware block.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180709/c83521e5/attachment.sig>

Re: [PATCH] drm/sun4i: fix build failure with CONFIG_DRM_SUN8I_MIXER=m

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 2021 bytes --]

On Mon, Jul 09, 2018 at 04:58:48PM +0800, Chen-Yu Tsai wrote:
> On Mon, Jul 9, 2018 at 4:07 PM, Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > On Fri, Jul 06, 2018 at 02:45:53PM +0200, Arnd Bergmann wrote:
> >> Having DRM_SUN4I built-in but DRM_SUN8I_MIXER as a loadable module results in
> >> a link error, as we try to access a symbol from the sun8i_tcon_top.ko module:
> >>
> >> ERROR: "sun8i_tcon_top_of_table" [drivers/gpu/drm/sun4i/sun8i-drm-hdmi.ko] undefined!
> >> ERROR: "sun8i_tcon_top_of_table" [drivers/gpu/drm/sun4i/sun4i-drm.ko] undefined!
> >>
> >> This solves the problem by making DRM_SUN8I_MIXER a 'bool' symbol, building
> >> the sun8i_tcon_top module the same way as the core sun4i-drm module whenever
> >> DRM_SUN8I_MIXER is enabled, or not building it at all otherwise.
> >>
> >> Alternatively, we could always build sun8i_tcon_top.ko along with sun4-drm.ko
> >> and detach it from the mixer module, I could not tell which way is more
> >> appropriate here.
> >
> > If that's easily doable, then yeah, that would be the preferred option
> > I guess. Jernej? Chen-Yu? Any opinion on this?
> 
> Yeah, that definitely works for me. Having TCON TOP being part of the core
> sun4i-drm makes more sense. The TCON code will likely call into it later on
> when Jernej adds the encoder muxing code.
> 
> I wonder if we shouldn't just build the whole display engine code, excluding
> downstream encoders (HDMI, DSI, TV) into one module. That would also fix the
> issue that DRM_SUN4I_BACKEND has to be built-in if DRM_SUN4I is built-in.
> That might be overkill though, given that one day we should be able to get
> rid of the frontend check.

We can't really do that (or at least without rewriting a significant
part of the driver), since we can have only one
module_init/module_exit function per module, and we have one
per-hardware block.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/sun4i: fix build failure with CONFIG_DRM_SUN8I_MIXER=m

[Chen-Yu Tsai]

On Mon, Jul 9, 2018 at 5:15 PM, Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> On Mon, Jul 09, 2018 at 04:58:48PM +0800, Chen-Yu Tsai wrote:
>> On Mon, Jul 9, 2018 at 4:07 PM, Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>> > On Fri, Jul 06, 2018 at 02:45:53PM +0200, Arnd Bergmann wrote:
>> >> Having DRM_SUN4I built-in but DRM_SUN8I_MIXER as a loadable module results in
>> >> a link error, as we try to access a symbol from the sun8i_tcon_top.ko module:
>> >>
>> >> ERROR: "sun8i_tcon_top_of_table" [drivers/gpu/drm/sun4i/sun8i-drm-hdmi.ko] undefined!
>> >> ERROR: "sun8i_tcon_top_of_table" [drivers/gpu/drm/sun4i/sun4i-drm.ko] undefined!
>> >>
>> >> This solves the problem by making DRM_SUN8I_MIXER a 'bool' symbol, building
>> >> the sun8i_tcon_top module the same way as the core sun4i-drm module whenever
>> >> DRM_SUN8I_MIXER is enabled, or not building it at all otherwise.
>> >>
>> >> Alternatively, we could always build sun8i_tcon_top.ko along with sun4-drm.ko
>> >> and detach it from the mixer module, I could not tell which way is more
>> >> appropriate here.
>> >
>> > If that's easily doable, then yeah, that would be the preferred option
>> > I guess. Jernej? Chen-Yu? Any opinion on this?
>>
>> Yeah, that definitely works for me. Having TCON TOP being part of the core
>> sun4i-drm makes more sense. The TCON code will likely call into it later on
>> when Jernej adds the encoder muxing code.
>>
>> I wonder if we shouldn't just build the whole display engine code, excluding
>> downstream encoders (HDMI, DSI, TV) into one module. That would also fix the
>> issue that DRM_SUN4I_BACKEND has to be built-in if DRM_SUN4I is built-in.
>> That might be overkill though, given that one day we should be able to get
>> rid of the frontend check.
>
> We can't really do that (or at least without rewriting a significant
> part of the driver), since we can have only one
> module_init/module_exit function per module, and we have one
> per-hardware block.

Right. That would require some serious plumbing to register all the drivers.

ChenYu

[PATCH] drm/sun4i: fix build failure with CONFIG_DRM_SUN8I_MIXER=m

[Chen-Yu Tsai]

On Mon, Jul 9, 2018 at 5:15 PM, Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> On Mon, Jul 09, 2018 at 04:58:48PM +0800, Chen-Yu Tsai wrote:
>> On Mon, Jul 9, 2018 at 4:07 PM, Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>> > On Fri, Jul 06, 2018 at 02:45:53PM +0200, Arnd Bergmann wrote:
>> >> Having DRM_SUN4I built-in but DRM_SUN8I_MIXER as a loadable module results in
>> >> a link error, as we try to access a symbol from the sun8i_tcon_top.ko module:
>> >>
>> >> ERROR: "sun8i_tcon_top_of_table" [drivers/gpu/drm/sun4i/sun8i-drm-hdmi.ko] undefined!
>> >> ERROR: "sun8i_tcon_top_of_table" [drivers/gpu/drm/sun4i/sun4i-drm.ko] undefined!
>> >>
>> >> This solves the problem by making DRM_SUN8I_MIXER a 'bool' symbol, building
>> >> the sun8i_tcon_top module the same way as the core sun4i-drm module whenever
>> >> DRM_SUN8I_MIXER is enabled, or not building it at all otherwise.
>> >>
>> >> Alternatively, we could always build sun8i_tcon_top.ko along with sun4-drm.ko
>> >> and detach it from the mixer module, I could not tell which way is more
>> >> appropriate here.
>> >
>> > If that's easily doable, then yeah, that would be the preferred option
>> > I guess. Jernej? Chen-Yu? Any opinion on this?
>>
>> Yeah, that definitely works for me. Having TCON TOP being part of the core
>> sun4i-drm makes more sense. The TCON code will likely call into it later on
>> when Jernej adds the encoder muxing code.
>>
>> I wonder if we shouldn't just build the whole display engine code, excluding
>> downstream encoders (HDMI, DSI, TV) into one module. That would also fix the
>> issue that DRM_SUN4I_BACKEND has to be built-in if DRM_SUN4I is built-in.
>> That might be overkill though, given that one day we should be able to get
>> rid of the frontend check.
>
> We can't really do that (or at least without rewriting a significant
> part of the driver), since we can have only one
> module_init/module_exit function per module, and we have one
> per-hardware block.

Right. That would require some serious plumbing to register all the drivers.

ChenYu

Re: [PATCH] drm/sun4i: fix build failure with CONFIG_DRM_SUN8I_MIXER=m

[Chen-Yu Tsai]

On Mon, Jul 9, 2018 at 5:15 PM, Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> On Mon, Jul 09, 2018 at 04:58:48PM +0800, Chen-Yu Tsai wrote:
>> On Mon, Jul 9, 2018 at 4:07 PM, Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>> > On Fri, Jul 06, 2018 at 02:45:53PM +0200, Arnd Bergmann wrote:
>> >> Having DRM_SUN4I built-in but DRM_SUN8I_MIXER as a loadable module results in
>> >> a link error, as we try to access a symbol from the sun8i_tcon_top.ko module:
>> >>
>> >> ERROR: "sun8i_tcon_top_of_table" [drivers/gpu/drm/sun4i/sun8i-drm-hdmi.ko] undefined!
>> >> ERROR: "sun8i_tcon_top_of_table" [drivers/gpu/drm/sun4i/sun4i-drm.ko] undefined!
>> >>
>> >> This solves the problem by making DRM_SUN8I_MIXER a 'bool' symbol, building
>> >> the sun8i_tcon_top module the same way as the core sun4i-drm module whenever
>> >> DRM_SUN8I_MIXER is enabled, or not building it at all otherwise.
>> >>
>> >> Alternatively, we could always build sun8i_tcon_top.ko along with sun4-drm.ko
>> >> and detach it from the mixer module, I could not tell which way is more
>> >> appropriate here.
>> >
>> > If that's easily doable, then yeah, that would be the preferred option
>> > I guess. Jernej? Chen-Yu? Any opinion on this?
>>
>> Yeah, that definitely works for me. Having TCON TOP being part of the core
>> sun4i-drm makes more sense. The TCON code will likely call into it later on
>> when Jernej adds the encoder muxing code.
>>
>> I wonder if we shouldn't just build the whole display engine code, excluding
>> downstream encoders (HDMI, DSI, TV) into one module. That would also fix the
>> issue that DRM_SUN4I_BACKEND has to be built-in if DRM_SUN4I is built-in.
>> That might be overkill though, given that one day we should be able to get
>> rid of the frontend check.
>
> We can't really do that (or at least without rewriting a significant
> part of the driver), since we can have only one
> module_init/module_exit function per module, and we have one
> per-hardware block.

Right. That would require some serious plumbing to register all the drivers.

ChenYu
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/sun4i: fix build failure with CONFIG_DRM_SUN8I_MIXER=m

[Arnd Bergmann]

On Thu, Sep 13, 2018 at 10:02 AM Maxime Ripard
<maxime.ripard@bootlin.com> wrote:
> On Wed, Sep 12, 2018 at 05:53:39PM +0200, Daniel Vetter wrote:
> > On Wed, Sep 12, 2018 at 5:47 PM, Maxime Ripard
> > <maxime.ripard@bootlin.com> wrote:
> > > On Wed, Sep 12, 2018 at 04:25:36PM +0200, Daniel Vetter wrote:
> > Yup, if you want to make drm_edid.c optional, you need LTO. Because I
> > think we've already gone way overboard with making stuff optional in
> > the drm core, there's lots of silly little Kconfigs with imo
> > questionable value. Also, 50kb ... what does that look like
> > compressed? Should compress exceedingly well.
> >
> > But that's not what I asked about really, I asked about all the
> > Kconfigs in su4i. Are those worth it? Especially compared to fixing
> > this for real, using something like LTO (plus making a few things
> > hard-coded, per machine configuration, so that gcc can figure it all
> > out).
>
> You're asking whether a 5 minutes effort is worth it compared to a 5
> weeks one (at best) to port the LTO patches, making it sure it works
> ok on ARM, and then debugging whether some entry point has been
> removed or not.
>
> Plus, given that it's a driver that could or couldn't be loaded
> depending on the device tree, you would have to keep that driver in
> even with LTO, even though you know you have zero chance to execute
> that code at runtime.

I have spent a few weeks on getting ARM kernels to build with
LTO, based on earlier work from Nico and others. This requires
a couple of patches for the common cases, but gets increasingly
complex when you add in some other options (thumb2 kernel,
XIP_KERNEL, MTD_XIP, OABI, ftrace, gcov, multi-cpu, ...)
that each add their own conflicting requirements. I gave up
in the end, before even trying to run any of those kernels.

I think for x86-64 and arm64, we have a realistic chance of making
it work in general, on arm32 the best case I can think of would be
to support some of the common configurations.

      Arnd
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/sun4i: fix build failure with CONFIG_DRM_SUN8I_MIXER=m

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 4338 bytes --]

On Wed, Sep 12, 2018 at 05:53:39PM +0200, Daniel Vetter wrote:
> On Wed, Sep 12, 2018 at 5:47 PM, Maxime Ripard
> <maxime.ripard@bootlin.com> wrote:
> > On Wed, Sep 12, 2018 at 04:25:36PM +0200, Daniel Vetter wrote:
> >> On Wed, Sep 12, 2018 at 11:53 AM, Maxime Ripard
> >> <maxime.ripard@bootlin.com> wrote:
> >> > On Tue, Sep 11, 2018 at 10:17:02PM +0200, Daniel Vetter wrote:
> >> >> On Tue, Sep 11, 2018 at 01:33:25PM +0200, Maxime Ripard wrote:
> >> >> > Having DRM_SUN4I built-in but DRM_SUN8I_MIXER as a loadable module results in
> >> >> > a link error, as we try to access a symbol from the sun8i_tcon_top.ko module:
> >> >> >
> >> >> > ERROR: "sun8i_tcon_top_de_config" [drivers/gpu/drm/sun4i/sun4i-tcon.ko] undefined!
> >> >> > ERROR: "sun8i_tcon_top_set_hdmi_src" [drivers/gpu/drm/sun4i/sun4i-tcon.ko] undefined!
> >> >> > ERROR: "sun8i_tcon_top_of_table" [drivers/gpu/drm/sun4i/sun4i-tcon.ko] undefined!
> >> >> >
> >> >> > This solves the problem by adding a silent symbol for the tcon_top module,
> >> >> > building it as a separate module in exactly the cases that we need it,
> >> >> > but in a way that it is reachable by the other modules.
> >> >> >
> >> >> > Fixes: cf77d79b4e29 ("drm/sun4i: tcon: Add another way for matching mixers with tcon")
> >> >> > Fixes: 0305189afb32 ("drm/sun4i: tcon: Add support for R40 TCON")
> >> >> > Tested-by: Jon Hunter <jonathanh@nvidia.com>
> >> >> > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> >> >>
> >> >> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >> >>
> >> >> But I can't help myself and drop the usual questions when I see a small
> >> >> soc driver with more Kconfigs than anything else ... is all this pain
> >> >> worth it? I know that maybe the desktop approach of stuffing half a
> >> >> million lines of driver code into one .ko might be a bit too much for
> >> >> socs, but this seems overkill.
> >> >>
> >> >> I'm also pretty sure it's not justified by any real data, compared to
> >> >> overall code size of a drm stack, that shows it's a substantial enough
> >> >> saving that it's worth it.
> >> >
> >> > I'm currently running on a project where the boot time to a qt
> >> > application from power off should be less than a second. You want to
> >> > remove anything you can spare in some situations. And yeah, DRM is the
> >> > biggest thing in the way to do that.
> >>
> >> Oh I know all about the 1s people. But is binary size really that
> >> important figure? I know it's a bit more to load&decompress, but it
> >> shouldn't have any impact on anything running at runtime.
> >
> > It really depends on the combination of the CPU speed, the storage
> > speed, and the compression algorithm. To give you a figure, a quite
> > good storage device in our case has a bandwith of 10MB/s. If you add a
> > MB, you lose a tenth of your budget, decompression excluded.
> >
> > The sole edid_cea_modes, drm_dmt_modes and edid_est_modes, combined,
> > already take around 50kB. That's around .5% of our time budget just
> > dedicated to loading structures we will never need, without the option
> > to compile them out.
> 
> Yup, if you want to make drm_edid.c optional, you need LTO. Because I
> think we've already gone way overboard with making stuff optional in
> the drm core, there's lots of silly little Kconfigs with imo
> questionable value. Also, 50kb ... what does that look like
> compressed? Should compress exceedingly well.
> 
> But that's not what I asked about really, I asked about all the
> Kconfigs in su4i. Are those worth it? Especially compared to fixing
> this for real, using something like LTO (plus making a few things
> hard-coded, per machine configuration, so that gcc can figure it all
> out).

You're asking whether a 5 minutes effort is worth it compared to a 5
weeks one (at best) to port the LTO patches, making it sure it works
ok on ARM, and then debugging whether some entry point has been
removed or not.

Plus, given that it's a driver that could or couldn't be loaded
depending on the device tree, you would have to keep that driver in
even with LTO, even though you know you have zero chance to execute
that code at runtime.

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/sun4i: fix build failure with CONFIG_DRM_SUN8I_MIXER=m

[Daniel Vetter]

On Wed, Sep 12, 2018 at 5:47 PM, Maxime Ripard
<maxime.ripard@bootlin.com> wrote:
> On Wed, Sep 12, 2018 at 04:25:36PM +0200, Daniel Vetter wrote:
>> On Wed, Sep 12, 2018 at 11:53 AM, Maxime Ripard
>> <maxime.ripard@bootlin.com> wrote:
>> > On Tue, Sep 11, 2018 at 10:17:02PM +0200, Daniel Vetter wrote:
>> >> On Tue, Sep 11, 2018 at 01:33:25PM +0200, Maxime Ripard wrote:
>> >> > Having DRM_SUN4I built-in but DRM_SUN8I_MIXER as a loadable module results in
>> >> > a link error, as we try to access a symbol from the sun8i_tcon_top.ko module:
>> >> >
>> >> > ERROR: "sun8i_tcon_top_de_config" [drivers/gpu/drm/sun4i/sun4i-tcon.ko] undefined!
>> >> > ERROR: "sun8i_tcon_top_set_hdmi_src" [drivers/gpu/drm/sun4i/sun4i-tcon.ko] undefined!
>> >> > ERROR: "sun8i_tcon_top_of_table" [drivers/gpu/drm/sun4i/sun4i-tcon.ko] undefined!
>> >> >
>> >> > This solves the problem by adding a silent symbol for the tcon_top module,
>> >> > building it as a separate module in exactly the cases that we need it,
>> >> > but in a way that it is reachable by the other modules.
>> >> >
>> >> > Fixes: cf77d79b4e29 ("drm/sun4i: tcon: Add another way for matching mixers with tcon")
>> >> > Fixes: 0305189afb32 ("drm/sun4i: tcon: Add support for R40 TCON")
>> >> > Tested-by: Jon Hunter <jonathanh@nvidia.com>
>> >> > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
>> >>
>> >> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> >>
>> >> But I can't help myself and drop the usual questions when I see a small
>> >> soc driver with more Kconfigs than anything else ... is all this pain
>> >> worth it? I know that maybe the desktop approach of stuffing half a
>> >> million lines of driver code into one .ko might be a bit too much for
>> >> socs, but this seems overkill.
>> >>
>> >> I'm also pretty sure it's not justified by any real data, compared to
>> >> overall code size of a drm stack, that shows it's a substantial enough
>> >> saving that it's worth it.
>> >
>> > I'm currently running on a project where the boot time to a qt
>> > application from power off should be less than a second. You want to
>> > remove anything you can spare in some situations. And yeah, DRM is the
>> > biggest thing in the way to do that.
>>
>> Oh I know all about the 1s people. But is binary size really that
>> important figure? I know it's a bit more to load&decompress, but it
>> shouldn't have any impact on anything running at runtime.
>
> It really depends on the combination of the CPU speed, the storage
> speed, and the compression algorithm. To give you a figure, a quite
> good storage device in our case has a bandwith of 10MB/s. If you add a
> MB, you lose a tenth of your budget, decompression excluded.
>
> The sole edid_cea_modes, drm_dmt_modes and edid_est_modes, combined,
> already take around 50kB. That's around .5% of our time budget just
> dedicated to loading structures we will never need, without the option
> to compile them out.

Yup, if you want to make drm_edid.c optional, you need LTO. Because I
think we've already gone way overboard with making stuff optional in
the drm core, there's lots of silly little Kconfigs with imo
questionable value. Also, 50kb ... what does that look like
compressed? Should compress exceedingly well.

But that's not what I asked about really, I asked about all the
Kconfigs in su4i. Are those worth it? Especially compared to fixing
this for real, using something like LTO (plus making a few things
hard-coded, per machine configuration, so that gcc can figure it all
out).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/sun4i: fix build failure with CONFIG_DRM_SUN8I_MIXER=m

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 2956 bytes --]

On Wed, Sep 12, 2018 at 04:25:36PM +0200, Daniel Vetter wrote:
> On Wed, Sep 12, 2018 at 11:53 AM, Maxime Ripard
> <maxime.ripard@bootlin.com> wrote:
> > On Tue, Sep 11, 2018 at 10:17:02PM +0200, Daniel Vetter wrote:
> >> On Tue, Sep 11, 2018 at 01:33:25PM +0200, Maxime Ripard wrote:
> >> > Having DRM_SUN4I built-in but DRM_SUN8I_MIXER as a loadable module results in
> >> > a link error, as we try to access a symbol from the sun8i_tcon_top.ko module:
> >> >
> >> > ERROR: "sun8i_tcon_top_de_config" [drivers/gpu/drm/sun4i/sun4i-tcon.ko] undefined!
> >> > ERROR: "sun8i_tcon_top_set_hdmi_src" [drivers/gpu/drm/sun4i/sun4i-tcon.ko] undefined!
> >> > ERROR: "sun8i_tcon_top_of_table" [drivers/gpu/drm/sun4i/sun4i-tcon.ko] undefined!
> >> >
> >> > This solves the problem by adding a silent symbol for the tcon_top module,
> >> > building it as a separate module in exactly the cases that we need it,
> >> > but in a way that it is reachable by the other modules.
> >> >
> >> > Fixes: cf77d79b4e29 ("drm/sun4i: tcon: Add another way for matching mixers with tcon")
> >> > Fixes: 0305189afb32 ("drm/sun4i: tcon: Add support for R40 TCON")
> >> > Tested-by: Jon Hunter <jonathanh@nvidia.com>
> >> > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> >>
> >> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>
> >> But I can't help myself and drop the usual questions when I see a small
> >> soc driver with more Kconfigs than anything else ... is all this pain
> >> worth it? I know that maybe the desktop approach of stuffing half a
> >> million lines of driver code into one .ko might be a bit too much for
> >> socs, but this seems overkill.
> >>
> >> I'm also pretty sure it's not justified by any real data, compared to
> >> overall code size of a drm stack, that shows it's a substantial enough
> >> saving that it's worth it.
> >
> > I'm currently running on a project where the boot time to a qt
> > application from power off should be less than a second. You want to
> > remove anything you can spare in some situations. And yeah, DRM is the
> > biggest thing in the way to do that.
> 
> Oh I know all about the 1s people. But is binary size really that
> important figure? I know it's a bit more to load&decompress, but it
> shouldn't have any impact on anything running at runtime.

It really depends on the combination of the CPU speed, the storage
speed, and the compression algorithm. To give you a figure, a quite
good storage device in our case has a bandwith of 10MB/s. If you add a
MB, you lose a tenth of your budget, decompression excluded.

The sole edid_cea_modes, drm_dmt_modes and edid_est_modes, combined,
already take around 50kB. That's around .5% of our time budget just
dedicated to loading structures we will never need, without the option
to compile them out.

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/sun4i: fix build failure with CONFIG_DRM_SUN8I_MIXER=m

[Daniel Vetter]

On Wed, Sep 12, 2018 at 11:53 AM, Maxime Ripard
<maxime.ripard@bootlin.com> wrote:
> On Tue, Sep 11, 2018 at 10:17:02PM +0200, Daniel Vetter wrote:
>> On Tue, Sep 11, 2018 at 01:33:25PM +0200, Maxime Ripard wrote:
>> > Having DRM_SUN4I built-in but DRM_SUN8I_MIXER as a loadable module results in
>> > a link error, as we try to access a symbol from the sun8i_tcon_top.ko module:
>> >
>> > ERROR: "sun8i_tcon_top_de_config" [drivers/gpu/drm/sun4i/sun4i-tcon.ko] undefined!
>> > ERROR: "sun8i_tcon_top_set_hdmi_src" [drivers/gpu/drm/sun4i/sun4i-tcon.ko] undefined!
>> > ERROR: "sun8i_tcon_top_of_table" [drivers/gpu/drm/sun4i/sun4i-tcon.ko] undefined!
>> >
>> > This solves the problem by adding a silent symbol for the tcon_top module,
>> > building it as a separate module in exactly the cases that we need it,
>> > but in a way that it is reachable by the other modules.
>> >
>> > Fixes: cf77d79b4e29 ("drm/sun4i: tcon: Add another way for matching mixers with tcon")
>> > Fixes: 0305189afb32 ("drm/sun4i: tcon: Add support for R40 TCON")
>> > Tested-by: Jon Hunter <jonathanh@nvidia.com>
>> > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
>>
>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>
>> But I can't help myself and drop the usual questions when I see a small
>> soc driver with more Kconfigs than anything else ... is all this pain
>> worth it? I know that maybe the desktop approach of stuffing half a
>> million lines of driver code into one .ko might be a bit too much for
>> socs, but this seems overkill.
>>
>> I'm also pretty sure it's not justified by any real data, compared to
>> overall code size of a drm stack, that shows it's a substantial enough
>> saving that it's worth it.
>
> I'm currently running on a project where the boot time to a qt
> application from power off should be less than a second. You want to
> remove anything you can spare in some situations. And yeah, DRM is the
> biggest thing in the way to do that.

Oh I know all about the 1s people. But is binary size really that
important figure? I know it's a bit more to load&decompress, but it
shouldn't have any impact on anything running at runtime.

>> Imo, if you really care about building a minimal driver, stuff everything
>> into one .ko and then LTO out the uneeded bits. We've done these
>> experiments for i915, that _actually_ saves a ton of binary size, with
>> fairly minimal code and maintenance impact. Still, we decided the payoff
>> is simply too small to bother making it a production thing.
>
> LTO isn't enabled yet in mainline, is it?

Unfortunately not :-/ We've done the analysis and reported to the
internal people who asked for this that "lto is what you want", but
nothing thus far. I guess the price tag of making LTO work is a bit
too much. Ofc doesn't just need LTO, but also needs some trickery to
make sure all the optional paths are statically determined to never
run. But (at least for us) that's just a few changes to the pci table,
and hard-coding the device info structure. OF/DT would probably need
some work, but should be doable too.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/sun4i: fix build failure with CONFIG_DRM_SUN8I_MIXER=m

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 1254 bytes --]

On Tue, Sep 11, 2018 at 10:17:02PM +0200, Daniel Vetter wrote:
> On Tue, Sep 11, 2018 at 01:33:25PM +0200, Maxime Ripard wrote:
> > Having DRM_SUN4I built-in but DRM_SUN8I_MIXER as a loadable module results in
> > a link error, as we try to access a symbol from the sun8i_tcon_top.ko module:
> > 
> > ERROR: "sun8i_tcon_top_de_config" [drivers/gpu/drm/sun4i/sun4i-tcon.ko] undefined!
> > ERROR: "sun8i_tcon_top_set_hdmi_src" [drivers/gpu/drm/sun4i/sun4i-tcon.ko] undefined!
> > ERROR: "sun8i_tcon_top_of_table" [drivers/gpu/drm/sun4i/sun4i-tcon.ko] undefined!
> > 
> > This solves the problem by adding a silent symbol for the tcon_top module,
> > building it as a separate module in exactly the cases that we need it,
> > but in a way that it is reachable by the other modules.
> > 
> > Fixes: cf77d79b4e29 ("drm/sun4i: tcon: Add another way for matching mixers with tcon")
> > Fixes: 0305189afb32 ("drm/sun4i: tcon: Add support for R40 TCON")
> > Tested-by: Jon Hunter <jonathanh@nvidia.com>
> > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

It's pushed, thanks!
Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/sun4i: fix build failure with CONFIG_DRM_SUN8I_MIXER=m

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 2394 bytes --]

On Tue, Sep 11, 2018 at 10:17:02PM +0200, Daniel Vetter wrote:
> On Tue, Sep 11, 2018 at 01:33:25PM +0200, Maxime Ripard wrote:
> > Having DRM_SUN4I built-in but DRM_SUN8I_MIXER as a loadable module results in
> > a link error, as we try to access a symbol from the sun8i_tcon_top.ko module:
> > 
> > ERROR: "sun8i_tcon_top_de_config" [drivers/gpu/drm/sun4i/sun4i-tcon.ko] undefined!
> > ERROR: "sun8i_tcon_top_set_hdmi_src" [drivers/gpu/drm/sun4i/sun4i-tcon.ko] undefined!
> > ERROR: "sun8i_tcon_top_of_table" [drivers/gpu/drm/sun4i/sun4i-tcon.ko] undefined!
> > 
> > This solves the problem by adding a silent symbol for the tcon_top module,
> > building it as a separate module in exactly the cases that we need it,
> > but in a way that it is reachable by the other modules.
> > 
> > Fixes: cf77d79b4e29 ("drm/sun4i: tcon: Add another way for matching mixers with tcon")
> > Fixes: 0305189afb32 ("drm/sun4i: tcon: Add support for R40 TCON")
> > Tested-by: Jon Hunter <jonathanh@nvidia.com>
> > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> But I can't help myself and drop the usual questions when I see a small
> soc driver with more Kconfigs than anything else ... is all this pain
> worth it? I know that maybe the desktop approach of stuffing half a
> million lines of driver code into one .ko might be a bit too much for
> socs, but this seems overkill.
> 
> I'm also pretty sure it's not justified by any real data, compared to
> overall code size of a drm stack, that shows it's a substantial enough
> saving that it's worth it.

I'm currently running on a project where the boot time to a qt
application from power off should be less than a second. You want to
remove anything you can spare in some situations. And yeah, DRM is the
biggest thing in the way to do that.

> Imo, if you really care about building a minimal driver, stuff everything
> into one .ko and then LTO out the uneeded bits. We've done these
> experiments for i915, that _actually_ saves a ton of binary size, with
> fairly minimal code and maintenance impact. Still, we decided the payoff
> is simply too small to bother making it a production thing.

LTO isn't enabled yet in mainline, is it?

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/sun4i: fix build failure with CONFIG_DRM_SUN8I_MIXER=m

[Daniel Vetter]

On Tue, Sep 11, 2018 at 01:33:25PM +0200, Maxime Ripard wrote:
> Having DRM_SUN4I built-in but DRM_SUN8I_MIXER as a loadable module results in
> a link error, as we try to access a symbol from the sun8i_tcon_top.ko module:
> 
> ERROR: "sun8i_tcon_top_de_config" [drivers/gpu/drm/sun4i/sun4i-tcon.ko] undefined!
> ERROR: "sun8i_tcon_top_set_hdmi_src" [drivers/gpu/drm/sun4i/sun4i-tcon.ko] undefined!
> ERROR: "sun8i_tcon_top_of_table" [drivers/gpu/drm/sun4i/sun4i-tcon.ko] undefined!
> 
> This solves the problem by adding a silent symbol for the tcon_top module,
> building it as a separate module in exactly the cases that we need it,
> but in a way that it is reachable by the other modules.
> 
> Fixes: cf77d79b4e29 ("drm/sun4i: tcon: Add another way for matching mixers with tcon")
> Fixes: 0305189afb32 ("drm/sun4i: tcon: Add support for R40 TCON")
> Tested-by: Jon Hunter <jonathanh@nvidia.com>
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

But I can't help myself and drop the usual questions when I see a small
soc driver with more Kconfigs than anything else ... is all this pain
worth it? I know that maybe the desktop approach of stuffing half a
million lines of driver code into one .ko might be a bit too much for
socs, but this seems overkill.

I'm also pretty sure it's not justified by any real data, compared to
overall code size of a drm stack, that shows it's a substantial enough
saving that it's worth it.

Imo, if you really care about building a minimal driver, stuff everything
into one .ko and then LTO out the uneeded bits. We've done these
experiments for i915, that _actually_ saves a ton of binary size, with
fairly minimal code and maintenance impact. Still, we decided the payoff
is simply too small to bother making it a production thing.

Cheers, Daniel

> ---
>  drivers/gpu/drm/sun4i/sun4i_tcon.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> index 4834c90b4912..c78cd35a1294 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> @@ -974,7 +974,8 @@ static bool sun4i_tcon_connected_to_tcon_top(struct device_node *node)
>  
>  	remote = of_graph_get_remote_node(node, 0, -1);
>  	if (remote) {
> -		ret = !!of_match_node(sun8i_tcon_top_of_table, remote);
> +		ret = !!(IS_ENABLED(CONFIG_DRM_SUN8I_TCON_TOP) &&
> +			 of_match_node(sun8i_tcon_top_of_table, remote));
>  		of_node_put(remote);
>  	}
>  
> @@ -1402,13 +1403,20 @@ static int sun8i_r40_tcon_tv_set_mux(struct sun4i_tcon *tcon,
>  	if (!pdev)
>  		return -EINVAL;
>  
> -	if (encoder->encoder_type == DRM_MODE_ENCODER_TMDS) {
> +	if (IS_ENABLED(CONFIG_DRM_SUN8I_TCON_TOP) &&
> +	    encoder->encoder_type == DRM_MODE_ENCODER_TMDS) {
>  		ret = sun8i_tcon_top_set_hdmi_src(&pdev->dev, id);
>  		if (ret)
>  			return ret;
>  	}
>  
> -	return sun8i_tcon_top_de_config(&pdev->dev, tcon->id, id);
> +	if (IS_ENABLED(CONFIG_DRM_SUN8I_TCON_TOP)) {
> +		ret = sun8i_tcon_top_de_config(&pdev->dev, tcon->id, id);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
>  }
>  
>  static const struct sun4i_tcon_quirks sun4i_a10_quirks = {
> -- 
> 2.17.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/sun4i: fix build failure with CONFIG_DRM_SUN8I_MIXER=m

[Maxime Ripard]

Having DRM_SUN4I built-in but DRM_SUN8I_MIXER as a loadable module results in
a link error, as we try to access a symbol from the sun8i_tcon_top.ko module:

ERROR: "sun8i_tcon_top_de_config" [drivers/gpu/drm/sun4i/sun4i-tcon.ko] undefined!
ERROR: "sun8i_tcon_top_set_hdmi_src" [drivers/gpu/drm/sun4i/sun4i-tcon.ko] undefined!
ERROR: "sun8i_tcon_top_of_table" [drivers/gpu/drm/sun4i/sun4i-tcon.ko] undefined!

This solves the problem by adding a silent symbol for the tcon_top module,
building it as a separate module in exactly the cases that we need it,
but in a way that it is reachable by the other modules.

Fixes: cf77d79b4e29 ("drm/sun4i: tcon: Add another way for matching mixers with tcon")
Fixes: 0305189afb32 ("drm/sun4i: tcon: Add support for R40 TCON")
Tested-by: Jon Hunter <jonathanh@nvidia.com>
Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/gpu/drm/sun4i/sun4i_tcon.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index 4834c90b4912..c78cd35a1294 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -974,7 +974,8 @@ static bool sun4i_tcon_connected_to_tcon_top(struct device_node *node)
 
 	remote = of_graph_get_remote_node(node, 0, -1);
 	if (remote) {
-		ret = !!of_match_node(sun8i_tcon_top_of_table, remote);
+		ret = !!(IS_ENABLED(CONFIG_DRM_SUN8I_TCON_TOP) &&
+			 of_match_node(sun8i_tcon_top_of_table, remote));
 		of_node_put(remote);
 	}
 
@@ -1402,13 +1403,20 @@ static int sun8i_r40_tcon_tv_set_mux(struct sun4i_tcon *tcon,
 	if (!pdev)
 		return -EINVAL;
 
-	if (encoder->encoder_type == DRM_MODE_ENCODER_TMDS) {
+	if (IS_ENABLED(CONFIG_DRM_SUN8I_TCON_TOP) &&
+	    encoder->encoder_type == DRM_MODE_ENCODER_TMDS) {
 		ret = sun8i_tcon_top_set_hdmi_src(&pdev->dev, id);
 		if (ret)
 			return ret;
 	}
 
-	return sun8i_tcon_top_de_config(&pdev->dev, tcon->id, id);
+	if (IS_ENABLED(CONFIG_DRM_SUN8I_TCON_TOP)) {
+		ret = sun8i_tcon_top_de_config(&pdev->dev, tcon->id, id);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
 }
 
 static const struct sun4i_tcon_quirks sun4i_a10_quirks = {
-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel