[Sound-open-firmware] [PATCH v3 00/27] add R/W pointers check for pipeline buffers
Pierre-Louis Bossart
pierre-louis.bossart at linux.intel.com
Tue Feb 21 23:56:20 CET 2017
On 2/13/17 1:56 AM, Ughreja, Rakesh A wrote:
>
>
>> -----Original Message-----
>> From: Keyon Jie [mailto:yang.jie at linux.intel.com]
>> Sent: Monday, February 13, 2017 12:35 PM
>> To: Ughreja, Rakesh A <rakesh.a.ughreja at intel.com>; Pierre-Louis Bossart
>> <pierre-louis.bossart at linux.intel.com>; sound-open-firmware at alsa-
>> project.org; liam.r.girdwood at linux.intel.com
>> Cc: Jie, Yang <yang.jie at intel.com>; Ingalsuo, Seppo
>> <seppo.ingalsuo at intel.com>
>> Subject: Re: [Sound-open-firmware] [PATCH v3 00/27] add R/W pointers
>> check for pipeline buffers
>>
>> On 2017年02月13日 12:31, Ughreja, Rakesh A wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Keyon Jie [mailto:yang.jie at linux.intel.com]
>>>> Sent: Monday, February 13, 2017 7:31 AM
>>>> To: Pierre-Louis Bossart <pierre-louis.bossart at linux.intel.com>; sound-
>> open-
>>>> firmware at alsa-project.org; liam.r.girdwood at linux.intel.com
>>>> Cc: Ughreja, Rakesh A <rakesh.a.ughreja at intel.com>; Jie, Yang
>>>> <yang.jie at intel.com>; Ingalsuo, Seppo <seppo.ingalsuo at 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?
>>
>
> Its common code for both.
But only for SKL and newer platforms. It's irrelevant for
BYT/CHT/HAS/BDW where the HDAudio DMA is not used for host-DSP transfers.
>
>>>
>>> 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 at alsa-project.org
>>>>> http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
>>>>>
More information about the Sound-open-firmware
mailing list