| Message ID | 1476948173-21093-8-git-send-email-m.szyprowski@samsung.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
Hi Marek, >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. > >Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> >--- > 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, &owner->controllers, owner_node) { >- pm_runtime_put_sync(data->sysmmu); >- } >- > mutex_lock(&owner->rpm_lock); > > list_for_each_entry(data, &owner->controllers, owner_node) { >@@ -848,10 +844,6 @@ static int exynos_iommu_attach_device(struct iommu_domain *iommu_domain, > > mutex_unlock(&owner->rpm_lock); > >- list_for_each_entry(data, &owner->controllers, owner_node) { >- pm_runtime_get_sync(data->sysmmu); >- } >- > dev_dbg(dev, "%s: Attached IOMMU with pgtable %pa\n", __func__, > &pagetable); > >@@ -1232,6 +1224,14 @@ static int exynos_iommu_of_xlate(struct device *dev, > > list_add_tail(&data->owner_node, &owner->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); >+ So in the case of master with multiple sids, this would be called multiple times for the same master ? Regards, Sricharan -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Sricharan, On 2016-10-23 11:49, Sricharan wrote: > Hi Marek, >> 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. >> >> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> >> --- >> 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, &owner->controllers, owner_node) { >> - pm_runtime_put_sync(data->sysmmu); >> - } >> - >> mutex_lock(&owner->rpm_lock); >> >> list_for_each_entry(data, &owner->controllers, owner_node) { >> @@ -848,10 +844,6 @@ static int exynos_iommu_attach_device(struct iommu_domain *iommu_domain, >> >> mutex_unlock(&owner->rpm_lock); >> >> - list_for_each_entry(data, &owner->controllers, owner_node) { >> - pm_runtime_get_sync(data->sysmmu); >> - } >> - >> dev_dbg(dev, "%s: Attached IOMMU with pgtable %pa\n", __func__, >> &pagetable); >> >> @@ -1232,6 +1224,14 @@ static int exynos_iommu_of_xlate(struct device *dev, >> >> list_add_tail(&data->owner_node, &owner->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); >> + > So in the case of master with multiple sids, this would be called multiple times > for the same master ? I don't know what is "multiple sids" case, but if given SYSMMU master device (supplier) has multiple SYSMMU controllers (consumers), then links will be created for each SYSMMU controller. Please note that this code is based on vanilla v4.9-rc1, which calls of_xlate() callback only once for every iommu for given master device. Your IOMMU deferred probe patches change this, but I already posted a fix for Exynos IOMMU driver to handle such case. Best regards
Hi Marek, >>> 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. >>> >>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> >>> --- >>> 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, &owner->controllers, owner_node) { >>> - pm_runtime_put_sync(data->sysmmu); >>> - } >>> - >>> mutex_lock(&owner->rpm_lock); >>> >>> list_for_each_entry(data, &owner->controllers, owner_node) { >>> @@ -848,10 +844,6 @@ static int exynos_iommu_attach_device(struct iommu_domain *iommu_domain, >>> >>> mutex_unlock(&owner->rpm_lock); >>> >>> - list_for_each_entry(data, &owner->controllers, owner_node) { >>> - pm_runtime_get_sync(data->sysmmu); >>> - } >>> - >>> dev_dbg(dev, "%s: Attached IOMMU with pgtable %pa\n", __func__, >>> &pagetable); >>> >>> @@ -1232,6 +1224,14 @@ static int exynos_iommu_of_xlate(struct device *dev, >>> >>> list_add_tail(&data->owner_node, &owner->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); >>> + >> So in the case of master with multiple sids, this would be called multiple times >> for the same master ? > >I don't know what is "multiple sids" case, but if given SYSMMU master >device (supplier) >has multiple SYSMMU controllers (consumers), then links will be created >for each SYSMMU >controller. Please note that this code is based on vanilla v4.9-rc1, >which calls >of_xlate() callback only once for every iommu for given master device. >Your IOMMU >deferred probe patches change this, but I already posted a fix for >Exynos IOMMU driver >to handle such case. By multiple sids, i meant iommus = <&phandle sid1 sid2 .. sidn> case, so xlate would be called multiples for the same master without deferred probing also. But the fix that you showed on the other thread would work here as well or maybe if you dont have masters with multiple sids you wont have any issues as well. Regards, Sricharan -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Sricharan, On 2016-10-24 14:29, Sricharan 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. >>>> >>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> >>>> --- >>>> 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, &owner->controllers, owner_node) { >>>> - pm_runtime_put_sync(data->sysmmu); >>>> - } >>>> - >>>> mutex_lock(&owner->rpm_lock); >>>> >>>> list_for_each_entry(data, &owner->controllers, owner_node) { >>>> @@ -848,10 +844,6 @@ static int exynos_iommu_attach_device(struct iommu_domain *iommu_domain, >>>> >>>> mutex_unlock(&owner->rpm_lock); >>>> >>>> - list_for_each_entry(data, &owner->controllers, owner_node) { >>>> - pm_runtime_get_sync(data->sysmmu); >>>> - } >>>> - >>>> dev_dbg(dev, "%s: Attached IOMMU with pgtable %pa\n", __func__, >>>> &pagetable); >>>> >>>> @@ -1232,6 +1224,14 @@ static int exynos_iommu_of_xlate(struct device *dev, >>>> >>>> list_add_tail(&data->owner_node, &owner->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); >>>> + >>> So in the case of master with multiple sids, this would be called multiple times >>> for the same master ? >> I don't know what is "multiple sids" case, but if given SYSMMU master >> device (supplier) >> has multiple SYSMMU controllers (consumers), then links will be created >> for each SYSMMU >> controller. Please note that this code is based on vanilla v4.9-rc1, >> which calls >> of_xlate() callback only once for every iommu for given master device. >> Your IOMMU >> deferred probe patches change this, but I already posted a fix for >> Exynos IOMMU driver >> to handle such case. > By multiple sids, i meant iommus = <&phandle sid1 sid2 .. sidn> case, > so xlate would be called multiples for the same master without deferred > probing also. But the fix that you showed on the other thread would work > here as well or maybe if you dont have masters with multiple sids you wont > have any issues as well. Exynos SYSMMU driver always use "#iommu-cells = <0>", so it doesn't support multiple sids. However there is a case with 2 SYSMMU controllers attached to the same master device: "iommus = <&sysmmu_mfc_l>, <&sysmmu_mfc_r>;". Best regards
Hi Marek, >>>>> 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, &owner->controllers, owner_node) { >>>>> - pm_runtime_put_sync(data->sysmmu); >>>>> - } >>>>> - >>>>> mutex_lock(&owner->rpm_lock); >>>>> >>>>> list_for_each_entry(data, &owner->controllers, owner_node) { >>>>> @@ -848,10 +844,6 @@ static int exynos_iommu_attach_device(struct iommu_domain *iommu_domain, >>>>> >>>>> mutex_unlock(&owner->rpm_lock); >>>>> >>>>> - list_for_each_entry(data, &owner->controllers, owner_node) { >>>>> - pm_runtime_get_sync(data->sysmmu); >>>>> - } >>>>> - >>>>> dev_dbg(dev, "%s: Attached IOMMU with pgtable %pa\n", __func__, >>>>> &pagetable); >>>>> >>>>> @@ -1232,6 +1224,14 @@ static int exynos_iommu_of_xlate(struct device *dev, >>>>> >>>>> list_add_tail(&data->owner_node, &owner->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); >>>>> + >>>> So in the case of master with multiple sids, this would be called multiple times >>>> for the same master ? >>> I don't know what is "multiple sids" case, but if given SYSMMU master >>> device (supplier) >>> has multiple SYSMMU controllers (consumers), then links will be created >>> for each SYSMMU >>> controller. Please note that this code is based on vanilla v4.9-rc1, >>> which calls >>> of_xlate() callback only once for every iommu for given master device. >>> Your IOMMU >>> deferred probe patches change this, but I already posted a fix for >>> Exynos IOMMU driver >>> to handle such case. >> By multiple sids, i meant iommus = <&phandle sid1 sid2 .. sidn> case, >> so xlate would be called multiples for the same master without deferred >> probing also. But the fix that you showed on the other thread would work >> here as well or maybe if you dont have masters with multiple sids you wont >> have any issues as well. > >Exynos SYSMMU driver always use "#iommu-cells = <0>", so it doesn't support >multiple sids. However there is a case with 2 SYSMMU controllers attached >to the same master device: "iommus = <&sysmmu_mfc_l>, <&sysmmu_mfc_r>;". > with iommu-cells = <0> always, its ok. The case of 2 SYSMMU controllers that is shown above is fine, as anyways both the suppliers (symmu) needs to linked seperately. So it looks all fine. Regards, Sricharan -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <m.szyprowski@samsung.com> > --- > 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, &owner->controllers, owner_node) { > - pm_runtime_put_sync(data->sysmmu); > - } > - > mutex_lock(&owner->rpm_lock); > > list_for_each_entry(data, &owner->controllers, owner_node) { > @@ -848,10 +844,6 @@ static int exynos_iommu_attach_device(struct iommu_domain *iommu_domain, > > mutex_unlock(&owner->rpm_lock); > > - list_for_each_entry(data, &owner->controllers, owner_node) { > - pm_runtime_get_sync(data->sysmmu); > - } > - > dev_dbg(dev, "%s: Attached IOMMU with pgtable %pa\n", __func__, > &pagetable); > > @@ -1232,6 +1224,14 @@ static int exynos_iommu_of_xlate(struct device *dev, > > list_add_tail(&data->owner_node, &owner->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 > >
Hi Luis, On 2016-11-07 22:47, Luis R. Rodriguez wrote: > 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 ? Nope, it has nothing to device tree nor ACPI. The dependency is a direct result of the way the devices operate. > 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 ? The main feature of device links that is used in this patch is enabling runtime pm dependency between Exynos SYSMMU controller (called it client device) and the device, for which it implements DMA address translation (called master device). The assumptions are following: 1. master device driver is completely unaware of the Exynos SYSMMU presence, IOMMU is transparently hooked up and managed by DMA-mapping framework 2. SYSMMU belongs to the same power domain as it's master device 3. SYSMMU is optional, master device can fully operate without it, with simple DMA address translation (DMA address == physical address) 4. Master device implements runtime pm, what in turn causes respective power domain to be turned on/off 5. DMA-mapping and IOMMU frameworks provides no calls to notify SYSMMU when its master device is performing DMA operations, so SYSMMU has to be runtime active 6. Currently SYSMMU always sets its runtime pm status to active after attaching to its master device to ensure proper hardware state. This prevents power domain to be turned off, even when master device sets its runtime pm status to suspended. 7. Exynos SYSMMU has to be runtime active at the same time when its master device is runtime active to it to perform DMA operations and allow the power domain to be turned off, when master device is runtime suspended. 8. The terms of device links, Exynos SYSMMU is a 'consumer' and master device is a 'supplier'. > 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. Best regards
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 Thanks, Lukas -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 8, 2016 at 4:30 PM, Lukas Wunner <lukas@wunner.de> 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 Well, that clearly fell off my radar, sorry about that. The idea, roughly, is that if there is a single on/off switch acting on multiple devices, you can (a) set up a PM domain tracking all of those device's runtime PM invocations and (b) maintaining a reference counter of devices still not suspended. This way it would only turn the switch off when all of the devices in question had been suspended. Analogously, it would turn the switch on before resuming the first device in the domain. Of course, that code isn't available as a library, you would need to implement it (or use genpd, but chances are it is too heavy weight for the job). In theory, it shouldn't really matter where the switch is and how it is operated as long as the PM domain "driver" knows how to access it. However, for that to work something would need to bind that "driver" to the domain and something would need to know which devices needed to be added to the PM domains during enumeration and the ordering of things bringup matters a lot here, which is a problem. So in short, you are right that for the GPU/HDA thing device links would likely to be a simpler to use and still get the job done. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 10, 2016 at 12:55 AM, Luis R. Rodriguez <mcgrof@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. 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. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <mcgrof@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 >
On Thu, Nov 10, 2016 at 1:12 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote: > 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 <mcgrof@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. Well, actually, you seem to be missing a bit of context. :-) Something similar to device links as submitted had already been considered in the 2010-or-so time frame (IIRC), but then we thought that maybe the extra complexity was not needed after all. Fast forward a few years and a few more-or-less failing attempts to address those problems in different ways and here we go again. Plus, there are more apparent problems of the nature in question now, but let me address that in a different branch of this thread. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 10, 2016 at 12:56:14AM +0100, Rafael J. Wysocki wrote: > The idea, roughly, is that if there is a single on/off switch acting > on multiple devices, you can (a) set up a PM domain tracking all of > those device's runtime PM invocations and (b) maintaining a reference > counter of devices still not suspended. This way it would only turn > the switch off when all of the devices in question had been suspended. > Analogously, it would turn the switch on before resuming the first > device in the domain. Of course, that code isn't available as a > library, you would need to implement it (or use genpd, but chances are > it is too heavy weight for the job). My understanding is that the hierarchy of struct generic_pm_domain is created by the platform on boot. For an embedded platform, this is encoded in the device tree, but what about ACPI which doesn't know anything about struct generic_pm_domain? I would have to lump devices into generic_pm_domains after the fact, after the platform has scanned the buses, but this seems to be forbidden according to this slide deck, which calls that a "layering violation": https://events.linuxfoundation.org/images/stories/pdf/lcjp2012_wysocki.pdf (Quote: "Adding and Removing Devices [...] Supposed to be called by the platform (calling one of them from a device driver is a layering violation).") So it seems that using struct generic_pm_domain is never an option on ACPI, is that correct? Thanks, Lukas -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 08, 2016 at 08:27:12AM +0100, Marek Szyprowski wrote: > On 2016-11-07 22:47, Luis R. Rodriguez wrote: > > 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 ? > > The main feature of device links that is used in this patch is enabling > runtime pm dependency between Exynos SYSMMU controller (called it client > device) and the device, for which it implements DMA address translation > (called master device). The assumptions are following: > 1. master device driver is completely unaware of the Exynos SYSMMU presence, > IOMMU is transparently hooked up and managed by DMA-mapping framework > 2. SYSMMU belongs to the same power domain as it's master device > 3. SYSMMU is optional, master device can fully operate without it, with > simple DMA address translation (DMA address == physical address) > 4. Master device implements runtime pm, what in turn causes respective > power domain to be turned on/off > 5. DMA-mapping and IOMMU frameworks provides no calls to notify SYSMMU > when its master device is performing DMA operations, so SYSMMU has > to be runtime active > 6. Currently SYSMMU always sets its runtime pm status to active after > attaching to its master device to ensure proper hardware state. This > prevents power domain to be turned off, even when master device sets > its runtime pm status to suspended. > 7. Exynos SYSMMU has to be runtime active at the same time when its > master device is runtime active to it to perform DMA operations and > allow the power domain to be turned off, when master device is > runtime suspended. > 8. The terms of device links, Exynos SYSMMU is a 'consumer' and master > device is a 'supplier'. You seem to have mixed up the consumer and supplier in point 8 above. Your code is such that the SYSMMU is the supplier and the master device is the consumer: device_link_add(dev, data->sysmmu, DL_FLAG_PM_RUNTIME); Prototype of device_link_add: struct device_link *device_link_add(struct device *consumer, struct device *supplier, u32 flags); Your code is correct, only point 8 above is wrong. Best regards, Lukas -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Lukas, On 2016-11-19 12:11, 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: >>> 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 ? >> The main feature of device links that is used in this patch is enabling >> runtime pm dependency between Exynos SYSMMU controller (called it client >> device) and the device, for which it implements DMA address translation >> (called master device). The assumptions are following: >> 1. master device driver is completely unaware of the Exynos SYSMMU presence, >> IOMMU is transparently hooked up and managed by DMA-mapping framework >> 2. SYSMMU belongs to the same power domain as it's master device >> 3. SYSMMU is optional, master device can fully operate without it, with >> simple DMA address translation (DMA address == physical address) >> 4. Master device implements runtime pm, what in turn causes respective >> power domain to be turned on/off >> 5. DMA-mapping and IOMMU frameworks provides no calls to notify SYSMMU >> when its master device is performing DMA operations, so SYSMMU has >> to be runtime active >> 6. Currently SYSMMU always sets its runtime pm status to active after >> attaching to its master device to ensure proper hardware state. This >> prevents power domain to be turned off, even when master device sets >> its runtime pm status to suspended. >> 7. Exynos SYSMMU has to be runtime active at the same time when its >> master device is runtime active to it to perform DMA operations and >> allow the power domain to be turned off, when master device is >> runtime suspended. >> 8. The terms of device links, Exynos SYSMMU is a 'consumer' and master >> device is a 'supplier'. > You seem to have mixed up the consumer and supplier in point 8 above. > Your code is such that the SYSMMU is the supplier and the master device > is the consumer: > > device_link_add(dev, data->sysmmu, DL_FLAG_PM_RUNTIME); > > Prototype of device_link_add: > > struct device_link *device_link_add(struct device *consumer, > struct device *supplier, > u32 flags); > > Your code is correct, only point 8 above is wrong. Thanks for checking this. You are right that I mixed up consumer and supplier in point 8. I'm sorry for the confusion. Best regards
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, &owner->controllers, owner_node) { - pm_runtime_put_sync(data->sysmmu); - } - mutex_lock(&owner->rpm_lock); list_for_each_entry(data, &owner->controllers, owner_node) { @@ -848,10 +844,6 @@ static int exynos_iommu_attach_device(struct iommu_domain *iommu_domain, mutex_unlock(&owner->rpm_lock); - list_for_each_entry(data, &owner->controllers, owner_node) { - pm_runtime_get_sync(data->sysmmu); - } - dev_dbg(dev, "%s: Attached IOMMU with pgtable %pa\n", __func__, &pagetable); @@ -1232,6 +1224,14 @@ static int exynos_iommu_of_xlate(struct device *dev, list_add_tail(&data->owner_node, &owner->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; }
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. Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> --- drivers/iommu/exynos-iommu.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)