[alsa-devel] imx-ssi fixes
Hi all,
Here are some bugfix patches for the imx-ssi driver. It should also fix the problems reported in FIQ mode.
Sascha
When checking if we are DMA capable we have to check for the IMX_SSI_DMA flag which is already set from platform_data instead of setting it again when we want to do DMA.
Signed-off-by: Sascha Hauer s.hauer@pengutronix.de --- sound/soc/imx/imx-ssi.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/sound/soc/imx/imx-ssi.c b/sound/soc/imx/imx-ssi.c index 6546b06..2a07fd3 100644 --- a/sound/soc/imx/imx-ssi.c +++ b/sound/soc/imx/imx-ssi.c @@ -653,7 +653,8 @@ static int imx_ssi_probe(struct platform_device *pdev) dai->private_data = ssi;
if ((cpu_is_mx27() || cpu_is_mx21()) && - !(ssi->flags & IMX_SSI_USE_AC97)) { + !(ssi->flags & IMX_SSI_USE_AC97) && + (ssi->flags & IMX_SSI_DMA)) { ssi->flags |= IMX_SSI_DMA; platform = imx_ssi_dma_mx2_init(pdev, ssi); } else
Signed-off-by: Sascha Hauer s.hauer@pengutronix.de --- sound/soc/imx/imx-pcm-dma-mx2.c | 15 ++++++++++++++- 1 files changed, 14 insertions(+), 1 deletions(-)
diff --git a/sound/soc/imx/imx-pcm-dma-mx2.c b/sound/soc/imx/imx-pcm-dma-mx2.c index 86668ab..aa88869 100644 --- a/sound/soc/imx/imx-pcm-dma-mx2.c +++ b/sound/soc/imx/imx-pcm-dma-mx2.c @@ -71,7 +71,12 @@ static void imx_ssi_dma_callback(int channel, void *data)
static void snd_imx_dma_err_callback(int channel, void *data, int err) { - pr_err("DMA error callback called\n"); + struct snd_pcm_substream *substream = data; + struct snd_soc_pcm_runtime *rtd = substream->private_data; + struct imx_pcm_dma_params *dma_params = rtd->dai->cpu_dai->dma_data; + struct snd_pcm_runtime *runtime = substream->runtime; + struct imx_pcm_runtime_data *iprtd = runtime->private_data; + int ret;
pr_err("DMA timeout on channel %d -%s%s%s%s\n", channel, @@ -79,6 +84,14 @@ static void snd_imx_dma_err_callback(int channel, void *data, int err) err & IMX_DMA_ERR_REQUEST ? " request" : "", err & IMX_DMA_ERR_TRANSFER ? " transfer" : "", err & IMX_DMA_ERR_BUFFER ? " buffer" : ""); + + imx_dma_disable(iprtd->dma); + ret = imx_dma_setup_sg(iprtd->dma, iprtd->sg_list, iprtd->sg_count, + IMX_DMA_LENGTH_LOOP, dma_params->dma_addr, + substream->stream == SNDRV_PCM_STREAM_PLAYBACK ? + DMA_MODE_WRITE : DMA_MODE_READ); + if (!ret) + imx_dma_enable(iprtd->dma); }
static int imx_ssi_dma_alloc(struct snd_pcm_substream *substream)
Using a regular timer results in poll times < 1 jiffie with small buffers, so we loaded the timer with the actual jiffie value. We can be more accurate using a hrtimer. Also, we have to call snd_pcm_period_elapsed after playing period_bytes and not runtime->period_size (which is in samples and not in bytes).
Signed-off-by: Sascha Hauer s.hauer@pengutronix.de --- sound/soc/imx/imx-pcm-fiq.c | 45 ++++++++++++++++++++---------------------- 1 files changed, 21 insertions(+), 24 deletions(-)
diff --git a/sound/soc/imx/imx-pcm-fiq.c b/sound/soc/imx/imx-pcm-fiq.c index f96a373..2646e05 100644 --- a/sound/soc/imx/imx-pcm-fiq.c +++ b/sound/soc/imx/imx-pcm-fiq.c @@ -39,20 +39,17 @@ struct imx_pcm_runtime_data { unsigned long offset; unsigned long last_offset; unsigned long size; - struct timer_list timer; - int poll_time; + struct hrtimer hrt; + int poll_time_ns; + struct snd_pcm_substream *substream; };
-static inline void imx_ssi_set_next_poll(struct imx_pcm_runtime_data *iprtd) +static enum hrtimer_restart snd_hrtimer_callback(struct hrtimer *hrt) { - iprtd->timer.expires = jiffies + iprtd->poll_time; -} - -static void imx_ssi_timer_callback(unsigned long data) -{ - struct snd_pcm_substream *substream = (void *)data; + struct imx_pcm_runtime_data *iprtd = + container_of(hrt, struct imx_pcm_runtime_data, hrt); + struct snd_pcm_substream *substream = iprtd->substream; struct snd_pcm_runtime *runtime = substream->runtime; - struct imx_pcm_runtime_data *iprtd = runtime->private_data; struct pt_regs regs; unsigned long delta;
@@ -72,16 +69,14 @@ static void imx_ssi_timer_callback(unsigned long data)
/* If we've transferred at least a period then report it and * reset our poll time */ - if (delta >= runtime->period_size) { + if (delta >= iprtd->period) { snd_pcm_period_elapsed(substream); iprtd->last_offset = iprtd->offset; - - imx_ssi_set_next_poll(iprtd); }
- /* Restart the timer; if we didn't report we'll run on the next tick */ - add_timer(&iprtd->timer); + hrtimer_forward_now(hrt, ns_to_ktime(iprtd->poll_time_ns));
+ return HRTIMER_RESTART; }
static struct fiq_handler fh = { @@ -99,8 +94,8 @@ static int snd_imx_pcm_hw_params(struct snd_pcm_substream *substream, iprtd->period = params_period_bytes(params) ; iprtd->offset = 0; iprtd->last_offset = 0; - iprtd->poll_time = HZ / (params_rate(params) / params_period_size(params)); - + iprtd->poll_time_ns = 1000000000 / params_rate(params) * + params_period_size(params); snd_pcm_set_runtime_buffer(substream, &substream->dma_buffer);
return 0; @@ -135,8 +130,8 @@ static int snd_imx_pcm_trigger(struct snd_pcm_substream *substream, int cmd) case SNDRV_PCM_TRIGGER_START: case SNDRV_PCM_TRIGGER_RESUME: case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: - imx_ssi_set_next_poll(iprtd); - add_timer(&iprtd->timer); + hrtimer_start(&iprtd->hrt, ns_to_ktime(iprtd->poll_time_ns), + HRTIMER_MODE_REL); if (++fiq_enable == 1) enable_fiq(imx_pcm_fiq);
@@ -145,7 +140,7 @@ static int snd_imx_pcm_trigger(struct snd_pcm_substream *substream, int cmd) case SNDRV_PCM_TRIGGER_STOP: case SNDRV_PCM_TRIGGER_SUSPEND: case SNDRV_PCM_TRIGGER_PAUSE_PUSH: - del_timer(&iprtd->timer); + hrtimer_cancel(&iprtd->hrt); if (--fiq_enable == 0) disable_fiq(imx_pcm_fiq);
@@ -194,9 +189,10 @@ static int snd_imx_open(struct snd_pcm_substream *substream) iprtd = kzalloc(sizeof(*iprtd), GFP_KERNEL); runtime->private_data = iprtd;
- init_timer(&iprtd->timer); - iprtd->timer.data = (unsigned long)substream; - iprtd->timer.function = imx_ssi_timer_callback; + iprtd->substream = substream; + + hrtimer_init(&iprtd->hrt, CLOCK_MONOTONIC, HRTIMER_MODE_REL); + iprtd->hrt.function = snd_hrtimer_callback;
ret = snd_pcm_hw_constraint_integer(substream->runtime, SNDRV_PCM_HW_PARAM_PERIODS); @@ -212,7 +208,8 @@ static int snd_imx_close(struct snd_pcm_substream *substream) struct snd_pcm_runtime *runtime = substream->runtime; struct imx_pcm_runtime_data *iprtd = runtime->private_data;
- del_timer_sync(&iprtd->timer); + hrtimer_cancel(&iprtd->hrt); + kfree(iprtd);
return 0;
On Thu, Apr 08, 2010 at 11:31:26AM +0200, Sascha Hauer wrote:
- /* Restart the timer; if we didn't report we'll run on the next tick */
- add_timer(&iprtd->timer);
- hrtimer_forward_now(hrt, ns_to_ktime(iprtd->poll_time_ns));
Hrm, this looks like it's going to have an issue with clock drift - we're now unconditionally advancing the timer every period, even if the data transfer hasn't pushed through a period of data. This will cause problems on lengthy playbacks (and shorter ones if the clocks are sufficiently out of sync).
On Thu, Apr 08, 2010 at 10:53:25AM +0100, Mark Brown wrote:
On Thu, Apr 08, 2010 at 11:31:26AM +0200, Sascha Hauer wrote:
- /* Restart the timer; if we didn't report we'll run on the next tick */
- add_timer(&iprtd->timer);
- hrtimer_forward_now(hrt, ns_to_ktime(iprtd->poll_time_ns));
Hrm, this looks like it's going to have an issue with clock drift - we're now unconditionally advancing the timer every period, even if the data transfer hasn't pushed through a period of data. This will cause problems on lengthy playbacks (and shorter ones if the clocks are sufficiently out of sync).
We are calling snd_pcm_period_elapsed when at least one period is over. As I see it the worst thing that could happen is that we have not transfered enough data for one period in the timer callback and thus we call snd_pcm_period_elapsed in the next timer callback, so about one period too late, but the comment in sound/core/pcm_lib.c says:
Even if more than one periods have elapsed since the last call, you have to call this only once.
So I think this should be save.
Sascha
On Thu, 2010-04-08 at 15:13 +0200, Sascha Hauer wrote:
On Thu, Apr 08, 2010 at 10:53:25AM +0100, Mark Brown wrote:
On Thu, Apr 08, 2010 at 11:31:26AM +0200, Sascha Hauer wrote:
- /* Restart the timer; if we didn't report we'll run on the next tick */
- add_timer(&iprtd->timer);
- hrtimer_forward_now(hrt, ns_to_ktime(iprtd->poll_time_ns));
Hrm, this looks like it's going to have an issue with clock drift - we're now unconditionally advancing the timer every period, even if the data transfer hasn't pushed through a period of data. This will cause problems on lengthy playbacks (and shorter ones if the clocks are sufficiently out of sync).
We are calling snd_pcm_period_elapsed when at least one period is over. As I see it the worst thing that could happen is that we have not transfered enough data for one period in the timer callback and thus we call snd_pcm_period_elapsed in the next timer callback, so about one period too late, but the comment in sound/core/pcm_lib.c says:
Even if more than one periods have elapsed since the last call, you have to call this only once.
So I think this should be save.
Yeah agreed, as long as it's called at least once then we should be safe here.
Btw, how does this driver behave under moderate -> heavy IO load. I do remember the SDMA based driver occasionally starved the SSI FIFO under moderate IO load (i.e. aplay reading wav file over NFS).
All Acked-by: Liam Girdwood lrg@slimlogic.co.uk
Liam
On Thu, Apr 08, 2010 at 02:30:13PM +0100, Liam Girdwood wrote:
On Thu, 2010-04-08 at 15:13 +0200, Sascha Hauer wrote:
On Thu, Apr 08, 2010 at 10:53:25AM +0100, Mark Brown wrote:
On Thu, Apr 08, 2010 at 11:31:26AM +0200, Sascha Hauer wrote:
- /* Restart the timer; if we didn't report we'll run on the next tick */
- add_timer(&iprtd->timer);
- hrtimer_forward_now(hrt, ns_to_ktime(iprtd->poll_time_ns));
Hrm, this looks like it's going to have an issue with clock drift - we're now unconditionally advancing the timer every period, even if the data transfer hasn't pushed through a period of data. This will cause problems on lengthy playbacks (and shorter ones if the clocks are sufficiently out of sync).
We are calling snd_pcm_period_elapsed when at least one period is over. As I see it the worst thing that could happen is that we have not transfered enough data for one period in the timer callback and thus we call snd_pcm_period_elapsed in the next timer callback, so about one period too late, but the comment in sound/core/pcm_lib.c says:
Even if more than one periods have elapsed since the last call, you have to call this only once.
So I think this should be save.
Yeah agreed, as long as it's called at least once then we should be safe here.
Btw, how does this driver behave under moderate -> heavy IO load. I do remember the SDMA based driver occasionally starved the SSI FIFO under moderate IO load (i.e. aplay reading wav file over NFS).
aplay works fine with files over nfs and survives moderate hackbench attacks. I do not check the underrun bit though so there might be glitches I did not hear.
Sascha
On Thu, Apr 08, 2010 at 03:13:53PM +0200, Sascha Hauer wrote:
On Thu, Apr 08, 2010 at 10:53:25AM +0100, Mark Brown wrote:
Hrm, this looks like it's going to have an issue with clock drift - we're now unconditionally advancing the timer every period, even if the data transfer hasn't pushed through a period of data. This will cause problems on lengthy playbacks (and shorter ones if the clocks are sufficiently out of sync).
We are calling snd_pcm_period_elapsed when at least one period is over. As I see it the worst thing that could happen is that we have not transfered enough data for one period in the timer callback and thus we call snd_pcm_period_elapsed in the next timer callback, so about one period too late, but the comment in sound/core/pcm_lib.c says:
Even if more than one periods have elapsed since the last call, you have to call this only once.
So I think this should be save.
The risk here is fragility caused by delaying the notification.
The issue is that if the period is long enough and/or the application is running too close to where the hardware is then the delay of what's likely to be almost an entire period is likely to glitch - you can end up with a situation where you're essentially notifying immediately before the end of the next period rather than immediately after the end of the current period.
Consider, for example what happens if the hrtimer runs slightly faster than the audio clock. Eventually you'll get:
- The timer runs, period A has a frame or two to go still. - Period A completes, period A' begins. - The timer fires again - period A is notified, period A' is almost complete.
In this situation the completion notifications lag the actual completion of the frame by almost a period (though this lag will go down over time). Most of the time this will still be fine and everything will work OK but I would expect this to cause issues with some applications, especially if they're trying to be latency sensitive.
On Thu, 2010-04-08 at 15:03 +0100, Mark Brown wrote:
On Thu, Apr 08, 2010 at 03:13:53PM +0200, Sascha Hauer wrote:
On Thu, Apr 08, 2010 at 10:53:25AM +0100, Mark Brown wrote:
Hrm, this looks like it's going to have an issue with clock drift - we're now unconditionally advancing the timer every period, even if the data transfer hasn't pushed through a period of data. This will cause problems on lengthy playbacks (and shorter ones if the clocks are sufficiently out of sync).
We are calling snd_pcm_period_elapsed when at least one period is over. As I see it the worst thing that could happen is that we have not transfered enough data for one period in the timer callback and thus we call snd_pcm_period_elapsed in the next timer callback, so about one period too late, but the comment in sound/core/pcm_lib.c says:
Even if more than one periods have elapsed since the last call, you have to call this only once.
So I think this should be save.
The risk here is fragility caused by delaying the notification.
The issue is that if the period is long enough and/or the application is running too close to where the hardware is then the delay of what's likely to be almost an entire period is likely to glitch - you can end up with a situation where you're essentially notifying immediately before the end of the next period rather than immediately after the end of the current period.
Consider, for example what happens if the hrtimer runs slightly faster than the audio clock. Eventually you'll get:
- The timer runs, period A has a frame or two to go still.
- Period A completes, period A' begins.
- The timer fires again - period A is notified, period A' is almost complete.
In this situation the completion notifications lag the actual completion of the frame by almost a period (though this lag will go down over time). Most of the time this will still be fine and everything will work OK but I would expect this to cause issues with some applications, especially if they're trying to be latency sensitive.
I agree this will drift but I _think_ we are probably workable here for most latency sensitive apps as long as the worst case notification delay is 1 period, pointer() is accurate to a few frames and there is a least
3 or 4 period buffers available for use in our driver buffer (for each
direction).
Currently the min periods for this driver is 2, so I would probably change to 4 to reduce drift related glitches.
Liam
On Thu, Apr 08, 2010 at 04:14:29PM +0100, Liam Girdwood wrote:
I agree this will drift but I _think_ we are probably workable here for most latency sensitive apps as long as the worst case notification delay is 1 period, pointer() is accurate to a few frames and there is a least
3 or 4 period buffers available for use in our driver buffer (for each
direction).
Yes, if they're running more than one period ahead everything should be fine.
Currently the min periods for this driver is 2, so I would probably change to 4 to reduce drift related glitches.
Good idea.
Sascha Hauer wrote:
Using a regular timer results in poll times < 1 jiffie with small buffers, so we loaded the timer with the actual jiffie value. We can be more accurate using a hrtimer. Also, we have to call snd_pcm_period_elapsed after playing period_bytes and not runtime->period_size (which is in samples and not in bytes).
Signed-off-by: Sascha Hauer s.hauer@pengutronix.de
sound/soc/imx/imx-pcm-fiq.c | 45 ++++++++++++++++++++---------------------- 1 files changed, 21 insertions(+), 24 deletions(-)
Sorry to bother you guys in your discussion (most of which I don't understand because of my poor alsa knowledge).
I just wanted to tell that I have given this patch a spin on a mx31moboard system. imx-ssi now works much better (the audio does not loop on one buffer anymore as it did with the earlier timer version) in various configuration (speaker-test with various rates up to 48 kHz as well as aplay with a wav file, which had never worked for me) and the CPU usage is much lower BUT I have experienced some deadlocks completely hanging the system looping on what should be the last buffer of the wave file.
Val
On Thu, Apr 08, 2010 at 11:31:23AM +0200, Sascha Hauer wrote:
Here are some bugfix patches for the imx-ssi driver. It should also fix the problems reported in FIQ mode.
Applied all three, thanks - the hrtimer stuff works an awful lot better than the vanilla timer stuff did. It's a shame I hadn't noticed that hrtimers did manage to give better resolution on i.MX when I looked into this previously.
It'd be good if you could look into the timer sync issue - doing something like deferring the timer by a proportion of a period if the period hasn't elapsed ought to cover it.
On Thu, Apr 08, 2010 at 03:47:50PM +0100, Mark Brown wrote:
On Thu, Apr 08, 2010 at 11:31:23AM +0200, Sascha Hauer wrote:
Here are some bugfix patches for the imx-ssi driver. It should also fix the problems reported in FIQ mode.
Applied all three, thanks - the hrtimer stuff works an awful lot better than the vanilla timer stuff did. It's a shame I hadn't noticed that hrtimers did manage to give better resolution on i.MX when I looked into this previously.
It'd be good if you could look into the timer sync issue - doing something like deferring the timer by a proportion of a period if the period hasn't elapsed ought to cover it.
I'll look into it. I'm thinking of
- busywaiting if the current period hasn't elapsed (should rarely if ever the case since the fiq works reliable and the timer interrupt introduces additional delays. - calculate the next callback time based on the current offset.
If you haven't pushed the patches forward it would be good to delay the hrtimer patch a bit. I managed to shoot the system down while playing audio with a hackbench 10. This doesn't happen without the hrtimer patch. This may be related to the problem Valentin reported.
Sascha
On Thu, Apr 08, 2010 at 05:51:13PM +0200, Sascha Hauer wrote:
If you haven't pushed the patches forward it would be good to delay the hrtimer patch a bit. I managed to shoot the system down while playing audio with a hackbench 10. This doesn't happen without the hrtimer patch. This may be related to the problem Valentin reported.
Already pushed to Takashi, but he's not pulled yet.
participants (4)
-
Liam Girdwood -
Mark Brown -
Sascha Hauer -
Valentin Longchamp