mainline build failure for x86_64 allmodconfig with clang
Hi All,
The latest mainline kernel branch fails to build for x86_64 allmodconfig with clang. The errors are:
drivers/gpu/drm/amd/amdgpu/../display/dc/dml/dcn30/display_mode_vba_30.c:3596:6: error: stack frame size (2216) exceeds limit (2048) in 'dml30_ModeSupportAndSystemConfigurationFull' [-Werror,-Wframe-larger-than] void dml30_ModeSupportAndSystemConfigurationFull(struct display_mode_lib *mode_lib)
drivers/gpu/drm/amd/amdgpu/../display/dc/dml/dcn31/display_mode_vba_31.c:3908:6: error: stack frame size (2184) exceeds limit (2048) in 'dml31_ModeSupportAndSystemConfigurationFull' [-Werror,-Wframe-larger-than] void dml31_ModeSupportAndSystemConfigurationFull(struct display_mode_lib *mode_lib)
drivers/gpu/drm/amd/amdgpu/../display/dc/dml/dcn32/display_mode_vba_32.c:1726:6: error: stack frame size (2176) exceeds limit (2048) in 'dml32_ModeSupportAndSystemConfigurationFull' [-Werror,-Wframe-larger-than] void dml32_ModeSupportAndSystemConfigurationFull(struct display_mode_lib *mode_lib)
git bisect points to 3876a8b5e241 ("drm/amd/display: Enable building new display engine with KCOV enabled").
My last good build for clang was with e2b542100719 ("Merge tag 'flexible-array-transformations-UAPI-6.0-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux").
I will be happy to test any patch or provide any extra log if needed.
--
Regards
Sudip
Quoted 2 lines · Sudip Mukherjee (Codethink)
git bisect points to
3876a8b5e241("drm/amd/display: Enable building new display engine with KCOV enabled").
Ahh. So that was presumably why it was disabled before - because it presumably does disgusting things that make KCOV generate even bigger stack frames than it already has.
Those functions do seem to have fairly big stack footprints already (I didn't try to look into why, I assume it's partly due to aggressive inlining, and probably some automatic structures on stack). But gcc doesn't seem to make it all that much worse with KCOV (and my clang build doesn't enable KCOV).
So it's presumably some KCOV-vs-clang thing. Nathan?
Linus
Quoted 10 lines · Linus Torvalds
On Thu, Aug 4, 2022 at 11:37 AM Sudip Mukherjee (Codethink) <[email protected]> wrote:cov_trace_cmp
Quoted 2 lines
git bisect points to
3876a8b5e241("drm/amd/display: Enable building new display engine with KCOV enabled").Ahh. So that was presumably why it was disabled before - because it presumably does disgusting things that make KCOV generate even bigger stack frames than it already has.
Quoted 7 lines
Those functions do seem to have fairly big stack footprints already (I didn't try to look into why, I assume it's partly due to aggressive inlining, and probably some automatic structures on stack). But gcc doesn't seem to make it all that much worse with KCOV (and my clang build doesn't enable KCOV).
So it's presumably some KCOV-vs-clang thing. Nathan?
The dependency was originally added to avoid a link failure in 9d1d02ff3678
("drm/amd/display: Don't build DCN1 when kcov is enabled") after I reported the
problem in https://lists.freedesktop.org/archives/dri-devel/2018-August/186131.html
The commit from the bisection just turns off KCOV for the entire directory to avoid the link failure, so it's not actually a problem with KCOV vs clang, but I think a problem with clang vs badly written code that was obscured in allmodconfig builds prior to this.
The dml30_ModeSupportAndSystemConfigurationFull() function exercises a few paths in the compiler that are otherwise rare. On thing it does is to pass up to 60 arguments to other functions, and it heavily uses float and double variables. Both of these make it rather fragile when it comes to unusual compiler options, so the files keep coming up whenever a new instrumentation feature gets added. There is probably some other flag in allmodconfig that we can disable to improve this again, but I have not checked this time.
Arnd
Quoted 12 lines · Arnd Bergmann
Quoted 10 lines · Linus Torvalds
On Thu, Aug 4, 2022 at 11:37 AM Sudip Mukherjee (Codethink) <[email protected]> wrote:cov_trace_cmp
Quoted 2 lines
git bisect points to
3876a8b5e241("drm/amd/display: Enable building new display engine with KCOV enabled").Ahh. So that was presumably why it was disabled before - because it presumably does disgusting things that make KCOV generate even bigger stack frames than it already has.
Quoted 7 lines
Quoted 7 lines
Those functions do seem to have fairly big stack footprints already (I didn't try to look into why, I assume it's partly due to aggressive inlining, and probably some automatic structures on stack). But gcc doesn't seem to make it all that much worse with KCOV (and my clang build doesn't enable KCOV).
So it's presumably some KCOV-vs-clang thing. Nathan?
Looks like Arnd beat me to it :)
Quoted 8 lines
The dependency was originally added to avoid a link failure in
9d1d02ff3678
("drm/amd/display: Don't build DCN1 when kcov is enabled") after I reported the
problem in https://lists.freedesktop.org/archives/dri-devel/2018-August/186131.htmlThe commit from the bisection just turns off KCOV for the entire directory to avoid the link failure, so it's not actually a problem with KCOV vs clang, but I think a problem with clang vs badly written code that was obscured in allmodconfig builds prior to this.
Right, I do think the sanitizers make things worse here too, as those get enabled with allmodconfig. I ran some really quick tests with allmodconfig and a few instrumentation options flipped on/off:
allmodconfig (CONFIG_KASAN=y, CONFIG_KCSAN=n, CONFIG_KCOV=y, and CONFIG_UBSAN=y):
warning: stack frame size (2216) exceeds limit (2048) in 'dml30_ModeSupportAndSystemConfigurationFull' [-Wframe-larger-than]
warning: stack frame size (2184) exceeds limit (2048) in 'dml31_ModeSupportAndSystemConfigurationFull' [-Wframe-larger-than]
warning: stack frame size (2176) exceeds limit (2048) in 'dml32_ModeSupportAndSystemConfigurationFull' [-Wframe-larger-than]
allmodconfig + CONFIG_KASAN=n:
warning: stack frame size (2112) exceeds limit (2048) in 'dml32_ModeSupportAndSystemConfigurationFull' [-Wframe-larger-than]
allmodconfig + CONFIG_KCOV=n:
warning: stack frame size (2216) exceeds limit (2048) in 'dml30_ModeSupportAndSystemConfigurationFull' [-Wframe-larger-than]
warning: stack frame size (2184) exceeds limit (2048) in 'dml31_ModeSupportAndSystemConfigurationFull' [-Wframe-larger-than]
warning: stack frame size (2176) exceeds limit (2048) in 'dml32_ModeSupportAndSystemConfigurationFull' [-Wframe-larger-than]
allmodconfig + CONFIG_UBSAN=n:
warning: stack frame size (2584) exceeds limit (2048) in 'dml30_ModeSupportAndSystemConfigurationFull' [-Wframe-larger-than]
warning: stack frame size (2680) exceeds limit (2048) in 'dml31_ModeSupportAndSystemConfigurationFull' [-Wframe-larger-than]
warning: stack frame size (2352) exceeds limit (2048) in 'dml32_ModeSupportAndSystemConfigurationFull' [-Wframe-larger-than]
allmodconfig + CONFIG_KASAN=n + CONFIG_KCSAN=y + CONFIG_UBSAN=n:
warning: stack frame size (2504) exceeds limit (2048) in 'dml30_ModeSupportAndSystemConfigurationFull' [-Wframe-larger-than]
warning: stack frame size (2600) exceeds limit (2048) in 'dml31_ModeSupportAndSystemConfigurationFull' [-Wframe-larger-than]
warning: stack frame size (2264) exceeds limit (2048) in 'dml32_ModeSupportAndSystemConfigurationFull' [-Wframe-larger-than]
allmodconfig + CONFIG_KASAN=n + CONFIG_KCSAN=n + CONFIG_UBSAN=n:
warning: stack frame size (2072) exceeds limit (2048) in 'dml31_ModeSupportAndSystemConfigurationFull' [-Wframe-larger-than]
There might be other debugging configurations that make this worse too, as I don't see those warnings on my distribution configuration.
Quoted 8 lines
The
dml30_ModeSupportAndSystemConfigurationFull()function exercises a few paths in the compiler that are otherwise rare. On thing it does is to pass up to 60 arguments to other functions, and it heavily uses float and double variables. Both of these make it rather fragile when it comes to unusual compiler options, so the files keep coming up whenever a new instrumentation feature gets added. There is probably some other flag in allmodconfig that we can disable to improve this again, but I have not checked this time.
I do notice that these files build with a non-configurable -Wframe-large-than value:
$ rg frame_warn_flag drivers/gpu/drm/amd/display/dc/dml/Makefile 54:frame_warn_flag := -Wframe-larger-than=2048 70:CFLAGS_$(AMDDALPATH)/dc/dml/dcn30/display_mode_vba_30.o := $(dml_ccflags) $(frame_warn_flag) 72:CFLAGS_$(AMDDALPATH)/dc/dml/dcn31/display_mode_vba_31.o := $(dml_ccflags) $(frame_warn_flag) 76:CFLAGS_$(AMDDALPATH)/dc/dml/dcn32/display_mode_vba_32.o := $(dml_ccflags) $(frame_warn_flag)
I suppose that could just be bumped as a quick workaround? Two of those files have a comment that implies modifying them in non-trivial ways is not recommended.
/*
* NOTE:
* This file is gcc-parsable HW gospel, coming straight from HW engineers.
*
* It doesn't adhere to Linux kernel style and sometimes will do things in odd
* ways. Unless there is something clearly wrong with it the code should
* remain as-is as it provides us with a guarantee from HW that it is correct. */
I do note that commit 1b54a0121dba ("drm/amd/display: Reduce stack size in the mode support function") did have a workaround for GCC. It appears clang will still inline mode_support_configuration(). If I mark it as 'noinline', the warning disappears in that file.
Cheers,
Nathan
Quoted 5 lines · Nathan Chancellor
I do note that commit
1b54a0121dba("drm/amd/display: Reduce stack size in the mode support function") did have a workaround for GCC. It appears clang will still inlinemode_support_configuration(). If I mark it as 'noinline', the warning disappears in that file.
That sounds like probably the best option for now. Gcc does not inline that function (at least for allmodconfig builds in my testing), so if that makes clang match what gcc does, it seems a reasonable thing to do.
Linus
Quoted 11 lines · Linus Torvalds
Quoted 5 lines · Nathan Chancellor
I do note that commit
1b54a0121dba("drm/amd/display: Reduce stack size in the mode support function") did have a workaround for GCC. It appears clang will still inlinemode_support_configuration(). If I mark it as 'noinline', the warning disappears in that file.That sounds like probably the best option for now. Gcc does not inline that function (at least for allmodconfig builds in my testing), so if that makes clang match what gcc does, it seems a reasonable thing to do.
Sounds good. That solution only takes care of the warning in display_mode_vba_32.c. I will try and come up with something similar for the other two files tomorrow, unless the AMD folks beat me to it, since they will know the driver better than I will ;)
Cheers,
Nathan
...
Quoted 2 lines
* NOTE:
* This file is gcc-parsable HW gospel, coming straight from HW engineers.
I never trust hardware engineers to write code :-) (Although at the moment they trust me to write VHDL...)
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Harry Wentland
maintainerQuoted 79 lines · Nathan Chancellor
Quoted 12 lines · Arnd Bergmann
Quoted 10 lines · Linus Torvalds
On Thu, Aug 4, 2022 at 11:37 AM Sudip Mukherjee (Codethink) <[email protected]> wrote:cov_trace_cmp
Quoted 2 lines
git bisect points to
3876a8b5e241("drm/amd/display: Enable building new display engine with KCOV enabled").Ahh. So that was presumably why it was disabled before - because it presumably does disgusting things that make KCOV generate even bigger stack frames than it already has.
Quoted 7 lines
Quoted 7 lines
Those functions do seem to have fairly big stack footprints already (I didn't try to look into why, I assume it's partly due to aggressive inlining, and probably some automatic structures on stack). But gcc doesn't seem to make it all that much worse with KCOV (and my clang build doesn't enable KCOV).
So it's presumably some KCOV-vs-clang thing. Nathan?
Looks like Arnd beat me to it :)
Quoted 7 lines
The dependency was originally added to avoid a link failure in
9d1d02ff3678
("drm/amd/display: Don't build DCN1 when kcov is enabled") after I reported the
problem in https://lists.freedesktop.org/archives/dri-devel/2018-August/186131.html>>>
The commit from the bisection just turns off KCOV for the entire directory to avoid the link failure, so it's not actually a problem with KCOV vs clang, but I think a problem with clang vs badly written code that was obscured in allmodconfig builds prior to this.Right, I do think the sanitizers make things worse here too, as those get enabled with allmodconfig. I ran some really quick tests with allmodconfig and a few instrumentation options flipped on/off:
allmodconfig (
CONFIG_KASAN=y,CONFIG_KCSAN=n,CONFIG_KCOV=y, andCONFIG_UBSAN=y):warning: stack frame size (2216) exceeds limit (2048) in '
dml30_ModeSupportAndSystemConfigurationFull' [-Wframe-larger-than]
warning: stack frame size (2184) exceeds limit (2048) in 'dml31_ModeSupportAndSystemConfigurationFull' [-Wframe-larger-than]
warning: stack frame size (2176) exceeds limit (2048) in 'dml32_ModeSupportAndSystemConfigurationFull' [-Wframe-larger-than]allmodconfig +
CONFIG_KASAN=n:warning: stack frame size (2112) exceeds limit (2048) in '
dml32_ModeSupportAndSystemConfigurationFull' [-Wframe-larger-than]allmodconfig +
CONFIG_KCOV=n:warning: stack frame size (2216) exceeds limit (2048) in '
dml30_ModeSupportAndSystemConfigurationFull' [-Wframe-larger-than]
warning: stack frame size (2184) exceeds limit (2048) in 'dml31_ModeSupportAndSystemConfigurationFull' [-Wframe-larger-than]
warning: stack frame size (2176) exceeds limit (2048) in 'dml32_ModeSupportAndSystemConfigurationFull' [-Wframe-larger-than]allmodconfig +
CONFIG_UBSAN=n:warning: stack frame size (2584) exceeds limit (2048) in '
dml30_ModeSupportAndSystemConfigurationFull' [-Wframe-larger-than]
warning: stack frame size (2680) exceeds limit (2048) in 'dml31_ModeSupportAndSystemConfigurationFull' [-Wframe-larger-than]
warning: stack frame size (2352) exceeds limit (2048) in 'dml32_ModeSupportAndSystemConfigurationFull' [-Wframe-larger-than]allmodconfig +
CONFIG_KASAN=n+CONFIG_KCSAN=y+CONFIG_UBSAN=n:warning: stack frame size (2504) exceeds limit (2048) in '
dml30_ModeSupportAndSystemConfigurationFull' [-Wframe-larger-than]
warning: stack frame size (2600) exceeds limit (2048) in 'dml31_ModeSupportAndSystemConfigurationFull' [-Wframe-larger-than]
warning: stack frame size (2264) exceeds limit (2048) in 'dml32_ModeSupportAndSystemConfigurationFull' [-Wframe-larger-than]allmodconfig +
CONFIG_KASAN=n+CONFIG_KCSAN=n+CONFIG_UBSAN=n:warning: stack frame size (2072) exceeds limit (2048) in '
dml31_ModeSupportAndSystemConfigurationFull' [-Wframe-larger-than]There might be other debugging configurations that make this worse too, as I don't see those warnings on my distribution configuration.
Quoted 8 lines
The
dml30_ModeSupportAndSystemConfigurationFull()function exercises a few paths in the compiler that are otherwise rare. On thing it does is to pass up to 60 arguments to other functions, and it heavily uses float and double variables. Both of these make it rather fragile when it comes to unusual compiler options, so the files keep coming up whenever a new instrumentation feature gets added. There is probably some other flag in allmodconfig that we can disable to improve this again, but I have not checked this time.
Quoted 5 lines
I do notice that these files build with a non-configurable -Wframe-large-than value:
$ rg frame_warn_flag drivers/gpu/drm/amd/display/dc/dml/Makefile 54:frame_warn_flag := -Wframe-larger-than=2048
Tbh, I was looking at the history and I can't find a good reason this was added. It should be safe to drop this. I would much rather use the CONFIG_FRAME_WARN value than override it.
AFAIK most builds use 2048 by default anyways.
Quoted 17 lines
70:CFLAGS_$(AMDDALPATH)/dc/dml/dcn30/display_mode_vba_30.o := $(dml_ccflags) $(frame_warn_flag) 72:CFLAGS_$(AMDDALPATH)/dc/dml/dcn31/display_mode_vba_31.o := $(dml_ccflags) $(frame_warn_flag) 76:CFLAGS_$(AMDDALPATH)/dc/dml/dcn32/display_mode_vba_32.o := $(dml_ccflags) $(frame_warn_flag)
I suppose that could just be bumped as a quick workaround? Two of those files have a comment that implies modifying them in non-trivial ways is not recommended.
/*
* NOTE:
* This file is gcc-parsable HW gospel, coming straight from HW engineers.
*
* It doesn't adhere to Linux kernel style and sometimes will do things in odd
* ways. Unless there is something clearly wrong with it the code should
* remain as-is as it provides us with a guarantee from HW that it is correct. */
Quoted 5 lines
I do note that commit
1b54a0121dba("drm/amd/display: Reduce stack size in the mode support function") did have a workaround for GCC. It appears clang will still inlinemode_support_configuration(). If I mark it as 'noinline', the warning disappears in that file.
That'd be the best quick fix. I guess if we split out functions to fix stack usage we should mark them as 'noinline' in the future to avoid agressive compiler optimizations.
Harry
Quoted 2 lines
Cheers,
Nathan
Quoted 11 lines · Harry Wentland
Quoted 5 lines
I do notice that these files build with a non-configurable -Wframe-large-than value:
$ rg frame_warn_flag drivers/gpu/drm/amd/display/dc/dml/Makefile 54:frame_warn_flag := -Wframe-larger-than=2048Tbh, I was looking at the history and I can't find a good reason this was added. It should be safe to drop this. I would much rather use the
CONFIG_FRAME_WARNvalue than override it.AFAIK most builds use 2048 by default anyways.
I'm fairly sure this was done for 32-bit builds, which default to a lower warning limit of 1024 bytes and would otherwise run into this problem when 64-bit platforms don't. With the default warning limit, clang warns even more about an i386 build:
display/dc/dml/dcn20/display_rq_dlg_calc_20.c:1549:6: error: stack
frame size (1324) exceeds limit (1024) in 'dml20_rq_dlg_get_dlg_reg' display/dc/dml/dcn20/display_rq_dlg_calc_20v2.c:1550:6: error: stack
frame size (1324) exceeds limit (1024) in 'dml20v2_rq_dlg_get_dlg_reg' display/dc/dml/dcn30/display_rq_dlg_calc_30.c:1742:6: error: stack
frame size (1484) exceeds limit (1024) in 'dml30_rq_dlg_get_dlg_reg' display/dc/dml/dcn31/display_rq_dlg_calc_31.c:1571:6: error: stack
frame size (1548) exceeds limit (1024) in 'dml31_rq_dlg_get_dlg_reg' display/dc/dml/dcn21/display_rq_dlg_calc_21.c:1657:6: error: stack
frame size (1388) exceeds limit (1024) in 'dml21_rq_dlg_get_dlg_reg' display/dc/dml/dcn32/display_rq_dlg_calc_32.c:206:6: error: stack
frame size (1276) exceeds limit (1024) in 'dml32_rq_dlg_get_dlg_reg' display/dc/dml/dcn31/display_mode_vba_31.c:2049:13: error: stack frame
size (1468) exceeds limit (1024) in 'DISPCLKDPPCLKDCFCLKDeepSleepPrefetchParametersWatermarksAndPerformanceCalculation' display/dc/dml/dcn20/display_mode_vba_20v2.c:1145:13: error: stack
frame size (1228) exceeds limit (1024) in 'dml20v2_DISPCLKDPPCLKDCFCLKDeepSleepPrefetchParametersWatermarksAndPerformanceCalculation' display/dc/dml/dcn20/display_mode_vba_20.c:1085:13: error: stack frame
size (1340) exceeds limit (1024) in 'dml20_DISPCLKDPPCLKDCFCLKDeepSleepPrefetchParametersWatermarksAndPerformanceCalculation' display/dc/dml/dcn31/display_mode_vba_31.c:3908:6: error: stack frame
size (1996) exceeds limit (1024) in 'dml31_ModeSupportAndSystemConfigurationFull' display/dc/dml/dcn21/display_mode_vba_21.c:1466:13: error: stack frame
size (1308) exceeds limit (1024) in 'DISPCLKDPPCLKDCFCLKDeepSleepPrefetchParametersWatermarksAndPerformanceCalculation' display/dc/dml/dcn20/display_mode_vba_20v2.c:3393:6: error: stack
frame size (1356) exceeds limit (1024) in 'dml20v2_ModeSupportAndSystemConfigurationFull' display/dc/dml/dcn20/display_mode_vba_20.c:3286:6: error: stack frame
size (1468) exceeds limit (1024) in 'dml20_ModeSupportAndSystemConfigurationFull' display/dc/dml/dcn21/display_mode_vba_21.c:3518:6: error: stack frame
size (1228) exceeds limit (1024) in 'dml21_ModeSupportAndSystemConfigurationFull' display/dc/dml/dcn30/display_mode_vba_30.c:1906:13: error: stack frame
size (1436) exceeds limit (1024) in 'DISPCLKDPPCLKDCFCLKDeepSleepPrefetchParametersWatermarksAndPerformanceCalculation' display/dc/dml/dcn30/display_mode_vba_30.c:3596:6: error: stack frame
size (2092) exceeds limit (1024) in 'dml30_ModeSupportAndSystemConfigurationFull'
Quoted 8 lines
Quoted 4 lines
I do note that commit
1b54a0121dba("drm/amd/display: Reduce stack size in the mode support function") did have a workaround for GCC. It appears clang will still inlinemode_support_configuration(). If I mark it as 'noinline', the warning disappears in that file.That'd be the best quick fix. I guess if we split out functions to fix stack usage we should mark them as 'noinline' in the future to avoid agressive compiler optimizations.
While splitting out sub-functions can help reduce the maximum stack usage, it seems that in this case it makes the actual problem worse:
I see 2168 bytes for the combined dml32_ModeSupportAndSystemConfigurationFull(), but marking mode_support_configuration() as noinline gives me 1992 bytes for the outer function plus 384 bytes for the inner one. So it does avoid the warning (barely), but not the problem that the warning tries to point out.
Arnd
Quoted 69 lines · Arnd Bergmann
Quoted 11 lines · Harry Wentland
Quoted 5 lines
I do notice that these files build with a non-configurable -Wframe-large-than value:
$ rg frame_warn_flag drivers/gpu/drm/amd/display/dc/dml/Makefile 54:frame_warn_flag := -Wframe-larger-than=2048Tbh, I was looking at the history and I can't find a good reason this was added. It should be safe to drop this. I would much rather use the
CONFIG_FRAME_WARNvalue than override it.AFAIK most builds use 2048 by default anyways.
I'm fairly sure this was done for 32-bit builds, which default to a lower warning limit of 1024 bytes and would otherwise run into this problem when 64-bit platforms don't. With the default warning limit, clang warns even more about an i386 build:
display/dc/dml/dcn20/display_rq_dlg_calc_20.c:1549:6: error: stack
frame size (1324) exceeds limit (1024) in 'dml20_rq_dlg_get_dlg_reg'display/dc/dml/dcn20/display_rq_dlg_calc_20v2.c:1550:6: error: stack
frame size (1324) exceeds limit (1024) in 'dml20v2_rq_dlg_get_dlg_reg'display/dc/dml/dcn30/display_rq_dlg_calc_30.c:1742:6: error: stack
frame size (1484) exceeds limit (1024) in 'dml30_rq_dlg_get_dlg_reg'display/dc/dml/dcn31/display_rq_dlg_calc_31.c:1571:6: error: stack
frame size (1548) exceeds limit (1024) in 'dml31_rq_dlg_get_dlg_reg'display/dc/dml/dcn21/display_rq_dlg_calc_21.c:1657:6: error: stack
frame size (1388) exceeds limit (1024) in 'dml21_rq_dlg_get_dlg_reg'display/dc/dml/dcn32/display_rq_dlg_calc_32.c:206:6: error: stack
frame size (1276) exceeds limit (1024) in 'dml32_rq_dlg_get_dlg_reg'display/dc/dml/dcn31/display_mode_vba_31.c:2049:13: error: stack frame
size (1468) exceeds limit (1024) in 'DISPCLKDPPCLKDCFCLKDeepSleepPrefetchParametersWatermarksAndPerformanceCalculation'display/dc/dml/dcn20/display_mode_vba_20v2.c:1145:13: error: stack
frame size (1228) exceeds limit (1024) in 'dml20v2_DISPCLKDPPCLKDCFCLKDeepSleepPrefetchParametersWatermarksAndPerformanceCalculation'display/dc/dml/dcn20/display_mode_vba_20.c:1085:13: error: stack frame
size (1340) exceeds limit (1024) in 'dml20_DISPCLKDPPCLKDCFCLKDeepSleepPrefetchParametersWatermarksAndPerformanceCalculation'display/dc/dml/dcn31/display_mode_vba_31.c:3908:6: error: stack frame
size (1996) exceeds limit (1024) in 'dml31_ModeSupportAndSystemConfigurationFull'display/dc/dml/dcn21/display_mode_vba_21.c:1466:13: error: stack frame
size (1308) exceeds limit (1024) in 'DISPCLKDPPCLKDCFCLKDeepSleepPrefetchParametersWatermarksAndPerformanceCalculation'display/dc/dml/dcn20/display_mode_vba_20v2.c:3393:6: error: stack
frame size (1356) exceeds limit (1024) in 'dml20v2_ModeSupportAndSystemConfigurationFull'display/dc/dml/dcn20/display_mode_vba_20.c:3286:6: error: stack frame
size (1468) exceeds limit (1024) in 'dml20_ModeSupportAndSystemConfigurationFull'display/dc/dml/dcn21/display_mode_vba_21.c:3518:6: error: stack frame
size (1228) exceeds limit (1024) in 'dml21_ModeSupportAndSystemConfigurationFull'display/dc/dml/dcn30/display_mode_vba_30.c:1906:13: error: stack frame
size (1436) exceeds limit (1024) in 'DISPCLKDPPCLKDCFCLKDeepSleepPrefetchParametersWatermarksAndPerformanceCalculation'display/dc/dml/dcn30/display_mode_vba_30.c:3596:6: error: stack frame
size (2092) exceeds limit (1024) in 'dml30_ModeSupportAndSystemConfigurationFull'Quoted 8 lines
Quoted 4 lines
I do note that commit
1b54a0121dba("drm/amd/display: Reduce stack size in the mode support function") did have a workaround for GCC. It appears clang will still inlinemode_support_configuration(). If I mark it as 'noinline', the warning disappears in that file.That'd be the best quick fix. I guess if we split out functions to fix stack usage we should mark them as 'noinline' in the future to avoid agressive compiler optimizations.
Quoted 8 lines
While splitting out sub-functions can help reduce the maximum stack usage, it seems that in this case it makes the actual problem worse:
I see 2168 bytes for the combineddml32_ModeSupportAndSystemConfigurationFull(), but markingmode_support_configuration()as noinline gives me 1992 bytes for the outer function plus 384 bytes for the inner one. So it does avoid the warning (barely), but not the problem that the warning tries to point out.
I haven't had a chance to take a look at splitting things up yet, would you recommend a different approach?
Cheers,
Nathan
Quoted 11 lines · Nathan Chancellor
Quoted 9 lines · Arnd Bergmann
On Fri, Aug 5, 2022 at 5:32 PM Harry Wentland <[email protected]> wrote:
While splitting out sub-functions can help reduce the maximum stack usage, it seems that in this case it makes the actual problem worse:
I see 2168 bytes for the combineddml32_ModeSupportAndSystemConfigurationFull(), but markingmode_support_configuration()as noinline gives me 1992 bytes
for the outer function plus 384 bytes for the inner one. So it does avoid the warning (barely), but not the problem that the warning tries to point out.
Quoted 2 lines
I haven't had a chance to take a look at splitting things up yet, would you recommend a different approach?
Splitting up large functions can help when you have large local variables that are used in different parts of the function, and the split gets the compiler to reuse stack locations.
I think in this particular function, the problem isn't actually local variables but either pushing variables on the stack for argument passing,
or something that causes the compiler to run out of registers so it has to spill registers to the stack.
In either case, one has to actually look at the generated output and then try to rearrange the codes so this does not happen.
One thing to try would be to condense a function call like
dml32_CalculateWatermarksMALLUseAndDRAMSpeedChangeSupport(
&v->dummy_vars.dml32_CalculateWatermarksMALLUseAndDRAMSpeedChangeSupport,
mode_lib->vba.USRRetrainingRequiredFinal,
mode_lib->vba.UsesMALLForPStateChange,mode_lib->vba.PrefetchModePerState[mode_lib->vba.VoltageLevel][mode_lib->vba.maxMpcComb],
mode_lib->vba.NumberOfActiveSurfaces,
mode_lib->vba.MaxLineBufferLines,
mode_lib->vba.LineBufferSizeFinal,
mode_lib->vba.WritebackInterfaceBufferSize,
mode_lib->vba.DCFCLK,
mode_lib->vba.ReturnBW,
mode_lib->vba.SynchronizeTimingsFinal,mode_lib->vba.SynchronizeDRRDisplaysForUCLKPStateChangeFinal,
mode_lib->vba.DRRDisplay,
v->dpte_group_bytes,
v->meta_row_height,
v->meta_row_height_chroma,v->dummy_vars.DISPCLKDPPCLKDCFCLKDeepSleepPrefetchParametersWatermarksAndPerformanceCalculation.mmSOCParameters,
mode_lib->vba.WritebackChunkSize,
mode_lib->vba.SOCCLK,
v->DCFCLKDeepSleep,
mode_lib->vba.DETBufferSizeY,
mode_lib->vba.DETBufferSizeC,
mode_lib->vba.SwathHeightY,
mode_lib->vba.SwathHeightC,
mode_lib->vba.LBBitPerPixel,
v->SwathWidthY,
v->SwathWidthC,
mode_lib->vba.HRatio,
mode_lib->vba.HRatioChroma,
mode_lib->vba.vtaps,
mode_lib->vba.VTAPsChroma,
mode_lib->vba.VRatio,
mode_lib->vba.VRatioChroma,
mode_lib->vba.HTotal,
mode_lib->vba.VTotal,
mode_lib->vba.VActive,
mode_lib->vba.PixelClock,
mode_lib->vba.BlendingAndTiming,
.... /* more arguments */);into calling conventions that take a pointer to 'mode_lib->vba' and another one to 'v', so these are no longer passed on the stack individually.
Arnd
From: Arnd Bergmann
Quoted 1 line
Sent: 05 August 2022 20:32
...
Quoted 4 lines
One thing to try would be to condense a function call like
dml32_CalculateWatermarksMALLUseAndDRAMSpeedChangeSupport(
...
Quoted 4 lines
.... /* more arguments */);into calling conventions that take a pointer to '
mode_lib->vba' and another one to 'v', so these are no longer passed on the stack individually.
Or, if it is only called once (I can't find the source) force it to be inlined.
Or just shoot the software engineer who thinks 100 arguments is sane. :-)
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Quoted 3 lines · David Laight
Or just shoot the software engineer who thinks 100 arguments is sane. :-)
I suspect the issue is that it's not primarily a software engineer who wrote that code.
Hardware people writing code are about as scary as software engineers with a soldering iron.
Linus
Hi Arnd,
Doubling back around to this now since I think this is the only thing breaking x86_64 allmodconfig with clang 11 through 15.
Quoted 74 lines · Arnd Bergmann
Quoted 11 lines · Nathan Chancellor
Quoted 9 lines · Arnd Bergmann
On Fri, Aug 5, 2022 at 5:32 PM Harry Wentland <[email protected]> wrote:
While splitting out sub-functions can help reduce the maximum stack usage, it seems that in this case it makes the actual problem worse:
I see 2168 bytes for the combineddml32_ModeSupportAndSystemConfigurationFull(), but markingmode_support_configuration()as noinline gives me 1992 bytes
for the outer function plus 384 bytes for the inner one. So it does avoid the warning (barely), but not the problem that the warning tries to point out.Quoted 2 lines
I haven't had a chance to take a look at splitting things up yet, would you recommend a different approach?
Splitting up large functions can help when you have large local variables that are used in different parts of the function, and the split gets the compiler to reuse stack locations.
I think in this particular function, the problem isn't actually local variables but either pushing variables on the stack for argument passing,
or something that causes the compiler to run out of registers so it has to spill registers to the stack.In either case, one has to actually look at the generated output and then try to rearrange the codes so this does not happen.
One thing to try would be to condense a function call like
dml32_CalculateWatermarksMALLUseAndDRAMSpeedChangeSupport(
&v->dummy_vars.dml32_CalculateWatermarksMALLUseAndDRAMSpeedChangeSupport,mode_lib->vba.USRRetrainingRequiredFinal, mode_lib->vba.UsesMALLForPStateChange,mode_lib->vba.PrefetchModePerState[mode_lib->vba.VoltageLevel][mode_lib->vba.maxMpcComb],
mode_lib->vba.NumberOfActiveSurfaces, mode_lib->vba.MaxLineBufferLines, mode_lib->vba.LineBufferSizeFinal, mode_lib->vba.WritebackInterfaceBufferSize, mode_lib->vba.DCFCLK, mode_lib->vba.ReturnBW, mode_lib->vba.SynchronizeTimingsFinal,
mode_lib->vba.SynchronizeDRRDisplaysForUCLKPStateChangeFinal,mode_lib->vba.DRRDisplay, v->dpte_group_bytes, v->meta_row_height, v->meta_row_height_chroma,
v->dummy_vars.DISPCLKDPPCLKDCFCLKDeepSleepPrefetchParametersWatermarksAndPerformanceCalculation.mmSOCParameters,mode_lib->vba.WritebackChunkSize, mode_lib->vba.SOCCLK, v->DCFCLKDeepSleep, mode_lib->vba.DETBufferSizeY, mode_lib->vba.DETBufferSizeC, mode_lib->vba.SwathHeightY, mode_lib->vba.SwathHeightC, mode_lib->vba.LBBitPerPixel, v->SwathWidthY, v->SwathWidthC, mode_lib->vba.HRatio, mode_lib->vba.HRatioChroma, mode_lib->vba.vtaps, mode_lib->vba.VTAPsChroma, mode_lib->vba.VRatio, mode_lib->vba.VRatioChroma, mode_lib->vba.HTotal, mode_lib->vba.VTotal, mode_lib->vba.VActive, mode_lib->vba.PixelClock, mode_lib->vba.BlendingAndTiming, .... /* more arguments */);
Quoted 2 lines
into calling conventions that take a pointer to '
mode_lib->vba' and another one to 'v', so these are no longer passed on the stack individually.
So I took a whack at reducing this function's number of parameters and ended up with the attached patch. I basically just removed any parameters that were identical between the two call sites and access them through the vba pointer, as you suggested.
AMD folks, is this an acceptable approach? It didn't take a trivial amount of time so I want to make sure this is okay before I do it to more functions/files.
Due to the potential size of these changes, I am a little weary of them going into 6.0; even though they should be a simple search and replace for the most part, it might be nice for them to have some decent soak time in -next. One solution would be to raise the warning limit for these files on 6.0 so that allmodconfig does not ship broken then reduce the limit for 6.1 once these patches have been applied.
Additionally, I took a look at the stack usage across all compilers that the kernel supports and I thought it was kind of interesting that the usage really jumps from GCC 7 to 8, which I am guessing is a result of commit e8a170ff9a35 ("drm/amdgpu: enable -msse2 for GCC 7.1+ users"). GCC 8 allmodconfig actually errors now too:
https://lore.kernel.org/[email protected]/
|-----------------------------------------------|-----------------------------------------------|-----------------------------------------------|
| dml30_ModeSupportAndSystemConfigurationFull() | dml31_ModeSupportAndSystemConfigurationFull() | dml32_ModeSupportAndSystemConfigurationFull() |
|---------|-----------------------------------------------|-----------------------------------------------|-----------------------------------------------|
| GCC 5 | 1056 bytes | 656 bytes | 1040 bytes |
| GCC 6 | 1024 bytes | 672 bytes | 1056 bytes |
| GCC 7 | 1040 bytes | 664 bytes | 1056 bytes |
| GCC 8 | 1760 bytes | 1608 bytes | 2144 bytes |
| GCC 9 | 1664 bytes | 1392 bytes | 1960 bytes |
| GCC 10 | 1648 bytes | 1368 bytes | 1952 bytes |
| GCC 11 | 1680 bytes | 1400 bytes | 1952 bytes |
| GCC 12 | 1680 bytes | 1400 bytes | 1984 bytes |
|---------|-----------------------------------------------|-----------------------------------------------|-----------------------------------------------|
| LLVM 11 | 2104 bytes | 2056 bytes | 2120 bytes |
| LLVM 12 | 2152 bytes | 2200 bytes | 2152 bytes |
| LLVM 13 | 2216 bytes | 2248 bytes | 2168 bytes |
| LLVM 14 | 2168 bytes | 2184 bytes | 2160 bytes |
| LLVM 15 | 2216 bytes | 2184 bytes | 2160 bytes |
| LLVM 16 | 2232 bytes | 2216 bytes | 2176 bytes |
|---------|-----------------------------------------------|-----------------------------------------------|-----------------------------------------------|With the patch I have attached, dml32_ModeSupportAndSystemConfigurationFull() drops from 2176 to 1944 for LLVM 16, which is obviously still not great but it at least avoids the warning.
Cheers,
Nathan
From 193f6f6f708df6949b0c4df9fe99903e1252211a Mon Sep 17 00:00:00 2001
From: Nathan Chancellor <[email protected]>
Date: Thu, 18 Aug 2022 08:36:51 -0700
Subject: [PATCH] drm/amd/display: Reduce number of arguments to
dml32_CalculateWatermarksMALLUseAndDRAMSpeedChangeSupport()
--- .../dc/dml/dcn32/display_mode_vba_32.c | 72 +---- .../dc/dml/dcn32/display_mode_vba_util_32.c | 259 ++++++++---------- .../dc/dml/dcn32/display_mode_vba_util_32.h | 36 +-- 3 files changed, 120 insertions(+), 247 deletions(-)
drivers/gpu/drm/amd/display/dc/dml/dcn32/display_mode_vba_32.c +4 -68
@@ -1167,66 +1167,34 @@ static void DISPCLKDPPCLKDCFCLKDeepSleepPrefetchParametersWatermarksAndPerforman v->dummy_vars.DISPCLKDPPCLKDCFCLKDeepSleepPrefetchParametersWatermarksAndPerformanceCalculation.mmSOCParameters.SMNLatency = mode_lib->vba.SMNLatency;dml32_CalculateWatermarksMALLUseAndDRAMSpeedChangeSupport ( - &v->dummy_vars.dml32_CalculateWatermarksMALLUseAndDRAMSpeedChangeSupport, - mode_lib->vba.USRRetrainingRequiredFinal, - mode_lib->vba.UsesMALLForPStateChange, + v, mode_lib->vba.PrefetchModePerState[mode_lib->vba.VoltageLevel][mode_lib->vba.maxMpcComb], - mode_lib->vba.NumberOfActiveSurfaces, - mode_lib->vba.MaxLineBufferLines, - mode_lib->vba.LineBufferSizeFinal, - mode_lib->vba.WritebackInterfaceBufferSize, mode_lib->vba.DCFCLK, mode_lib->vba.ReturnBW, - mode_lib->vba.SynchronizeTimingsFinal, - mode_lib->vba.SynchronizeDRRDisplaysForUCLKPStateChangeFinal, - mode_lib->vba.DRRDisplay, - v->dpte_group_bytes, - v->meta_row_height, - v->meta_row_height_chroma, v->dummy_vars.DISPCLKDPPCLKDCFCLKDeepSleepPrefetchParametersWatermarksAndPerformanceCalculation.mmSOCParameters, - mode_lib->vba.WritebackChunkSize, mode_lib->vba.SOCCLK, v->DCFCLKDeepSleep, mode_lib->vba.DETBufferSizeY, mode_lib->vba.DETBufferSizeC, mode_lib->vba.SwathHeightY, mode_lib->vba.SwathHeightC, - mode_lib->vba.LBBitPerPixel, v->SwathWidthY, v->SwathWidthC, - mode_lib->vba.HRatio, - mode_lib->vba.HRatioChroma, - mode_lib->vba.vtaps, - mode_lib->vba.VTAPsChroma, - mode_lib->vba.VRatio, - mode_lib->vba.VRatioChroma, - mode_lib->vba.HTotal, - mode_lib->vba.VTotal, - mode_lib->vba.VActive, - mode_lib->vba.PixelClock, - mode_lib->vba.BlendingAndTiming, mode_lib->vba.DPPPerPlane, v->BytePerPixelDETY, v->BytePerPixelDETC, v->DSTXAfterScaler, v->DSTYAfterScaler, - mode_lib->vba.WritebackEnable, - mode_lib->vba.WritebackPixelFormat, - mode_lib->vba.WritebackDestinationWidth, - mode_lib->vba.WritebackDestinationHeight, - mode_lib->vba.WritebackSourceHeight, v->UnboundedRequestEnabled, v->CompressedBufferSizeInkByte,/* Output */ - &v->Watermark, &v->dummy_vars.DISPCLKDPPCLKDCFCLKDeepSleepPrefetchParametersWatermarksAndPerformanceCalculation.dummy_dramchange_support, v->MaxActiveDRAMClockChangeLatencySupported, v->SubViewportLinesNeededInMALL, &v->dummy_vars.DISPCLKDPPCLKDCFCLKDeepSleepPrefetchParametersWatermarksAndPerformanceCalculation.dummy_fclkchange_support, &v->MinActiveFCLKChangeLatencySupported, - &v->dummy_vars.DISPCLKDPPCLKDCFCLKDeepSleepPrefetchParametersWatermarksAndPerformanceCalculation.dummy_USRRetrainingSupport, - mode_lib->vba.ActiveDRAMClockChangeLatencyMargin); + &v->dummy_vars.DISPCLKDPPCLKDCFCLKDeepSleepPrefetchParametersWatermarksAndPerformanceCalculation.dummy_USRRetrainingSupport);/* DCN32 has a new struct Watermarks (typedef) which is used to store * calculated WM values. Copy over values from struct to vba varaibles
@@ -3566,66 +3534,34 @@ void dml32_ModeSupportAndSystemConfigurationFull(struct display_mode_lib *mode_l {dml32_CalculateWatermarksMALLUseAndDRAMSpeedChangeSupport ( - &v->dummy_vars.dml32_CalculateWatermarksMALLUseAndDRAMSpeedChangeSupport, - mode_lib->vba.USRRetrainingRequiredFinal, - mode_lib->vba.UsesMALLForPStateChange, + v, mode_lib->vba.PrefetchModePerState[i][j], - mode_lib->vba.NumberOfActiveSurfaces, - mode_lib->vba.MaxLineBufferLines, - mode_lib->vba.LineBufferSizeFinal, - mode_lib->vba.WritebackInterfaceBufferSize, mode_lib->vba.DCFCLKState[i][j], mode_lib->vba.ReturnBWPerState[i][j], - mode_lib->vba.SynchronizeTimingsFinal, - mode_lib->vba.SynchronizeDRRDisplaysForUCLKPStateChangeFinal, - mode_lib->vba.DRRDisplay, - mode_lib->vba.dpte_group_bytes, - mode_lib->vba.meta_row_height, - mode_lib->vba.meta_row_height_chroma, v->dummy_vars.dml32_ModeSupportAndSystemConfigurationFull.mSOCParameters, - mode_lib->vba.WritebackChunkSize, mode_lib->vba.SOCCLKPerState[i], mode_lib->vba.ProjectedDCFCLKDeepSleep[i][j], mode_lib->vba.DETBufferSizeYThisState, mode_lib->vba.DETBufferSizeCThisState, mode_lib->vba.SwathHeightYThisState, mode_lib->vba.SwathHeightCThisState, - mode_lib->vba.LBBitPerPixel, mode_lib->vba.SwathWidthYThisState,// 24 mode_lib->vba.SwathWidthCThisState, - mode_lib->vba.HRatio, - mode_lib->vba.HRatioChroma, - mode_lib->vba.vtaps, - mode_lib->vba.VTAPsChroma, - mode_lib->vba.VRatio, - mode_lib->vba.VRatioChroma, - mode_lib->vba.HTotal, - mode_lib->vba.VTotal, - mode_lib->vba.VActive, - mode_lib->vba.PixelClock, - mode_lib->vba.BlendingAndTiming, mode_lib->vba.NoOfDPPThisState, mode_lib->vba.BytePerPixelInDETY, mode_lib->vba.BytePerPixelInDETC, v->dummy_vars.dml32_ModeSupportAndSystemConfigurationFull.DSTXAfterScaler, v->dummy_vars.dml32_ModeSupportAndSystemConfigurationFull.DSTYAfterScaler, - mode_lib->vba.WritebackEnable, - mode_lib->vba.WritebackPixelFormat, - mode_lib->vba.WritebackDestinationWidth, - mode_lib->vba.WritebackDestinationHeight, - mode_lib->vba.WritebackSourceHeight, mode_lib->vba.UnboundedRequestEnabledThisState, mode_lib->vba.CompressedBufferSizeInkByteThisState,/* Output */ - &mode_lib->vba.Watermark,// Store the values in vba &mode_lib->vba.DRAMClockChangeSupport[i][j], &v->dummy_vars.dml32_ModeSupportAndSystemConfigurationFull.dummy_single2[0 ],// double *MaxActiveDRAMClockChangeLatencySupported &v->dummy_vars.dml32_ModeSupportAndSystemConfigurationFull.dummy_integer[0 ],// Long SubViewportLinesNeededInMALL[] &mode_lib->vba.FCLKChangeSupport[i][j], &v->dummy_vars.dml32_ModeSupportAndSystemConfigurationFull.dummy_single2[1 ],// double *MinActiveFCLKChangeLatencySupported - &mode_lib->vba.USRRetrainingSupport[i][j], - mode_lib->vba.ActiveDRAMClockChangeLatencyMargin); + &mode_lib->vba.USRRetrainingSupport[i][j]); } } }// End of Prefetch Check
drivers/gpu/drm/amd/display/dc/dml/dcn32/display_mode_vba_util_32.c +114 -145
446 lines not on this page · raw message
drivers/gpu/drm/amd/display/dc/dml/dcn32/display_mode_vba_util_32.h +2 -34
@@ -811,66 +811,34 @@ void dml32_CalculateFlipSchedule( bool *ImmediateFlipSupportedForPipe);void dml32_CalculateWatermarksMALLUseAndDRAMSpeedChangeSupport ( -struct dml32_CalculateWatermarksMALLUseAndDRAMSpeedChangeSupport *st_vars, -bool USRRetrainingRequiredFinal, -enum dm_use_mall_for_pstate_change_mode UseMALLForPStateChange[], +struct vba_vars_st *v,unsigned int PrefetchMode, -unsigned int NumberOfActiveSurfaces, -unsigned int MaxLineBufferLines, -unsigned int LineBufferSize, -unsigned int WritebackInterfaceBufferSize,double DCFCLK ,double ReturnBW, -bool SynchronizeTimingsFinal, -bool SynchronizeDRRDisplaysForUCLKPStateChangeFinal, -bool DRRDisplay[], -unsigned int dpte_group_bytes[], -unsigned int meta_row_height[], -unsigned int meta_row_height_chroma[],SOCParametersList mmSOCParameters, -unsigned int WritebackChunkSize,double SOCCLK ,double DCFClkDeepSleep,unsigned int DETBufferSizeY[],unsigned int DETBufferSizeC[],unsigned int SwathHeightY[],unsigned int SwathHeightC[], -unsigned int LBBitPerPixel[],double SwathWidthY[],double SwathWidthC[], -double HRatio[], -double HRatioChroma[], -unsigned int VTaps[], -unsigned int VTapsChroma[], -double VRatio[], -double VRatioChroma[], -unsigned int HTotal[], -unsigned int VTotal[], -unsigned int VActive[], -double PixelClock[], -unsigned int BlendingAndTiming[],unsigned int DPPPerSurface[],double BytePerPixelDETY[],double BytePerPixelDETC[],double DSTXAfterScaler[],double DSTYAfterScaler[], -bool WritebackEnable[], -enum source_format_class WritebackPixelFormat[], -double WritebackDestinationWidth[], -double WritebackDestinationHeight[], -double WritebackSourceHeight[],bool UnboundedRequestEnabled,unsigned int CompressedBufferSizeInkByte,/* Output */ -Watermarks *Watermark,enum clock_change_support *DRAMClockChangeSupport,double MaxActiveDRAMClockChangeLatencySupported[],unsigned int SubViewportLinesNeededInMALL[],enum dm_fclock_change_support *FCLKChangeSupport,double *MinActiveFCLKChangeLatencySupported, -bool *USRRetrainingSupport, -double ActiveDRAMClockChangeLatencyMargin[]); +bool *USRRetrainingSupport); double dml32_CalculateWriteBackDISPCLK( enum source_format_class WritebackPixelFormat,
Signature · 1 line
2.37.2
Hi AMD folks,
Top posting because it might not have been obvious but I was looking for your feedback on this message (which can be viewed on lore.kernel.org if you do not have the original [1]) so that we can try to get this fixed in some way for 6.0/6.1. If my approach is not welcome, please consider suggesting another one or looking to see if this is something you all could look into.
[1]: https://lore.kernel.org/[email protected]/
Cheers,
Nathan
Quoted 134 lines · Nathan Chancellor
Hi Arnd,
Doubling back around to this now since I think this is the only thing breaking
x86_64allmodconfig with clang 11 through 15.Quoted 74 lines · Arnd Bergmann
Quoted 11 lines · Nathan Chancellor
Quoted 9 lines · Arnd Bergmann
On Fri, Aug 5, 2022 at 5:32 PM Harry Wentland <[email protected]> wrote:
While splitting out sub-functions can help reduce the maximum stack usage, it seems that in this case it makes the actual problem worse:
I see 2168 bytes for the combineddml32_ModeSupportAndSystemConfigurationFull(), but markingmode_support_configuration()as noinline gives me 1992 bytes
for the outer function plus 384 bytes for the inner one. So it does avoid the warning (barely), but not the problem that the warning tries to point out.Quoted 2 lines
I haven't had a chance to take a look at splitting things up yet, would you recommend a different approach?
Splitting up large functions can help when you have large local variables that are used in different parts of the function, and the split gets the compiler to reuse stack locations.
I think in this particular function, the problem isn't actually local variables but either pushing variables on the stack for argument passing,
or something that causes the compiler to run out of registers so it has to spill registers to the stack.In either case, one has to actually look at the generated output and then try to rearrange the codes so this does not happen.
One thing to try would be to condense a function call like
dml32_CalculateWatermarksMALLUseAndDRAMSpeedChangeSupport(
&v->dummy_vars.dml32_CalculateWatermarksMALLUseAndDRAMSpeedChangeSupport,mode_lib->vba.USRRetrainingRequiredFinal, mode_lib->vba.UsesMALLForPStateChange,mode_lib->vba.PrefetchModePerState[mode_lib->vba.VoltageLevel][mode_lib->vba.maxMpcComb],
mode_lib->vba.NumberOfActiveSurfaces, mode_lib->vba.MaxLineBufferLines, mode_lib->vba.LineBufferSizeFinal, mode_lib->vba.WritebackInterfaceBufferSize, mode_lib->vba.DCFCLK, mode_lib->vba.ReturnBW, mode_lib->vba.SynchronizeTimingsFinal,
mode_lib->vba.SynchronizeDRRDisplaysForUCLKPStateChangeFinal,mode_lib->vba.DRRDisplay, v->dpte_group_bytes, v->meta_row_height, v->meta_row_height_chroma,
v->dummy_vars.DISPCLKDPPCLKDCFCLKDeepSleepPrefetchParametersWatermarksAndPerformanceCalculation.mmSOCParameters,mode_lib->vba.WritebackChunkSize, mode_lib->vba.SOCCLK, v->DCFCLKDeepSleep, mode_lib->vba.DETBufferSizeY, mode_lib->vba.DETBufferSizeC, mode_lib->vba.SwathHeightY, mode_lib->vba.SwathHeightC, mode_lib->vba.LBBitPerPixel, v->SwathWidthY, v->SwathWidthC, mode_lib->vba.HRatio, mode_lib->vba.HRatioChroma, mode_lib->vba.vtaps, mode_lib->vba.VTAPsChroma, mode_lib->vba.VRatio, mode_lib->vba.VRatioChroma, mode_lib->vba.HTotal, mode_lib->vba.VTotal, mode_lib->vba.VActive, mode_lib->vba.PixelClock, mode_lib->vba.BlendingAndTiming, .... /* more arguments */);Quoted 2 lines
into calling conventions that take a pointer to '
mode_lib->vba' and another one to 'v', so these are no longer passed on the stack individually.So I took a whack at reducing this function's number of parameters and ended up with the attached patch. I basically just removed any parameters that were identical between the two call sites and access them through the vba pointer, as you suggested.
AMD folks, is this an acceptable approach? It didn't take a trivial amount of time so I want to make sure this is okay before I do it to more functions/files.
Due to the potential size of these changes, I am a little weary of them going into 6.0; even though they should be a simple search and replace for the most part, it might be nice for them to have some decent soak time in -next. One solution would be to raise the warning limit for these files on 6.0 so that allmodconfig does not ship broken then reduce the limit for 6.1 once these patches have been applied.
Additionally, I took a look at the stack usage across all compilers that the kernel supports and I thought it was kind of interesting that the usage really jumps from GCC 7 to 8, which I am guessing is a result of commit
e8a170ff9a35("drm/amdgpu: enable -msse2 for GCC 7.1+ users"). GCC 8 allmodconfig actually errors now too:https://lore.kernel.org/[email protected]/
|-----------------------------------------------|-----------------------------------------------|-----------------------------------------------| | dml30_ModeSupportAndSystemConfigurationFull() | dml31_ModeSupportAndSystemConfigurationFull() | dml32_ModeSupportAndSystemConfigurationFull() | |---------|-----------------------------------------------|-----------------------------------------------|-----------------------------------------------| | GCC 5 | 1056 bytes | 656 bytes | 1040 bytes | | GCC 6 | 1024 bytes | 672 bytes | 1056 bytes | | GCC 7 | 1040 bytes | 664 bytes | 1056 bytes | | GCC 8 | 1760 bytes | 1608 bytes | 2144 bytes | | GCC 9 | 1664 bytes | 1392 bytes | 1960 bytes | | GCC 10 | 1648 bytes | 1368 bytes | 1952 bytes | | GCC 11 | 1680 bytes | 1400 bytes | 1952 bytes | | GCC 12 | 1680 bytes | 1400 bytes | 1984 bytes | |---------|-----------------------------------------------|-----------------------------------------------|-----------------------------------------------| | LLVM 11 | 2104 bytes | 2056 bytes | 2120 bytes | | LLVM 12 | 2152 bytes | 2200 bytes | 2152 bytes | | LLVM 13 | 2216 bytes | 2248 bytes | 2168 bytes | | LLVM 14 | 2168 bytes | 2184 bytes | 2160 bytes | | LLVM 15 | 2216 bytes | 2184 bytes | 2160 bytes | | LLVM 16 | 2232 bytes | 2216 bytes | 2176 bytes | |---------|-----------------------------------------------|-----------------------------------------------|-----------------------------------------------|With the patch I have attached,
dml32_ModeSupportAndSystemConfigurationFull()drops from 2176 to 1944 for LLVM 16, which is obviously still not great but it at least avoids the warning.Cheers,
Nathan
Quoted 9 lines · Nathan Chancellor
Hi AMD folks,
Top posting because it might not have been obvious but I was looking for your feedback on this message (which can be viewed on lore.kernel.org if you do not have the original [1]) so that we can try to get this fixed in some way for 6.0/6.1. If my approach is not welcome, please consider suggesting another one or looking to see if this is something you all could look into.
The patch looks good to me. I was hoping Harry or Rodrigo could comment more since they are more familiar with this code and trying to keep it in sync with what we get from the hardware teams.
Alex
Quoted 141 lines
[1]: https://lore.kernel.org/[email protected]/
Cheers,
NathanQuoted 134 lines · Nathan Chancellor
Hi Arnd,
Doubling back around to this now since I think this is the only thing breaking
x86_64allmodconfig with clang 11 through 15.Quoted 74 lines · Arnd Bergmann
Quoted 11 lines · Nathan Chancellor
Quoted 9 lines · Arnd Bergmann
On Fri, Aug 5, 2022 at 5:32 PM Harry Wentland <[email protected]> wrote:
While splitting out sub-functions can help reduce the maximum stack usage, it seems that in this case it makes the actual problem worse:
I see 2168 bytes for the combineddml32_ModeSupportAndSystemConfigurationFull(), but markingmode_support_configuration()as noinline gives me 1992 bytes
for the outer function plus 384 bytes for the inner one. So it does avoid the warning (barely), but not the problem that the warning tries to point out.Quoted 2 lines
I haven't had a chance to take a look at splitting things up yet, would you recommend a different approach?
Splitting up large functions can help when you have large local variables that are used in different parts of the function, and the split gets the compiler to reuse stack locations.
I think in this particular function, the problem isn't actually local variables but either pushing variables on the stack for argument passing,
or something that causes the compiler to run out of registers so it has to spill registers to the stack.In either case, one has to actually look at the generated output and then try to rearrange the codes so this does not happen.
One thing to try would be to condense a function call like
dml32_CalculateWatermarksMALLUseAndDRAMSpeedChangeSupport(
&v->dummy_vars.dml32_CalculateWatermarksMALLUseAndDRAMSpeedChangeSupport,mode_lib->vba.USRRetrainingRequiredFinal, mode_lib->vba.UsesMALLForPStateChange,mode_lib->vba.PrefetchModePerState[mode_lib->vba.VoltageLevel][mode_lib->vba.maxMpcComb],
mode_lib->vba.NumberOfActiveSurfaces, mode_lib->vba.MaxLineBufferLines, mode_lib->vba.LineBufferSizeFinal, mode_lib->vba.WritebackInterfaceBufferSize, mode_lib->vba.DCFCLK, mode_lib->vba.ReturnBW, mode_lib->vba.SynchronizeTimingsFinal,
mode_lib->vba.SynchronizeDRRDisplaysForUCLKPStateChangeFinal,mode_lib->vba.DRRDisplay, v->dpte_group_bytes, v->meta_row_height, v->meta_row_height_chroma,
v->dummy_vars.DISPCLKDPPCLKDCFCLKDeepSleepPrefetchParametersWatermarksAndPerformanceCalculation.mmSOCParameters,mode_lib->vba.WritebackChunkSize, mode_lib->vba.SOCCLK, v->DCFCLKDeepSleep, mode_lib->vba.DETBufferSizeY, mode_lib->vba.DETBufferSizeC, mode_lib->vba.SwathHeightY, mode_lib->vba.SwathHeightC, mode_lib->vba.LBBitPerPixel, v->SwathWidthY, v->SwathWidthC, mode_lib->vba.HRatio, mode_lib->vba.HRatioChroma, mode_lib->vba.vtaps, mode_lib->vba.VTAPsChroma, mode_lib->vba.VRatio, mode_lib->vba.VRatioChroma, mode_lib->vba.HTotal, mode_lib->vba.VTotal, mode_lib->vba.VActive, mode_lib->vba.PixelClock, mode_lib->vba.BlendingAndTiming, .... /* more arguments */);Quoted 2 lines
into calling conventions that take a pointer to '
mode_lib->vba' and another one to 'v', so these are no longer passed on the stack individually.So I took a whack at reducing this function's number of parameters and ended up with the attached patch. I basically just removed any parameters that were identical between the two call sites and access them through the vba pointer, as you suggested.
AMD folks, is this an acceptable approach? It didn't take a trivial amount of time so I want to make sure this is okay before I do it to more functions/files.
Due to the potential size of these changes, I am a little weary of them going into 6.0; even though they should be a simple search and replace for the most part, it might be nice for them to have some decent soak time in -next. One solution would be to raise the warning limit for these files on 6.0 so that allmodconfig does not ship broken then reduce the limit for 6.1 once these patches have been applied.
Additionally, I took a look at the stack usage across all compilers that the kernel supports and I thought it was kind of interesting that the usage really jumps from GCC 7 to 8, which I am guessing is a result of commit
e8a170ff9a35("drm/amdgpu: enable -msse2 for GCC 7.1+ users"). GCC 8 allmodconfig actually errors now too:https://lore.kernel.org/[email protected]/
|-----------------------------------------------|-----------------------------------------------|-----------------------------------------------| | dml30_ModeSupportAndSystemConfigurationFull() | dml31_ModeSupportAndSystemConfigurationFull() | dml32_ModeSupportAndSystemConfigurationFull() | |---------|-----------------------------------------------|-----------------------------------------------|-----------------------------------------------| | GCC 5 | 1056 bytes | 656 bytes | 1040 bytes | | GCC 6 | 1024 bytes | 672 bytes | 1056 bytes | | GCC 7 | 1040 bytes | 664 bytes | 1056 bytes | | GCC 8 | 1760 bytes | 1608 bytes | 2144 bytes | | GCC 9 | 1664 bytes | 1392 bytes | 1960 bytes | | GCC 10 | 1648 bytes | 1368 bytes | 1952 bytes | | GCC 11 | 1680 bytes | 1400 bytes | 1952 bytes | | GCC 12 | 1680 bytes | 1400 bytes | 1984 bytes | |---------|-----------------------------------------------|-----------------------------------------------|-----------------------------------------------| | LLVM 11 | 2104 bytes | 2056 bytes | 2120 bytes | | LLVM 12 | 2152 bytes | 2200 bytes | 2152 bytes | | LLVM 13 | 2216 bytes | 2248 bytes | 2168 bytes | | LLVM 14 | 2168 bytes | 2184 bytes | 2160 bytes | | LLVM 15 | 2216 bytes | 2184 bytes | 2160 bytes | | LLVM 16 | 2232 bytes | 2216 bytes | 2176 bytes | |---------|-----------------------------------------------|-----------------------------------------------|-----------------------------------------------|With the patch I have attached,
dml32_ModeSupportAndSystemConfigurationFull()drops from 2176 to 1944 for LLVM 16, which is obviously still not great but it at least avoids the warning.Cheers,
Nathan
Quoted 11 lines · Alex Deucher
Quoted 9 lines · Nathan Chancellor
Hi AMD folks,
Top posting because it might not have been obvious but I was looking for your feedback on this message (which can be viewed on lore.kernel.org if you do not have the original [1]) so that we can try to get this fixed in some way for 6.0/6.1. If my approach is not welcome, please consider suggesting another one or looking to see if this is something you all could look into.
Quoted 3 lines
The patch looks good to me. I was hoping Harry or Rodrigo could comment more since they are more familiar with this code and trying to keep it in sync with what we get from the hardware teams.
Thanks a lot for the input! That patch was broken but I have polished it and a few other patches up and sent them along for review:
https://lore.kernel.org/[email protected]/
I did not CC everyone from this thread but it is on lore if others want to comment on it. Hopefully we can get this all sorted out for 6.0 final.
Cheers,
Nathan
Quoted 140 lines
Quoted 140 lines
[1]: https://lore.kernel.org/[email protected]/
Cheers,
NathanQuoted 134 lines · Nathan Chancellor
Hi Arnd,
Doubling back around to this now since I think this is the only thing breaking
x86_64allmodconfig with clang 11 through 15.Quoted 74 lines · Arnd Bergmann
Quoted 11 lines · Nathan Chancellor
Quoted 9 lines · Arnd Bergmann
On Fri, Aug 5, 2022 at 5:32 PM Harry Wentland <[email protected]> wrote:
While splitting out sub-functions can help reduce the maximum stack usage, it seems that in this case it makes the actual problem worse:
I see 2168 bytes for the combineddml32_ModeSupportAndSystemConfigurationFull(), but markingmode_support_configuration()as noinline gives me 1992 bytes
for the outer function plus 384 bytes for the inner one. So it does avoid the warning (barely), but not the problem that the warning tries to point out.Quoted 2 lines
I haven't had a chance to take a look at splitting things up yet, would you recommend a different approach?
Splitting up large functions can help when you have large local variables that are used in different parts of the function, and the split gets the compiler to reuse stack locations.
I think in this particular function, the problem isn't actually local variables but either pushing variables on the stack for argument passing,
or something that causes the compiler to run out of registers so it has to spill registers to the stack.In either case, one has to actually look at the generated output and then try to rearrange the codes so this does not happen.
One thing to try would be to condense a function call like
dml32_CalculateWatermarksMALLUseAndDRAMSpeedChangeSupport(
&v->dummy_vars.dml32_CalculateWatermarksMALLUseAndDRAMSpeedChangeSupport,mode_lib->vba.USRRetrainingRequiredFinal, mode_lib->vba.UsesMALLForPStateChange,mode_lib->vba.PrefetchModePerState[mode_lib->vba.VoltageLevel][mode_lib->vba.maxMpcComb],
mode_lib->vba.NumberOfActiveSurfaces, mode_lib->vba.MaxLineBufferLines, mode_lib->vba.LineBufferSizeFinal, mode_lib->vba.WritebackInterfaceBufferSize, mode_lib->vba.DCFCLK, mode_lib->vba.ReturnBW, mode_lib->vba.SynchronizeTimingsFinal,
mode_lib->vba.SynchronizeDRRDisplaysForUCLKPStateChangeFinal,mode_lib->vba.DRRDisplay, v->dpte_group_bytes, v->meta_row_height, v->meta_row_height_chroma,
v->dummy_vars.DISPCLKDPPCLKDCFCLKDeepSleepPrefetchParametersWatermarksAndPerformanceCalculation.mmSOCParameters,mode_lib->vba.WritebackChunkSize, mode_lib->vba.SOCCLK, v->DCFCLKDeepSleep, mode_lib->vba.DETBufferSizeY, mode_lib->vba.DETBufferSizeC, mode_lib->vba.SwathHeightY, mode_lib->vba.SwathHeightC, mode_lib->vba.LBBitPerPixel, v->SwathWidthY, v->SwathWidthC, mode_lib->vba.HRatio, mode_lib->vba.HRatioChroma, mode_lib->vba.vtaps, mode_lib->vba.VTAPsChroma, mode_lib->vba.VRatio, mode_lib->vba.VRatioChroma, mode_lib->vba.HTotal, mode_lib->vba.VTotal, mode_lib->vba.VActive, mode_lib->vba.PixelClock, mode_lib->vba.BlendingAndTiming, .... /* more arguments */);Quoted 2 lines
into calling conventions that take a pointer to '
mode_lib->vba' and another one to 'v', so these are no longer passed on the stack individually.So I took a whack at reducing this function's number of parameters and ended up with the attached patch. I basically just removed any parameters that were identical between the two call sites and access them through the vba pointer, as you suggested.
AMD folks, is this an acceptable approach? It didn't take a trivial amount of time so I want to make sure this is okay before I do it to more functions/files.
Due to the potential size of these changes, I am a little weary of them going into 6.0; even though they should be a simple search and replace for the most part, it might be nice for them to have some decent soak time in -next. One solution would be to raise the warning limit for these files on 6.0 so that allmodconfig does not ship broken then reduce the limit for 6.1 once these patches have been applied.
Additionally, I took a look at the stack usage across all compilers that the kernel supports and I thought it was kind of interesting that the usage really jumps from GCC 7 to 8, which I am guessing is a result of commit
e8a170ff9a35("drm/amdgpu: enable -msse2 for GCC 7.1+ users"). GCC 8 allmodconfig actually errors now too:https://lore.kernel.org/[email protected]/
|-----------------------------------------------|-----------------------------------------------|-----------------------------------------------| | dml30_ModeSupportAndSystemConfigurationFull() | dml31_ModeSupportAndSystemConfigurationFull() | dml32_ModeSupportAndSystemConfigurationFull() | |---------|-----------------------------------------------|-----------------------------------------------|-----------------------------------------------| | GCC 5 | 1056 bytes | 656 bytes | 1040 bytes | | GCC 6 | 1024 bytes | 672 bytes | 1056 bytes | | GCC 7 | 1040 bytes | 664 bytes | 1056 bytes | | GCC 8 | 1760 bytes | 1608 bytes | 2144 bytes | | GCC 9 | 1664 bytes | 1392 bytes | 1960 bytes | | GCC 10 | 1648 bytes | 1368 bytes | 1952 bytes | | GCC 11 | 1680 bytes | 1400 bytes | 1952 bytes | | GCC 12 | 1680 bytes | 1400 bytes | 1984 bytes | |---------|-----------------------------------------------|-----------------------------------------------|-----------------------------------------------| | LLVM 11 | 2104 bytes | 2056 bytes | 2120 bytes | | LLVM 12 | 2152 bytes | 2200 bytes | 2152 bytes | | LLVM 13 | 2216 bytes | 2248 bytes | 2168 bytes | | LLVM 14 | 2168 bytes | 2184 bytes | 2160 bytes | | LLVM 15 | 2216 bytes | 2184 bytes | 2160 bytes | | LLVM 16 | 2232 bytes | 2216 bytes | 2176 bytes | |---------|-----------------------------------------------|-----------------------------------------------|-----------------------------------------------|With the patch I have attached,
dml32_ModeSupportAndSystemConfigurationFull()drops from 2176 to 1944 for LLVM 16, which is obviously still not great but it at least avoids the warning.Cheers,
Nathan