| Message ID | 1251761505-11353-1-git-send-email-troy.kisky@boundarydevices.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
Mark Brown wrote: > On Mon, Aug 31, 2009 at 04:31:43PM -0700, Troy Kisky wrote: > >> + /* >> + * Allowing this is more efficient and eliminates left and right swaps >> + * caused by underruns, but will swap the left and right channels >> + * when compared to previous behavior. >> + */ >> + unsigned disable_channel_combine:1; > > Last time you submitted this I suggested changing this to make the > default the other way round so that there's no breakage on existing > boards which aren't designed for this channel swap behaviour. Is there > some reason not to do that? > I think that it is better to make sure that they know the possible problems disabling this may cause. Channels always swapped seems a lot better than channels randomly swapped. Troy
Mark Brown wrote: > On Tue, Sep 01, 2009 at 11:23:27AM -0700, Troy Kisky wrote: >> Mark Brown wrote: > >>> Last time you submitted this I suggested changing this to make the >>> default the other way round so that there's no breakage on existing >>> boards which aren't designed for this channel swap behaviour. Is there >>> some reason not to do that? > >> I think that it is better to make sure that they know the possible problems >> disabling this may cause. Channels always swapped seems a lot better than >> channels randomly swapped. > > Of course, the other way of looking at it is that with the channel > swapping you've got guaranteed breakage - it sounds less good if you say > it that way :) How common are the channel swaps? > I was getting a lot when playing videos. It mainly just sounded bad until I got a stereo audio file with different frequency sine waves to understand better what was happening. Then, you could hear the channels swap frequently, more than once per second. If using sram, it is not an issue unless another device is using the same TC. But sram isn't on by default either. And probably shouldn't be since the newer chips don't have an underrun problem. Does anyone else want to share their thoughts/ experience? Thanks Troy
Mark Brown wrote: > On Tue, Sep 01, 2009 at 12:19:26PM -0700, Troy Kisky wrote: >> Mark Brown wrote: > >>> Of course, the other way of looking at it is that with the channel >>> swapping you've got guaranteed breakage - it sounds less good if you say >>> it that way :) How common are the channel swaps? > >> I was getting a lot when playing videos. It mainly just sounded bad until I got >> a stereo audio file with different frequency sine waves to understand better >> what was happening. Then, you could hear the channels swap frequently, more >> than once per second. If using sram, it is not an issue unless another device > > So, very often under heavy load then? That'd be consistent with simlar > problems on other devices. Part of the trouble here is that things like > video can make the channel swap more noticable - if stereo is used to > track the movement of an object on screen the channel swap would stop > the effect tying in with the visuals. > >> is using the same TC. But sram isn't on by default either. And probably shouldn't >> be since the newer chips don't have an underrun problem. > > Hrm, that suggests that if it's enabled at all the default should depend > on the chip in use? > That seems unnecessarily complex to me. As long as platform data can specify what you need, you'll eventually get it right. If tracking of an object is always wrong because of a channel swap, that is easier to notice, and debug, and fix, then if the tracking is only occasionally wrong. I'd much rather have a repeatable bug. And most codecs do allow you to swap the left and right channels. So, for most people, the fix will not be to disable channel combining. Troy
Mark Brown wrote: > On Tue, Sep 01, 2009 at 01:42:02PM -0700, Troy Kisky wrote: >> Mark Brown wrote: > >>>> is using the same TC. But sram isn't on by default either. And probably shouldn't >>>> be since the newer chips don't have an underrun problem. > >>> Hrm, that suggests that if it's enabled at all the default should depend >>> on the chip in use? > >> That seems unnecessarily complex to me. As long as platform data can >> specify what you need, you'll eventually get it right. If tracking of >> an object is always wrong because of a channel swap, that is easier >> to notice, and debug, and fix, then if the tracking is only occasionally >> wrong. I'd much rather have a repeatable bug. And most codecs do allow >> you to swap the left and right channels. So, for most people, the fix >> will not be to disable channel combining. > > My thinking was that if the newer chips don't have the underrun issue at > all then it seems like a bad move to enable the workaround for them > since they're currently fine. There should be no intermittent problems > if the underrun issue isn't present. > True, there shouldn't be a problem. However, from a efficiency point of view, it is still better to have the workaround. Fewer memory accesses may free a little bandwidth for other uses. Troy
diff --git a/arch/arm/mach-davinci/include/mach/asp.h b/arch/arm/mach-davinci/include/mach/asp.h index 18e4ce3..a3d2aa1 100644 --- a/arch/arm/mach-davinci/include/mach/asp.h +++ b/arch/arm/mach-davinci/include/mach/asp.h @@ -51,6 +51,12 @@ struct snd_platform_data { u32 rx_dma_offset; enum dma_event_q eventq_no; /* event queue number */ unsigned int codec_fmt; + /* + * Allowing this is more efficient and eliminates left and right swaps + * caused by underruns, but will swap the left and right channels + * when compared to previous behavior. + */ + unsigned disable_channel_combine:1; /* McASP specific fields */ int tdm_slots; diff --git a/sound/soc/davinci/davinci-i2s.c b/sound/soc/davinci/davinci-i2s.c index 12a6c54..081b2d4 100644 --- a/sound/soc/davinci/davinci-i2s.c +++ b/sound/soc/davinci/davinci-i2s.c @@ -97,6 +97,23 @@ enum { DAVINCI_MCBSP_WORD_32, }; +static const unsigned char data_type[SNDRV_PCM_FORMAT_S32_LE + 1] = { + [SNDRV_PCM_FORMAT_S8] = 1, + [SNDRV_PCM_FORMAT_S16_LE] = 2, + [SNDRV_PCM_FORMAT_S32_LE] = 4, +}; + +static const unsigned char asp_word_length[SNDRV_PCM_FORMAT_S32_LE + 1] = { + [SNDRV_PCM_FORMAT_S8] = DAVINCI_MCBSP_WORD_8, + [SNDRV_PCM_FORMAT_S16_LE] = DAVINCI_MCBSP_WORD_16, + [SNDRV_PCM_FORMAT_S32_LE] = DAVINCI_MCBSP_WORD_32, +}; + +static const unsigned char double_fmt[SNDRV_PCM_FORMAT_S32_LE + 1] = { + [SNDRV_PCM_FORMAT_S8] = SNDRV_PCM_FORMAT_S16_LE, + [SNDRV_PCM_FORMAT_S16_LE] = SNDRV_PCM_FORMAT_S32_LE, +}; + static struct davinci_pcm_dma_params davinci_i2s_pcm_out = { .name = "I2S PCM Stereo out", }; @@ -113,6 +130,27 @@ struct davinci_mcbsp_dev { u32 pcr; struct clk *clk; struct davinci_pcm_dma_params *dma_params[2]; + /* + * Combining both channels into 1 element will at least double the + * amount of time between servicing the dma channel, increase + * effiency, and reduce the chance of overrun/underrun. But, + * it will result in the left & right channels being swapped. + * + * If relabeling the left and right channels is not possible, + * you may want to let the codec know to swap them back. + * + * It may allow x10 the amount of time to service dma requests, + * if the codec is master and is using an unnecessarily fast bit clock + * (ie. tlvaic23b), independent of the sample rate. So, having an + * entire frame at once means it can be serviced at the sample rate + * instead of the bit clock rate. + * + * In the now unlikely case that an underrun still + * occurs, both the left and right samples will be repeated + * so that no pops are heard, and the left and right channels + * won't end up being swapped because of the underrun. + */ + unsigned disable_channel_combine:1; }; static inline void davinci_mcbsp_write_reg(struct davinci_mcbsp_dev *dev, @@ -359,6 +397,8 @@ static int davinci_i2s_hw_params(struct snd_pcm_substream *substream, int mcbsp_word_length; unsigned int rcr, xcr, srgr; u32 spcr; + snd_pcm_format_t fmt; + unsigned element_cnt = 1; /* general line settings */ spcr = davinci_mcbsp_read_reg(dev, DAVINCI_MCBSP_SPCR_REG); @@ -388,27 +428,22 @@ static int davinci_i2s_hw_params(struct snd_pcm_substream *substream, xcr |= DAVINCI_MCBSP_XCR_XDATDLY(1); } /* Determine xfer data type */ - switch (params_format(params)) { - case SNDRV_PCM_FORMAT_S8: - dma_params->data_type = 1; - mcbsp_word_length = DAVINCI_MCBSP_WORD_8; - break; - case SNDRV_PCM_FORMAT_S16_LE: - dma_params->data_type = 2; - mcbsp_word_length = DAVINCI_MCBSP_WORD_16; - break; - case SNDRV_PCM_FORMAT_S32_LE: - dma_params->data_type = 4; - mcbsp_word_length = DAVINCI_MCBSP_WORD_32; - break; - default: + fmt = params_format(params); + if ((fmt > SNDRV_PCM_FORMAT_S32_LE) || !data_type[fmt]) { printk(KERN_WARNING "davinci-i2s: unsupported PCM format\n"); return -EINVAL; } - - dma_params->acnt = dma_params->data_type; - rcr |= DAVINCI_MCBSP_RCR_RFRLEN1(1); - xcr |= DAVINCI_MCBSP_XCR_XFRLEN1(1); + if (params_channels(params) == 2) { + element_cnt = 2; + if (double_fmt[fmt] && !dev->disable_channel_combine) { + element_cnt = 1; + fmt = double_fmt[fmt]; + } + } + dma_params->acnt = dma_params->data_type = data_type[fmt]; + mcbsp_word_length = asp_word_length[fmt]; + rcr |= DAVINCI_MCBSP_RCR_RFRLEN1(element_cnt - 1); + xcr |= DAVINCI_MCBSP_XCR_XFRLEN1(element_cnt - 1); rcr |= DAVINCI_MCBSP_RCR_RWDLEN1(mcbsp_word_length) | DAVINCI_MCBSP_RCR_RWDLEN2(mcbsp_word_length); @@ -524,7 +559,8 @@ static int davinci_i2s_probe(struct platform_device *pdev) ret = -ENOMEM; goto err_release_region; } - + if (pdata) + dev->disable_channel_combine = pdata->disable_channel_combine; dev->clk = clk_get(&pdev->dev, NULL); if (IS_ERR(dev->clk)) { ret = -ENODEV;
Allow the left and right 16 bit samples to be shifted out as 1 32 bit sample. Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com> -- This applies to Kevin's temp/asoc branch --- arch/arm/mach-davinci/include/mach/asp.h | 6 ++ sound/soc/davinci/davinci-i2s.c | 74 ++++++++++++++++++++++-------- 2 files changed, 61 insertions(+), 19 deletions(-)