From mboxrd@z Thu Jan 1 00:00:00 1970 Content-Type: multipart/mixed; boundary="===============8776715536155573528==" MIME-Version: 1.0 From: Oded Gabbay To: lkp@lists.01.org Subject: Re: [PATCH v2] drm/radeon: Init amdkfd only if it was compiled Date: Mon, 29 Dec 2014 14:49:47 +0200 Message-ID: <54A14DEB.4090503@amd.com> In-Reply-To: <54A14C0C.8090401@vodafone.de> List-Id: --===============8776715536155573528== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable On 12/29/2014 02:41 PM, Christian K=C3=B6nig wrote: > Am 29.12.2014 um 09:59 schrieb Oded Gabbay: >> This patch changes the radeon_kfd_init(), which is used to initialize the >> interface between radeon and amdkfd, so the interface will be initialize= d only >> if amdkfd was build, either as module or inside the kernel image. >> >> In the modules case, the symbol_request() will be used (same as old code= ). In >> the in-image compilation case, a direct call to kgd2kfd_init() will be d= one. >> For other cases, radeon_kfd_init() will just return false. >> >> This patch is necessary because in case of the following specific >> configuration: kernel 32-bit, no modules support, random kernel base and= no >> hibernation, the symbol_request() doesn't work as expected - it doesn't = return >> NULL if the symbol doesn't exists - which makes the kernel panic. >> >> Signed-off-by: Oded Gabbay > > Using symbol_request() in general doesn't seem to be such a good idea, be= cause > it breaks symbol versioning and as far as I can see is only very rarely u= sed in > drivers (AFAIK only V4L drivers). > > But for the moment I don't have another idea either except for always loa= ding > the module which on most hardware is completely unnecessary. Maybe we sho= uld > consider restructuring the module load dependencies in the next kernel re= lease. > > For now the patch is Reviewed-by: Christian K=C3=B6nig > Thanks Christian. I agree that this is somewhat a mess, that was created unfortunately at the= very = beginning of kfd. We will definitely fix this in 2015, as we also need to think how kfd will = integrate with both radeon and amdgpu if they are present on the same syste= m. = The current method we use doesn't enable that situation. Ideally, I would like to see total separation (between init of radeon and i= nit = of amdkfd) and use of exported symbols instead of symbol_request. Couple th= at = with probe deferral and I think the end result will be much better than the = current design. Oded >> --- >> drivers/gpu/drm/radeon/radeon_kfd.c | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> diff --git a/drivers/gpu/drm/radeon/radeon_kfd.c >> b/drivers/gpu/drm/radeon/radeon_kfd.c >> index 242fd8b..d3e78b4 100644 >> --- a/drivers/gpu/drm/radeon/radeon_kfd.c >> +++ b/drivers/gpu/drm/radeon/radeon_kfd.c >> @@ -101,6 +101,7 @@ static const struct kgd2kfd_calls *kgd2kfd; >> bool radeon_kfd_init(void) >> { >> +#if defined(CONFIG_HSA_AMD_MODULE) >> bool (*kgd2kfd_init_p)(unsigned, const struct kfd2kgd_calls*, >> const struct kgd2kfd_calls**); >> @@ -117,6 +118,17 @@ bool radeon_kfd_init(void) >> } >> return true; >> +#elif defined(CONFIG_HSA_AMD) >> + if (!kgd2kfd_init(KFD_INTERFACE_VERSION, &kfd2kgd, &kgd2kfd)) { >> + kgd2kfd =3D NULL; >> + >> + return false; >> + } >> + >> + return true; >> +#else >> + return false; >> +#endif >> } >> void radeon_kfd_fini(void) > --===============8776715536155573528==-- From mboxrd@z Thu Jan 1 00:00:00 1970 From: Oded Gabbay Subject: Re: [PATCH v2] drm/radeon: Init amdkfd only if it was compiled Date: Mon, 29 Dec 2014 14:49:47 +0200 Message-ID: <54A14DEB.4090503@amd.com> References: <1419843586-29864-1-git-send-email-oded.gabbay@amd.com> <54A14C0C.8090401@vodafone.de> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; Format="flowed" Content-Transfer-Encoding: base64 Return-path: Received: from na01-bn1-obe.outbound.protection.outlook.com (mail-bn1bbn0106.outbound.protection.outlook.com [157.56.111.106]) by gabe.freedesktop.org (Postfix) with ESMTP id 5C9EB89A1F for ; Mon, 29 Dec 2014 04:50:19 -0800 (PST) In-Reply-To: <54A14C0C.8090401@vodafone.de> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: =?ISO-8859-15?Q?Christian_K=F6nig?= , dri-devel@lists.freedesktop.org, Alexander Deucher Cc: ak@linux.intel.com, rusty@rustcorp.com.au, linux-kernel@vger.kernel.org, Huang Ying , LKP ML List-Id: dri-devel@lists.freedesktop.org CgpPbiAxMi8yOS8yMDE0IDAyOjQxIFBNLCBDaHJpc3RpYW4gS8O2bmlnIHdyb3RlOgo+IEFtIDI5 LjEyLjIwMTQgdW0gMDk6NTkgc2NocmllYiBPZGVkIEdhYmJheToKPj4gVGhpcyBwYXRjaCBjaGFu Z2VzIHRoZSByYWRlb25fa2ZkX2luaXQoKSwgd2hpY2ggaXMgdXNlZCB0byBpbml0aWFsaXplIHRo ZQo+PiBpbnRlcmZhY2UgYmV0d2VlbiByYWRlb24gYW5kIGFtZGtmZCwgc28gdGhlIGludGVyZmFj ZSB3aWxsIGJlIGluaXRpYWxpemVkIG9ubHkKPj4gaWYgYW1ka2ZkIHdhcyBidWlsZCwgZWl0aGVy IGFzIG1vZHVsZSBvciBpbnNpZGUgdGhlIGtlcm5lbCBpbWFnZS4KPj4KPj4gSW4gdGhlIG1vZHVs ZXMgY2FzZSwgdGhlIHN5bWJvbF9yZXF1ZXN0KCkgd2lsbCBiZSB1c2VkIChzYW1lIGFzIG9sZCBj b2RlKS4gSW4KPj4gdGhlIGluLWltYWdlIGNvbXBpbGF0aW9uIGNhc2UsIGEgZGlyZWN0IGNhbGwg dG8ga2dkMmtmZF9pbml0KCkgd2lsbCBiZSBkb25lLgo+PiBGb3Igb3RoZXIgY2FzZXMsIHJhZGVv bl9rZmRfaW5pdCgpIHdpbGwganVzdCByZXR1cm4gZmFsc2UuCj4+Cj4+IFRoaXMgcGF0Y2ggaXMg bmVjZXNzYXJ5IGJlY2F1c2UgaW4gY2FzZSBvZiB0aGUgZm9sbG93aW5nIHNwZWNpZmljCj4+IGNv bmZpZ3VyYXRpb246IGtlcm5lbCAzMi1iaXQsIG5vIG1vZHVsZXMgc3VwcG9ydCwgcmFuZG9tIGtl cm5lbCBiYXNlIGFuZCBubwo+PiBoaWJlcm5hdGlvbiwgdGhlIHN5bWJvbF9yZXF1ZXN0KCkgZG9l c24ndCB3b3JrIGFzIGV4cGVjdGVkIC0gaXQgZG9lc24ndCByZXR1cm4KPj4gTlVMTCBpZiB0aGUg c3ltYm9sIGRvZXNuJ3QgZXhpc3RzIC0gd2hpY2ggbWFrZXMgdGhlIGtlcm5lbCBwYW5pYy4KPj4K Pj4gU2lnbmVkLW9mZi1ieTogT2RlZCBHYWJiYXkgPG9kZWQuZ2FiYmF5QGFtZC5jb20+Cj4KPiBV c2luZyBzeW1ib2xfcmVxdWVzdCgpIGluIGdlbmVyYWwgZG9lc24ndCBzZWVtIHRvIGJlIHN1Y2gg YSBnb29kIGlkZWEsIGJlY2F1c2UKPiBpdCBicmVha3Mgc3ltYm9sIHZlcnNpb25pbmcgYW5kIGFz IGZhciBhcyBJIGNhbiBzZWUgaXMgb25seSB2ZXJ5IHJhcmVseSB1c2VkIGluCj4gZHJpdmVycyAo QUZBSUsgb25seSBWNEwgZHJpdmVycykuCj4KPiBCdXQgZm9yIHRoZSBtb21lbnQgSSBkb24ndCBo YXZlIGFub3RoZXIgaWRlYSBlaXRoZXIgZXhjZXB0IGZvciBhbHdheXMgbG9hZGluZwo+IHRoZSBt b2R1bGUgd2hpY2ggb24gbW9zdCBoYXJkd2FyZSBpcyBjb21wbGV0ZWx5IHVubmVjZXNzYXJ5LiBN YXliZSB3ZSBzaG91bGQKPiBjb25zaWRlciByZXN0cnVjdHVyaW5nIHRoZSBtb2R1bGUgbG9hZCBk ZXBlbmRlbmNpZXMgaW4gdGhlIG5leHQga2VybmVsIHJlbGVhc2UuCj4KPiBGb3Igbm93IHRoZSBw YXRjaCBpcyBSZXZpZXdlZC1ieTogQ2hyaXN0aWFuIEvDtm5pZyA8Y2hyaXN0aWFuLmtvZW5pZ0Bh bWQuY29tPgo+ClRoYW5rcyBDaHJpc3RpYW4uCkkgYWdyZWUgdGhhdCB0aGlzIGlzIHNvbWV3aGF0 IGEgbWVzcywgdGhhdCB3YXMgY3JlYXRlZCB1bmZvcnR1bmF0ZWx5IGF0IHRoZSB2ZXJ5IApiZWdp bm5pbmcgb2Yga2ZkLgoKV2Ugd2lsbCBkZWZpbml0ZWx5IGZpeCB0aGlzIGluIDIwMTUsIGFzIHdl IGFsc28gbmVlZCB0byB0aGluayBob3cga2ZkIHdpbGwgCmludGVncmF0ZSB3aXRoIGJvdGggcmFk ZW9uIGFuZCBhbWRncHUgaWYgdGhleSBhcmUgcHJlc2VudCBvbiB0aGUgc2FtZSBzeXN0ZW0uIApU aGUgY3VycmVudCBtZXRob2Qgd2UgdXNlIGRvZXNuJ3QgZW5hYmxlIHRoYXQgc2l0dWF0aW9uLgoK SWRlYWxseSwgSSB3b3VsZCBsaWtlIHRvIHNlZSB0b3RhbCBzZXBhcmF0aW9uIChiZXR3ZWVuIGlu aXQgb2YgcmFkZW9uIGFuZCBpbml0IApvZiBhbWRrZmQpIGFuZCB1c2Ugb2YgZXhwb3J0ZWQgc3lt Ym9scyBpbnN0ZWFkIG9mIHN5bWJvbF9yZXF1ZXN0LiBDb3VwbGUgdGhhdCAKd2l0aCBwcm9iZSBk ZWZlcnJhbCBhbmQgSSB0aGluayB0aGUgZW5kIHJlc3VsdCB3aWxsIGJlIG11Y2ggYmV0dGVyIHRo YW4gdGhlIApjdXJyZW50IGRlc2lnbi4KCglPZGVkCgo+PiAtLS0KPj4gICBkcml2ZXJzL2dwdS9k cm0vcmFkZW9uL3JhZGVvbl9rZmQuYyB8IDEyICsrKysrKysrKysrKwo+PiAgIDEgZmlsZSBjaGFu Z2VkLCAxMiBpbnNlcnRpb25zKCspCj4+Cj4+IGRpZmYgLS1naXQgYS9kcml2ZXJzL2dwdS9kcm0v cmFkZW9uL3JhZGVvbl9rZmQuYwo+PiBiL2RyaXZlcnMvZ3B1L2RybS9yYWRlb24vcmFkZW9uX2tm ZC5jCj4+IGluZGV4IDI0MmZkOGIuLmQzZTc4YjQgMTAwNjQ0Cj4+IC0tLSBhL2RyaXZlcnMvZ3B1 L2RybS9yYWRlb24vcmFkZW9uX2tmZC5jCj4+ICsrKyBiL2RyaXZlcnMvZ3B1L2RybS9yYWRlb24v cmFkZW9uX2tmZC5jCj4+IEBAIC0xMDEsNiArMTAxLDcgQEAgc3RhdGljIGNvbnN0IHN0cnVjdCBr Z2Qya2ZkX2NhbGxzICprZ2Qya2ZkOwo+PiAgIGJvb2wgcmFkZW9uX2tmZF9pbml0KHZvaWQpCj4+ ICAgewo+PiArI2lmIGRlZmluZWQoQ09ORklHX0hTQV9BTURfTU9EVUxFKQo+PiAgICAgICBib29s ICgqa2dkMmtmZF9pbml0X3ApKHVuc2lnbmVkLCBjb25zdCBzdHJ1Y3Qga2ZkMmtnZF9jYWxscyos Cj4+ICAgICAgICAgICAgICAgICAgIGNvbnN0IHN0cnVjdCBrZ2Qya2ZkX2NhbGxzKiopOwo+PiBA QCAtMTE3LDYgKzExOCwxNyBAQCBib29sIHJhZGVvbl9rZmRfaW5pdCh2b2lkKQo+PiAgICAgICB9 Cj4+ICAgICAgIHJldHVybiB0cnVlOwo+PiArI2VsaWYgZGVmaW5lZChDT05GSUdfSFNBX0FNRCkK Pj4gKyAgICBpZiAoIWtnZDJrZmRfaW5pdChLRkRfSU5URVJGQUNFX1ZFUlNJT04sICZrZmQya2dk LCAma2dkMmtmZCkpIHsKPj4gKyAgICAgICAga2dkMmtmZCA9IE5VTEw7Cj4+ICsKPj4gKyAgICAg ICAgcmV0dXJuIGZhbHNlOwo+PiArICAgIH0KPj4gKwo+PiArICAgIHJldHVybiB0cnVlOwo+PiAr I2Vsc2UKPj4gKyAgICByZXR1cm4gZmFsc2U7Cj4+ICsjZW5kaWYKPj4gICB9Cj4+ICAgdm9pZCBy YWRlb25fa2ZkX2Zpbmkodm9pZCkKPgpfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fXwpkcmktZGV2ZWwgbWFpbGluZyBsaXN0CmRyaS1kZXZlbEBsaXN0cy5mcmVl ZGVza3RvcC5vcmcKaHR0cDovL2xpc3RzLmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFuL2xpc3RpbmZv L2RyaS1kZXZlbAo= From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752081AbaL2NGN (ORCPT ); Mon, 29 Dec 2014 08:06:13 -0500 Received: from mail-bn1bon0114.outbound.protection.outlook.com ([157.56.111.114]:45367 "EHLO na01-bn1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751435AbaL2NGL convert rfc822-to-8bit (ORCPT ); Mon, 29 Dec 2014 08:06:11 -0500 X-WSS-ID: 0NHCGZQ-07-6T2-02 X-M-MSG: Message-ID: <54A14DEB.4090503@amd.com> Date: Mon, 29 Dec 2014 14:49:47 +0200 From: Oded Gabbay Organization: AMD User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.3.0 MIME-Version: 1.0 To: =?ISO-8859-15?Q?Christian_K=F6nig?= , , Alexander Deucher CC: , Huang Ying , LKP ML , , , Subject: Re: [PATCH v2] drm/radeon: Init amdkfd only if it was compiled References: <1419843586-29864-1-git-send-email-oded.gabbay@amd.com> <54A14C0C.8090401@vodafone.de> In-Reply-To: <54A14C0C.8090401@vodafone.de> Content-Type: text/plain; charset="iso-8859-15"; format=flowed X-Originating-IP: [10.20.0.84] Content-Transfer-Encoding: 8BIT X-EOPAttributedMessage: 0 Authentication-Results: spf=none (sender IP is 165.204.84.221) smtp.mailfrom=Oded.Gabbay@amd.com; X-Forefront-Antispam-Report: CIP:165.204.84.221;CTRY:US;IPV:NLI;EFV:NLI;SFV:NSPM;SFS:(10019020)(6009001)(428002)(189002)(479174004)(24454002)(199003)(377454003)(51704005)(2950100001)(76176999)(62966003)(77096005)(105586002)(106466001)(23756003)(36756003)(86362001)(19580405001)(19580395003)(59896002)(50466002)(64126003)(107046002)(97736003)(54356999)(77156002)(68736005)(99396003)(33656002)(47776003)(20776003)(64706001)(21056001)(65956001)(101416001)(87266999)(84676001)(120916001)(4396001)(31966008)(65806001)(50986999)(92566001)(46102003)(87936001)(65816999)(83506001)(2101003);DIR:OUT;SFP:1102;SCL:1;SRVR:BY2PR02MB204;H:atltwp01.amd.com;FPR:;SPF:None;MLV:sfv;PTR:InfoDomainNonexistent;MX:1;A:1;LANG:en; X-Microsoft-Antispam: UriScan:; X-Microsoft-Antispam: BCL:0;PCL:0;RULEID:;SRVR:BY2PR02MB204; X-Exchange-Antispam-Report-Test: UriScan:; X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(601004);SRVR:BY2PR02MB204; X-Forefront-PRVS: 0440AC9990 X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:;SRVR:BY2PR02MB204; X-OriginatorOrg: amd4.onmicrosoft.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Dec 2014 12:50:16.2312 (UTC) X-MS-Exchange-CrossTenant-Id: fde4dada-be84-483f-92cc-e026cbee8e96 X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=fde4dada-be84-483f-92cc-e026cbee8e96;Ip=[165.204.84.221] X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY2PR02MB204 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 12/29/2014 02:41 PM, Christian König wrote: > Am 29.12.2014 um 09:59 schrieb Oded Gabbay: >> This patch changes the radeon_kfd_init(), which is used to initialize the >> interface between radeon and amdkfd, so the interface will be initialized only >> if amdkfd was build, either as module or inside the kernel image. >> >> In the modules case, the symbol_request() will be used (same as old code). In >> the in-image compilation case, a direct call to kgd2kfd_init() will be done. >> For other cases, radeon_kfd_init() will just return false. >> >> This patch is necessary because in case of the following specific >> configuration: kernel 32-bit, no modules support, random kernel base and no >> hibernation, the symbol_request() doesn't work as expected - it doesn't return >> NULL if the symbol doesn't exists - which makes the kernel panic. >> >> Signed-off-by: Oded Gabbay > > Using symbol_request() in general doesn't seem to be such a good idea, because > it breaks symbol versioning and as far as I can see is only very rarely used in > drivers (AFAIK only V4L drivers). > > But for the moment I don't have another idea either except for always loading > the module which on most hardware is completely unnecessary. Maybe we should > consider restructuring the module load dependencies in the next kernel release. > > For now the patch is Reviewed-by: Christian König > Thanks Christian. I agree that this is somewhat a mess, that was created unfortunately at the very beginning of kfd. We will definitely fix this in 2015, as we also need to think how kfd will integrate with both radeon and amdgpu if they are present on the same system. The current method we use doesn't enable that situation. Ideally, I would like to see total separation (between init of radeon and init of amdkfd) and use of exported symbols instead of symbol_request. Couple that with probe deferral and I think the end result will be much better than the current design. Oded >> --- >> drivers/gpu/drm/radeon/radeon_kfd.c | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> diff --git a/drivers/gpu/drm/radeon/radeon_kfd.c >> b/drivers/gpu/drm/radeon/radeon_kfd.c >> index 242fd8b..d3e78b4 100644 >> --- a/drivers/gpu/drm/radeon/radeon_kfd.c >> +++ b/drivers/gpu/drm/radeon/radeon_kfd.c >> @@ -101,6 +101,7 @@ static const struct kgd2kfd_calls *kgd2kfd; >> bool radeon_kfd_init(void) >> { >> +#if defined(CONFIG_HSA_AMD_MODULE) >> bool (*kgd2kfd_init_p)(unsigned, const struct kfd2kgd_calls*, >> const struct kgd2kfd_calls**); >> @@ -117,6 +118,17 @@ bool radeon_kfd_init(void) >> } >> return true; >> +#elif defined(CONFIG_HSA_AMD) >> + if (!kgd2kfd_init(KFD_INTERFACE_VERSION, &kfd2kgd, &kgd2kfd)) { >> + kgd2kfd = NULL; >> + >> + return false; >> + } >> + >> + return true; >> +#else >> + return false; >> +#endif >> } >> void radeon_kfd_fini(void) >