Issue observed with pm_runtime_put_sync

Issue observed with pm_runtime_put_sync

[G, Manjunath Kondaiah]

 
Hi Kevin,
While using pm runtime API's with DMA, it was observed that, during omap_free_dma, 
pm_runtime_put_sync is called which results in warning dump. 
It looks like, when cpu idle patch is under processing, DMA interrupt gets fired 
which inturn call omap_free_dma which results multiple spin_locks.
 
The DMA driver uses *_get_sync during omap_request_dma and *_put_sync during 
omap_free_dma.

Environment:
Kernel:
git://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap-2.6.git
Branch: master
Commit: 99cf630 Linux-omap rebuilt: Fixed omap4 kernel panic without nosmp, updated to -rc7
+ 
DMA hwmod patches
defconfig: omap2plus_defconfig
board: OMAP3630 Zoom3
 
Is this a known issue or issue with pm runtime API usage in DMA driver?
 
log:
http://pastebin.com/93KPk2ng
 
Regards,
Manjunath 

Re: Issue observed with pm_runtime_put_sync

[Kevin Hilman]

Manjunath,

"G, Manjunath Kondaiah" <manjugk@ti.com> writes:

> While using pm runtime API's with DMA, it was observed that, during
> omap_free_dma, pm_runtime_put_sync is called which results in warning
> dump.  It looks like, when cpu idle patch is under processing, DMA
> interrupt gets fired which inturn call omap_free_dma which results
> multiple spin_locks.
>  
> The DMA driver uses *_get_sync during omap_request_dma and *_put_sync during
> omap_free_dma.
>  
> Environment:
> Kernel:
> git://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap-2.6.git
> Branch: master
> Commit: 99cf630 Linux-omap rebuilt: Fixed omap4 kernel panic without nosmp,
> updated to -rc7
> +
> DMA hwmod patches
>  
> defconfig: omap2plus_defconfig
> board: OMAP3630 Zoom3
>  
> Is this a known issue or issue with pm runtime API usage in DMA driver?

It's an issue in the runtime PM usage in DMA driver.

Specifically, the _sync versions of the API cannot be used from
interrupt context because they can sleep.

Are the _sync versions really needed at that point?  Without having the
code, I cannot tell, but I susupect that the async versions could be
used there instead.

If not, then the code will need to be reworked so the ISR is not doing
the actual work, but instead is scheduling work to be done later in
process context.

Kevin

RE: Issue observed with pm_runtime_put_sync

[G, Manjunath Kondaiah]

 
Kevin,

> -----Original Message-----
> From: Kevin Hilman [mailto:khilman@deeprootsystems.com] 
> Sent: Monday, October 18, 2010 9:01 PM
> To: G, Manjunath Kondaiah
> Cc: linux-omap@vger.kernel.org
> Subject: Re: Issue observed with pm_runtime_put_sync
> 
> Manjunath,
> 
> "G, Manjunath Kondaiah" <manjugk@ti.com> writes:
> >  

[...]

> > Is this a known issue or issue with pm runtime API usage in 
> DMA driver?
> 
> It's an issue in the runtime PM usage in DMA driver.
> 
> Specifically, the _sync versions of the API cannot be used 
> from interrupt context because they can sleep.
> 
> Are the _sync versions really needed at that point?  Without 
> having the code, I cannot tell, but I susupect that the async 
> versions could be used there instead.
> 
> If not, then the code will need to be reworked so the ISR is 
> not doing the actual work, but instead is scheduling work to 
> be done later in process context.

It looks to me this issue is related to DMA client driver since 
DMA driver only invokes call back function registered during dma channel 
setup. The control will be passed to DMA client driver. Now it is 
responsibility of DMA client driver to invoke free_dma(free_dma will
invoke put_sync) from non interrupt context.

Most of the times, callback will indicate end of data transfer, the
client driver will release all DMA resources from interrupt context
itself.

-Manjunath

RE: Issue observed with pm_runtime_put_sync

[Shilimkar, Santosh]

