| Message ID | 1347543485-339-4-git-send-email-peter.ujfalusi@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 09/13/2012 03:37 PM, Peter Ujfalusi wrote: > The dmaengine_prep_dma_cyclic() function primarily used by audio for cyclic > transfer required by ALSA. > With this new parameter it is going to be possible to enable the > SNDRV_PCM_INFO_NO_PERIOD_WAKEUP mode on platforms where it is possible. > This patch only changes the public API first. Followup patch will change > the device_prep_dma_cyclic() callback parameters to pass this information > towards the dma drivers. > Hi, Hm... Do you think it would work as well if we implement this by setting the callback for the descriptor to NULL? If the callback is NULL there is nothing to at the end of a transfer/period and the dma engine driver may choose to disable interrupts. This would also benefit non cyclic transfers where the callback is NULL and we do not need add the new parameter to dmaengine_prep_dma_cyclic. - Lars > --- > include/linux/dmaengine.h | 3 ++- > sound/soc/soc-dmaengine-pcm.c | 3 ++- > 2 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h > index 9c02a45..990444b 100644 > --- a/include/linux/dmaengine.h > +++ b/include/linux/dmaengine.h > @@ -653,7 +653,8 @@ static inline struct dma_async_tx_descriptor *dmaengine_prep_rio_sg( > > static inline struct dma_async_tx_descriptor *dmaengine_prep_dma_cyclic( > struct dma_chan *chan, dma_addr_t buf_addr, size_t buf_len, > - size_t period_len, enum dma_transfer_direction dir) > + size_t period_len, enum dma_transfer_direction dir, > + bool no_wakeup) > { > return chan->device->device_prep_dma_cyclic(chan, buf_addr, buf_len, > period_len, dir, NULL); > diff --git a/sound/soc/soc-dmaengine-pcm.c b/sound/soc/soc-dmaengine-pcm.c > index 5df529e..6b70adb 100644 > --- a/sound/soc/soc-dmaengine-pcm.c > +++ b/sound/soc/soc-dmaengine-pcm.c > @@ -147,7 +147,8 @@ static int dmaengine_pcm_prepare_and_submit(struct snd_pcm_substream *substream) > desc = dmaengine_prep_dma_cyclic(chan, > substream->runtime->dma_addr, > snd_pcm_lib_buffer_bytes(substream), > - snd_pcm_lib_period_bytes(substream), direction); > + snd_pcm_lib_period_bytes(substream), direction, > + substream->runtime->no_period_wakeup); > > if (!desc) > return -ENOMEM;
On Thu, Sep 13, 2012 at 05:27:09PM +0200, Lars-Peter Clausen wrote: > Hm... Do you think it would work as well if we implement this by setting the > callback for the descriptor to NULL? If the callback is NULL there is > nothing to at the end of a transfer/period and the dma engine driver may > choose to disable interrupts. This would also benefit non cyclic transfers > where the callback is NULL and we do not need add the new parameter to > dmaengine_prep_dma_cyclic. Actually, there's a way to do that already. DMA_PREP_INTERRUPT. Unfortunately, most DMA engine slave API users don't set it when they setup their transfer: * @DMA_PREP_INTERRUPT - trigger an interrupt (callback) upon completion of * this transaction if we fixed that, then we could use the lack of it to avoid the interrupt. However, cyclic transfers don't have the flags parameter used to pass this bit. Yet another bit of yucky inconsistent design in DMA engine land...
On Thu, 2012-09-13 at 17:27 +0200, Lars-Peter Clausen wrote: > Hi, > > Hm... Do you think it would work as well if we implement this by > setting the > callback for the descriptor to NULL? If the callback is NULL there is > nothing to at the end of a transfer/period and the dma engine driver > may > choose to disable interrupts. This would also benefit non cyclic > transfers > where the callback is NULL and we do not need add the new parameter to > dmaengine_prep_dma_cyclic. That will work too.... BUT the idea of no_wake mode in ALSA is that we should not have any interrupts, so anything which is going to cause interrupts to AP in undesired. The interrupts still happen and it just that dmaengine driver is not notifying client.
On Thu, 2012-09-13 at 16:38 +0100, Russell King - ARM Linux wrote: > On Thu, Sep 13, 2012 at 05:27:09PM +0200, Lars-Peter Clausen wrote: > > Hm... Do you think it would work as well if we implement this by setting the > > callback for the descriptor to NULL? If the callback is NULL there is > > nothing to at the end of a transfer/period and the dma engine driver may > > choose to disable interrupts. This would also benefit non cyclic transfers > > where the callback is NULL and we do not need add the new parameter to > > dmaengine_prep_dma_cyclic. > > Actually, there's a way to do that already. DMA_PREP_INTERRUPT. > Unfortunately, most DMA engine slave API users don't set it when they > setup their transfer: > > * @DMA_PREP_INTERRUPT - trigger an interrupt (callback) upon completion of > * this transaction Looks like I have repeated the correct action! > > if we fixed that, then we could use the lack of it to avoid the interrupt. > > However, cyclic transfers don't have the flags parameter used to pass this > bit. Yet another bit of yucky inconsistent design in DMA engine land... > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi, On 09/13/2012 06:27 PM, Lars-Peter Clausen wrote: > On 09/13/2012 03:37 PM, Peter Ujfalusi wrote: >> The dmaengine_prep_dma_cyclic() function primarily used by audio for cyclic >> transfer required by ALSA. >> With this new parameter it is going to be possible to enable the >> SNDRV_PCM_INFO_NO_PERIOD_WAKEUP mode on platforms where it is possible. >> This patch only changes the public API first. Followup patch will change >> the device_prep_dma_cyclic() callback parameters to pass this information >> towards the dma drivers. >> > > Hi, > > Hm... Do you think it would work as well if we implement this by setting the > callback for the descriptor to NULL? If the callback is NULL there is > nothing to at the end of a transfer/period and the dma engine driver may > choose to disable interrupts. This would also benefit non cyclic transfers > where the callback is NULL and we do not need add the new parameter to > dmaengine_prep_dma_cyclic. We could do that but dma drivers enable the interrupts within dmaengine_prep_dma_cyclic() call, and we fill up the callback for dmaengine_submit() dmaengine API call. We need to tell the dma drivers in dmaengine_prep_dma_cyclic() to suppress the interrupts. Note: First I was trying this to be done in hw_params() time via the dmaengine_slave_config() call, but substream->runtime->no_period_wakeup is not configured in there. It is set for _prepare() and _trigger(). As Vinod and Russell suggested I will modify the dmaengine_prep_dma_cyclic() API to pass flags as well instead of the no_wakeup parameter. > > - Lars > >> --- >> include/linux/dmaengine.h | 3 ++- >> sound/soc/soc-dmaengine-pcm.c | 3 ++- >> 2 files changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h >> index 9c02a45..990444b 100644 >> --- a/include/linux/dmaengine.h >> +++ b/include/linux/dmaengine.h >> @@ -653,7 +653,8 @@ static inline struct dma_async_tx_descriptor *dmaengine_prep_rio_sg( >> >> static inline struct dma_async_tx_descriptor *dmaengine_prep_dma_cyclic( >> struct dma_chan *chan, dma_addr_t buf_addr, size_t buf_len, >> - size_t period_len, enum dma_transfer_direction dir) >> + size_t period_len, enum dma_transfer_direction dir, >> + bool no_wakeup) >> { >> return chan->device->device_prep_dma_cyclic(chan, buf_addr, buf_len, >> period_len, dir, NULL); >> diff --git a/sound/soc/soc-dmaengine-pcm.c b/sound/soc/soc-dmaengine-pcm.c >> index 5df529e..6b70adb 100644 >> --- a/sound/soc/soc-dmaengine-pcm.c >> +++ b/sound/soc/soc-dmaengine-pcm.c >> @@ -147,7 +147,8 @@ static int dmaengine_pcm_prepare_and_submit(struct snd_pcm_substream *substream) >> desc = dmaengine_prep_dma_cyclic(chan, >> substream->runtime->dma_addr, >> snd_pcm_lib_buffer_bytes(substream), >> - snd_pcm_lib_period_bytes(substream), direction); >> + snd_pcm_lib_period_bytes(substream), direction, >> + substream->runtime->no_period_wakeup); >> >> if (!desc) >> return -ENOMEM; >
On 09/13/2012 06:38 PM, Russell King - ARM Linux wrote: > On Thu, Sep 13, 2012 at 05:27:09PM +0200, Lars-Peter Clausen wrote: >> Hm... Do you think it would work as well if we implement this by setting the >> callback for the descriptor to NULL? If the callback is NULL there is >> nothing to at the end of a transfer/period and the dma engine driver may >> choose to disable interrupts. This would also benefit non cyclic transfers >> where the callback is NULL and we do not need add the new parameter to >> dmaengine_prep_dma_cyclic. > > Actually, there's a way to do that already. DMA_PREP_INTERRUPT. > Unfortunately, most DMA engine slave API users don't set it when they > setup their transfer: > > * @DMA_PREP_INTERRUPT - trigger an interrupt (callback) upon completion of > * this transaction As I mentioned already to Vinod: the description of this flag is close enough for our needs (other than the notion of 'this transaction' is not really correct for cyclic mode). I will resend the series to add the flags as new parameter instead of the no_wakeup. > > if we fixed that, then we could use the lack of it to avoid the interrupt. > > However, cyclic transfers don't have the flags parameter used to pass this > bit. Yet another bit of yucky inconsistent design in DMA engine land... >
On 09/14/2012 06:26 AM, Vinod Koul wrote: > On Thu, 2012-09-13 at 17:27 +0200, Lars-Peter Clausen wrote: >> Hi, >> >> Hm... Do you think it would work as well if we implement this by >> setting the >> callback for the descriptor to NULL? If the callback is NULL there is >> nothing to at the end of a transfer/period and the dma engine driver >> may >> choose to disable interrupts. This would also benefit non cyclic >> transfers >> where the callback is NULL and we do not need add the new parameter to >> dmaengine_prep_dma_cyclic. > That will work too.... BUT the idea of no_wake mode in ALSA is that we > should not have any interrupts, so anything which is going to cause > interrupts to AP in undesired. The interrupts still happen and it just > that dmaengine driver is not notifying client. Yes, this is correct. We could go and hack around later if the callback is not present and modify the irq requests for the platform but it is really not elegant way of dealing with the issue IMHO.
On 09/14/2012 05:26 AM, Vinod Koul wrote: > On Thu, 2012-09-13 at 17:27 +0200, Lars-Peter Clausen wrote: >> Hi, >> >> Hm... Do you think it would work as well if we implement this by >> setting the >> callback for the descriptor to NULL? If the callback is NULL there is >> nothing to at the end of a transfer/period and the dma engine driver >> may >> choose to disable interrupts. This would also benefit non cyclic >> transfers >> where the callback is NULL and we do not need add the new parameter to >> dmaengine_prep_dma_cyclic. > That will work too.... BUT the idea of no_wake mode in ALSA is that we > should not have any interrupts, so anything which is going to cause > interrupts to AP in undesired. The interrupts still happen and it just > that dmaengine driver is not notifying client. Well, the idea was that the driver would disable interrupts if there is no callback to call, since there would be nothing to do in the interrupt handler anyway. But I guess the flags approach should work fine as well.
On Fri, 2012-09-14 at 10:13 +0200, Lars-Peter Clausen wrote: > On 09/14/2012 05:26 AM, Vinod Koul wrote: > > On Thu, 2012-09-13 at 17:27 +0200, Lars-Peter Clausen wrote: > >> Hi, > >> > >> Hm... Do you think it would work as well if we implement this by > >> setting the > >> callback for the descriptor to NULL? If the callback is NULL there is > >> nothing to at the end of a transfer/period and the dma engine driver > >> may > >> choose to disable interrupts. This would also benefit non cyclic > >> transfers > >> where the callback is NULL and we do not need add the new parameter to > >> dmaengine_prep_dma_cyclic. > > That will work too.... BUT the idea of no_wake mode in ALSA is that we > > should not have any interrupts, so anything which is going to cause > > interrupts to AP in undesired. The interrupts still happen and it just > > that dmaengine driver is not notifying client. > > Well, the idea was that the driver would disable interrupts if there is no > callback to call, since there would be nothing to do in the interrupt > handler anyway. But I guess the flags approach should work fine as well. Yes we _could_ do that, but this relies on dmaengine driver to have this implicit understanding. Anyone using dmaengine library in ASoC may or may not be aware of this, so i would consider it hackish. Using this flag explicitly makes everyone aware what the intended behaviour is.
Hi, On 09/14/2012 11:50 AM, Vinod Koul wrote: >> Well, the idea was that the driver would disable interrupts if there is no >> callback to call, since there would be nothing to do in the interrupt >> handler anyway. But I guess the flags approach should work fine as well. > Yes we _could_ do that, but this relies on dmaengine driver to have this > implicit understanding. Anyone using dmaengine library in ASoC may or > may not be aware of this, so i would consider it hackish. > > Using this flag explicitly makes everyone aware what the intended > behaviour is. I'm not sure about which flags should ASoC set for the two case we are going to have. I think it should be something like this: unsigned long flags = DMA_CTRL_ACK; if (!substream->runtime->no_period_wakeup) flags |= DMA_PREP_INTERRUPT; I'm not 100% sure of the role of DMA_CTRL_ACK in this case. Or should we only handle the DMA_PREP_INTERRUPT flag, like this: unsigned long flags = 0; if (!substream->runtime->no_period_wakeup) flags |= DMA_PREP_INTERRUPT; What do you think?
On Fri, 2012-09-14 at 12:28 +0300, Peter Ujfalusi wrote: > Hi, > > On 09/14/2012 11:50 AM, Vinod Koul wrote: > >> Well, the idea was that the driver would disable interrupts if there is no > >> callback to call, since there would be nothing to do in the interrupt > >> handler anyway. But I guess the flags approach should work fine as well. > > Yes we _could_ do that, but this relies on dmaengine driver to have this > > implicit understanding. Anyone using dmaengine library in ASoC may or > > may not be aware of this, so i would consider it hackish. > > > > Using this flag explicitly makes everyone aware what the intended > > behaviour is. > > I'm not sure about which flags should ASoC set for the two case we are going > to have. I think it should be something like this: > > unsigned long flags = DMA_CTRL_ACK; usage of ACK flag is mostly async_tx thingy, we can skip it for slave. > > if (!substream->runtime->no_period_wakeup) > flags |= DMA_PREP_INTERRUPT; > I'm not 100% sure of the role of DMA_CTRL_ACK in this case. Or should we only > handle the DMA_PREP_INTERRUPT flag, like this: > > unsigned long flags = 0; > > if (!substream->runtime->no_period_wakeup) > flags |= DMA_PREP_INTERRUPT; > > > What do you think? Looks fine.
On Fri, Sep 14, 2012 at 12:28:28PM +0300, Peter Ujfalusi wrote: > I'm not sure about which flags should ASoC set for the two case we are going > to have. I think it should be something like this: > > unsigned long flags = DMA_CTRL_ACK; > > if (!substream->runtime->no_period_wakeup) > flags |= DMA_PREP_INTERRUPT; That looks about right. I would encourage DMA_CTRL_ACK to always be set for slave stuff, because it will (according to my understanding of it) make the behaviour no different from the async_tx stuff... which is important when support for that stuff gets added. We could, of course, set DMA_CTRL_ACK in the slave APIs beneath the user, but the user has access to tx->flags between the prepare and submit calls... And doing it in the submit callback will force DMA engine drivers to have two separate submission paths (as tx'es can't be acked at submission time.)
diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h index 9c02a45..990444b 100644 --- a/include/linux/dmaengine.h +++ b/include/linux/dmaengine.h @@ -653,7 +653,8 @@ static inline struct dma_async_tx_descriptor *dmaengine_prep_rio_sg( static inline struct dma_async_tx_descriptor *dmaengine_prep_dma_cyclic( struct dma_chan *chan, dma_addr_t buf_addr, size_t buf_len, - size_t period_len, enum dma_transfer_direction dir) + size_t period_len, enum dma_transfer_direction dir, + bool no_wakeup) { return chan->device->device_prep_dma_cyclic(chan, buf_addr, buf_len, period_len, dir, NULL); diff --git a/sound/soc/soc-dmaengine-pcm.c b/sound/soc/soc-dmaengine-pcm.c index 5df529e..6b70adb 100644 --- a/sound/soc/soc-dmaengine-pcm.c +++ b/sound/soc/soc-dmaengine-pcm.c @@ -147,7 +147,8 @@ static int dmaengine_pcm_prepare_and_submit(struct snd_pcm_substream *substream) desc = dmaengine_prep_dma_cyclic(chan, substream->runtime->dma_addr, snd_pcm_lib_buffer_bytes(substream), - snd_pcm_lib_period_bytes(substream), direction); + snd_pcm_lib_period_bytes(substream), direction, + substream->runtime->no_period_wakeup); if (!desc) return -ENOMEM;
The dmaengine_prep_dma_cyclic() function primarily used by audio for cyclic transfer required by ALSA. With this new parameter it is going to be possible to enable the SNDRV_PCM_INFO_NO_PERIOD_WAKEUP mode on platforms where it is possible. This patch only changes the public API first. Followup patch will change the device_prep_dma_cyclic() callback parameters to pass this information towards the dma drivers. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> CC: Lars-Peter Clausen <lars@metafoo.de> --- include/linux/dmaengine.h | 3 ++- sound/soc/soc-dmaengine-pcm.c | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-)