[bkokoszx]

In KPB topology we have to different pipelines (not considering playback): kpb pipeline (from host to dai) and detector pipeline (from selector to detector). We have timer driven pipelines, so we are able to define their periods (here, we have 20 ms for capture pipelines and we add pipeline_task for every different pipeline in pipeline_prepare()), but we are not able to predict (or assume) whether one pipeline_task would scheduled just after the second one.

                pipe_plb
               +------------------+
        +----+ | +---+  +-------+ |
        |Host|-->|Buf|->|SSP Dai|----------------------------------+
        +----+ | +---+  +-------+ |                                |
               +----------------- +                                |
                                                                   |
                   +-------------------------+                     |
        +--------+ | +---+   +-----+   +---+ |                     |
        |KD Dummy|<->|Buf|<->|ChSel|<->|Buf|<->+                   |
        +--------+ | +---+   +-----+   +---+ | |  pipe_cap         |
                   +-------------------------+ |                   |
                                               |                   |
                             +-----------------+                   |
        +--------------------|-----------------------------------+ |
        |+----+  +------+  +---+  +---+  +---+  +---+  +-------+ | |
        ||Host|<-|Buffer|<-|Kpb|<-|Buf|<-|Vol|<-|Buf|<-|SSP Dai|---+
        |+----+  +------+  +---+  +---+  +---+  +---+  +-------+ |
        +--------------------------------------------------------+

Currently, on master in most cases we see following flow:

1. pipeline_task() on KPB pipeline.
2. pipeline_task() on detector pipeline.
   2a. Detector detects signal and send notification to KPB.
3. KPB gets notification.
   3a. KPB init draining (here, KPB calculates its r_ptr in history buffer in order to perform draining
   properly and schedules draining_task).
4. draining_task() is scheduled by ll scheduler.
5. pipeline_task() on KPB pipeline.

Because KPB and detector pipelines has huge periods we usually have a lot of time to init draining task and do the draining, but we are not able to guarantee that following flow will not happen:

1. pipeline_task() on detector pipeline.
   1a. Detector detects signal and send notification to KPB.
2. KPB gets notification.
   2a. KPB init_draining() (here, KPB calculates its r_ptr in history buffer in order to perform draining properly and schedules draining_task()).
3. Here, just after draining initialization, there is new KPB pipeline_task(). New kpb_copy() comes, but there is a possibility, that we do not have enough memory in history buffer, because the only way we check free space is (in kpb_buffer_data()):

	/* Check how much space there is in current write buffer */
		space_avail = (uint32_t)buff->end_addr - (uint32_t)buff->w_ptr;

We do not consider r_ptr. The new data are those data we see at the beginning of record (also in issue #2270 ).

This PR contains PR #2331 (I will rebase it after merging #2331). This commit adds new KPB private variable hb_free_space in order to track how much space is available in history buffer. It fixes #2270.

[lgirdwood]

I really think we need the state transition diagram now, this is becoming too complex to review.

[lgirdwood]

@mrajwa ok, so we now have a finite state machine, so we need to have a state transition diagram like we do for general purpose components. Can you add this to make this easier to track and understand.
Another useful thing would be (after the diagram is added) is to add some conditional DEBUG code in kpb_change_state() that validates each state transition i.e. will emit trace_error if invalid transition is attempted.

[mrajwa]

I agree, where would you like me to add this state transition diagram?

[lgirdwood]

yes please

[mrajwa]

The problem is during "KPB_STATE_INIT_DRAINING" there should be no internal buffering as this can corrupt read pointer calculation.

[bkokoszx]

@mrajwa
I agree, but after adding spin lock in #2329, that situation will never happen. We want to properly handle situation when new kpb_copy() will come between kpb_init_draining() and kpb_draining_task().

[mmaka1]

I believe there is no such thing as "not enough memory in history buffer". It is circular buffer, so the copy function appends the data as it comes from mics in a circular manner and write pointer (wp) should be always advanced accordingly. While the read pointer (rp) is initialized at the beginning of data on the detector request and wp must never pass rp to avoid described situation where newer data are drained before older piece. But a right way to avoid this is to move rp along with wp and just lost a piece of the oldest data in case rp has not started moving yet (because of described latency of the draining task). Once draining starts, we expect rp to move much faster than wp so that initial push should occur only between draining init and draining start and drained data will be continuous.

If the buffer is large enough, the beginning of key phrase is in the safe distance from the end (approx. place where rp is initialized) so rp might be pushed with no harm. While freezing the wp, as originally proposed, is dangerous because it may make anything that comes after the key phrase (like a command which is important) non continuous.

[paulstelian97]

I believe there is no such thing as "not enough memory in history buffer". It is circular buffer, so the copy function appends the data as it comes from mics in a circular manner and write pointer (wp) should be always advanced accordingly. While the read pointer (rp) is initialized at the beginning of data on the detector request and wp must never pass rp to avoid described situation where newer data are drained before older piece. But a right way to avoid this is to move rp along with wp and just lost a piece of the oldest data in case rp has not started moving yet (because of described latency of the draining task). Once draining starts, we expect rp to move much faster than wp so that initial push should occur only between draining init and draining start and drained data will be continuous.

If the buffer is large enough, the beginning of key phrase is in the safe distance from the end (approx. place where rp is initialized) so rp might be pushed with no harm. While freezing the wp, as originally proposed, is dangerous because it may make anything that comes after the key phrase (like a command which is important) non continuous.

How long is the host allowed to stall while resuming? Sometimes it could take quite a whiiiiiile (several seconds). Are you sure you have enough time until the host is ready to start consuming the data?

[mrajwa]

@paulstelian97, yes we are safe with that. Here we are fighting delays of DSP side only.
Okay this PR needs to be cancelled now, as I am going to implement more elegant solution of this issue.

[mmaka1]

The host starts receiving the drained data immediately. The DMA transfers are running while the host is waking up. So the host DMA buffer should just be large enough to fit the drained data as well as ongoing real time stream until application is ready to process it. Otherwise DMA transfers stop and overflow occur either in the drained data phase or the subsequent real time streaming phase.

[lgirdwood]

@bkokoszx any update here ?

[bkokoszx]

Referring to the comment: #2343 (comment)
I will close this PR.