[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

[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>

[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

[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

[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>

[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