[Sound-open-firmware] [PATCH v3 00/27] add R/W pointers check for pipeline buffers

[Pierre-Louis Bossart]

On 2/12/17 8:01 PM, Keyon Jie wrote:
> On 2017年02月11日 10:37, Pierre-Louis Bossart wrote:
>> On 2/10/17 8:17 PM, Keyon Jie wrote:
>>> This series adding the R/W buffer pointers check and make sure we won't
>>> copy wrong/missed/repeated data for the whole pipeline.
>>>
>>> For ALSA applications, usually we have two kinds of trigger command
>>> sequences:
>>> 1. (e.g. aplay) prefill datas ==> trigger start ==> refill buffer(when
>>> buffer
>>>         free is enough) ==> ... ==> data filling finished ==> trigger
>>> drain ==>
>>>         when all datas are rendered(via read pointer feedback) ==>
>>> trigger stop;
>>> 2. (e.g. speaker-test with large buffer size > wav file size):
>>>         prefill all datas one time ==> trigger start ==> wait until
>>> all datas are
>>>         rendered(via feedback read pointer) ==> trigger stop ==>
>>> prefill another
>>>         wav file ==> trigger start ==> ...
>>>
>>> So the feedback from firmware about the read pointer is very important
>>> for
>>> the pipeline's running, to make the firmware can handle these two
>>> kinds of
>>> sequences, this series design/assume with preconditions:
>>>
>>> 1. We do down stream full preload(full fill all pipeline buffers,
>>> except host
>>> side one);
>>> 2. Using pull mode at running stage and pulling one period per time;
>>> 3. When in normal running status, if buffer a->avail is 0, then all
>>> those
>>> upstream buffers are empty;
>>>
>>> For those two sequence, we use a same logic to handle it:
>>>
>>> 1. We put all components in running status even some of them are
>>> empty(dai is
>>> an exception);
>>> 2. We stop dai/ssp once we found there is no more data for it to
>>> render, and
>>> notify this last read position to host side;
>>> 3. Host side will trigger stop and we will reset all buffer pointers and
>>> related resources;
>>> 4. For case #1, trigger reset will reset/free more
>>> resources(allocated/init
>>> in set_params()); For case #2, another start cmd will trigger preload
>>> and start
>>> in firmware, before that all pointers are reset to original ones.
>>>
>>> TODOs: It is only verified for playback, for capture, we may need some
>>> vivid
>>> code to implement them.
>>
>> I may be missing something here - the cover letter isn't very clear - ,
>> but I see mentions of 'pull one period at a time' and no mention of the
>> start/stop threshold. Is this really going to work in no-irq mode where
>> periods are irrelevant and with start/stop thresholds that are smaller
>> than a period.
>
> Um, we don't actually use start/stop thresholds inside sof, or you can
> think of we are using one period as thresholds and it is
> non-configurable: after preloaded, we trigger dai dma start, in dma tfr
> callback(the tfr copies one period each time), we will schedule upstream
> copy(what I called it 'pull mode' upper) that will copy one period
> upstream.

Sorry, I may be thick and old, but what is the relationship between a 
period (in the ALSA sense) and a DMA transfer? It should be decoupled 
really, if the host uses a 2s ring buffer with 2 periods, and there is 
no way you will ever have enough memory in the DSP to DMA 1s of audio.
Conversely, if you have a deep buffer out and a low-latency one, you'd 
want separate DMA granularities for each, so you don't want to use a 
hard coded DMA transfer size.
My understanding is that the period should be a multiple of the DMA 
granularity size, that's how we enabled S0i1.

>
>> If you really want to make the transfers reliable then you should also
>> assume that there are no rewinds and compare read/write pointers to
>> avoid fetching stale data. that's a useful mode to have
>
> We have added handling for rewinds and comparing R/W pointers in this
> series, and won't fetch stale data with them applied.
>
> Thanks,
> ~Keyon
>
>>
>>>
>>> Changes in v3:
>>> 1. add one preload patch for pipeline full preload, which make
>>> playback works.
>>> 2. fix DMA channel status wrong after trigger stop issue.
>>>
>>> Changes in v2:
>>> 1. extract some common code to separate functions to save code;
>>> 2. add comments for component status;
>>> 3. other slight changes per Liam's suggestion.
>>>
>>> Keyon Jie (26):
>>>   host: handle more possibility for playback spitting
>>>   dai: reset dai_pos at MMAP_PPOS command
>>>   pipleline: op_sink: don't need clear the buffer for each prepare
>>>   host: start: don't real start for start command
>>>   ipc: presentation_posn should be uint64_t type
>>>   intel-ipc: add application buffer pointer update from host side
>>>   intel-ipc: add stream operation IPC_INTEL_STR_STOP
>>>   host: add host_update_buffer for produce and consume
>>>   host: rename host_pos_blks to host_pos_read
>>>   host: for the last bytes/period, send read notification later
>>>   host: add R/W pointers check for playback
>>>   host: add a completion work to notify host side
>>>   host: reset pointers at new() and reset()
>>>   host: cleanup host_cmd() cases
>>>   host: host_copy: only copy when there is data and wait until finish
>>>   volume: volume_params(): set the sink buffer params depending on its
>>>     sink
>>>   component: status: rename COMP_STATE_STOPPED to COMP_STATE_SETUP
>>>   volume: volume_cmd(): only stop/pause at running
>>>   volume: add R/W pointer check for volume_copy()
>>>   mixer: mixer_params(): set the sink buffer params to
>>>     STREAM_FORMAT_S32_LE.
>>>   mixer: add R/W pointer check for mixer_copy()
>>>   mixer: add dd->last_bytes for the last bytes(< period) caculation
>>>   dai: add R/W pointer check for dai copy
>>>   dw-dma: handle different cases in irq handler
>>>   component: host/volume/mixer: reset buffers for stop cmd
>>>   host: create host_elements_reset() and host_pointer_reset() for common
>>>     use
>>>
>>> Liam Girdwood (1):
>>>   pipeline: Add support for full pipeline pre-loading.
>>>
>>>  src/audio/dai.c                                   |  71 ++++-
>>>  src/audio/host.c                                  | 313
>>> ++++++++++++++++------
>>>  src/audio/mixer.c                                 |  85 +++++-
>>>  src/audio/pipeline.c                              | 107 +++++++-
>>>  src/audio/volume.c                                |  59 +++-
>>>  src/drivers/dw-dma.c                              |  30 ++-
>>>  src/include/reef/audio/component.h                |  59 +++-
>>>  src/include/reef/audio/pipeline.h                 |   2 +
>>>  src/include/reef/dma.h                            |   7 +
>>>  src/include/uapi/intel-ipc.h                      |   3 +-
>>>  src/ipc/intel-ipc.c                               |  69 +++++
>>>  src/platform/baytrail/include/platform/platform.h |   6 +
>>>  12 files changed, 661 insertions(+), 150 deletions(-)
>>>
>>
>> _______________________________________________
>> Sound-open-firmware mailing list
>> Sound-open-firmware at alsa-project.org
>> http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
>>
> _______________________________________________
> Sound-open-firmware mailing list
> Sound-open-firmware at alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware