| Message ID | 20210308153359.2513446-1-arnd@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | drm/amdkfd: fix build error with missing AMD_IOMMU_V2 | expand |
The driver build should work without IOMMUv2. In amdkfd/Makefile, we have this condition: ifneq ($(CONFIG_AMD_IOMMU_V2),) AMDKFD_FILES += $(AMDKFD_PATH)/kfd_iommu.o endif In amdkfd/kfd_iommu.h we define inline stubs of the functions that are causing your link-failures if IOMMU_V2 is not enabled: #if defined(CONFIG_AMD_IOMMU_V2_MODULE) || defined(CONFIG_AMD_IOMMU_V2) ... function declarations ... #else ... stubs ... #endif Regards, Felix Am 2021-03-08 um 10:33 a.m. schrieb Arnd Bergmann: > From: Arnd Bergmann <arnd@arndb.de> > > Using 'imply AMD_IOMMU_V2' does not guarantee that the driver can link > against the exported functions. If the GPU driver is built-in but the > IOMMU driver is a loadable module, the kfd_iommu.c file is indeed > built but does not work: > > x86_64-linux-ld: drivers/gpu/drm/amd/amdkfd/kfd_iommu.o: in function `kfd_iommu_bind_process_to_device': > kfd_iommu.c:(.text+0x516): undefined reference to `amd_iommu_bind_pasid' > x86_64-linux-ld: drivers/gpu/drm/amd/amdkfd/kfd_iommu.o: in function `kfd_iommu_unbind_process': > kfd_iommu.c:(.text+0x691): undefined reference to `amd_iommu_unbind_pasid' > x86_64-linux-ld: drivers/gpu/drm/amd/amdkfd/kfd_iommu.o: in function `kfd_iommu_suspend': > kfd_iommu.c:(.text+0x966): undefined reference to `amd_iommu_set_invalidate_ctx_cb' > x86_64-linux-ld: kfd_iommu.c:(.text+0x97f): undefined reference to `amd_iommu_set_invalid_ppr_cb' > x86_64-linux-ld: kfd_iommu.c:(.text+0x9a4): undefined reference to `amd_iommu_free_device' > x86_64-linux-ld: drivers/gpu/drm/amd/amdkfd/kfd_iommu.o: in function `kfd_iommu_resume': > kfd_iommu.c:(.text+0xa9a): undefined reference to `amd_iommu_init_device' > x86_64-linux-ld: kfd_iommu.c:(.text+0xadc): undefined reference to `amd_iommu_set_invalidate_ctx_cb' > x86_64-linux-ld: kfd_iommu.c:(.text+0xaff): undefined reference to `amd_iommu_set_invalid_ppr_cb' > x86_64-linux-ld: kfd_iommu.c:(.text+0xc72): undefined reference to `amd_iommu_bind_pasid' > x86_64-linux-ld: kfd_iommu.c:(.text+0xe08): undefined reference to `amd_iommu_set_invalidate_ctx_cb' > x86_64-linux-ld: kfd_iommu.c:(.text+0xe26): undefined reference to `amd_iommu_set_invalid_ppr_cb' > x86_64-linux-ld: kfd_iommu.c:(.text+0xe42): undefined reference to `amd_iommu_free_device' > > Use a stronger 'select' instead. > > Fixes: 64d1c3a43a6f ("drm/amdkfd: Centralize IOMMUv2 code and make it conditional") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/gpu/drm/amd/amdkfd/Kconfig | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdkfd/Kconfig b/drivers/gpu/drm/amd/amdkfd/Kconfig > index f02c938f75da..91f85dfb7ba6 100644 > --- a/drivers/gpu/drm/amd/amdkfd/Kconfig > +++ b/drivers/gpu/drm/amd/amdkfd/Kconfig > @@ -5,8 +5,9 @@ > > config HSA_AMD > bool "HSA kernel driver for AMD GPU devices" > - depends on DRM_AMDGPU && (X86_64 || ARM64 || PPC64) > - imply AMD_IOMMU_V2 if X86_64 > + depends on DRM_AMDGPU && ((X86_64 && IOMMU_SUPPORT && ACPI) || ARM64 || PPC64) > + select AMD_IOMMU if X86_64 > + select AMD_IOMMU_V2 if X86_64 > select HMM_MIRROR > select MMU_NOTIFIER > select DRM_AMDGPU_USERPTR
On Mon, Mar 8, 2021 at 5:24 PM Felix Kuehling <felix.kuehling@amd.com> wrote: > > The driver build should work without IOMMUv2. In amdkfd/Makefile, we > have this condition: > > ifneq ($(CONFIG_AMD_IOMMU_V2),) > AMDKFD_FILES += $(AMDKFD_PATH)/kfd_iommu.o > endif > > In amdkfd/kfd_iommu.h we define inline stubs of the functions that are > causing your link-failures if IOMMU_V2 is not enabled: > > #if defined(CONFIG_AMD_IOMMU_V2_MODULE) || defined(CONFIG_AMD_IOMMU_V2) > ... function declarations ... > #else > ... stubs ... > #endif Right, that is the problem I tried to explain in my patch description. Should we just drop the 'imply' then and add a proper dependency like this? depends on DRM_AMDGPU && (X86_64 || ARM64 || PPC64) depends on AMD_IOMMU_V2=y || DRM_AMDGPU=m I can send a v2 after some testing if you prefer this version. Arnd _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Am 2021-03-08 um 2:05 p.m. schrieb Arnd Bergmann: > On Mon, Mar 8, 2021 at 5:24 PM Felix Kuehling <felix.kuehling@amd.com> wrote: >> The driver build should work without IOMMUv2. In amdkfd/Makefile, we >> have this condition: >> >> ifneq ($(CONFIG_AMD_IOMMU_V2),) >> AMDKFD_FILES += $(AMDKFD_PATH)/kfd_iommu.o >> endif >> >> In amdkfd/kfd_iommu.h we define inline stubs of the functions that are >> causing your link-failures if IOMMU_V2 is not enabled: >> >> #if defined(CONFIG_AMD_IOMMU_V2_MODULE) || defined(CONFIG_AMD_IOMMU_V2) >> ... function declarations ... >> #else >> ... stubs ... >> #endif > Right, that is the problem I tried to explain in my patch description. > > Should we just drop the 'imply' then and add a proper dependency like this? > > depends on DRM_AMDGPU && (X86_64 || ARM64 || PPC64) > depends on AMD_IOMMU_V2=y || DRM_AMDGPU=m > > I can send a v2 after some testing if you prefer this version. No. My point is, there should not be a hard dependency. The build should work without CONFIG_AMD_IOMMU_V2. I don't understand why it's not working for you. It looks like you're building kfd_iommu.o, which should not be happening when AMD_IOMMU_V2 is not enabled. The condition in amdkfd/Makefile should make sure that kfd_iommu.o doesn't get built with your kernel config. Regards, Felix > > Arnd
On Mon, Mar 8, 2021 at 8:11 PM Felix Kuehling <felix.kuehling@amd.com> wrote: > > Am 2021-03-08 um 2:05 p.m. schrieb Arnd Bergmann: > > On Mon, Mar 8, 2021 at 5:24 PM Felix Kuehling <felix.kuehling@amd.com> wrote: > >> The driver build should work without IOMMUv2. In amdkfd/Makefile, we > >> have this condition: > >> > >> ifneq ($(CONFIG_AMD_IOMMU_V2),) > >> AMDKFD_FILES += $(AMDKFD_PATH)/kfd_iommu.o > >> endif > >> > >> In amdkfd/kfd_iommu.h we define inline stubs of the functions that are > >> causing your link-failures if IOMMU_V2 is not enabled: > >> > >> #if defined(CONFIG_AMD_IOMMU_V2_MODULE) || defined(CONFIG_AMD_IOMMU_V2) > >> ... function declarations ... > >> #else > >> ... stubs ... > >> #endif > > Right, that is the problem I tried to explain in my patch description. > > > > Should we just drop the 'imply' then and add a proper dependency like this? > > > > depends on DRM_AMDGPU && (X86_64 || ARM64 || PPC64) > > depends on AMD_IOMMU_V2=y || DRM_AMDGPU=m > > > > I can send a v2 after some testing if you prefer this version. > > No. My point is, there should not be a hard dependency. The build should > work without CONFIG_AMD_IOMMU_V2. I don't understand why it's not > working for you. It looks like you're building kfd_iommu.o, which should > not be happening when AMD_IOMMU_V2 is not enabled. The condition in > amdkfd/Makefile should make sure that kfd_iommu.o doesn't get built with > your kernel config. Again, as I explained in the changelog text, AMD_IOMMU_V2 configured as a loadable module, while AMDGPU is configured as built-in. The causes a link failure for the vmlinux file, because the linker cannot resolve addresses of loadable modules at compile time -- they have not been loaded yet. Arnd _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Am 2021-03-08 um 2:33 p.m. schrieb Arnd Bergmann: > On Mon, Mar 8, 2021 at 8:11 PM Felix Kuehling <felix.kuehling@amd.com> wrote: >> Am 2021-03-08 um 2:05 p.m. schrieb Arnd Bergmann: >>> On Mon, Mar 8, 2021 at 5:24 PM Felix Kuehling <felix.kuehling@amd.com> wrote: >>>> The driver build should work without IOMMUv2. In amdkfd/Makefile, we >>>> have this condition: >>>> >>>> ifneq ($(CONFIG_AMD_IOMMU_V2),) >>>> AMDKFD_FILES += $(AMDKFD_PATH)/kfd_iommu.o >>>> endif >>>> >>>> In amdkfd/kfd_iommu.h we define inline stubs of the functions that are >>>> causing your link-failures if IOMMU_V2 is not enabled: >>>> >>>> #if defined(CONFIG_AMD_IOMMU_V2_MODULE) || defined(CONFIG_AMD_IOMMU_V2) >>>> ... function declarations ... >>>> #else >>>> ... stubs ... >>>> #endif >>> Right, that is the problem I tried to explain in my patch description. >>> >>> Should we just drop the 'imply' then and add a proper dependency like this? >>> >>> depends on DRM_AMDGPU && (X86_64 || ARM64 || PPC64) >>> depends on AMD_IOMMU_V2=y || DRM_AMDGPU=m >>> >>> I can send a v2 after some testing if you prefer this version. >> No. My point is, there should not be a hard dependency. The build should >> work without CONFIG_AMD_IOMMU_V2. I don't understand why it's not >> working for you. It looks like you're building kfd_iommu.o, which should >> not be happening when AMD_IOMMU_V2 is not enabled. The condition in >> amdkfd/Makefile should make sure that kfd_iommu.o doesn't get built with >> your kernel config. > Again, as I explained in the changelog text, AMD_IOMMU_V2 configured as > a loadable module, while AMDGPU is configured as built-in. I'm sorry, I didn't read it carefully. And I thought "imply" was meant to fix exactly this kind of issue. I don't want to create a hard dependency on AMD_IOMMU_V2 if I can avoid it, because it is only really needed for a small number of AMD APUs, and even there it is now optional for more recent ones. Is there a better way to avoid build failures without creating a hard dependency? The documentation in Documentation/kbuild/kconfig-language.rst suggests using if (IS_REACHABLE(CONFIG_AMD_IOMMU_V2)) to guard those problematic function calls. I think more generally, we could guard all of kfd_iommu.c with #if IS_REACHABLE(CONFIG_AMD_IOMMU_V2) And use the same condition to define the stubs in kfd_iommu.h. Regards, Felix > > The causes a link failure for the vmlinux file, because the linker cannot > resolve addresses of loadable modules at compile time -- they have > not been loaded yet. > > Arnd > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Am 08.03.21 um 21:02 schrieb Felix Kuehling: > Am 2021-03-08 um 2:33 p.m. schrieb Arnd Bergmann: >> On Mon, Mar 8, 2021 at 8:11 PM Felix Kuehling <felix.kuehling@amd.com> wrote: >>> Am 2021-03-08 um 2:05 p.m. schrieb Arnd Bergmann: >>>> On Mon, Mar 8, 2021 at 5:24 PM Felix Kuehling <felix.kuehling@amd.com> wrote: >>>>> The driver build should work without IOMMUv2. In amdkfd/Makefile, we >>>>> have this condition: >>>>> >>>>> ifneq ($(CONFIG_AMD_IOMMU_V2),) >>>>> AMDKFD_FILES += $(AMDKFD_PATH)/kfd_iommu.o >>>>> endif >>>>> >>>>> In amdkfd/kfd_iommu.h we define inline stubs of the functions that are >>>>> causing your link-failures if IOMMU_V2 is not enabled: >>>>> >>>>> #if defined(CONFIG_AMD_IOMMU_V2_MODULE) || defined(CONFIG_AMD_IOMMU_V2) >>>>> ... function declarations ... >>>>> #else >>>>> ... stubs ... >>>>> #endif >>>> Right, that is the problem I tried to explain in my patch description. >>>> >>>> Should we just drop the 'imply' then and add a proper dependency like this? >>>> >>>> depends on DRM_AMDGPU && (X86_64 || ARM64 || PPC64) >>>> depends on AMD_IOMMU_V2=y || DRM_AMDGPU=m >>>> >>>> I can send a v2 after some testing if you prefer this version. >>> No. My point is, there should not be a hard dependency. The build should >>> work without CONFIG_AMD_IOMMU_V2. I don't understand why it's not >>> working for you. It looks like you're building kfd_iommu.o, which should >>> not be happening when AMD_IOMMU_V2 is not enabled. The condition in >>> amdkfd/Makefile should make sure that kfd_iommu.o doesn't get built with >>> your kernel config. >> Again, as I explained in the changelog text, AMD_IOMMU_V2 configured as >> a loadable module, while AMDGPU is configured as built-in. > I'm sorry, I didn't read it carefully. And I thought "imply" was meant > to fix exactly this kind of issue. > > I don't want to create a hard dependency on AMD_IOMMU_V2 if I can avoid > it, because it is only really needed for a small number of AMD APUs, and > even there it is now optional for more recent ones. > > Is there a better way to avoid build failures without creating a hard > dependency? What you need is the same trick we used for AGP on radeon/nouveau: depends on AMD_IOMMU_V2 || !AMD_IOMMU_V2 This way when AMD_IOMMU_V2 is build as a module DRM_AMDGPU will be build as a module as well. When it is disabled completely we don't care. Regards, Christian. > The documentation in > Documentation/kbuild/kconfig-language.rst suggests using if > (IS_REACHABLE(CONFIG_AMD_IOMMU_V2)) to guard those problematic function > calls. I think more generally, we could guard all of kfd_iommu.c with > > #if IS_REACHABLE(CONFIG_AMD_IOMMU_V2) > > And use the same condition to define the stubs in kfd_iommu.h. > > Regards, > Felix > > >> The causes a link failure for the vmlinux file, because the linker cannot >> resolve addresses of loadable modules at compile time -- they have >> not been loaded yet. >> >> Arnd >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel > _______________________________________________ > amd-gfx mailing list > amd-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
On Mon, Mar 8, 2021 at 9:12 PM Christian König <ckoenig.leichtzumerken@gmail.com> wrote: > Am 08.03.21 um 21:02 schrieb Felix Kuehling: > > Am 2021-03-08 um 2:33 p.m. schrieb Arnd Bergmann: > > I don't want to create a hard dependency on AMD_IOMMU_V2 if I can avoid > > it, because it is only really needed for a small number of AMD APUs, and > > even there it is now optional for more recent ones. > > > > Is there a better way to avoid build failures without creating a hard > > dependency? > > What you need is the same trick we used for AGP on radeon/nouveau: > > depends on AMD_IOMMU_V2 || !AMD_IOMMU_V2 > > This way when AMD_IOMMU_V2 is build as a module DRM_AMDGPU will be build > as a module as well. When it is disabled completely we don't care. Note that this trick only works if you put it into the DRM_AMDGPU Kconfig option itself, since that decides if the driver is built-in or a loadable module. If HSA_AMD is disabled, that dependency is not really necessary. The version I suggested -- adding "depends on AMD_IOMMU_V2=y || DRM_AMDGPU=m" to the HSA_AMD option -- might be slightly nicer since it lets you still build a kernel with DRM_AMDGPU=y and AMD_IOMMU_V2=m, but without the HSA_AMD. I can send a patch with either of those two options to replace my original patch. > > The documentation in > > Documentation/kbuild/kconfig-language.rst suggests using if > > (IS_REACHABLE(CONFIG_AMD_IOMMU_V2)) to guard those problematic function > > calls. I think more generally, we could guard all of kfd_iommu.c with > > > > #if IS_REACHABLE(CONFIG_AMD_IOMMU_V2) > > > > And use the same condition to define the stubs in kfd_iommu.h. This would fix the compile-time error, but it's also the one I'd least recommend out of all the options, because that causes the driver to silently not work as expected. This seems even worse than failing the build. Arnd
diff --git a/drivers/gpu/drm/amd/amdkfd/Kconfig b/drivers/gpu/drm/amd/amdkfd/Kconfig index f02c938f75da..91f85dfb7ba6 100644 --- a/drivers/gpu/drm/amd/amdkfd/Kconfig +++ b/drivers/gpu/drm/amd/amdkfd/Kconfig @@ -5,8 +5,9 @@ config HSA_AMD bool "HSA kernel driver for AMD GPU devices" - depends on DRM_AMDGPU && (X86_64 || ARM64 || PPC64) - imply AMD_IOMMU_V2 if X86_64 + depends on DRM_AMDGPU && ((X86_64 && IOMMU_SUPPORT && ACPI) || ARM64 || PPC64) + select AMD_IOMMU if X86_64 + select AMD_IOMMU_V2 if X86_64 select HMM_MIRROR select MMU_NOTIFIER select DRM_AMDGPU_USERPTR