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

[Rajwa, Marcin]

On 7/19/2019 4:25 PM, Pierre-Louis Bossart wrote:
>
>>> 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.
>
> When I tested this feature with the closed-source firmware on KBL, 
> arecord worked fine. Care to provide more details so that we are on 
> the same page?
>
>>
>>>
>>> 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.
>
> ok good. I was afraid this would be a different concept.
>
> So what you are saying is that the draining happens by bursts whose 
> size is tied to the period defined by the host, yes?
Yes. We try to fill host buffer as much as we can to gain fast draining 
but in the same time we give host time to read it.
>
>>
>> *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?
>
> The text is quite difficult to read with all the *... Please use plain 
> text.
Ahh I used bold text to stress some important keywords but it seems your 
mail client can't read it back. I will stop using it, thanks for letting 
me know.
>
> It just occurred to me that the traditional use of the timer-based 
> scheduling (with no_irq mode) is not very smart with this sort of 
> application. The host has absolutely no way of predicting for how long 
> it needs to sleep before the firmware completes the initial flush. 
> This time is linked to hardware, bandwidth to memory, etc, and might 
> vary quite a bit between platforms. In this case, it's much better to 
> rely on events set by hardware upon data availability.
Exactly. We were facing DMA issues along the way which slowed down 
draining considerably. So if it all was driven by some timer it would 
never work properly.
>
> in terms of naming, no_stream_position_ipc would be my choice, but 
> it's a bit long.

Heh, I had the same idea and the same problem - length of the variable. 
So I cut _ipc suffix and I think it is good enough.

>
>>>>
>>>> Hope it helped to understand the need of *host_period_bytes *in the 
>>>> firmware.
>
> Yes, thanks for taking the time to explain the issue.