| Message ID | 20231122030621.3759313-4-samuel.holland@sifive.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | riscv: Add kernel-mode FPU support for amdgpu | expand |
> - select DRM_AMD_DC_FP if (X86 || LOONGARCH || (PPC64 && ALTIVEC) || (ARM64 && KERNEL_MODE_NEON && !CC_IS_CLANG)) > + select DRM_AMD_DC_FP if ARM64 && KERNEL_MODE_NEON && !CC_IS_CLANG > + select DRM_AMD_DC_FP if PPC64 && ALTIVEC > + select DRM_AMD_DC_FP if RISCV && FPU > + select DRM_AMD_DC_FP if LOONGARCH || X86 This really is a mess. Can you add a ARCH_HAS_KERNEL_FPU_SUPPORT symbol that all architetures that have it select instead, and them make DRM_AMD_DC_FP depend on it? > -#if defined(CONFIG_X86) || defined(CONFIG_LOONGARCH) > +#if defined(CONFIG_X86) || defined(CONFIG_LOONGARCH) || defined(CONFIG_RISCV) > kernel_fpu_begin(); > #elif defined(CONFIG_PPC64) > if (cpu_has_feature(CPU_FTR_VSX_COMP)) > @@ -122,7 +124,7 @@ void dc_fpu_end(const char *function_name, const int line) > > depth = __this_cpu_dec_return(fpu_recursion_depth); > if (depth == 0) { > -#if defined(CONFIG_X86) || defined(CONFIG_LOONGARCH) > +#if defined(CONFIG_X86) || defined(CONFIG_LOONGARCH) || defined(CONFIG_RISCV) > kernel_fpu_end(); > #elif defined(CONFIG_PPC64) > if (cpu_has_feature(CPU_FTR_VSX_COMP)) And then this mess can go away. We'll need to decide if we want to cover all the in-kernel vector support as part of it, which would seem reasonable to me, or have a separate generic kernel_vector_begin with it's own option. > diff --git a/drivers/gpu/drm/amd/display/dc/dml/Makefile b/drivers/gpu/drm/amd/display/dc/dml/Makefile > index ea7d60f9a9b4..5c8f840ef323 100644 > --- a/drivers/gpu/drm/amd/display/dc/dml/Makefile > +++ b/drivers/gpu/drm/amd/display/dc/dml/Makefile > @@ -43,6 +43,12 @@ dml_ccflags := -mfpu=64 > dml_rcflags := -msoft-float > endif > > +ifdef CONFIG_RISCV > +include $(srctree)/arch/riscv/Makefile.isa > +# Remove V from the ISA string, like in arch/riscv/Makefile, but keep F and D. > +dml_ccflags := -march=$(shell echo $(riscv-march-y) | sed -E 's/(rv32ima|rv64ima)([^v_]*)v?/\1\2/') > +endif > + > ifdef CONFIG_CC_IS_GCC > ifneq ($(call gcc-min-version, 70100),y) > IS_OLD_GCC = 1 And this is again not really something we should be doing. Instead we need a generic way in Kconfig to enable FPU support for an object file or set of, that the arch support can hook into. Btw, I'm also really worried about folks using the FPU instructions outside the kernel_fpu_begin/end windows in general (not directly related to the RISC-V support). Can we have objecttool checks for that similar to only allowing the unsafe uaccess in the uaccess begin/end pairs?
On Tue, Nov 21, 2023 at 07:05:15PM -0800, Samuel Holland wrote: > RISC-V uses kernel_fpu_begin()/kernel_fpu_end() like several other > architectures. Enabling hardware FP requires overriding the ISA string > for the relevant compilation units. Ah yes, bringing the joy of frame-larger-than warnings to RISC-V: ../drivers/gpu/drm/amd/amdgpu/../display/dc/dml/dcn32/display_mode_vba_32.c:58:13: warning: stack frame size (2416) exceeds limit (2048) in 'DISPCLKDPPCLKDCFCLKDeepSleepPrefetchParametersWatermarksAndPerformanceCalculation' [-Wframe-larger-than] Nathan, have you given up on these being sorted out? Also, what on earth is that function name, it exceeds 80 characters before even considering anything else? Actually, I don't think I want to know.
On Thu, Nov 23, 2023 at 02:23:01PM +0000, Conor Dooley wrote: > On Tue, Nov 21, 2023 at 07:05:15PM -0800, Samuel Holland wrote: > > RISC-V uses kernel_fpu_begin()/kernel_fpu_end() like several other > > architectures. Enabling hardware FP requires overriding the ISA string > > for the relevant compilation units. > > Ah yes, bringing the joy of frame-larger-than warnings to RISC-V: > ../drivers/gpu/drm/amd/amdgpu/../display/dc/dml/dcn32/display_mode_vba_32.c:58:13: warning: stack frame size (2416) exceeds limit (2048) in 'DISPCLKDPPCLKDCFCLKDeepSleepPrefetchParametersWatermarksAndPerformanceCalculation' [-Wframe-larger-than] :( > Nathan, have you given up on these being sorted out? Does your configuration have KASAN (I don't think RISC-V supports KCSAN)? It is possible that dml/dcn32 needs something similar to commit 6740ec97bcdb ("drm/amd/display: Increase frame warning limit with KASAN or KCSAN in dml2")? I am not really interested in playing whack-a-mole with these warnings like I have done in the past for the reasons I outlined here: https://lore.kernel.org/20231019205117.GA839902@dev-arch.thelio-3990X/ > Also, what on earth is that function name, it exceeds 80 characters > before even considering anything else? Actually, I don't think I want > to know. Welcome to "gcc-parsable HW gospel, coming straight from HW engineers" :) Cheers, Nathan
On Wed, Nov 29, 2023 at 05:42:24PM -0700, Nathan Chancellor wrote: > On Thu, Nov 23, 2023 at 02:23:01PM +0000, Conor Dooley wrote: > > On Tue, Nov 21, 2023 at 07:05:15PM -0800, Samuel Holland wrote: > > > RISC-V uses kernel_fpu_begin()/kernel_fpu_end() like several other > > > architectures. Enabling hardware FP requires overriding the ISA string > > > for the relevant compilation units. > > > > Ah yes, bringing the joy of frame-larger-than warnings to RISC-V: > > ../drivers/gpu/drm/amd/amdgpu/../display/dc/dml/dcn32/display_mode_vba_32.c:58:13: warning: stack frame size (2416) exceeds limit (2048) in 'DISPCLKDPPCLKDCFCLKDeepSleepPrefetchParametersWatermarksAndPerformanceCalculation' [-Wframe-larger-than] > > :( > > > Nathan, have you given up on these being sorted out? > > Does your configuration have KASAN (I don't think RISC-V supports > KCSAN)? It is possible that dml/dcn32 needs something similar to commit > 6740ec97bcdb ("drm/amd/display: Increase frame warning limit with KASAN > or KCSAN in dml2")? It's from allmodconfig, so yes, I think so.
Hi Christoph, On 2023-11-22 2:40 AM, Christoph Hellwig wrote: >> - select DRM_AMD_DC_FP if (X86 || LOONGARCH || (PPC64 && ALTIVEC) || (ARM64 && KERNEL_MODE_NEON && !CC_IS_CLANG)) >> + select DRM_AMD_DC_FP if ARM64 && KERNEL_MODE_NEON && !CC_IS_CLANG >> + select DRM_AMD_DC_FP if PPC64 && ALTIVEC >> + select DRM_AMD_DC_FP if RISCV && FPU >> + select DRM_AMD_DC_FP if LOONGARCH || X86 > > This really is a mess. Can you add a ARCH_HAS_KERNEL_FPU_SUPPORT > symbol that all architetures that have it select instead, and them > make DRM_AMD_DC_FP depend on it? Yes, I have done this for v2, which I will send shortly. >> -#if defined(CONFIG_X86) || defined(CONFIG_LOONGARCH) >> +#if defined(CONFIG_X86) || defined(CONFIG_LOONGARCH) || defined(CONFIG_RISCV) >> kernel_fpu_begin(); >> #elif defined(CONFIG_PPC64) >> if (cpu_has_feature(CPU_FTR_VSX_COMP)) >> @@ -122,7 +124,7 @@ void dc_fpu_end(const char *function_name, const int line) >> >> depth = __this_cpu_dec_return(fpu_recursion_depth); >> if (depth == 0) { >> -#if defined(CONFIG_X86) || defined(CONFIG_LOONGARCH) >> +#if defined(CONFIG_X86) || defined(CONFIG_LOONGARCH) || defined(CONFIG_RISCV) >> kernel_fpu_end(); >> #elif defined(CONFIG_PPC64) >> if (cpu_has_feature(CPU_FTR_VSX_COMP)) > > And then this mess can go away. We'll need to decide if we want to > cover all the in-kernel vector support as part of it, which would > seem reasonable to me, or have a separate generic kernel_vector_begin > with it's own option. I think we may want to keep vector separate for performance on architectures with separate FP and vector register files. For now, I have limited my changes to FPU support only, which means I have removed VSX/Altivec from here; the AMDGPU code doesn't need Altivec anyway. >> diff --git a/drivers/gpu/drm/amd/display/dc/dml/Makefile b/drivers/gpu/drm/amd/display/dc/dml/Makefile >> index ea7d60f9a9b4..5c8f840ef323 100644 >> --- a/drivers/gpu/drm/amd/display/dc/dml/Makefile >> +++ b/drivers/gpu/drm/amd/display/dc/dml/Makefile >> @@ -43,6 +43,12 @@ dml_ccflags := -mfpu=64 >> dml_rcflags := -msoft-float >> endif >> >> +ifdef CONFIG_RISCV >> +include $(srctree)/arch/riscv/Makefile.isa >> +# Remove V from the ISA string, like in arch/riscv/Makefile, but keep F and D. >> +dml_ccflags := -march=$(shell echo $(riscv-march-y) | sed -E 's/(rv32ima|rv64ima)([^v_]*)v?/\1\2/') >> +endif >> + >> ifdef CONFIG_CC_IS_GCC >> ifneq ($(call gcc-min-version, 70100),y) >> IS_OLD_GCC = 1 > > And this is again not really something we should be doing. > Instead we need a generic way in Kconfig to enable FPU support > for an object file or set of, that the arch support can hook > into. I've included this in v2 as well. > Btw, I'm also really worried about folks using the FPU instructions > outside the kernel_fpu_begin/end windows in general (not directly > related to the RISC-V support). Can we have objecttool checks > for that similar to only allowing the unsafe uaccess in the > uaccess begin/end pairs? ARM partially enforces this at compile time: it disallows calling kernel_neon_begin() inside a translation unit that has NEON enabled. That doesn't prevent the programmer from calling a FPU-enabled function from outside a begin/end section, but it does prevent the compiler from generating unexpected FPU usage behind your back. I implemented this same functionality for RISC-V. Actually tracking all possibly-FPU-tainted functions and their call sites is probably possible, but a much larger task. Regards, Samuel
Hi Nathan, On 2023-11-29 6:42 PM, Nathan Chancellor wrote: > On Thu, Nov 23, 2023 at 02:23:01PM +0000, Conor Dooley wrote: >> On Tue, Nov 21, 2023 at 07:05:15PM -0800, Samuel Holland wrote: >>> RISC-V uses kernel_fpu_begin()/kernel_fpu_end() like several other >>> architectures. Enabling hardware FP requires overriding the ISA string >>> for the relevant compilation units. >> >> Ah yes, bringing the joy of frame-larger-than warnings to RISC-V: >> ../drivers/gpu/drm/amd/amdgpu/../display/dc/dml/dcn32/display_mode_vba_32.c:58:13: warning: stack frame size (2416) exceeds limit (2048) in 'DISPCLKDPPCLKDCFCLKDeepSleepPrefetchParametersWatermarksAndPerformanceCalculation' [-Wframe-larger-than] > > :( > >> Nathan, have you given up on these being sorted out? > > Does your configuration have KASAN (I don't think RISC-V supports > KCSAN)? It is possible that dml/dcn32 needs something similar to commit > 6740ec97bcdb ("drm/amd/display: Increase frame warning limit with KASAN > or KCSAN in dml2")? > > I am not really interested in playing whack-a-mole with these warnings > like I have done in the past for the reasons I outlined here: > > https://lore.kernel.org/20231019205117.GA839902@dev-arch.thelio-3990X/ I also see one of these with clang 17 even with KASAN disabled: drivers/gpu/drm/amd/amdgpu/../display/dc/dml/dcn32/display_mode_vba_32.c:37:6: warning: stack frame size (2208) exceeds limit (2048) in 'dml32_recalculate' [-Wframe-larger-than] void dml32_recalculate(struct display_mode_lib *mode_lib) ^ 1532/2208 (69.38%) spills, 676/2208 (30.62%) variables So I'm in favor of just raising the limit for these files for clang, like you suggested in the linked thread. Regards, Samuel
On Fri, Dec 8, 2023, at 06:04, Samuel Holland wrote: > On 2023-11-29 6:42 PM, Nathan Chancellor wrote: >> On Thu, Nov 23, 2023 at 02:23:01PM +0000, Conor Dooley wrote: >>> On Tue, Nov 21, 2023 at 07:05:15PM -0800, Samuel Holland wrote: >>>> RISC-V uses kernel_fpu_begin()/kernel_fpu_end() like several other >>>> architectures. Enabling hardware FP requires overriding the ISA string >>>> for the relevant compilation units. >>> >>> Ah yes, bringing the joy of frame-larger-than warnings to RISC-V: >>> ../drivers/gpu/drm/amd/amdgpu/../display/dc/dml/dcn32/display_mode_vba_32.c:58:13: warning: stack frame size (2416) exceeds limit (2048) in 'DISPCLKDPPCLKDCFCLKDeepSleepPrefetchParametersWatermarksAndPerformanceCalculation' [-Wframe-larger-than] >> >> :( >> >>> Nathan, have you given up on these being sorted out? >> >> Does your configuration have KASAN (I don't think RISC-V supports >> KCSAN)? It is possible that dml/dcn32 needs something similar to commit >> 6740ec97bcdb ("drm/amd/display: Increase frame warning limit with KASAN >> or KCSAN in dml2")? >> >> I am not really interested in playing whack-a-mole with these warnings >> like I have done in the past for the reasons I outlined here: >> >> https://lore.kernel.org/20231019205117.GA839902@dev-arch.thelio-3990X/ > > I also see one of these with clang 17 even with KASAN disabled: > > drivers/gpu/drm/amd/amdgpu/../display/dc/dml/dcn32/display_mode_vba_32.c:37:6: > warning: stack frame size (2208) exceeds limit (2048) in 'dml32_recalculate' > [-Wframe-larger-than] > void dml32_recalculate(struct display_mode_lib *mode_lib) > > ^ > 1532/2208 (69.38%) spills, 676/2208 (30.62%) variables > > So I'm in favor of just raising the limit for these files for clang, like you > suggested in the linked thread. How about just adding a BUG_ON(IS_ENABLED(CONFIG_RISCV)) in that function? That should also avoid the build failure but give a better indication of where the problem is if someone actually runs into that function and triggers a runtime stack overflow. Arnd
Hi Arnd, On 2023-12-09 2:38 PM, Arnd Bergmann wrote: > On Fri, Dec 8, 2023, at 06:04, Samuel Holland wrote: >> On 2023-11-29 6:42 PM, Nathan Chancellor wrote: >>> On Thu, Nov 23, 2023 at 02:23:01PM +0000, Conor Dooley wrote: >>>> On Tue, Nov 21, 2023 at 07:05:15PM -0800, Samuel Holland wrote: >>>>> RISC-V uses kernel_fpu_begin()/kernel_fpu_end() like several other >>>>> architectures. Enabling hardware FP requires overriding the ISA string >>>>> for the relevant compilation units. >>>> >>>> Ah yes, bringing the joy of frame-larger-than warnings to RISC-V: >>>> ../drivers/gpu/drm/amd/amdgpu/../display/dc/dml/dcn32/display_mode_vba_32.c:58:13: warning: stack frame size (2416) exceeds limit (2048) in 'DISPCLKDPPCLKDCFCLKDeepSleepPrefetchParametersWatermarksAndPerformanceCalculation' [-Wframe-larger-than] >>> >>> :( >>> >>>> Nathan, have you given up on these being sorted out? >>> >>> Does your configuration have KASAN (I don't think RISC-V supports >>> KCSAN)? It is possible that dml/dcn32 needs something similar to commit >>> 6740ec97bcdb ("drm/amd/display: Increase frame warning limit with KASAN >>> or KCSAN in dml2")? >>> >>> I am not really interested in playing whack-a-mole with these warnings >>> like I have done in the past for the reasons I outlined here: >>> >>> https://lore.kernel.org/20231019205117.GA839902@dev-arch.thelio-3990X/ >> >> I also see one of these with clang 17 even with KASAN disabled: >> >> drivers/gpu/drm/amd/amdgpu/../display/dc/dml/dcn32/display_mode_vba_32.c:37:6: >> warning: stack frame size (2208) exceeds limit (2048) in 'dml32_recalculate' >> [-Wframe-larger-than] >> void dml32_recalculate(struct display_mode_lib *mode_lib) >> >> ^ >> 1532/2208 (69.38%) spills, 676/2208 (30.62%) variables >> >> So I'm in favor of just raising the limit for these files for clang, like you >> suggested in the linked thread. > > How about just adding a BUG_ON(IS_ENABLED(CONFIG_RISCV)) > in that function? That should also avoid the build failure > but give a better indication of where the problem is > if someone actually runs into that function and triggers > a runtime stack overflow. Won't that break actual users of the driver, trading an unlikely but theoretically possible stack overflow for a guaranteed crash? The intent of this series is that I have one of these GPUs plugged in to a RISC-V board, and I want to use it. Regards, Samuel
On Sat, Dec 9, 2023, at 22:29, Samuel Holland wrote: > On 2023-12-09 2:38 PM, Arnd Bergmann wrote: >> On Fri, Dec 8, 2023, at 06:04, Samuel Holland wrote: >>> On 2023-11-29 6:42 PM, Nathan Chancellor wrote: >>>> >>>> https://lore.kernel.org/20231019205117.GA839902@dev-arch.thelio-3990X/ >>> >>> I also see one of these with clang 17 even with KASAN disabled: >>> >>> drivers/gpu/drm/amd/amdgpu/../display/dc/dml/dcn32/display_mode_vba_32.c:37:6: >>> warning: stack frame size (2208) exceeds limit (2048) in 'dml32_recalculate' >>> [-Wframe-larger-than] >>> void dml32_recalculate(struct display_mode_lib *mode_lib) >>> >>> ^ >>> 1532/2208 (69.38%) spills, 676/2208 (30.62%) variables >>> >>> So I'm in favor of just raising the limit for these files for clang, like you >>> suggested in the linked thread. >> >> How about just adding a BUG_ON(IS_ENABLED(CONFIG_RISCV)) >> in that function? That should also avoid the build failure >> but give a better indication of where the problem is >> if someone actually runs into that function and triggers >> a runtime stack overflow. > > Won't that break actual users of the driver, trading an unlikely but > theoretically possible stack overflow for a guaranteed crash? The intent of this > series is that I have one of these GPUs plugged in to a RISC-V board, and I want > to use it. Ah, I thought you just wanted to get it to compile cleanly so you could use some of the more common cards. If you are trying to run the dcn32 code specifically, then you should definitely fix the stack usage of that function instead. Arnd
On Sun, Dec 10, 2023 at 5:10 AM Samuel Holland <samuel.holland@sifive.com> wrote: > > Hi Arnd, > > On 2023-12-09 2:38 PM, Arnd Bergmann wrote: > > On Fri, Dec 8, 2023, at 06:04, Samuel Holland wrote: > >> On 2023-11-29 6:42 PM, Nathan Chancellor wrote: > >>> On Thu, Nov 23, 2023 at 02:23:01PM +0000, Conor Dooley wrote: > >>>> On Tue, Nov 21, 2023 at 07:05:15PM -0800, Samuel Holland wrote: > >>>>> RISC-V uses kernel_fpu_begin()/kernel_fpu_end() like several other > >>>>> architectures. Enabling hardware FP requires overriding the ISA string > >>>>> for the relevant compilation units. > >>>> > >>>> Ah yes, bringing the joy of frame-larger-than warnings to RISC-V: > >>>> ../drivers/gpu/drm/amd/amdgpu/../display/dc/dml/dcn32/display_mode_vba_32.c:58:13: warning: stack frame size (2416) exceeds limit (2048) in 'DISPCLKDPPCLKDCFCLKDeepSleepPrefetchParametersWatermarksAndPerformanceCalculation' [-Wframe-larger-than] > >>> > >>> :( > >>> > >>>> Nathan, have you given up on these being sorted out? > >>> > >>> Does your configuration have KASAN (I don't think RISC-V supports > >>> KCSAN)? It is possible that dml/dcn32 needs something similar to commit > >>> 6740ec97bcdb ("drm/amd/display: Increase frame warning limit with KASAN > >>> or KCSAN in dml2")? > >>> > >>> I am not really interested in playing whack-a-mole with these warnings > >>> like I have done in the past for the reasons I outlined here: > >>> > >>> https://lore.kernel.org/20231019205117.GA839902@dev-arch.thelio-3990X/ > >> > >> I also see one of these with clang 17 even with KASAN disabled: > >> > >> drivers/gpu/drm/amd/amdgpu/../display/dc/dml/dcn32/display_mode_vba_32.c:37:6: > >> warning: stack frame size (2208) exceeds limit (2048) in 'dml32_recalculate' > >> [-Wframe-larger-than] > >> void dml32_recalculate(struct display_mode_lib *mode_lib) > >> > >> ^ > >> 1532/2208 (69.38%) spills, 676/2208 (30.62%) variables > >> > >> So I'm in favor of just raising the limit for these files for clang, like you > >> suggested in the linked thread. > > > > How about just adding a BUG_ON(IS_ENABLED(CONFIG_RISCV)) > > in that function? That should also avoid the build failure > > but give a better indication of where the problem is > > if someone actually runs into that function and triggers > > a runtime stack overflow. > > Won't that break actual users of the driver, trading an unlikely but > theoretically possible stack overflow for a guaranteed crash? The intent of this > series is that I have one of these GPUs plugged in to a RISC-V board, and I want > to use it. Does this patch address the issue? https://gitlab.freedesktop.org/agd5f/linux/-/commit/72ada8603e36291ad91e4f40f10ef742ef79bc4e Alex
Hi Alex, On 2023-12-11 9:17 AM, Alex Deucher wrote: > On Sun, Dec 10, 2023 at 5:10 AM Samuel Holland > <samuel.holland@sifive.com> wrote: >> >> Hi Arnd, >> >> On 2023-12-09 2:38 PM, Arnd Bergmann wrote: >>> On Fri, Dec 8, 2023, at 06:04, Samuel Holland wrote: >>>> On 2023-11-29 6:42 PM, Nathan Chancellor wrote: >>>>> On Thu, Nov 23, 2023 at 02:23:01PM +0000, Conor Dooley wrote: >>>>>> On Tue, Nov 21, 2023 at 07:05:15PM -0800, Samuel Holland wrote: >>>>>>> RISC-V uses kernel_fpu_begin()/kernel_fpu_end() like several other >>>>>>> architectures. Enabling hardware FP requires overriding the ISA string >>>>>>> for the relevant compilation units. >>>>>> >>>>>> Ah yes, bringing the joy of frame-larger-than warnings to RISC-V: >>>>>> ../drivers/gpu/drm/amd/amdgpu/../display/dc/dml/dcn32/display_mode_vba_32.c:58:13: warning: stack frame size (2416) exceeds limit (2048) in 'DISPCLKDPPCLKDCFCLKDeepSleepPrefetchParametersWatermarksAndPerformanceCalculation' [-Wframe-larger-than] >>>>> >>>>> :( >>>>> >>>>>> Nathan, have you given up on these being sorted out? >>>>> >>>>> Does your configuration have KASAN (I don't think RISC-V supports >>>>> KCSAN)? It is possible that dml/dcn32 needs something similar to commit >>>>> 6740ec97bcdb ("drm/amd/display: Increase frame warning limit with KASAN >>>>> or KCSAN in dml2")? >>>>> >>>>> I am not really interested in playing whack-a-mole with these warnings >>>>> like I have done in the past for the reasons I outlined here: >>>>> >>>>> https://lore.kernel.org/20231019205117.GA839902@dev-arch.thelio-3990X/ >>>> >>>> I also see one of these with clang 17 even with KASAN disabled: >>>> >>>> drivers/gpu/drm/amd/amdgpu/../display/dc/dml/dcn32/display_mode_vba_32.c:37:6: >>>> warning: stack frame size (2208) exceeds limit (2048) in 'dml32_recalculate' >>>> [-Wframe-larger-than] >>>> void dml32_recalculate(struct display_mode_lib *mode_lib) >>>> >>>> ^ >>>> 1532/2208 (69.38%) spills, 676/2208 (30.62%) variables >>>> >>>> So I'm in favor of just raising the limit for these files for clang, like you >>>> suggested in the linked thread. >>> >>> How about just adding a BUG_ON(IS_ENABLED(CONFIG_RISCV)) >>> in that function? That should also avoid the build failure >>> but give a better indication of where the problem is >>> if someone actually runs into that function and triggers >>> a runtime stack overflow. >> >> Won't that break actual users of the driver, trading an unlikely but >> theoretically possible stack overflow for a guaranteed crash? The intent of this >> series is that I have one of these GPUs plugged in to a RISC-V board, and I want >> to use it. > > Does this patch address the issue? > https://gitlab.freedesktop.org/agd5f/linux/-/commit/72ada8603e36291ad91e4f40f10ef742ef79bc4e No, I get the warning without any of these debugging options enabled. I can reproduce with just defconfig + CONFIG_DRM_AMDGPU=m when built with clang 17. Regards, Samuel
On Thu, Dec 07, 2023 at 10:49:53PM -0600, Samuel Holland wrote: > Actually tracking all possibly-FPU-tainted functions and their call sites is > probably possible, but a much larger task. I think objtool should be able to do that reasonably easily, it already does it for checking section where userspace address access is enabled or not, which is very similar.
On Mon, Dec 11, 2023 at 11:12:42PM -0800, Christoph Hellwig wrote: > On Thu, Dec 07, 2023 at 10:49:53PM -0600, Samuel Holland wrote: > > Actually tracking all possibly-FPU-tainted functions and their call sites is > > probably possible, but a much larger task. > > I think objtool should be able to do that reasonably easily, it already > does it for checking section where userspace address access is enabled > or not, which is very similar. Yeah, that might be doable. I can look into it.
diff --git a/drivers/gpu/drm/amd/display/Kconfig b/drivers/gpu/drm/amd/display/Kconfig index 901d1961b739..49b33b2f6701 100644 --- a/drivers/gpu/drm/amd/display/Kconfig +++ b/drivers/gpu/drm/amd/display/Kconfig @@ -8,7 +8,10 @@ config DRM_AMD_DC depends on BROKEN || !CC_IS_CLANG || ARM64 || RISCV || SPARC64 || X86_64 select SND_HDA_COMPONENT if SND_HDA_CORE # !CC_IS_CLANG: https://github.com/ClangBuiltLinux/linux/issues/1752 - select DRM_AMD_DC_FP if (X86 || LOONGARCH || (PPC64 && ALTIVEC) || (ARM64 && KERNEL_MODE_NEON && !CC_IS_CLANG)) + select DRM_AMD_DC_FP if ARM64 && KERNEL_MODE_NEON && !CC_IS_CLANG + select DRM_AMD_DC_FP if PPC64 && ALTIVEC + select DRM_AMD_DC_FP if RISCV && FPU + select DRM_AMD_DC_FP if LOONGARCH || X86 help Choose this option if you want to use the new display engine support for AMDGPU. This adds required support for Vega and diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c index 4ae4720535a5..834dca0396f1 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c @@ -35,6 +35,8 @@ #include <asm/neon.h> #elif defined(CONFIG_LOONGARCH) #include <asm/fpu.h> +#elif defined(CONFIG_RISCV) +#include <asm/switch_to.h> #endif /** @@ -89,7 +91,7 @@ void dc_fpu_begin(const char *function_name, const int line) depth = __this_cpu_inc_return(fpu_recursion_depth); if (depth == 1) { -#if defined(CONFIG_X86) || defined(CONFIG_LOONGARCH) +#if defined(CONFIG_X86) || defined(CONFIG_LOONGARCH) || defined(CONFIG_RISCV) kernel_fpu_begin(); #elif defined(CONFIG_PPC64) if (cpu_has_feature(CPU_FTR_VSX_COMP)) @@ -122,7 +124,7 @@ void dc_fpu_end(const char *function_name, const int line) depth = __this_cpu_dec_return(fpu_recursion_depth); if (depth == 0) { -#if defined(CONFIG_X86) || defined(CONFIG_LOONGARCH) +#if defined(CONFIG_X86) || defined(CONFIG_LOONGARCH) || defined(CONFIG_RISCV) kernel_fpu_end(); #elif defined(CONFIG_PPC64) if (cpu_has_feature(CPU_FTR_VSX_COMP)) diff --git a/drivers/gpu/drm/amd/display/dc/dml/Makefile b/drivers/gpu/drm/amd/display/dc/dml/Makefile index ea7d60f9a9b4..5c8f840ef323 100644 --- a/drivers/gpu/drm/amd/display/dc/dml/Makefile +++ b/drivers/gpu/drm/amd/display/dc/dml/Makefile @@ -43,6 +43,12 @@ dml_ccflags := -mfpu=64 dml_rcflags := -msoft-float endif +ifdef CONFIG_RISCV +include $(srctree)/arch/riscv/Makefile.isa +# Remove V from the ISA string, like in arch/riscv/Makefile, but keep F and D. +dml_ccflags := -march=$(shell echo $(riscv-march-y) | sed -E 's/(rv32ima|rv64ima)([^v_]*)v?/\1\2/') +endif + ifdef CONFIG_CC_IS_GCC ifneq ($(call gcc-min-version, 70100),y) IS_OLD_GCC = 1 diff --git a/drivers/gpu/drm/amd/display/dc/dml2/Makefile b/drivers/gpu/drm/amd/display/dc/dml2/Makefile index acff3449b8d7..15ad6e3a2173 100644 --- a/drivers/gpu/drm/amd/display/dc/dml2/Makefile +++ b/drivers/gpu/drm/amd/display/dc/dml2/Makefile @@ -42,6 +42,12 @@ dml2_ccflags := -mfpu=64 dml2_rcflags := -msoft-float endif +ifdef CONFIG_RISCV +include $(srctree)/arch/riscv/Makefile.isa +# Remove V from the ISA string, like in arch/riscv/Makefile, but keep F and D. +dml2_ccflags := -march=$(shell echo $(riscv-march-y) | sed -E 's/(rv32ima|rv64ima)([^v_]*)v?/\1\2/') +endif + ifdef CONFIG_CC_IS_GCC ifeq ($(call cc-ifversion, -lt, 0701, y), y) IS_OLD_GCC = 1
RISC-V uses kernel_fpu_begin()/kernel_fpu_end() like several other architectures. Enabling hardware FP requires overriding the ISA string for the relevant compilation units. Signed-off-by: Samuel Holland <samuel.holland@sifive.com> --- drivers/gpu/drm/amd/display/Kconfig | 5 ++++- drivers/gpu/drm/amd/display/amdgpu_dm/dc_fpu.c | 6 ++++-- drivers/gpu/drm/amd/display/dc/dml/Makefile | 6 ++++++ drivers/gpu/drm/amd/display/dc/dml2/Makefile | 6 ++++++ 4 files changed, 20 insertions(+), 3 deletions(-)