| Message ID | 20180706124628.3192421-1-arnd@arndb.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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,
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(-)