davici-mcasp: "tx-num-evt" confusion with number of serializers

davici-mcasp: "tx-num-evt" confusion with number of serializers

[Michal Bachraty]

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

Re: davici-mcasp: "tx-num-evt" confusion with number of serializers

[Mike Looijmans]

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.

-- 
Mike Looijmans - Topic Automation

Re: davici-mcasp: "tx-num-evt" confusion with number of serializers

[Michal Bachraty]

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.

Re: davici-mcasp: "tx-num-evt" confusion with number of serializers

[Mike Looijmans]

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.

Re: davici-mcasp: "tx-num-evt" confusion with number of serializers

[Michal Bachraty]

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

Re: davici-mcasp: "tx-num-evt" confusion with number of serializers

[Mike Looijmans]

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.

Re: davici-mcasp: "tx-num-evt" confusion with number of serializers

[Michal Bachraty]

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.

Re: davici-mcasp: "tx-num-evt" confusion with number of serializers

[Daniel Mack]

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

Re: davici-mcasp: "tx-num-evt" confusion with number of serializers

[Daniel Mack]

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

Re: davici-mcasp: "tx-num-evt" confusion with number of serializers

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 957 bytes --]

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.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: davici-mcasp: "tx-num-evt" confusion with number of serializers

[Bedia, Vaibhav]

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

Re: davici-mcasp: "tx-num-evt" confusion with number of serializers

[Michal Bachraty]

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. 

Re: davici-mcasp: "tx-num-evt" confusion with number of serializers

[Bedia, Vaibhav]

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