Use workqueue for device init in amdkfd
Subsystems (3)
cover Use workqueue for device init in amdkfd
This small patch-set, together with amd_iommu_v2 patch at
http://lists.linuxfoundation.org/pipermail/iommu/2014-December/011435.html
was created to solve the bug described at
https://bugzilla.kernel.org/show_bug.cgi?id=89661 (Kernel panic when trying use amdkfd driver on Kaveri).
That bug appears only when radeon, amdkfd and amd_iommu_v2 are compiled inside the kernel (not as modules). In that case, the correct loading order, as determined by the exported symbol used by each driver, is not enforced anymore and the kernel loads them based on who was linked first. That makes radeon load first, amdkfd second and amd_iommu_v2 third.
Because the initialization of a device in amdkfd is initiated by radeon, and can only be completed if amdkfd and amd_iommu_v2 were loaded and initialized, then in the case mentioned above, this initalization fails and there is a kernel panic as some pointers are not initialized but used nontheless.
To solve this problem, amdkfd now checks if both it and amd_iommu_v2 were loaded before trying to initalize the device. If not, it enqueue the work using a workqueue, which allows radeon to continue its device initialization (because radeon calls amdkfd to initalize the device). The work function schedules itself as long as amdkfd and amd_iommu_v2 were not initialized.
Detection of when the modules finished their initialization is done by a simple variable that is initialized to 1 when the module_init function is completed successfully. Other methods for detection were checked, e.g. module_is_live() and MODULE_SOFTDEP(), but they were proved not to work when all modules are compiled in the kernel image (which is the problematic scenario to begin with).
Oded
Oded Gabbay (3):
amdkfd: Don't clear *kfd2kgd on kfd_module_init
amdkfd: Track when amdkfd init is complete
amdkfd: Use workqueue for GPU init
drivers/gpu/drm/amd/amdkfd/kfd_device.c | 72 +++++++++++++++++++++++++++++++-- drivers/gpu/drm/amd/amdkfd/kfd_module.c | 14 +++++-- drivers/gpu/drm/amd/amdkfd/kfd_priv.h | 4 ++ 3 files changed, 83 insertions(+), 7 deletions(-)
Signature · 6 lines
2.1.0 _______________________________________________ dri-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/dri-devel
PATCH 1/3 amdkfd: Don't clear *kfd2kgd on kfd_module_init
When amdkfd and radeon are compiled inside the kernel image (not as modules), radeon will load before amdkfd and will set *kfd2kgd to its interface structure. Therefore, we must not set *kfd2kgd to NULL when amdkfd is loaded because it will override radeon's initialization and cause kernel BUG.
--- drivers/gpu/drm/amd/amdkfd/kfd_module.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
drivers/gpu/drm/amd/amdkfd/kfd_module.c +2 -3
@@ -34,7 +34,7 @@#define KFD_DRIVER_MINOR 7#define KFD_DRIVER_PATCHLEVEL 0 -const struct kfd2kgd_calls *kfd2kgd; +const struct kfd2kgd_calls *kfd2kgd =NULL ;static const struct kgd2kfd_calls kgd2kfd = { .exit = kgd2kfd_exit, .probe = kgd2kfd_probe,
@@ -84,14 +84,13 @@ EXPORT_SYMBOL(kgd2kfd_init);void kgd2kfd_exit (void ) { + kfd2kgd =NULL ; }static int __init kfd_module_init (void ) {int err; - kfd2kgd =NULL ; -/* Verify module parameters */ if ((sched_policy <KFD_SCHED_POLICY_HWS ) || (sched_policy >KFD_SCHED_POLICY_NO_HWS )) {
Signature · 6 lines
2.1.0 _______________________________________________ dri-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/3] amdkfd: Don't clear *kfd2kgd on kfd_module_init
Quoted 15 lines · Oded Gabbay
When amdkfd and radeon are compiled inside the kernel image (not as modules), radeon will load before amdkfd and will set *kfd2kgd to its interface structure. Therefore, we must not set *kfd2kgd to NULL when amdkfd is loaded because it will override radeon's initialization and cause kernel BUG.
Signed-off-by Oded Gabbay <[email protected]>--- drivers/gpu/drm/amd/amdkfd/kfd_module.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)drivers/gpu/drm/amd/amdkfd/kfd_module.c +0
-0
drivers/gpu/drm/amd/amdkfd/kfd_module.c line 37
Quoted 5 lines
drivers/gpu/drm/amd/amdkfd/kfd_module.c +1
-1@@ -34,7 +34,7 @@#define KFD_DRIVER_MINOR 7#define KFD_DRIVER_PATCHLEVEL 0 -const struct kfd2kgd_calls *kfd2kgd; +const struct kfd2kgd_calls *kfd2kgd =NULL ;
This change does nothing, that is what the original code did already.
thanks,
greg k-h
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/3] amdkfd: Don't clear *kfd2kgd on kfd_module_init
Quoted 19 lines · Greg KH
Quoted 18 lines · Oded Gabbay
When amdkfd and radeon are compiled inside the kernel image (not as modules), radeon will load before amdkfd and will set *kfd2kgd to its interface structure. Therefore, we must not set *kfd2kgd to NULL when amdkfd is loaded because it will override radeon's initialization and cause kernel BUG.
Signed-off-by Oded Gabbay <[email protected]>--- drivers/gpu/drm/amd/amdkfd/kfd_module.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)drivers/gpu/drm/amd/amdkfd/kfd_module.c +0
-0@@ -34,7 +34,7 @@#define KFD_DRIVER_MINOR 7#define KFD_DRIVER_PATCHLEVEL 0
Quoted 8 lines
drivers/gpu/drm/amd/amdkfd/kfd_module.c line 37
Quoted 2 lines
drivers/gpu/drm/amd/amdkfd/kfd_module.c +1
-1-const struct kfd2kgd_calls *kfd2kgd; +const struct kfd2kgd_calls *kfd2kgd =NULL ;This change does nothing, that is what the original code did already.
thanks,
greg k-h
Yeah. Sorry, I will remove it.
Oded
Quoted 6 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/3] amdkfd: Don't clear *kfd2kgd on kfd_module_init
In reply to Oded GabbayAm 20.12.2014 um 21:46 schrieb Oded Gabbay:
Quoted 6 lines
When amdkfd and radeon are compiled inside the kernel image (not as modules), radeon will load before amdkfd and will set *kfd2kgd to its interface structure. Therefore, we must not set *kfd2kgd to NULL when amdkfd is loaded because it will override radeon's initialization and cause kernel BUG.
Signed-off-by Oded Gabbay <[email protected]>
You should probably rather fix the dependency between the two modules to get an determined load order instead of doing such nasty workarounds.
Christian.
Quoted 23 lines
--- drivers/gpu/drm/amd/amdkfd/kfd_module.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)drivers/gpu/drm/amd/amdkfd/kfd_module.c +2
-1@@ -34,7 +34,7 @@#define KFD_DRIVER_MINOR 7#define KFD_DRIVER_PATCHLEVEL 0 -const struct kfd2kgd_calls *kfd2kgd; +const struct kfd2kgd_calls *kfd2kgd =NULL ;static const struct kgd2kfd_calls kgd2kfd = { .exit = kgd2kfd_exit, .probe = kgd2kfd_probe,@@ -84,14 +84,13 @@ EXPORT_SYMBOL(kgd2kfd_init);void kgd2kfd_exit (void ) { + kfd2kgd =NULL ; }
drivers/gpu/drm/amd/amdkfd/kfd_module.c EXPORT_SYMBOL(kgd2kfd_init); · line 96
Quoted 10 lines
drivers/gpu/drm/amd/amdkfd/kfd_module.c +0
-2static int __init kfd_module_init (void ) {int err; - kfd2kgd =NULL ; -/* Verify module parameters */ if ((sched_policy <KFD_SCHED_POLICY_HWS ) || (sched_policy >KFD_SCHED_POLICY_NO_HWS )) {
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/3] amdkfd: Don't clear *kfd2kgd on kfd_module_init
Quoted 6 lines · Christian König
Am 20.12.2014 um 21:46 schrieb Oded Gabbay:
Quoted 5 lines
When amdkfd and radeon are compiled inside the kernel image (not as modules), radeon will load before amdkfd and will set *kfd2kgd to its interface structure. Therefore, we must not set *kfd2kgd to NULL when amdkfd is loaded because it will override radeon's initialization and cause kernel BUG.
Quoted 6 lines
Quoted 1 line
Signed-off-by Oded Gabbay <[email protected]>You should probably rather fix the dependency between the two modules to get an determined load order instead of doing such nasty workarounds.
Christian.
The problem is that when modules are compiled inside the kernel, there is NO determined load order and there is no mechanism to enforce that. If there is/was such a mechanism, I would of course prefer to use it.
Actually, I don't understand why the kernel doesn't enforce the order according to the use of exported symbols, like it does with modules.
There will always be dependencies between kgd (radeon) and amdkfd and between amdkfd and amd_iommu_v2. I don't think I can eliminate those dependencies, not without a very complex solution. And the fact that this complex solution occurs only in a very specific use case (all modules compiled in), makes me less inclined to do that.
So I don't see it as a "nasty workaround". I would call it just a "workaround" for a specific use case, which should be solved by a generic solution to the kernel enforcing load orders.
Oded
Quoted 23 lines
Quoted 23 lines
--- drivers/gpu/drm/amd/amdkfd/kfd_module.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)drivers/gpu/drm/amd/amdkfd/kfd_module.c +2
-1@@ -34,7 +34,7 @@#define KFD_DRIVER_MINOR 7#define KFD_DRIVER_PATCHLEVEL 0 -const struct kfd2kgd_calls *kfd2kgd; +const struct kfd2kgd_calls *kfd2kgd =NULL ;static const struct kgd2kfd_calls kgd2kfd = { .exit = kgd2kfd_exit, .probe = kgd2kfd_probe,@@ -84,14 +84,13 @@ EXPORT_SYMBOL(kgd2kfd_init);void kgd2kfd_exit (void ) { + kfd2kgd =NULL ; }
Quoted 9 lines
drivers/gpu/drm/amd/amdkfd/kfd_module.c EXPORT_SYMBOL(kgd2kfd_init); · line 96
Quoted 8 lines
drivers/gpu/drm/amd/amdkfd/kfd_module.c +0
-2static int __init kfd_module_init (void ) {int err; - kfd2kgd =NULL ; -/* Verify module parameters */ if ((sched_policy <KFD_SCHED_POLICY_HWS ) || (sched_policy >KFD_SCHED_POLICY_NO_HWS )) {
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/3] amdkfd: Don't clear *kfd2kgd on kfd_module_init
Am 21.12.2014 um 12:34 schrieb Oded Gabbay:
Quoted 9 lines · Christian König
Quoted 6 lines
Am 20.12.2014 um 21:46 schrieb Oded Gabbay:
Quoted 5 lines
When amdkfd and radeon are compiled inside the kernel image (not as modules), radeon will load before amdkfd and will set *kfd2kgd to its interface structure. Therefore, we must not set *kfd2kgd to NULL when amdkfd is loaded because it will override radeon's initialization and cause kernel BUG.
Quoted 8 lines
Quoted 6 lines
Quoted 1 line
Signed-off-by Oded Gabbay <[email protected]>You should probably rather fix the dependency between the two modules to get an determined load order instead of doing such nasty workarounds.
Christian.
The problem is that when modules are compiled inside the kernel, there is NO determined load order and there is no mechanism to enforce that. If there is/was such a mechanism, I would of course prefer to use it.
There should be an determined order based on the symbol use, otherwise initializing most of the kernel modules wouldn't work as expected. For example radeon depends on the drm module must be loaded before radeon is loaded.
Quoted 2 lines
Actually, I don't understand why the kernel doesn't enforce the order according to the use of exported symbols, like it does with modules.
Yeah, that's indeed rather strange. There must be something in the amdkfd code which broke that somehow.
As far as I understand you the desired init order is radeon and amd_iommu_v2 first and then amdkfd, right? So what happens when you boot with radeon, amd_iommu_v2 and amdkfd blacklisted for automatically load and only load amdkfd manually?
Quoted 3 lines
There will always be dependencies between kgd (radeon) and amdkfd and between amdkfd and
amd_iommu_v2. I don't think I can eliminate those dependencies, not without a very complex solution. And the fact that this complex solution occurs only in a very specific use case (all modules compiled in), makes me less inclined to do that.So I don't see it as a "nasty workaround". I would call it just a "workaround" for a specific use case, which should be solved by a generic solution to the kernel enforcing load orders.
The normal kernel module handling already should provide the correct init order, so I would still call this a rather nasty workaround because we couldn't find the underlying problem.
Christian.
Quoted 34 lines
Oded
Quoted 32 lines
Quoted 31 lines
--- drivers/gpu/drm/amd/amdkfd/kfd_module.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)drivers/gpu/drm/amd/amdkfd/kfd_module.c +2
-3@@ -34,7 +34,7 @@#define KFD_DRIVER_MINOR 7#define KFD_DRIVER_PATCHLEVEL 0 -const struct kfd2kgd_calls *kfd2kgd; +const struct kfd2kgd_calls *kfd2kgd =NULL ;static const struct kgd2kfd_calls kgd2kfd = { .exit = kgd2kfd_exit, .probe = kgd2kfd_probe,@@ -84,14 +84,13 @@ EXPORT_SYMBOL(kgd2kfd_init);void kgd2kfd_exit (void ) { + kfd2kgd =NULL ; }static int __init kfd_module_init (void ) {int err; - kfd2kgd =NULL ; -/* Verify module parameters */ if ((sched_policy <KFD_SCHED_POLICY_HWS ) || (sched_policy >KFD_SCHED_POLICY_NO_HWS )) {
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/3] amdkfd: Don't clear *kfd2kgd on kfd_module_init
Quoted 17 lines · Christian König
Am 21.12.2014 um 12:34 schrieb Oded Gabbay:
Quoted 16 lines · Christian König
Quoted 12 lines
Am 20.12.2014 um 21:46 schrieb Oded Gabbay:
Quoted 6 lines
When amdkfd and radeon are compiled inside the kernel image (not as modules), radeon will load before amdkfd and will set *kfd2kgd to its interface structure. Therefore, we must not set *kfd2kgd to NULL when amdkfd is loaded because it will override radeon's initialization and cause kernel BUG.
Signed-off-by Oded Gabbay <[email protected]>You should probably rather fix the dependency between the two modules to get an determined load order instead of doing such nasty workarounds.
Christian.
Quoted 7 lines
Quoted 3 lines
The problem is that when modules are compiled inside the kernel, there is NO determined load order and there is no mechanism to enforce that. If there is/was such a mechanism, I would of course prefer to use it.
There should be an determined order based on the symbol use, otherwise initializing most of the kernel modules wouldn't work as expected. For example radeon depends on the drm module must be loaded before radeon is loaded.
There should be, but when the modules are compiled in, they are loaded based on link order only, if they are in the same group, and the groups are loaded by a pre-defined order. The groups are: pure, core, postcore, arch, subsys, fs, device (which represents all the modules) and late. See init.h
So radeon, amdkfd and amd_iommu_v2 are all in device group, and in the group they are ordered by their link order.
Yes, radeon loads after drm, because drm*.o are before radeon*.o in the Makefile. See http://stackoverflow.com/questions/5669647/linux-order-of-statically-linked-module-loading
Quoted 7 lines
Quoted 4 lines
Actually, I don't understand why the kernel doesn't enforce the order according to the use of exported symbols, like it does with modules.
Yeah, that's indeed rather strange. There must be something in the amdkfd code which broke that somehow.
IMO, that's a far-fetched guess. Could you point to something more specific ?
Quoted 3 lines
As far as I understand you the desired init order is radeon and
amd_iommu_v2first and then amdkfd, right?
Actually no. The preferred order is amd_iommu_v2, amdkfd and radeon last. This is the order that happens when all three are built as modules. More accurately, radeon inits, but its init triggers amdkfd init, which triggers amd_iommu_v2 init. So before radeon reaches its probe stage, all the modules were initialized.
So what happens when you boot with radeon,
Quoted 2 lines
amd_iommu_v2and amdkfd blacklisted for automatically load and only load amdkfd manually?
As said above, that's ok.
Quoted 7 lines
Quoted 7 lines
There will always be dependencies between kgd (radeon) and amdkfd and between amdkfd and
amd_iommu_v2. I don't think I can eliminate those dependencies, not without a very complex solution. And the fact that this complex solution occurs only in a very specific use case (all modules compiled in), makes me less inclined to do that.
Quoted 7 lines
Quoted 3 lines
So I don't see it as a "nasty workaround". I would call it just a "workaround" for a specific use case, which should be solved by a generic solution to the kernel enforcing load orders.
The normal kernel module handling already should provide the correct init order, so I would still call this a rather nasty workaround because we couldn't find the underlying problem.
Well, the normal kernel module handling doesn't work when all modules are compiled in. I'm not a huge expert on this issue so I had Joerg Roedel help me analyze this (thanks Joerg) and he agreed that there is no enforcement of order in this case.
Quoted 38 lines
Christian.
Quoted 34 lines
Oded
Quoted 32 lines
Quoted 31 lines
--- drivers/gpu/drm/amd/amdkfd/kfd_module.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)drivers/gpu/drm/amd/amdkfd/kfd_module.c +2
-3@@ -34,7 +34,7 @@#define KFD_DRIVER_MINOR 7#define KFD_DRIVER_PATCHLEVEL 0 -const struct kfd2kgd_calls *kfd2kgd; +const struct kfd2kgd_calls *kfd2kgd =NULL ;static const struct kgd2kfd_calls kgd2kfd = { .exit = kgd2kfd_exit, .probe = kgd2kfd_probe,@@ -84,14 +84,13 @@ EXPORT_SYMBOL(kgd2kfd_init);void kgd2kfd_exit (void ) { + kfd2kgd =NULL ; }static int __init kfd_module_init (void ) {int err; - kfd2kgd =NULL ; -/* Verify module parameters */ if ((sched_policy <KFD_SCHED_POLICY_HWS ) || (sched_policy >KFD_SCHED_POLICY_NO_HWS )) {
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/3] amdkfd: Don't clear *kfd2kgd on kfd_module_init
Quoted 33 lines · Oded Gabbay
Quoted 17 lines · Christian König
Am 21.12.2014 um 12:34 schrieb Oded Gabbay:
Quoted 16 lines · Christian König
Quoted 12 lines
Am 20.12.2014 um 21:46 schrieb Oded Gabbay:
Quoted 6 lines
When amdkfd and radeon are compiled inside the kernel image (not as modules), radeon will load before amdkfd and will set *kfd2kgd to its interface structure. Therefore, we must not set *kfd2kgd to NULL when amdkfd is loaded because it will override radeon's initialization and cause kernel BUG.
Signed-off-by Oded Gabbay <[email protected]>You should probably rather fix the dependency between the two modules to get an determined load order instead of doing such nasty workarounds.
Christian.
Quoted 7 lines
Quoted 3 lines
The problem is that when modules are compiled inside the kernel, there is NO determined load order and there is no mechanism to enforce that. If there is/was such a mechanism, I would of course prefer to use it.
There should be an determined order based on the symbol use, otherwise initializing most of the kernel modules wouldn't work as expected. For example radeon depends on the drm module must be loaded before radeon is loaded.
There should be, but when the modules are compiled in, they are loaded based on link order only, if they are in the same group, and the groups are loaded by a pre-defined order.
The groups are: pure, core, postcore, arch, subsys, fs, device (which represents all the modules) and late. Seeinit.h
Quoted 7 lines
So radeon, amdkfd and
amd_iommu_v2are all in device group, and in the group they are ordered by their link order.Yes, radeon loads after drm, because drm*.o are before radeon*.o in the
Makefile. See
http://stackoverflow.com/questions/5669647/linux-order-of-statically-linked-module-loading
So I tried moving amdkfd inside the Makefile before radeon, and that made amdkfd load before radeon did. This solves my first problem - order between amdkfd and radeon. However, amd_iommu_v2 doesn't belong to the drm Makefile, and I don't want to move iommu before gpu, so I don't have a solution for the order between amdkfd and amd_iommu_v2.
Oded
Quoted 80 lines
Quoted 9 lines
Quoted 6 lines
Actually, I don't understand why the kernel doesn't enforce the order according to the use of exported symbols, like it does with modules.
Yeah, that's indeed rather strange. There must be something in the amdkfd code which broke that somehow.
IMO, that's a far-fetched guess. Could you point to something more specific ?
Quoted 3 lines
As far as I understand you the desired init order is radeon and
amd_iommu_v2first and then amdkfd, right?Actually no. The preferred order is
amd_iommu_v2, amdkfd and radeon last. This is the order that happens when all three are built as modules. More accurately, radeon inits, but its init triggers amdkfd init, which triggersamd_iommu_v2init. So before radeon reaches its probe stage, all the modules were initialized.So what happens when you boot with radeon,
Quoted 2 lines
amd_iommu_v2and amdkfd blacklisted for automatically load and only load amdkfd manually?As said above, that's ok.
Quoted 7 lines
Quoted 7 lines
There will always be dependencies between kgd (radeon) and amdkfd and between amdkfd and
amd_iommu_v2. I don't think I can eliminate those dependencies, not without a very complex solution. And the fact that this complex solution occurs only in a very specific use case (all modules compiled in), makes me less inclined to do that.Quoted 7 lines
Quoted 3 lines
So I don't see it as a "nasty workaround". I would call it just a "workaround" for a specific use case, which should be solved by a generic solution to the kernel enforcing load orders.
The normal kernel module handling already should provide the correct init order, so I would still call this a rather nasty workaround because we couldn't find the underlying problem.
Well, the normal kernel module handling doesn't work when all modules are compiled in. I'm not a huge expert on this issue so I had Joerg Roedel help me analyze this (thanks Joerg) and he agreed that there is no enforcement of order in this case.
Quoted 38 lines
Christian.
Quoted 34 lines
Oded
Quoted 32 lines
Quoted 31 lines
--- drivers/gpu/drm/amd/amdkfd/kfd_module.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)drivers/gpu/drm/amd/amdkfd/kfd_module.c +2
-3@@ -34,7 +34,7 @@#define KFD_DRIVER_MINOR 7#define KFD_DRIVER_PATCHLEVEL 0 -const struct kfd2kgd_calls *kfd2kgd; +const struct kfd2kgd_calls *kfd2kgd =NULL ;static const struct kgd2kfd_calls kgd2kfd = { .exit = kgd2kfd_exit, .probe = kgd2kfd_probe,@@ -84,14 +84,13 @@ EXPORT_SYMBOL(kgd2kfd_init);void kgd2kfd_exit (void ) { + kfd2kgd =NULL ; }static int __init kfd_module_init (void ) {int err; - kfd2kgd =NULL ; -/* Verify module parameters */ if ((sched_policy <KFD_SCHED_POLICY_HWS ) || (sched_policy >KFD_SCHED_POLICY_NO_HWS )) {
Quoted 4 lines
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/3] amdkfd: Don't clear *kfd2kgd on kfd_module_init
Quoted 3 lines
There should be, but when the modules are compiled in, they are loaded based on link order only, if they are in the same group, and the groups are loaded by a pre-defined order.
Is that really still up to date? I've seen effort to change that something like 10+ years ago when Rusty reworked the module system. And it is comming up on the lists again from time to time.
Quoted 1 line
I don't want to move iommu before gpu, so I don't have a solution for the order between amdkfd and
amd_iommu_v2.
Why not? That's still better than creating a kernel workqueue, scheduling one work item on it, rescheduling the task until everything is completed and you can continue.
Christian.
Am 21.12.2014 um 14:24 schrieb Oded Gabbay:
Quoted 45 lines · Oded Gabbay
Quoted 33 lines · Christian König
Quoted 17 lines
Am 21.12.2014 um 12:34 schrieb Oded Gabbay:
Quoted 16 lines · Christian König
Quoted 12 lines
Am 20.12.2014 um 21:46 schrieb Oded Gabbay:
Quoted 6 lines
When amdkfd and radeon are compiled inside the kernel image (not as modules), radeon will load before amdkfd and will set *kfd2kgd to its interface structure. Therefore, we must not set *kfd2kgd to NULL when amdkfd is loaded because it will override radeon's initialization and cause kernel BUG.
Signed-off-by Oded Gabbay <[email protected]>You should probably rather fix the dependency between the two modules to get an determined load order instead of doing such nasty workarounds.
Christian.
Quoted 7 lines
Quoted 3 lines
The problem is that when modules are compiled inside the kernel, there is NO determined load order and there is no mechanism to enforce that. If there is/was such a mechanism, I would of course prefer to use it.
There should be an determined order based on the symbol use, otherwise initializing most of the kernel modules wouldn't work as expected. For example radeon depends on the drm module must be loaded before radeon is loaded.
There should be, but when the modules are compiled in, they are loaded based on link order only, if they are in the same group, and the groups are loaded by a pre-defined order.
The groups are: pure, core, postcore, arch, subsys, fs, device (which represents all the modules) and late. Seeinit.hQuoted 8 lines
So radeon, amdkfd and
amd_iommu_v2are all in device group, and in the group they are ordered by their link order.Yes, radeon loads after drm, because drm*.o are before radeon*.o in the
Makefile. See
http://stackoverflow.com/questions/5669647/linux-order-of-statically-linked-module-loading
Quoted 87 lines
So I tried moving amdkfd inside the
Makefilebefore radeon, and that made amdkfd load before radeon did. This solves my first problem - order between amdkfd and radeon. However,amd_iommu_v2doesn't belong to the drmMakefile, and I don't want to move iommu before gpu, so I don't have a solution for the order between amdkfd andamd_iommu_v2.Oded
Quoted 84 lines
Quoted 9 lines
Quoted 6 lines
Actually, I don't understand why the kernel doesn't enforce the order according to the use of exported symbols, like it does with modules.
Yeah, that's indeed rather strange. There must be something in the amdkfd code which broke that somehow.
IMO, that's a far-fetched guess. Could you point to something more specific ?
Quoted 3 lines
As far as I understand you the desired init order is radeon and
amd_iommu_v2first and then amdkfd, right?Actually no. The preferred order is
amd_iommu_v2, amdkfd and radeon last. This is the order that happens when all three are built as modules. More accurately, radeon inits, but its init triggers amdkfd init, which triggersamd_iommu_v2init. So before radeon reaches its probe stage, all the modules were initialized.So what happens when you boot with radeon,
Quoted 2 lines
amd_iommu_v2and amdkfd blacklisted for automatically load and only load amdkfd manually?As said above, that's ok.
Quoted 7 lines
Quoted 7 lines
There will always be dependencies between kgd (radeon) and amdkfd and between amdkfd and
amd_iommu_v2. I don't think I can eliminate those dependencies, not without a very complex solution. And the fact that this complex solution occurs only in a very specific use case (all modules compiled in), makes me less inclined to do that.Quoted 7 lines
Quoted 3 lines
So I don't see it as a "nasty workaround". I would call it just a "workaround" for a specific use case, which should be solved by a generic solution to the kernel enforcing load orders.
The normal kernel module handling already should provide the correct init order, so I would still call this a rather nasty workaround because we couldn't find the underlying problem.
Well, the normal kernel module handling doesn't work when all modules are compiled in. I'm not a huge expert on this issue so I had Joerg Roedel help me analyze this (thanks Joerg) and he agreed that there is no enforcement of order in this case.
Quoted 38 lines
Christian.
Quoted 34 lines
Oded
Quoted 32 lines
Quoted 31 lines
--- drivers/gpu/drm/amd/amdkfd/kfd_module.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)drivers/gpu/drm/amd/amdkfd/kfd_module.c +2
-3@@ -34,7 +34,7 @@#define KFD_DRIVER_MINOR 7#define KFD_DRIVER_PATCHLEVEL 0 -const struct kfd2kgd_calls *kfd2kgd; +const struct kfd2kgd_calls *kfd2kgd =NULL ;static const struct kgd2kfd_calls kgd2kfd = { .exit = kgd2kfd_exit, .probe = kgd2kfd_probe,@@ -84,14 +84,13 @@ EXPORT_SYMBOL(kgd2kfd_init);void kgd2kfd_exit (void ) { + kfd2kgd =NULL ; }static int __init kfd_module_init (void ) {int err; - kfd2kgd =NULL ; -/* Verify module parameters */ if ((sched_policy <KFD_SCHED_POLICY_HWS ) || (sched_policy >KFD_SCHED_POLICY_NO_HWS )) {_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/3] amdkfd: Don't clear *kfd2kgd on kfd_module_init
Quoted 6 lines · Christian König
Quoted 3 lines
There should be, but when the modules are compiled in, they are loaded based on link order only, if they are in the same group, and the groups are loaded by a pre-defined order.
Is that really still up to date? I've seen effort to change that something like 10+ years ago when Rusty reworked the module system. And it is comming up on the lists again from time to time.
From what I can see in the Makefile rules, code and google, yes, that's still the situation. If someone will prove me wrong I will be more than happy to correct my code.
Quoted 6 lines
Quoted 3 lines
I don't want to move iommu before gpu, so I don't have a solution for the order between amdkfd and
amd_iommu_v2.Why not? That's still better than creating a kernel workqueue, scheduling one work item on it, rescheduling the task until everything is completed and you can continue.
Because I don't know the consequences of moving an entire subsystem in front of another one. In addition, even if everyone agrees, I'm pretty sure that Linus won't be happy to do that in -rc stages. So maybe this is something to consider for 3.20 merge window, but I would still like to provide a solution for 3.19.
Oded
Quoted 145 lines
Christian.
Am 21.12.2014 um 14:24 schrieb Oded Gabbay:
Quoted 47 lines · Oded Gabbay
Quoted 35 lines · Christian König
Quoted 19 lines
Am 21.12.2014 um 12:34 schrieb Oded Gabbay:
Quoted 18 lines · Christian König
Quoted 14 lines
Am 20.12.2014 um 21:46 schrieb Oded Gabbay:
Quoted 7 lines
When amdkfd and radeon are compiled inside the kernel image (not as modules),
radeon will load before amdkfd and will set *kfd2kgd to its interface structure. Therefore, we must not set *kfd2kgd to NULL when amdkfd is loaded because it will override radeon's initialization and cause kernel BUG.Signed-off-by Oded Gabbay <[email protected]>You should probably rather fix the dependency between the two modules to get an
determined load order instead of doing such nasty workarounds.Christian.
Quoted 7 lines
Quoted 3 lines
The problem is that when modules are compiled inside the kernel, there is NO determined load order and there is no mechanism to enforce that. If there is/was such a mechanism, I would of course prefer to use it.
There should be an determined order based on the symbol use, otherwise initializing most of the kernel modules wouldn't work as expected. For example radeon depends on the drm module must be loaded before radeon is loaded.
There should be, but when the modules are compiled in, they are loaded based on link order only, if they are in the same group, and the groups are loaded by a pre-defined order.
The groups are: pure, core, postcore, arch, subsys, fs, device (which represents all the modules) and late. Seeinit.hQuoted 8 lines
So radeon, amdkfd and
amd_iommu_v2are all in device group, and in the group they are ordered by their link order.Yes, radeon loads after drm, because drm*.o are before radeon*.o in the
Makefile. See
http://stackoverflow.com/questions/5669647/linux-order-of-statically-linked-module-loadingQuoted 93 lines
So I tried moving amdkfd inside the
Makefilebefore radeon, and that made amdkfd load before radeon did. This solves my first problem - order between amdkfd and radeon. However,amd_iommu_v2doesn't belong to the drmMakefile, and I don't want to move iommu before gpu, so I don't have a solution for the order between amdkfd andamd_iommu_v2.Oded
Quoted 86 lines
Quoted 9 lines
Quoted 6 lines
Actually, I don't understand why the kernel doesn't enforce the order according to the use of exported symbols, like it does with modules.
Yeah, that's indeed rather strange. There must be something in the amdkfd code which broke that somehow.
IMO, that's a far-fetched guess. Could you point to something more specific ?
Quoted 3 lines
As far as I understand you the desired init order is radeon and
amd_iommu_v2first and then amdkfd, right?Actually no. The preferred order is
amd_iommu_v2, amdkfd and radeon last. This is the order that happens when all three are built as modules. More accurately, radeon inits, but its init triggers amdkfd init, which triggersamd_iommu_v2init. So before radeon reaches its probe stage, all the modules were initialized.So what happens when you boot with radeon,
Quoted 2 lines
amd_iommu_v2and amdkfd blacklisted for automatically load and only load amdkfd manually?As said above, that's ok.
Quoted 7 lines
Quoted 7 lines
There will always be dependencies between kgd (radeon) and amdkfd and between amdkfd and
amd_iommu_v2. I don't think I can eliminate those dependencies, not without a very complex solution. And the fact that this complex solution occurs only in a very specific use case (all modules compiled in), makes me less inclined to do that.Quoted 8 lines
Quoted 3 lines
So I don't see it as a "nasty workaround". I would call it just a "workaround" for a specific use case, which should be solved by a generic solution to the kernel enforcing load orders.
The normal kernel module handling already should provide the correct init order,
so I would still call this a rather nasty workaround because we couldn't find the underlying problem.Well, the normal kernel module handling doesn't work when all modules are compiled in. I'm not a huge expert on this issue so I had Joerg Roedel help me analyze this (thanks Joerg) and he agreed that there is no enforcement of order in this case.
Quoted 38 lines
Christian.
Quoted 34 lines
Oded
Quoted 32 lines
Quoted 31 lines
--- drivers/gpu/drm/amd/amdkfd/kfd_module.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)drivers/gpu/drm/amd/amdkfd/kfd_module.c +2
-3@@ -34,7 +34,7 @@#define KFD_DRIVER_MINOR 7#define KFD_DRIVER_PATCHLEVEL 0 -const struct kfd2kgd_calls *kfd2kgd; +const struct kfd2kgd_calls *kfd2kgd =NULL ;static const struct kgd2kfd_calls kgd2kfd = { .exit = kgd2kfd_exit, .probe = kgd2kfd_probe,@@ -84,14 +84,13 @@ EXPORT_SYMBOL(kgd2kfd_init);void kgd2kfd_exit (void ) { + kfd2kgd =NULL ; }static int __init kfd_module_init (void ) {int err; - kfd2kgd =NULL ; -/* Verify module parameters */ if ((sched_policy <KFD_SCHED_POLICY_HWS ) || (sched_policy >KFD_SCHED_POLICY_NO_HWS )) {_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Quoted 5 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/3] amdkfd: Don't clear *kfd2kgd on kfd_module_init
Am 21.12.2014 um 17:03 schrieb Oded Gabbay:
Quoted 9 lines · Christian König
Quoted 6 lines
Quoted 3 lines
There should be, but when the modules are compiled in, they are loaded based on link order only, if they are in the same group, and the groups are loaded by a pre-defined order.
Is that really still up to date? I've seen effort to change that something like 10+ years ago when Rusty reworked the module system. And it is comming up on the lists again from time to time.
Quoted 8 lines
From what I can see in the
Makefilerules, code and google, yes, that's still the situation. If someone will prove me wrong I will be more than happy to correct my code.Quoted 6 lines
Quoted 3 lines
I don't want to move iommu before gpu, so I don't have a solution for the order between amdkfd and
amd_iommu_v2.Why not? That's still better than creating a kernel workqueue, scheduling one work item on it, rescheduling the task until everything is completed and you can continue.
Because I don't know the consequences of moving an entire subsystem in front of another one. In addition, even if everyone agrees, I'm pretty sure that Linus won't be happy to do that in -rc stages. So maybe this is something to consider for 3.20 merge window, but I would still like to provide a solution for 3.19.
Yeah, true indeed. How about depending on everything being compiled as module for 3.19 then? Still better than having such a hack in the driver for as a temporary workaround for one release.
Christian.
Quoted 153 lines
Oded
Quoted 151 lines
Christian.
Am 21.12.2014 um 14:24 schrieb Oded Gabbay:
Quoted 141 lines · Oded Gabbay
Quoted 35 lines · Christian König
Quoted 19 lines
Am 21.12.2014 um 12:34 schrieb Oded Gabbay:
Quoted 18 lines · Christian König
Quoted 14 lines
Am 20.12.2014 um 21:46 schrieb Oded Gabbay:
Quoted 7 lines
When amdkfd and radeon are compiled inside the kernel image (not as modules),
radeon will load before amdkfd and will set *kfd2kgd to its interface structure. Therefore, we must not set *kfd2kgd to NULL when amdkfd is loaded because it will override radeon's initialization and cause kernel BUG.Signed-off-by Oded Gabbay <[email protected]>You should probably rather fix the dependency between the two modules to get an
determined load order instead of doing such nasty workarounds.Christian.
Quoted 7 lines
Quoted 3 lines
The problem is that when modules are compiled inside the kernel, there is NO determined load order and there is no mechanism to enforce that. If there is/was such a mechanism, I would of course prefer to use it.
There should be an determined order based on the symbol use, otherwise initializing most of the kernel modules wouldn't work as expected. For example radeon depends on the drm module must be loaded before radeon is loaded.
There should be, but when the modules are compiled in, they are loaded based on link order only, if they are in the same group, and the groups are loaded by a pre-defined order.
The groups are: pure, core, postcore, arch, subsys, fs, device (which represents all the modules) and late. Seeinit.hQuoted 9 lines
So radeon, amdkfd and
amd_iommu_v2are all in device group, and in the group they are ordered by their link order.Yes, radeon loads after drm, because drm*.o are before radeon*.o in the
Makefile. See
http://stackoverflow.com/questions/5669647/linux-order-of-statically-linked-module-loadingSo I tried moving amdkfd inside the
Makefilebefore radeon, and that made amdkfd load before radeon did. This solves my first problem - order between amdkfd and radeon. However,amd_iommu_v2doesn't belong to the drmMakefile, and I don't want to move iommu before gpu, so I don't have a solution for the order between amdkfd andamd_iommu_v2.Oded
Quoted 86 lines
Quoted 9 lines
Quoted 6 lines
Actually, I don't understand why the kernel doesn't enforce the order according to the use of exported symbols, like it does with modules.
Yeah, that's indeed rather strange. There must be something in the amdkfd code which broke that somehow.
IMO, that's a far-fetched guess. Could you point to something more specific ?
Quoted 3 lines
As far as I understand you the desired init order is radeon and
amd_iommu_v2first and then amdkfd, right?Actually no. The preferred order is
amd_iommu_v2, amdkfd and radeon last. This is the order that happens when all three are built as modules. More accurately, radeon inits, but its init triggers amdkfd init, which triggersamd_iommu_v2init. So before radeon reaches its probe stage, all the modules were initialized.So what happens when you boot with radeon,
Quoted 2 lines
amd_iommu_v2and amdkfd blacklisted for automatically load and only load amdkfd manually?As said above, that's ok.
Quoted 7 lines
Quoted 7 lines
There will always be dependencies between kgd (radeon) and amdkfd and between amdkfd and
amd_iommu_v2. I don't think I can eliminate those dependencies, not without a very complex solution. And the fact that this complex solution occurs only in a very specific use case (all modules compiled in), makes me less inclined to do that.Quoted 8 lines
Quoted 3 lines
So I don't see it as a "nasty workaround". I would call it just a "workaround" for a specific use case, which should be solved by a generic solution to the kernel enforcing load orders.
The normal kernel module handling already should provide the correct init order,
so I would still call this a rather nasty workaround because we couldn't find the underlying problem.Well, the normal kernel module handling doesn't work when all modules are compiled in. I'm not a huge expert on this issue so I had Joerg Roedel help me analyze this (thanks Joerg) and he agreed that there is no enforcement of order in this case.
Quoted 38 lines
Christian.
Quoted 34 lines
Oded
Quoted 32 lines
Quoted 31 lines
--- drivers/gpu/drm/amd/amdkfd/kfd_module.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)drivers/gpu/drm/amd/amdkfd/kfd_module.c +2
-3@@ -34,7 +34,7 @@#define KFD_DRIVER_MINOR 7#define KFD_DRIVER_PATCHLEVEL 0 -const struct kfd2kgd_calls *kfd2kgd; +const struct kfd2kgd_calls *kfd2kgd =NULL ;static const struct kgd2kfd_calls kgd2kfd = { .exit = kgd2kfd_exit, .probe = kgd2kfd_probe,@@ -84,14 +84,13 @@ EXPORT_SYMBOL(kgd2kfd_init);void kgd2kfd_exit (void ) { + kfd2kgd =NULL ; }static int __init kfd_module_init (void ) {int err; - kfd2kgd =NULL ; -/* Verify module parameters */ if ((sched_policy <KFD_SCHED_POLICY_HWS ) || (sched_policy >KFD_SCHED_POLICY_NO_HWS )) {_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-develSignature · 4 lines
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/3] amdkfd: Don't clear *kfd2kgd on kfd_module_init
Quoted 25 lines · Christian König
Am 21.12.2014 um 17:03 schrieb Oded Gabbay:
Quoted 15 lines · Christian König
Quoted 6 lines
Quoted 3 lines
There should be, but when the modules are compiled in, they are loaded based on link order only, if they are in the same group, and the groups are loaded by a pre-defined order.
Is that really still up to date? I've seen effort to change that something like 10+ years ago when Rusty reworked the module system. And it is comming up on the lists again from time to time.
From what I can see in the
Makefilerules, code and google, yes, that's still the situation. If someone will prove me wrong I will be more than happy to correct my code.Quoted 3 lines
Quoted 3 lines
I don't want to move iommu before gpu, so I don't have a solution for the order between amdkfd and
amd_iommu_v2.Quoted 8 lines
Quoted 3 lines
Why not? That's still better than creating a kernel workqueue, scheduling one work item on it, rescheduling the task until everything is completed and you can continue.
Because I don't know the consequences of moving an entire subsystem in front of another one. In addition, even if everyone agrees, I'm pretty sure that Linus won't be happy to do that in -rc stages. So maybe this is something to consider for 3.20 merge window, but I would still like to provide a solution for 3.19.
Quoted 4 lines
Yeah, true indeed. How about depending on everything being compiled as module for 3.19 then? Still better than having such a hack in the driver for as a temporary workaround for one release.
I thought about it, but because this problem was originally reported by a user that told us he couldn't use modules because of his setup, I decided not to. I assume there are other users out there who needs this option (compiled everything in the kernel - embedded ?), so I don't want to make their life harder.
In addition, saying it is a workaround for one release is true in case moving iommu subsystem in front of gpu subsystem is acceptable and doesn't cause other problems, unknown at this point.
Bottom line, my personal preference is to help the users _now_ and if a better fix is found in the future, change the code accordingly.
Oded
Quoted 167 lines
Christian.
Quoted 164 lines
Oded
Quoted 162 lines
Christian.
Am 21.12.2014 um 14:24 schrieb Oded Gabbay:
Quoted 52 lines · Oded Gabbay
Quoted 39 lines · Christian König
Quoted 20 lines
Am 21.12.2014 um 12:34 schrieb Oded Gabbay:
Quoted 19 lines · Christian König
Quoted 15 lines
Am 20.12.2014 um 21:46 schrieb Oded Gabbay:
Quoted 8 lines
When amdkfd and radeon are compiled inside the kernel image (not as modules),
radeon will load before amdkfd and will set *kfd2kgd to its interface structure. Therefore, we must not set *kfd2kgd to NULL when amdkfd is loaded
because it will override radeon's initialization and cause kernel BUG.Signed-off-by Oded Gabbay <[email protected]>You should probably rather fix the dependency between the two modules to get an
determined load order instead of doing such nasty workarounds.Christian.
Quoted 8 lines
Quoted 3 lines
The problem is that when modules are compiled inside the kernel, there is NO determined load order and there is no mechanism to enforce that. If there is/was such a mechanism, I would of course prefer to use it.
There should be an determined order based on the symbol use, otherwise initializing most of the kernel modules wouldn't work as expected. For example
radeon depends on the drm module must be loaded before radeon is loaded.There should be, but when the modules are compiled in, they are loaded based on
link order only, if they are in the same group, and the groups are loaded by a pre-defined order.
The groups are: pure, core, postcore, arch, subsys, fs, device (which represents
all the modules) and late. Seeinit.hQuoted 9 lines
So radeon, amdkfd and
amd_iommu_v2are all in device group, and in the group they are ordered by their link order.Yes, radeon loads after drm, because drm*.o are before radeon*.o in the
Makefile. See
http://stackoverflow.com/questions/5669647/linux-order-of-statically-linked-module-loadingQuoted 100 lines
So I tried moving amdkfd inside the
Makefilebefore radeon, and that made amdkfd load before radeon did. This solves my first problem - order between amdkfd and radeon. However,amd_iommu_v2doesn't belong to the drmMakefile, and I don't want to move iommu before gpu, so I don't have a solution for the order between amdkfd andamd_iommu_v2.Oded
Quoted 93 lines
Quoted 10 lines
Quoted 6 lines
Actually, I don't understand why the kernel doesn't enforce the order according to the use of exported symbols, like it does with modules.
Yeah, that's indeed rather strange. There must be something in the amdkfd code
which broke that somehow.IMO, that's a far-fetched guess. Could you point to something more specific ?
Quoted 3 lines
As far as I understand you the desired init order is radeon and
amd_iommu_v2first and then amdkfd, right?Actually no. The preferred order is
amd_iommu_v2, amdkfd and radeon last. This is the order that happens when all three are built as modules. More accurately,
radeon inits, but its init triggers amdkfd init, which triggersamd_iommu_v2init. So before radeon reaches its probe stage, all the modules were initialized.So what happens when you boot with radeon,
Quoted 3 lines
amd_iommu_v2and amdkfd blacklisted for automatically load and only load amdkfd
manually?As said above, that's ok.
Quoted 14 lines
Quoted 9 lines
There will always be dependencies between kgd (radeon) and amdkfd and between
amdkfd andamd_iommu_v2. I don't think I can eliminate those dependencies, not
without a very complex solution. And the fact that this complex solution occurs only in a very specific use case (all modules compiled in), makes me less inclined to do that.Quoted 4 lines
So I don't see it as a "nasty workaround". I would call it just a "workaround"
for a specific use case, which should be solved by a generic solution to the kernel enforcing load orders.Quoted 4 lines
The normal kernel module handling already should provide the correct init order,
so I would still call this a rather nasty workaround because we couldn't find the underlying problem.Well, the normal kernel module handling doesn't work when all modules are compiled in. I'm not a huge expert on this issue so I had Joerg Roedel help me analyze this (thanks Joerg) and he agreed that there is no enforcement of order
in this case.Quoted 38 lines
Christian.
Quoted 34 lines
Oded
Quoted 32 lines
Quoted 31 lines
--- drivers/gpu/drm/amd/amdkfd/kfd_module.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)drivers/gpu/drm/amd/amdkfd/kfd_module.c +2
-3@@ -34,7 +34,7 @@#define KFD_DRIVER_MINOR 7#define KFD_DRIVER_PATCHLEVEL 0 -const struct kfd2kgd_calls *kfd2kgd; +const struct kfd2kgd_calls *kfd2kgd =NULL ;static const struct kgd2kfd_calls kgd2kfd = { .exit = kgd2kfd_exit, .probe = kgd2kfd_probe,@@ -84,14 +84,13 @@ EXPORT_SYMBOL(kgd2kfd_init);void kgd2kfd_exit (void ) { + kfd2kgd =NULL ; }static int __init kfd_module_init (void ) {int err; - kfd2kgd =NULL ; -/* Verify module parameters */ if ((sched_policy <KFD_SCHED_POLICY_HWS ) || (sched_policy >KFD_SCHED_POLICY_NO_HWS )) {_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Dave Airlie
maintainerRe: [PATCH 1/3] amdkfd: Don't clear *kfd2kgd on kfd_module_init
Quoted 52 lines
Quoted 40 lines
Quoted 29 lines
Quoted 11 lines
Quoted 5 lines
There should be, but when the modules are compiled in, they are loaded based on
link order only, if they are in the same group, and the groups are loaded by a
pre-defined order.Is that really still up to date? I've seen effort to change that something like
10+ years ago when Rusty reworked the module system. And it is comming up on the
lists again from time to time.From what I can see in the
Makefilerules, code and google, yes, that's still
the situation. If someone will prove me wrong I will be more than happy to
correct my code.Quoted 11 lines
Quoted 5 lines
I don't want to move iommu before gpu, so I don't have a solution for the
order between amdkfd andamd_iommu_v2.Why not? That's still better than creating a kernel workqueue, scheduling one
work item on it, rescheduling the task until everything is completed and you can
continue.Quoted 9 lines
Because I don't know the consequences of moving an entire subsystem in front
of another one. In addition, even if everyone agrees, I'm pretty sure that
Linus won't be happy to do that in -rc stages. So maybe this is something to
consider for 3.20 merge window, but I would still like to provide a solution
for 3.19.Quoted 5 lines
Yeah, true indeed. How about depending on everything being compiled as module
for 3.19 then? Still better than having such a hack in the driver for as a temporary workaround for one release.I thought about it, but because this problem was originally reported by a user that told us he couldn't use modules because of his setup, I decided not to.
I assume there are other users out there who needs this option (compiled everything in the kernel - embedded ?), so I don't want to make their life harder.
Quoted 6 lines
In addition, saying it is a workaround for one release is true in case moving iommu subsystem in front of gpu subsystem is acceptable and doesn't cause other problems, unknown at this point.
Bottom line, my personal preference is to help the users _now_ and if a better fix is found in the future, change the code accordingly.
My guess is moving the iommu subsystem in front of the GPU would be rational.
It does seem like it would generally have a depend in that order.
Dave.
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/3] amdkfd: Don't clear *kfd2kgd on kfd_module_init
Quoted 56 lines · Dave Airlie
Quoted 56 lines
Quoted 40 lines
Quoted 29 lines
Quoted 11 lines
Quoted 5 lines
There should be, but when the modules are compiled in, they are loaded based on
link order only, if they are in the same group, and the groups are loaded by a
pre-defined order.Is that really still up to date? I've seen effort to change that something like
10+ years ago when Rusty reworked the module system. And it is comming up on the
lists again from time to time.From what I can see in the
Makefilerules, code and google, yes, that's
still
the situation. If someone will prove me wrong I will be more than happy to
correct my code.Quoted 11 lines
Quoted 5 lines
I don't want to move iommu before gpu, so I don't have a solution for the
order between amdkfd andamd_iommu_v2.Why not? That's still better than creating a kernel workqueue, scheduling one
work item on it, rescheduling the task until everything is completed and you can
continue.Quoted 9 lines
Because I don't know the consequences of moving an entire subsystem in front
of another one. In addition, even if everyone agrees, I'm pretty sure that
Linus won't be happy to do that in -rc stages. So maybe this is something to
consider for 3.20 merge window, but I would still like to provide a solution
for 3.19.Quoted 5 lines
Yeah, true indeed. How about depending on everything being compiled as module
for 3.19 then? Still better than having such a hack in the driver for as a temporary workaround for one release.I thought about it, but because this problem was originally reported by a user that told us he couldn't use modules because of his setup, I decided not to.
I assume there are other users out there who needs this option (compiled everything in the kernel - embedded ?), so I don't want to make their life harder.In addition, saying it is a workaround for one release is true in case moving iommu subsystem in front of gpu subsystem is acceptable and doesn't cause other problems, unknown at this point.
Quoted 9 lines
Quoted 2 lines
Bottom line, my personal preference is to help the users _now_ and if a better fix is found in the future, change the code accordingly.
My guess is moving the iommu subsystem in front of the GPU would be rational.
It does seem like it would generally have a depend in that order.
Dave.
Dave,
I agree, but don't you think it is too risky for -rc stages ?
If not, I can try it and if it works on KV, I can submit a patch.
But if you do think it is risky, what do you recommend for 3.19 ? Do the fix I suggested or disable build-in compilation option ?
Oded
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/3] amdkfd: Don't clear *kfd2kgd on kfd_module_init
Am 22.12.2014 um 08:43 schrieb Oded Gabbay:
Quoted 65 lines · Dave Airlie
Quoted 62 lines
Quoted 53 lines
Quoted 41 lines
Quoted 30 lines
Quoted 11 lines
Quoted 5 lines
There should be, but when the modules are compiled in, they are loaded based on
link order only, if they are in the same group, and the groups are loaded by a
pre-defined order.Is that really still up to date? I've seen effort to change that something like
10+ years ago when Rusty reworked the module system. And it is comming up on the
lists again from time to time.From what I can see in the
Makefilerules, code and google, yes,
that's
still
the situation. If someone will prove me wrong I will be more than happy to
correct my code.Quoted 11 lines
Quoted 5 lines
I don't want to move iommu before gpu, so I don't have a solution for the
order between amdkfd andamd_iommu_v2.Why not? That's still better than creating a kernel workqueue, scheduling one
work item on it, rescheduling the task until everything is completed and you can
continue.Quoted 9 lines
Because I don't know the consequences of moving an entire subsystem in front
of another one. In addition, even if everyone agrees, I'm pretty sure that
Linus won't be happy to do that in -rc stages. So maybe this is something to
consider for 3.20 merge window, but I would still like to provide a solution
for 3.19.Quoted 5 lines
Yeah, true indeed. How about depending on everything being compiled as module
for 3.19 then? Still better than having such a hack in the driver for as a temporary workaround for one release.I thought about it, but because this problem was originally reported by a user that told us he couldn't use modules because of his setup, I decided not to.
I assume there are other users out there who needs this option (compiled everything in the kernel - embedded ?), so I don't want to make their life harder.Quoted 6 lines
In addition, saying it is a workaround for one release is true in case moving iommu subsystem in front of gpu subsystem is acceptable and doesn't cause other problems, unknown at this point.
Bottom line, my personal preference is to help the users _now_ and if a better fix is found in the future, change the code accordingly.
My guess is moving the iommu subsystem in front of the GPU would be rational.
Quoted 8 lines
Quoted 4 lines
It does seem like it would generally have a depend in that order.
Dave.
Dave,
I agree, but don't you think it is too risky for -rc stages ?
If not, I can try it and if it works on KV, I can submit a patch.
But if you do think it is risky, what do you recommend for 3.19 ? Do the fix I suggested or disable build-in compilation option ?
I would say create the patch of changing the order (should be trivial), describe in detail in the commit message what this is supposed to fix and why such an severe change was done in -rc1 and submit it upstream.
We can still revert it in -rc2 if it breaks anything.
Christian.
Quoted 2 lines
Oded
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/3] amdkfd: Don't clear *kfd2kgd on kfd_module_init
Quoted 75 lines · Christian König
Am 22.12.2014 um 08:43 schrieb Oded Gabbay:
Quoted 66 lines · Dave Airlie
Quoted 63 lines
Quoted 52 lines
Quoted 40 lines
Quoted 29 lines
Quoted 11 lines
Quoted 5 lines
There should be, but when the modules are compiled in, they are loaded based on
link order only, if they are in the same group, and the groups are loaded by a
pre-defined order.Is that really still up to date? I've seen effort to change that something like
10+ years ago when Rusty reworked the module system. And it is comming up on the
lists again from time to time.From what I can see in the
Makefilerules, code and google, yes, that's
still
the situation. If someone will prove me wrong I will be more than happy to
correct my code.Quoted 11 lines
Quoted 5 lines
I don't want to move iommu before gpu, so I don't have a solution for the
order between amdkfd andamd_iommu_v2.Why not? That's still better than creating a kernel workqueue, scheduling one
work item on it, rescheduling the task until everything is completed and you can
continue.Quoted 9 lines
Because I don't know the consequences of moving an entire subsystem in front
of another one. In addition, even if everyone agrees, I'm pretty sure that
Linus won't be happy to do that in -rc stages. So maybe this is something to
consider for 3.20 merge window, but I would still like to provide a solution
for 3.19.Quoted 5 lines
Yeah, true indeed. How about depending on everything being compiled as module
for 3.19 then? Still better than having such a hack in the driver for as a temporary workaround for one release.I thought about it, but because this problem was originally reported by a user that told us he couldn't use modules because of his setup, I decided not to.
I assume there are other users out there who needs this option (compiled everything in the kernel - embedded ?), so I don't want to make their life harder.Quoted 6 lines
In addition, saying it is a workaround for one release is true in case moving iommu subsystem in front of gpu subsystem is acceptable and doesn't cause other problems, unknown at this point.
Bottom line, my personal preference is to help the users _now_ and if a better fix is found in the future, change the code accordingly.
My guess is moving the iommu subsystem in front of the GPU would be rational.
It does seem like it would generally have a depend in that order.
Quoted 7 lines
Quoted 2 lines
Dave.
Dave,
I agree, but don't you think it is too risky for -rc stages ?
If not, I can try it and if it works on KV, I can submit a patch. But if you do think it is risky, what do you recommend for 3.19 ? Do the fix I suggested or disable build-in compilation option ?
Quoted 11 lines
I would say create the patch of changing the order (should be trivial), describe in detail in the commit message what this is supposed to fix and why such an severe change was done in -rc1 and submit it upstream.
We can still revert it in -rc2 if it breaks anything.
Christian.
Quoted 2 lines
Oded
OK, I'll try it on my machine and if it works, I will send the patch to the list.
Oded
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/3] amdkfd: Don't clear *kfd2kgd on kfd_module_init
Quoted 90 lines · Oded Gabbay
Quoted 75 lines · Christian König
Am 22.12.2014 um 08:43 schrieb Oded Gabbay:
Quoted 66 lines · Dave Airlie
Quoted 63 lines
Quoted 52 lines
Quoted 40 lines
Quoted 29 lines
Quoted 11 lines
Quoted 5 lines
There should be, but when the modules are compiled in, they are loaded based on
link order only, if they are in the same group, and the groups are loaded by a
pre-defined order.Is that really still up to date? I've seen effort to change that something like
10+ years ago when Rusty reworked the module system. And it is comming up on the
lists again from time to time.From what I can see in the
Makefilerules, code and google, yes, that's
still
the situation. If someone will prove me wrong I will be more than happy to
correct my code.Quoted 11 lines
Quoted 5 lines
I don't want to move iommu before gpu, so I don't have a solution for the
order between amdkfd andamd_iommu_v2.Why not? That's still better than creating a kernel workqueue, scheduling one
work item on it, rescheduling the task until everything is completed and you can
continue.Quoted 9 lines
Because I don't know the consequences of moving an entire subsystem in front
of another one. In addition, even if everyone agrees, I'm pretty sure that
Linus won't be happy to do that in -rc stages. So maybe this is something to
consider for 3.20 merge window, but I would still like to provide a solution
for 3.19.Quoted 5 lines
Yeah, true indeed. How about depending on everything being compiled as module
for 3.19 then? Still better than having such a hack in the driver for as a temporary workaround for one release.I thought about it, but because this problem was originally reported by a user that told us he couldn't use modules because of his setup, I decided not to.
I assume there are other users out there who needs this option (compiled everything in the kernel - embedded ?), so I don't want to make their life harder.Quoted 6 lines
In addition, saying it is a workaround for one release is true in case moving iommu subsystem in front of gpu subsystem is acceptable and doesn't cause other problems, unknown at this point.
Bottom line, my personal preference is to help the users _now_ and if a better fix is found in the future, change the code accordingly.
My guess is moving the iommu subsystem in front of the GPU would be rational.
It does seem like it would generally have a depend in that order.
Quoted 7 lines
Quoted 2 lines
Dave.
Dave,
I agree, but don't you think it is too risky for -rc stages ?
If not, I can try it and if it works on KV, I can submit a patch. But if you do think it is risky, what do you recommend for 3.19 ? Do the fix I suggested or disable build-in compilation option ?Quoted 11 lines
I would say create the patch of changing the order (should be trivial), describe in detail in the commit message what this is supposed to fix and why such an severe change was done in -rc1 and submit it upstream.
We can still revert it in -rc2 if it breaks anything.
Christian.
Quoted 2 lines
Oded
Quoted 7 lines
OK, I'll try it on my machine and if it works, I will send the patch to the list.
Oded
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
So I checked it and all my HSA tests are passing on KV machine. I will send the patches today. Please discard the current patch-set.
Oded
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
PATCH 2/3 amdkfd: Track when amdkfd init is complete
In reply to Oded GabbayThis patch adds a new function to amdkfd, which returns 1 if the amdkfd initialization function has completed, and 0 otherwise.
This is necessary for the case when amdkfd and radeon are both compiled inside the kernel image (not as modules). In that case, radeon probes the existing GPU before amdkfd is even loaded. When radeon encounters an AMD GPU, it will pass that information to amdkfd. However, if amdkfd is not loaded than that call will fail and when amdkfd is eventually loaded, it won't know about that GPU.
Even if that call is delayed to a later stage, we will still need to know whether amdkfd has already been initialized.
Note that when the two modules are compiled as modules, this situation can't occur as the kernel enforces the order of loading.
--- drivers/gpu/drm/amd/amdkfd/kfd_module.c | 9 +++++++++ drivers/gpu/drm/amd/amdkfd/kfd_priv.h | 2 ++ 2 files changed, 11 insertions(+)
drivers/gpu/drm/amd/amdkfd/kfd_module.c +9 -0
@@ -60,6 +60,13 @@ module_param(max_num_of_queues_per_process, int, 0444);MODULE_PARM_DESC (max_num_of_queues_per_process,"Kernel cmdline parameter that defines the amdkfd maximum number of supported queues per process" ); +static int amdkfd_init_completed; + +int amdkfd_is_init_completed (void ) +{ +return amdkfd_init_completed; +} +bool kgd2kfd_init (unsigned interface_version,const struct kfd2kgd_calls *f2g,const struct kgd2kfd_calls **g2f)
@@ -128,6 +135,8 @@ static int __init kfd_module_init(void)dev_info (kfd_device,"Initialized module\n" ); + amdkfd_init_completed =1 ; +return 0 ; err_topology:
drivers/gpu/drm/amd/amdkfd/kfd_priv.h +2 -0
@@ -153,6 +153,8 @@ struct kfd_dev {bool interrupts_active; }; +int amdkfd_is_init_completed (void ); +/* KGD2KFD callbacks */ void kgd2kfd_exit (void );struct kfd_dev *kgd2kfd_probe (struct kgd_dev *kgd,struct pci_dev *pdev);
Signature · 6 lines
2.1.0 _______________________________________________ dri-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/dri-devel
PATCH 3/3 amdkfd: Use workqueue for GPU init
In reply to Oded GabbayWhen amd_iommu_v2, amdkfd and radeon are all compiled inside the kernel image (not as modules), radeon probes the existing GPU before amdkfd and amd_iommu_v2 are even loaded. When radeon encounters an AMD GPU, it will pass that information to amdkfd. However, that call will fail and will cause a kernel BUG.
We could poll in radeon on when amdkfd and amd_iommu_v2 have been loaded, but that would stall radeon.
Therefore, this patch moves the amdkfd part of GPU initialization to a workqueue. When radeon calls amdkfd to perform GPU related initialization, it will check if both amdkfd and amd_iommu_v2 have been loaded. If so, which is the situation when the three drivers are compiled as modules, it will call the relevant amdkfd function directly. If not, it will queue the initialization work on the workqueue. The work function will schedule itself until both amdkfd and amd_iommu_v2 have been loaded. Then, it will call the relevant amdkfd function.
The workqueue is defined per kfd_dev structure (per GPU).
--- drivers/gpu/drm/amd/amdkfd/kfd_device.c | 72 +++++++++++++++++++++++++++++++-- drivers/gpu/drm/amd/amdkfd/kfd_priv.h | 2 + 2 files changed, 70 insertions(+), 4 deletions(-)
drivers/gpu/drm/amd/amdkfd/kfd_device.c +68 -4
@@ -24,6 +24,7 @@#include <linux/bsearch.h> #include <linux/pci.h> #include <linux/slab.h> +#include <linux/sched.h> #include "kfd_priv.h" #include "kfd_device_queue_manager.h"
@@ -40,6 +41,11 @@ struct kfd_deviceid {const struct kfd_device_info *device_info; }; +struct kfd_device_init_work { +struct work_struct kfd_work; +struct kfd_dev *dev; +}; +/* Please keep this sorted by increasing device id. */ static const struct kfd_deviceid supported_devices[] = { {0x1304 , &kaveri_device_info },/* Kaveri */
@@ -99,6 +105,8 @@ struct kfd_dev *kgd2kfd_probe(struct kgd_dev *kgd, struct pci_dev *pdev) kfd->pdev = pdev; kfd->init_complete = false; + kfd->kfd_dev_wq =create_workqueue ("kfd_dev_wq" ); +return kfd; }
@@ -161,13 +169,10 @@ static void iommu_pasid_shutdown_callback(struct pci_dev *pdev, int pasid)kfd_unbind_process_from_device (dev, pasid); } -bool kgd2kfd_device_init (struct kfd_dev *kfd, -const struct kgd2kfd_shared_resources *gpu_resources) +static bool kfd_kgd_device_init (struct kfd_dev *kfd) {unsigned int size; - kfd->shared_resources = *gpu_resources; -/* calculate max size of mqds needed for queues */ size = max_num_of_processes * max_num_of_queues_per_process *
@@ -249,6 +254,63 @@ out:return kfd->init_complete; } +static void kfd_device_wq_init_device (struct work_struct *work) +{ +struct kfd_device_init_work *my_work; +struct kfd_dev *kfd; + + my_work = (struct kfd_device_init_work *) work; + + kfd = my_work->dev; + +/* +* As long as amdkfd or amd_iommu_v2 are not initialized, we +* yield the processor +*/ +while ((!amdkfd_is_init_completed ()) || + (!amd_iommu_v2_is_init_completed ())) +schedule (); + +kfd_kgd_device_init (kfd); +} + +bool kgd2kfd_device_init (struct kfd_dev *kfd, +const struct kgd2kfd_shared_resources *gpu_resources) +{ +struct kfd_device_init_work *work; + + kfd->shared_resources = *gpu_resources; + +/* +* When amd_iommu_v2, amdkfd and radeon are compiled inside the kernel, +* there is no mechanism to enforce order of loading between the +* drivers. Therefore, we need to use an explicit form of +* synchronization to know when amdkfd and amd_iommu_v2 have finished +* there initialization routines +*/ +if ((!amdkfd_is_init_completed ()) || + (!amd_iommu_v2_is_init_completed ())) { +BUG_ON (!kfd->kfd_dev_wq); + + work = (struct kfd_device_init_work *) +kmalloc (sizeof (struct kfd_device_init_work ), +GFP_ATOMIC ); + +if (!work) { +pr_err ("kfd: no memory for device work queue\n" ); +return false; + } + +INIT_WORK ((struct work_struct *) work, + kfd_device_wq_init_device); + work->dev = kfd; +queue_work (kfd->kfd_dev_wq, (struct work_struct *) work); +return true; + } + +return kfd_kgd_device_init (kfd); +} +void kgd2kfd_device_exit (struct kfd_dev *kfd) {if (kfd->init_complete) {
@@ -258,6 +320,8 @@ void kgd2kfd_device_exit(struct kfd_dev *kfd)kfd_topology_remove_device (kfd); } +flush_workqueue (kfd->kfd_dev_wq); +destroy_workqueue (kfd->kfd_dev_wq);kfree (kfd); }
drivers/gpu/drm/amd/amdkfd/kfd_priv.h +2 -0
@@ -151,6 +151,8 @@ struct kfd_dev {* from the HW ring into a SW ring. */ bool interrupts_active; + +struct workqueue_struct *kfd_dev_wq; };int amdkfd_is_init_completed (void );
Signature · 6 lines
2.1.0 _______________________________________________ dri-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/dri-devel