From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756069AbcEaTFB (ORCPT ); Tue, 31 May 2016 15:05:01 -0400 Received: from mail-wm0-f66.google.com ([74.125.82.66]:35730 "EHLO mail-wm0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755623AbcEaTE7 (ORCPT ); Tue, 31 May 2016 15:04:59 -0400 Date: Tue, 31 May 2016 21:04:53 +0200 From: Daniel Vetter To: "Luis R. Rodriguez" Cc: Daniel Vetter , "Luis R. Rodriguez" , vw@iommu.org, Oded Gabbay , Joerg Roedel , Christian =?iso-8859-1?Q?K=F6nig?= , "alexander.deucher@amd.com" , Dave Airlie , iommu@lists.linux-foundation.org, Linux Kernel Mailing List , dri-devel , gregkh@linuxfoundation.org, hpa@zytor.com Subject: Re: [RFT v3] drm: use late_initcall() for amdkfd and radeon Message-ID: <20160531190453.GL7231@phenom.ffwll.local> Mail-Followup-To: "Luis R. Rodriguez" , "Luis R. Rodriguez" , vw@iommu.org, Oded Gabbay , Joerg Roedel , Christian =?iso-8859-1?Q?K=F6nig?= , "alexander.deucher@amd.com" , Dave Airlie , iommu@lists.linux-foundation.org, Linux Kernel Mailing List , dri-devel , gregkh@linuxfoundation.org, hpa@zytor.com References: <20160527004642.GQ11948@wotan.suse.de> <1464311916-10065-1-git-send-email-mcgrof@kernel.org> <20160531165834.GG11948@wotan.suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160531165834.GG11948@wotan.suse.de> X-Operating-System: Linux phenom 4.6.0-rc5+ User-Agent: Mutt/1.6.0 (2016-04-01) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, May 31, 2016 at 06:58:34PM +0200, Luis R. Rodriguez wrote: > On Sun, May 29, 2016 at 08:27:07PM +0200, Daniel Vetter wrote: > > On Fri, May 27, 2016 at 3:18 AM, Luis R. Rodriguez wrote: > > > To get KFD support in radeon we need the following > > > initialization to happen in this order, their > > > respective driver file that has its init routine > > > listed next to it: > > > > > > 0. AMD IOMMUv1: arch/x86/kernel/pci-dma.c > > > 1. AMD IOMMUv2: drivers/iommu/amd_iommu_v2.c > > > 2. AMD KFD: drivers/gpu/drm/amd/amdkfd/kfd_module.c > > > 3. AMD Radeon: drivers/gpu/drm/radeon/radeon_drv.c > > > > > > Order is rather implicit, but these drivers can currently > > > only do so much given the amount of leg room available. > > > Below are the respective init routines and how they are > > > initialized: > > > > > > arch/x86/kernel/pci-dma.c rootfs_initcall(pci_iommu_init); > > > drivers/iommu/amd_iommu_v2.c module_init(amd_iommu_v2_init); > > > drivers/gpu/drm/amd/amdkfd/kfd_module.c module_init(kfd_module_init); > > > drivers/gpu/drm/radeon/radeon_drv.c module_init(radeon_init); > > > > > > When a driver is built-in module_init() folds to use > > > device_initcall(), and we have the following possible > > > orders: > > > > > > #define pure_initcall(fn) __define_initcall(fn, 0) > > > #define core_initcall(fn) __define_initcall(fn, 1) > > > #define postcore_initcall(fn)__define_initcall(fn, 2) > > > #define arch_initcall(fn) __define_initcall(fn, 3) > > > #define subsys_initcall(fn) __define_initcall(fn, 4) > > > #define fs_initcall(fn) __define_initcall(fn, 5) > > > --------------------------------------------------------- > > > #define rootfs_initcall(fn) __define_initcall(fn, rootfs) > > > #define device_initcall(fn) __define_initcall(fn, 6) > > > #define late_initcall(fn) __define_initcall(fn, 7) > > > > > > Since we start off from rootfs_initcall(), it gives us 3 more > > > levels of leg room to play with for order semantics, this isn't > > > enough to address all required levels of dependencies, this > > > is specially true given that AMD-KFD needs to be loaded before > > > the radeon driver -- -but this it not enforced by symbols. > > > If the AMD-KFD driver is not loaded prior to the radeon driver > > > because otherwise the radeon driver will not initialize the > > > AMD-KFD driver and you get no KFD functionality in userspace. > > > > > > Commit 1bacc894c227fad8a7 ("drivers: Move iommu/ before gpu/ in > > > Makefile") works around some of the possibe races between > > > the AMD IOMMU v2 and GPU drivers by changing the link order. > > > This is fragile, however its the bets we can do, given that > > > making the GPU drivers use late_initcall() would also implicate > > > a similar race between them. That possible race is fortunatley > > > addressed given that the drm Makefile currently has amdkfd > > > linked prior to radeon: > > > > > > drivers/gpu/drm/Makefile > > > ... > > > obj-$(CONFIG_HSA_AMD) += amd/amdkfd/ > > > obj-$(CONFIG_DRM_RADEON)+= radeon/ > > > ... > > > > > > Changing amdkfd and radeon to late_initcall() however is > > > still the right call in orde to annotate explicitly a > > > delayed dependency requirement between the GPU drivers > > > and the IOMMUs. > > > > > > We can't address the fragile nature of the link order > > > right now, but in the future that might be possible. > > > > > > Signed-off-by: Luis R. Rodriguez > > > --- > > > > > > Please note, the changes to drivers/Makefile are just > > > for the sake of forcing the possible race to occur, > > > if this works well the actual [PATCH] submission will > > > skip those changes as its pointless to remove those > > > work arounds as it stands, due to the limited nature > > > of the levels available for addressing requirements. > > > > > > Also, if you are aware of further dependency hell > > > things like these -- please do let me know as I am > > > interested in looking at addressing them. > > > > This should be fixed with -EDEFER_PROBE instead. Frobbing initcall > > levels should then just be done as an optimization to avoid too much > > reprobing. > > radeon already uses -EDEFER_PROBE but it assumes that amdkfd *is* loaded first, > and only if it is already loaded can it count on getting -EDEFER_PROBE. The > radeon driver will deffer probe *iff* kgd2kfd_init() returns -EDEFER_PROBE, > which only happens when amdkfd_init_completed is no longer present. If radeon > gets linked first though the symbol fetch for kgd2kfd_init() will make it as > not-present though. So the current heuristic used does not address proper link > / load order. Part of the issue mentioned on the commit log is another race > underneath the hood with the AMD IOMMU v2 which is needed for amdkfd. The > underlying issue however really is the lack of available clear semantics for > dependencies over 3 levels here. This is solved one way or another by link > order in the Makefiles, but as I've noted so far this has been rather implicit > and fragile. The change here makes at least the order of the GPU drivers > explicitly later than the IOMMUs. The specific race between radeon and amdfkd > is solved currently through link order through the Makefiles. In the future we > maybe able to make things more explicit. Sounds like the EDEFER_PROBE handling is broken - if the module isn't set up yet but selected in Kconfig, and needed for that hw generation then it should not just silently fail. > -EDEFER_PROBE also introduces a latency on load which we should not need if we > can handle proper link / load order dependency annotations. This change is a > small part of that work, but as it the commit log also notes future further > work is possible to build stronger semantics. Some of the work I'm doing with > linker-tables may help with this in the future [0], but for now this should > help with what the semantics we have in place. > > [0] http://lkml.kernel.org/r/1455889559-9428-1-git-send-email-mcgrof@kernel.org That's what I meant with "avoiding too much reprobing". But in the end the current solution to cross-driver deps we have is EDEFER_PROBE. Fiddling with the link order is all well for optimizing stuff, but imo _way_ too fragile for correctness. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch