[alsa-devel] [PATCH v3 1/2] ASoC: SOF: add flag for position update ipc

[Pierre-Louis Bossart]

On 7/18/19 3:42 AM, Rajwa, Marcin wrote:
> 
> On 7/18/2019 7:06 AM, Keyon Jie wrote:
>>
>>
>> On 2019/7/18 上午11:35, Pierre-Louis Bossart wrote:
>>>
>>>
>>> On 7/17/19 10:11 PM, Keyon Jie wrote:
>>>> From: Marcin Rajwa <marcin.rajwa at linux.intel.com>
>>>>
>>>> In some cases, FW might need to use the host_period_bytes field to
>>>> synchronize the DMA copying (with host side) but the driver does not
>>>
>>> it's your right to edit my suggested wording, but the notion of 
>>> 'synchronization' is far from clear. it's my understanding that the 
>>> host_period_bytes defines the DMA transfer size requested by the 
>>> firmware, which isn't a value that matters to the host except for 
>>> rewind usages.
>>
>> Hi Pierre, here the host_period_bytes is requested by host, FW has its 
>> own period size, and DMA will transfer data in FW buffer period size. 
>> It works like this:
>>
>> FW buffer[period 0, period 1, ...] <==> DMA <==> host/alsa 
>> buffer[host_period 0, host_priod 1, ...]
>>
>> We need this host_preiod_bytes information in FW to do fast 
>> draining(e.g. record 2 seconds data within 10ms) in mmap capture, we 
>> are slowing down the draining in smaller host_period_bytes cases, 
>> otherwise, arecord can't read the buffer in time and overrun will happen.
>>
>> Maybe the wording "synchronize" here is inaccurate, how about 
>> something like this:
>>
>> "FW might need to use the host_period_bytes field to configure and 
>> control the DMA copying speed but the driver does not..."
>>
> 
> Hi,
> 
> we need *host_period_bytes *in FW to properly drain data - by properly I 
> mean to no override host buffer but in the same time to avoid situation 
> when host is waiting for data and doesn't get it. The former is known as 
> *overrun *while the later *underrun**.
> *
> 
> So that's why I originally used the word *"to synchronize" *because it 
> best reflects the use of this variable in FW.
> 
> *The problem *- host establishes the *period_size/**host_period_bytes 
> *and uses it as a "data copy tempo controller" meaning it will copy data 
> buffered in its own buffer (copied there by FW) in *period_size *time 
> intervals. Now, in regular copy (real time stream) firmware doesn't
> 
> need to care about how fast host copy because the host buffer is big 
> enough to store even several FW periods (each one or few milliseconds)  
> without overriding it. That is why we did not need this *period_size 
> *variable before. However the draining task is very
> 
> different ball game - it copies *2,1 seconds* of data as fast as 
> possible to the host. Therefore we are very prone to *XRUNs*
> 
> *The solution* - in FW we need to know how often host copies data out 
> from its own buffer and this information is stored in 
> *host_period_bytes, *lets send this information down to firmware. Now, 
> the FW knowing this can fill the host buffer and wait the time 
> calculated by *host_period_bytes*
> 
> before next copy. This way we copy as much/fast as we can and in the 
> same time we are safe that host will handle this and no XRUN will ever 
> happen.


We knew from Day1 that draining faster than real-time could potentially 
lead to the driver detecting overflows or timeouts. It's been documented 
left and right, with an assumption that the ring buffer is actually big 
enough to contain all the data stored in the DSP.

Before I provide more feedback, can you clarify if the 
'host_period_bytes' is the same value as the ALSA period size (in 
bytes)? And what would be the value when the no_irq mode is used?

> 
> 
> Hope it helped to understand the need of *host_period_bytes *in the 
> firmware.
> **
> 
> Marcin
>