kpb: merge with cml-005-drop-stable by mrajwa · Pull Request #1672 · thesofproject/sof

mrajwa · 2019-07-25T21:20:56Z

This PR merges the KPB component to the state from KWD release (cml-005-drop-stable)

Signed-off-by: Marcin Rajwa marcin.rajwa@linux.intel.com

jajanusz

Looks good. It has just few minor style issues.

src/audio/kpb.c

jajanusz · 2019-07-26T06:50:44Z

@mrajwa Please fix UT and checkpatch warnings,

tlauda · 2019-07-26T06:36:31Z

src/audio/kpb.c

Why do we need it? Every generic component API function operates on dev from which you can obtain comp_data, not other way around.

@tlauda it is stored in component data so functions like buffering or draining can use it to finish requested reset. The reset procedure in KPB is a bit more complicated than it is in other components.

I see it only used in kpb_buffer_data just to pass it to kpb_reset. You have function kpb_copy, which takes comp_dev and from comp_dev you are getting comp_data, which is passed to kpb_buffer_data, which needs to pass comp_dev retrieved from comp_data. It doesn't seem right to me.

src/audio/kpb.c

tlauda · 2019-07-26T06:47:01Z

src/audio/kpb.c

It's a little bit misleading. RESET_FINISH, but still we return EBUSY.

I agree, what about KPB_STATE_RESET_FINISHING ?

Sounds good.

src/audio/kpb.c

tlauda · 2019-07-26T06:51:56Z

src/audio/kpb.c

It would be better to check for ret < 0 in case the function will return some non-error positive status in the future.

@tlauda but the point is kpb_buffer_data() will return non zero value only if there was some error. Sometimes it will return negative linux codes, sometimes positive sof errors like PPL_STATUS_PATH_STOP.

tlauda · 2019-07-26T06:52:07Z

src/audio/kpb.c

jajanusz · 2019-07-26T17:04:44Z

src/audio/kpb.c

Why EIO initially?

Can you also add a little more detail in the commit message.

@jajanusz corrected, @lgirdwood , commit message updated.

jajanusz · 2019-07-26T17:08:07Z

src/include/sof/audio/kpb.h

Is it read anywhere? Because in this patch I just saw it being only written to.

New commits were pushed, there is the answer.

lgirdwood · 2019-07-29T13:17:02Z

src/audio/kpb.c

Commit message needs to say why you are doing this.

lgirdwood · 2019-07-29T13:19:20Z

src/audio/kpb.c

Do we have a state transition diagram like we do for generic components ? This is needed given the number of states we have in KPB.

@lgirdwood, I am afraid we don't have such a diagram. These states are KPB internal, no other component or pipeline should be aware of it. The main purpose of these quite detailed states is to decide if we can reset/free KPB right away or should be postpone after we gracefully terminate current KPB task - draining or buffering states are a good example here. Secondly, since KPB is a heart of KWD its good to have such detailed states in regards to debugging - I am going to add a new patch which records last eight states of KPB. So every time something wrong happened we can check how KPB was changing its states.

lgirdwood · 2019-07-29T13:21:47Z

src/audio/kpb.c

Probably worth a trace message here for debugability.

Definitely.

lgirdwood · 2019-07-29T13:23:08Z

src/audio/kpb.c

Can you also add a little more detail in the commit message.

jajanusz · 2019-07-29T22:00:08Z

src/audio/kpb.c

checkpatch complains here about multiple blank lines

mrajwa · 2019-07-30T10:35:13Z

@lgirdwood @jajanusz please review as this is high priority PR which blocks upcoming release.

This patch renames component data local variables name from 'cd' to 'kpb'. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

This patch passes comp device pointer to buffering and draining function as this pointer is needed to finish reset requests. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

mrajwa · 2019-07-30T15:15:44Z

I had to temporarily disable cmocka test case regarding to local buffering as it is not capable to handle to do it right with latest changes. I will updated in next PR as there is no time for it before the release.

jajanusz

please squash UT commit with change-introduction commit to not break git bisect

This change moves history buffer allocation from kpb_new() to kpb_prepare(). This is to folow the calling convention with kernel. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

This change unregisters KPB from async notifications during kpb_free(). Additionally reset main history buffer pointer. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

This commit adds more detailed states into KPB Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

This patch takes care of secure reset. Now we check in what state KPB currently is. If this state involves some ongoing work like draining or buffering then KPB will register reset request and terminate this job gracefully. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

This adds function to reset history buffer key pointers. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

This patch takes care of secure reset while KPB is in buffering state. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

This patch enables draining task to know how much data was buffered since draining task has been started. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

This patch covers calculations of history depth based on sample container size rather than sample width itself. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

This patch checks if KPB is in proper state beforeinitialize draining. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

