| Message ID | 1464311916-10065-1-git-send-email-mcgrof@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, May 27, 2016 at 4:18 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote: > To get KFD support in radeon we need the following > initialization to happen in this order, their > respective driver file that has its init routine > listed next to it: > > 0. AMD IOMMUv1: arch/x86/kernel/pci-dma.c > 1. AMD IOMMUv2: drivers/iommu/amd_iommu_v2.c > 2. AMD KFD: drivers/gpu/drm/amd/amdkfd/kfd_module.c > 3. AMD Radeon: drivers/gpu/drm/radeon/radeon_drv.c Also AMD amdgpu (for VI+ APUs) > > Order is rather implicit, but these drivers can currently > only do so much given the amount of leg room available. > Below are the respective init routines and how they are > initialized: > > arch/x86/kernel/pci-dma.c rootfs_initcall(pci_iommu_init); > drivers/iommu/amd_iommu_v2.c module_init(amd_iommu_v2_init); > drivers/gpu/drm/amd/amdkfd/kfd_module.c module_init(kfd_module_init); > drivers/gpu/drm/radeon/radeon_drv.c module_init(radeon_init); > > When a driver is built-in module_init() folds to use > device_initcall(), and we have the following possible > orders: > > #define pure_initcall(fn) __define_initcall(fn, 0) > #define core_initcall(fn) __define_initcall(fn, 1) > #define postcore_initcall(fn)__define_initcall(fn, 2) > #define arch_initcall(fn) __define_initcall(fn, 3) > #define subsys_initcall(fn) __define_initcall(fn, 4) > #define fs_initcall(fn) __define_initcall(fn, 5) > --------------------------------------------------------- > #define rootfs_initcall(fn) __define_initcall(fn, rootfs) > #define device_initcall(fn) __define_initcall(fn, 6) > #define late_initcall(fn) __define_initcall(fn, 7) > > Since we start off from rootfs_initcall(), it gives us 3 more > levels of leg room to play with for order semantics, this isn't > enough to address all required levels of dependencies, this > is specially true given that AMD-KFD needs to be loaded before > the radeon driver -- -but this it not enforced by symbols. > If the AMD-KFD driver is not loaded prior to the radeon driver > because otherwise the radeon driver will not initialize the > AMD-KFD driver and you get no KFD functionality in userspace. > > Commit 1bacc894c227fad8a7 ("drivers: Move iommu/ before gpu/ in > Makefile") works around some of the possibe races between > the AMD IOMMU v2 and GPU drivers by changing the link order. > This is fragile, however its the bets we can do, given that > making the GPU drivers use late_initcall() would also implicate > a similar race between them. That possible race is fortunatley > addressed given that the drm Makefile currently has amdkfd > linked prior to radeon: > > drivers/gpu/drm/Makefile > ... > obj-$(CONFIG_HSA_AMD) += amd/amdkfd/ > obj-$(CONFIG_DRM_RADEON)+= radeon/ > ... > > Changing amdkfd and radeon to late_initcall() however is > still the right call in orde to annotate explicitly a > delayed dependency requirement between the GPU drivers > and the IOMMUs. > > We can't address the fragile nature of the link order > right now, but in the future that might be possible. > > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> > --- > > Please note, the changes to drivers/Makefile are just > for the sake of forcing the possible race to occur, > if this works well the actual [PATCH] submission will > skip those changes as its pointless to remove those > work arounds as it stands, due to the limited nature > of the levels available for addressing requirements. > > Also, if you are aware of further dependency hell > things like these -- please do let me know as I am > interested in looking at addressing them. > > drivers/Makefile | 6 ++---- > drivers/gpu/drm/amd/amdkfd/kfd_module.c | 2 +- > drivers/gpu/drm/radeon/radeon_drv.c | 2 +- > 3 files changed, 4 insertions(+), 6 deletions(-) > > diff --git a/drivers/Makefile b/drivers/Makefile > index 0b6f3d60193d..0fbe3982041f 100644 > --- a/drivers/Makefile > +++ b/drivers/Makefile > @@ -50,10 +50,7 @@ obj-$(CONFIG_RESET_CONTROLLER) += reset/ > obj-y += tty/ > obj-y += char/ > > -# iommu/ comes before gpu as gpu are using iommu controllers > -obj-$(CONFIG_IOMMU_SUPPORT) += iommu/ > - > -# gpu/ comes after char for AGP vs DRM startup and after iommu > +# gpu/ comes after char for AGP vs DRM startup > obj-y += gpu/ > > obj-$(CONFIG_CONNECTOR) += connector/ > @@ -147,6 +144,7 @@ obj-y += clk/ > > obj-$(CONFIG_MAILBOX) += mailbox/ > obj-$(CONFIG_HWSPINLOCK) += hwspinlock/ > +obj-$(CONFIG_IOMMU_SUPPORT) += iommu/ > obj-$(CONFIG_REMOTEPROC) += remoteproc/ > obj-$(CONFIG_RPMSG) += rpmsg/ > > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_module.c b/drivers/gpu/drm/amd/amdkfd/kfd_module.c > index 850a5623661f..3d1dab8a31c7 100644 > --- a/drivers/gpu/drm/amd/amdkfd/kfd_module.c > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_module.c > @@ -141,7 +141,7 @@ static void __exit kfd_module_exit(void) > dev_info(kfd_device, "Removed module\n"); > } > > -module_init(kfd_module_init); > +late_initcall(kfd_module_init); > module_exit(kfd_module_exit); > > MODULE_AUTHOR(KFD_DRIVER_AUTHOR); > diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c > index b55aa740171f..1fa1b7f3a89c 100644 > --- a/drivers/gpu/drm/radeon/radeon_drv.c > +++ b/drivers/gpu/drm/radeon/radeon_drv.c > @@ -609,7 +609,7 @@ static void __exit radeon_exit(void) > radeon_unregister_atpx_handler(); > } > > -module_init(radeon_init); > +late_initcall(radeon_init); > module_exit(radeon_exit); Need to modify also amdgpu module_init > > MODULE_AUTHOR(DRIVER_AUTHOR); > -- > 2.8.2 > I tested this on Kaveri, and amdkfd is working. For amdkfd that's fine, but IMO that's not enough testing for radeon/amdgpu. I would like to hear AMD's developers take on this. Oded
On Fri, May 27, 2016 at 3:18 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote: > To get KFD support in radeon we need the following > initialization to happen in this order, their > respective driver file that has its init routine > listed next to it: > > 0. AMD IOMMUv1: arch/x86/kernel/pci-dma.c > 1. AMD IOMMUv2: drivers/iommu/amd_iommu_v2.c > 2. AMD KFD: drivers/gpu/drm/amd/amdkfd/kfd_module.c > 3. AMD Radeon: drivers/gpu/drm/radeon/radeon_drv.c > > Order is rather implicit, but these drivers can currently > only do so much given the amount of leg room available. > Below are the respective init routines and how they are > initialized: > > arch/x86/kernel/pci-dma.c rootfs_initcall(pci_iommu_init); > drivers/iommu/amd_iommu_v2.c module_init(amd_iommu_v2_init); > drivers/gpu/drm/amd/amdkfd/kfd_module.c module_init(kfd_module_init); > drivers/gpu/drm/radeon/radeon_drv.c module_init(radeon_init); > > When a driver is built-in module_init() folds to use > device_initcall(), and we have the following possible > orders: > > #define pure_initcall(fn) __define_initcall(fn, 0) > #define core_initcall(fn) __define_initcall(fn, 1) > #define postcore_initcall(fn)__define_initcall(fn, 2) > #define arch_initcall(fn) __define_initcall(fn, 3) > #define subsys_initcall(fn) __define_initcall(fn, 4) > #define fs_initcall(fn) __define_initcall(fn, 5) > --------------------------------------------------------- > #define rootfs_initcall(fn) __define_initcall(fn, rootfs) > #define device_initcall(fn) __define_initcall(fn, 6) > #define late_initcall(fn) __define_initcall(fn, 7) > > Since we start off from rootfs_initcall(), it gives us 3 more > levels of leg room to play with for order semantics, this isn't > enough to address all required levels of dependencies, this > is specially true given that AMD-KFD needs to be loaded before > the radeon driver -- -but this it not enforced by symbols. > If the AMD-KFD driver is not loaded prior to the radeon driver > because otherwise the radeon driver will not initialize the > AMD-KFD driver and you get no KFD functionality in userspace. > > Commit 1bacc894c227fad8a7 ("drivers: Move iommu/ before gpu/ in > Makefile") works around some of the possibe races between > the AMD IOMMU v2 and GPU drivers by changing the link order. > This is fragile, however its the bets we can do, given that > making the GPU drivers use late_initcall() would also implicate > a similar race between them. That possible race is fortunatley > addressed given that the drm Makefile currently has amdkfd > linked prior to radeon: > > drivers/gpu/drm/Makefile > ... > obj-$(CONFIG_HSA_AMD) += amd/amdkfd/ > obj-$(CONFIG_DRM_RADEON)+= radeon/ > ... > > Changing amdkfd and radeon to late_initcall() however is > still the right call in orde to annotate explicitly a > delayed dependency requirement between the GPU drivers > and the IOMMUs. > > We can't address the fragile nature of the link order > right now, but in the future that might be possible. > > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> > --- > > Please note, the changes to drivers/Makefile are just > for the sake of forcing the possible race to occur, > if this works well the actual [PATCH] submission will > skip those changes as its pointless to remove those > work arounds as it stands, due to the limited nature > of the levels available for addressing requirements. > > Also, if you are aware of further dependency hell > things like these -- please do let me know as I am > interested in looking at addressing them. This should be fixed with -EDEFER_PROBE instead. Frobbing initcall levels should then just be done as an optimization to avoid too much reprobing. Thanks, Daniel
On Sun, May 29, 2016 at 08:27:07PM +0200, Daniel Vetter wrote: > On Fri, May 27, 2016 at 3:18 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote: > > To get KFD support in radeon we need the following > > initialization to happen in this order, their > > respective driver file that has its init routine > > listed next to it: > > > > 0. AMD IOMMUv1: arch/x86/kernel/pci-dma.c > > 1. AMD IOMMUv2: drivers/iommu/amd_iommu_v2.c > > 2. AMD KFD: drivers/gpu/drm/amd/amdkfd/kfd_module.c > > 3. AMD Radeon: drivers/gpu/drm/radeon/radeon_drv.c > > > > Order is rather implicit, but these drivers can currently > > only do so much given the amount of leg room available. > > Below are the respective init routines and how they are > > initialized: > > > > arch/x86/kernel/pci-dma.c rootfs_initcall(pci_iommu_init); > > drivers/iommu/amd_iommu_v2.c module_init(amd_iommu_v2_init); > > drivers/gpu/drm/amd/amdkfd/kfd_module.c module_init(kfd_module_init); > > drivers/gpu/drm/radeon/radeon_drv.c module_init(radeon_init); > > > > When a driver is built-in module_init() folds to use > > device_initcall(), and we have the following possible > > orders: > > > > #define pure_initcall(fn) __define_initcall(fn, 0) > > #define core_initcall(fn) __define_initcall(fn, 1) > > #define postcore_initcall(fn)__define_initcall(fn, 2) > > #define arch_initcall(fn) __define_initcall(fn, 3) > > #define subsys_initcall(fn) __define_initcall(fn, 4) > > #define fs_initcall(fn) __define_initcall(fn, 5) > > --------------------------------------------------------- > > #define rootfs_initcall(fn) __define_initcall(fn, rootfs) > > #define device_initcall(fn) __define_initcall(fn, 6) > > #define late_initcall(fn) __define_initcall(fn, 7) > > > > Since we start off from rootfs_initcall(), it gives us 3 more > > levels of leg room to play with for order semantics, this isn't > > enough to address all required levels of dependencies, this > > is specially true given that AMD-KFD needs to be loaded before > > the radeon driver -- -but this it not enforced by symbols. > > If the AMD-KFD driver is not loaded prior to the radeon driver > > because otherwise the radeon driver will not initialize the > > AMD-KFD driver and you get no KFD functionality in userspace. > > > > Commit 1bacc894c227fad8a7 ("drivers: Move iommu/ before gpu/ in > > Makefile") works around some of the possibe races between > > the AMD IOMMU v2 and GPU drivers by changing the link order. > > This is fragile, however its the bets we can do, given that > > making the GPU drivers use late_initcall() would also implicate > > a similar race between them. That possible race is fortunatley > > addressed given that the drm Makefile currently has amdkfd > > linked prior to radeon: > > > > drivers/gpu/drm/Makefile > > ... > > obj-$(CONFIG_HSA_AMD) += amd/amdkfd/ > > obj-$(CONFIG_DRM_RADEON)+= radeon/ > > ... > > > > Changing amdkfd and radeon to late_initcall() however is > > still the right call in orde to annotate explicitly a > > delayed dependency requirement between the GPU drivers > > and the IOMMUs. > > > > We can't address the fragile nature of the link order > > right now, but in the future that might be possible. > > > > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> > > --- > > > > Please note, the changes to drivers/Makefile are just > > for the sake of forcing the possible race to occur, > > if this works well the actual [PATCH] submission will > > skip those changes as its pointless to remove those > > work arounds as it stands, due to the limited nature > > of the levels available for addressing requirements. > > > > Also, if you are aware of further dependency hell > > things like these -- please do let me know as I am > > interested in looking at addressing them. > > This should be fixed with -EDEFER_PROBE instead. Frobbing initcall > levels should then just be done as an optimization to avoid too much > reprobing. radeon already uses -EDEFER_PROBE but it assumes that amdkfd *is* loaded first, and only if it is already loaded can it count on getting -EDEFER_PROBE. The radeon driver will deffer probe *iff* kgd2kfd_init() returns -EDEFER_PROBE, which only happens when amdkfd_init_completed is no longer present. If radeon gets linked first though the symbol fetch for kgd2kfd_init() will make it as not-present though. So the current heuristic used does not address proper link / load order. Part of the issue mentioned on the commit log is another race underneath the hood with the AMD IOMMU v2 which is needed for amdkfd. The underlying issue however really is the lack of available clear semantics for dependencies over 3 levels here. This is solved one way or another by link order in the Makefiles, but as I've noted so far this has been rather implicit and fragile. The change here makes at least the order of the GPU drivers explicitly later than the IOMMUs. The specific race between radeon and amdfkd is solved currently through link order through the Makefiles. In the future we maybe able to make things more explicit. -EDEFER_PROBE also introduces a latency on load which we should not need if we can handle proper link / load order dependency annotations. This change is a small part of that work, but as it the commit log also notes future further work is possible to build stronger semantics. Some of the work I'm doing with linker-tables may help with this in the future [0], but for now this should help with what the semantics we have in place. [0] http://lkml.kernel.org/r/1455889559-9428-1-git-send-email-mcgrof@kernel.org Luis
On Tue, May 31, 2016 at 8:15 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote: > On Sun, May 29, 2016 at 05:49:17PM +0300, Oded Gabbay wrote: >> On Fri, May 27, 2016 at 4:18 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote: >> > diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c >> > index b55aa740171f..1fa1b7f3a89c 100644 >> > --- a/drivers/gpu/drm/radeon/radeon_drv.c >> > +++ b/drivers/gpu/drm/radeon/radeon_drv.c >> > @@ -609,7 +609,7 @@ static void __exit radeon_exit(void) >> > radeon_unregister_atpx_handler(); >> > } >> > >> > -module_init(radeon_init); >> > +late_initcall(radeon_init); >> > module_exit(radeon_exit); >> >> Need to modify also amdgpu module_init > > Thanks, so other than considering the first two hunks are only for testing purposes > (a proper [PATCH] will drop these hunks on the Makefile), you're suggestng this > then, is that right?: Yes, the below should cover the amdgpu case as well. > > --- > drivers/Makefile | 6 ++---- > drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 2 +- > drivers/gpu/drm/amd/amdkfd/kfd_module.c | 2 +- > drivers/gpu/drm/radeon/radeon_drv.c | 2 +- > 4 files changed, 5 insertions(+), 7 deletions(-) > > diff --git a/drivers/Makefile b/drivers/Makefile > index 0b6f3d60193d..0fbe3982041f 100644 > --- a/drivers/Makefile > +++ b/drivers/Makefile > @@ -50,10 +50,7 @@ obj-$(CONFIG_RESET_CONTROLLER) += reset/ > obj-y += tty/ > obj-y += char/ > > -# iommu/ comes before gpu as gpu are using iommu controllers > -obj-$(CONFIG_IOMMU_SUPPORT) += iommu/ > - > -# gpu/ comes after char for AGP vs DRM startup and after iommu > +# gpu/ comes after char for AGP vs DRM startup > obj-y += gpu/ > > obj-$(CONFIG_CONNECTOR) += connector/ > @@ -147,6 +144,7 @@ obj-y += clk/ > > obj-$(CONFIG_MAILBOX) += mailbox/ > obj-$(CONFIG_HWSPINLOCK) += hwspinlock/ > +obj-$(CONFIG_IOMMU_SUPPORT) += iommu/ > obj-$(CONFIG_REMOTEPROC) += remoteproc/ > obj-$(CONFIG_RPMSG) += rpmsg/ > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c > index 1dab5f2b725b..1ca448f2b4d2 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c > @@ -589,7 +589,7 @@ static void __exit amdgpu_exit(void) > amdgpu_sync_fini(); > } > > -module_init(amdgpu_init); > +late_initcall(amdgpu_init); > module_exit(amdgpu_exit); > > MODULE_AUTHOR(DRIVER_AUTHOR); > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_module.c b/drivers/gpu/drm/amd/amdkfd/kfd_module.c > index 850a5623661f..3d1dab8a31c7 100644 > --- a/drivers/gpu/drm/amd/amdkfd/kfd_module.c > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_module.c > @@ -141,7 +141,7 @@ static void __exit kfd_module_exit(void) > dev_info(kfd_device, "Removed module\n"); > } > > -module_init(kfd_module_init); > +late_initcall(kfd_module_init); > module_exit(kfd_module_exit); > > MODULE_AUTHOR(KFD_DRIVER_AUTHOR); > diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c > index b55aa740171f..1fa1b7f3a89c 100644 > --- a/drivers/gpu/drm/radeon/radeon_drv.c > +++ b/drivers/gpu/drm/radeon/radeon_drv.c > @@ -609,7 +609,7 @@ static void __exit radeon_exit(void) > radeon_unregister_atpx_handler(); > } > > -module_init(radeon_init); > +late_initcall(radeon_init); > module_exit(radeon_exit); > > MODULE_AUTHOR(DRIVER_AUTHOR); > -- > 2.8.2 > > >> I tested this on Kaveri, and amdkfd is working. For amdkfd that's >> fine, but IMO that's not enough testing for radeon/amdgpu. I would >> like to hear AMD's developers take on this. > > Sure. Hopefully the above extensions suffice. In the future we should be able > to also replace the radeon amdkfd -EPROBE_DEFER kludge and the implicit > sensitivity in Makefiles over GPU drivers and IOMMUs, but a bit more work is > needed before that happens. > > Luis
On Tue, May 31, 2016 at 06:58:34PM +0200, Luis R. Rodriguez wrote: > On Sun, May 29, 2016 at 08:27:07PM +0200, Daniel Vetter wrote: > > On Fri, May 27, 2016 at 3:18 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote: > > > To get KFD support in radeon we need the following > > > initialization to happen in this order, their > > > respective driver file that has its init routine > > > listed next to it: > > > > > > 0. AMD IOMMUv1: arch/x86/kernel/pci-dma.c > > > 1. AMD IOMMUv2: drivers/iommu/amd_iommu_v2.c > > > 2. AMD KFD: drivers/gpu/drm/amd/amdkfd/kfd_module.c > > > 3. AMD Radeon: drivers/gpu/drm/radeon/radeon_drv.c > > > > > > Order is rather implicit, but these drivers can currently > > > only do so much given the amount of leg room available. > > > Below are the respective init routines and how they are > > > initialized: > > > > > > arch/x86/kernel/pci-dma.c rootfs_initcall(pci_iommu_init); > > > drivers/iommu/amd_iommu_v2.c module_init(amd_iommu_v2_init); > > > drivers/gpu/drm/amd/amdkfd/kfd_module.c module_init(kfd_module_init); > > > drivers/gpu/drm/radeon/radeon_drv.c module_init(radeon_init); > > > > > > When a driver is built-in module_init() folds to use > > > device_initcall(), and we have the following possible > > > orders: > > > > > > #define pure_initcall(fn) __define_initcall(fn, 0) > > > #define core_initcall(fn) __define_initcall(fn, 1) > > > #define postcore_initcall(fn)__define_initcall(fn, 2) > > > #define arch_initcall(fn) __define_initcall(fn, 3) > > > #define subsys_initcall(fn) __define_initcall(fn, 4) > > > #define fs_initcall(fn) __define_initcall(fn, 5) > > > --------------------------------------------------------- > > > #define rootfs_initcall(fn) __define_initcall(fn, rootfs) > > > #define device_initcall(fn) __define_initcall(fn, 6) > > > #define late_initcall(fn) __define_initcall(fn, 7) > > > > > > Since we start off from rootfs_initcall(), it gives us 3 more > > > levels of leg room to play with for order semantics, this isn't > > > enough to address all required levels of dependencies, this > > > is specially true given that AMD-KFD needs to be loaded before > > > the radeon driver -- -but this it not enforced by symbols. > > > If the AMD-KFD driver is not loaded prior to the radeon driver > > > because otherwise the radeon driver will not initialize the > > > AMD-KFD driver and you get no KFD functionality in userspace. > > > > > > Commit 1bacc894c227fad8a7 ("drivers: Move iommu/ before gpu/ in > > > Makefile") works around some of the possibe races between > > > the AMD IOMMU v2 and GPU drivers by changing the link order. > > > This is fragile, however its the bets we can do, given that > > > making the GPU drivers use late_initcall() would also implicate > > > a similar race between them. That possible race is fortunatley > > > addressed given that the drm Makefile currently has amdkfd > > > linked prior to radeon: > > > > > > drivers/gpu/drm/Makefile > > > ... > > > obj-$(CONFIG_HSA_AMD) += amd/amdkfd/ > > > obj-$(CONFIG_DRM_RADEON)+= radeon/ > > > ... > > > > > > Changing amdkfd and radeon to late_initcall() however is > > > still the right call in orde to annotate explicitly a > > > delayed dependency requirement between the GPU drivers > > > and the IOMMUs. > > > > > > We can't address the fragile nature of the link order > > > right now, but in the future that might be possible. > > > > > > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> > > > --- > > > > > > Please note, the changes to drivers/Makefile are just > > > for the sake of forcing the possible race to occur, > > > if this works well the actual [PATCH] submission will > > > skip those changes as its pointless to remove those > > > work arounds as it stands, due to the limited nature > > > of the levels available for addressing requirements. > > > > > > Also, if you are aware of further dependency hell > > > things like these -- please do let me know as I am > > > interested in looking at addressing them. > > > > This should be fixed with -EDEFER_PROBE instead. Frobbing initcall > > levels should then just be done as an optimization to avoid too much > > reprobing. > > radeon already uses -EDEFER_PROBE but it assumes that amdkfd *is* loaded first, > and only if it is already loaded can it count on getting -EDEFER_PROBE. The > radeon driver will deffer probe *iff* kgd2kfd_init() returns -EDEFER_PROBE, > which only happens when amdkfd_init_completed is no longer present. If radeon > gets linked first though the symbol fetch for kgd2kfd_init() will make it as > not-present though. So the current heuristic used does not address proper link > / load order. Part of the issue mentioned on the commit log is another race > underneath the hood with the AMD IOMMU v2 which is needed for amdkfd. The > underlying issue however really is the lack of available clear semantics for > dependencies over 3 levels here. This is solved one way or another by link > order in the Makefiles, but as I've noted so far this has been rather implicit > and fragile. The change here makes at least the order of the GPU drivers > explicitly later than the IOMMUs. The specific race between radeon and amdfkd > is solved currently through link order through the Makefiles. In the future we > maybe able to make things more explicit. Sounds like the EDEFER_PROBE handling is broken - if the module isn't set up yet but selected in Kconfig, and needed for that hw generation then it should not just silently fail. > -EDEFER_PROBE also introduces a latency on load which we should not need if we > can handle proper link / load order dependency annotations. This change is a > small part of that work, but as it the commit log also notes future further > work is possible to build stronger semantics. Some of the work I'm doing with > linker-tables may help with this in the future [0], but for now this should > help with what the semantics we have in place. > > [0] http://lkml.kernel.org/r/1455889559-9428-1-git-send-email-mcgrof@kernel.org That's what I meant with "avoiding too much reprobing". But in the end the current solution to cross-driver deps we have is EDEFER_PROBE. Fiddling with the link order is all well for optimizing stuff, but imo _way_ too fragile for correctness. -Daniel
On Tue, May 31, 2016 at 09:04:53PM +0200, Daniel Vetter wrote: > On Tue, May 31, 2016 at 06:58:34PM +0200, Luis R. Rodriguez wrote: > > On Sun, May 29, 2016 at 08:27:07PM +0200, Daniel Vetter wrote: > > > On Fri, May 27, 2016 at 3:18 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote: > > > > To get KFD support in radeon we need the following > > > > initialization to happen in this order, their > > > > respective driver file that has its init routine > > > > listed next to it: > > > > > > > > 0. AMD IOMMUv1: arch/x86/kernel/pci-dma.c > > > > 1. AMD IOMMUv2: drivers/iommu/amd_iommu_v2.c > > > > 2. AMD KFD: drivers/gpu/drm/amd/amdkfd/kfd_module.c > > > > 3. AMD Radeon: drivers/gpu/drm/radeon/radeon_drv.c > > > > > > > > Order is rather implicit, but these drivers can currently > > > > only do so much given the amount of leg room available. > > > > Below are the respective init routines and how they are > > > > initialized: > > > > > > > > arch/x86/kernel/pci-dma.c rootfs_initcall(pci_iommu_init); > > > > drivers/iommu/amd_iommu_v2.c module_init(amd_iommu_v2_init); > > > > drivers/gpu/drm/amd/amdkfd/kfd_module.c module_init(kfd_module_init); > > > > drivers/gpu/drm/radeon/radeon_drv.c module_init(radeon_init); > > > > > > > > When a driver is built-in module_init() folds to use > > > > device_initcall(), and we have the following possible > > > > orders: > > > > > > > > #define pure_initcall(fn) __define_initcall(fn, 0) > > > > #define core_initcall(fn) __define_initcall(fn, 1) > > > > #define postcore_initcall(fn)__define_initcall(fn, 2) > > > > #define arch_initcall(fn) __define_initcall(fn, 3) > > > > #define subsys_initcall(fn) __define_initcall(fn, 4) > > > > #define fs_initcall(fn) __define_initcall(fn, 5) > > > > --------------------------------------------------------- > > > > #define rootfs_initcall(fn) __define_initcall(fn, rootfs) > > > > #define device_initcall(fn) __define_initcall(fn, 6) > > > > #define late_initcall(fn) __define_initcall(fn, 7) > > > > > > > > Since we start off from rootfs_initcall(), it gives us 3 more > > > > levels of leg room to play with for order semantics, this isn't > > > > enough to address all required levels of dependencies, this > > > > is specially true given that AMD-KFD needs to be loaded before > > > > the radeon driver -- -but this it not enforced by symbols. > > > > If the AMD-KFD driver is not loaded prior to the radeon driver > > > > because otherwise the radeon driver will not initialize the > > > > AMD-KFD driver and you get no KFD functionality in userspace. > > > > > > > > Commit 1bacc894c227fad8a7 ("drivers: Move iommu/ before gpu/ in > > > > Makefile") works around some of the possibe races between > > > > the AMD IOMMU v2 and GPU drivers by changing the link order. > > > > This is fragile, however its the bets we can do, given that > > > > making the GPU drivers use late_initcall() would also implicate > > > > a similar race between them. That possible race is fortunatley > > > > addressed given that the drm Makefile currently has amdkfd > > > > linked prior to radeon: > > > > > > > > drivers/gpu/drm/Makefile > > > > ... > > > > obj-$(CONFIG_HSA_AMD) += amd/amdkfd/ > > > > obj-$(CONFIG_DRM_RADEON)+= radeon/ > > > > ... > > > > > > > > Changing amdkfd and radeon to late_initcall() however is > > > > still the right call in orde to annotate explicitly a > > > > delayed dependency requirement between the GPU drivers > > > > and the IOMMUs. > > > > > > > > We can't address the fragile nature of the link order > > > > right now, but in the future that might be possible. > > > > > > > > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> > > > > --- > > > > > > > > Please note, the changes to drivers/Makefile are just > > > > for the sake of forcing the possible race to occur, > > > > if this works well the actual [PATCH] submission will > > > > skip those changes as its pointless to remove those > > > > work arounds as it stands, due to the limited nature > > > > of the levels available for addressing requirements. > > > > > > > > Also, if you are aware of further dependency hell > > > > things like these -- please do let me know as I am > > > > interested in looking at addressing them. > > > > > > This should be fixed with -EPROBE_DEFER instead. Frobbing initcall > > > levels should then just be done as an optimization to avoid too much > > > reprobing. > > > > radeon already uses -EPROBE_DEFER but it assumes that amdkfd *is* loaded first, > > and only if it is already loaded can it count on getting -EPROBE_DEFER. The > > radeon driver will defer probe *iff* kgd2kfd_init() returns -EPROBE_DEFER, > > which only happens when amdkfd_init_completed is no longer 0. If radeon > > gets linked first though the symbol fetch for kgd2kfd_init() will make it as > > not-present though. I did some more homework and I no longer believe this was the issue. More below. > > So the current heuristic used does not address proper link > > / load order. Part of the issue mentioned on the commit log is another race > > underneath the hood with the AMD IOMMU v2 which is needed for amdkfd. The > > underlying issue however really is the lack of available clear semantics for > > dependencies over 3 levels here. This is solved one way or another by link > > order in the Makefiles, but as I've noted so far this has been rather implicit > > and fragile. The change here makes at least the order of the GPU drivers > > explicitly later than the IOMMUs. The specific race between radeon and amdfkd > > is solved currently through link order through the Makefiles. In the future we > > maybe able to make things more explicit. > > Sounds like the EPROBE_DEFER handling is broken - if the module isn't set > up yet but selected in Kconfig, and needed for that hw generation then it > should not just silently fail. Although I cannot confirm through testing, I did an under the hood inspection of symbol_request() which both radeon and amdgpu uses and have a better idea of why things where failing, it should not really be the inability to trust symbol_request() to work if link order changes between amdkfd and radeon or amdgpu, its the issue of link order also needed of the AMD IOMMU *and* amdkfd. So my above assumption here that -EPROBE_DEFER could fail should be invalid given that the real issue should have been that amdkfd was being kicked off prior to the AMD IOMMU v2, at that point kgd2kfd_init() would fail. The silent failure then was an issue not of radeon but rather higher order drivers, and in this case neither radeon nor amdgpu could address that regardless of what they do. Oded fixed this by changing the link order between all IOMMUs and GPU drivers via commit 1bacc894c227fad8a7 ("drivers: Move iommu/ before gpu/ in Makefile"). > > -EPROBE_DEFER also introduces a latency on load which we should not need if we > > can handle proper link / load order dependency annotations. This change is a > > small part of that work, but as it the commit log also notes future further > > work is possible to build stronger semantics. Some of the work I'm doing with > > linker-tables may help with this in the future [0], but for now this should > > help with what the semantics we have in place. > > > > [0] http://lkml.kernel.org/r/1455889559-9428-1-git-send-email-mcgrof@kernel.org > > That's what I meant with "avoiding too much reprobing". But in the end the > current solution to cross-driver deps we have is EPROBE_DEFER. Fiddling > with the link order is all well for optimizing stuff, but imo _way_ too > fragile for correctness. Agreed, but EPROBE_DEFER cannot ensure layers below are correct either. By moving the GPU drivers radon and amdgpu to late_initcall() we'd actually be taking one more explicit ordering step for correctness to ensure that in case the Makefile order is different in other environments at least the IOMMU and GPU driver initialization is explicitly correct. Luis
On Wed, Jun 1, 2016 at 2:11 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote: > On Tue, May 31, 2016 at 09:04:53PM +0200, Daniel Vetter wrote: >> On Tue, May 31, 2016 at 06:58:34PM +0200, Luis R. Rodriguez wrote: >> > On Sun, May 29, 2016 at 08:27:07PM +0200, Daniel Vetter wrote: >> > > On Fri, May 27, 2016 at 3:18 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote: >> > > > To get KFD support in radeon we need the following >> > > > initialization to happen in this order, their >> > > > respective driver file that has its init routine >> > > > listed next to it: >> > > > >> > > > 0. AMD IOMMUv1: arch/x86/kernel/pci-dma.c >> > > > 1. AMD IOMMUv2: drivers/iommu/amd_iommu_v2.c >> > > > 2. AMD KFD: drivers/gpu/drm/amd/amdkfd/kfd_module.c >> > > > 3. AMD Radeon: drivers/gpu/drm/radeon/radeon_drv.c >> > > > >> > > > Order is rather implicit, but these drivers can currently >> > > > only do so much given the amount of leg room available. >> > > > Below are the respective init routines and how they are >> > > > initialized: >> > > > >> > > > arch/x86/kernel/pci-dma.c rootfs_initcall(pci_iommu_init); >> > > > drivers/iommu/amd_iommu_v2.c module_init(amd_iommu_v2_init); >> > > > drivers/gpu/drm/amd/amdkfd/kfd_module.c module_init(kfd_module_init); >> > > > drivers/gpu/drm/radeon/radeon_drv.c module_init(radeon_init); >> > > > >> > > > When a driver is built-in module_init() folds to use >> > > > device_initcall(), and we have the following possible >> > > > orders: >> > > > >> > > > #define pure_initcall(fn) __define_initcall(fn, 0) >> > > > #define core_initcall(fn) __define_initcall(fn, 1) >> > > > #define postcore_initcall(fn)__define_initcall(fn, 2) >> > > > #define arch_initcall(fn) __define_initcall(fn, 3) >> > > > #define subsys_initcall(fn) __define_initcall(fn, 4) >> > > > #define fs_initcall(fn) __define_initcall(fn, 5) >> > > > --------------------------------------------------------- >> > > > #define rootfs_initcall(fn) __define_initcall(fn, rootfs) >> > > > #define device_initcall(fn) __define_initcall(fn, 6) >> > > > #define late_initcall(fn) __define_initcall(fn, 7) >> > > > >> > > > Since we start off from rootfs_initcall(), it gives us 3 more >> > > > levels of leg room to play with for order semantics, this isn't >> > > > enough to address all required levels of dependencies, this >> > > > is specially true given that AMD-KFD needs to be loaded before >> > > > the radeon driver -- -but this it not enforced by symbols. >> > > > If the AMD-KFD driver is not loaded prior to the radeon driver >> > > > because otherwise the radeon driver will not initialize the >> > > > AMD-KFD driver and you get no KFD functionality in userspace. >> > > > >> > > > Commit 1bacc894c227fad8a7 ("drivers: Move iommu/ before gpu/ in >> > > > Makefile") works around some of the possibe races between >> > > > the AMD IOMMU v2 and GPU drivers by changing the link order. >> > > > This is fragile, however its the bets we can do, given that >> > > > making the GPU drivers use late_initcall() would also implicate >> > > > a similar race between them. That possible race is fortunatley >> > > > addressed given that the drm Makefile currently has amdkfd >> > > > linked prior to radeon: >> > > > >> > > > drivers/gpu/drm/Makefile >> > > > ... >> > > > obj-$(CONFIG_HSA_AMD) += amd/amdkfd/ >> > > > obj-$(CONFIG_DRM_RADEON)+= radeon/ >> > > > ... >> > > > >> > > > Changing amdkfd and radeon to late_initcall() however is >> > > > still the right call in orde to annotate explicitly a >> > > > delayed dependency requirement between the GPU drivers >> > > > and the IOMMUs. >> > > > >> > > > We can't address the fragile nature of the link order >> > > > right now, but in the future that might be possible. >> > > > >> > > > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> >> > > > --- >> > > > >> > > > Please note, the changes to drivers/Makefile are just >> > > > for the sake of forcing the possible race to occur, >> > > > if this works well the actual [PATCH] submission will >> > > > skip those changes as its pointless to remove those >> > > > work arounds as it stands, due to the limited nature >> > > > of the levels available for addressing requirements. >> > > > >> > > > Also, if you are aware of further dependency hell >> > > > things like these -- please do let me know as I am >> > > > interested in looking at addressing them. >> > > >> > > This should be fixed with -EPROBE_DEFER instead. Frobbing initcall >> > > levels should then just be done as an optimization to avoid too much >> > > reprobing. >> > >> > radeon already uses -EPROBE_DEFER but it assumes that amdkfd *is* loaded first, >> > and only if it is already loaded can it count on getting -EPROBE_DEFER. The >> > radeon driver will defer probe *iff* kgd2kfd_init() returns -EPROBE_DEFER, >> > which only happens when amdkfd_init_completed is no longer 0. If radeon >> > gets linked first though the symbol fetch for kgd2kfd_init() will make it as >> > not-present though. > > I did some more homework and I no longer believe this was the issue. More below. > >> > So the current heuristic used does not address proper link >> > / load order. Part of the issue mentioned on the commit log is another race >> > underneath the hood with the AMD IOMMU v2 which is needed for amdkfd. The >> > underlying issue however really is the lack of available clear semantics for >> > dependencies over 3 levels here. This is solved one way or another by link >> > order in the Makefiles, but as I've noted so far this has been rather implicit >> > and fragile. The change here makes at least the order of the GPU drivers >> > explicitly later than the IOMMUs. The specific race between radeon and amdfkd >> > is solved currently through link order through the Makefiles. In the future we >> > maybe able to make things more explicit. >> >> Sounds like the EPROBE_DEFER handling is broken - if the module isn't set >> up yet but selected in Kconfig, and needed for that hw generation then it >> should not just silently fail. > > Although I cannot confirm through testing, I did an under the hood inspection > of symbol_request() which both radeon and amdgpu uses and have a better idea > of why things where failing, it should not really be the inability to trust > symbol_request() to work if link order changes between amdkfd and radeon or > amdgpu, its the issue of link order also needed of the AMD IOMMU *and* amdkfd. > So my above assumption here that -EPROBE_DEFER could fail should be > invalid given that the real issue should have been that amdkfd was being > kicked off prior to the AMD IOMMU v2, at that point kgd2kfd_init() would > fail. The silent failure then was an issue not of radeon but rather higher > order drivers, and in this case neither radeon nor amdgpu could address > that regardless of what they do. Oded fixed this by changing the link order > between all IOMMUs and GPU drivers via commit 1bacc894c227fad8a7 ("drivers: > Move iommu/ before gpu/ in Makefile"). > >> > -EPROBE_DEFER also introduces a latency on load which we should not need if we >> > can handle proper link / load order dependency annotations. This change is a >> > small part of that work, but as it the commit log also notes future further >> > work is possible to build stronger semantics. Some of the work I'm doing with >> > linker-tables may help with this in the future [0], but for now this should >> > help with what the semantics we have in place. >> > >> > [0] http://lkml.kernel.org/r/1455889559-9428-1-git-send-email-mcgrof@kernel.org >> >> That's what I meant with "avoiding too much reprobing". But in the end the >> current solution to cross-driver deps we have is EPROBE_DEFER. Fiddling >> with the link order is all well for optimizing stuff, but imo _way_ too >> fragile for correctness. > > Agreed, but EPROBE_DEFER cannot ensure layers below are correct either. By > moving the GPU drivers radon and amdgpu to late_initcall() we'd actually be > taking one more explicit ordering step for correctness to ensure that in case > the Makefile order is different in other environments at least the IOMMU and > GPU driver initialization is explicitly correct. Other than addressing more init levels in the future for more device categories in the kernel, a module section, say with MODULE_SUGGESTS() might help to enable the core to request_module() on behalf of drivers... if this seems like it could help I could try it out. Luis
diff --git a/drivers/Makefile b/drivers/Makefile index 0b6f3d60193d..0fbe3982041f 100644 --- a/drivers/Makefile +++ b/drivers/Makefile @@ -50,10 +50,7 @@ obj-$(CONFIG_RESET_CONTROLLER) += reset/ obj-y += tty/ obj-y += char/ -# iommu/ comes before gpu as gpu are using iommu controllers -obj-$(CONFIG_IOMMU_SUPPORT) += iommu/ - -# gpu/ comes after char for AGP vs DRM startup and after iommu +# gpu/ comes after char for AGP vs DRM startup obj-y += gpu/ obj-$(CONFIG_CONNECTOR) += connector/ @@ -147,6 +144,7 @@ obj-y += clk/ obj-$(CONFIG_MAILBOX) += mailbox/ obj-$(CONFIG_HWSPINLOCK) += hwspinlock/ +obj-$(CONFIG_IOMMU_SUPPORT) += iommu/ obj-$(CONFIG_REMOTEPROC) += remoteproc/ obj-$(CONFIG_RPMSG) += rpmsg/ diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_module.c b/drivers/gpu/drm/amd/amdkfd/kfd_module.c index 850a5623661f..3d1dab8a31c7 100644 --- a/drivers/gpu/drm/amd/amdkfd/kfd_module.c +++ b/drivers/gpu/drm/amd/amdkfd/kfd_module.c @@ -141,7 +141,7 @@ static void __exit kfd_module_exit(void) dev_info(kfd_device, "Removed module\n"); } -module_init(kfd_module_init); +late_initcall(kfd_module_init); module_exit(kfd_module_exit); MODULE_AUTHOR(KFD_DRIVER_AUTHOR); diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c index b55aa740171f..1fa1b7f3a89c 100644 --- a/drivers/gpu/drm/radeon/radeon_drv.c +++ b/drivers/gpu/drm/radeon/radeon_drv.c @@ -609,7 +609,7 @@ static void __exit radeon_exit(void) radeon_unregister_atpx_handler(); } -module_init(radeon_init); +late_initcall(radeon_init); module_exit(radeon_exit); MODULE_AUTHOR(DRIVER_AUTHOR);
To get KFD support in radeon we need the following initialization to happen in this order, their respective driver file that has its init routine listed next to it: 0. AMD IOMMUv1: arch/x86/kernel/pci-dma.c 1. AMD IOMMUv2: drivers/iommu/amd_iommu_v2.c 2. AMD KFD: drivers/gpu/drm/amd/amdkfd/kfd_module.c 3. AMD Radeon: drivers/gpu/drm/radeon/radeon_drv.c Order is rather implicit, but these drivers can currently only do so much given the amount of leg room available. Below are the respective init routines and how they are initialized: arch/x86/kernel/pci-dma.c rootfs_initcall(pci_iommu_init); drivers/iommu/amd_iommu_v2.c module_init(amd_iommu_v2_init); drivers/gpu/drm/amd/amdkfd/kfd_module.c module_init(kfd_module_init); drivers/gpu/drm/radeon/radeon_drv.c module_init(radeon_init); When a driver is built-in module_init() folds to use device_initcall(), and we have the following possible orders: #define pure_initcall(fn) __define_initcall(fn, 0) #define core_initcall(fn) __define_initcall(fn, 1) #define postcore_initcall(fn)__define_initcall(fn, 2) #define arch_initcall(fn) __define_initcall(fn, 3) #define subsys_initcall(fn) __define_initcall(fn, 4) #define fs_initcall(fn) __define_initcall(fn, 5) --------------------------------------------------------- #define rootfs_initcall(fn) __define_initcall(fn, rootfs) #define device_initcall(fn) __define_initcall(fn, 6) #define late_initcall(fn) __define_initcall(fn, 7) Since we start off from rootfs_initcall(), it gives us 3 more levels of leg room to play with for order semantics, this isn't enough to address all required levels of dependencies, this is specially true given that AMD-KFD needs to be loaded before the radeon driver -- -but this it not enforced by symbols. If the AMD-KFD driver is not loaded prior to the radeon driver because otherwise the radeon driver will not initialize the AMD-KFD driver and you get no KFD functionality in userspace. Commit 1bacc894c227fad8a7 ("drivers: Move iommu/ before gpu/ in Makefile") works around some of the possibe races between the AMD IOMMU v2 and GPU drivers by changing the link order. This is fragile, however its the bets we can do, given that making the GPU drivers use late_initcall() would also implicate a similar race between them. That possible race is fortunatley addressed given that the drm Makefile currently has amdkfd linked prior to radeon: drivers/gpu/drm/Makefile ... obj-$(CONFIG_HSA_AMD) += amd/amdkfd/ obj-$(CONFIG_DRM_RADEON)+= radeon/ ... Changing amdkfd and radeon to late_initcall() however is still the right call in orde to annotate explicitly a delayed dependency requirement between the GPU drivers and the IOMMUs. We can't address the fragile nature of the link order right now, but in the future that might be possible. Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> --- Please note, the changes to drivers/Makefile are just for the sake of forcing the possible race to occur, if this works well the actual [PATCH] submission will skip those changes as its pointless to remove those work arounds as it stands, due to the limited nature of the levels available for addressing requirements. Also, if you are aware of further dependency hell things like these -- please do let me know as I am interested in looking at addressing them. drivers/Makefile | 6 ++---- drivers/gpu/drm/amd/amdkfd/kfd_module.c | 2 +- drivers/gpu/drm/radeon/radeon_drv.c | 2 +- 3 files changed, 4 insertions(+), 6 deletions(-)