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,
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
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
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
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
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@lists.freedesktop.org
-
Arnd Bergmann -
Chen-Yu Tsai -
Jernej Škrabec -
Maxime Ripard