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

[Rajwa, Marcin]

On 7/18/2019 5:24 PM, Pierre-Louis Bossart wrote:
>
>
> 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.

@Pierre, indeed the buffer that kernel allocates is big enough to store 
all the data. However *arecord* introduces its own buffer which is just 
a multiple of *period_sizes *- and it is the buffer which overflows.

>
> 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?

Yes, this is the same value. It is obtained by *params_period_bytes**()* 
in kernel.

*no_irq mode *- it does not affect the value of *host_period_bytes 
*after my patch has been applied. Before my patch however, driver and FW 
used this value to send stream position information from FW to kernel. 
In short, when *(hda && hda->no_ipc_position)*

then *ipc_params->host_period_bytes = 0; *was set in kernel.**Firmware 
then, read this *host_period_bytes = 0 *and treated it as "OK does not 
send any IPC regarding stream position". So once *no_ipc_position *was 
set we lost information about *host_period_bytes *in the FW.

So all my patches in FW and Kernel do is to *relax****host_period_bytes 
*and introduce new parameter dedicated for this stream position IPC. At 
that time we called it *no_period_irq *to resemble old name but now I 
think it would be better if we rename it to something like

*no_stream_position *as it is more informative. What do you think?


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