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.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
