| Message ID | 1419108374-7020-2-git-send-email-oded.gabbay@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Dec 20, 2014 at 10:46:12PM +0200, Oded Gabbay wrote: > 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 <oded.gabbay@amd.com> > --- > drivers/gpu/drm/amd/amdkfd/kfd_module.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_module.c b/drivers/gpu/drm/amd/amdkfd/kfd_module.c > index 95d5af1..236562f 100644 > --- a/drivers/gpu/drm/amd/amdkfd/kfd_module.c > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_module.c > @@ -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
On 12/20/2014 11:25 PM, Greg KH wrote: > On Sat, Dec 20, 2014 at 10:46:12PM +0200, Oded Gabbay wrote: >> 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 <oded.gabbay@amd.com> >> --- >> drivers/gpu/drm/amd/amdkfd/kfd_module.c | 5 ++--- >> 1 file changed, 2 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_module.c b/drivers/gpu/drm/amd/amdkfd/kfd_module.c >> index 95d5af1..236562f 100644 >> --- a/drivers/gpu/drm/amd/amdkfd/kfd_module.c >> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_module.c >> @@ -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 Yeah. Sorry, I will remove it. Oded > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ >
Am 20.12.2014 um 21:46 schrieb 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 <oded.gabbay@amd.com> You should probably rather fix the dependency between the two modules to get an determined load order instead of doing such nasty workarounds. Christian. > --- > drivers/gpu/drm/amd/amdkfd/kfd_module.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_module.c b/drivers/gpu/drm/amd/amdkfd/kfd_module.c > index 95d5af1..236562f 100644 > --- a/drivers/gpu/drm/amd/amdkfd/kfd_module.c > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_module.c > @@ -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)) {
On 12/21/2014 01:27 PM, Christian König wrote: > Am 20.12.2014 um 21:46 schrieb 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 <oded.gabbay@amd.com> > > 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 > >> --- >> drivers/gpu/drm/amd/amdkfd/kfd_module.c | 5 ++--- >> 1 file changed, 2 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_module.c >> b/drivers/gpu/drm/amd/amdkfd/kfd_module.c >> index 95d5af1..236562f 100644 >> --- a/drivers/gpu/drm/amd/amdkfd/kfd_module.c >> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_module.c >> @@ -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)) { >
Am 21.12.2014 um 12:34 schrieb Oded Gabbay: > > > On 12/21/2014 01:27 PM, Christian König wrote: >> Am 20.12.2014 um 21:46 schrieb 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 <oded.gabbay@amd.com> >> >> 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. > > 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? > 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. > > Oded >> >>> --- >>> drivers/gpu/drm/amd/amdkfd/kfd_module.c | 5 ++--- >>> 1 file changed, 2 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_module.c >>> b/drivers/gpu/drm/amd/amdkfd/kfd_module.c >>> index 95d5af1..236562f 100644 >>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_module.c >>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_module.c >>> @@ -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)) { >>
On 12/21/2014 02:19 PM, Christian König wrote: > Am 21.12.2014 um 12:34 schrieb Oded Gabbay: >> >> >> On 12/21/2014 01:27 PM, Christian König wrote: >>> Am 20.12.2014 um 21:46 schrieb 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 <oded.gabbay@amd.com> >>> >>> 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. 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 > >> >> 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 ? > > As far as I understand you the desired init order is radeon and amd_iommu_v2 > first 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, > amd_iommu_v2 and amdkfd blacklisted for automatically load and only load amdkfd > manually? As said above, that's ok. > >> 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. 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. > > Christian. > >> >> Oded >>> >>>> --- >>>> drivers/gpu/drm/amd/amdkfd/kfd_module.c | 5 ++--- >>>> 1 file changed, 2 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_module.c >>>> b/drivers/gpu/drm/amd/amdkfd/kfd_module.c >>>> index 95d5af1..236562f 100644 >>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_module.c >>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_module.c >>>> @@ -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)) { >>> >
On 12/21/2014 03:06 PM, Oded Gabbay wrote: > > > On 12/21/2014 02:19 PM, Christian König wrote: >> Am 21.12.2014 um 12:34 schrieb Oded Gabbay: >>> >>> >>> On 12/21/2014 01:27 PM, Christian König wrote: >>>> Am 20.12.2014 um 21:46 schrieb 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 <oded.gabbay@amd.com> >>>> >>>> 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. > 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 > 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 > > >> >>> >>> 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 ? > >> >> As far as I understand you the desired init order is radeon and amd_iommu_v2 >> first 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, >> amd_iommu_v2 and amdkfd blacklisted for automatically load and only load amdkfd >> manually? > As said above, that's ok. >> >>> 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. > 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. > >> >> Christian. >> >>> >>> Oded >>>> >>>>> --- >>>>> drivers/gpu/drm/amd/amdkfd/kfd_module.c | 5 ++--- >>>>> 1 file changed, 2 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_module.c >>>>> b/drivers/gpu/drm/amd/amdkfd/kfd_module.c >>>>> index 95d5af1..236562f 100644 >>>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_module.c >>>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_module.c >>>>> @@ -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 > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
> 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. > 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: > > > On 12/21/2014 03:06 PM, Oded Gabbay wrote: >> >> >> On 12/21/2014 02:19 PM, Christian König wrote: >>> Am 21.12.2014 um 12:34 schrieb Oded Gabbay: >>>> >>>> >>>> On 12/21/2014 01:27 PM, Christian König wrote: >>>>> Am 20.12.2014 um 21:46 schrieb 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 <oded.gabbay@amd.com> >>>>> >>>>> 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. >> 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 >> >> > > 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 >> >> >>> >>>> >>>> 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 ? >> >>> >>> As far as I understand you the desired init order is radeon and >>> amd_iommu_v2 >>> first 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, >>> amd_iommu_v2 and amdkfd blacklisted for automatically load and only >>> load amdkfd >>> manually? >> As said above, that's ok. >>> >>>> 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. >> 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. >> >>> >>> Christian. >>> >>>> >>>> Oded >>>>> >>>>>> --- >>>>>> drivers/gpu/drm/amd/amdkfd/kfd_module.c | 5 ++--- >>>>>> 1 file changed, 2 insertions(+), 3 deletions(-) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_module.c >>>>>> b/drivers/gpu/drm/amd/amdkfd/kfd_module.c >>>>>> index 95d5af1..236562f 100644 >>>>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_module.c >>>>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_module.c >>>>>> @@ -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 >> dri-devel@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/dri-devel
On 12/21/2014 05:57 PM, Christian König wrote: >> 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. > >> 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 > > Christian. > > Am 21.12.2014 um 14:24 schrieb Oded Gabbay: >> >> >> On 12/21/2014 03:06 PM, Oded Gabbay wrote: >>> >>> >>> On 12/21/2014 02:19 PM, Christian König wrote: >>>> Am 21.12.2014 um 12:34 schrieb Oded Gabbay: >>>>> >>>>> >>>>> On 12/21/2014 01:27 PM, Christian König wrote: >>>>>> Am 20.12.2014 um 21:46 schrieb 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 <oded.gabbay@amd.com> >>>>>> >>>>>> 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. >>> 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 >>> >>> >> >> 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 >>> >>> >>>> >>>>> >>>>> 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 ? >>> >>>> >>>> As far as I understand you the desired init order is radeon and amd_iommu_v2 >>>> first 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, >>>> amd_iommu_v2 and amdkfd blacklisted for automatically load and only load amdkfd >>>> manually? >>> As said above, that's ok. >>>> >>>>> 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. >>> 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. >>> >>>> >>>> Christian. >>>> >>>>> >>>>> Oded >>>>>> >>>>>>> --- >>>>>>> drivers/gpu/drm/amd/amdkfd/kfd_module.c | 5 ++--- >>>>>>> 1 file changed, 2 insertions(+), 3 deletions(-) >>>>>>> >>>>>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_module.c >>>>>>> b/drivers/gpu/drm/amd/amdkfd/kfd_module.c >>>>>>> index 95d5af1..236562f 100644 >>>>>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_module.c >>>>>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_module.c >>>>>>> @@ -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 >>> dri-devel@lists.freedesktop.org >>> 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 majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/
Am 21.12.2014 um 17:03 schrieb Oded Gabbay: > > > On 12/21/2014 05:57 PM, Christian König wrote: >>> 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. >> >>> 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. > > Oded >> >> Christian. >> >> Am 21.12.2014 um 14:24 schrieb Oded Gabbay: >>> >>> >>> On 12/21/2014 03:06 PM, Oded Gabbay wrote: >>>> >>>> >>>> On 12/21/2014 02:19 PM, Christian König wrote: >>>>> Am 21.12.2014 um 12:34 schrieb Oded Gabbay: >>>>>> >>>>>> >>>>>> On 12/21/2014 01:27 PM, Christian König wrote: >>>>>>> Am 20.12.2014 um 21:46 schrieb 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 <oded.gabbay@amd.com> >>>>>>> >>>>>>> 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. >>>> 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 >>>> >>>> >>>> >>> >>> 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 >>>> >>>> >>>>> >>>>>> >>>>>> 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 ? >>>> >>>>> >>>>> As far as I understand you the desired init order is radeon and >>>>> amd_iommu_v2 >>>>> first 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, >>>>> amd_iommu_v2 and amdkfd blacklisted for automatically load and >>>>> only load amdkfd >>>>> manually? >>>> As said above, that's ok. >>>>> >>>>>> 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. >>>> 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. >>>> >>>>> >>>>> Christian. >>>>> >>>>>> >>>>>> Oded >>>>>>> >>>>>>>> --- >>>>>>>> drivers/gpu/drm/amd/amdkfd/kfd_module.c | 5 ++--- >>>>>>>> 1 file changed, 2 insertions(+), 3 deletions(-) >>>>>>>> >>>>>>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_module.c >>>>>>>> b/drivers/gpu/drm/amd/amdkfd/kfd_module.c >>>>>>>> index 95d5af1..236562f 100644 >>>>>>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_module.c >>>>>>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_module.c >>>>>>>> @@ -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 >>>> dri-devel@lists.freedesktop.org >>>> 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 majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> Please read the FAQ at http://www.tux.org/lkml/
On 12/21/2014 06:10 PM, Christian König wrote: > Am 21.12.2014 um 17:03 schrieb Oded Gabbay: >> >> >> On 12/21/2014 05:57 PM, Christian König wrote: >>>> 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. >>> >>>> 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. > 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 > Christian. > >> >> Oded >>> >>> Christian. >>> >>> Am 21.12.2014 um 14:24 schrieb Oded Gabbay: >>>> >>>> >>>> On 12/21/2014 03:06 PM, Oded Gabbay wrote: >>>>> >>>>> >>>>> On 12/21/2014 02:19 PM, Christian König wrote: >>>>>> Am 21.12.2014 um 12:34 schrieb Oded Gabbay: >>>>>>> >>>>>>> >>>>>>> On 12/21/2014 01:27 PM, Christian König wrote: >>>>>>>> Am 20.12.2014 um 21:46 schrieb 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 <oded.gabbay@amd.com> >>>>>>>> >>>>>>>> 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. >>>>> 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 >>>>> >>>>> >>>>> >>>> >>>> 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 >>>>> >>>>> >>>>>> >>>>>>> >>>>>>> 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 ? >>>>> >>>>>> >>>>>> As far as I understand you the desired init order is radeon and amd_iommu_v2 >>>>>> first 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, >>>>>> amd_iommu_v2 and amdkfd blacklisted for automatically load and only load >>>>>> amdkfd >>>>>> manually? >>>>> As said above, that's ok. >>>>>> >>>>>>> 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. >>>>> 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. >>>>> >>>>>> >>>>>> Christian. >>>>>> >>>>>>> >>>>>>> Oded >>>>>>>> >>>>>>>>> --- >>>>>>>>> drivers/gpu/drm/amd/amdkfd/kfd_module.c | 5 ++--- >>>>>>>>> 1 file changed, 2 insertions(+), 3 deletions(-) >>>>>>>>> >>>>>>>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_module.c >>>>>>>>> b/drivers/gpu/drm/amd/amdkfd/kfd_module.c >>>>>>>>> index 95d5af1..236562f 100644 >>>>>>>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_module.c >>>>>>>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_module.c >>>>>>>>> @@ -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 >>>>> dri-devel@lists.freedesktop.org >>>>> 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 majordomo@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>> Please read the FAQ at http://www.tux.org/lkml/ >
>>>>> 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. >>>> >>>> >>>>> 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. >> > 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. 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.
On 12/22/2014 09:40 AM, Dave Airlie wrote: >>>>>> 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. >>>>> >>>>> >>>>>> 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. >>> >> 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. > > 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
Am 22.12.2014 um 08:43 schrieb Oded Gabbay: > > > On 12/22/2014 09:40 AM, Dave Airlie wrote: >>>>>>> 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. >>>>>> >>>>>> >>>>>>> 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. >>>> >>> 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. >> >> 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 ? 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. > > Oded
On 12/22/2014 10:57 AM, Christian König wrote: > Am 22.12.2014 um 08:43 schrieb Oded Gabbay: >> >> >> On 12/22/2014 09:40 AM, Dave Airlie wrote: >>>>>>>> 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. >>>>>>> >>>>>>> >>>>>>>> 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. >>>>> >>>> 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. >>> >>> 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 ? > > 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. > >> >> Oded > OK, I'll try it on my machine and if it works, I will send the patch to the list. Oded
On 12/22/2014 11:26 AM, Oded Gabbay wrote: > > > On 12/22/2014 10:57 AM, Christian König wrote: >> Am 22.12.2014 um 08:43 schrieb Oded Gabbay: >>> >>> >>> On 12/22/2014 09:40 AM, Dave Airlie wrote: >>>>>>>>> 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. >>>>>>>> >>>>>>>> >>>>>>>>> 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. >>>>>> >>>>> 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. >>>> >>>> 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 ? >> >> 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. >> >>> >>> 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 > dri-devel@lists.freedesktop.org > 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
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_module.c b/drivers/gpu/drm/amd/amdkfd/kfd_module.c index 95d5af1..236562f 100644 --- a/drivers/gpu/drm/amd/amdkfd/kfd_module.c +++ b/drivers/gpu/drm/amd/amdkfd/kfd_module.c @@ -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)) {
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 <oded.gabbay@amd.com> --- drivers/gpu/drm/amd/amdkfd/kfd_module.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)