[PATCH] dma: pl330: Add support for DMA_PAUSE command

[PATCH] dma: pl330: Add support for DMA_PAUSE command

[Tushar Behera]

While playing back audio, pmc_dmaengine requests the DMA channel to
stop DMA transmission through DMA_PAUSE command.

Currently PL330 driver doesn't support DMA pause command, leaving
the DMA state inconsistent when the system resumes. Instead, it would
be better to terminate the DMA transfer during suspend and restart
again during resume.

Tested with audio playback across a suspend-resume cycle.

Signed-off-by: Tushar Behera <tushar.behera@linaro.org>
---
 drivers/dma/pl330.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 73fa9b7..cd70f42 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -2362,6 +2362,7 @@ static int pl330_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd, unsigned
 	LIST_HEAD(list);
 
 	switch (cmd) {
+	case DMA_PAUSE:
 	case DMA_TERMINATE_ALL:
 		spin_lock_irqsave(&pch->lock, flags);
 
-- 
1.7.9.5

Re: [PATCH] dma: pl330: Add support for DMA_PAUSE command

[Jassi Brar]

On Wed, May 14, 2014 at 8:53 AM, Tushar Behera <tushar.behera@linaro.org> wrote:
> While playing back audio, pmc_dmaengine requests the DMA channel to
> stop DMA transmission through DMA_PAUSE command.
>
> Currently PL330 driver doesn't support DMA pause command, leaving
> the DMA state inconsistent when the system resumes. Instead, it would
> be better to terminate the DMA transfer during suspend and restart
> again during resume.
>
> Tested with audio playback across a suspend-resume cycle.
>
What is pmc_dmaengine? How does DMA_PAUSE help, when there is no DMA_RESUME?

DMA_PAUSE means freezing the transfer, not dropping it. DMA_RESUME
then moves the transfer again from that point. Clubbing PAUSE with
TERMINATE is plain wrong.  Sorry.

Regards
Jassi

Re: [PATCH] dma: pl330: Add support for DMA_PAUSE command

[Tushar Behera]

On 14 May 2014 17:29, Jassi Brar <jassisinghbrar@gmail.com> wrote:
> On Wed, May 14, 2014 at 8:53 AM, Tushar Behera <tushar.behera@linaro.org> wrote:
>> While playing back audio, pmc_dmaengine requests the DMA channel to
>> stop DMA transmission through DMA_PAUSE command.
>>
>> Currently PL330 driver doesn't support DMA pause command, leaving
>> the DMA state inconsistent when the system resumes. Instead, it would
>> be better to terminate the DMA transfer during suspend and restart
>> again during resume.
>>
>> Tested with audio playback across a suspend-resume cycle.
>>
> What is pmc_dmaengine? How does DMA_PAUSE help, when there is no DMA_RESUME?
>

Sorry, it is a typo.

sound/core/pcm_dmaengine.c:snd_dmaengine_pcm_trigger() -->
dmaengine_pause() is called during system suspend.

> DMA_PAUSE means freezing the transfer, not dropping it. DMA_RESUME
> then moves the transfer again from that point. Clubbing PAUSE with
> TERMINATE is plain wrong.  Sorry.
>

Would you please suggest how should we go ahead with implementing
DMA_PAUSE/DMA_RESUME support for PL330 driver?

> Regards
> Jassi

Thanks,
-- 
Tushar Behera

Re: [PATCH] dma: pl330: Add support for DMA_PAUSE command

[Lars-Peter Clausen]

On 05/14/2014 02:07 PM, Tushar Behera wrote:
> On 14 May 2014 17:29, Jassi Brar <jassisinghbrar@gmail.com> wrote:
>> On Wed, May 14, 2014 at 8:53 AM, Tushar Behera <tushar.behera@linaro.org> wrote:
>>> While playing back audio, pmc_dmaengine requests the DMA channel to
>>> stop DMA transmission through DMA_PAUSE command.
>>>
>>> Currently PL330 driver doesn't support DMA pause command, leaving
>>> the DMA state inconsistent when the system resumes. Instead, it would
>>> be better to terminate the DMA transfer during suspend and restart
>>> again during resume.
>>>
>>> Tested with audio playback across a suspend-resume cycle.
>>>
>> What is pmc_dmaengine? How does DMA_PAUSE help, when there is no DMA_RESUME?
>>
>
> Sorry, it is a typo.
>
> sound/core/pcm_dmaengine.c:snd_dmaengine_pcm_trigger() -->
> dmaengine_pause() is called during system suspend.

It is only called if the DMA driver has support for pausing and resuming DMA 
transfers. Or at least that is the intention.

- Lars

Re: [PATCH] dma: pl330: Add support for DMA_PAUSE command

[Jassi Brar]

On Wed, May 14, 2014 at 5:37 PM, Tushar Behera <tushar.behera@linaro.org> wrote:
> On 14 May 2014 17:29, Jassi Brar <jassisinghbrar@gmail.com> wrote:
>> On Wed, May 14, 2014 at 8:53 AM, Tushar Behera <tushar.behera@linaro.org> wrote:
>>> While playing back audio, pmc_dmaengine requests the DMA channel to
>>> stop DMA transmission through DMA_PAUSE command.
>>>
>>> Currently PL330 driver doesn't support DMA pause command, leaving
>>> the DMA state inconsistent when the system resumes. Instead, it would
>>> be better to terminate the DMA transfer during suspend and restart
>>> again during resume.
>>>
>>> Tested with audio playback across a suspend-resume cycle.
>>>
>> What is pmc_dmaengine? How does DMA_PAUSE help, when there is no DMA_RESUME?
>>
>
> Sorry, it is a typo.
>
> sound/core/pcm_dmaengine.c:snd_dmaengine_pcm_trigger() -->
> dmaengine_pause() is called during system suspend.
>
pl330.c does specify  cmd_pause = false, which should prevent
sound/soc/soc-generic-dmaengine-pcm.c:dmaengine_pcm_set_runtime_hwparams()
from declaring PAUSE/RESUME support. Which kernel are you using?

-jassi

Re: [PATCH] dma: pl330: Add support for DMA_PAUSE command

[Tushar Behera]

On 14 May 2014 17:54, Lars-Peter Clausen <lars@metafoo.de> wrote:
> On 05/14/2014 02:07 PM, Tushar Behera wrote:
>>
>> On 14 May 2014 17:29, Jassi Brar <jassisinghbrar@gmail.com> wrote:
>>>
>>> On Wed, May 14, 2014 at 8:53 AM, Tushar Behera <tushar.behera@linaro.org>
>>> wrote:
>>>>
>>>> While playing back audio, pmc_dmaengine requests the DMA channel to
>>>> stop DMA transmission through DMA_PAUSE command.
>>>>
>>>> Currently PL330 driver doesn't support DMA pause command, leaving
>>>> the DMA state inconsistent when the system resumes. Instead, it would
>>>> be better to terminate the DMA transfer during suspend and restart
>>>> again during resume.
>>>>
>>>> Tested with audio playback across a suspend-resume cycle.
>>>>
>>> What is pmc_dmaengine? How does DMA_PAUSE help, when there is no
>>> DMA_RESUME?
>>>
>>
>> Sorry, it is a typo.
>>
>> sound/core/pcm_dmaengine.c:snd_dmaengine_pcm_trigger() -->
>> dmaengine_pause() is called during system suspend.
>
>
> It is only called if the DMA driver has support for pausing and resuming DMA
> transfers. Or at least that is the intention.
>
> - Lars

During suspend, snd_dmaengine_pcm_trigger():SNDRV_PCM_TRIGGER_SUSPEND
is called which unconditionally calls dmaengine_pause(). Should we
update snd_dmaengine_pcm_trigger() to check for DMA pause/resume
support and call dmaengine_pause() or dmaengine_terminate_all()
accordingly?


-- 
Tushar Behera

Re: [PATCH] dma: pl330: Add support for DMA_PAUSE command

[Lars-Peter Clausen]

On 05/15/2014 02:01 PM, Tushar Behera wrote:
> On 14 May 2014 17:54, Lars-Peter Clausen <lars@metafoo.de> wrote:
>> On 05/14/2014 02:07 PM, Tushar Behera wrote:
>>>
>>> On 14 May 2014 17:29, Jassi Brar <jassisinghbrar@gmail.com> wrote:
>>>>
>>>> On Wed, May 14, 2014 at 8:53 AM, Tushar Behera <tushar.behera@linaro.org>
>>>> wrote:
>>>>>
>>>>> While playing back audio, pmc_dmaengine requests the DMA channel to
>>>>> stop DMA transmission through DMA_PAUSE command.
>>>>>
>>>>> Currently PL330 driver doesn't support DMA pause command, leaving
>>>>> the DMA state inconsistent when the system resumes. Instead, it would
>>>>> be better to terminate the DMA transfer during suspend and restart
>>>>> again during resume.
>>>>>
>>>>> Tested with audio playback across a suspend-resume cycle.
>>>>>
>>>> What is pmc_dmaengine? How does DMA_PAUSE help, when there is no
>>>> DMA_RESUME?
>>>>
>>>
>>> Sorry, it is a typo.
>>>
>>> sound/core/pcm_dmaengine.c:snd_dmaengine_pcm_trigger() -->
>>> dmaengine_pause() is called during system suspend.
>>
>>
>> It is only called if the DMA driver has support for pausing and resuming DMA
>> transfers. Or at least that is the intention.
>>
>> - Lars
>
> During suspend, snd_dmaengine_pcm_trigger():SNDRV_PCM_TRIGGER_SUSPEND
> is called which unconditionally calls dmaengine_pause(). Should we
> update snd_dmaengine_pcm_trigger() to check for DMA pause/resume
> support and call dmaengine_pause() or dmaengine_terminate_all()
> accordingly?

As far as I understand it we do not have to do anything for TRIGGER_SUSPEND 
if we do not set the SNDRV_PCM_INFO_RESUME flag. It looks like 
TRIGGER_SUSPEND is called unconditionally during suspend. But since the 
error code is ignored it should be fine if we just call dmaengine_pause() 
and that return -ENOSYS or similar.

Are you seeing an actual issue that you are trying to fix with your patch?

- Lars

Re: [PATCH] dma: pl330: Add support for DMA_PAUSE command

[Lars-Peter Clausen]

On 05/15/2014 02:01 PM, Tushar Behera wrote:
> On 14 May 2014 17:54, Lars-Peter Clausen <lars@metafoo.de> wrote:
>> On 05/14/2014 02:07 PM, Tushar Behera wrote:
>>>
>>> On 14 May 2014 17:29, Jassi Brar <jassisinghbrar@gmail.com> wrote:
>>>>
>>>> On Wed, May 14, 2014 at 8:53 AM, Tushar Behera <tushar.behera@linaro.org>
>>>> wrote:
>>>>>
>>>>> While playing back audio, pmc_dmaengine requests the DMA channel to
>>>>> stop DMA transmission through DMA_PAUSE command.
>>>>>
>>>>> Currently PL330 driver doesn't support DMA pause command, leaving
>>>>> the DMA state inconsistent when the system resumes. Instead, it would
>>>>> be better to terminate the DMA transfer during suspend and restart
>>>>> again during resume.
>>>>>
>>>>> Tested with audio playback across a suspend-resume cycle.
>>>>>
>>>> What is pmc_dmaengine? How does DMA_PAUSE help, when there is no
>>>> DMA_RESUME?
>>>>
>>>
>>> Sorry, it is a typo.
>>>
>>> sound/core/pcm_dmaengine.c:snd_dmaengine_pcm_trigger() -->
>>> dmaengine_pause() is called during system suspend.
>>
>>
>> It is only called if the DMA driver has support for pausing and resuming DMA
>> transfers. Or at least that is the intention.
>>
>> - Lars
>
> During suspend, snd_dmaengine_pcm_trigger():SNDRV_PCM_TRIGGER_SUSPEND
> is called which unconditionally calls dmaengine_pause(). Should we
> update snd_dmaengine_pcm_trigger() to check for DMA pause/resume
> support and call dmaengine_pause() or dmaengine_terminate_all()
> accordingly?

As far as I understand it we do not have to do anything for TRIGGER_SUSPEND 
if we do not set the SNDRV_PCM_INFO_RESUME flag. It looks like 
TRIGGER_SUSPEND is called unconditionally during suspend. But since the 
error code is ignored it should be fine if we just call dmaengine_pause() 
and that return -ENOSYS or similar.

Are you seeing an actual issue that you are trying to fix with your patch?

- Lars

Re: [PATCH] dma: pl330: Add support for DMA_PAUSE command

[Takashi Iwai]

At Thu, 15 May 2014 21:21:12 +0200,
Lars-Peter Clausen wrote:
> 
> On 05/15/2014 02:01 PM, Tushar Behera wrote:
> > On 14 May 2014 17:54, Lars-Peter Clausen <lars@metafoo.de> wrote:
> >> On 05/14/2014 02:07 PM, Tushar Behera wrote:
> >>>
> >>> On 14 May 2014 17:29, Jassi Brar <jassisinghbrar@gmail.com> wrote:
> >>>>
> >>>> On Wed, May 14, 2014 at 8:53 AM, Tushar Behera <tushar.behera@linaro.org>
> >>>> wrote:
> >>>>>
> >>>>> While playing back audio, pmc_dmaengine requests the DMA channel to
> >>>>> stop DMA transmission through DMA_PAUSE command.
> >>>>>
> >>>>> Currently PL330 driver doesn't support DMA pause command, leaving
> >>>>> the DMA state inconsistent when the system resumes. Instead, it would
> >>>>> be better to terminate the DMA transfer during suspend and restart
> >>>>> again during resume.
> >>>>>
> >>>>> Tested with audio playback across a suspend-resume cycle.
> >>>>>
> >>>> What is pmc_dmaengine? How does DMA_PAUSE help, when there is no
> >>>> DMA_RESUME?
> >>>>
> >>>
> >>> Sorry, it is a typo.
> >>>
> >>> sound/core/pcm_dmaengine.c:snd_dmaengine_pcm_trigger() -->
> >>> dmaengine_pause() is called during system suspend.
> >>
> >>
> >> It is only called if the DMA driver has support for pausing and resuming DMA
> >> transfers. Or at least that is the intention.
> >>
> >> - Lars
> >
> > During suspend, snd_dmaengine_pcm_trigger():SNDRV_PCM_TRIGGER_SUSPEND
> > is called which unconditionally calls dmaengine_pause(). Should we
> > update snd_dmaengine_pcm_trigger() to check for DMA pause/resume
> > support and call dmaengine_pause() or dmaengine_terminate_all()
> > accordingly?
> 
> As far as I understand it we do not have to do anything for TRIGGER_SUSPEND 
> if we do not set the SNDRV_PCM_INFO_RESUME flag. It looks like 
> TRIGGER_SUSPEND is called unconditionally during suspend. But since the 
> error code is ignored it should be fine if we just call dmaengine_pause() 
> and that return -ENOSYS or similar.

Well, TRIGGER_SUSPEND is issued by PCM core no matter whether
SNDRV_PCM_INFO_RESUME flag is set or not.  The resume behavior depends
on the flag (SNDRV_PCM_TRIGGER_RESUME is issued if the flag is set,
otherwise the normal setup is done by alsa-lib at resume), but the
suspend is always triggered in the same way.

So, PCM core assumes that the driver stops the stream somehow by
SNDRV_PCM_TRIGGER_SUSPEND.


Takashi

Re: [PATCH] dma: pl330: Add support for DMA_PAUSE command

[Takashi Iwai]

At Thu, 15 May 2014 21:21:12 +0200,
Lars-Peter Clausen wrote:
> 
> On 05/15/2014 02:01 PM, Tushar Behera wrote:
> > On 14 May 2014 17:54, Lars-Peter Clausen <lars@metafoo.de> wrote:
> >> On 05/14/2014 02:07 PM, Tushar Behera wrote:
> >>>
> >>> On 14 May 2014 17:29, Jassi Brar <jassisinghbrar@gmail.com> wrote:
> >>>>
> >>>> On Wed, May 14, 2014 at 8:53 AM, Tushar Behera <tushar.behera@linaro.org>
> >>>> wrote:
> >>>>>
> >>>>> While playing back audio, pmc_dmaengine requests the DMA channel to
> >>>>> stop DMA transmission through DMA_PAUSE command.
> >>>>>
> >>>>> Currently PL330 driver doesn't support DMA pause command, leaving
> >>>>> the DMA state inconsistent when the system resumes. Instead, it would
> >>>>> be better to terminate the DMA transfer during suspend and restart
> >>>>> again during resume.
> >>>>>
> >>>>> Tested with audio playback across a suspend-resume cycle.
> >>>>>
> >>>> What is pmc_dmaengine? How does DMA_PAUSE help, when there is no
> >>>> DMA_RESUME?
> >>>>
> >>>
> >>> Sorry, it is a typo.
> >>>
> >>> sound/core/pcm_dmaengine.c:snd_dmaengine_pcm_trigger() -->
> >>> dmaengine_pause() is called during system suspend.
> >>
> >>
> >> It is only called if the DMA driver has support for pausing and resuming DMA
> >> transfers. Or at least that is the intention.
> >>
> >> - Lars
> >
> > During suspend, snd_dmaengine_pcm_trigger():SNDRV_PCM_TRIGGER_SUSPEND
> > is called which unconditionally calls dmaengine_pause(). Should we
> > update snd_dmaengine_pcm_trigger() to check for DMA pause/resume
> > support and call dmaengine_pause() or dmaengine_terminate_all()
> > accordingly?
> 
> As far as I understand it we do not have to do anything for TRIGGER_SUSPEND 
> if we do not set the SNDRV_PCM_INFO_RESUME flag. It looks like 
> TRIGGER_SUSPEND is called unconditionally during suspend. But since the 
> error code is ignored it should be fine if we just call dmaengine_pause() 
> and that return -ENOSYS or similar.

Well, TRIGGER_SUSPEND is issued by PCM core no matter whether
SNDRV_PCM_INFO_RESUME flag is set or not.  The resume behavior depends
on the flag (SNDRV_PCM_TRIGGER_RESUME is issued if the flag is set,
otherwise the normal setup is done by alsa-lib at resume), but the
suspend is always triggered in the same way.

So, PCM core assumes that the driver stops the stream somehow by
SNDRV_PCM_TRIGGER_SUSPEND.


Takashi

Re: [alsa-devel] [PATCH] dma: pl330: Add support for DMA_PAUSE command

[Lars-Peter Clausen]

On 05/16/2014 07:49 AM, Takashi Iwai wrote:
> At Thu, 15 May 2014 21:21:12 +0200,
> Lars-Peter Clausen wrote:
>>
>> On 05/15/2014 02:01 PM, Tushar Behera wrote:
>>> On 14 May 2014 17:54, Lars-Peter Clausen <lars@metafoo.de> wrote:
>>>> On 05/14/2014 02:07 PM, Tushar Behera wrote:
>>>>>
>>>>> On 14 May 2014 17:29, Jassi Brar <jassisinghbrar@gmail.com> wrote:
>>>>>>
>>>>>> On Wed, May 14, 2014 at 8:53 AM, Tushar Behera <tushar.behera@linaro.org>
>>>>>> wrote:
>>>>>>>
>>>>>>> While playing back audio, pmc_dmaengine requests the DMA channel to
>>>>>>> stop DMA transmission through DMA_PAUSE command.
>>>>>>>
>>>>>>> Currently PL330 driver doesn't support DMA pause command, leaving
>>>>>>> the DMA state inconsistent when the system resumes. Instead, it would
>>>>>>> be better to terminate the DMA transfer during suspend and restart
>>>>>>> again during resume.
>>>>>>>
>>>>>>> Tested with audio playback across a suspend-resume cycle.
>>>>>>>
>>>>>> What is pmc_dmaengine? How does DMA_PAUSE help, when there is no
>>>>>> DMA_RESUME?
>>>>>>
>>>>>
>>>>> Sorry, it is a typo.
>>>>>
>>>>> sound/core/pcm_dmaengine.c:snd_dmaengine_pcm_trigger() -->
>>>>> dmaengine_pause() is called during system suspend.
>>>>
>>>>
>>>> It is only called if the DMA driver has support for pausing and resuming DMA
>>>> transfers. Or at least that is the intention.
>>>>
>>>> - Lars
>>>
>>> During suspend, snd_dmaengine_pcm_trigger():SNDRV_PCM_TRIGGER_SUSPEND
>>> is called which unconditionally calls dmaengine_pause(). Should we
>>> update snd_dmaengine_pcm_trigger() to check for DMA pause/resume
>>> support and call dmaengine_pause() or dmaengine_terminate_all()
>>> accordingly?
>>
>> As far as I understand it we do not have to do anything for TRIGGER_SUSPEND
>> if we do not set the SNDRV_PCM_INFO_RESUME flag. It looks like
>> TRIGGER_SUSPEND is called unconditionally during suspend. But since the
>> error code is ignored it should be fine if we just call dmaengine_pause()
>> and that return -ENOSYS or similar.
>
> Well, TRIGGER_SUSPEND is issued by PCM core no matter whether
> SNDRV_PCM_INFO_RESUME flag is set or not.  The resume behavior depends
> on the flag (SNDRV_PCM_TRIGGER_RESUME is issued if the flag is set,
> otherwise the normal setup is done by alsa-lib at resume), but the
> suspend is always triggered in the same way.
>
> So, PCM core assumes that the driver stops the stream somehow by
> SNDRV_PCM_TRIGGER_SUSPEND.

Ok, so we should call terminate_all() when we get a 
SNDRV_PCM_TRIGGER_SUSPEND if the dmaengine driver does not support pauseing 
the transfer. Thanks for the clarification.

On a related note it seems like it is still possible to get 
PAUSE_PUSH/PAUSE_RELEASE events even if SNDRV_PCM_INFO_PAUSE is not set. Do 
you have an idea what should be the right thing to do in such a case? Return 
-ENOSYS?

- Lars

Re: [PATCH] dma: pl330: Add support for DMA_PAUSE command

[Lars-Peter Clausen]

On 05/16/2014 07:49 AM, Takashi Iwai wrote:
> At Thu, 15 May 2014 21:21:12 +0200,
> Lars-Peter Clausen wrote:
>>
>> On 05/15/2014 02:01 PM, Tushar Behera wrote:
>>> On 14 May 2014 17:54, Lars-Peter Clausen <lars@metafoo.de> wrote:
>>>> On 05/14/2014 02:07 PM, Tushar Behera wrote:
>>>>>
>>>>> On 14 May 2014 17:29, Jassi Brar <jassisinghbrar@gmail.com> wrote:
>>>>>>
>>>>>> On Wed, May 14, 2014 at 8:53 AM, Tushar Behera <tushar.behera@linaro.org>
>>>>>> wrote:
>>>>>>>
>>>>>>> While playing back audio, pmc_dmaengine requests the DMA channel to
>>>>>>> stop DMA transmission through DMA_PAUSE command.
>>>>>>>
>>>>>>> Currently PL330 driver doesn't support DMA pause command, leaving
>>>>>>> the DMA state inconsistent when the system resumes. Instead, it would
>>>>>>> be better to terminate the DMA transfer during suspend and restart
>>>>>>> again during resume.
>>>>>>>
>>>>>>> Tested with audio playback across a suspend-resume cycle.
>>>>>>>
>>>>>> What is pmc_dmaengine? How does DMA_PAUSE help, when there is no
>>>>>> DMA_RESUME?
>>>>>>
>>>>>
>>>>> Sorry, it is a typo.
>>>>>
>>>>> sound/core/pcm_dmaengine.c:snd_dmaengine_pcm_trigger() -->
>>>>> dmaengine_pause() is called during system suspend.
>>>>
>>>>
>>>> It is only called if the DMA driver has support for pausing and resuming DMA
>>>> transfers. Or at least that is the intention.
>>>>
>>>> - Lars
>>>
>>> During suspend, snd_dmaengine_pcm_trigger():SNDRV_PCM_TRIGGER_SUSPEND
>>> is called which unconditionally calls dmaengine_pause(). Should we
>>> update snd_dmaengine_pcm_trigger() to check for DMA pause/resume
>>> support and call dmaengine_pause() or dmaengine_terminate_all()
>>> accordingly?
>>
>> As far as I understand it we do not have to do anything for TRIGGER_SUSPEND
>> if we do not set the SNDRV_PCM_INFO_RESUME flag. It looks like
>> TRIGGER_SUSPEND is called unconditionally during suspend. But since the
>> error code is ignored it should be fine if we just call dmaengine_pause()
>> and that return -ENOSYS or similar.
>
> Well, TRIGGER_SUSPEND is issued by PCM core no matter whether
> SNDRV_PCM_INFO_RESUME flag is set or not.  The resume behavior depends
> on the flag (SNDRV_PCM_TRIGGER_RESUME is issued if the flag is set,
> otherwise the normal setup is done by alsa-lib at resume), but the
> suspend is always triggered in the same way.
>
> So, PCM core assumes that the driver stops the stream somehow by
> SNDRV_PCM_TRIGGER_SUSPEND.

Ok, so we should call terminate_all() when we get a 
SNDRV_PCM_TRIGGER_SUSPEND if the dmaengine driver does not support pauseing 
the transfer. Thanks for the clarification.

On a related note it seems like it is still possible to get 
PAUSE_PUSH/PAUSE_RELEASE events even if SNDRV_PCM_INFO_PAUSE is not set. Do 
you have an idea what should be the right thing to do in such a case? Return 
-ENOSYS?

- Lars

Re: [PATCH] dma: pl330: Add support for DMA_PAUSE command

[Tushar Behera]

On 16 May 2014 00:51, Lars-Peter Clausen <lars@metafoo.de> wrote:
> On 05/15/2014 02:01 PM, Tushar Behera wrote:
>>
>> On 14 May 2014 17:54, Lars-Peter Clausen <lars@metafoo.de> wrote:
>>>
>>> On 05/14/2014 02:07 PM, Tushar Behera wrote:
>>>>
>>>>
>>>> On 14 May 2014 17:29, Jassi Brar <jassisinghbrar@gmail.com> wrote:
>>>>>
>>>>>
>>>>> On Wed, May 14, 2014 at 8:53 AM, Tushar Behera
>>>>> <tushar.behera@linaro.org>
>>>>> wrote:
>>>>>>
>>>>>>
>>>>>> While playing back audio, pmc_dmaengine requests the DMA channel to
>>>>>> stop DMA transmission through DMA_PAUSE command.
>>>>>>
>>>>>> Currently PL330 driver doesn't support DMA pause command, leaving
>>>>>> the DMA state inconsistent when the system resumes. Instead, it would
>>>>>> be better to terminate the DMA transfer during suspend and restart
>>>>>> again during resume.
>>>>>>
>>>>>> Tested with audio playback across a suspend-resume cycle.
>>>>>>
>>>>> What is pmc_dmaengine? How does DMA_PAUSE help, when there is no
>>>>> DMA_RESUME?
>>>>>
>>>>
>>>> Sorry, it is a typo.
>>>>
>>>> sound/core/pcm_dmaengine.c:snd_dmaengine_pcm_trigger() -->
>>>> dmaengine_pause() is called during system suspend.
>>>
>>>
>>>
>>> It is only called if the DMA driver has support for pausing and resuming
>>> DMA
>>> transfers. Or at least that is the intention.
>>>
>>> - Lars
>>
>>
>> During suspend, snd_dmaengine_pcm_trigger():SNDRV_PCM_TRIGGER_SUSPEND
>> is called which unconditionally calls dmaengine_pause(). Should we
>> update snd_dmaengine_pcm_trigger() to check for DMA pause/resume
>> support and call dmaengine_pause() or dmaengine_terminate_all()
>> accordingly?
>
>
> As far as I understand it we do not have to do anything for TRIGGER_SUSPEND
> if we do not set the SNDRV_PCM_INFO_RESUME flag. It looks like
> TRIGGER_SUSPEND is called unconditionally during suspend. But since the
> error code is ignored it should be fine if we just call dmaengine_pause()
> and that return -ENOSYS or similar.
>
> Are you seeing an actual issue that you are trying to fix with your patch?
>
> - Lars
>

Without this patch applied, if audio is playing back while suspend is
triggered, it doesn't playback after resume. Stopping the stream and
replaying works.

-- 
Tushar Behera

Re: [PATCH] dma: pl330: Add support for DMA_PAUSE command

[Lars-Peter Clausen]

On 05/19/2014 05:10 AM, Tushar Behera wrote:
> On 16 May 2014 00:51, Lars-Peter Clausen <lars@metafoo.de> wrote:
>> On 05/15/2014 02:01 PM, Tushar Behera wrote:
>>>
>>> On 14 May 2014 17:54, Lars-Peter Clausen <lars@metafoo.de> wrote:
>>>>
>>>> On 05/14/2014 02:07 PM, Tushar Behera wrote:
>>>>>
>>>>>
>>>>> On 14 May 2014 17:29, Jassi Brar <jassisinghbrar@gmail.com> wrote:
>>>>>>
>>>>>>
>>>>>> On Wed, May 14, 2014 at 8:53 AM, Tushar Behera
>>>>>> <tushar.behera@linaro.org>
>>>>>> wrote:
>>>>>>>
>>>>>>>
>>>>>>> While playing back audio, pmc_dmaengine requests the DMA channel to
>>>>>>> stop DMA transmission through DMA_PAUSE command.
>>>>>>>
>>>>>>> Currently PL330 driver doesn't support DMA pause command, leaving
>>>>>>> the DMA state inconsistent when the system resumes. Instead, it would
>>>>>>> be better to terminate the DMA transfer during suspend and restart
>>>>>>> again during resume.
>>>>>>>
>>>>>>> Tested with audio playback across a suspend-resume cycle.
>>>>>>>
>>>>>> What is pmc_dmaengine? How does DMA_PAUSE help, when there is no
>>>>>> DMA_RESUME?
>>>>>>
>>>>>
>>>>> Sorry, it is a typo.
>>>>>
>>>>> sound/core/pcm_dmaengine.c:snd_dmaengine_pcm_trigger() -->
>>>>> dmaengine_pause() is called during system suspend.
>>>>
>>>>
>>>>
>>>> It is only called if the DMA driver has support for pausing and resuming
>>>> DMA
>>>> transfers. Or at least that is the intention.
>>>>
>>>> - Lars
>>>
>>>
>>> During suspend, snd_dmaengine_pcm_trigger():SNDRV_PCM_TRIGGER_SUSPEND
>>> is called which unconditionally calls dmaengine_pause(). Should we
>>> update snd_dmaengine_pcm_trigger() to check for DMA pause/resume
>>> support and call dmaengine_pause() or dmaengine_terminate_all()
>>> accordingly?
>>
>>
>> As far as I understand it we do not have to do anything for TRIGGER_SUSPEND
>> if we do not set the SNDRV_PCM_INFO_RESUME flag. It looks like
>> TRIGGER_SUSPEND is called unconditionally during suspend. But since the
>> error code is ignored it should be fine if we just call dmaengine_pause()
>> and that return -ENOSYS or similar.
>>
>> Are you seeing an actual issue that you are trying to fix with your patch?
>>
>> - Lars
>>
>
> Without this patch applied, if audio is playing back while suspend is
> triggered, it doesn't playback after resume. Stopping the stream and
> replaying works.
>

Ok, I think your second suggestion was correct. Call 
dmaengine_terminate_all() instead of damengine_pause() in 
snd_dmaengine_pcm_trigger() if the dmaengine driver does not support pausing 
the transfer.

- Lars

Re: [alsa-devel] [PATCH] dma: pl330: Add support for DMA_PAUSE command

[Takashi Iwai]

At Fri, 16 May 2014 12:51:51 +0200,
Lars-Peter Clausen wrote:
> 
> On 05/16/2014 07:49 AM, Takashi Iwai wrote:
> > At Thu, 15 May 2014 21:21:12 +0200,
> > Lars-Peter Clausen wrote:
> >>
> >> On 05/15/2014 02:01 PM, Tushar Behera wrote:
> >>> On 14 May 2014 17:54, Lars-Peter Clausen <lars@metafoo.de> wrote:
> >>>> On 05/14/2014 02:07 PM, Tushar Behera wrote:
> >>>>>
> >>>>> On 14 May 2014 17:29, Jassi Brar <jassisinghbrar@gmail.com> wrote:
> >>>>>>
> >>>>>> On Wed, May 14, 2014 at 8:53 AM, Tushar Behera <tushar.behera@linaro.org>
> >>>>>> wrote:
> >>>>>>>
> >>>>>>> While playing back audio, pmc_dmaengine requests the DMA channel to
> >>>>>>> stop DMA transmission through DMA_PAUSE command.
> >>>>>>>
> >>>>>>> Currently PL330 driver doesn't support DMA pause command, leaving
> >>>>>>> the DMA state inconsistent when the system resumes. Instead, it would
> >>>>>>> be better to terminate the DMA transfer during suspend and restart
> >>>>>>> again during resume.
> >>>>>>>
> >>>>>>> Tested with audio playback across a suspend-resume cycle.
> >>>>>>>
> >>>>>> What is pmc_dmaengine? How does DMA_PAUSE help, when there is no
> >>>>>> DMA_RESUME?
> >>>>>>
> >>>>>
> >>>>> Sorry, it is a typo.
> >>>>>
> >>>>> sound/core/pcm_dmaengine.c:snd_dmaengine_pcm_trigger() -->
> >>>>> dmaengine_pause() is called during system suspend.
> >>>>
> >>>>
> >>>> It is only called if the DMA driver has support for pausing and resuming DMA
> >>>> transfers. Or at least that is the intention.
> >>>>
> >>>> - Lars
> >>>
> >>> During suspend, snd_dmaengine_pcm_trigger():SNDRV_PCM_TRIGGER_SUSPEND
> >>> is called which unconditionally calls dmaengine_pause(). Should we
> >>> update snd_dmaengine_pcm_trigger() to check for DMA pause/resume
> >>> support and call dmaengine_pause() or dmaengine_terminate_all()
> >>> accordingly?
> >>
> >> As far as I understand it we do not have to do anything for TRIGGER_SUSPEND
> >> if we do not set the SNDRV_PCM_INFO_RESUME flag. It looks like
> >> TRIGGER_SUSPEND is called unconditionally during suspend. But since the
> >> error code is ignored it should be fine if we just call dmaengine_pause()
> >> and that return -ENOSYS or similar.
> >
> > Well, TRIGGER_SUSPEND is issued by PCM core no matter whether
> > SNDRV_PCM_INFO_RESUME flag is set or not.  The resume behavior depends
> > on the flag (SNDRV_PCM_TRIGGER_RESUME is issued if the flag is set,
> > otherwise the normal setup is done by alsa-lib at resume), but the
> > suspend is always triggered in the same way.
> >
> > So, PCM core assumes that the driver stops the stream somehow by
> > SNDRV_PCM_TRIGGER_SUSPEND.
> 
> Ok, so we should call terminate_all() when we get a 
> SNDRV_PCM_TRIGGER_SUSPEND if the dmaengine driver does not support pauseing 
> the transfer. Thanks for the clarification.
> 
> On a related note it seems like it is still possible to get 
> PAUSE_PUSH/PAUSE_RELEASE events even if SNDRV_PCM_INFO_PAUSE is not set. Do 
> you have an idea what should be the right thing to do in such a case? Return 
> -ENOSYS?

That's weird.  We have already a check in snd_pcm_pre_pause() in
pcm_native.c to filter only for substreams with SNDRV_PCM_INFO_PAUSE
flag.

Could you check in which code path it is triggered?


thanks,

Takashi

Re: [alsa-devel] [PATCH] dma: pl330: Add support for DMA_PAUSE command

[Lars-Peter Clausen]

On 05/19/2014 10:37 AM, Takashi Iwai wrote:
> At Fri, 16 May 2014 12:51:51 +0200,
> Lars-Peter Clausen wrote:
>>
>> On 05/16/2014 07:49 AM, Takashi Iwai wrote:
>>> At Thu, 15 May 2014 21:21:12 +0200,
>>> Lars-Peter Clausen wrote:
>>>>
>>>> On 05/15/2014 02:01 PM, Tushar Behera wrote:
>>>>> On 14 May 2014 17:54, Lars-Peter Clausen <lars@metafoo.de> wrote:
>>>>>> On 05/14/2014 02:07 PM, Tushar Behera wrote:
>>>>>>>
>>>>>>> On 14 May 2014 17:29, Jassi Brar <jassisinghbrar@gmail.com> wrote:
>>>>>>>>
>>>>>>>> On Wed, May 14, 2014 at 8:53 AM, Tushar Behera <tushar.behera@linaro.org>
>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>> While playing back audio, pmc_dmaengine requests the DMA channel to
>>>>>>>>> stop DMA transmission through DMA_PAUSE command.
>>>>>>>>>
>>>>>>>>> Currently PL330 driver doesn't support DMA pause command, leaving
>>>>>>>>> the DMA state inconsistent when the system resumes. Instead, it would
>>>>>>>>> be better to terminate the DMA transfer during suspend and restart
>>>>>>>>> again during resume.
>>>>>>>>>
>>>>>>>>> Tested with audio playback across a suspend-resume cycle.
>>>>>>>>>
>>>>>>>> What is pmc_dmaengine? How does DMA_PAUSE help, when there is no
>>>>>>>> DMA_RESUME?
>>>>>>>>
>>>>>>>
>>>>>>> Sorry, it is a typo.
>>>>>>>
>>>>>>> sound/core/pcm_dmaengine.c:snd_dmaengine_pcm_trigger() -->
>>>>>>> dmaengine_pause() is called during system suspend.
>>>>>>
>>>>>>
>>>>>> It is only called if the DMA driver has support for pausing and resuming DMA
>>>>>> transfers. Or at least that is the intention.
>>>>>>
>>>>>> - Lars
>>>>>
>>>>> During suspend, snd_dmaengine_pcm_trigger():SNDRV_PCM_TRIGGER_SUSPEND
>>>>> is called which unconditionally calls dmaengine_pause(). Should we
>>>>> update snd_dmaengine_pcm_trigger() to check for DMA pause/resume
>>>>> support and call dmaengine_pause() or dmaengine_terminate_all()
>>>>> accordingly?
>>>>
>>>> As far as I understand it we do not have to do anything for TRIGGER_SUSPEND
>>>> if we do not set the SNDRV_PCM_INFO_RESUME flag. It looks like
>>>> TRIGGER_SUSPEND is called unconditionally during suspend. But since the
>>>> error code is ignored it should be fine if we just call dmaengine_pause()
>>>> and that return -ENOSYS or similar.
>>>
>>> Well, TRIGGER_SUSPEND is issued by PCM core no matter whether
>>> SNDRV_PCM_INFO_RESUME flag is set or not.  The resume behavior depends
>>> on the flag (SNDRV_PCM_TRIGGER_RESUME is issued if the flag is set,
>>> otherwise the normal setup is done by alsa-lib at resume), but the
>>> suspend is always triggered in the same way.
>>>
>>> So, PCM core assumes that the driver stops the stream somehow by
>>> SNDRV_PCM_TRIGGER_SUSPEND.
>>
>> Ok, so we should call terminate_all() when we get a
>> SNDRV_PCM_TRIGGER_SUSPEND if the dmaengine driver does not support pauseing
>> the transfer. Thanks for the clarification.
>>
>> On a related note it seems like it is still possible to get
>> PAUSE_PUSH/PAUSE_RELEASE events even if SNDRV_PCM_INFO_PAUSE is not set. Do
>> you have an idea what should be the right thing to do in such a case? Return
>> -ENOSYS?
>
> That's weird.  We have already a check in snd_pcm_pre_pause() in
> pcm_native.c to filter only for substreams with SNDRV_PCM_INFO_PAUSE
> flag.
>
> Could you check in which code path it is triggered?

Uhm, yes, I was only looking at the code, but couldn't find the check that 
makes sure that the PAUSE_PUSH/PAUSE_RELEASE events are only triggered if 
SNDRV_PCM_INFO_PAUSE is set. Looks like I need a pair of glasses.

Thanks and sorry for the noise,
- Lars