This change dma-copy type from blocking to normal. This is because blocking mode slows down draining considerably and from safety standpoint we cover it by period-interval now. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

This patch introduces and calulates draining intervals as well as bytes limit per interval. It is all needed to synchronize firmware draining speed with host read speed. Basically in one period we fill half of host buffer and give host some time (period_interval) to read the data. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

This patchs logs requested draining length in milliseconds in kpb_init_draining() procedure. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

This patch checks if host period size and well as host buffer size are sane and can be used for draining calculations. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

This patches moves the switch of KPB state from RUN to DRAINING at the time draining task starts. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

This patch stops selector copy before draining starts. This is temporary change as driver should take care of it. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

This patch adds a check in draining task for reset request. If it happened while we were draining then draining task shall me carefully terminated. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

This patch makes calculation of time taken to drain history buffer more evident and clear. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

This patch limit the speed of draining to the speed of host reads. We fill half of the host buffer and give a time of one period to read it. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

This patch takes care of changing kpb buffer if last drained size made read pointer equal to buffer end address. This is important since in slow draining cases we might need to update history size by real time stream and therefore continue draining. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

This patch checks if real time stream provided new data while we were draining. If so we need to continue draining that new data which are wainiting in history buffer. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

This patch updates function kpb_drain_samples() which copies samples from history buffer to host buffer to support 24b & 32b samples. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

This patch updates function kpb_copy_samples() which copies samples from KPB source buffer to sink buffer to support 24b & 32b samples. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

This patch adds a check if history depth request is bigger than history buffer size. If so refuse to drain. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

Draining task can take a long time depending on various params. This patch disables system agent which raises DSP PANIC in case firmware is hanged. However draining task can look to system agent in exactly the same manner thus we disable it right before draining task and reenable after draining is done. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

This patch switches KPB into RUN state after prepare. This is expected state once KPB was properly initialized. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

This patch makes kpb_copy() to return -EIO if it was called while KPB was in wrong state. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

jajanusz · 2019-07-30T17:35:21Z

@lgirdwood Your requests seem to be addressed

mrajwa · 2019-07-30T19:04:55Z

As for checkpatch error, it can be ignored I believe. However the UT fail is a surprise to me as all of them passes on my machine. Looks like some random fail. I have triggered new verification.

mrajwa · 2019-07-30T19:34:08Z

I was right it was random UT failure, after I retriggered QB the issue is gone. @jajanusz can we merge now?

jajanusz · 2019-07-30T19:47:26Z

I was right it was random UT failure, after I retriggered QB the issue is gone. @jajanusz can we merge now?

It wasn't UT failure. Py smoke tests failed because of connection problem to DUT platform ;)
As for checkpatch - it complains about type camelcase type from 3rd party library, so nonissue.

Changes have been addressed

marc-hb

error: array subscript is above array bounds [-Werror=array-bounds]

sof/test/cmocka/src/audio/kpb/kpb_buffer.c:270:7:
tests[1].name = "KPB real time copy and buffering (Double Buffer)";

marc-hb · 2020-03-19T22:30:20Z

