imx-ssi fixes

imx-ssi fixes

[Sascha Hauer]

Hi all,

Here are some bugfix patches for the imx-ssi driver. It should also fix
the problems reported in FIQ mode.

Sascha

[PATCH 1/3] imx-ssi: honor IMX_SSI_DMA flag

[Sascha Hauer]

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
-- 
1.7.0

[PATCH 2/3] imx-pcm-dma-mx2: restart DMA after an error

[Sascha Hauer]

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)
-- 
1.7.0

[PATCH 3/3] imx-ssi: Use a hrtimer in FIQ mode

[Sascha Hauer]

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;
-- 
1.7.0

Re: [PATCH 3/3] imx-ssi: Use a hrtimer in FIQ mode

[Mark Brown]

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).

Re: [PATCH 3/3] imx-ssi: Use a hrtimer in FIQ mode

[Sascha Hauer]

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

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 3/3] imx-ssi: Use a hrtimer in FIQ mode

[Liam Girdwood]

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

-- 
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk

Re: [PATCH 3/3] imx-ssi: Use a hrtimer in FIQ mode

[Sascha Hauer]

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


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 3/3] imx-ssi: Use a hrtimer in FIQ mode

[Mark Brown]

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.

Re: [PATCH 3/3] imx-ssi: Use a hrtimer in FIQ mode

[Liam Girdwood]

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

-- 
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk

Re: [PATCH 3/3] imx-ssi: Use a hrtimer in FIQ mode

[Mark Brown]

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.

Re: [PATCH 3/3] imx-ssi: Use a hrtimer in FIQ mode

[Valentin Longchamp]

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

-- 
Valentin Longchamp, PhD Student, EPFL-STI-LSRO1
valentin.longchamp@epfl.ch, Phone: +41216937827
http://people.epfl.ch/valentin.longchamp
MEB3494, Station 9, CH-1015 Lausanne

Re: imx-ssi fixes

[Mark Brown]

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.

Re: imx-ssi fixes

[Sascha Hauer]

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

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: imx-ssi fixes

[Mark Brown]

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.