| Message ID | 1413795888-18559-2-git-send-email-k.kozlowski@samsung.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On pon, 2014-10-20 at 11:04 +0200, Krzysztof Kozlowski wrote: > Add a simple getter pm_runtime_is_irq_safe() for querying whether runtime > PM IRQ safe was set or not. > > Various bus drivers implementing runtime PM may use choose to suspend > differently based on IRQ safeness status of child driver (e.g. do not > unprepare the clock if IRQ safe is not set). > > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> > Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> Rafael, Len, Pavel, Is proposed API ok? Do you have any comments? I'll upload whole patchset to Russell's patch tracking system. However an ack from PM maintainer is probably needed. Best regards, Krzysztof > --- > Documentation/power/runtime_pm.txt | 4 ++++ > include/linux/pm_runtime.h | 6 ++++++ > 2 files changed, 10 insertions(+) > > diff --git a/Documentation/power/runtime_pm.txt b/Documentation/power/runtime_pm.txt > index f32ce5419573..397b81593142 100644 > --- a/Documentation/power/runtime_pm.txt > +++ b/Documentation/power/runtime_pm.txt > @@ -468,6 +468,10 @@ drivers/base/power/runtime.c and include/linux/pm_runtime.h: > - set the power.irq_safe flag for the device, causing the runtime-PM > callbacks to be invoked with interrupts off > > + bool pm_runtime_is_irq_safe(struct device *dev); > + - return true if power.irq_safe flag was set for the device, causing > + the runtime-PM callbacks to be invoked with interrupts off > + > void pm_runtime_mark_last_busy(struct device *dev); > - set the power.last_busy field to the current time > > diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h > index 367f49b9a1c9..44d74f0f182e 100644 > --- a/include/linux/pm_runtime.h > +++ b/include/linux/pm_runtime.h > @@ -128,6 +128,11 @@ static inline void pm_runtime_mark_last_busy(struct device *dev) > ACCESS_ONCE(dev->power.last_busy) = jiffies; > } > > +static inline bool pm_runtime_is_irq_safe(struct device *dev) > +{ > + return dev->power.irq_safe; > +} > + > #else /* !CONFIG_PM_RUNTIME */ > > static inline int __pm_runtime_idle(struct device *dev, int rpmflags) > @@ -167,6 +172,7 @@ static inline bool pm_runtime_enabled(struct device *dev) { return false; } > > static inline void pm_runtime_no_callbacks(struct device *dev) {} > static inline void pm_runtime_irq_safe(struct device *dev) {} > +static inline bool pm_runtime_is_irq_safe(struct device *dev) { return false; } > > static inline bool pm_runtime_callbacks_present(struct device *dev) { return false; } > static inline void pm_runtime_mark_last_busy(struct device *dev) {} -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 31 October 2014 10:14, Krzysztof Kozlowski <k.kozlowski@samsung.com> wrote: > On pon, 2014-10-20 at 11:04 +0200, Krzysztof Kozlowski wrote: >> Add a simple getter pm_runtime_is_irq_safe() for querying whether runtime >> PM IRQ safe was set or not. >> >> Various bus drivers implementing runtime PM may use choose to suspend >> differently based on IRQ safeness status of child driver (e.g. do not >> unprepare the clock if IRQ safe is not set). >> >> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> >> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> > > Rafael, Len, Pavel, > > Is proposed API ok? Do you have any comments? > > I'll upload whole patchset to Russell's patch tracking system. However > an ack from PM maintainer is probably needed. I would actually prefer the opposite. It's better if we can take these patches through Rafaels tree. 1) I have posted patches for the amba bus, these may have merge conflicts with your changes. 2) We may start using the new runtime PM API within genpd as of now. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 31, 2014 at 10:29:36AM +0100, Ulf Hansson wrote: > On 31 October 2014 10:14, Krzysztof Kozlowski <k.kozlowski@samsung.com> wrote: > > On pon, 2014-10-20 at 11:04 +0200, Krzysztof Kozlowski wrote: > >> Add a simple getter pm_runtime_is_irq_safe() for querying whether runtime > >> PM IRQ safe was set or not. > >> > >> Various bus drivers implementing runtime PM may use choose to suspend > >> differently based on IRQ safeness status of child driver (e.g. do not > >> unprepare the clock if IRQ safe is not set). > >> > >> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> > >> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> > > > > Rafael, Len, Pavel, > > > > Is proposed API ok? Do you have any comments? > > > > I'll upload whole patchset to Russell's patch tracking system. However > > an ack from PM maintainer is probably needed. > > I would actually prefer the opposite. It's better if we can take these > patches through Rafaels tree. > > 1) I have posted patches for the amba bus, these may have merge > conflicts with your changes. > 2) We may start using the new runtime PM API within genpd as of now. Stop this broken thinking. The solution is simple. Put the generic runtime PM changes into the PM tree in a separate /clean/ branch. Merge them into the PM-next branch. Notify me where I can pull that /clean/ branch. I pull that clean branch into my tree, and then apply the AMBA specific patches on top. That means Rafael has the PM changes, I have the dependent PM changes, and I also have the AMBA changes, and no one ends up carrying changes which are inappropriate.
On pi?, 2014-10-31 at 10:29 +0100, Ulf Hansson wrote: > On 31 October 2014 10:14, Krzysztof Kozlowski <k.kozlowski@samsung.com> wrote: > > On pon, 2014-10-20 at 11:04 +0200, Krzysztof Kozlowski wrote: > >> Add a simple getter pm_runtime_is_irq_safe() for querying whether runtime > >> PM IRQ safe was set or not. > >> > >> Various bus drivers implementing runtime PM may use choose to suspend > >> differently based on IRQ safeness status of child driver (e.g. do not > >> unprepare the clock if IRQ safe is not set). > >> > >> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> > >> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> > > > > Rafael, Len, Pavel, > > > > Is proposed API ok? Do you have any comments? > > > > I'll upload whole patchset to Russell's patch tracking system. However > > an ack from PM maintainer is probably needed. > > I would actually prefer the opposite. It's better if we can take these > patches through Rafaels tree. > > 1) I have posted patches for the amba bus, these may have merge > conflicts with your changes. > 2) We may start using the new runtime PM API within genpd as of now. I don't mind and it makes sense to me. This way I need an ack from Russell or Vinod... Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 31 October 2014 10:33, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Fri, Oct 31, 2014 at 10:29:36AM +0100, Ulf Hansson wrote: >> On 31 October 2014 10:14, Krzysztof Kozlowski <k.kozlowski@samsung.com> wrote: >> > On pon, 2014-10-20 at 11:04 +0200, Krzysztof Kozlowski wrote: >> >> Add a simple getter pm_runtime_is_irq_safe() for querying whether runtime >> >> PM IRQ safe was set or not. >> >> >> >> Various bus drivers implementing runtime PM may use choose to suspend >> >> differently based on IRQ safeness status of child driver (e.g. do not >> >> unprepare the clock if IRQ safe is not set). >> >> >> >> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> >> >> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> >> > >> > Rafael, Len, Pavel, >> > >> > Is proposed API ok? Do you have any comments? >> > >> > I'll upload whole patchset to Russell's patch tracking system. However >> > an ack from PM maintainer is probably needed. >> >> I would actually prefer the opposite. It's better if we can take these >> patches through Rafaels tree. >> >> 1) I have posted patches for the amba bus, these may have merge >> conflicts with your changes. >> 2) We may start using the new runtime PM API within genpd as of now. > > Stop this broken thinking. The solution is simple. > > Put the generic runtime PM changes into the PM tree in a separate /clean/ > branch. Merge them into the PM-next branch. Notify me where I can > pull that /clean/ branch. > > I pull that clean branch into my tree, and then apply the AMBA specific > patches on top. > > That means Rafael has the PM changes, I have the dependent PM changes, > and I also have the AMBA changes, and no one ends up carrying changes > which are inappropriate. Okay, I have no concerns with this approach. I have also checked the potential upcoming merge conflict in the amba bus, since I doubt you will pull in bigger changes from his tree!? Anyway, that conflict should be trivial to resolve if that happens. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri 2014-10-31 10:14:55, Krzysztof Kozlowski wrote: > On pon, 2014-10-20 at 11:04 +0200, Krzysztof Kozlowski wrote: > > Add a simple getter pm_runtime_is_irq_safe() for querying whether runtime > > PM IRQ safe was set or not. > > > > Various bus drivers implementing runtime PM may use choose to suspend > > differently based on IRQ safeness status of child driver (e.g. do not > > unprepare the clock if IRQ safe is not set). > > > > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> > > Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> > > Rafael, Len, Pavel, > > Is proposed API ok? Do you have any comments? > > I'll upload whole patchset to Russell's patch tracking system. However > an ack from PM maintainer is probably needed. I don't like the API. Having callbacks work in different context (irq / noirq) based on what another function reports is ugly. What is the penalty if we always decide callbacks are not IRQ safe? Pavel > > --- a/Documentation/power/runtime_pm.txt > > +++ b/Documentation/power/runtime_pm.txt > > @@ -468,6 +468,10 @@ drivers/base/power/runtime.c and include/linux/pm_runtime.h: > > - set the power.irq_safe flag for the device, causing the runtime-PM > > callbacks to be invoked with interrupts off > > > > + bool pm_runtime_is_irq_safe(struct device *dev); > > + - return true if power.irq_safe flag was set for the device, causing > > + the runtime-PM callbacks to be invoked with interrupts off > > + > > void pm_runtime_mark_last_busy(struct device *dev); > > - set the power.last_busy field to the current time > >
On pi?, 2014-10-31 at 15:22 +0100, Pavel Machek wrote: > On Fri 2014-10-31 10:14:55, Krzysztof Kozlowski wrote: > > On pon, 2014-10-20 at 11:04 +0200, Krzysztof Kozlowski wrote: > > > Add a simple getter pm_runtime_is_irq_safe() for querying whether runtime > > > PM IRQ safe was set or not. > > > > > > Various bus drivers implementing runtime PM may use choose to suspend > > > differently based on IRQ safeness status of child driver (e.g. do not > > > unprepare the clock if IRQ safe is not set). > > > > > > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> > > > Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> > > > > Rafael, Len, Pavel, > > > > Is proposed API ok? Do you have any comments? > > > > I'll upload whole patchset to Russell's patch tracking system. However > > an ack from PM maintainer is probably needed. > > I don't like the API. Having callbacks work in different context (irq > / noirq) based on what another function reports is ugly. > > What is the penalty if we always decide callbacks are not IRQ safe? Then pm_runtime_get_sync() could not be called in atomic context. The pl330 runtime PM would have to be completely reworked because one pm_runtime_get_sync() is called in device_issue_pending which cannot sleep (at least in non preemptible kernels). Probably this can be solved some way... Best regards, Krzysztof > > > > --- a/Documentation/power/runtime_pm.txt > > > +++ b/Documentation/power/runtime_pm.txt > > > @@ -468,6 +468,10 @@ drivers/base/power/runtime.c and include/linux/pm_runtime.h: > > > - set the power.irq_safe flag for the device, causing the runtime-PM > > > callbacks to be invoked with interrupts off > > > > > > + bool pm_runtime_is_irq_safe(struct device *dev); > > > + - return true if power.irq_safe flag was set for the device, causing > > > + the runtime-PM callbacks to be invoked with interrupts off > > > + > > > void pm_runtime_mark_last_busy(struct device *dev); > > > - set the power.last_busy field to the current time > > > -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Krzysztof, On Friday 31 October 2014 15:40:16 Krzysztof Kozlowski wrote: > On pi?, 2014-10-31 at 15:22 +0100, Pavel Machek wrote: > > On Fri 2014-10-31 10:14:55, Krzysztof Kozlowski wrote: > >> On pon, 2014-10-20 at 11:04 +0200, Krzysztof Kozlowski wrote: > >>> Add a simple getter pm_runtime_is_irq_safe() for querying whether > >>> runtime PM IRQ safe was set or not. > >>> > >>> Various bus drivers implementing runtime PM may use choose to suspend > >>> differently based on IRQ safeness status of child driver (e.g. do not > >>> unprepare the clock if IRQ safe is not set). > >>> > >>> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> > >>> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> > >> > >> Rafael, Len, Pavel, > >> > >> Is proposed API ok? Do you have any comments? > >> > >> I'll upload whole patchset to Russell's patch tracking system. However > >> an ack from PM maintainer is probably needed. > > > > I don't like the API. Having callbacks work in different context (irq > > / noirq) based on what another function reports is ugly. > > > > What is the penalty if we always decide callbacks are not IRQ safe? > > Then pm_runtime_get_sync() could not be called in atomic context. The > pl330 runtime PM would have to be completely reworked because one > pm_runtime_get_sync() is called in device_issue_pending which cannot > sleep (at least in non preemptible kernels). Probably this can be solved > some way... Many other drivers suffer from the same problem. While I won't reject your proposed fix, I would prefer a more generic approach. One option that has been discussed previously was to use a work queue to delay starting the DMA transfer to an interruptible context where pm_runtime_get_sync() could be called. However, as Russell pointed out [1], even that won't work in all cases as the DMA slave might need the transfer to be started before enabling part of its hardware (OMAP audio seem to be such a case). I've heard a rumor of a possible DMA engine rework to forbid calling the descriptor preparation API from atomic context. This could be used as a base to implement runtime PM, as DMA slave drivers should not prepare descriptors if they don't need to use them. However that's a long term plan, and we need a solution sooner than that. I've been toying with the idea of adding explicit open/close (or whatever we would call them) operations to the DMA engine API. Those would be used by DMA slave drivers to signal that they will start/stop using the DMA engine. If (1) we must start the DMA synchronously with a DMA slave call, (2) need to sleep to handle PM, and (3) don't want to keep the DMA engine powered for as long as one channel is requested, then we need to turn at least preparation as not callable in atomic context, or introduce a new operation. [1] http://www.spinics.net/lists/dmaengine/msg01548.html > >>> --- a/Documentation/power/runtime_pm.txt > >>> +++ b/Documentation/power/runtime_pm.txt > >>> > >>> @@ -468,6 +468,10 @@ drivers/base/power/runtime.c and > >>> include/linux/pm_runtime.h: > >>> - set the power.irq_safe flag for the device, causing the > >>> runtime-PM > >>> > >>> callbacks to be invoked with interrupts off > >>> > >>> + bool pm_runtime_is_irq_safe(struct device *dev); > >>> + - return true if power.irq_safe flag was set for the device, > >>> causing > >>> + the runtime-PM callbacks to be invoked with interrupts off > >>> + > >>> > >>> void pm_runtime_mark_last_busy(struct device *dev); > >>> > >>> - set the power.last_busy field to the current time
On sob, 2014-11-01 at 02:29 +0200, Laurent Pinchart wrote: > Hi Krzysztof, > > On Friday 31 October 2014 15:40:16 Krzysztof Kozlowski wrote: > > On pi?, 2014-10-31 at 15:22 +0100, Pavel Machek wrote: > > > On Fri 2014-10-31 10:14:55, Krzysztof Kozlowski wrote: > > >> On pon, 2014-10-20 at 11:04 +0200, Krzysztof Kozlowski wrote: > > >>> Add a simple getter pm_runtime_is_irq_safe() for querying whether > > >>> runtime PM IRQ safe was set or not. > > >>> > > >>> Various bus drivers implementing runtime PM may use choose to suspend > > >>> differently based on IRQ safeness status of child driver (e.g. do not > > >>> unprepare the clock if IRQ safe is not set). > > >>> > > >>> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> > > >>> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> > > >> > > >> Rafael, Len, Pavel, > > >> > > >> Is proposed API ok? Do you have any comments? > > >> > > >> I'll upload whole patchset to Russell's patch tracking system. However > > >> an ack from PM maintainer is probably needed. > > > > > > I don't like the API. Having callbacks work in different context (irq > > > / noirq) based on what another function reports is ugly. > > > > > > What is the penalty if we always decide callbacks are not IRQ safe? > > > > Then pm_runtime_get_sync() could not be called in atomic context. The > > pl330 runtime PM would have to be completely reworked because one > > pm_runtime_get_sync() is called in device_issue_pending which cannot > > sleep (at least in non preemptible kernels). Probably this can be solved > > some way... > > Many other drivers suffer from the same problem. While I won't reject your > proposed fix, I would prefer a more generic approach. > > One option that has been discussed previously was to use a work queue to delay > starting the DMA transfer to an interruptible context where > pm_runtime_get_sync() could be called. However, as Russell pointed out [1], > even that won't work in all cases as the DMA slave might need the transfer to > be started before enabling part of its hardware (OMAP audio seem to be such a > case). > > I've heard a rumor of a possible DMA engine rework to forbid calling the > descriptor preparation API from atomic context. This could be used as a base > to implement runtime PM, as DMA slave drivers should not prepare descriptors > if they don't need to use them. However that's a long term plan, and we need a > solution sooner than that. > > I've been toying with the idea of adding explicit open/close (or whatever we > would call them) operations to the DMA engine API. Those would be used by DMA > slave drivers to signal that they will start/stop using the DMA engine. > > If (1) we must start the DMA synchronously with a DMA slave call, (2) need to > sleep to handle PM, and (3) don't want to keep the DMA engine powered for as > long as one channel is requested, then we need to turn at least preparation as > not callable in atomic context, or introduce a new operation. > > [1] http://www.spinics.net/lists/dmaengine/msg01548.html That makes sense. However I am not familiar with DMA core code as much as I think it would be needed to make such generic changes :). I'll stick to one driver for now. Thanks for comments! Krzysztof -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Nov 01, 2014 at 02:29:42AM +0200, Laurent Pinchart wrote: > Hi Krzysztof, > > On Friday 31 October 2014 15:40:16 Krzysztof Kozlowski wrote: > > On pi?, 2014-10-31 at 15:22 +0100, Pavel Machek wrote: > > > On Fri 2014-10-31 10:14:55, Krzysztof Kozlowski wrote: > > >> On pon, 2014-10-20 at 11:04 +0200, Krzysztof Kozlowski wrote: > > >>> Add a simple getter pm_runtime_is_irq_safe() for querying whether > > >>> runtime PM IRQ safe was set or not. > > >>> > > >>> Various bus drivers implementing runtime PM may use choose to suspend > > >>> differently based on IRQ safeness status of child driver (e.g. do not > > >>> unprepare the clock if IRQ safe is not set). > > >>> > > >>> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> > > >>> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> > > >> > > >> Rafael, Len, Pavel, > > >> > > >> Is proposed API ok? Do you have any comments? > > >> > > >> I'll upload whole patchset to Russell's patch tracking system. However > > >> an ack from PM maintainer is probably needed. > > > > > > I don't like the API. Having callbacks work in different context (irq > > > / noirq) based on what another function reports is ugly. > > > > > > What is the penalty if we always decide callbacks are not IRQ safe? > > > > Then pm_runtime_get_sync() could not be called in atomic context. The > > pl330 runtime PM would have to be completely reworked because one > > pm_runtime_get_sync() is called in device_issue_pending which cannot > > sleep (at least in non preemptible kernels). Probably this can be solved > > some way... > > Many other drivers suffer from the same problem. While I won't reject your > proposed fix, I would prefer a more generic approach. > > One option that has been discussed previously was to use a work queue to delay > starting the DMA transfer to an interruptible context where > pm_runtime_get_sync() could be called. However, as Russell pointed out [1], > even that won't work in all cases as the DMA slave might need the transfer to > be started before enabling part of its hardware (OMAP audio seem to be such a > case). > > I've heard a rumor of a possible DMA engine rework to forbid calling the > descriptor preparation API from atomic context. This could be used as a base > to implement runtime PM, as DMA slave drivers should not prepare descriptors > if they don't need to use them. However that's a long term plan, and we need a > solution sooner than that. Well it is not a rumour :) I have been contemplating that now that async_tx will be killed so we dont have to worry about that usage. For the slave dma usage, we can change the prepare API to be non atomic. I think the users will be okay with approach. This way drivers can use runtime pm calls in prepare.
Hi Vinod, On Monday 03 November 2014 21:57:28 Vinod Koul wrote: > On Sat, Nov 01, 2014 at 02:29:42AM +0200, Laurent Pinchart wrote: > > On Friday 31 October 2014 15:40:16 Krzysztof Kozlowski wrote: > >> On pi?, 2014-10-31 at 15:22 +0100, Pavel Machek wrote: > >>> On Fri 2014-10-31 10:14:55, Krzysztof Kozlowski wrote: > >>>> On pon, 2014-10-20 at 11:04 +0200, Krzysztof Kozlowski wrote: > >>>>> Add a simple getter pm_runtime_is_irq_safe() for querying whether > >>>>> runtime PM IRQ safe was set or not. > >>>>> > >>>>> Various bus drivers implementing runtime PM may use choose to > >>>>> suspend differently based on IRQ safeness status of child driver > >>>>> (e.g. do not unprepare the clock if IRQ safe is not set). > >>>>> > >>>>> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> > >>>>> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> > >>>> > >>>> Rafael, Len, Pavel, > >>>> > >>>> Is proposed API ok? Do you have any comments? > >>>> > >>>> I'll upload whole patchset to Russell's patch tracking system. > >>>> However an ack from PM maintainer is probably needed. > >>> > >>> I don't like the API. Having callbacks work in different context (irq > >>> / noirq) based on what another function reports is ugly. > >>> > >>> What is the penalty if we always decide callbacks are not IRQ safe? > >> > >> Then pm_runtime_get_sync() could not be called in atomic context. The > >> pl330 runtime PM would have to be completely reworked because one > >> pm_runtime_get_sync() is called in device_issue_pending which cannot > >> sleep (at least in non preemptible kernels). Probably this can be solved > >> some way... > > > > Many other drivers suffer from the same problem. While I won't reject your > > proposed fix, I would prefer a more generic approach. > > > > One option that has been discussed previously was to use a work queue to > > delay starting the DMA transfer to an interruptible context where > > pm_runtime_get_sync() could be called. However, as Russell pointed out > > [1], > > even that won't work in all cases as the DMA slave might need the transfer > > to be started before enabling part of its hardware (OMAP audio seem to be > > such a case). > > > > I've heard a rumor of a possible DMA engine rework to forbid calling the > > descriptor preparation API from atomic context. This could be used as a > > base to implement runtime PM, as DMA slave drivers should not prepare > > descriptors if they don't need to use them. However that's a long term > > plan, and we need a solution sooner than that. > > Well it is not a rumour :) I didn't want to put too much pressure on you by quoting you on that :-) > I have been contemplating that now that async_tx will be killed so we dont > have to worry about that usage. For the slave dma usage, we can change the > prepare API to be non atomic. I think the users will be okay with approach. > This way drivers can use runtime pm calls in prepare. I'd certainly welcome that. There's a couple of users we will need to fix as they call the prepare API from an atomic context. I'm thinking about ASoC drivers in particular. Do you have any time line in mind ? > > I've been toying with the idea of adding explicit open/close (or whatever > > we would call them) operations to the DMA engine API. Those would be used > > by DMA slave drivers to signal that they will start/stop using the DMA > > engine. > > > > If (1) we must start the DMA synchronously with a DMA slave call, (2) need > > to sleep to handle PM, and (3) don't want to keep the DMA engine powered > > for as long as one channel is requested, then we need to turn at least > > preparation as not callable in atomic context, or introduce a new > > operation. > > > > [1] http://www.spinics.net/lists/dmaengine/msg01548.html > > > > > >>> --- a/Documentation/power/runtime_pm.txt > > > >>> +++ b/Documentation/power/runtime_pm.txt > > > >>> > > > >>> @@ -468,6 +468,10 @@ drivers/base/power/runtime.c and > > > >>> > > > >>> include/linux/pm_runtime.h: > > > >>> - set the power.irq_safe flag for the device, causing the > > > >>> runtime-PM callbacks to be invoked with interrupts off > > > >>> > > > >>> + bool pm_runtime_is_irq_safe(struct device *dev); > > > >>> + - return true if power.irq_safe flag was set for the device, > > > >>> causing > > > >>> + the runtime-PM callbacks to be invoked with interrupts off > > > >>> + > > > >>> > > > >>> void pm_runtime_mark_last_busy(struct device *dev); > > > >>> - set the power.last_busy field to the current time
On Mon, Nov 03, 2014 at 09:57:28PM +0530, Vinod Koul wrote: > On Sat, Nov 01, 2014 at 02:29:42AM +0200, Laurent Pinchart wrote: > > Many other drivers suffer from the same problem. While I won't reject your > > proposed fix, I would prefer a more generic approach. > > > > One option that has been discussed previously was to use a work queue to delay > > starting the DMA transfer to an interruptible context where > > pm_runtime_get_sync() could be called. However, as Russell pointed out [1], > > even that won't work in all cases as the DMA slave might need the transfer to > > be started before enabling part of its hardware (OMAP audio seem to be such a > > case). > > > > I've heard a rumor of a possible DMA engine rework to forbid calling the > > descriptor preparation API from atomic context. This could be used as a base > > to implement runtime PM, as DMA slave drivers should not prepare descriptors > > if they don't need to use them. However that's a long term plan, and we need a > > solution sooner than that. > > Well it is not a rumour :) > > I have been contemplating that now that async_tx will be killed so we dont > have to worry about that usage. For the slave dma usage, we can change the > prepare API to be non atomic. I think the users will be okay with approach. > This way drivers can use runtime pm calls in prepare. Except we /do/ have a fair number of places where the prep calls are made from atomic contexts, particularly in serial drivers. You'd need to introduce a tasklet into almost every serial driver which doesn't already have one to restart RX DMA after an error or pause. Eg, drivers/tty/serial/amba-pl011.c drivers/tty/serial/pch_uart.c drivers/tty/serial/imx.c Probably also: drivers/net/ethernet/micrel/ks8842.c There could well be other places as well, I've not gone through and checked exhaustively.
On Mon, Nov 03, 2014 at 05:04:08PM +0000, Russell King - ARM Linux wrote: > On Mon, Nov 03, 2014 at 09:57:28PM +0530, Vinod Koul wrote: > > On Sat, Nov 01, 2014 at 02:29:42AM +0200, Laurent Pinchart wrote: > > > Many other drivers suffer from the same problem. While I won't reject your > > > proposed fix, I would prefer a more generic approach. > > > > > > One option that has been discussed previously was to use a work queue to delay > > > starting the DMA transfer to an interruptible context where > > > pm_runtime_get_sync() could be called. However, as Russell pointed out [1], > > > even that won't work in all cases as the DMA slave might need the transfer to > > > be started before enabling part of its hardware (OMAP audio seem to be such a > > > case). > > > > > > I've heard a rumor of a possible DMA engine rework to forbid calling the > > > descriptor preparation API from atomic context. This could be used as a base > > > to implement runtime PM, as DMA slave drivers should not prepare descriptors > > > if they don't need to use them. However that's a long term plan, and we need a > > > solution sooner than that. > > > > Well it is not a rumour :) > > > > I have been contemplating that now that async_tx will be killed so we dont > > have to worry about that usage. For the slave dma usage, we can change the > > prepare API to be non atomic. I think the users will be okay with approach. > > This way drivers can use runtime pm calls in prepare. > > Except we /do/ have a fair number of places where the prep calls are made > from atomic contexts, particularly in serial drivers. You'd need to > introduce a tasklet into almost every serial driver which doesn't > already have one to restart RX DMA after an error or pause. Eg, > > drivers/tty/serial/amba-pl011.c > drivers/tty/serial/pch_uart.c > drivers/tty/serial/imx.c > > Probably also: > > drivers/net/ethernet/micrel/ks8842.c > > There could well be other places as well, I've not gone through and > checked exhaustively. Yes that would be required to be done first. Or is there any better alternative proposal, am all ears
On Mon, Nov 03, 2014 at 06:59:37PM +0200, Laurent Pinchart wrote: > > > Many other drivers suffer from the same problem. While I won't reject your > > > proposed fix, I would prefer a more generic approach. > > > > > > One option that has been discussed previously was to use a work queue to > > > delay starting the DMA transfer to an interruptible context where > > > pm_runtime_get_sync() could be called. However, as Russell pointed out > > > [1], > > > even that won't work in all cases as the DMA slave might need the transfer > > > to be started before enabling part of its hardware (OMAP audio seem to be > > > such a case). > > > > > > I've heard a rumor of a possible DMA engine rework to forbid calling the > > > descriptor preparation API from atomic context. This could be used as a > > > base to implement runtime PM, as DMA slave drivers should not prepare > > > descriptors if they don't need to use them. However that's a long term > > > plan, and we need a solution sooner than that. > > > > Well it is not a rumour :) > > I didn't want to put too much pressure on you by quoting you on that :-) > > > I have been contemplating that now that async_tx will be killed so we dont > > have to worry about that usage. For the slave dma usage, we can change the > > prepare API to be non atomic. I think the users will be okay with approach. > > This way drivers can use runtime pm calls in prepare. > > I'd certainly welcome that. There's a couple of users we will need to fix as > they call the prepare API from an atomic context. I'm thinking about ASoC > drivers in particular. ASoC is impler to fix as we can do this is ASoC prepare which is not atomic. So we would need to split prepare and submit on that > Do you have any time line in mind ? Not yet, perhpaps the one after next merge window will be good target.
Hi Russell, On Monday 03 November 2014 17:04:08 Russell King - ARM Linux wrote: > On Mon, Nov 03, 2014 at 09:57:28PM +0530, Vinod Koul wrote: > > On Sat, Nov 01, 2014 at 02:29:42AM +0200, Laurent Pinchart wrote: > >> Many other drivers suffer from the same problem. While I won't reject > >> your proposed fix, I would prefer a more generic approach. > >> > >> One option that has been discussed previously was to use a work queue to > >> delay starting the DMA transfer to an interruptible context where > >> pm_runtime_get_sync() could be called. However, as Russell pointed out > >> [1], > >> even that won't work in all cases as the DMA slave might need the > >> transfer to be started before enabling part of its hardware (OMAP audio > >> seem to be such a case). > >> > >> I've heard a rumor of a possible DMA engine rework to forbid calling the > >> descriptor preparation API from atomic context. This could be used as a > >> base to implement runtime PM, as DMA slave drivers should not prepare > >> descriptors if they don't need to use them. However that's a long term > >> plan, and we need a solution sooner than that. > > > > Well it is not a rumour :) > > > > I have been contemplating that now that async_tx will be killed so we dont > > have to worry about that usage. For the slave dma usage, we can change the > > prepare API to be non atomic. I think the users will be okay with > > approach. This way drivers can use runtime pm calls in prepare. > > Except we /do/ have a fair number of places where the prep calls are made > from atomic contexts, particularly in serial drivers. You'd need to > introduce a tasklet into almost every serial driver which doesn't > already have one to restart RX DMA after an error or pause. Eg, > > drivers/tty/serial/amba-pl011.c > drivers/tty/serial/pch_uart.c > drivers/tty/serial/imx.c I wonder whether it would be possible to decouple descriptor allocation and descriptor initialization/preparation. If drivers could allocate descriptors and reuse them after they complete, not only would it lower the memory management pressure by getting rid of alloc/free during transmission, but it would also make it possible to easily allocate the transaction descriptors beforehand in non-atomic context. > Probably also: > > drivers/net/ethernet/micrel/ks8842.c > > There could well be other places as well, I've not gone through and > checked exhaustively.
diff --git a/Documentation/power/runtime_pm.txt b/Documentation/power/runtime_pm.txt index f32ce5419573..397b81593142 100644 --- a/Documentation/power/runtime_pm.txt +++ b/Documentation/power/runtime_pm.txt @@ -468,6 +468,10 @@ drivers/base/power/runtime.c and include/linux/pm_runtime.h: - set the power.irq_safe flag for the device, causing the runtime-PM callbacks to be invoked with interrupts off + bool pm_runtime_is_irq_safe(struct device *dev); + - return true if power.irq_safe flag was set for the device, causing + the runtime-PM callbacks to be invoked with interrupts off + void pm_runtime_mark_last_busy(struct device *dev); - set the power.last_busy field to the current time diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h index 367f49b9a1c9..44d74f0f182e 100644 --- a/include/linux/pm_runtime.h +++ b/include/linux/pm_runtime.h @@ -128,6 +128,11 @@ static inline void pm_runtime_mark_last_busy(struct device *dev) ACCESS_ONCE(dev->power.last_busy) = jiffies; } +static inline bool pm_runtime_is_irq_safe(struct device *dev) +{ + return dev->power.irq_safe; +} + #else /* !CONFIG_PM_RUNTIME */ static inline int __pm_runtime_idle(struct device *dev, int rpmflags) @@ -167,6 +172,7 @@ static inline bool pm_runtime_enabled(struct device *dev) { return false; } static inline void pm_runtime_no_callbacks(struct device *dev) {} static inline void pm_runtime_irq_safe(struct device *dev) {} +static inline bool pm_runtime_is_irq_safe(struct device *dev) { return false; } static inline bool pm_runtime_callbacks_present(struct device *dev) { return false; } static inline void pm_runtime_mark_last_busy(struct device *dev) {}