test/cmocka/src/audio/kpb/kpb_buffer.c

 int main(void)
 {
-	struct CMUnitTest tests[2];
+	struct CMUnitTest tests[1];


error: array subscript is above array bounds [-Werror=array-bounds] sof/test/cmocka/src/audio/kpb/kpb_buffer.c:270:7: tests[1].name = "KPB real time copy and buffering (Double Buffer)";

etc.

One-character revert submitted in PR #2591

mrajwa requested review from lbetlej, lgirdwood, mmaka1 and plbossart as code owners July 25, 2019 21:20

mrajwa added the P1 Blocker bugs or important features label Jul 25, 2019

michalgrodzicki requested review from jajanusz and tlauda July 25, 2019 21:34

jajanusz suggested changes Jul 26, 2019

View reviewed changes

src/audio/kpb.c Outdated Show resolved Hide resolved

src/audio/kpb.c Outdated Show resolved Hide resolved

src/audio/kpb.c Outdated Show resolved Hide resolved

tlauda suggested changes Jul 26, 2019

View reviewed changes

mrajwa force-pushed the kwd_kpb_merge branch from 1842246 to 788a10f Compare July 26, 2019 12:57

mrajwa requested a review from keyonjie July 26, 2019 12:58

jajanusz reviewed Jul 26, 2019

View reviewed changes

lgirdwood previously requested changes Jul 29, 2019

View reviewed changes

mrajwa force-pushed the kwd_kpb_merge branch from 788a10f to 32bd96f Compare July 29, 2019 21:52

jajanusz reviewed Jul 29, 2019

View reviewed changes

tlauda approved these changes Jul 30, 2019

View reviewed changes

mrajwa added 2 commits July 30, 2019 16:24

kpb: unify component data naming

79cc7eb

This patch renames component data local variables name from 'cd' to 'kpb'. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

kpb: pass component device in events

a57de0a

This patch passes comp device pointer to buffering and draining function as this pointer is needed to finish reset requests. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

mrajwa force-pushed the kwd_kpb_merge branch from 32bd96f to 5e4e452 Compare July 30, 2019 15:12

mrajwa force-pushed the kwd_kpb_merge branch from 5e4e452 to 8334b9a Compare July 30, 2019 15:42

jajanusz suggested changes Jul 30, 2019

View reviewed changes

mrajwa added 5 commits July 30, 2019 18:04

kpb: move history buffer allocation to prepare

0402935

This change moves history buffer allocation from kpb_new() to kpb_prepare(). This is to folow the calling convention with kernel. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

kpb: unregister kpb from notifications at free()

6d9af31

This change unregisters KPB from async notifications during kpb_free(). Additionally reset main history buffer pointer. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

kpb: extend internal states

9d1a34f

This commit adds more detailed states into KPB Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

kpb: add function to reset history buffer pointers

d21d988

This adds function to reset history buffer key pointers. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

mrajwa added 21 commits July 30, 2019 18:04

kpb: add secure reset while buffering

ff8a8af

This patch takes care of secure reset while KPB is in buffering state. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

kpb: record history buffer update while draining

23b0522

This patch enables draining task to know how much data was buffered since draining task has been started. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

kpb: calculate history depth depending on sample container size

9f41387

This patch covers calculations of history depth based on sample container size rather than sample width itself. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

kpb: don't allow draining if KPB is in wrong state

4ec95bc

This patch checks if KPB is in proper state beforeinitialize draining. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

kpb: change draining copy type to normal

dc2cc52

This change dma-copy type from blocking to normal. This is because blocking mode slows down draining considerably and from safety standpoint we cover it by period-interval now. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

kpb: log requested draining

85a9b47

This patchs logs requested draining length in milliseconds in kpb_init_draining() procedure. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

kpb: add sanity check of host params

cf40ffb

This patch checks if host period size and well as host buffer size are sane and can be used for draining calculations. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

kpb: change KPB state to draining only if draining started

66ba310

This patches moves the switch of KPB state from RUN to DRAINING at the time draining task starts. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

kpb: pause selector before draining starts

3f88342

This patch stops selector copy before draining starts. This is temporary change as driver should take care of it. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

kpb: add secure reset to draining task

169c902

This patch adds a check in draining task for reset request. If it happened while we were draining then draining task shall me carefully terminated. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

kpb: rework calculations of time taken to drain data

e91752e

This patch makes calculation of time taken to drain history buffer more evident and clear. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

kpb: limit draining speed to match host params

701e402

This patch limit the speed of draining to the speed of host reads. We fill half of the host buffer and give a time of one period to read it. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

kpb: take care of real time stream during draining

08ceb10

This patch checks if real time stream provided new data while we were draining. If so we need to continue draining that new data which are wainiting in history buffer. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

kpb: update kpb_drain_samples() to support 24b samples

1fa3105

This patch updates function kpb_drain_samples() which copies samples from history buffer to host buffer to support 24b & 32b samples. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

kpb: update kpb_copy_samples() to support 24b samples

96061c0

This patch updates function kpb_copy_samples() which copies samples from KPB source buffer to sink buffer to support 24b & 32b samples. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

kpb: forbid draining requests bigger than history_buffer

435a63c

This patch adds a check if history depth request is bigger than history buffer size. If so refuse to drain. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

kpb: fix states at prepare time

d9e3b5d

This patch switches KPB into RUN state after prepare. This is expected state once KPB was properly initialized. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

kpb: return -EIO if copy was called with wrong state

f8be2b2

This patch makes kpb_copy() to return -EIO if it was called while KPB was in wrong state. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

mrajwa force-pushed the kwd_kpb_merge branch from 8334b9a to f8be2b2 Compare July 30, 2019 16:05

jajanusz approved these changes Jul 30, 2019

View reviewed changes

jajanusz merged commit 33388bb into thesofproject:master Jul 30, 2019

marc-hb reviewed Mar 19, 2020

View reviewed changes

Conversation

mrajwa commented Jul 25, 2019

Uh oh!

jajanusz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jajanusz commented Jul 26, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrajwa commented Jul 30, 2019

Uh oh!

mrajwa commented Jul 30, 2019

Uh oh!

jajanusz left a comment

Choose a reason for hiding this comment

Uh oh!

jajanusz commented Jul 30, 2019

Uh oh!

mrajwa commented Jul 30, 2019

Uh oh!

mrajwa commented Jul 30, 2019

Uh oh!

jajanusz commented Jul 30, 2019

Uh oh!

marc-hb left a comment

Choose a reason for hiding this comment