Manju,
> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] On Behalf Of G, Manjunath Kondaiah
> Sent: Monday, October 18, 2010 9:31 PM
> To: Kevin Hilman
> Cc: linux-omap@vger.kernel.org
> Subject: RE: Issue observed with pm_runtime_put_sync
> 
> 
> Kevin,
> 
> > -----Original Message-----
> > From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
> > Sent: Monday, October 18, 2010 9:01 PM
> > To: G, Manjunath Kondaiah
> > Cc: linux-omap@vger.kernel.org
> > Subject: Re: Issue observed with pm_runtime_put_sync
> >
> > Manjunath,
> >
> > "G, Manjunath Kondaiah" <manjugk@ti.com> writes:
> > >
> 
> [...]
> 
> > > Is this a known issue or issue with pm runtime API usage in
> > DMA driver?
> >
> > It's an issue in the runtime PM usage in DMA driver.
> >
> > Specifically, the _sync versions of the API cannot be used
> > from interrupt context because they can sleep.
> >
> > Are the _sync versions really needed at that point?  Without
> > having the code, I cannot tell, but I susupect that the async
> > versions could be used there instead.
> >
> > If not, then the code will need to be reworked so the ISR is
> > not doing the actual work, but instead is scheduling work to
> > be done later in process context.
> 
> It looks to me this issue is related to DMA client driver since
> DMA driver only invokes call back function registered during dma channel
> setup. The control will be passed to DMA client driver. Now it is
> responsibility of DMA client driver to invoke free_dma(free_dma will
> invoke put_sync) from non interrupt context.
> 
> Most of the times, callback will indicate end of data transfer, the
> client driver will release all DMA resources from interrupt context
> itself.
> 
Why are you doing put_sync/get_sync per channel alloc/free. DMA has 
a single clock and not each for per channel. The driver should release
DMA clocks when all channels are free and acquire it on the first channel
request.

We did discuss this sometime back on the LO, right ?

This may not solve your problem but just wanted to understand this.

Regards,
Santosh

RE: Issue observed with pm_runtime_put_sync

[G, Manjunath Kondaiah]

> -----Original Message-----
> From: Shilimkar, Santosh 
> Sent: Monday, October 18, 2010 9:42 PM
> To: G, Manjunath Kondaiah; Kevin Hilman
> Cc: linux-omap@vger.kernel.org
> Subject: RE: Issue observed with pm_runtime_put_sync
> 
> Manju,
> > -----Original Message-----
> > From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> > owner@vger.kernel.org] On Behalf Of G, Manjunath Kondaiah
> > Sent: Monday, October 18, 2010 9:31 PM
> > To: Kevin Hilman
> > Cc: linux-omap@vger.kernel.org
> > Subject: RE: Issue observed with pm_runtime_put_sync
> > 
> > 
> > Kevin,
> > 
> > > -----Original Message-----
> > > From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
> > > Sent: Monday, October 18, 2010 9:01 PM
> > > To: G, Manjunath Kondaiah
> > > Cc: linux-omap@vger.kernel.org
> > > Subject: Re: Issue observed with pm_runtime_put_sync
> > >
> > > Manjunath,
> > >
> > > "G, Manjunath Kondaiah" <manjugk@ti.com> writes:
> > > >
> > 
> > [...]
> > 
> > > > Is this a known issue or issue with pm runtime API usage in
> > > DMA driver?
> > >
> > > It's an issue in the runtime PM usage in DMA driver.
> > >
> > > Specifically, the _sync versions of the API cannot be used
> > > from interrupt context because they can sleep.
> > >
> > > Are the _sync versions really needed at that point?  Without
> > > having the code, I cannot tell, but I susupect that the async
> > > versions could be used there instead.
> > >
> > > If not, then the code will need to be reworked so the ISR is
> > > not doing the actual work, but instead is scheduling work to
> > > be done later in process context.
> > 
> > It looks to me this issue is related to DMA client driver since
> > DMA driver only invokes call back function registered 
> during dma channel
> > setup. The control will be passed to DMA client driver. Now it is
> > responsibility of DMA client driver to invoke free_dma(free_dma will
> > invoke put_sync) from non interrupt context.
> > 
> > Most of the times, callback will indicate end of data transfer, the
> > client driver will release all DMA resources from interrupt context
> > itself.
> > 
> Why are you doing put_sync/get_sync per channel alloc/free. DMA has 
> a single clock and not each for per channel. The driver should release
> DMA clocks when all channels are free and acquire it on the 
> first channel
> request.
> 
> We did discuss this sometime back on the LO, right ?

In this case, we might have to check all the 32 channels status(free or
used) for every free_dma call.
 
I was thinking to use dev->power.usage_count field for each get_sysnc and put_sync.

We can have additional logic in free_dma if get_sync/put_sync is overhead.
 
> 
> This may not solve your problem but just wanted to understand this.

Yes. We need to align on this part. Waitning for kevin's comment on this issue.

-Manjunath 

Re: Issue observed with pm_runtime_put_sync

[Kevin Hilman]

"G, Manjunath Kondaiah" <manjugk@ti.com> writes:

>  
> Kevin,
>
>> -----Original Message-----
>> From: Kevin Hilman [mailto:khilman@deeprootsystems.com] 
>> Sent: Monday, October 18, 2010 9:01 PM
>> To: G, Manjunath Kondaiah
>> Cc: linux-omap@vger.kernel.org
>> Subject: Re: Issue observed with pm_runtime_put_sync
>> 
>> Manjunath,
>> 
>> "G, Manjunath Kondaiah" <manjugk@ti.com> writes:
>> >  
>
> [...]
>
>> > Is this a known issue or issue with pm runtime API usage in 
>> DMA driver?
>> 
>> It's an issue in the runtime PM usage in DMA driver.
>> 
>> Specifically, the _sync versions of the API cannot be used 
>> from interrupt context because they can sleep.
>> 
>> Are the _sync versions really needed at that point?  Without 
>> having the code, I cannot tell, but I susupect that the async 
>> versions could be used there instead.
>> 
>> If not, then the code will need to be reworked so the ISR is 
>> not doing the actual work, but instead is scheduling work to 
>> be done later in process context.
>
> It looks to me this issue is related to DMA client driver since 
> DMA driver only invokes call back function registered during dma channel 
> setup. The control will be passed to DMA client driver. Now it is 
> responsibility of DMA client driver to invoke free_dma(free_dma will
> invoke put_sync) from non interrupt context.

IMO, that is an unnecessary restriction.

There is no reason that I can think of that omap_free_dma() needs to do
a _put_sync().  It should just do a normal (asynchronous) _put(), which
is safe from either process or interrupt context.

> Most of the times, callback will indicate end of data transfer, the
> client driver will release all DMA resources from interrupt context
> itself.

That's fine, and is something the DMA API has supported in the past, and
there's no reason it couldn't continue to support that.

Kevin

Re: Issue observed with pm_runtime_put_sync

[Kevin Hilman]

"G, Manjunath Kondaiah" <manjugk@ti.com> writes:

>> -----Original Message-----
>> From: Shilimkar, Santosh 
>> Sent: Monday, October 18, 2010 9:42 PM
>> To: G, Manjunath Kondaiah; Kevin Hilman
>> Cc: linux-omap@vger.kernel.org
>> Subject: RE: Issue observed with pm_runtime_put_sync
>> 
>> Manju,
>> > -----Original Message-----
>> > From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
>> > owner@vger.kernel.org] On Behalf Of G, Manjunath Kondaiah
>> > Sent: Monday, October 18, 2010 9:31 PM
>> > To: Kevin Hilman
>> > Cc: linux-omap@vger.kernel.org
>> > Subject: RE: Issue observed with pm_runtime_put_sync
>> > 
>> > 
>> > Kevin,
>> > 
>> > > -----Original Message-----
>> > > From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
>> > > Sent: Monday, October 18, 2010 9:01 PM
>> > > To: G, Manjunath Kondaiah
>> > > Cc: linux-omap@vger.kernel.org
>> > > Subject: Re: Issue observed with pm_runtime_put_sync
>> > >
>> > > Manjunath,
>> > >
>> > > "G, Manjunath Kondaiah" <manjugk@ti.com> writes:
>> > > >
>> > 
>> > [...]
>> > 
>> > > > Is this a known issue or issue with pm runtime API usage in
>> > > DMA driver?
>> > >
>> > > It's an issue in the runtime PM usage in DMA driver.
>> > >
>> > > Specifically, the _sync versions of the API cannot be used
>> > > from interrupt context because they can sleep.
>> > >
>> > > Are the _sync versions really needed at that point?  Without
>> > > having the code, I cannot tell, but I susupect that the async
>> > > versions could be used there instead.
>> > >
>> > > If not, then the code will need to be reworked so the ISR is
>> > > not doing the actual work, but instead is scheduling work to
>> > > be done later in process context.
>> > 
>> > It looks to me this issue is related to DMA client driver since
>> > DMA driver only invokes call back function registered 
>> during dma channel
>> > setup. The control will be passed to DMA client driver. Now it is
>> > responsibility of DMA client driver to invoke free_dma(free_dma will
>> > invoke put_sync) from non interrupt context.
>> > 
>> > Most of the times, callback will indicate end of data transfer, the
>> > client driver will release all DMA resources from interrupt context
>> > itself.
>> > 
>> Why are you doing put_sync/get_sync per channel alloc/free. DMA has 
>> a single clock and not each for per channel. The driver should release
>> DMA clocks when all channels are free and acquire it on the 
>> first channel
>> request.
>> 
>> We did discuss this sometime back on the LO, right ?
>
> In this case, we might have to check all the 32 channels status(free or
> used) for every free_dma call.
>  
> I was thinking to use dev->power.usage_count field for each get_sysnc and put_sync.
>
> We can have additional logic in free_dma if get_sync/put_sync is overhead.

Either you do the use counting in the DMA layer, or you just let the
runtime PM layer do the use counting.  Either way, only when usecount
transitions to/from zero will the actual omap_device API be invoked,
so I prefer to just let runtime PM do the usecounting.

In fact, the runtime PM API also provides useful statistics as well as
sysfs controls for either preventing a device from going idle, or
bringing it out of idle.

If you continue to use the runtime PM API, you will have these
statistics and controls per-channel, which is probably rather useful.
In fact, recent versions of powertop will even report stats from runtime
PM, so beinga able to see per-channel DMA stats could be quite useful.

Kevin

RE: Issue observed with pm_runtime_put_sync

[G, Manjunath Kondaiah]

 

> -----Original Message-----
> From: Kevin Hilman [mailto:khilman@deeprootsystems.com] 
> Sent: Tuesday, October 19, 2010 3:12 AM
> To: G, Manjunath Kondaiah
> Cc: Shilimkar, Santosh; linux-omap@vger.kernel.org
> Subject: Re: Issue observed with pm_runtime_put_sync
> 
> "G, Manjunath Kondaiah" <manjugk@ti.com> writes:
> 
> >> -----Original Message-----
> >> From: Shilimkar, Santosh 
> >> Sent: Monday, October 18, 2010 9:42 PM
> >> To: G, Manjunath Kondaiah; Kevin Hilman
> >> Cc: linux-omap@vger.kernel.org
> >> Subject: RE: Issue observed with pm_runtime_put_sync
> >> 
> >> Manju,
> >> > -----Original Message-----
> >> > From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> >> > owner@vger.kernel.org] On Behalf Of G, Manjunath Kondaiah
> >> > Sent: Monday, October 18, 2010 9:31 PM
> >> > To: Kevin Hilman
> >> > Cc: linux-omap@vger.kernel.org
> >> > Subject: RE: Issue observed with pm_runtime_put_sync
> >> > 
> >> > 
> >> > Kevin,
> >> > 
> >> > > -----Original Message-----
> >> > > From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
> >> > > Sent: Monday, October 18, 2010 9:01 PM
> >> > > To: G, Manjunath Kondaiah
> >> > > Cc: linux-omap@vger.kernel.org
> >> > > Subject: Re: Issue observed with pm_runtime_put_sync
> >> > >
> >> > > Manjunath,
> >> > >
> >> > > "G, Manjunath Kondaiah" <manjugk@ti.com> writes:
> >> > > >
> >> > 
> >> > [...]
> >> > 
> >> > > > Is this a known issue or issue with pm runtime API usage in
> >> > > DMA driver?
> >> > >
> >> > > It's an issue in the runtime PM usage in DMA driver.
> >> > >
> >> > > Specifically, the _sync versions of the API cannot be used
> >> > > from interrupt context because they can sleep.
> >> > >
> >> > > Are the _sync versions really needed at that point?  Without
> >> > > having the code, I cannot tell, but I susupect that the async
> >> > > versions could be used there instead.
> >> > >
> >> > > If not, then the code will need to be reworked so the ISR is
> >> > > not doing the actual work, but instead is scheduling work to
> >> > > be done later in process context.
> >> > 
> >> > It looks to me this issue is related to DMA client driver since
> >> > DMA driver only invokes call back function registered 
> >> during dma channel
> >> > setup. The control will be passed to DMA client driver. Now it is
> >> > responsibility of DMA client driver to invoke 
> free_dma(free_dma will
> >> > invoke put_sync) from non interrupt context.
> >> > 
> >> > Most of the times, callback will indicate end of data 
> transfer, the
> >> > client driver will release all DMA resources from 
> interrupt context
> >> > itself.
> >> > 
> >> Why are you doing put_sync/get_sync per channel 
> alloc/free. DMA has 
> >> a single clock and not each for per channel. The driver 
> should release
> >> DMA clocks when all channels are free and acquire it on the 
> >> first channel
> >> request.
> >> 
> >> We did discuss this sometime back on the LO, right ?
> >
> > In this case, we might have to check all the 32 channels 
> status(free or
> > used) for every free_dma call.
> >  
> > I was thinking to use dev->power.usage_count field for each 
> get_sysnc and put_sync.
> >
> > We can have additional logic in free_dma if 
> get_sync/put_sync is overhead.
> 
> Either you do the use counting in the DMA layer, or you just let the
> runtime PM layer do the use counting.  Either way, only when usecount
> transitions to/from zero will the actual omap_device API be invoked,
> so I prefer to just let runtime PM do the usecounting.
> 
> In fact, the runtime PM API also provides useful statistics as well as
> sysfs controls for either preventing a device from going idle, or
> bringing it out of idle.
> 
> If you continue to use the runtime PM API, you will have these
> statistics and controls per-channel, which is probably rather useful.
> In fact, recent versions of powertop will even report stats 
> from runtime
> PM, so beinga able to see per-channel DMA stats could be quite useful.

Thanks for this info. I will use pm layer use count along with 
pm_runtime_get and pm_runtime_put.

-Manjunath