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

[Keyon Jie]

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.

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