Re: [PATCH 1/2] Documentation/core-api/device_link: Add initial documentation

[Luis R. Rodriguez]

On Sun, Dec 04, 2016 at 01:10:04PM +0100, Lukas Wunner wrote:
> Document device links as introduced in v4.10 with commits:
> 4bdb35506b89 ("driver core: Add a wrapper around
>__device_release_driver()")
> 9ed9895370ae ("driver core: Functional dependencies tracking
>support")
> 8c73b4288496 ("PM / sleep: Make async suspend/resume of devices use
>device links")
> 21d5c57b3726 ("PM / runtime: Use device links")
> baa8809f6097 ("PM / runtime: Optimize the use of device links")
> Signed-off-by: Lukas Wunner 

First, thanks for doing this work.

> ---
>  Documentation/core-api/device_link.rst | 279 
> +
>  Documentation/core-api/index.rst   |   8 +
>  2 files changed, 287 insertions(+)
>  create mode 100644 Documentation/core-api/device_link.rst
> 
> diff --git a/Documentation/core-api/device_link.rst 
> b/Documentation/core-api/device_link.rst
> new file mode 100644
> index 000..5f57134
> --- /dev/null
> +++ b/Documentation/core-api/device_link.rst
> @@ -0,0 +1,279 @@
> +
> +Device links
> +
> +
> +By default, the driver core only enforces dependencies between devices
> +that are borne out of a parent/child relationship within the device
> +hierarchy: 

This "device hierarchy" was rather confusing to grasp, I specially hard
a hard time understanding *why* the device links work did not do any
topological sorting at all. I'm also afraid this is not just "tribal
knowledge" -- not everyone was able to answer my questions about these
things, Rafafel however did do justice to some degree and I did provide notes
from this. I think it would be important then to jot down to help humans
grok:

  o where the implicit order we embrace comes from
  o what mechanisms are used to help provide some of this order
  o why this is *not* sufficient

The last one justifies the work. And more importantly, it may help
replace a whole bunch of other parallel work which had similar type of
solutions. To what extent it can do that well remains to be seen but
from what I can tell, that's future work worth looking into.

<-- snip -->

> +Limitations
> +===
> +
> +Driver authors should be aware that a driver presence dependency (i.e. when
> +``DL_FLAG_STATELESS`` is not specified on link addition) may cause probing of
> +the consumer to be deferred indefinitely.  This can become a problem if the
> +consumer is required to probe before a certain initcall level is reached.
> +Worse, if the supplier driver is blacklisted or missing, the consumer will
> +never be probed.
> +
> +Sometimes drivers depend on optional resources.  They are able to operate
> +in a degraded mode (reduced feature set or performance) when those resources
> +are not present.  An example is an SPI controller that can use a DMA engine
> +or work in PIO mode.  The controller can determine presence of the optional
> +resources at probe time but on non-presence there is no way to know whether
> +they will become available in the near future (due to a supplier driver
> +probing) or never.  Consequently it cannot be determined whether to defer
> +probing or not.  It would be possible to notify drivers when optional
> +resources become available after probing, but it would come at a high cost
> +for drivers as switching between modes of operation at runtime based on the
> +availability of such resources would be much more complex than a mechanism
> +based on probe deferral.  In any case optional resources are beyond the
> +scope of device links.

Well you also forgot to mention the issues with deferred probe. It uses
deferred probe so if for whatever reason your subsystem or driver has issues
with it, this won't help.

<-- snip -->
> +* ACPI allows definition of a device start order by way of _DEP objects.
> +  A classical example is when ACPI power management methods on one device
> +  are implemented in terms of I\ :sup:`2`\ C accesses and require a specific
> +  I\ :sup:`2`\ C controller to be present and functional for the power
> +  management of the device in question to work.

Here is an example of current mechanisms used which to driver developers
provides implicit order --its all magical. This has can have limitations,
its important to annotate how this is limiting and also why this perhaps
was not considered as an enhancement in ACPI space. Ie, if semantics
existed in ACPI space for dependency info, why are we now left to our hims
for some cases on driver code?

> +
> +Alternatives
> +
> +
> +* A :c:type:`struct dev_pm_domain` can be used to override the bus,
> +  class or device type callbacks.  It is intended for devices sharing
> +  a single on/off switch, however it does not guarantee a specific
> +  suspend/resume ordering, this needs to be implemented separately.
> +  It also does not by itself track the runtime PM status of the involved
> +  devices and turn off the power switch 

Re: [PATCH v4 2/2] iommu/exynos: Add proper runtime pm support

[Luis R. Rodriguez]

On Thu, Nov 10, 2016 at 01:36:29AM +0100, Rafael J. Wysocki wrote:
> On Wed, Nov 9, 2016 at 4:07 PM, Marek Szyprowski
> <m.szyprow...@samsung.com> wrote:
> > Hi Luis,
> >
> >
> > On 2016-11-08 23:14, Luis R. Rodriguez wrote:
> >>
> >> On Mon, Oct 10, 2016 at 03:32:06PM +0200, Marek Szyprowski wrote:
> >>>
> >>> Hi Luis
> >>>
> >>>
> >>> On 2016-10-06 19:37, Luis R. Rodriguez wrote:
> >>>>
> >>>> On Thu, Sep 29, 2016 at 10:12:31AM +0200, Marek Szyprowski wrote:
> >>>>>
> >>>>> This patch uses recently introduced device links to track the runtime
> >>>>> pm
> >>>>> state of the master's device. This way each SYSMMU controller is
> >>>>> runtime
> >>>>> activated when its master's device is active
> >>>>
> >>>> instead of?
> >>>
> >>> instead of keeping SYSMMU controller runtime active all the time.
> >>
> >> I thought Rafael's work was for suspend/resume, not for runtime suspend.
> >> Is it for both ?
> >
> >
> > Yes, it solves both problems, although the suspend/resume was easy to
> > workaround just by using LATE_SLEEP_OPS.
> 
> Right, but that's just in this particular case, because the dependency
> chain is of length 2. :-)
> 
> If you had a longer chain, you might in theory use  the _noirq() stage
> somehow, but that has limitations.
> 
> >> Because as far as I can tell this was painted to help
> >> with suspend/resume ?
> 
> It helps with three things, (async) suspend/resume, runtime PM and
> shutdown (that last part is the hardest to figure out).  The ordering
> in which all of these are carried out is analogous and cannot be
> determined correctly by the device registration ordering itself in
> general (which has been a known fact for years, but some localized
> workarounds were put in some places to work around that).

Thanks for the clarification, this is due to the implicit sort you
had explained (and I provided notes for on ksummit-discuss) right?

Can you itemize a few of the workarounds that are used today?
As you clarify below, getting this order of device registration
correct does not necessarily guarantee devices will wait for their
provider to be ready.

> Moreover, even if the list ordering (of dpm_list, for instance) is
> correct, it still doesn't guarantee the right ordering of actions that
> are carried out asynchronously.  They are all started in the list
> order, but they may be running in parallel with each other and
> complete at different times.  For this reason, there needs to be a way
> to ensure that, say, the suspend operations for consumer devices
> complete before their suppliers will become unavailable and so on.

Thanks this helps as well!

> Both runtime PM and system suspend/resume have this problem.  It is
> not present in the system shutdown case, but it still helps to get a
> correct list ordering (ie. such that won't cause supplier devices to
> be shut down before their consumers) in this case too.

Is the fact that its not on shutdown just because we don't care about
being sloppy about shutdown ? Shouldn't some devices care about that
order ?

  Luis
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm

[Luis R. Rodriguez]

On Thu, Nov 10, 2016 at 01:05:42AM +0100, Rafael J. Wysocki wrote:
> On Thu, Nov 10, 2016 at 12:55 AM, Luis R. Rodriguez <mcg...@kernel.org> wrote:
> > On Tue, Nov 08, 2016 at 04:30:44PM +0100, Lukas Wunner wrote:
> >> On Tue, Nov 08, 2016 at 08:27:12AM +0100, Marek Szyprowski wrote:
> >> > On 2016-11-07 22:47, Luis R. Rodriguez wrote:
> >> > > Has there been any review of the existing similar solutions out there
> >> > > such as the DRM / audio component framework? Would that help ?
> >> >
> >> > Nope, none of that solution deals with runtime pm.
> >>
> >> Well, they do.  Hybrid graphics laptops often have an HDA controller
> >> on the discrete GPU and I assume that's what Luis meant. There's code
> >> in drivers/gpu/vga/vga_switcheroo.c to make this (only sort of) work:
> >>
> >> * When the GPU is powered up/down, the HDA controller's driver is
> >>   instructed to pm_runtime_get/put the HDA device (see call to
> >>   set_audio_state() in vga_switcheroo_set_dynamic_switch()).
> >>
> >> * When a runtime PM ref is acquired on the HDA device, the
> >>   GPU is powered up (see vga_switcheroo_runtime_resume_hdmi_audio()).
> >>
> >>
> >> Unfortunately this is all fairly broken, e.g.:
> >>
> >> * If a runtime PM ref on the HDA device is held for more than 5 sec
> >>   (autosuspend delay of the GPU), the GPU will be powered down and
> >>   the HDA device will become inaccessible, regardless of the runtime
> >>   PM ref still being held (because vga_switcheroo_set_dynamic_switch()
> >>   doesn't check the refcount of the HDA device).
> >>
> >> * The DRM device is afforded direct-complete but the HDA device is not.
> >>   If the GPU is handled earlier by dpm_suspend(), then runtime PM will
> >>   have been disabled on the GPU and thus the HDA device will fail to
> >>   runtime resume before system sleep.
> >>
> >> Rafael's series allows representation of such inter-device dependencies
> >> in the PM core and can thus replace kludgy and broken "solutions" like
> >> the one above.
> >>
> >> There's one thing that I haven't understood myself though:  In an e-mail
> >> exchange in September Rafael has argued that the above-mentioned hybrid
> >> graphics use case "isn't a good [example] IMO.  That clearly is a case
> >> when two (or more) devices share power resources controlled by a single
> >> on/off switch.  Which is a clear use case for a PM domain."
> >>
> >> The same seems to apply to Marek's SYSMMU use case.  When applying device
> >> links to SYSMMU or hybrid graphics, we select one of the devices in the
> >> PM domain as master and have the other one depend on it as slave, i.e.
> >> a synthetic hierarchical relationship is established.
> >>
> >> I've responded to Rafael on September 18 that this can't be solved with
> >> a struct dev_pm_domain, but haven't received a reply since:
> >> https://lkml.org/lkml/2016/9/18/103
> >
> > Rafael note:
> >
> > The one he asked here.
> 
> OK, so please see the message I've just sent. :-)
> 
> The bottom line is that there may be multiple ways to approach a
> problem like this and which of them works best really depends on the
> particular case.
> 
> You seem to be thinking that the device links infrastructure may not
> be necessary after all if there are other ways to address the problems
> it is intended for, but those other ways may still be less viable
> practically due to the complexity involved and similar.  Also they may
> lead to code duplication in different places that try to address those
> problems in a similar fashion, but slightly differently.

I was not arguing that, I have been suggesting that pre-existing solutions
should at least be reviewed and considered, if they are sub-par and
the device links infrastructure is much simpler and provides the same
solution folks should be alert and consider embracing it. If not and
its the other way around and we could generalize the others, it should
mean we could learn from them.

>From what I gather from Plumbers its not clear to me any of this review
was done, that's all. This leaves a series of open questions about those
existing solutions.

> All this is about providing people with reasonably straightforward and
> common ways to deal with certain class of problems showing up in
> multiple places.  It is not about getting the driver probe ordering
> right magically in one go.

Right, I just want us to avoid re-inventing the wheel.

  Luis
> 
> Thanks,
> Rafael
> 

-- 
Luis Rodriguez, SUSE LINUX GmbH
Maxfeldstrasse 5; D-90409 Nuernberg
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm

[Luis R. Rodriguez]

On Tue, Nov 08, 2016 at 04:30:44PM +0100, Lukas Wunner wrote:
> On Tue, Nov 08, 2016 at 08:27:12AM +0100, Marek Szyprowski wrote:
> > On 2016-11-07 22:47, Luis R. Rodriguez wrote:
> > > Has there been any review of the existing similar solutions out there
> > > such as the DRM / audio component framework? Would that help ?
> > 
> > Nope, none of that solution deals with runtime pm.
> 
> Well, they do.  Hybrid graphics laptops often have an HDA controller
> on the discrete GPU and I assume that's what Luis meant. There's code
> in drivers/gpu/vga/vga_switcheroo.c to make this (only sort of) work:
> 
> * When the GPU is powered up/down, the HDA controller's driver is
>   instructed to pm_runtime_get/put the HDA device (see call to
>   set_audio_state() in vga_switcheroo_set_dynamic_switch()).
> 
> * When a runtime PM ref is acquired on the HDA device, the
>   GPU is powered up (see vga_switcheroo_runtime_resume_hdmi_audio()).
> 
> 
> Unfortunately this is all fairly broken, e.g.:
> 
> * If a runtime PM ref on the HDA device is held for more than 5 sec
>   (autosuspend delay of the GPU), the GPU will be powered down and
>   the HDA device will become inaccessible, regardless of the runtime
>   PM ref still being held (because vga_switcheroo_set_dynamic_switch()
>   doesn't check the refcount of the HDA device).
> 
> * The DRM device is afforded direct-complete but the HDA device is not.
>   If the GPU is handled earlier by dpm_suspend(), then runtime PM will
>   have been disabled on the GPU and thus the HDA device will fail to
>   runtime resume before system sleep.
> 
> Rafael's series allows representation of such inter-device dependencies
> in the PM core and can thus replace kludgy and broken "solutions" like
> the one above.
> 
> There's one thing that I haven't understood myself though:  In an e-mail
> exchange in September Rafael has argued that the above-mentioned hybrid
> graphics use case "isn't a good [example] IMO.  That clearly is a case
> when two (or more) devices share power resources controlled by a single
> on/off switch.  Which is a clear use case for a PM domain."
> 
> The same seems to apply to Marek's SYSMMU use case.  When applying device
> links to SYSMMU or hybrid graphics, we select one of the devices in the
> PM domain as master and have the other one depend on it as slave, i.e.
> a synthetic hierarchical relationship is established.
> 
> I've responded to Rafael on September 18 that this can't be solved with
> a struct dev_pm_domain, but haven't received a reply since:
> https://lkml.org/lkml/2016/9/18/103

Rafael note:

The one he asked here.

  Luis
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v6 7/7] iommu/exynos: Use device dependency links to control runtime pm

[Luis R. Rodriguez]

On Tue, Nov 08, 2016 at 02:29:24PM +0100, Marek Szyprowski wrote:
> This patch uses recently introduced device dependency links to track the
> runtime pm state of the master's device. The goal is to let SYSMMU
> controller device's runtime PM to follow the runtime PM state of the
> respective master's device. This way each SYSMMU controller is active
> only when its master's device is active and can properly restore or save
> its state instead on runtime PM transition of master's device.
> This approach replaces old behavior, when SYSMMU controller was set to
> runtime active once after attaching to the master device. In the new
> approach SYSMMU controllers no longer prevents respective power domains
> to be turned off when master's device is not being used.
> 
> The dependency links also enforces proper order of suspending/restoring
> devices during system sleep transition, so there is no more need to use
> LATE_SYSTEM_SLEEP_PM_OPS-based workaround for ensuring that SYSMMUs are
> suspended after their master devices.

Patches 1-6 seems reasonable to me, however in so far as this patch is
concerned I'd appreaciate if you and Rafael can reply to Lukas Wunner's
questions.

  Luis

> 
> Signed-off-by: Marek Szyprowski 
> ---
>  drivers/iommu/exynos-iommu.c | 20 ++--
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
> index 5e6d7bb..4b05a15 100644
> --- a/drivers/iommu/exynos-iommu.c
> +++ b/drivers/iommu/exynos-iommu.c
> @@ -633,8 +633,8 @@ static int __maybe_unused exynos_sysmmu_resume(struct 
> device *dev)
>  
>  static const struct dev_pm_ops sysmmu_pm_ops = {
>   SET_RUNTIME_PM_OPS(exynos_sysmmu_suspend, exynos_sysmmu_resume, NULL)
> - SET_LATE_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> -  pm_runtime_force_resume)
> + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> + pm_runtime_force_resume)
>  };
>  
>  static const struct of_device_id sysmmu_of_match[] __initconst = {
> @@ -781,10 +781,6 @@ static void exynos_iommu_detach_device(struct 
> iommu_domain *iommu_domain,
>   if (!has_sysmmu(dev) || owner->domain != iommu_domain)
>   return;
>  
> - list_for_each_entry(data, >controllers, owner_node) {
> - pm_runtime_put_sync(data->sysmmu);
> - }
> -
>   mutex_lock(>rpm_lock);
>  
>   list_for_each_entry(data, >controllers, owner_node) {
> @@ -848,10 +844,6 @@ static int exynos_iommu_attach_device(struct 
> iommu_domain *iommu_domain,
>  
>   mutex_unlock(>rpm_lock);
>  
> - list_for_each_entry(data, >controllers, owner_node) {
> - pm_runtime_get_sync(data->sysmmu);
> - }
> -
>   dev_dbg(dev, "%s: Attached IOMMU with pgtable %pa\n", __func__,
>   );
>  
> @@ -1232,6 +1224,14 @@ static int exynos_iommu_of_xlate(struct device *dev,
>  
>   list_add_tail(>owner_node, >controllers);
>   data->master = dev;
> +
> + /*
> +  * SYSMMU will be runtime activated via device link (dependency) to its
> +  * master device, so there are no direct calls to pm_runtime_get/put
> +  * in this driver.
> +  */
> + device_link_add(dev, data->sysmmu, DL_FLAG_PM_RUNTIME);
> +
>   return 0;
>  }
>  
> -- 
> 1.9.1
> 
> 

-- 
Luis Rodriguez, SUSE LINUX GmbH
Maxfeldstrasse 5; D-90409 Nuernberg
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 2/2] iommu/exynos: Add proper runtime pm support

[Luis R. Rodriguez]

On Mon, Oct 10, 2016 at 03:32:06PM +0200, Marek Szyprowski wrote:
> Hi Luis
> 
> 
> On 2016-10-06 19:37, Luis R. Rodriguez wrote:
> > On Thu, Sep 29, 2016 at 10:12:31AM +0200, Marek Szyprowski wrote:
> > > This patch uses recently introduced device links to track the runtime pm
> > > state of the master's device. This way each SYSMMU controller is runtime
> > > activated when its master's device is active
> > instead of?
> 
> instead of keeping SYSMMU controller runtime active all the time.

I thought Rafael's work was for suspend/resume, not for runtime suspend.
Is it for both ? Because as far as I can tell this was painted to help
with suspend/resume ?

> > BTW what is the master device of a SYSMMU? I have no clue about these
> > IOMMU devices here.
> 
> Here is a more detailed description of IOMMU hardware I wrote a few days ago
> for Ulf:
> http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1231006.html
> 
> In short: there is a SYSMMU controller and its master device - a device,
> which performs DMA operations. That SYSMMU sits in between system memory
> and the master device, so it performs mapping of DMA addresses to physical
> memory addresses on each DMA operation.

So you seek a run time power optimization ? Or a fix on suspend? Or both?

> > > and can save/restore its state instead of being enabled all the time.
> > I take it this means currently even if the master device is disabled
> > (whatever that is) all SYSMMU controllers are kept enabled, is that right?
> > The issue here is this wastes power? Or what is the issue?
> 
> Yes, the issue here is the fact that SYSMMU is kept active all the time,
> what in turn prevent the power domain for turning off even if master device
> doesn't do anything and is already suspended. This directly (some clocks
> enabled) and in-directly (leakage current) causes power looses.

Thanks for the confirmation so really the biggest concern here was run time PM.

> > > This way SYSMMU controllers no
> > > longer prevents respective power domains to be turned off when master's
> > > device is not used.
> > So when the master device is idle we want to also remove power from the
> > controllers ? How much power does this save on a typical device in the
> > market BTW ?
> 
> The main purpose of this patchset is to let power domains to be turned off,
> because with the current code all domains are all the time turned on,
> because
> SYSMMU controllers prevent them from turning them off.

I see.. I think the audio folks already addressed this with DAPM, but granted
this was for audio. Then I was also referred to the DRM / Audio component
framework, has this been looked into? v4l folks have v4l async stuff but
its not clear if that help with run time PM. I'm mentioning these given it'd be
silly to re-invent the wheel, additionally if we now have a generic solution
everyone can jump on board with there is quite a bit of work we can do to
dump a lot of old legacy crap.

> If you want I can measure the power consumption of the idle board with all
> domains enabled and disabled if you want to see the numbers. On the other
> board
> disabling most power domains in idle state (the clocks were already
> disabled)
> gave me about 20mA savings (at 3.7V), what is a significant value for the
> battery powered device.

Thanks, this means nothing to me, however it would be value-add to the commit 
log
as anyone reviewing  this can understand what the goal / savings was for 
exactly.

> > 
> > > Signed-off-by: Marek Szyprowski <m.szyprow...@samsung.com>
> > > ---
> > >   drivers/iommu/exynos-iommu.c | 225 
> > > ++-
> > >   1 file changed, 94 insertions(+), 131 deletions(-)
> > I'm reviewing the device link patches now but since this is a demo of
> > use of that I'll note the changes here are pretty large and it makes
> > it terribly difficult for review. Is there any way this patch can be split
> > up in to logical atomic pieces that only do one task upon change ?
> 
> I will try to split it a bit, but I cannot promise that much can be done
> to improve readability for someone not very familiar with the driver
> internals.

I've heard this before, I don't buy it but lets see!

  Luis
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm

[Luis R. Rodriguez]

On Thu, Oct 20, 2016 at 09:22:53AM +0200, Marek Szyprowski wrote:
> This patch uses recently introduced device dependency links to track the
> runtime pm state of the master's device. This way each SYSMMU controller
> is set to runtime active only when its master's device is active and can
> restore or save its state instead of being activated all the time when
> attached to the given master device. This way SYSMMU controllers no longer
> prevents respective power domains to be turned off when master's device
> is not being used.

Its unclear why you need this based on this commit log -- is this
needed only on platforms that lack ACPI and use device tree ? If so
why? If this issue is present also on systems that only use ACPI is
this possibly due to an ACPI firmware bug  or the lack of some semantics
in ACPI to express ordering in a better way? If the issue is device
tree related only is this due to the lack of semantics in device tree
to express some more complex dependency ?

Has there been any review of the existing similar solutions out there
such as the DRM / audio component framework? Would that help ?

  Luis

> 
> Signed-off-by: Marek Szyprowski 
> ---
>  drivers/iommu/exynos-iommu.c | 16 
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
> index 5e6d7bbf9b70..59b4f2ce4f5f 100644
> --- a/drivers/iommu/exynos-iommu.c
> +++ b/drivers/iommu/exynos-iommu.c
> @@ -781,10 +781,6 @@ static void exynos_iommu_detach_device(struct 
> iommu_domain *iommu_domain,
>   if (!has_sysmmu(dev) || owner->domain != iommu_domain)
>   return;
>  
> - list_for_each_entry(data, >controllers, owner_node) {
> - pm_runtime_put_sync(data->sysmmu);
> - }
> -
>   mutex_lock(>rpm_lock);
>  
>   list_for_each_entry(data, >controllers, owner_node) {
> @@ -848,10 +844,6 @@ static int exynos_iommu_attach_device(struct 
> iommu_domain *iommu_domain,
>  
>   mutex_unlock(>rpm_lock);
>  
> - list_for_each_entry(data, >controllers, owner_node) {
> - pm_runtime_get_sync(data->sysmmu);
> - }
> -
>   dev_dbg(dev, "%s: Attached IOMMU with pgtable %pa\n", __func__,
>   );
>  
> @@ -1232,6 +1224,14 @@ static int exynos_iommu_of_xlate(struct device *dev,
>  
>   list_add_tail(>owner_node, >controllers);
>   data->master = dev;
> +
> + /*
> +  * SYSMMU will be runtime activated via device link (dependency) to its
> +  * master device, so there are no direct calls to pm_runtime_get/put
> +  * in this driver.
> +  */
> + device_link_add(dev, data->sysmmu, DL_FLAG_PM_RUNTIME);
> +
>   return 0;
>  }
>  
> -- 
> 1.9.1
> 
> 

-- 
Luis Rodriguez, SUSE LINUX GmbH
Maxfeldstrasse 5; D-90409 Nuernberg
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 2/2] iommu/exynos: Add proper runtime pm support

[Luis R. Rodriguez]

On Thu, Sep 29, 2016 at 10:12:31AM +0200, Marek Szyprowski wrote:
> This patch uses recently introduced device links to track the runtime pm
> state of the master's device. This way each SYSMMU controller is runtime
> activated when its master's device is active 

instead of?

BTW what is the master device of a SYSMMU? I have no clue about these
IOMMU devices here.

> and can save/restore its state instead of being enabled all the time.

I take it this means currently even if the master device is disabled
(whatever that is) all SYSMMU controllers are kept enabled, is that right?
The issue here is this wastes power? Or what is the issue?

> This way SYSMMU controllers no
> longer prevents respective power domains to be turned off when master's
> device is not used.

So when the master device is idle we want to also remove power from the
controllers ? How much power does this save on a typical device in the
market BTW ?

> Signed-off-by: Marek Szyprowski 
> ---
>  drivers/iommu/exynos-iommu.c | 225 
> ++-
>  1 file changed, 94 insertions(+), 131 deletions(-)

I'm reviewing the device link patches now but since this is a demo of
use of that I'll note the changes here are pretty large and it makes
it terribly difficult for review. Is there any way this patch can be split
up in to logical atomic pieces that only do one task upon change ?

  Luis
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFT v3] drm: use late_initcall() for amdkfd and radeon

[Luis R. Rodriguez]

On Tue, May 31, 2016 at 09:04:53PM +0200, Daniel Vetter wrote:
> On Tue, May 31, 2016 at 06:58:34PM +0200, Luis R. Rodriguez wrote:
> > On Sun, May 29, 2016 at 08:27:07PM +0200, Daniel Vetter wrote:
> > > On Fri, May 27, 2016 at 3:18 AM, Luis R. Rodriguez <mcg...@kernel.org> 
> > > wrote:
> > > > To get KFD support in radeon we need the following
> > > > initialization to happen in this order, their
> > > > respective driver file that has its init routine
> > > > listed next to it:
> > > >
> > > > 0. AMD IOMMUv1:arch/x86/kernel/pci-dma.c
> > > > 1. AMD IOMMUv2:drivers/iommu/amd_iommu_v2.c
> > > > 2. AMD KFD:drivers/gpu/drm/amd/amdkfd/kfd_module.c
> > > > 3. AMD Radeon: drivers/gpu/drm/radeon/radeon_drv.c
> > > >
> > > > Order is rather implicit, but these drivers can currently
> > > > only do so much given the amount of leg room available.
> > > > Below are the respective init routines and how they are
> > > > initialized:
> > > >
> > > > arch/x86/kernel/pci-dma.c   rootfs_initcall(pci_iommu_init);
> > > > drivers/iommu/amd_iommu_v2.cmodule_init(amd_iommu_v2_init);
> > > > drivers/gpu/drm/amd/amdkfd/kfd_module.c module_init(kfd_module_init);
> > > > drivers/gpu/drm/radeon/radeon_drv.c module_init(radeon_init);
> > > >
> > > > When a driver is built-in module_init() folds to use
> > > > device_initcall(), and we have the following possible
> > > > orders:
> > > >
> > > > #define pure_initcall(fn)__define_initcall(fn, 0)
> > > > #define core_initcall(fn)__define_initcall(fn, 1)
> > > > #define postcore_initcall(fn)__define_initcall(fn, 2)
> > > > #define arch_initcall(fn)__define_initcall(fn, 3)
> > > > #define subsys_initcall(fn)  __define_initcall(fn, 4)
> > > > #define fs_initcall(fn)  __define_initcall(fn, 5)
> > > > -
> > > > #define rootfs_initcall(fn)  __define_initcall(fn, rootfs)
> > > > #define device_initcall(fn)  __define_initcall(fn, 6)
> > > > #define late_initcall(fn)__define_initcall(fn, 7)
> > > >
> > > > Since we start off from rootfs_initcall(), it gives us 3 more
> > > > levels of leg room to play with for order semantics, this isn't
> > > > enough to address all required levels of dependencies, this
> > > > is specially true given that AMD-KFD needs to be loaded before
> > > > the radeon driver -- -but this it not enforced by symbols.
> > > > If the AMD-KFD driver is not loaded prior to the radeon driver
> > > > because otherwise the radeon driver will not initialize the
> > > > AMD-KFD driver and you get no KFD functionality in userspace.
> > > >
> > > > Commit 1bacc894c227fad8a7 ("drivers: Move iommu/ before gpu/ in
> > > > Makefile") works around some of the possibe races between
> > > > the AMD IOMMU v2 and GPU drivers by changing the link order.
> > > > This is fragile, however its the bets we can do, given that
> > > > making the GPU drivers use late_initcall() would also implicate
> > > > a similar race between them. That possible race is fortunatley
> > > > addressed given that the drm Makefile currently has amdkfd
> > > > linked prior to radeon:
> > > >
> > > > drivers/gpu/drm/Makefile
> > > > ...
> > > > obj-$(CONFIG_HSA_AMD) += amd/amdkfd/
> > > > obj-$(CONFIG_DRM_RADEON)+= radeon/
> > > > ...
> > > >
> > > > Changing amdkfd and radeon to late_initcall() however is
> > > > still the right call in orde to annotate explicitly a
> > > > delayed dependency requirement between the GPU drivers
> > > > and the IOMMUs.
> > > >
> > > > We can't address the fragile nature of the link order
> > > > right now, but in the future that might be possible.
> > > >
> > > > Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
> > > > ---
> > > >
> > > > Please note, the changes to drivers/Makefile are just
> > > > for the sake of forcing the possible race to occur,
> > > > if this works well the actual [PATCH] submission will
> > > > skip those changes as its pointles

Re: [RFT v3] drm: use late_initcall() for amdkfd and radeon

[Luis R. Rodriguez]

On Sun, May 29, 2016 at 08:27:07PM +0200, Daniel Vetter wrote:
> On Fri, May 27, 2016 at 3:18 AM, Luis R. Rodriguez <mcg...@kernel.org> wrote:
> > To get KFD support in radeon we need the following
> > initialization to happen in this order, their
> > respective driver file that has its init routine
> > listed next to it:
> >
> > 0. AMD IOMMUv1:arch/x86/kernel/pci-dma.c
> > 1. AMD IOMMUv2:drivers/iommu/amd_iommu_v2.c
> > 2. AMD KFD:drivers/gpu/drm/amd/amdkfd/kfd_module.c
> > 3. AMD Radeon: drivers/gpu/drm/radeon/radeon_drv.c
> >
> > Order is rather implicit, but these drivers can currently
> > only do so much given the amount of leg room available.
> > Below are the respective init routines and how they are
> > initialized:
> >
> > arch/x86/kernel/pci-dma.c   rootfs_initcall(pci_iommu_init);
> > drivers/iommu/amd_iommu_v2.cmodule_init(amd_iommu_v2_init);
> > drivers/gpu/drm/amd/amdkfd/kfd_module.c module_init(kfd_module_init);
> > drivers/gpu/drm/radeon/radeon_drv.c module_init(radeon_init);
> >
> > When a driver is built-in module_init() folds to use
> > device_initcall(), and we have the following possible
> > orders:
> >
> > #define pure_initcall(fn)__define_initcall(fn, 0)
> > #define core_initcall(fn)__define_initcall(fn, 1)
> > #define postcore_initcall(fn)__define_initcall(fn, 2)
> > #define arch_initcall(fn)__define_initcall(fn, 3)
> > #define subsys_initcall(fn)  __define_initcall(fn, 4)
> > #define fs_initcall(fn)  __define_initcall(fn, 5)
> > -
> > #define rootfs_initcall(fn)  __define_initcall(fn, rootfs)
> > #define device_initcall(fn)  __define_initcall(fn, 6)
> > #define late_initcall(fn)__define_initcall(fn, 7)
> >
> > Since we start off from rootfs_initcall(), it gives us 3 more
> > levels of leg room to play with for order semantics, this isn't
> > enough to address all required levels of dependencies, this
> > is specially true given that AMD-KFD needs to be loaded before
> > the radeon driver -- -but this it not enforced by symbols.
> > If the AMD-KFD driver is not loaded prior to the radeon driver
> > because otherwise the radeon driver will not initialize the
> > AMD-KFD driver and you get no KFD functionality in userspace.
> >
> > Commit 1bacc894c227fad8a7 ("drivers: Move iommu/ before gpu/ in
> > Makefile") works around some of the possibe races between
> > the AMD IOMMU v2 and GPU drivers by changing the link order.
> > This is fragile, however its the bets we can do, given that
> > making the GPU drivers use late_initcall() would also implicate
> > a similar race between them. That possible race is fortunatley
> > addressed given that the drm Makefile currently has amdkfd
> > linked prior to radeon:
> >
> > drivers/gpu/drm/Makefile
> > ...
> > obj-$(CONFIG_HSA_AMD) += amd/amdkfd/
> > obj-$(CONFIG_DRM_RADEON)+= radeon/
> > ...
> >
> > Changing amdkfd and radeon to late_initcall() however is
> > still the right call in orde to annotate explicitly a
> > delayed dependency requirement between the GPU drivers
> > and the IOMMUs.
> >
> > We can't address the fragile nature of the link order
> > right now, but in the future that might be possible.
> >
> > Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
> > ---
> >
> > Please note, the changes to drivers/Makefile are just
> > for the sake of forcing the possible race to occur,
> > if this works well the actual [PATCH] submission will
> > skip those changes as its pointless to remove those
> > work arounds as it stands, due to the limited nature
> > of the levels available for addressing requirements.
> >
> > Also, if you are aware of further dependency hell
> > things like these -- please do let me know as I am
> > interested in looking at addressing them.
> 
> This should be fixed with -EDEFER_PROBE instead. Frobbing initcall
> levels should then just be done as an optimization to avoid too much
> reprobing.

radeon already uses -EDEFER_PROBE but it assumes that amdkfd *is* loaded first,
and only if it is already loaded can it count on getting -EDEFER_PROBE. The
radeon driver will deffer probe *iff* kgd2kfd_init() returns -EDEFER_PROBE,
which only happens when amdkfd_init_completed is no longer present. If radeon
gets linked first though the symbol fetch for kgd2kfd_init() will make it as
not-present though. So the curren

Re: [RFT v3] drm: use late_initcall() for amdkfd and radeon

[Luis R. Rodriguez]

On Sun, May 29, 2016 at 05:49:17PM +0300, Oded Gabbay wrote:
> On Fri, May 27, 2016 at 4:18 AM, Luis R. Rodriguez <mcg...@kernel.org> wrote:
> > diff --git a/drivers/gpu/drm/radeon/radeon_drv.c 
> > b/drivers/gpu/drm/radeon/radeon_drv.c
> > index b55aa740171f..1fa1b7f3a89c 100644
> > --- a/drivers/gpu/drm/radeon/radeon_drv.c
> > +++ b/drivers/gpu/drm/radeon/radeon_drv.c
> > @@ -609,7 +609,7 @@ static void __exit radeon_exit(void)
> > radeon_unregister_atpx_handler();
> >  }
> >
> > -module_init(radeon_init);
> > +late_initcall(radeon_init);
> >  module_exit(radeon_exit);
> 
> Need to modify also amdgpu module_init

Thanks, so other than considering the first two hunks are only for testing 
purposes
(a proper [PATCH] will drop these hunks on the Makefile), you're suggestng this
then, is that right?:

---
 drivers/Makefile| 6 ++
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 2 +-
 drivers/gpu/drm/amd/amdkfd/kfd_module.c | 2 +-
 drivers/gpu/drm/radeon/radeon_drv.c | 2 +-
 4 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/Makefile b/drivers/Makefile
index 0b6f3d60193d..0fbe3982041f 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -50,10 +50,7 @@ obj-$(CONFIG_RESET_CONTROLLER)   += reset/
 obj-y  += tty/
 obj-y  += char/
 
-# iommu/ comes before gpu as gpu are using iommu controllers
-obj-$(CONFIG_IOMMU_SUPPORT)+= iommu/
-
-# gpu/ comes after char for AGP vs DRM startup and after iommu
+# gpu/ comes after char for AGP vs DRM startup
 obj-y  += gpu/
 
 obj-$(CONFIG_CONNECTOR)+= connector/
@@ -147,6 +144,7 @@ obj-y   += clk/
 
 obj-$(CONFIG_MAILBOX)  += mailbox/
 obj-$(CONFIG_HWSPINLOCK)   += hwspinlock/
+obj-$(CONFIG_IOMMU_SUPPORT)+= iommu/
 obj-$(CONFIG_REMOTEPROC)   += remoteproc/
 obj-$(CONFIG_RPMSG)+= rpmsg/
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 1dab5f2b725b..1ca448f2b4d2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -589,7 +589,7 @@ static void __exit amdgpu_exit(void)
amdgpu_sync_fini();
 }
 
-module_init(amdgpu_init);
+late_initcall(amdgpu_init);
 module_exit(amdgpu_exit);
 
 MODULE_AUTHOR(DRIVER_AUTHOR);
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_module.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_module.c
index 850a5623661f..3d1dab8a31c7 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_module.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_module.c
@@ -141,7 +141,7 @@ static void __exit kfd_module_exit(void)
dev_info(kfd_device, "Removed module\n");
 }
 
-module_init(kfd_module_init);
+late_initcall(kfd_module_init);
 module_exit(kfd_module_exit);
 
 MODULE_AUTHOR(KFD_DRIVER_AUTHOR);
diff --git a/drivers/gpu/drm/radeon/radeon_drv.c 
b/drivers/gpu/drm/radeon/radeon_drv.c
index b55aa740171f..1fa1b7f3a89c 100644
--- a/drivers/gpu/drm/radeon/radeon_drv.c
+++ b/drivers/gpu/drm/radeon/radeon_drv.c
@@ -609,7 +609,7 @@ static void __exit radeon_exit(void)
radeon_unregister_atpx_handler();
 }
 
-module_init(radeon_init);
+late_initcall(radeon_init);
 module_exit(radeon_exit);
 
 MODULE_AUTHOR(DRIVER_AUTHOR);
-- 
2.8.2


> I tested this on Kaveri, and amdkfd is working. For amdkfd that's
> fine, but IMO that's not enough testing for radeon/amdgpu. I would
> like to hear AMD's developers take on this.

Sure. Hopefully the above extensions suffice. In the future we should be able
to also replace the radeon amdkfd -EPROBE_DEFER kludge and the implicit
sensitivity in Makefiles over GPU drivers and IOMMUs, but a bit more work is
needed before that happens.

  Luis
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[RFT v3] drm: use late_initcall() for amdkfd and radeon

[Luis R. Rodriguez]

To get KFD support in radeon we need the following
initialization to happen in this order, their
respective driver file that has its init routine
listed next to it:

0. AMD IOMMUv1:arch/x86/kernel/pci-dma.c
1. AMD IOMMUv2:drivers/iommu/amd_iommu_v2.c
2. AMD KFD:drivers/gpu/drm/amd/amdkfd/kfd_module.c
3. AMD Radeon: drivers/gpu/drm/radeon/radeon_drv.c

Order is rather implicit, but these drivers can currently
only do so much given the amount of leg room available.
Below are the respective init routines and how they are
initialized:

arch/x86/kernel/pci-dma.c   rootfs_initcall(pci_iommu_init);
drivers/iommu/amd_iommu_v2.cmodule_init(amd_iommu_v2_init);
drivers/gpu/drm/amd/amdkfd/kfd_module.c module_init(kfd_module_init);
drivers/gpu/drm/radeon/radeon_drv.c module_init(radeon_init);

When a driver is built-in module_init() folds to use
device_initcall(), and we have the following possible
orders:

#define pure_initcall(fn)__define_initcall(fn, 0)
#define core_initcall(fn)__define_initcall(fn, 1)
#define postcore_initcall(fn)__define_initcall(fn, 2)
#define arch_initcall(fn)__define_initcall(fn, 3)
#define subsys_initcall(fn)  __define_initcall(fn, 4)
#define fs_initcall(fn)  __define_initcall(fn, 5)
-
#define rootfs_initcall(fn)  __define_initcall(fn, rootfs)
#define device_initcall(fn)  __define_initcall(fn, 6)
#define late_initcall(fn)__define_initcall(fn, 7)

Since we start off from rootfs_initcall(), it gives us 3 more
levels of leg room to play with for order semantics, this isn't
enough to address all required levels of dependencies, this
is specially true given that AMD-KFD needs to be loaded before
the radeon driver -- -but this it not enforced by symbols.
If the AMD-KFD driver is not loaded prior to the radeon driver
because otherwise the radeon driver will not initialize the
AMD-KFD driver and you get no KFD functionality in userspace.

Commit 1bacc894c227fad8a7 ("drivers: Move iommu/ before gpu/ in
Makefile") works around some of the possibe races between
the AMD IOMMU v2 and GPU drivers by changing the link order.
This is fragile, however its the bets we can do, given that
making the GPU drivers use late_initcall() would also implicate
a similar race between them. That possible race is fortunatley
addressed given that the drm Makefile currently has amdkfd
linked prior to radeon:

drivers/gpu/drm/Makefile
...
obj-$(CONFIG_HSA_AMD) += amd/amdkfd/
obj-$(CONFIG_DRM_RADEON)+= radeon/
...

Changing amdkfd and radeon to late_initcall() however is
still the right call in orde to annotate explicitly a
delayed dependency requirement between the GPU drivers
and the IOMMUs.

We can't address the fragile nature of the link order
right now, but in the future that might be possible.

Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---

Please note, the changes to drivers/Makefile are just
for the sake of forcing the possible race to occur,
if this works well the actual [PATCH] submission will
skip those changes as its pointless to remove those
work arounds as it stands, due to the limited nature
of the levels available for addressing requirements.

Also, if you are aware of further dependency hell
things like these -- please do let me know as I am
interested in looking at addressing them.

 drivers/Makefile| 6 ++
 drivers/gpu/drm/amd/amdkfd/kfd_module.c | 2 +-
 drivers/gpu/drm/radeon/radeon_drv.c | 2 +-
 3 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/Makefile b/drivers/Makefile
index 0b6f3d60193d..0fbe3982041f 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -50,10 +50,7 @@ obj-$(CONFIG_RESET_CONTROLLER)   += reset/
 obj-y  += tty/
 obj-y  += char/
 
-# iommu/ comes before gpu as gpu are using iommu controllers
-obj-$(CONFIG_IOMMU_SUPPORT)+= iommu/
-
-# gpu/ comes after char for AGP vs DRM startup and after iommu
+# gpu/ comes after char for AGP vs DRM startup
 obj-y  += gpu/
 
 obj-$(CONFIG_CONNECTOR)+= connector/
@@ -147,6 +144,7 @@ obj-y   += clk/
 
 obj-$(CONFIG_MAILBOX)  += mailbox/
 obj-$(CONFIG_HWSPINLOCK)   += hwspinlock/
+obj-$(CONFIG_IOMMU_SUPPORT)+= iommu/
 obj-$(CONFIG_REMOTEPROC)   += remoteproc/
 obj-$(CONFIG_RPMSG)+= rpmsg/
 
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_module.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_module.c
index 850a5623661f..3d1dab8a31c7 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_module.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_module.c
@@ -141,7 +141,7 @@ static void __exit kfd_module_exit(void)
dev_info(kfd_device, "Removed module\n");
 }
 
-module_init(kfd_module_init);
+late_initcall(kfd_module_init);
 module_exit(kfd_modu

Re: [RFT v2] iommu/amd: use subsys_initcall() on amdv2 iommu

[Luis R. Rodriguez]

On Mon, Apr 25, 2016 at 12:23:51PM +0200, Joerg Roedel wrote:
> On Mon, Apr 18, 2016 at 02:03:50PM +0200, Luis R. Rodriguez wrote:
> > You said that with my patch you saw AMD IOMMUv2 kick off first,
> > that was intentional as I thought that's what you needed. Can
> > someone please describe the requirements?
> > 
> > Also what does drm use that you say has a conflict already? What
> > drm code are we talking about exactly ?
> 
> The required order is:
> 
>   1. AMD IOMMUv1 (in-kernel only, initialized at boot time after PCI)
>   2. AMD IOMMUv2 (in-kernel or as module, provides demand paging
>   and uses v1 interfaces to talk to the IOMMU)
>   3. AMD-KFD (Implements compute offloading to the GPU and
>   uses AMD IOMMUv2 functionality, also provides a
>   symbol for the radeon driver)
>   4. DRM with(Checks if the symbol provided by AMD-KFD is
>  Radeon   available at init time and does the KFD
>   initialization from there, because the GPU needs
>   to be up first)
> 
> So AMD IOMMUv2 does not initialize (but it does load to have the symbols
> available for drivers that optionally use its functionality) without the
> AMD IOMMUv1 driver, as it can't access the IOMMU hardware then.
> 
> AMD-KFD does not load without AMD IOMMUv2 being loaded, as it depends on
> its symbols. AMD-KFD on the other hand needs to be loaded before the
> radeon driver (but this it not enforced by symbols), because otherwise
> the radeon driver will not initialize the AMD-KFD driver.
> 
> When AMD-KFD is loaded and you load radeon then, you get the KFD
> functionality in the kernel. Then you can move on to the fun getting the
> userspace running and actually execute anything on the GPU. But thats
> another story.
> 
> I know what you think, and I agree: It's a big mess :)

Its no concern for me that its a mess, frankly most drivers are. This however
illustrates a semantic gap in the driver core when you have more than 3
dependencies after a rootfs_initcall(), given that the buck stops at
late_initcall(), only 3 levels above. With 4 dependencies you run out
of semantics to specify explicit requirements.

Ties can be disputed through link order though, but this is obviously fragile,
and the dependencies are then left implicit. Nothing vets for correctness,
either in software or through static analysers.

To summarize the specific code in question is (and order required):

AMD IOMMUv1:arch/x86/kernel/pci-dma.c   
rootfs_initcall(pci_iommu_init);
AMD IOMMUv2:drivers/iommu/amd_iommu_v2.c
module_init(amd_iommu_v2_init); (device_initcall())
AMD KFD:drivers/gpu/drm/amd/amdkfd/kfd_module.c 
module_init(kfd_module_init); (device_initcall())
AMD Radeon: drivers/gpu/drm/radeon/radeon_drv.c 
module_init(radeon_init); (device_initcall())

Commit 1bacc894c227fad8a7 ("drivers: Move iommu/ before gpu/ in Makefile") 
resolved
the race between IOMMU and GPU drivers this way, however an alternative is to 
make
the dependencies in levels explicit by making the amdkfd and radeon drivers both
use late_initcall(), this however is technically still racy specially since you
note that the amdkfd driver is not loaded prior to radeon through symbol
dependencies, if happens to be loaded it then you get KFD functionality,
otherwise you don't.

Making both amdkfd and radeon use late_initcall() would actually work now though
but that's only because the link order also happens to match the dependency
requirements:

drivers/gpu/drm/Makefile
...
obj-$(CONFIG_HSA_AMD) += amd/amdkfd/
obj-$(CONFIG_DRM_RADEON)+= radeon/  
...

Since we currently lack proper semantics to define clear dependencies for all
this reverting 1bacc894c227fad8a7 after using late_initcall() on both amdkfd
and radeon would not be useful other than to just test the race fix, given that
such work around would still be present also on drivers/gpu/drm/Makefile to
account for the race between amdkfd and radeon. Reverting 1bacc894c227fad8a7
after using late_initcall() on both amdkfd and radeon would just bump the
race work around another level.

Modifying both amdkfd and radeon to use late_initcall() however seems well
justified for now, and given the current state of affairs with link order
one should be able to then correctly build all these things as built-in
and it should work correctly.

I'm still not satisfied with the issues on semantics here though. A fix
for that, if desired, should be possible using linker-tables, currently
in RFCv2 [0] stage. Linker tables offer practically infinite number (as
long as a valid ELF section name, as the order level is part of the
section name) of order levels. It also would offer the opportunity
to build 

Re: [RFT v2] iommu/amd: use subsys_initcall() on amdv2 iommu

[Luis R. Rodriguez]

On Tue, Apr 19, 2016 at 10:02:52AM +0800, Wan Zongshun wrote:
> 
> You have to take carefully to arrange the calling sequence for
> iommuv1, iommuv2, kfd module, and drm like the following sequence :
> v1 ->v2->kfd, drm.
> 
> iommuv1 -- rootfs_initcall(fn)
> IOMMUV2 -- device_initcall(fn)
> kfd module -- late_initcall(fn)
> drm -- late_initcall(fn)

Thanks, it turns out this is not exactly enough, given as 
Joerg notes:

--
AMD-KFD on the other hand needs to be loaded before the 
 
radeon driver (but this it not enforced by symbols), because otherwise  
  
the radeon driver will not initialize the AMD-KFD driver. 
---

We have a theoretical race still possible between the kfd module
and the drm driver. I'll reply to Joerg's e-mail with more feedback.

  Luis
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFT v2] iommu/amd: use subsys_initcall() on amdv2 iommu

[Luis R. Rodriguez]

On Mon, Apr 18, 2016 at 10:02:24AM +0300, Oded Gabbay wrote:
> On Mon, Apr 18, 2016 at 9:55 AM, Luis R. Rodriguez <mcg...@kernel.org> wrote:
> >
> > On Apr 18, 2016 7:48 AM, "Oded Gabbay" <oded.gab...@gmail.com> wrote:
> >>
> >> On Wed, Apr 13, 2016 at 1:07 AM, Luis R. Rodriguez <mcg...@kernel.org>
> >> wrote:
> >> > On Mon, Apr 11, 2016 at 03:52:43PM +0200, Christian König wrote:
> >> >> Am 11.04.2016 um 15:39 schrieb Oded Gabbay:
> >> >> >On Mon, Apr 11, 2016 at 4:28 PM, Christian König
> >> >> ><christian.koe...@amd.com> wrote:
> >> >> >>Am 09.04.2016 um 02:25 schrieb Luis R. Rodriguez:
> >> >> >>>On Tue, Mar 29, 2016 at 10:41 AM, Luis R. Rodriguez
> >> >> >>> <mcg...@kernel.org> wrote:
> >> >> >>>>We need to ensure amd iommu v2 initializes before
> >> >> >>>>driver uses such as drivers/gpu/drm/amd/amdkfd/kfd_module.c,
> >> >> >>>>to do this make its init routine a subsys_initcall() which
> >> >> >>>>ensures its load init is called first than modules when
> >> >> >>>>built-in.
> >> >> >>>>
> >> >> >>>>This reverts the old work around implemented through commit
> >> >> >>>>1bacc894c227fad8a7 ("drivers: Move iommu/ before gpu/ in
> >> >> >>>> Makefile"),
> >> >> >>>>instead of making the dependency implicit by linker order this
> >> >> >>>>makes the ordering requirement explicit through proper kernel
> >> >> >>>>APIs.
> >> >> >>>>
> >> >> >>>>Cc: Oded Gabbay <oded.gab...@amd.com>
> >> >> >>>>Cc: Christian König <christian.koe...@amd.com>
> >> >> >>>>Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
> >> >> >>
> >> >> >>Sorry for not responding earlier. Just coming back to all the stuff
> >> >> >> on my TODO list.
> >> >> >>
> >> >> >>Patch is Acked-by: Christian König <christian.koe...@amd.com>
> >> >> >
> >> >> >Christian,
> >> >> >Just wanted to be sure if you tested this patch-set or not.
> >> >>
> >> >> I did NOT tested it. If AMD IOMMU requires something which will now
> >> >> initialize after the IOMMU module we will obviously run into trouble
> >> >> again.
> >> >>
> >> >> I assumed that the creator of the patch did some testing.
> >> >
> >> > Nope, hence [RTF] Request For Testing.
> >> >
> >> >> >I don't think it should be merged without testing. If you already
> >> >> >tested it than fine. If not, I think I can do it in the next week or
> >> >> >so (just came back from PTO).
> >> >>
> >> >> Yeah, agree totally.
> >> >
> >> > Agreed, please let me know if someone is able to test and confirm
> >> > this works. It should work.
> >> >
> >> >   Luis
> >>
> >> Hi,
> >> So I finally got to test this patch and it's not working.
> >> The reason is that AMD IOMMUv2 gets initialized *before* AMD IOMMUv1
> >> driver !
> >
> > Thanks can you try using late_initcall() instead then?
> >
> >   Luis
> 
> That will make it initialize *after* drm subsystem, which will cause
> another bug.

Hold up, I thought that we needed AMD IOMMUv2 to get initialized
before AMD IOMMUv1 ? That's what the patch did. Can someone clarify
the requirements then?

I'll provide some review of the current state of affairs first, without the
patch. AMD IOMMUv1 uses x86_init.iommu.iommu_init and that has its own init
semantics. Specifically that gets called via pci_iommu_init() which is pegged
on the init order via rootfs_initcall(pci_iommu_init);

Then AMD IOMMUv2 uses module_init() and that when is built-in falls on to
__initcall() which is device_initcall().

The order is:

#define pure_initcall(fn)   __define_initcall(fn, 0)

#define core_initcall(fn)   __define_initcall(fn, 1)
#define core_initcall_sync(fn)  __define_initcall(fn, 1s)   
#define postcore_initcall(fn)   __define_initcall(fn, 2) 

Re: [RFT v2] iommu/amd: use subsys_initcall() on amdv2 iommu

[Luis R. Rodriguez]

On Apr 18, 2016 7:48 AM, "Oded Gabbay" <oded.gab...@gmail.com> wrote:
>
> On Wed, Apr 13, 2016 at 1:07 AM, Luis R. Rodriguez <mcg...@kernel.org>
wrote:
> > On Mon, Apr 11, 2016 at 03:52:43PM +0200, Christian König wrote:
> >> Am 11.04.2016 um 15:39 schrieb Oded Gabbay:
> >> >On Mon, Apr 11, 2016 at 4:28 PM, Christian König
> >> ><christian.koe...@amd.com> wrote:
> >> >>Am 09.04.2016 um 02:25 schrieb Luis R. Rodriguez:
> >> >>>On Tue, Mar 29, 2016 at 10:41 AM, Luis R. Rodriguez <
mcg...@kernel.org> wrote:
> >> >>>>We need to ensure amd iommu v2 initializes before
> >> >>>>driver uses such as drivers/gpu/drm/amd/amdkfd/kfd_module.c,
> >> >>>>to do this make its init routine a subsys_initcall() which
> >> >>>>ensures its load init is called first than modules when
> >> >>>>built-in.
> >> >>>>
> >> >>>>This reverts the old work around implemented through commit
> >> >>>>1bacc894c227fad8a7 ("drivers: Move iommu/ before gpu/ in
Makefile"),
> >> >>>>instead of making the dependency implicit by linker order this
> >> >>>>makes the ordering requirement explicit through proper kernel
> >> >>>>APIs.
> >> >>>>
> >> >>>>Cc: Oded Gabbay <oded.gab...@amd.com>
> >> >>>>Cc: Christian König <christian.koe...@amd.com>
> >> >>>>Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
> >> >>
> >> >>Sorry for not responding earlier. Just coming back to all the stuff
on my TODO list.
> >> >>
> >> >>Patch is Acked-by: Christian König <christian.koe...@amd.com>
> >> >
> >> >Christian,
> >> >Just wanted to be sure if you tested this patch-set or not.
> >>
> >> I did NOT tested it. If AMD IOMMU requires something which will now
> >> initialize after the IOMMU module we will obviously run into trouble
> >> again.
> >>
> >> I assumed that the creator of the patch did some testing.
> >
> > Nope, hence [RTF] Request For Testing.
> >
> >> >I don't think it should be merged without testing. If you already
> >> >tested it than fine. If not, I think I can do it in the next week or
> >> >so (just came back from PTO).
> >>
> >> Yeah, agree totally.
> >
> > Agreed, please let me know if someone is able to test and confirm
> > this works. It should work.
> >
> >   Luis
>
> Hi,
> So I finally got to test this patch and it's not working.
> The reason is that AMD IOMMUv2 gets initialized *before* AMD IOMMUv1
driver !

Thanks can you try using late_initcall() instead then?

  Luis
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFT v2] iommu/amd: use subsys_initcall() on amdv2 iommu

[Luis R. Rodriguez]

On Mon, Apr 11, 2016 at 03:52:43PM +0200, Christian König wrote:
> Am 11.04.2016 um 15:39 schrieb Oded Gabbay:
> >On Mon, Apr 11, 2016 at 4:28 PM, Christian König
> ><christian.koe...@amd.com> wrote:
> >>Am 09.04.2016 um 02:25 schrieb Luis R. Rodriguez:
> >>>On Tue, Mar 29, 2016 at 10:41 AM, Luis R. Rodriguez <mcg...@kernel.org> 
> >>>wrote:
> >>>>We need to ensure amd iommu v2 initializes before
> >>>>driver uses such as drivers/gpu/drm/amd/amdkfd/kfd_module.c,
> >>>>to do this make its init routine a subsys_initcall() which
> >>>>ensures its load init is called first than modules when
> >>>>built-in.
> >>>>
> >>>>This reverts the old work around implemented through commit
> >>>>1bacc894c227fad8a7 ("drivers: Move iommu/ before gpu/ in Makefile"),
> >>>>instead of making the dependency implicit by linker order this
> >>>>makes the ordering requirement explicit through proper kernel
> >>>>APIs.
> >>>>
> >>>>Cc: Oded Gabbay <oded.gab...@amd.com>
> >>>>Cc: Christian König <christian.koe...@amd.com>
> >>>>Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
> >>
> >>Sorry for not responding earlier. Just coming back to all the stuff on my 
> >>TODO list.
> >>
> >>Patch is Acked-by: Christian König <christian.koe...@amd.com>
> >
> >Christian,
> >Just wanted to be sure if you tested this patch-set or not.
> 
> I did NOT tested it. If AMD IOMMU requires something which will now
> initialize after the IOMMU module we will obviously run into trouble
> again.
> 
> I assumed that the creator of the patch did some testing.

Nope, hence [RTF] Request For Testing.

> >I don't think it should be merged without testing. If you already
> >tested it than fine. If not, I think I can do it in the next week or
> >so (just came back from PTO).
> 
> Yeah, agree totally.

Agreed, please let me know if someone is able to test and confirm
this works. It should work.

  Luis
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFT v2] iommu/amd: use subsys_initcall() on amdv2 iommu

[Luis R. Rodriguez]

On Tue, Mar 29, 2016 at 10:41 AM, Luis R. Rodriguez <mcg...@kernel.org> wrote:
> We need to ensure amd iommu v2 initializes before
> driver uses such as drivers/gpu/drm/amd/amdkfd/kfd_module.c,
> to do this make its init routine a subsys_initcall() which
> ensures its load init is called first than modules when
> built-in.
>
> This reverts the old work around implemented through commit
> 1bacc894c227fad8a7 ("drivers: Move iommu/ before gpu/ in Makefile"),
> instead of making the dependency implicit by linker order this
> makes the ordering requirement explicit through proper kernel
> APIs.
>
> Cc: Oded Gabbay <oded.gab...@amd.com>
> Cc: Christian König <christian.koe...@amd.com>
> Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>

*poke*

 Luis
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[RFT v2] iommu/amd: use subsys_initcall() on amdv2 iommu

[Luis R. Rodriguez]

We need to ensure amd iommu v2 initializes before
driver uses such as drivers/gpu/drm/amd/amdkfd/kfd_module.c,
to do this make its init routine a subsys_initcall() which
ensures its load init is called first than modules when
built-in.

This reverts the old work around implemented through commit
1bacc894c227fad8a7 ("drivers: Move iommu/ before gpu/ in Makefile"),
instead of making the dependency implicit by linker order this
makes the ordering requirement explicit through proper kernel
APIs.

Cc: Oded Gabbay <oded.gab...@amd.com>
Cc: Christian König <christian.koe...@amd.com>
Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---

This goes along with the revert included. Can someone please test?

 drivers/Makefile | 6 ++
 drivers/iommu/amd_iommu_v2.c | 2 +-
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/Makefile b/drivers/Makefile
index 8f5d076baeb0..cc3cfbddc376 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -50,10 +50,7 @@ obj-$(CONFIG_RESET_CONTROLLER)   += reset/
 obj-y  += tty/
 obj-y  += char/
 
-# iommu/ comes before gpu as gpu are using iommu controllers
-obj-$(CONFIG_IOMMU_SUPPORT)+= iommu/
-
-# gpu/ comes after char for AGP vs DRM startup and after iommu
+# gpu/ comes after char for AGP vs DRM startup
 obj-y  += gpu/
 
 obj-$(CONFIG_CONNECTOR)+= connector/
@@ -146,6 +143,7 @@ obj-y   += clk/
 
 obj-$(CONFIG_MAILBOX)  += mailbox/
 obj-$(CONFIG_HWSPINLOCK)   += hwspinlock/
+obj-$(CONFIG_IOMMU_SUPPORT)+= iommu/
 obj-$(CONFIG_REMOTEPROC)   += remoteproc/
 obj-$(CONFIG_RPMSG)+= rpmsg/
 
diff --git a/drivers/iommu/amd_iommu_v2.c b/drivers/iommu/amd_iommu_v2.c
index 56999d2fac07..60df645b9927 100644
--- a/drivers/iommu/amd_iommu_v2.c
+++ b/drivers/iommu/amd_iommu_v2.c
@@ -1004,5 +1004,5 @@ static void __exit amd_iommu_v2_exit(void)
destroy_workqueue(iommu_wq);
 }
 
-module_init(amd_iommu_v2_init);
+subsys_initcall(amd_iommu_v2_init);
 module_exit(amd_iommu_v2_exit);
-- 
2.7.2

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFT] iommu/amd: use subsys_initcall() on amdv2 iommu

[Luis R. Rodriguez]

On Wed, Mar 16, 2016 at 05:39:00PM +0100, Joerg Roedel wrote:
> On Wed, Mar 16, 2016 at 05:17:37PM +0100, Luis R. Rodriguez wrote:
> > On Wed, Mar 16, 2016 at 12:16:57PM +0200, Oded Gabbay wrote:
> > > In theory it should, but I would prefer that it would be tested on
> > > actual hardware.
> > 
> > Hence, RFT. Anyone have hardware to test ? And would the other hack
> > mentioned need to be reverted first ?
> 
> Yes, the other hack needs to be reverted first to see if this one helps.

I'm afraid I am not sure where that hack is, can someone construct a patch to
revert that so this is a proper RFT series ?

  Luis
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[RFT] iommu/amd: use subsys_initcall() on amdv2 iommu

[Luis R. Rodriguez]

We need to ensure amd iommu v2 initializes before
driver uses such as drivers/gpu/drm/amd/amdkfd/kfd_module.c,
to do this make its init routine a subsys_initcall() which
ensures its load init is called first than modules when
built-in.

Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---

Can someone test if this patch enables both CONFIG_AMD_IOMMU_V2 and
CONFIG_HSA_AMD to be =y (built-in) without any conflicts ?

 drivers/iommu/amd_iommu_v2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/amd_iommu_v2.c b/drivers/iommu/amd_iommu_v2.c
