<html> <head> <meta content="text/html; charset=ISO-8859-1" http-equiv="Content-Type"> </head> <body text="#000000" bgcolor="#FFFFFF"> <div class="moz-cite-prefix">Am 17.07.2014 14:30, schrieb Oded Gabbay: </div> <blockquote cite="mid:53C7C1EA.3080002@amd.com" type="cite">On 17/07/14 15:29, Christian König wrote: <blockquote type="cite">Am 17.07.2014 13:57, schrieb Oded Gabbay: <blockquote type="cite">On 11/07/14 19:36, Jerome Glisse wrote: <blockquote type="cite">On Fri, Jul 11, 2014 at 12:50:08AM +0300, Oded Gabbay wrote: <blockquote type="cite">The KFD driver should be loaded when the radeon driver is loaded and should be finalized when the radeon driver is removed. This patch adds a function call to initialize kfd from radeon_init and a function call to finalize kfd from radeon_exit. If the KFD driver is not present in the system, the initialize call fails and the radeon driver continues normally. This patch also adds calls to probe, initialize and finalize a kfd device per radeon device using the kgd-->kfd interface. Signed-off-by: Oded Gabbay <a class="moz-txt-link-rfc2396E" href="mailto:oded.gabbay@amd.com"><oded.gabbay@amd.com></a> </blockquote> It might be nice to allow to build radeon without HSA so i think an CONFIG_HSA should be added and have other thing depends on it. Otherwise this one is. Reviewed-by: Jérôme Glisse <a class="moz-txt-link-rfc2396E" href="mailto:jglisse@redhat.com"><jglisse@redhat.com></a> </blockquote> We do allow it :) There is no problem building radeon without the kfd. In that case, when radeon finds out that kfd is not available, it simply moves on with its initialization procedure. </blockquote> At least off hand I don't see how this should work. Radeon directly calls radeon_kfd_(probe|init|fini) and so has a direct dependency on it. Christian. </blockquote> But radeon_kfd.c is now a permanent part of the radeon driver. I talked with Alex about it and we both agreed on that. So radeon_kfd_* functions are *always* there when you build radeon. </blockquote> Ah, I see. So radeon_kfd_init then tries to load the other module through symbol_request(). Long story short that's a bad idea for a couple of reasons. First of all it only works when you build everything as module and second by doing so the radeon<->kfd interface must be handled as internal stable interface. Only a very few drivers/subsystem do use symbol_request() and to see how to use it correctly please take a look at (for example) sound/pci/hda/hda_codec.c. Essentially you need to handle all different combination of module vs. builtin like this: <blockquote type="cite"> <pre><a name="L1660" href="http://lxr.free-electrons.com/source/sound/pci/hda/hda_codec.c#L1660">1660</a> #if <a href="http://lxr.free-electrons.com/ident?i=IS_MODULE">IS_MODULE</a>(CONFIG_SND_HDA_GENERIC) <a name="L1661" href="http://lxr.free-electrons.com/source/sound/pci/hda/hda_codec.c#L1661">1661</a> <a href="http://lxr.free-electrons.com/ident?i=patch">patch</a> = <a href="http://lxr.free-electrons.com/ident?i=load_parser">load_parser</a>(<a href="http://lxr.free-electrons.com/ident?i=codec">codec</a>, <a href="http://lxr.free-electrons.com/ident?i=snd_hda_parse_generic_codec">snd_hda_parse_generic_codec</a>); <a name="L1662" href="http://lxr.free-electrons.com/source/sound/pci/hda/hda_codec.c#L1662">1662</a> #elif <a href="http://lxr.free-electrons.com/ident?i=IS_BUILTIN">IS_BUILTIN</a>(CONFIG_SND_HDA_GENERIC) <a name="L1663" href="http://lxr.free-electrons.com/source/sound/pci/hda/hda_codec.c#L1663">1663</a> <a href="http://lxr.free-electrons.com/ident?i=patch">patch</a> = <a href="http://lxr.free-electrons.com/ident?i=snd_hda_parse_generic_codec">snd_hda_parse_generic_codec</a>; <a name="L1664" href="http://lxr.free-electrons.com/source/sound/pci/hda/hda_codec.c#L1664">1664</a> #endif</pre> </blockquote> I strongly suggest to just make the radeon module depend directly on the KFD module through a CONFIG_RADEON_KFD option. Regards, Christian. <blockquote cite="mid:53C7C1EA.3080002@amd.com" type="cite"> Oded <blockquote type="cite"> <blockquote type="cite"> Oded <blockquote type="cite"> <blockquote type="cite">--- drivers/gpu/drm/radeon/radeon_drv.c | 6 ++++++ drivers/gpu/drm/radeon/radeon_kms.c | 9 +++++++++ 2 files changed, 15 insertions(+) diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c index cb14213..88a45a0 100644 --- a/drivers/gpu/drm/radeon/radeon_drv.c +++ b/drivers/gpu/drm/radeon/radeon_drv.c @@ -151,6 +151,9 @@ static inline void radeon_register_atpx_handler(void) {} static inline void radeon_unregister_atpx_handler(void) {} #endif +extern bool radeon_kfd_init(void); +extern void radeon_kfd_fini(void); + int radeon_no_wb; int radeon_modeset = -1; int radeon_dynclks = -1; @@ -630,12 +633,15 @@ static int __init radeon_init(void) #endif } + radeon_kfd_init(); + /* let modprobe override vga console setting */ return drm_pci_init(driver, pdriver); } static void __exit radeon_exit(void) { + radeon_kfd_fini(); drm_pci_exit(driver, pdriver); radeon_unregister_atpx_handler(); } diff --git a/drivers/gpu/drm/radeon/radeon_kms.c b/drivers/gpu/drm/radeon/radeon_kms.c index 35d9318..0748284 100644 --- a/drivers/gpu/drm/radeon/radeon_kms.c +++ b/drivers/gpu/drm/radeon/radeon_kms.c @@ -34,6 +34,10 @@ #include <linux/slab.h> #include <linux/pm_runtime.h> +extern void radeon_kfd_device_probe(struct radeon_device *rdev); +extern void radeon_kfd_device_init(struct radeon_device *rdev); +extern void radeon_kfd_device_fini(struct radeon_device *rdev); + #if defined(CONFIG_VGA_SWITCHEROO) bool radeon_has_atpx(void); #else @@ -63,6 +67,8 @@ int radeon_driver_unload_kms(struct drm_device *dev) pm_runtime_get_sync(dev->dev); + radeon_kfd_device_fini(rdev); + radeon_acpi_fini(rdev); radeon_modeset_fini(rdev); @@ -142,6 +148,9 @@ int radeon_driver_load_kms(struct drm_device *dev, unsigned long flags) "Error during ACPI methods call\n"); } + radeon_kfd_device_probe(rdev); + radeon_kfd_device_init(rdev); + if (radeon_is_px(dev)) { pm_runtime_use_autosuspend(dev->dev); pm_runtime_set_autosuspend_delay(dev->dev, 5000); -- 1.9.1 </blockquote> </blockquote> </blockquote> </blockquote> </blockquote> </body> </html>