Re: [PATCH 08/83] drm/radeon: Add calls to initialize and finalize kfd from radeon

["Christian König" <christian.koenig@amd.com>]

[-- Attachment #1.1: Type: text/plain, Size: 6238 bytes --]

Am 17.07.2014 14:30, schrieb Oded Gabbay:
> On 17/07/14 15:29, Christian König wrote:
>> Am 17.07.2014 13:57, schrieb Oded Gabbay:
>>> On 11/07/14 19:36, Jerome Glisse wrote:
>>>> On Fri, Jul 11, 2014 at 12:50:08AM +0300, Oded Gabbay wrote:
>>>>> 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 <oded.gabbay@amd.com>
>>>>
>>>> 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 <jglisse@redhat.com>
>>>>
>>> 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.
>>
>> 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.
> 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.

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:
> 1660  <http://lxr.free-electrons.com/source/sound/pci/hda/hda_codec.c#L1660>  #ifIS_MODULE  <http://lxr.free-electrons.com/ident?i=IS_MODULE>(CONFIG_SND_HDA_GENERIC)
> 1661  <http://lxr.free-electrons.com/source/sound/pci/hda/hda_codec.c#L1661>                          patch  <http://lxr.free-electrons.com/ident?i=patch>  =load_parser  <http://lxr.free-electrons.com/ident?i=load_parser>(codec  <http://lxr.free-electrons.com/ident?i=codec>,snd_hda_parse_generic_codec  <http://lxr.free-electrons.com/ident?i=snd_hda_parse_generic_codec>);
> 1662  <http://lxr.free-electrons.com/source/sound/pci/hda/hda_codec.c#L1662>  #elifIS_BUILTIN  <http://lxr.free-electrons.com/ident?i=IS_BUILTIN>(CONFIG_SND_HDA_GENERIC)
> 1663  <http://lxr.free-electrons.com/source/sound/pci/hda/hda_codec.c#L1663>                          patch  <http://lxr.free-electrons.com/ident?i=patch>  =snd_hda_parse_generic_codec  <http://lxr.free-electrons.com/ident?i=snd_hda_parse_generic_codec>;
> 1664  <http://lxr.free-electrons.com/source/sound/pci/hda/hda_codec.c#L1664>  #endif

I strongly suggest to just make the radeon module depend directly on the 
KFD module through a CONFIG_RADEON_KFD option.

Regards,
Christian.

>     Oded
>>
>>>
>>>     Oded
>>>>
>>>>> ---
>>>>>   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
>>>>>
>>>
>>
>


[-- Attachment #1.2: Type: text/html, Size: 10959 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel