[alsa-devel] [RFC 0/3] ASoC: davinci- pcm and McASP error detection
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
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
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.
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
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
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?
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
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.
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
participants (2)
-
Ben Gardiner -
Mark Brown