[RFC 0/3] ASoC: davinci- pcm and McASP error detection

[RFC 0/3] ASoC: davinci- pcm and McASP error detection

[Ben Gardiner]

Both the EDMA3 and McASP have the ability to report error conditions. In both
cases these errors are silently dropped on the floor. 

I would like very much to get HW errors up the stack to the aplay process using
the harware. This is an RFC series demonstrating a first attempt. 

1/3 isn't nearly as half-baked as the others. It proposes to return
SNDRV_PCM_POS_XRUN from davinci-pcm's pointer() function like is currently done
in fsl_dma. The problem I have observed in test is that the stream does not abort; 
several underruns are reported before aplay finally dies. I would prefer immeadiate
death with a reason.

2/3 and 3/3 are a first-strike at registering 'health' callbacks provided by the 
CPU DAI so that error conditions can be reported back. The health callback is polled
on calls to pointer(). This may be too often. Also the current method of registering
these callbacks seems wrong -- please help me out here.

If there is an established method of bubbling HW errors up to userspace that I
completely missed: I am sorry for the noise and would be glad to hear about how to 
use it with davinci-pcm.

Best Regards,

Ben Gardiner

Ben Gardiner (3):
  [RFC] ASoC: davinci-pcm: latch EDMA errors
  [RFC] ASoC: davinci-pcm: add cpu-dai health callbacks
  [RFC] ASoC: davinci-mcasp: add cpu dai health callback

 sound/soc/davinci/davinci-mcasp.c |   26 ++++++++++++++++++++++++++
 sound/soc/davinci/davinci-pcm.c   |   19 +++++++++++++++++--
 sound/soc/davinci/davinci-pcm.h   |    3 +++
 3 files changed, 46 insertions(+), 2 deletions(-)

-- 
1.7.4.1

[PATCH 1/3] ASoC: davinci-pcm: latch EDMA errors

[Ben Gardiner]

The davinci-pcm driver currently ignores all EDMA completion callbacks that
could be indicating an error.

Latch any edma error-status callbacks and report them as SNDDRV_PCM_POS_XRUN
like is done in fsl_dma.c.

Signed-off-by: Ben Gardiner <bengardiner@nanometrics.ca>

---

In testing when an error occured early-on in playback the stream did not halt,
but several underruns were reported until eventually the stream halted.

Is there a better way to report HW errors up the stack?
---
 sound/soc/davinci/davinci-pcm.c |   13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/sound/soc/davinci/davinci-pcm.c b/sound/soc/davinci/davinci-pcm.c
index d5fe08c..41a3b5b 100644
--- a/sound/soc/davinci/davinci-pcm.c
+++ b/sound/soc/davinci/davinci-pcm.c
@@ -155,6 +155,7 @@ struct davinci_runtime_data {
 	int ram_link2;
 	struct edmacc_param asp_params;
 	struct edmacc_param ram_params;
+	unsigned error:1;
 };
 
 static void davinci_pcm_period_elapsed(struct snd_pcm_substream *substream)
@@ -245,8 +246,12 @@ static void davinci_pcm_dma_irq(unsigned link, u16 ch_status, void *data)
 	print_buf_info(prtd->ram_channel, "i ram_channel");
 	pr_debug("davinci_pcm: link=%d, status=0x%x\n", link, ch_status);
 
-	if (unlikely(ch_status != DMA_COMPLETE))
+	if (unlikely(ch_status != DMA_COMPLETE)) {
+		spin_lock(&prtd->lock);
+		prtd->error = 1;
+		spin_unlock(&prtd->lock);
 		return;
+	}
 
 	if (snd_pcm_running(substream)) {
 		spin_lock(&prtd->lock);
@@ -635,7 +640,7 @@ davinci_pcm_pointer(struct snd_pcm_substream *substream)
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	struct davinci_runtime_data *prtd = runtime->private_data;
 	unsigned int offset;
-	int asp_count;
+	int asp_count, error;
 	unsigned int period_size = snd_pcm_lib_period_bytes(substream);
 
 	/*
@@ -647,8 +652,12 @@ davinci_pcm_pointer(struct snd_pcm_substream *substream)
 	 */
 	spin_lock(&prtd->lock);
 	asp_count = prtd->period - 2;
+	error = prtd->error;
 	spin_unlock(&prtd->lock);
 
+	if (error)
+		return SNDRV_PCM_POS_XRUN;
+
 	if (asp_count < 0)
 		asp_count += runtime->periods;
 	asp_count *= period_size;
-- 
1.7.4.1

Re: [PATCH 1/3] ASoC: davinci-pcm: latch EDMA errors

[Mark Brown]

On Fri, Sep 30, 2011 at 05:23:01PM -0400, Ben Gardiner wrote:
> The davinci-pcm driver currently ignores all EDMA completion callbacks that
> could be indicating an error.

> Latch any edma error-status callbacks and report them as SNDDRV_PCM_POS_XRUN
> like is done in fsl_dma.c.

Nothing in this patch ever seems to clear the flag which seems rather
extreme.  I'd expect that if you're going to do this then the flag would
be cleared after one error has been reported.

> In testing when an error occured early-on in playback the stream did not halt,
> but several underruns were reported until eventually the stream halted.

> Is there a better way to report HW errors up the stack?

Not really, and it's not clear that it's constructive to try -  if
there's a problem that doesn't otherwise cause a failure then generally
the user will intervene.

Re: [PATCH 1/3] ASoC: davinci-pcm: latch EDMA errors

[Ben Gardiner]

Hi Mark,

Thank you for offering your insights.

On Sun, Oct 2, 2011 at 2:48 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Fri, Sep 30, 2011 at 05:23:01PM -0400, Ben Gardiner wrote:
>> The davinci-pcm driver currently ignores all EDMA completion callbacks that
>> could be indicating an error.
>
>> Latch any edma error-status callbacks and report them as SNDDRV_PCM_POS_XRUN
>> like is done in fsl_dma.c.
>
> Nothing in this patch ever seems to clear the flag which seems rather
> extreme.  I'd expect that if you're going to do this then the flag would
> be cleared after one error has been reported.

Ok. I think you have impressed upon me the intent of the _POS_XRUN
retum -- i see now that it should not be latched. I will fix this in
spin 2.

>> In testing when an error occured early-on in playback the stream did not halt,
>> but several underruns were reported until eventually the stream halted.
>
>> Is there a better way to report HW errors up the stack?
>
> Not really, and it's not clear that it's constructive to try -  if
> there's a problem that doesn't otherwise cause a failure then generally
> the user will intervene.

Again, thank you for your insight. My understanding now is that
_POS_XRUN is _the_ way to report HW errors and that it is up to the
application to determine the consequences of an xrun.

Best Regards,
Ben Gardiner

---
Nanometrics Inc.
http://www.nanometrics.ca

[RFC 2/3] ASoC: davinci-pcm: add cpu-dai health callbacks

[Ben Gardiner]

The CPU DAIs available to the davinci-pcm driver have the capability of
detecting and reporting errors.

Add callbacks to the struct davinci_pcm_dma_params passed to davinci-pcm
from the CPU DAI.

This has several shortcomings:
1) It bubbles up to the user as underruns, not a fatal error -- some may prefer
the former, I realize but the latter is more attractive to me. Same problem
as with the previous patch in this series.
2) passing it in the dma_params struct seems like dual-purposing that structure
3) the device instance must be passed as drvdata since I did not know how to get
back to the cpudai instance from a substream (sorry, please help!)

[not ready yet, please comment]
Not-Signed-off-by: Ben Gardiner <bengardiner@nanometrics.ca>
---
 sound/soc/davinci/davinci-pcm.c |    8 +++++++-
 sound/soc/davinci/davinci-pcm.h |    3 +++
 2 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/sound/soc/davinci/davinci-pcm.c b/sound/soc/davinci/davinci-pcm.c
index 41a3b5b..cb0e296 100644
--- a/sound/soc/davinci/davinci-pcm.c
+++ b/sound/soc/davinci/davinci-pcm.c
@@ -156,6 +156,8 @@ struct davinci_runtime_data {
 	struct edmacc_param asp_params;
 	struct edmacc_param ram_params;
 	unsigned error:1;
+	int (*cpudai_health)(struct snd_pcm_substream *, void *);
+	void *health_drvdata;
 };
 
 static void davinci_pcm_period_elapsed(struct snd_pcm_substream *substream)
@@ -655,7 +657,8 @@ davinci_pcm_pointer(struct snd_pcm_substream *substream)
 	error = prtd->error;
 	spin_unlock(&prtd->lock);
 
-	if (error)
+	if (error || (prtd->cpudai_health &&
+			prtd->cpudai_health(substream, prtd->health_drvdata)))
 		return SNDRV_PCM_POS_XRUN;
 
 	if (asp_count < 0)
@@ -706,6 +709,9 @@ static int davinci_pcm_open(struct snd_pcm_substream *substream)
 	prtd->ram_link = -1;
 	prtd->ram_link2 = -1;
 
+	prtd->cpudai_health = pa->health;
+	prtd->health_drvdata = pa->health_drvdata;
+
 	runtime->private_data = prtd;
 
 	ret = davinci_pcm_dma_request(substream);
diff --git a/sound/soc/davinci/davinci-pcm.h b/sound/soc/davinci/davinci-pcm.h
index c0d6c9b..0474d97 100644
--- a/sound/soc/davinci/davinci-pcm.h
+++ b/sound/soc/davinci/davinci-pcm.h
@@ -26,6 +26,9 @@ struct davinci_pcm_dma_params {
 	unsigned char data_type;	/* xfer data type */
 	unsigned char convert_mono_stereo;
 	unsigned int fifo_level;
+
+	int (*health)(struct snd_pcm_substream *, void *drvdata);
+	void *health_drvdata;
 };
 
 #endif
-- 
1.7.4.1

Re: [RFC 2/3] ASoC: davinci-pcm: add cpu-dai health callbacks

[Mark Brown]

