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

[Jie, Yang]

> -----Original Message-----
> From: Pierre-Louis Bossart [mailto:pierre-louis.bossart at linux.intel.com]
> Sent: Wednesday, February 22, 2017 6:55 AM
> To: Keyon Jie <yang.jie at linux.intel.com>; sound-open-firmware at alsa-
> project.org; liam.r.girdwood at linux.intel.com
> Cc: Ingalsuo, Seppo <seppo.ingalsuo at intel.com>; Jie, Yang
> <yang.jie at intel.com>; Ughreja, Rakesh A <rakesh.a.ughreja at intel.com>
> Subject: Re: [Sound-open-firmware] [PATCH v3 00/27] add R/W pointers check
> for pipeline buffers
> 
> 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.

This is good point. Actually the threshold(1ms) I mentioned above is meant to
be the ones for buffer inside DSP. For ALSA ring buffer period, it can be 1s or
even 64s, we can configure the FW read offset(to this ALSA ring buffer) 
notification interval(to host side) to n*ALSA_period via IPC.

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

Yes, currently we set FW inside period size as DMA transfer size for both host
side and dai/ssp side DMA, they are 1ms buffer size and 'hard coded' via macros.

Right, suppose the DMA granularity is 4 Bytes, frame size is 4 Bytes, theoretically,
we can use small FW inside period up to 4 Bytes(1 Frame), it's good that we
make this period size(or DMA transfer size) configurable, we can treat the macros
definition as 'half configurable', and it's easy to add configure APIs if needed.

Thanks,
~Keyon

> 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