index 56999d2fac07..60df645b9927 100644
--- a/drivers/iommu/amd_iommu_v2.c
+++ b/drivers/iommu/amd_iommu_v2.c
@@ -1004,5 +1004,5 @@ static void __exit amd_iommu_v2_exit(void)
destroy_workqueue(iommu_wq);
 }
 
-module_init(amd_iommu_v2_init);
+subsys_initcall(amd_iommu_v2_init);
 module_exit(amd_iommu_v2_exit);
-- 
2.7.2

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFT] iommu/amd: use subsys_initcall() on amdv2 iommu

[Luis R. Rodriguez]

On Wed, Mar 16, 2016 at 12:16:57PM +0200, Oded Gabbay wrote:
> On Wed, Mar 16, 2016 at 12:14 PM, Joerg Roedel  wrote:
> > On Wed, Mar 16, 2016 at 09:02:43AM +0200, Oded Gabbay wrote:
> >> fwiw, we currently have this covered by the ugly hack of putting iommu
> >> subsystem in front of gpu subsystem in the main drivers makefile (See
> >> 1bacc894c227fad8a727eb99728df708eba57654)
> >
> > Sure, but the above is a less ugly hack to solve the problem. So the
> > question is, would that work too?
> >
> >
> > Joerg
> >
> 
> In theory it should, but I would prefer that it would be tested on
> actual hardware.

Hence, RFT. Anyone have hardware to test ? And would the other hack
mentioned need to be reverted first ?

  Luis
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu