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

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Jul 17, 2014 at 02:45:09PM +0200, Christian König wrote:
> 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.

We do this in i915 to coordinate a bunch of things with the snd_hda
driver. And it's a major pain. Imo the proper way to do this is for one
driver to expose a platform driver with a bunch of specific interfaces and
for the other driver to register as a platform driver against that device.

Then all the usual linux hotplug infrastructure will make sure that this
all works and there's a clear runtime depency. For i915 that's what I've
requested the audio guys to look into, and also what I'll require for
other such sub-driver stuff (e.g. we have a non-intel video codec on vlv
gfx). Well for audio it will be a bit fancier since we also want some
standardized stuff to allow userspace to see the association between the
gfx output and the audio side. Atm you can only guess if you have more
than one screen connected.

This approach gives you full flexibility and you can e.g. blacklist the
subdriver for debugging, without a kernel recompile.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch