On 2017年02月13日 12:31, Ughreja, Rakesh A wrote:

...

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

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.

Hi Keyon,

There is a register called SDnFIFO. Its defined in the HDA Spec and used by ALSA framework. FW should advertise the buffering via this parameter.

By default this value is 1ms in the HDA Spec. (legacy mode)

Thanks for sharing that, Rakesh. BTW, does this SDnFIFO register only influence HDA audio, or it is needed for ASoC also?

The buffer on the firmware side should be configurable by the driver. By default driver may assign it to 1ms and when no-rewind flag is set it can set it to larger value for offload playback.

Our target is definitely to make the buffer on firmware side configurable, by the driver, we may introduce topology support in next milestone release, where the pipeline/buffer can be changed dynamically.

It is not wrong to fetch 1 period as long as you advertise that via SDxFIFO. In order to comply with legacy applications, which does not assume too much buffering it is better to start with minimum buffering possible that FW can handle.

Um, we haven't advertised this yet, for ASoC, we preset ALSA buffer size/period size/num range in snd_pcm_hw, application will set it in initial stage. For firmware inside buffer size, we are using quite small ones, but will make it configurable in short future.

Thanks, ~Keyon

...

...

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@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware