Re: [RFT v3] drm: use late_initcall() for amdkfd and radeon

[Oded Gabbay <oded.gabbay@gmail.com>]

On Tue, May 31, 2016 at 8:15 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> On Sun, May 29, 2016 at 05:49:17PM +0300, Oded Gabbay wrote:
>> On Fri, May 27, 2016 at 4:18 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
>> > diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c
>> > index b55aa740171f..1fa1b7f3a89c 100644
>> > --- a/drivers/gpu/drm/radeon/radeon_drv.c
>> > +++ b/drivers/gpu/drm/radeon/radeon_drv.c
>> > @@ -609,7 +609,7 @@ static void __exit radeon_exit(void)
>> >         radeon_unregister_atpx_handler();
>> >  }
>> >
>> > -module_init(radeon_init);
>> > +late_initcall(radeon_init);
>> >  module_exit(radeon_exit);
>>
>> Need to modify also amdgpu module_init
>
> Thanks, so other than considering the first two hunks are only for testing purposes
> (a proper [PATCH] will drop these hunks on the Makefile), you're suggestng this
> then, is that right?:

Yes, the below should cover the amdgpu case as well.
>
> ---
>  drivers/Makefile                        | 6 ++----
>  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 2 +-
>  drivers/gpu/drm/amd/amdkfd/kfd_module.c | 2 +-
>  drivers/gpu/drm/radeon/radeon_drv.c     | 2 +-
>  4 files changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/Makefile b/drivers/Makefile
> index 0b6f3d60193d..0fbe3982041f 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -50,10 +50,7 @@ obj-$(CONFIG_RESET_CONTROLLER)       += reset/
>  obj-y                          += tty/
>  obj-y                          += char/
>
> -# iommu/ comes before gpu as gpu are using iommu controllers
> -obj-$(CONFIG_IOMMU_SUPPORT)    += iommu/
> -
> -# gpu/ comes after char for AGP vs DRM startup and after iommu
> +# gpu/ comes after char for AGP vs DRM startup
>  obj-y                          += gpu/
>
>  obj-$(CONFIG_CONNECTOR)                += connector/
> @@ -147,6 +144,7 @@ obj-y                               += clk/
>
>  obj-$(CONFIG_MAILBOX)          += mailbox/
>  obj-$(CONFIG_HWSPINLOCK)       += hwspinlock/
> +obj-$(CONFIG_IOMMU_SUPPORT)    += iommu/
>  obj-$(CONFIG_REMOTEPROC)       += remoteproc/
>  obj-$(CONFIG_RPMSG)            += rpmsg/
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 1dab5f2b725b..1ca448f2b4d2 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -589,7 +589,7 @@ static void __exit amdgpu_exit(void)
>         amdgpu_sync_fini();
>  }
>
> -module_init(amdgpu_init);
> +late_initcall(amdgpu_init);
>  module_exit(amdgpu_exit);
>
>  MODULE_AUTHOR(DRIVER_AUTHOR);
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_module.c b/drivers/gpu/drm/amd/amdkfd/kfd_module.c
> index 850a5623661f..3d1dab8a31c7 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_module.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_module.c
> @@ -141,7 +141,7 @@ static void __exit kfd_module_exit(void)
>         dev_info(kfd_device, "Removed module\n");
>  }
>
> -module_init(kfd_module_init);
> +late_initcall(kfd_module_init);
>  module_exit(kfd_module_exit);
>
>  MODULE_AUTHOR(KFD_DRIVER_AUTHOR);
> diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c
> index b55aa740171f..1fa1b7f3a89c 100644
> --- a/drivers/gpu/drm/radeon/radeon_drv.c
> +++ b/drivers/gpu/drm/radeon/radeon_drv.c
> @@ -609,7 +609,7 @@ static void __exit radeon_exit(void)
>         radeon_unregister_atpx_handler();
>  }
>
> -module_init(radeon_init);
> +late_initcall(radeon_init);
>  module_exit(radeon_exit);
>
>  MODULE_AUTHOR(DRIVER_AUTHOR);
> --
> 2.8.2
>
>
>> I tested this on Kaveri, and amdkfd is working. For amdkfd that's
>> fine, but IMO that's not enough testing for radeon/amdgpu. I would
>> like to hear AMD's developers take on this.
>
> Sure. Hopefully the above extensions suffice. In the future we should be able
> to also replace the radeon amdkfd -EPROBE_DEFER kludge and the implicit
> sensitivity in Makefiles over GPU drivers and IOMMUs, but a bit more work is
> needed before that happens.
>
>   Luis