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 = {
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));@@ -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) {
- 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
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
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
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
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
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
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
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
dri-devel@lists.freedesktop.org
-
Arnd Bergmann -
Daniel Vetter -
Maxime Ripard