On Fri, Sep 30, 2011 at 05:23:02PM -0400, Ben Gardiner wrote:
> The CPU DAIs available to the davinci-pcm driver have the capability of
> detecting and reporting errors.

> Add callbacks to the struct davinci_pcm_dma_params passed to davinci-pcm
> from the CPU DAI.

This looks like something we should be doing at the subsystem level, the
DaVinci is far from unique in having the ability to detect errors at the
DAI level.

> This has several shortcomings:
> 1) It bubbles up to the user as underruns, not a fatal error -- some may prefer
> the former, I realize but the latter is more attractive to me. Same problem
> as with the previous patch in this series.

Why do you prefer a fatal error, and how do you distinguish a fatal
error from a glitch?

Re: [RFC 2/3] ASoC: davinci-pcm: add cpu-dai health callbacks

[Ben Gardiner]

On Sun, Oct 2, 2011 at 2:54 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> This looks like something we should be doing at the subsystem level, the
> DaVinci is far from unique in having the ability to detect errors at the
> DAI level.

Agreed; I started out with the intent to call DAI health ops from
snd_pcm_update_hw_ptr0() but I couldn't find may way to a cpudai
instance from the given substream. Any tips on this or a plan of
attack would be apreciated.

> Why do you prefer a fatal error, and how do you distinguish a fatal
> error from a glitch?

Our use of ALSA is such that in the rare case of either a glitch or a
hardware error we would prefer to fail fatally rather than ignore.
This is because we would like to guarantee that playback occurred
correctly or not at all.

>From your other email I see now that the intended behaviour can be
accomplished entirely in the application. Returning _POS_XRUN should
be sufficient here in ASoC.

Best Regards,
Ben Gardiner

---
Nanometrics Inc.
http://www.nanometrics.ca

Re: [RFC 2/3] ASoC: davinci-pcm: add cpu-dai health callbacks

[Mark Brown]

On Mon, Oct 03, 2011 at 10:05:45PM -0400, Ben Gardiner wrote:

> From your other email I see now that the intended behaviour can be
> accomplished entirely in the application. Returning _POS_XRUN should
> be sufficient here in ASoC.

OK, great - I was worried this might get complicated but if the drivers
can just punt the decision to userspace that makes life much easier.

[RFC 3/3] ASoC: davinci-mcasp: add cpu dai health callback

[Ben Gardiner]

The McASP has RERR and XERR bits in its RSTAT and XSTAT registers which
report the OR'd state of several potential errors.

Register a function to check the status of these bits and report HW errors
back up the stack.

[not ready yet, please comment]
Not-Signed-off-by: Ben Gardiner <bengardiner@nanometrics.ca>
---
 sound/soc/davinci/davinci-mcasp.c |   26 ++++++++++++++++++++++++++
 1 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/sound/soc/davinci/davinci-mcasp.c b/sound/soc/davinci/davinci-mcasp.c
index 7173df2..ca4073f 100644
--- a/sound/soc/davinci/davinci-mcasp.c
+++ b/sound/soc/davinci/davinci-mcasp.c
@@ -273,6 +273,11 @@
 #define MUTETXDMAERR	BIT(12)
 
 /*
+ * DAVINCI_MCASP_RXSTAT_REG - Receiver Status Register bits
+ */
+#define RERR		BIT(8)
+
+/*
  * DAVINCI_MCASP_REVTCTL_REG - Receiver DMA Event Control Register bits
  */
 #define RXDATADMADIS	BIT(0)
@@ -283,6 +288,11 @@
 #define TXDATADMADIS	BIT(0)
 
 /*
+ * DAVINCI_MCASP_TXSTAT_REG - Transmitter Status Register bits
+ */
+#define XERR		BIT(8)
+
+/*
  * DAVINCI_MCASP_W[R]FIFOCTL - Write/Read FIFO Control Register bits
  */
 #define FIFO_ENABLE	BIT(16)
@@ -813,6 +823,19 @@ static int davinci_mcasp_startup(struct snd_pcm_substream *substream,
 	return 0;
 }
 
+static int davinci_mcasp_health(struct snd_pcm_substream *substream,
+		void *data)
+{
+	struct davinci_audio_dev *dev = data;
+
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+		return  mcasp_get_reg(dev->base + DAVINCI_MCASP_TXSTAT_REG) &
+			XERR;
+	else
+		return  mcasp_get_reg(dev->base + DAVINCI_MCASP_RXSTAT_REG) &
+			RERR;
+}
+
 static struct snd_soc_dai_ops davinci_mcasp_dai_ops = {
 	.startup	= davinci_mcasp_startup,
 	.trigger	= davinci_mcasp_trigger,
@@ -919,6 +942,9 @@ static int davinci_mcasp_probe(struct platform_device *pdev)
 	dma_data->dma_addr = (dma_addr_t) (pdata->tx_dma_offset +
 							mem->start);
 
+	dma_data->health = davinci_mcasp_health;
+	dma_data->health_drvdata = dev;
+
 	/* first TX, then RX */
 	res = platform_get_resource(pdev, IORESOURCE_DMA, 0);
 	if (!res) {
-- 
1.7.4.1