From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Pinchart Subject: Re: [PATCH 0/2] Change order of linkage in kernel makefiles for amdkfd Date: Mon, 29 Dec 2014 10:16:31 +0200 Message-ID: <1678486.9ZXZnn5och@avalon> References: <1419246435-7050-1-git-send-email-oded.gabbay@amd.com> <1621810.aTt9O4Ghzs@avalon> <549FEB52.4020103@amd.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <549FEB52.4020103@amd.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Oded Gabbay Cc: Arnd Bergmann , Geert Uytterhoeven , Greg Kroah-Hartman , Will Deacon , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, iommu@lists.linux-foundation.org, Alexander Deucher , Christian =?ISO-8859-1?Q?K=F6nig?= List-Id: iommu@lists.linux-foundation.org SGkgT2RlZCwKCk9uIFN1bmRheSAyOCBEZWNlbWJlciAyMDE0IDEzOjM2OjUwIE9kZWQgR2FiYmF5 IHdyb3RlOgo+IE9uIDEyLzI2LzIwMTQgMTE6MTkgQU0sIExhdXJlbnQgUGluY2hhcnQgd3JvdGU6 Cj4gPiBPbiBUaHVyc2RheSAyNSBEZWNlbWJlciAyMDE0IDE0OjIwOjU5IFRoaWVycnkgUmVkaW5n IHdyb3RlOgo+ID4+IE9uIE1vbiwgRGVjIDIyLCAyMDE0IGF0IDAxOjA3OjEzUE0gKzAyMDAsIE9k ZWQgR2FiYmF5IHdyb3RlOgo+ID4+PiBUaGlzIHNtYWxsIHBhdGNoLXNldCwgd2FzIGNyZWF0ZWQg dG8gc29sdmUgdGhlIGJ1ZyBkZXNjcmliZWQgYXQKPiA+Pj4gaHR0cHM6Ly9idWd6aWxsYS5rZXJu ZWwub3JnL3Nob3dfYnVnLmNnaT9pZD04OTY2MSAoS2VybmVsIHBhbmljIHdoZW4KPiA+Pj4gdHJ5 aW5nIHVzZSBhbWRrZmQgZHJpdmVyIG9uIEthdmVyaSkuIEl0IHJlcGxhY2VzIHRoZSBwcmV2aW91 cyBwYXRjaC1zZXQKPiA+Pj4gY2FsbGVkIFtQQVRDSCAwLzNdIFVzZSB3b3JrcXVldWUgZm9yIGRl dmljZSBpbml0IGluIGFtZGtmZAo+ID4+PiAoaHR0cDovL2xpc3RzLmZyZWVkZXNrdG9wLm9yZy9h cmNoaXZlcy9kcmktZGV2ZWwvMjAxNC1EZWNlbWJlci8wNzQ0MDEuaHQKPiA+Pj4gbWwpCj4gPj4+ IAo+ID4+PiBUaGF0IGJ1ZyBhcHBlYXJzIG9ubHkgd2hlbiByYWRlb24sIGFtZGtmZCBhbmQgYW1k X2lvbW11X3YyIGFyZSBjb21waWxlZAo+ID4+PiBpbnNpZGUgdGhlIGtlcm5lbCAobm90IGFzIG1v ZHVsZXMpLiBJbiB0aGF0IGNhc2UsIHRoZSBjb3JyZWN0IGxvYWRpbmcKPiA+Pj4gb3JkZXIsIGFz IGRldGVybWluZWQgYnkgdGhlIGV4cG9ydGVkIHN5bWJvbCB1c2VkIGJ5IGVhY2ggZHJpdmVyLCBp cwo+ID4+PiBub3QgZW5mb3JjZWQgYW55bW9yZSBhbmQgdGhlIGtlcm5lbCBsb2FkcyB0aGVtIGJh c2VkIG9uIHdobyB3YXMgbGlua2VkCj4gPj4+IGZpcnN0LiBUaGF0IG1ha2VzIHJhZGVvbiBsb2Fk IGZpcnN0LCBhbWRrZmQgc2Vjb25kIGFuZCBhbWRfaW9tbXVfdjIKPiA+Pj4gdGhpcmQuCj4gPj4+ IAo+ID4+PiBCZWNhdXNlIHRoZSBpbml0aWFsaXphdGlvbiBvZiBhIGRldmljZSBpbiBhbWRrZmQg aXMgaW5pdGlhdGVkIGJ5IHJhZGVvbiwKPiA+Pj4gYW5kIGNhbiBvbmx5IGJlIGNvbXBsZXRlZCBp ZiBhbWRrZmQgYW5kIGFtZF9pb21tdV92MiB3ZXJlIGxvYWRlZCBhbmQKPiA+Pj4gaW5pdGlhbGl6 ZWQsIHRoZW4gaW4gdGhlIGNhc2UgbWVudGlvbmVkIGFib3ZlLCB0aGlzIGluaXRhbGl6YXRpb24g ZmFpbHMKPiA+Pj4gYW5kIHRoZXJlIGlzIGEga2VybmVsIHBhbmljIGFzIHNvbWUgcG9pbnRlcnMg YXJlIG5vdCBpbml0aWFsaXplZCBidXQKPiA+Pj4gdXNlZCBub250aGVsZXNzLgo+ID4+PiAKPiA+ Pj4gVG8gc29sdmUgdGhpcyBidWcsIHRoaXMgcGF0Y2gtc2V0IG1vdmVzIGlvbW11LyBiZWZvcmUg Z3B1LyBpbgo+ID4+PiBkcml2ZXJzL01ha2VmaWxlIGFuZCBhbHNvIG1vdmVzIGFtZGtmZC8gYmVm b3JlIHJhZGVvbi8gaW4KPiA+Pj4gZHJpdmVycy9ncHUvZHJtL01ha2VmaWxlLgo+ID4+PiAKPiA+ Pj4gVGhlIHJhdGlvbmFsZSBpcyB0aGF0IGluIGdlbmVyYWwsIEFNRCBHUFUgZGV2aWNlcyBhcmUg ZGVwZW5kZW50IG9uIEFNRAo+ID4+PiBJT01NVSBjb250cm9sbGVyIGZ1bmN0aW9uYWxpdHkgdG8g YWxsb3cgdGhlIEdQVSB0byBhY2Nlc3MgYSBwcm9jZXNzJ3MKPiA+Pj4gdmlydHVhbCBtZW1vcnkg YWRkcmVzcyBzcGFjZSwgd2l0aG91dCB0aGUgbmVlZCBmb3IgcGlubmluZyB0aGUgbWVtb3J5Lgo+ ID4+PiBUaGF0J3Mgd2h5IGl0IG1ha2VzIHNlbnNlIHRvIGluaXRpYWxpemUgdGhlIGlvbW11LyBz dWJzeXN0ZW0gYWhlYWQgb2YKPiA+Pj4gdGhlIGdwdS8gc3Vic3lzdGVtLgo+ID4+IAo+ID4+IEkg c3Ryb25nbHkgb2JqZWN0IHRvIHRoaXMgcGF0Y2ggc2V0LiBUaGlzIG1ha2VzIGFzc3VtcHRpb25z IGFib3V0IGhvdwo+ID4+IHRoZSBidWlsZCBzeXN0ZW0gaW5mbHVlbmNlcyBwcm9iZSBvcmRlci4g VGhhdCdzIGJhZCBiZWNhdXNlIHNlZW1pbmdseQo+ID4+IHVucmVsYXRlZCBjaGFuZ2VzIGNvdWxk IGVhc2lseSBicmVhayB0aGlzIGluIHRoZSBmdXR1cmUuCj4gPj4gCj4gPj4gV2UgYWxyZWFkeSBo YXZlIHdheXMgdG8gc29sdmUgdGhpcyBraW5kIG9mIGRlcGVuZGVuY3kgKGRyaXZlciBwcm9iZQo+ ID4+IGRlZmVycmFsKSwgYW5kIEkgdGhpbmsgeW91IHNob3VsZCBiZSB1c2luZyBpdCB0byBzb2x2 ZSB0aGlzIHBhcnRpY3VsYXIKPiA+PiBwcm9ibGVtIHJhdGhlciB0aGFuIHNvbWUgbGlua2luZyBv cmRlciBoYWNrLgo+ID4gCj4gPiBXaGlsZSBJIGFncmVlIHdpdGggeW91IHRoYXQgcHJvYmUgZGVm ZXJyYWwgaXMgdGhlIHdheSB0byBnbywgSSBiZWxpZXZlCj4gPiBsaW5rYWdlIG9yZGVyaW5nIGNh biBzdGlsbCBiZSB1c2VkIGFzIGFuIG9wdGltaXphdGlvbiB0byBhdm9pZCBkZWZlcnJpbmcKPiA+ IHByb2JlIGluIHRoZSBtb3N0IGNvbW1vbiBjYXNlcy4gSSdtIHRodXMgbm90IG9wcG9zZWQgdG8g bW92aW5nIGlvbW11Lwo+ID4gZWFybGllciBpbiBsaW5rIG9yZGVyIChwcm92aWRlZCB3ZSBjYW4g cHJvcGVybHkgdGVzdCBmb3Igc2lkZSBlZmZlY3RzLCBhcwo+ID4gdGhlIGp1bXAgaXMgcHJldHR5 IGxhcmdlKSwgYnV0IG5vdCBhcyBhIHJlcGxhY2VtZW50IGZvciBwcm9iZSBkZWZlcnJhbC4KPiAK PiBNeSB0aG91Z2h0cyBleGFjdGx5LiBJZiB0aGlzIHdhcyBzb21lIGV4dHJlbWUgdXNlIGNhc2Us IHRoYW4gaXQgd291bGQgYmUKPiBqdXN0aWZpZWQgdG8gc29sdmUgaXQgd2l0aCBwcm9iZSBkZWZl cnJhbC4gQnV0IEkgdGhpbmsgdGhhdCBmb3IgbW9zdCBjb21tb24KPiBjYXNlcywgR1BVIGFyZSBk ZXBlbmRlbnQgb24gSU9NTVUgYW5kICpub3QqIHZpY2UtdmVyc2EuCj4gCj4gQlRXLCBteSBmaXJz dCB0cnkgYXQgc29sdmluZyB0aGlzIHdhcyB0byB1c2UgcHJvYmUgZGVmZXJyYWwgKHVzaW5nCj4g d29ya3F1ZXVlKSwgYnV0IHRoZSBmZWVkYmFjayBJIGdvdCBmcm9tIENocmlzdGlhbiBhbmQgRGF2 ZSB3YXMgdGhhdCBtb3ZpbmcKPiBpb21tdS8gbGlua2FnZSBiZWZvcmUgZ3B1LyB3YXMgYSBtdWNo IG1vcmUgc2ltcGxlciBzb2x1dGlvbi4KClRvIGNsYXJpZnkgbXkgcG9zaXRpb24sIEkgYmVsaWV2 ZSBjaGFuZ2luZyB0aGUgbGluayBvcmRlciBjYW4gYmUgYSB3b3J0aHdoaWxlIApvcHRpbWl6YXRp b24sIGJ1dCBJJ20gdW5jZXJ0YWluIGFib3V0IHRoZSBsb25nIHRlcm0gdmlhYmlsaXR5IG9mIHRo YXQgY2hhbmdlIAphcyBhIGZpeC4gUHJvYmUgZGVmZXJyYWwgaGFzIGJlZW4gaW50cm9kdWNlZCBi ZWNhdXNlIG5vdCBhbGwgcHJvYmUgb3JkZXJpbmcgCmlzc3VlcyBjYW4gYmUgZml4ZWQgdGhyb3Vn aCBsaW5rIG9yZGVyaW5nLCBzbyB3ZSBzaG91bGQgZml4IHRoZSBwcm9ibGVtIApwcm9wZXJseS4K ClRoaXMgYmVpbmcgc2FpZCwgaWYgbW9kaWZ5aW5nIHRoZSBsaW5rIG9yZGVyIGNhbiBoZWxwIGZv ciBub3cgd2l0aG91dCAKaW50cm9kdWNpbmcgbmVnYXRpdmUgc2lkZSBlZmZlY3RzLCBpdCB3b3Vs ZCBvbmx5IHBvc3Rwb25lIHRoZSByZWFsIGZpeCwgc28gSSdtIApub3Qgb3Bwb3NlZCB0byBpdC4K Cj4gSW4gYWRkaXRpb24sIExpbnVzIHNhaWQgaGUgZG9lc24ndCBvYmplY3QgdG8gdGhpcyAiYmFu ZC1haWQiLiBTZWU6Cj4gaHR0cHM6Ly9sa21sLm9yZy9sa21sLzIwMTQvMTIvMjUvMTUyCj4gCj4g CU9kZWQKPiAKPiA+PiBDb2luY2lkZW50YWxseSB0aGVyZSdzIGEgc2VwYXJhdGUgdGhyZWFkIGN1 cnJlbnRseSBnb2luZyBvbiB0aGF0IGRlYWxzCj4gPj4gd2l0aCBJT01NVXMgYW5kIHByb2JlIG9y ZGVyLiBUaGUgc29sdXRpb24gYmVpbmcgd29ya2VkIG9uIGlzIGN1cnJlbnRseQo+ID4+IHNvbWV3 aGF0IEFSTS1zcGVjaWZpYywgc28gYWRkaW5nIGEgY291cGxlIG9mIGZvbGtzIGZvciB2aXNpYmls aXR5LiBJdAo+ID4+IGxvb2tzIGxpa2Ugd2UncmUgZ29pbmcgdG8gbmVlZCBzb21ldGhpbmcgbW9y ZSBnZW5lcmljIHNpbmNlIHRoaXMgaXMgYQo+ID4+IHByb2JsZW0gdGhhdCBldmVuIHRoZSAiYmln IiBhcmNoaXRlY3R1cmVzIG5lZWQgdG8gc29sdmUuCgotLSAKUmVnYXJkcywKCkxhdXJlbnQgUGlu Y2hhcnQKCl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fCmRy aS1kZXZlbCBtYWlsaW5nIGxpc3QKZHJpLWRldmVsQGxpc3RzLmZyZWVkZXNrdG9wLm9yZwpodHRw Oi8vbGlzdHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752015AbaL2IQ1 (ORCPT ); Mon, 29 Dec 2014 03:16:27 -0500 Received: from galahad.ideasonboard.com ([185.26.127.97]:32942 "EHLO galahad.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751232AbaL2IQ0 (ORCPT ); Mon, 29 Dec 2014 03:16:26 -0500 From: Laurent Pinchart To: Oded Gabbay Cc: Thierry Reding , David Airlie , Greg Kroah-Hartman , Geert Uytterhoeven , Dana Elifaz , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, Alexander Deucher , iommu@lists.linux-foundation.org, Christian =?ISO-8859-1?Q?K=F6nig?= , Arnd Bergmann , Will Deacon Subject: Re: [PATCH 0/2] Change order of linkage in kernel makefiles for amdkfd Date: Mon, 29 Dec 2014 10:16:31 +0200 Message-ID: <1678486.9ZXZnn5och@avalon> User-Agent: KMail/4.14.3 (Linux/3.16.5-gentoo; KDE/4.14.3; x86_64; ; ) In-Reply-To: <549FEB52.4020103@amd.com> References: <1419246435-7050-1-git-send-email-oded.gabbay@amd.com> <1621810.aTt9O4Ghzs@avalon> <549FEB52.4020103@amd.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Oded, On Sunday 28 December 2014 13:36:50 Oded Gabbay wrote: > On 12/26/2014 11:19 AM, Laurent Pinchart wrote: > > On Thursday 25 December 2014 14:20:59 Thierry Reding wrote: > >> On Mon, Dec 22, 2014 at 01:07:13PM +0200, Oded Gabbay wrote: > >>> This small patch-set, 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). It replaces the previous patch-set > >>> called [PATCH 0/3] Use workqueue for device init in amdkfd > >>> (http://lists.freedesktop.org/archives/dri-devel/2014-December/074401.ht > >>> ml) > >>> > >>> 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 bug, this patch-set moves iommu/ before gpu/ in > >>> drivers/Makefile and also moves amdkfd/ before radeon/ in > >>> drivers/gpu/drm/Makefile. > >>> > >>> The rationale is that in general, AMD GPU devices are dependent on AMD > >>> IOMMU controller functionality to allow the GPU to access a process's > >>> virtual memory address space, without the need for pinning the memory. > >>> That's why it makes sense to initialize the iommu/ subsystem ahead of > >>> the gpu/ subsystem. > >> > >> I strongly object to this patch set. This makes assumptions about how > >> the build system influences probe order. That's bad because seemingly > >> unrelated changes could easily break this in the future. > >> > >> We already have ways to solve this kind of dependency (driver probe > >> deferral), and I think you should be using it to solve this particular > >> problem rather than some linking order hack. > > > > While I agree with you that probe deferral is the way to go, I believe > > linkage ordering can still be used as an optimization to avoid deferring > > probe in the most common cases. I'm thus not opposed to moving iommu/ > > earlier in link order (provided we can properly test for side effects, as > > the jump is pretty large), but not as a replacement for probe deferral. > > My thoughts exactly. If this was some extreme use case, than it would be > justified to solve it with probe deferral. But I think that for most common > cases, GPU are dependent on IOMMU and *not* vice-versa. > > BTW, my first try at solving this was to use probe deferral (using > workqueue), but the feedback I got from Christian and Dave was that moving > iommu/ linkage before gpu/ was a much more simpler solution. To clarify my position, I believe changing the link order can be a worthwhile optimization, but I'm uncertain about the long term viability of that change as a fix. Probe deferral has been introduced because not all probe ordering issues can be fixed through link ordering, so we should fix the problem properly. This being said, if modifying the link order can help for now without introducing negative side effects, it would only postpone the real fix, so I'm not opposed to it. > In addition, Linus said he doesn't object to this "band-aid". See: > https://lkml.org/lkml/2014/12/25/152 > > Oded > > >> Coincidentally there's a separate thread currently going on that deals > >> with IOMMUs and probe order. The solution being worked on is currently > >> somewhat ARM-specific, so adding a couple of folks for visibility. It > >> looks like we're going to need something more generic since this is a > >> problem that even the "big" architectures need to solve. -- Regards, Laurent Pinchart