[alsa-devel] davici-mcasp: "tx-num-evt" confusion with number of serializers
Hi, I'm working on multichannel version of davinci-mcasp and also davinci-pcm. I have first version running and now I want to refine code. I found one confusion in davinci-mcasp with using of DT property "tx-num-evt". In DT binding documentation "tx-num-evt" is defined as "FIFO levels", but in Mcasp src, there is code, which mixes tx-num-evt with number of serializers (i2s data lines) that are enabled for data playback and receive (dev->txnumevt * tx_ser)
mcasp_mod_bits(dev->base + MCASP_VER3_WFIFOCTL, (((dev->txnumevt * tx_ser) << 8), NUMEVT_MASK);
From dacinci pcm, DMA data tranfer use txnumevt as number of serializers and
also for data prefetching. I undestand definition "FIFO levels" as how many prefetched data are in FIFO. Prefetched data are for me frame data / 2 ( all left or right channels for one sampling time). This results me from AM335x FIFO documantation. What is original purpose for "tx-num-evt" parameter?
Thanks, Michal
On 02/26/2013 03:06 PM, Michal Bachraty wrote:
Hi, I'm working on multichannel version of davinci-mcasp and also davinci-pcm. I have first version running and now I want to refine code. I found one confusion in davinci-mcasp with using of DT property "tx-num-evt". In DT binding documentation "tx-num-evt" is defined as "FIFO levels", but in Mcasp src, there is code, which mixes tx-num-evt with number of serializers (i2s data lines) that are enabled for data playback and receive (dev->txnumevt * tx_ser)
mcasp_mod_bits(dev->base + MCASP_VER3_WFIFOCTL, (((dev->txnumevt * tx_ser) << 8), NUMEVT_MASK);
From dacinci pcm, DMA data tranfer use txnumevt as number of serializers and also for data prefetching. I undestand definition "FIFO levels" as how many prefetched data are in FIFO. Prefetched data are for me frame data / 2 ( all left or right channels for one sampling time). This results me from AM335x FIFO documantation. What is original purpose for "tx-num-evt" parameter?
From what I remember (I'm using the davinci to record from up to 8 codecs chips simultaneously) the value sets a threshold for the DMA request (if the FIFO is enabled - and I can't think of a reason why anyone would NOT want to enable the fifo...).
Postponing the DMA request until there are #channels data entries in the fifo buffer makes sense to me. Setting the txnumevt to a higher value might reduce the load on the memory controller (that's what TI claims), at the cost of a higher risk of overrunning the fifo, and increased latency. Setting it to less than the number of channels isn't useful either - which application would be interested in "half" the channel data?
I have always considered the parameter to be a boolean (please use the fifo) rather than an integer setting.
Hi Mike,
Thanks for your opinion.
From what I remember (I'm using the davinci to record from up to 8 codecs chips simultaneously) the value sets a threshold for the DMA request (if the FIFO is enabled - and I can't think of a reason why anyone would NOT want to enable the fifo...).
Can you tell me, which processor are you using, how many serializers (data lines) do you use and which mode are you using (TDM - I2S?)?
Postponing the DMA request until there are #channels data entries in the fifo buffer makes sense to me. Setting the txnumevt to a higher value might reduce the load on the memory controller (that's what TI claims), at the cost of a higher risk of overrunning the fifo, and increased latency. Setting it to less than the number of channels isn't useful either - which application would be interested in "half" the channel data?
AM335x FIFO waits for data for all enabled serializers. For I2S format, there are two (L/R) data samples for one sampling time. This data are sended one after another to dataline. Therefore Am335x serializer need only "half"sample data, but it works in double sampling time frequency. When more than two channels will be used, for example four, then two serializers are enabled. For this example, FIFO need to be filled with two samples in one DMA frame, each for one serializer. If not, no DMA event is generated.
Michal.
On 02/27/2013 12:36 PM, Michal Bachraty wrote:
Hi Mike,
Thanks for your opinion.
From what I remember (I'm using the davinci to record from up to 8 codecs chips simultaneously) the value sets a threshold for the DMA request (if the FIFO is enabled - and I can't think of a reason why anyone would NOT want to enable the fifo...).
Can you tell me, which processor are you using, how many serializers (data lines) do you use and which mode are you using (TDM - I2S?)?
I'm using an OMAP-L138. Using (up to) 8 serializers (8 AXR lines to 8 codecs of 3 different types). I'm using the I2S protocol for the serial lines.
I patched the McASP driver and simply put my settings in there, because i'm on an old kernel (2.6.37).
I mapped the tdm mask function of the interface to actually enable/disable the serializers, and created a fake "multiplex" codec that exports switches to enable/disable the actual codecs and propagates the stream operations to the codecs that are enabled. This way, user space can set everything up via the mixer interface, and can then record from any subset of the 8 codecs.
Postponing the DMA request until there are #channels data entries in the fifo buffer makes sense to me. Setting the txnumevt to a higher value might reduce the load on the memory controller (that's what TI claims), at the cost of a higher risk of overrunning the fifo, and increased latency. Setting it to less than the number of channels isn't useful either - which application would be interested in "half" the channel data?
AM335x FIFO waits for data for all enabled serializers. For I2S format, there are two (L/R) data samples for one sampling time. This data are sended one after another to dataline. Therefore Am335x serializer need only "half"sample data, but it works in double sampling time frequency. When more than two channels will be used, for example four, then two serializers are enabled. For this example, FIFO need to be filled with two samples in one DMA frame, each for one serializer. If not, no DMA event is generated.
True. I forgot about the "stereo" part, the current code sets the FIFO to request DMA transfer when one half (L or R) of the data is complete.
If you have only few channels in use, it might improve memory performance to wait for both channels to arrive, as the DMA can then use larger blocks. I'd expect somewhat better results for 16-byte transfers than for 8 or 4 bytes, but I don't expect any improvement over that. TI is quit fuzzy on that too, and I didn't actually notice any measurable difference in my system. So you might want to set the tx_num_evt to "2" for systems with only few channels, and "1" when using 4 or more serializers (or TDM channels).
Maybe the misleading "rx_num_evt" should be renamed to "use_rx_fifo". I understand that not all McASP interfaces have a FIFO
Hmm. Just read some of this back, and noticed that I'm talking about recording data from codecs, while you're playing back data. Not that it makes much of a difference. The playback queue will wait for the buffer to fill to a certain level before starting to output it to the serializer. This increases latency, but may prevent underruns due to DMA congestion.
Mike.
Hi Mike,
Many thanks! Now all about "tx-num-evt" or "rx-num-evt" make sense.
Maybe the misleading "rx_num_evt" should be renamed to "use_rx_fifo". I understand that not all McASP interfaces have a FIFO
Maybe, "rx_fifo_depth" could be also used.
Best Michal
On 02/28/2013 10:06 AM, Michal Bachraty wrote:
Hi Mike,
Many thanks! Now all about "tx-num-evt" or "rx-num-evt" make sense.
Maybe the misleading "rx_num_evt" should be renamed to "use_rx_fifo". I understand that not all McASP interfaces have a FIFO
Maybe, "rx_fifo_depth" could be also used.
That's a big improvement over "rx_num_evt". The first time I saw this struct, I thought I had to set rx_num_evt to the number of receiving AXR pins in use.
Maybe call it "rx_min_fifo_depth" because just "depth" might be interpreted as the capaciity or maximum size of the fifo, while in fact the parameter is a minimum amount of data to be put into the FIFO buffer. Of maybe "rx_fifo_thd" to indicate it's a threshhold. And use the same name for the "tx" part as well (tx_fifo_thd or tx_fifo_min_depth).
Mike.
On Thursday, February 28, 2013 10:32:15 Mike Looijmans wrote:
That's a big improvement over "rx_num_evt". The first time I saw this struct, I thought I had to set rx_num_evt to the number of receiving AXR pins in use.
Maybe call it "rx_min_fifo_depth" because just "depth" might be interpreted as the capaciity or maximum size of the fifo, while in fact the parameter is a minimum amount of data to be put into the FIFO buffer. Of maybe "rx_fifo_thd" to indicate it's a threshhold. And use the same name for the "tx" part as well (tx_fifo_thd or tx_fifo_min_depth).
"rx_min_fifo_depth" or "tx_min_fifo_depth" seem to be fine. If I'll have time, I'll prepare patch. That's cosmetic upgrade, but should help others.
Michal.
On 28.02.2013 12:02, Michal Bachraty wrote:
On Thursday, February 28, 2013 10:32:15 Mike Looijmans wrote:
That's a big improvement over "rx_num_evt". The first time I saw this struct, I thought I had to set rx_num_evt to the number of receiving AXR pins in use.
Maybe call it "rx_min_fifo_depth" because just "depth" might be interpreted as the capaciity or maximum size of the fifo, while in fact the parameter is a minimum amount of data to be put into the FIFO buffer. Of maybe "rx_fifo_thd" to indicate it's a threshhold. And use the same name for the "tx" part as well (tx_fifo_thd or tx_fifo_min_depth).
"rx_min_fifo_depth" or "tx_min_fifo_depth" seem to be fine. If I'll have time, I'll prepare patch. That's cosmetic upgrade, but should help others.
Be careful with such changes. In general, DT bindings are forever in general, because you can't update all the users out there, and so every binding that has ever existed has to be supported in the future, for backwards compatibility.
In this case, we might make an exception, given that for AM33xx, the necessary DMA bits are still not missing, and Davinci is not yet fully ported to DT either. So nobody really uses the driver on this platforms.
I copied Mark, Grant and Hebbar to get their opinions.
Daniel
On 28.02.2013 14:19, Daniel Mack wrote:
On 28.02.2013 12:02, Michal Bachraty wrote:
On Thursday, February 28, 2013 10:32:15 Mike Looijmans wrote:
That's a big improvement over "rx_num_evt". The first time I saw this struct, I thought I had to set rx_num_evt to the number of receiving AXR pins in use.
Maybe call it "rx_min_fifo_depth" because just "depth" might be interpreted as the capaciity or maximum size of the fifo, while in fact the parameter is a minimum amount of data to be put into the FIFO buffer. Of maybe "rx_fifo_thd" to indicate it's a threshhold. And use the same name for the "tx" part as well (tx_fifo_thd or tx_fifo_min_depth).
"rx_min_fifo_depth" or "tx_min_fifo_depth" seem to be fine. If I'll have time, I'll prepare patch. That's cosmetic upgrade, but should help others.
Be careful with such changes. In general, DT bindings are forever in general, because you can't update all the users out there, and so every binding that has ever existed has to be supported in the future, for backwards compatibility.
In this case, we might make an exception, given that for AM33xx, the necessary DMA bits are still not missing, and Davinci is not yet fully ported to DT either. So nobody really uses the driver on this platforms.
Sorry, let me rephrase that last paragraph:
In this case, we might make an exception, given that for AM33xx, the necessary DMA bits are still missing, and Davinci is not yet fully ported to DT either. So supposedly nobody really uses the driver on this platforms via DT.
Daniel
On Thu, Feb 28, 2013 at 02:26:52PM +0100, Daniel Mack wrote:
On 28.02.2013 14:19, Daniel Mack wrote:
Be careful with such changes. In general, DT bindings are forever in general, because you can't update all the users out there, and so every binding that has ever existed has to be supported in the future, for backwards compatibility.
In this case, we might make an exception, given that for AM33xx, the necessary DMA bits are still not missing, and Davinci is not yet fully ported to DT either. So nobody really uses the driver on this platforms.
Sorry, let me rephrase that last paragraph:
In this case, we might make an exception, given that for AM33xx, the necessary DMA bits are still missing, and Davinci is not yet fully ported to DT either. So supposedly nobody really uses the driver on this platforms via DT.
Sounds plausible, though without having seen the actual change I can't comment for certain.
Hi,
On Fri, Mar 01, 2013 at 15:38:27, Mark Brown wrote:
On Thu, Feb 28, 2013 at 02:26:52PM +0100, Daniel Mack wrote:
On 28.02.2013 14:19, Daniel Mack wrote:
Be careful with such changes. In general, DT bindings are forever in general, because you can't update all the users out there, and so every binding that has ever existed has to be supported in the future, for backwards compatibility.
In this case, we might make an exception, given that for AM33xx, the necessary DMA bits are still not missing, and Davinci is not yet fully ported to DT either. So nobody really uses the driver on this platforms.
Sorry, let me rephrase that last paragraph:
In this case, we might make an exception, given that for AM33xx, the necessary DMA bits are still missing, and Davinci is not yet fully ported to DT either. So supposedly nobody really uses the driver on this platforms via DT.
Sounds plausible, though without having seen the actual change I can't comment for certain.
Even I have found this field name to be a bit confusing. As per my understanding, tx/rx_num_evt is the trigger-level for generating the DMA request. Taking the case of Tx, a DMA event is generated when the FIFO has space of x words where x < 64. At the same time, y words are transferred from the FIFO to the McASP, where y must be the number of active serializers. To ensure that the FIFO always has enough words to service all the active serializers, the driver needs to ensure that x is a non-zero integral multiple of the number of active serializers.
Assuming tx-num-evt is set to 1 and 1 serializer in I2S mode is active (which is the case before multi-serializer support), McASP is programmed to generate a DMA event when there's a space for 1*1 = 1 word in the FIFO. This minimizes the chance of under-runs but puts keeps the DMA controller active. At the same time, the DMA controller is programmed to transfer tx-num-evt ie. 1 sample (L or R). This is in sync with what the McASP expects.
With tx-num-evt set to 1 and 4 serializers in I2S mode, MCASP is programmed to generate a DMA event when there's a space for 1*4 = 4 words in the FIFO. The DMA controller is programmed with Acnt = 2 (data_type) and Bcnt = tx-num-evt = 1. So, every DMA event ends up transferring only 2 samples whereas the data is being drained out @ 4 words! So, when you add-in multi-serializer support, the current implementation results in a mismatch between the DMA programming and McASP leading to DMA errors.
To fix the above issue, instead of passing around the number of active serializers, like you did in your other patch, I would suggest making sure dma_params->fifo_level is programmed to (tx/rx_num_evt * active serializers). Do you see any issues with this approach?
Regards, Vaibhav
Hi Vaibhav,
On Wednesday, March 06, 2013 10:51:05 Bedia, Vaibhav wrote:
Hi,
On Fri, Mar 01, 2013 at 15:38:27, Mark Brown wrote:
On Thu, Feb 28, 2013 at 02:26:52PM +0100, Daniel Mack wrote:
On 28.02.2013 14:19, Daniel Mack wrote:
Be careful with such changes. In general, DT bindings are forever in general, because you can't update all the users out there, and so every binding that has ever existed has to be supported in the future, for backwards compatibility.
In this case, we might make an exception, given that for AM33xx, the necessary DMA bits are still not missing, and Davinci is not yet fully ported to DT either. So nobody really uses the driver on this platforms.
Sorry, let me rephrase that last paragraph:
In this case, we might make an exception, given that for AM33xx, the necessary DMA bits are still missing, and Davinci is not yet fully ported to DT either. So supposedly nobody really uses the driver on this platforms via DT.
Sounds plausible, though without having seen the actual change I can't comment for certain.
Even I have found this field name to be a bit confusing. As per my understanding, tx/rx_num_evt is the trigger-level for generating the DMA request. Taking the case of Tx, a DMA event is generated when the FIFO has space of x words where x < 64. At the same time, y words are transferred from the FIFO to the McASP, where y must be the number of active serializers. To ensure that the FIFO always has enough words to service all the active serializers, the driver needs to ensure that x is a non-zero integral multiple of the number of active serializers.
Assuming tx-num-evt is set to 1 and 1 serializer in I2S mode is active (which is the case before multi-serializer support), McASP is programmed to generate a DMA event when there's a space for 1*1 = 1 word in the FIFO. This minimizes the chance of under-runs but puts keeps the DMA controller active. At the same time, the DMA controller is programmed to transfer tx-num-evt ie. 1 sample (L or R). This is in sync with what the McASP expects.
With tx-num-evt set to 1 and 4 serializers in I2S mode, MCASP is programmed to generate a DMA event when there's a space for 1*4 = 4 words in the FIFO. The DMA controller is programmed with Acnt = 2 (data_type) and Bcnt = tx-num-evt = 1. So, every DMA event ends up transferring only 2 samples whereas the data is being drained out @ 4 words! So, when you add-in multi-serializer support, the current implementation results in a mismatch between the DMA programming and McASP leading to DMA errors.
To fix the above issue, instead of passing around the number of active serializers, like you did in your other patch, I would suggest making sure dma_params->fifo_level is programmed to (tx/rx_num_evt * active serializers). Do you see any issues with this approach?
No, In davinci-mcasp code, there is fifo_level assigned with txnumevt
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) fifo_level = dev->txnumevt; else fifo_level = dev->rxnumevt;
That code can be changed to computing fifo_level to dev->txnumevt * active_serializers and then it is needed to remove active_serializers from davinci-pcm. Code fuctionality will be same as is now with applied patch [PATCH v3] davinci-mcasp: Add support for multichannel playback Also there is need to change dst_cidx from 4 to 0, othewise when no fifo is used, it won't work properly.
On Fri, Mar 08, 2013 at 21:41:05, Michal Bachraty wrote:
No, In davinci-mcasp code, there is fifo_level assigned with txnumevt
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) fifo_level = dev->txnumevt; else fifo_level = dev->rxnumevt;
That code can be changed to computing fifo_level to dev->txnumevt * active_serializers and then it is needed to remove active_serializers from davinci-pcm. Code fuctionality will be same as is now with applied patch [PATCH v3] davinci-mcasp: Add support for multichannel playback Also there is need to change dst_cidx from 4 to 0, othewise when no fifo is used, it won't work properly.
Ok. Can you submit the next version addressing these issues?
Regards, Vaibhav
participants (5)
-
Bedia, Vaibhav -
Daniel Mack -
Mark Brown -
Michal Bachraty -
Mike Looijmans