| Message ID | 1251761505-11353-2-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:44PM -0700, Troy Kisky wrote: >> Rename variable master_lch to asp_master_lch >> Rename variable slave_lch to asp_link_lch[0] >> Rename local variables: >> count to asp_count >> src to asp_src >> dst to asp_dst > > These other two patches are OK but I'll wait for after the merge window > to apply them partly due to the dependency on the merged tree and also > since it's very near to the merge window opening and there's already > been a fair bit of churn and testing with the DaVinci drivers for > 2.6.32. > > I'll probably also apply the first patch since nobody else seems to care > one way or another, but I would urge you to look at changing the default > for the platform data to at most select the workaround only on CPUs that > have problems with channel swapping - it's going to cause confusion for > people to have it on by default. > I think the ones without a problem use davinci-mcasp instead of davinci-i2s but share davinci-pcm. So, I don't know of any machines to exclude in davinci-i2s. But if someone else knows, speak up. Thanks Troy
On Thu, Sep 03, 2009 at 05:45:08, Troy Kisky wrote: > Mark Brown wrote: > > On Mon, Aug 31, 2009 at 04:31:44PM -0700, Troy Kisky wrote: [...] > > > > I'll probably also apply the first patch since nobody else seems to care > > one way or another, but I would urge you to look at changing the default > > for the platform data to at most select the workaround only on CPUs that > > have problems with channel swapping - it's going to cause confusion for > > people to have it on by default. > > > I think the ones without a problem use davinci-mcasp instead of davinci-i2s > but share davinci-pcm. So, I don't know of any machines to exclude in davinci-i2s. > But if someone else knows, speak up. > In my experience too, the channel swap issues got reported only with ASP (aka McBSP) and not with McASP. The swap was almost always at the start of playback, and supposedly because of the errata "2.1.5 ASP: Initialization Procedure When External Device is Frame-Sync Master" in http://focus.ti.com/lit/er/sprz241l/sprz241l.pdf Using EDMA acount=4 instead of 2 (32-bit transfers) did fix that issue on the OSS drivers but I don't recall the problem morphing into an "always channel swapped" case. Have you tested your patch (1/3) with DM644x EVM? If not, we can do that and see if it leads to channels being always swapped on that hardware as well. One feedback we have received on this solution is that it does not work for 24-bit audio. In which case, implementing the workaround described in the errata is the only way around. Thanks, Sekhar
Nori, Sekhar wrote: > On Thu, Sep 03, 2009 at 05:45:08, Troy Kisky wrote: >> Mark Brown wrote: >>> On Mon, Aug 31, 2009 at 04:31:44PM -0700, Troy Kisky wrote: > [...] >>> I'll probably also apply the first patch since nobody else seems to care >>> one way or another, but I would urge you to look at changing the default >>> for the platform data to at most select the workaround only on CPUs that >>> have problems with channel swapping - it's going to cause confusion for >>> people to have it on by default. >>> >> I think the ones without a problem use davinci-mcasp instead of davinci-i2s >> but share davinci-pcm. So, I don't know of any machines to exclude in davinci-i2s. >> But if someone else knows, speak up. >> > > In my experience too, the channel swap issues got reported only with ASP > (aka McBSP) and not with McASP. > > The swap was almost always at the start of playback, and supposedly because > of the errata "2.1.5 ASP: Initialization Procedure When External Device is > Frame-Sync Master" in http://focus.ti.com/lit/er/sprz241l/sprz241l.pdf This should have been fixed when I added davinci_i2s_prepare because it will call davinci_mcbsp_start if the codec is master, giving plenty of time for the first dma to be serviced. So, all that ugly code in davinci_mcbsp_start to "/* wait for any unexpected frame sync error to occur */" can probably be removed. But since I didn't know the reason for it, I haven't tried. If you can give this a try I'd like to know the results. > > Using EDMA acount=4 instead of 2 (32-bit transfers) did fix that issue on the > OSS drivers but I don't recall the problem morphing into an "always channel > swapped" case. > > Have you tested your patch (1/3) with DM644x EVM? If not, we can do that and > see if it leads to channels being always swapped on that hardware as well. Yes, I have tested with dm644x, not evm. I haven't tried to hear the channel swap, but I have no doubt that it is. > > One feedback we have received on this solution is that it does not work for > 24-bit audio. In which case, implementing the workaround described in the > errata is the only way around. Yes, you cannot shift more than 32 bits at once, so 48 bits is out. Although 24 bit format would be easy to add, currently only 8, 16, and 32 are supported by davinci-i2s. > > Thanks, > Sekhar > >
On Monday 31 August 2009, Troy Kisky wrote: > -Â Â Â Â Â Â Â int master_lch;Â Â Â Â Â Â Â Â Â /* Master DMA channel */ > -Â Â Â Â Â Â Â int slave_lch;Â Â Â Â Â Â Â Â Â Â /* linked parameter RAM reload slot */ > +Â Â Â Â Â Â Â int asp_master_lch;Â Â Â Â Â /* Master DMA channel */ > +Â Â Â Â Â Â Â int asp_link_lch[2];Â Â Â Â /* asp parameter link channel, ping/pong */ If you're going to rename things, may I suggest getting rid of "lch" ... use either "channel" or "link", since those are the two basic hardware roles in EDMA. The original "lch" ("logical channel") terminology was an IMO misguded attempt to hide the distinction between channels and links. But links can not be used when a channel is required, so that attempt was doomed to fail. (Channels could double as links, but as a rule that's not done since they're relatively scarce.) - Dave
David Brownell wrote: > On Monday 31 August 2009, Troy Kisky wrote: >> - int master_lch; /* Master DMA channel */ >> - int slave_lch; /* linked parameter RAM reload slot */ >> + int asp_master_lch; /* Master DMA channel */ >> + int asp_link_lch[2]; /* asp parameter link channel, ping/pong */ > > If you're going to rename things, may I suggest getting > rid of "lch" ... use either "channel" or "link", since > those are the two basic hardware roles in EDMA. > > The original "lch" ("logical channel") terminology was > an IMO misguded attempt to hide the distinction between > channels and links. But links can not be used when a > channel is required, so that attempt was doomed to fail. > (Channels could double as links, but as a rule that's > not done since they're relatively scarce.) > > - Dave > > > I agree, but can that be a separate patch...
On Thursday 03 September 2009, Troy Kisky wrote: > David Brownell wrote: > > On Monday 31 August 2009, Troy Kisky wrote: > >> - int master_lch; /* Master DMA channel */ > >> - int slave_lch; /* linked parameter RAM reload slot */ > >> + int asp_master_lch; /* Master DMA channel */ > >> + int asp_link_lch[2]; /* asp parameter link channel, ping/pong */ > > > > If you're going to rename things, may I suggest getting > > rid of "lch" ... use either "channel" or "link", since > > those are the two basic hardware roles in EDMA. > > I agree, but can that be a separate patch... However you like; I'd just suggest that *one* rename-things patch is normally preferred. If this one's already in the merge queue that might be awkward.
David Brownell wrote: > On Thursday 03 September 2009, Troy Kisky wrote: >> David Brownell wrote: >>> On Monday 31 August 2009, Troy Kisky wrote: >>>> - int master_lch; /* Master DMA channel */ >>>> - int slave_lch; /* linked parameter RAM reload slot */ >>>> + int asp_master_lch; /* Master DMA channel */ >>>> + int asp_link_lch[2]; /* asp parameter link channel, ping/pong */ >>> If you're going to rename things, may I suggest getting >>> rid of "lch" ... use either "channel" or "link", since >>> those are the two basic hardware roles in EDMA. >> I agree, but can that be a separate patch... > > However you like; I'd just suggest that *one* rename-things > patch is normally preferred. If this one's already in the > merge queue that might be awkward. > Kevin, Is the window too close, or is there time to change this? Troy
On Fri, Sep 04, 2009 at 00:25:57, Troy Kisky wrote: > Nori, Sekhar wrote: > > On Thu, Sep 03, 2009 at 05:45:08, Troy Kisky wrote: > >> Mark Brown wrote: > >>> On Mon, Aug 31, 2009 at 04:31:44PM -0700, Troy Kisky wrote: > > [...] > >>> I'll probably also apply the first patch since nobody else seems to care > >>> one way or another, but I would urge you to look at changing the default > >>> for the platform data to at most select the workaround only on CPUs that > >>> have problems with channel swapping - it's going to cause confusion for > >>> people to have it on by default. > >>> > >> I think the ones without a problem use davinci-mcasp instead of davinci-i2s > >> but share davinci-pcm. So, I don't know of any machines to exclude in davinci-i2s. > >> But if someone else knows, speak up. > >> [...] > > > > > Using EDMA acount=4 instead of 2 (32-bit transfers) did fix that issue on the > > OSS drivers but I don't recall the problem morphing into an "always channel > > swapped" case. > > > > Have you tested your patch (1/3) with DM644x EVM? If not, we can do that and > > see if it leads to channels being always swapped on that hardware as well. > > Yes, I have tested with dm644x, not evm. I haven't tried to hear the channel swap, > but I have no doubt that it is. I finally got around to testing your patch 1/3 on DM6446 EVM. Without your patch, channel swap is quite easy to reproduce using audio loopback: arecord -fcd | aplay -fcd The audio source is a PC which speaker balance set to an extreme. By starting and stopping this command repeatedly, you can see the audio moving from one channel to the other. Applying your patch fixes this issue. Also, I did not notice any permanent channel swap. Used aplay to play data which was first left-only and then right-only. Plays the same way on a Linux PC. I will test on couple of other platform using davinci-i2s (DM355 etc) before acking the patch. However, with or without your patch, I noticed a segmentation fault for the first time the 'arecord | aplay' loop is created. There is no fault the second time around. I haven't spent time debugging this yet. root@arago:~# arecord -fcd | aplay -fcd Recording WAVE 'stdin' : Signed 16 bit Little Endian, Rate 44100 Hz, Stereo Division by zero in kernel. Backtrace: [<c002b784>] (dump_backtrace+0x0/0x114) from [<c0248cf0>] (dump_stack+0x18/0x1c) r7:86a60000 r6:c77cdae0 r5:00000040 r4:00000000 [<c0248cd8>] (dump_stack+0x0/0x1c) from [<c002b8cc>] (__div0+0x18/0x20) [<c002b8b4>] (__div0+0x0/0x20) from [<c014b0fc>] (Ldiv0+0x8/0x10) [<c01bad64>] (davinci_pcm_enqueue_dma+0x0/0x110) from [<c01baea0>] (davinci_pcm_ prepare+0x2c/0x58) [<c01bae74>] (davinci_pcm_prepare+0x0/0x58) from [<c01b5260>] (soc_pcm_prepare+0 x84/0x184) r7:c030df38 r6:c030d1b8 r5:c70d3900 r4:00000000 [<c01b51dc>] (soc_pcm_prepare+0x0/0x184) from [<c01aac3c>] (snd_pcm_do_prepare+0 x1c/0x34) [<c01aac20>] (snd_pcm_do_prepare+0x0/0x34) from [<c01aa87c>] (snd_pcm_action_sin gle+0x40/0x7c) r5:c70d3900 r4:c030cbc8 [<c01aa83c>] (snd_pcm_action_single+0x0/0x7c) from [<c01abd80>] (snd_pcm_action_ nonatomic+0x58/0x70) r7:c70d3900 r6:00000002 r5:c030cbc8 r4:c70d3900 [<c01abd28>] (snd_pcm_action_nonatomic+0x0/0x70) from [<c01ae2cc>] (snd_pcm_comm on_ioctl1+0x814/0x1308) r7:c70d3900 r6:0002a690 r5:0002a690 r4:c77b6c80 [<c01adab8>] (snd_pcm_common_ioctl1+0x0/0x1308) from [<c01af20c>] (snd_pcm_captu re_ioctl1+0x44c/0x470) [<c01aedc0>] (snd_pcm_capture_ioctl1+0x0/0x470) from [<c01af268>] (snd_pcm_captu re_ioctl+0x38/0x3c) r7:c77b6c80 r6:00004140 r5:0002a690 r4:c77b6c80 [<c01af230>] (snd_pcm_capture_ioctl+0x0/0x3c) from [<c00a2680>] (vfs_ioctl+0x34/ 0x94) [<c00a264c>] (vfs_ioctl+0x0/0x94) from [<c00a2d44>] (do_vfs_ioctl+0x56c/0x5c8) r7:c77b6c80 r6:00000004 r5:c77b6c80 r4:c6b8c8a8 [<c00a27d8>] (do_vfs_ioctl+0x0/0x5c8) from [<c00a2de0>] (sys_ioctl+0x40/0x64) [<c00a2da0>] (sys_ioctl+0x0/0x64) from [<c0027ea0>] (ret_fast_syscall+0x0/0x2c) r7:00000036 r6:bec987d8 r5:0002a468 r4:000b567f Division by zero in kernel. Backtrace: [<c002b784>] (dump_backtrace+0x0/0x114) from [<c0248cf0>] (dump_stack+0x18/0x1c) r7:86a62000 r6:c77cdae0 r5:00000040 r4:00000000 [<c0248cd8>] (dump_stack+0x0/0x1c) from [<c002b8cc>] (__div0+0x18/0x20) [<c002b8b4>] (__div0+0x0/0x20) from [<c014b0fc>] (Ldiv0+0x8/0x10) [<c01bad64>] (davinci_pcm_enqueue_dma+0x0/0x110) from [<c01baec0>] (davinci_pcm_ prepare+0x4c/0x58) [<c01bae74>] (davinci_pcm_prepare+0x0/0x58) from [<c01b5260>] (soc_pcm_prepare+0 x84/0x184) r7:c030df38 r6:c030d1b8 r5:c70d3900 r4:00000000 [<c01b51dc>] (soc_pcm_prepare+0x0/0x184) from [<c01aac3c>] (snd_pcm_do_prepare+0 x1c/0x34) [<c01aac20>] (snd_pcm_do_prepare+0x0/0x34) from [<c01aa87c>] (snd_pcm_action_sin gle+0x40/0x7c) r5:c70d3900 r4:c030cbc8 [<c01aa83c>] (snd_pcm_action_single+0x0/0x7c) from [<c01abd80>] (snd_pcm_action_ nonatomic+0x58/0x70) r7:c70d3900 r6:00000002 r5:c030cbc8 r4:c70d3900 [<c01abd28>] (snd_pcm_action_nonatomic+0x0/0x70) from [<c01ae2cc>] (snd_pcm_comm on_ioctl1+0x814/0x1308) r7:c70d3900 r6:0002a690 r5:0002a690 r4:c77b6c80 [<c01adab8>] (snd_pcm_common_ioctl1+0x0/0x1308) from [<c01af20c>] (snd_pcm_captu re_ioctl1+0x44c/0x470) [<c01aedc0>] (snd_pcm_capture_ioctl1+0x0/0x470) from [<c01af268>] (snd_pcm_captu re_ioctl+0x38/0x3c) r7:c77b6c80 r6:00004140 r5:0002a690 r4:c77b6c80 [<c01af230>] (snd_pcm_capture_ioctl+0x0/0x3c) from [<c00a2680>] (vfs_ioctl+0x34/ 0x94) [<c00a264c>] (vfs_ioctl+0x0/0x94) from [<c00a2d44>] (do_vfs_ioctl+0x56c/0x5c8) r7:c77b6c80 r6:00000004 r5:c77b6c80 r4:c6b8c8a8 [<c00a27d8>] (do_vfs_ioctl+0x0/0x5c8) from [<c00a2de0>] (sys_ioctl+0x40/0x64) [<c00a2da0>] (sys_ioctl+0x0/0x64) from [<c0027ea0>] (ret_fast_syscall+0x0/0x2c) r7:00000036 r6:bec987d8 r5:0002a468 r4:000b567f Division by zero in kernel. Playing WAVE 'stBacktrace: din' : Signed 16 bit Little Endi[<c002b784>] an, Rate 44100 H(dump_backtrace+0x0/0x114) z, Stere o from [<c0248cf0>] (dump_stack+0x18/0x1c) r7:86a64000 r6:c77cdae0 r5:00000040 r4:00000000 [<c0248cd8>] (dump_stack+0x0/0x1c) from [<c002b8cc>] (__div0+0x18/0x20) [<c002b8b4>] (__div0+0x0/0x20) from [<c014b0fc>] (Ldiv0+0x8/0x10) [<c01bad64>] (davinci_pcm_enqueue_dma+0x0/0x110) from [<c01bb0bc>] (davinci_pcm_ dma_irq+0x58/0x80) [<c01bb064>] (davinci_pcm_dma_irq+0x0/0x80) from [<c0031904>] (dma_irq_handler+0 xec/0x12c) r5:00000003 r4:00000004 [<c0031818>] (dma_irq_handler+0x0/0x12c) from [<c0068ee8>] (handle_IRQ_event+0x4 4/0x114) [<c0068ea4>] (handle_IRQ_event+0x0/0x114) from [<c006ab74>] (handle_edge_irq+0x1 48/0x1b4) r7:c7007440 r6:00000010 r5:c02ff060 r4:c6baa000 [<c006aa2c>] (handle_edge_irq+0x0/0x1b4) from [<c0027070>] (_text+0x70/0x8c) r7:00000002 r6:00000800 r5:00000000 r4:00000010 [<c0027000>] (_text+0x0/0x8c) from [<c0027aac>] (__irq_svc+0x4c/0x90) Exception stack(0xc6babdf0 to 0xc6babe38) bde0: 00000001 00000000 c6baa000 40000013 be00: 00000800 c76f2800 00000800 c70d3900 00000000 00000800 00000000 c6babe8c be20: c6baa000 c6babe38 c01b16f4 c01b1714 40000013 ffffffff r5:fec48000 r4:ffffffff [<c01b1548>] (snd_pcm_lib_read1+0x0/0x30c) from [<c01b1934>] (snd_pcm_lib_read+0 x60/0x6c) [<c01b18d4>] (snd_pcm_lib_read+0x0/0x6c) from [<c01aee8c>] (snd_pcm_capture_ioct l1+0xcc/0x470) r6:bec98ab4 r5:bec98ab4 r4:00000000 [<c01aedc0>] (snd_pcm_capture_ioctl1+0x0/0x470) from [<c01af268>] (snd_pcm_captu re_ioctl+0x38/0x3c) r7:c77b6c80 r6:800c4151 r5:bec98ab4 r4:c77b6c80 [<c01af230>] (snd_pcm_capture_ioctl+0x0/0x3c) from [<c00a2680>] (vfs_ioctl+0x34/ 0x94) [<c00a264c>] (vfs_ioctl+0x0/0x94) from [<c00a2d44>] (do_vfs_ioctl+0x56c/0x5c8) r7:c77b6c80 r6:00000004 r5:c77b6c80 r4:c6b8c8a8 [<c00a27d8>] (do_vfs_ioctl+0x0/0x5c8) from [<c00a2de0>] (sys_ioctl+0x40/0x64) [<c00a2da0>] (sys_ioctl+0x0/0x64) from [<c0027ea0>] (ret_fast_syscall+0x0/0x2c) r7:00000036 r6:00000000 r5:0002a4b8 r4:0002a468 arecord: pcm_read:1529: read error: Input/output error root@arago:~# Thanks, Sekhar > > > > > One feedback we have received on this solution is that it does not work for > > 24-bit audio. In which case, implementing the workaround described in the > > errata is the only way around. > > Yes, you cannot shift more than 32 bits at once, so 48 bits is out. > Although 24 bit format would be easy to add, currently only 8, 16, and 32 > are supported by davinci-i2s. > > > > > Thanks, > > Sekhar > > > > > > >
On Wednesday 09 September 2009 15:08:42 Nori, Sekhar wrote: > On Fri, Sep 04, 2009 at 00:25:57, Troy Kisky wrote: > > Nori, Sekhar wrote: > > > On Thu, Sep 03, 2009 at 05:45:08, Troy Kisky wrote: > > >> Mark Brown wrote: > > >>> On Mon, Aug 31, 2009 at 04:31:44PM -0700, Troy Kisky wrote: > > > > > > [...] > > > > > >>> I'll probably also apply the first patch since nobody else seems to > > >>> care one way or another, but I would urge you to look at changing the > > >>> default for the platform data to at most select the workaround only > > >>> on CPUs that have problems with channel swapping - it's going to > > >>> cause confusion for people to have it on by default. > > >> > > >> I think the ones without a problem use davinci-mcasp instead of > > >> davinci-i2s but share davinci-pcm. So, I don't know of any machines to > > >> exclude in davinci-i2s. But if someone else knows, speak up. > > [...] > > > > Using EDMA acount=4 instead of 2 (32-bit transfers) did fix that issue > > > on the OSS drivers but I don't recall the problem morphing into an > > > "always channel swapped" case. > > > > > > Have you tested your patch (1/3) with DM644x EVM? If not, we can do > > > that and see if it leads to channels being always swapped on that > > > hardware as well. > > > > Yes, I have tested with dm644x, not evm. I haven't tried to hear the > > channel swap, but I have no doubt that it is. > > I finally got around to testing your patch 1/3 on DM6446 EVM. > > Without your patch, channel swap is quite easy to reproduce using audio > loopback: > > arecord -fcd | aplay -fcd > > The audio source is a PC which speaker balance set to an extreme. > By starting and stopping this command repeatedly, you can see the audio > moving from one channel to the other. > > Applying your patch fixes this issue. > > Also, I did not notice any permanent channel swap. Used aplay to play data > which was first left-only and then right-only. Plays the same way on a > Linux PC. > > I will test on couple of other platform using davinci-i2s (DM355 etc) > before acking the patch. > > However, with or without your patch, I noticed a segmentation fault for the > first time the 'arecord | aplay' loop is created. There is no fault the > second time around. I haven't spent time debugging this yet. > This is due to these lines in davinci_pcm_enqueue_dma: data_type = prtd->params->data_type; count = period_size / data_type; First time running data_type is set to 0. This is a long time issue, I remember modifying my kernels to check for zero before this division.. Regards, Caglar > root@arago:~# arecord -fcd | aplay -fcd > Recording WAVE 'stdin' : Signed 16 bit Little Endian, Rate 44100 Hz, Stereo > Division by zero in kernel. > Backtrace: > [<c002b784>] (dump_backtrace+0x0/0x114) from [<c0248cf0>] > (dump_stack+0x18/0x1c) r7:86a60000 r6:c77cdae0 r5:00000040 r4:00000000 > [<c0248cd8>] (dump_stack+0x0/0x1c) from [<c002b8cc>] (__div0+0x18/0x20) > [<c002b8b4>] (__div0+0x0/0x20) from [<c014b0fc>] (Ldiv0+0x8/0x10) > [<c01bad64>] (davinci_pcm_enqueue_dma+0x0/0x110) from [<c01baea0>] > (davinci_pcm_ prepare+0x2c/0x58) > [<c01bae74>] (davinci_pcm_prepare+0x0/0x58) from [<c01b5260>] > (soc_pcm_prepare+0 x84/0x184) > r7:c030df38 r6:c030d1b8 r5:c70d3900 r4:00000000 > [<c01b51dc>] (soc_pcm_prepare+0x0/0x184) from [<c01aac3c>] > (snd_pcm_do_prepare+0 x1c/0x34) > [<c01aac20>] (snd_pcm_do_prepare+0x0/0x34) from [<c01aa87c>] > (snd_pcm_action_sin gle+0x40/0x7c) > r5:c70d3900 r4:c030cbc8 > [<c01aa83c>] (snd_pcm_action_single+0x0/0x7c) from [<c01abd80>] > (snd_pcm_action_ nonatomic+0x58/0x70) > r7:c70d3900 r6:00000002 r5:c030cbc8 r4:c70d3900 > [<c01abd28>] (snd_pcm_action_nonatomic+0x0/0x70) from [<c01ae2cc>] > (snd_pcm_comm on_ioctl1+0x814/0x1308) > r7:c70d3900 r6:0002a690 r5:0002a690 r4:c77b6c80 > [<c01adab8>] (snd_pcm_common_ioctl1+0x0/0x1308) from [<c01af20c>] > (snd_pcm_captu re_ioctl1+0x44c/0x470) > [<c01aedc0>] (snd_pcm_capture_ioctl1+0x0/0x470) from [<c01af268>] > (snd_pcm_captu re_ioctl+0x38/0x3c) > r7:c77b6c80 r6:00004140 r5:0002a690 r4:c77b6c80 > [<c01af230>] (snd_pcm_capture_ioctl+0x0/0x3c) from [<c00a2680>] > (vfs_ioctl+0x34/ 0x94) > [<c00a264c>] (vfs_ioctl+0x0/0x94) from [<c00a2d44>] > (do_vfs_ioctl+0x56c/0x5c8) r7:c77b6c80 r6:00000004 r5:c77b6c80 r4:c6b8c8a8 > [<c00a27d8>] (do_vfs_ioctl+0x0/0x5c8) from [<c00a2de0>] > (sys_ioctl+0x40/0x64) [<c00a2da0>] (sys_ioctl+0x0/0x64) from [<c0027ea0>] > (ret_fast_syscall+0x0/0x2c) r7:00000036 r6:bec987d8 r5:0002a468 r4:000b567f > Division by zero in kernel. > Backtrace: > [<c002b784>] (dump_backtrace+0x0/0x114) from [<c0248cf0>] > (dump_stack+0x18/0x1c) r7:86a62000 r6:c77cdae0 r5:00000040 r4:00000000 > [<c0248cd8>] (dump_stack+0x0/0x1c) from [<c002b8cc>] (__div0+0x18/0x20) > [<c002b8b4>] (__div0+0x0/0x20) from [<c014b0fc>] (Ldiv0+0x8/0x10) > [<c01bad64>] (davinci_pcm_enqueue_dma+0x0/0x110) from [<c01baec0>] > (davinci_pcm_ prepare+0x4c/0x58) > [<c01bae74>] (davinci_pcm_prepare+0x0/0x58) from [<c01b5260>] > (soc_pcm_prepare+0 x84/0x184) > r7:c030df38 r6:c030d1b8 r5:c70d3900 r4:00000000 > [<c01b51dc>] (soc_pcm_prepare+0x0/0x184) from [<c01aac3c>] > (snd_pcm_do_prepare+0 x1c/0x34) > [<c01aac20>] (snd_pcm_do_prepare+0x0/0x34) from [<c01aa87c>] > (snd_pcm_action_sin gle+0x40/0x7c) > r5:c70d3900 r4:c030cbc8 > [<c01aa83c>] (snd_pcm_action_single+0x0/0x7c) from [<c01abd80>] > (snd_pcm_action_ nonatomic+0x58/0x70) > r7:c70d3900 r6:00000002 r5:c030cbc8 r4:c70d3900 > [<c01abd28>] (snd_pcm_action_nonatomic+0x0/0x70) from [<c01ae2cc>] > (snd_pcm_comm on_ioctl1+0x814/0x1308) > r7:c70d3900 r6:0002a690 r5:0002a690 r4:c77b6c80 > [<c01adab8>] (snd_pcm_common_ioctl1+0x0/0x1308) from [<c01af20c>] > (snd_pcm_captu re_ioctl1+0x44c/0x470) > [<c01aedc0>] (snd_pcm_capture_ioctl1+0x0/0x470) from [<c01af268>] > (snd_pcm_captu re_ioctl+0x38/0x3c) > r7:c77b6c80 r6:00004140 r5:0002a690 r4:c77b6c80 > [<c01af230>] (snd_pcm_capture_ioctl+0x0/0x3c) from [<c00a2680>] > (vfs_ioctl+0x34/ 0x94) > [<c00a264c>] (vfs_ioctl+0x0/0x94) from [<c00a2d44>] > (do_vfs_ioctl+0x56c/0x5c8) r7:c77b6c80 r6:00000004 r5:c77b6c80 r4:c6b8c8a8 > [<c00a27d8>] (do_vfs_ioctl+0x0/0x5c8) from [<c00a2de0>] > (sys_ioctl+0x40/0x64) [<c00a2da0>] (sys_ioctl+0x0/0x64) from [<c0027ea0>] > (ret_fast_syscall+0x0/0x2c) r7:00000036 r6:bec987d8 r5:0002a468 r4:000b567f > Division by zero in kernel. > Playing WAVE 'stBacktrace: din' : Signed 16 > bit Little Endi[<c002b784>] an, Rate 44100 H(dump_backtrace+0x0/0x114) z, > Stere o > from [<c0248cf0>] (dump_stack+0x18/0x1c) > r7:86a64000 r6:c77cdae0 r5:00000040 r4:00000000 > [<c0248cd8>] (dump_stack+0x0/0x1c) from [<c002b8cc>] (__div0+0x18/0x20) > [<c002b8b4>] (__div0+0x0/0x20) from [<c014b0fc>] (Ldiv0+0x8/0x10) > [<c01bad64>] (davinci_pcm_enqueue_dma+0x0/0x110) from [<c01bb0bc>] > (davinci_pcm_ dma_irq+0x58/0x80) > [<c01bb064>] (davinci_pcm_dma_irq+0x0/0x80) from [<c0031904>] > (dma_irq_handler+0 xec/0x12c) > r5:00000003 r4:00000004 > [<c0031818>] (dma_irq_handler+0x0/0x12c) from [<c0068ee8>] > (handle_IRQ_event+0x4 4/0x114) > [<c0068ea4>] (handle_IRQ_event+0x0/0x114) from [<c006ab74>] > (handle_edge_irq+0x1 48/0x1b4) > r7:c7007440 r6:00000010 r5:c02ff060 r4:c6baa000 > [<c006aa2c>] (handle_edge_irq+0x0/0x1b4) from [<c0027070>] > (_text+0x70/0x8c) r7:00000002 r6:00000800 r5:00000000 r4:00000010 > [<c0027000>] (_text+0x0/0x8c) from [<c0027aac>] (__irq_svc+0x4c/0x90) > Exception stack(0xc6babdf0 to 0xc6babe38) > bde0: 00000001 00000000 c6baa000 > 40000013 be00: 00000800 c76f2800 00000800 c70d3900 00000000 00000800 > 00000000 c6babe8c be20: c6baa000 c6babe38 c01b16f4 c01b1714 40000013 > ffffffff > r5:fec48000 r4:ffffffff > [<c01b1548>] (snd_pcm_lib_read1+0x0/0x30c) from [<c01b1934>] > (snd_pcm_lib_read+0 x60/0x6c) > [<c01b18d4>] (snd_pcm_lib_read+0x0/0x6c) from [<c01aee8c>] > (snd_pcm_capture_ioct l1+0xcc/0x470) > r6:bec98ab4 r5:bec98ab4 r4:00000000 > [<c01aedc0>] (snd_pcm_capture_ioctl1+0x0/0x470) from [<c01af268>] > (snd_pcm_captu re_ioctl+0x38/0x3c) > r7:c77b6c80 r6:800c4151 r5:bec98ab4 r4:c77b6c80 > [<c01af230>] (snd_pcm_capture_ioctl+0x0/0x3c) from [<c00a2680>] > (vfs_ioctl+0x34/ 0x94) > [<c00a264c>] (vfs_ioctl+0x0/0x94) from [<c00a2d44>] > (do_vfs_ioctl+0x56c/0x5c8) r7:c77b6c80 r6:00000004 r5:c77b6c80 r4:c6b8c8a8 > [<c00a27d8>] (do_vfs_ioctl+0x0/0x5c8) from [<c00a2de0>] > (sys_ioctl+0x40/0x64) [<c00a2da0>] (sys_ioctl+0x0/0x64) from [<c0027ea0>] > (ret_fast_syscall+0x0/0x2c) r7:00000036 r6:00000000 r5:0002a4b8 r4:0002a468 > arecord: pcm_read:1529: read error: Input/output error > root@arago:~# > > Thanks, > Sekhar > > > > One feedback we have received on this solution is that it does not work > > > for 24-bit audio. In which case, implementing the workaround described > > > in the errata is the only way around. > > > > Yes, you cannot shift more than 32 bits at once, so 48 bits is out. > > Although 24 bit format would be easy to add, currently only 8, 16, and 32 > > are supported by davinci-i2s. > > > > > Thanks, > > > Sekhar > > _______________________________________________ > Davinci-linux-open-source mailing list > Davinci-linux-open-source@linux.davincidsp.com > http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
On Wed, Sep 09, 2009 at 18:03:58, Caglar Akyuz wrote: > On Wednesday 09 September 2009 15:08:42 Nori, Sekhar wrote: [...] > > > > However, with or without your patch, I noticed a segmentation fault for the > > first time the 'arecord | aplay' loop is created. There is no fault the > > second time around. I haven't spent time debugging this yet. > > > > This is due to these lines in davinci_pcm_enqueue_dma: > > data_type = prtd->params->data_type; > count = period_size / data_type; > > First time running data_type is set to 0. This is a long time issue, I > remember modifying my kernels to check for zero before this division.. Ah, thanks! Planning to submit a fix for this? We can work on the patch if you are busy otherwise. Regards, Sekhar
> -----Original Message----- > From: davinci-linux-open-source-bounces@linux.davincidsp.com > [mailto:davinci-linux-open-source-bounces@linux.davincidsp.com] On Behalf > Of Nori, Sekhar > Sent: Wednesday, September 09, 2009 9:06 AM > To: caglarakyuz@gmail.com; davinci-linux-open-source@linux.davincidsp.com > Cc: alsa-devel@alsa-project.org; Mark Brown > Subject: RE: [alsa-devel] [PATCH 2/3] ASoC: DaVinci: pcm, rename variables > in prep for ping/pong > > On Wed, Sep 09, 2009 at 18:03:58, Caglar Akyuz wrote: > > On Wednesday 09 September 2009 15:08:42 Nori, Sekhar wrote: > > [...] > > > > > > > However, with or without your patch, I noticed a segmentation fault > for the > > > first time the 'arecord | aplay' loop is created. There is no fault > the > > > second time around. I haven't spent time debugging this yet. > > > > > > > This is due to these lines in davinci_pcm_enqueue_dma: > > > > data_type = prtd->params->data_type; > > count = period_size / data_type; > > > > First time running data_type is set to 0. This is a long time issue, I > > remember modifying my kernels to check for zero before this division.. > > Ah, thanks! Planning to submit a fix for this? We can work on the patch > if you are busy otherwise. There was a patch from Arun Mani on Arago linux-davinci staging tree - http://arago-project.org/git/projects/?p=linux-davinci.git;a=commitdiff;h=0025e7d8024e9b03bada7a1d65f1aa7b7dfec061 Thanks Sneha > > Regards, > Sekhar > _______________________________________________ > Davinci-linux-open-source mailing list > Davinci-linux-open-source@linux.davincidsp.com > http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
On Wed, Sep 09, 2009 at 17:38:42, Nori, Sekhar wrote: > On Fri, Sep 04, 2009 at 00:25:57, Troy Kisky wrote: > > Nori, Sekhar wrote: > > > On Thu, Sep 03, 2009 at 05:45:08, Troy Kisky wrote: > > >> Mark Brown wrote: > > >>> On Mon, Aug 31, 2009 at 04:31:44PM -0700, Troy Kisky wrote: > > > [...] > > >>> I'll probably also apply the first patch since nobody else seems to care > > >>> one way or another, but I would urge you to look at changing the default > > >>> for the platform data to at most select the workaround only on CPUs that > > >>> have problems with channel swapping - it's going to cause confusion for > > >>> people to have it on by default. > > >>> > > >> I think the ones without a problem use davinci-mcasp instead of davinci-i2s > > >> but share davinci-pcm. So, I don't know of any machines to exclude in davinci-i2s. > > >> But if someone else knows, speak up. > > >> > > [...] > > > > > > > > > Using EDMA acount=4 instead of 2 (32-bit transfers) did fix that issue on the > > > OSS drivers but I don't recall the problem morphing into an "always channel > > > swapped" case. > > > > > > Have you tested your patch (1/3) with DM644x EVM? If not, we can do that and > > > see if it leads to channels being always swapped on that hardware as well. > > > > Yes, I have tested with dm644x, not evm. I haven't tried to hear the channel swap, > > but I have no doubt that it is. > > I finally got around to testing your patch 1/3 on DM6446 EVM. > > Without your patch, channel swap is quite easy to reproduce using audio > loopback: > > arecord -fcd | aplay -fcd > > The audio source is a PC which speaker balance set to an extreme. > By starting and stopping this command repeatedly, you can see the audio > moving from one channel to the other. > > Applying your patch fixes this issue. > > Also, I did not notice any permanent channel swap. Used aplay to play data > which was first left-only and then right-only. Plays the same way on a Linux PC. > > I will test on couple of other platform using davinci-i2s (DM355 etc) before > acking the patch. Even on the DM355 EVM this patch fixes the random channel swap on audio loopback 'arecord -fcd | aplay -fcd'. However, on playback, the channels do seem to be permanently swapped. This cannot surely be blamed on this patch because, without it, the channels get randomly swapped. Since this was not observed on DM6446 EVM, I have to see if the DM355 EVM hardware swaps the channels. I briefly compared the schematics of the two EVMs, but nothing seems to be wrong there. Troy, do you have any theory yet on why your patch should permanently swap channels? Anyway, the patch surely improves the situation on the EVMs, so, for patch 1/3 of this series: Tested-by: Sekhar Nori <nsekhar@ti.com> [tested on DM6446 and DM355 EVMs] Thanks, Sekhar
Hi Sekhar, DM355 uses MCBSP 1 instead of 0 as in DM644x. Do you think this will affect the channel swapping? Another thing is I was able to run aplay and arecord without Troy's patches, but seems like gstreamer still fails with his patches. Thanks, Arun > -----Original Message----- > From: davinci-linux-open-source-bounces+avm=ti.com@linux.davincidsp.com > [mailto:davinci-linux-open-source-bounces+avm=ti.com@linux.davincidsp.com] > On Behalf Of Nori, Sekhar > Sent: Tuesday, September 29, 2009 6:46 AM > To: Nori, Sekhar; Troy Kisky > Cc: davinci-linux-open-source@linux.davincidsp.com; Mark Brown; alsa- > devel@alsa-project.org > Subject: RE: [alsa-devel] [PATCH 2/3] ASoC: DaVinci: pcm, rename variables > in prep for ping/pong > > On Wed, Sep 09, 2009 at 17:38:42, Nori, Sekhar wrote: > > On Fri, Sep 04, 2009 at 00:25:57, Troy Kisky wrote: > > > Nori, Sekhar wrote: > > > > On Thu, Sep 03, 2009 at 05:45:08, Troy Kisky wrote: > > > >> Mark Brown wrote: > > > >>> On Mon, Aug 31, 2009 at 04:31:44PM -0700, Troy Kisky wrote: > > > > [...] > > > >>> I'll probably also apply the first patch since nobody else seems > to care > > > >>> one way or another, but I would urge you to look at changing the > default > > > >>> for the platform data to at most select the workaround only on > CPUs that > > > >>> have problems with channel swapping - it's going to cause > confusion for > > > >>> people to have it on by default. > > > >>> > > > >> I think the ones without a problem use davinci-mcasp instead of > davinci-i2s > > > >> but share davinci-pcm. So, I don't know of any machines to exclude > in davinci-i2s. > > > >> But if someone else knows, speak up. > > > >> > > > > [...] > > > > > > > > > > > > > Using EDMA acount=4 instead of 2 (32-bit transfers) did fix that > issue on the > > > > OSS drivers but I don't recall the problem morphing into an "always > channel > > > > swapped" case. > > > > > > > > Have you tested your patch (1/3) with DM644x EVM? If not, we can do > that and > > > > see if it leads to channels being always swapped on that hardware as > well. > > > > > > Yes, I have tested with dm644x, not evm. I haven't tried to hear the > channel swap, > > > but I have no doubt that it is. > > > > I finally got around to testing your patch 1/3 on DM6446 EVM. > > > > Without your patch, channel swap is quite easy to reproduce using audio > > loopback: > > > > arecord -fcd | aplay -fcd > > > > The audio source is a PC which speaker balance set to an extreme. > > By starting and stopping this command repeatedly, you can see the audio > > moving from one channel to the other. > > > > Applying your patch fixes this issue. > > > > Also, I did not notice any permanent channel swap. Used aplay to play > data > > which was first left-only and then right-only. Plays the same way on a > Linux PC. > > > > I will test on couple of other platform using davinci-i2s (DM355 etc) > before > > acking the patch. > > Even on the DM355 EVM this patch fixes the random channel swap on audio > loopback 'arecord -fcd | aplay -fcd'. > > However, on playback, the channels do seem to be permanently swapped. This > cannot > surely be blamed on this patch because, without it, the channels get > randomly > swapped. > > Since this was not observed on DM6446 EVM, I have to see if the DM355 EVM > hardware swaps the channels. I briefly compared the schematics of the two > EVMs, > but nothing seems to be wrong there. > > Troy, do you have any theory yet on why your patch should permanently swap > channels? > > Anyway, the patch surely improves the situation on the EVMs, so, for patch > 1/3 > of this series: > > Tested-by: Sekhar Nori <nsekhar@ti.com> > [tested on DM6446 and DM355 EVMs] > > Thanks, > Sekhar > _______________________________________________ > Davinci-linux-open-source mailing list > Davinci-linux-open-source@linux.davincidsp.com > http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
diff --git a/sound/soc/davinci/davinci-pcm.c b/sound/soc/davinci/davinci-pcm.c index 2f7da49..a96655c 100644 --- a/sound/soc/davinci/davinci-pcm.c +++ b/sound/soc/davinci/davinci-pcm.c @@ -51,8 +51,8 @@ static struct snd_pcm_hardware davinci_pcm_hardware = { struct davinci_runtime_data { spinlock_t lock; int period; /* current DMA period */ - int master_lch; /* Master DMA channel */ - int slave_lch; /* linked parameter RAM reload slot */ + int asp_master_lch; /* Master DMA channel */ + int asp_link_lch[2]; /* asp parameter link channel, ping/pong */ struct davinci_pcm_dma_params *params; /* DMA params */ }; @@ -60,7 +60,7 @@ static void davinci_pcm_enqueue_dma(struct snd_pcm_substream *substream) { struct davinci_runtime_data *prtd = substream->runtime->private_data; struct snd_pcm_runtime *runtime = substream->runtime; - int lch = prtd->slave_lch; + int lch = prtd->asp_link_lch[0]; unsigned int period_size; unsigned int dma_offset; dma_addr_t dma_pos; @@ -142,15 +142,15 @@ static int davinci_pcm_dma_request(struct snd_pcm_substream *substream) EVENTQ_0); if (ret < 0) return ret; - prtd->master_lch = ret; + prtd->asp_master_lch = ret; /* Request parameter RAM reload slot */ - ret = edma_alloc_slot(EDMA_CTLR(prtd->master_lch), EDMA_SLOT_ANY); + ret = edma_alloc_slot(EDMA_CTLR(prtd->asp_master_lch), EDMA_SLOT_ANY); if (ret < 0) { - edma_free_channel(prtd->master_lch); + edma_free_channel(prtd->asp_master_lch); return ret; } - prtd->slave_lch = ret; + prtd->asp_link_lch[0] = ret; /* Issue transfer completion IRQ when the channel completes a * transfer, then always reload from the same slot (by a kind @@ -161,10 +161,10 @@ static int davinci_pcm_dma_request(struct snd_pcm_substream *substream) * the buffer and its length (ccnt) ... use it as a template * so davinci_pcm_enqueue_dma() takes less time in IRQ. */ - edma_read_slot(prtd->slave_lch, &p_ram); - p_ram.opt |= TCINTEN | EDMA_TCC(EDMA_CHAN_SLOT(prtd->master_lch)); - p_ram.link_bcntrld = EDMA_CHAN_SLOT(prtd->slave_lch) << 5; - edma_write_slot(prtd->slave_lch, &p_ram); + edma_read_slot(prtd->asp_link_lch[0], &p_ram); + p_ram.opt |= TCINTEN | EDMA_TCC(EDMA_CHAN_SLOT(prtd->asp_master_lch)); + p_ram.link_bcntrld = EDMA_CHAN_SLOT(prtd->asp_link_lch[0]) << 5; + edma_write_slot(prtd->asp_link_lch[0], &p_ram); return 0; } @@ -180,12 +180,12 @@ static int davinci_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: - edma_start(prtd->master_lch); + edma_start(prtd->asp_master_lch); break; case SNDRV_PCM_TRIGGER_STOP: case SNDRV_PCM_TRIGGER_SUSPEND: case SNDRV_PCM_TRIGGER_PAUSE_PUSH: - edma_stop(prtd->master_lch); + edma_stop(prtd->asp_master_lch); break; default: ret = -EINVAL; @@ -206,8 +206,8 @@ static int davinci_pcm_prepare(struct snd_pcm_substream *substream) davinci_pcm_enqueue_dma(substream); /* Copy self-linked parameter RAM entry into master channel */ - edma_read_slot(prtd->slave_lch, &temp); - edma_write_slot(prtd->master_lch, &temp); + edma_read_slot(prtd->asp_link_lch[0], &temp); + edma_write_slot(prtd->asp_master_lch, &temp); davinci_pcm_enqueue_dma(substream); return 0; @@ -219,20 +219,20 @@ 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; - dma_addr_t count; - dma_addr_t src, dst; + int asp_count; + dma_addr_t asp_src, asp_dst; spin_lock(&prtd->lock); - edma_get_position(prtd->master_lch, &src, &dst); + edma_get_position(prtd->asp_master_lch, &asp_src, &asp_dst); if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) - count = src - runtime->dma_addr; + asp_count = asp_src - runtime->dma_addr; else - count = dst - runtime->dma_addr; + asp_count = asp_dst - runtime->dma_addr; spin_unlock(&prtd->lock); - offset = bytes_to_frames(runtime, count); + offset = bytes_to_frames(runtime, asp_count); if (offset >= runtime->buffer_size) offset = 0; @@ -274,10 +274,10 @@ static int davinci_pcm_close(struct snd_pcm_substream *substream) struct snd_pcm_runtime *runtime = substream->runtime; struct davinci_runtime_data *prtd = runtime->private_data; - edma_unlink(prtd->slave_lch); + edma_unlink(prtd->asp_link_lch[0]); - edma_free_slot(prtd->slave_lch); - edma_free_channel(prtd->master_lch); + edma_free_slot(prtd->asp_link_lch[0]); + edma_free_channel(prtd->asp_master_lch); kfree(prtd);
Rename variable master_lch to asp_master_lch Rename variable slave_lch to asp_link_lch[0] Rename local variables: count to asp_count src to asp_src dst to asp_dst Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com> --- sound/soc/davinci/davinci-pcm.c | 48 +++++++++++++++++++------------------- 1 files changed, 24 insertions(+), 24 deletions(-)