KWD: merge with release branch by mrajwa · Pull Request #1462 · thesofproject/sof

mrajwa · 2019-05-21T12:56:42Z

This PR adds latest patches from KWD release branch glk-012-stable-branch into SoF master.
The functionality is tested and works properly. Sporadically we still observe #1406 issue.

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

keyonjie

Basically looks good to me, several comments inside the commits.

keyonjie · 2019-05-22T02:47:17Z

Can we rename this to sth easier to be understood? It is actually real-time copy(do in kpb_copy(), not in draining), not "draining" at all?

@keyonjie, I think it is now understandable and according to cAVS naming convention:
KPB_STATE_BUFFERING -> kpb is buffering incoming data into history buffer
KPB_STATE_DRAINING -> kpb is draining data out of internal history buffer to host (no buffering or any other copy is performed in this state)
KPB_STATE_DRAINING_ON_DEMAND -> kpb copies incoming, real time data directly to host (no buffering)

@mrajwa I assume that this change replaces #1435? I must agree with @keyonjie. KPB_STATE_DRAINING_ON_DEMAND sounds like some kind of different type of draining. KPB_STATE_COPYING makes much more sense. Also I'm not sure how are we addressing the problem with potential loss of data between start and end of draining. @lbetlej Was it agreed to fix it in the future PRs?

KPB_STATE_DRAINING_ON_DEMAND sounds like some kind of different type of draining

According to cAVS documentation it is another version of draining. Guys I just wanted to code according to that document so we are consistent across platforms. However since you feel like this is not very clear we can change KPB_STATE_DRAINING_ON_DEMAND to KPB_STATE_HOST_COPY would it be OK for you?

PS: I assume all other KPB state are fine for you and we only disscuss the KPB_STATE_DRAINING_ON_DEMAND ?

@mrajwa That would be OK.

keyonjie · 2019-05-22T02:55:30Z

Here the naming "keyphrase_length" sounds more reasonable to me. :P

Or "keyphrase_ms"?

Here the naming "keyphrase_length" sounds more reasonable to me. :P

Hah really? To me "keyphrase length" is just confusing. It suggest how much buffered data we want to get which is what history_depth does. preamble_time however clearly says "After what time detection shall be activated?"

@lgirdwood what do you think?

@mrajwa do you mean that those data before preamble_time will be ignored and dropped?

@keyonjie, they are ignored but not dropped - they will always be stored in history buffer inside KPB but detector won't trigger detection before preamble time is over.

keyonjie · 2019-05-22T03:06:54Z

Here you are adding more and more dependencies to kpb? @slawblauciak what do you think about this?

We could discuss if it is good or bad however I decided to set this default preamble to 0 as this seems to be more natural. By default you expect KWD to be active from start, right?

keyonjie · 2019-05-22T03:14:07Z

How shall we handle it once we are running with 24bit captured data? better to get it from fmt?

The support for more sampling options will be provided as separate patch.

jajanusz

Code itself looks ok, but do we have change for uapi/* headers in kernel?

lgirdwood

@mrajwa I stopped reviewing at patch 2 as I had no idea what each patch was trying to achieve. Please use the commit messages to explain what, why and how.

mrajwa · 2019-05-29T21:58:47Z

Code itself looks ok, but do we have change for uapi/* headers in kernel?

@jajanusz, no we don't have to update kernel side.

lgirdwood

Some commits still missing a commit message. Memory commit needs removed.

mrajwa · 2019-05-30T09:46:25Z

@lgirdwood, this PR has been updated. Please review and lets merge it as it is the base for further 24 bit support.

tlauda · 2019-05-30T10:22:36Z

@mrajwa I assume that this change replaces #1435? I must agree with @keyonjie. KPB_STATE_DRAINING_ON_DEMAND sounds like some kind of different type of draining. KPB_STATE_COPYING makes much more sense. Also I'm not sure how are we addressing the problem with potential loss of data between start and end of draining. @lbetlej Was it agreed to fix it in the future PRs?

tlauda · 2019-05-30T10:30:22Z

It is highly desirable to use buffer_write_frag API here to handle potential buffer wrap-ups.

Yes I agree, added function which do care about it. Once this PR is merged I will create some container which keeps different copy function for different format and they will be selected by looking up cd->config.sampling_width

tlauda · 2019-05-31T08:17:05Z

@mrajwa That would be OK.

tlauda · 2019-05-31T08:24:45Z

Could we change function names to be aligned with the rest of processing components?
Also I don't think we need to specify number of channels, since your functions are universal.
kpb_copy_s16
kpb_drain_s16

I have yet another idea, lets call them kpb_copy_samples and kpb_drain_samples and inside these function decide what buffer_read/write API to choose by format. Although for this PR selecting different samples format is out of scope as it is will be covered in different PR.

tlauda · 2019-05-31T08:32:59Z

Is this really needed? I thought we are returning PPL_STATUS_PATH_STOP on draining. I would really like to avoid setting component's state internally without kernel's knowledge. I guess detect pipeline should be stopped explicitly by the host on received notification? @lbetlej Any thoughts?

I agree this setting here is redundant as well as kernel taking care of it. The only concern I have is timing. I mean when we get FW notification about detection we can stop further copy imediately inside FW. Kernel on the other hand first need to wait for IPC about notification, then send another one aimed to stop some pipeline.

tlauda · 2019-06-06T08:33:40Z

@lgirdwood @plbossart uapi/user/detect_test.h is changed, so it looks like ABI should be incremented, but I'm a little bit confused. Kernel apparently doesn't use uapi/user/* headers at all, so from kernel's perspective it is backward compatible. However any user client using this won't be, so do we need another ABI?

lgirdwood · 2019-06-06T11:09:30Z

@tlauda we use same ABI for usespace, that way it can check if both kernel and FW support the feature. Likewise FW and kernel can check userspace for ABI feature alignment based on the same data.

mrajwa · 2019-06-06T11:23:06Z

@tlauda, I am bit confused why would you like to introduce another ABI? The new field used in uapi/user/detect_test.h is used only inside FW and passed from outside in form of topology file right?

tlauda · 2019-06-06T11:24:14Z

@mrajwa Because now you need to create PR for kernel just to increase ABI number, which will look totally weird.

jajanusz · 2019-06-06T11:28:09Z

@lgirdwood @tlauda @mrajwa
If we are going keep 1 ABI version, then yes, you are going to have PRs to kernel that just bump abi version and provide no new content, if headers were just for userspace apps.

But I don't know if having 2 versions is better.

mrajwa · 2019-06-06T12:38:03Z

So, what should I do next, to me opening a PR in kernel repository which only update ABI version doesn't sound reasonable. @lgirdwood, @plbossart, what do you think?

lgirdwood · 2019-06-06T20:43:56Z

@mrajwa no need to update kernel if this is user only. I've assigned MINOR 8. Not need to bump ABI as there are other PRs pending for MINOR 8 (last PR merged will bump)

This patch adds new state KPB_STATE_DRAINING. In this state kpb drains data from internal history buffer to host sink bypassig any other component. In the same time internal buffering is turned off. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

Detector was using keyphrase_length to: 1. delay detection 2. to calculate the amount of data that shall be drained from history buffer. This was confusing people, therefore we use two separate variables name with better naming. 1. history_depth -> to define default draining request. (in bytes) 2. preamble_time -> to define delay after which detection should be active. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

This patch provides default parameters for newly added detect_preamble and history_depth. Both by default are set to max buffering time of KPB. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

Remove history_begin and history_end members from kpb as they are not used anywhere. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

This patch adds two new trace messages into draining task - the amount of drained data and time needed to drain them in milliseconds. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

Remove unnecessary check of preamble time. Previously it was allowing preamble time smaller than kpb buffering time. There is no reason for such a constrain. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

With this patch default preamble is set to zero meaning KWD is activated from start, no delay. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

mrajwa · 2019-06-10T08:23:17Z

@lgirdwood @tlauda can we merge this PR? This week we have another KWD release and this is prerequisite.

tlauda

Ready for approval after update of function descriptions.

memcpy function doesn't care about "wrap-ups" and sample sizes which are crucial in copy of audio signals. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

Kpb rt_sink is confusing and was replaced with sel_sink which stands for selector sink - the component to which KPB copies during BUFFERING state. Likewise the cli_sink was replaced to host_sink clearly stating that we copy out of DSP. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

After detection of keyword we shall stop further copy to selector pipeline. We do this in regular copy() function so there is no need to do it also in init_draining() function. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

mrajwa · 2019-06-10T10:19:53Z

@tlauda, I have just updated.

The CLK_CPU macro is undefined for imx8 platform therefore shall be added. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

lgirdwood · 2019-06-10T14:10:20Z

@mrajwa UT failing.

KPB source code started to use timer related features therefore cmocka tests need to include these dependencies. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

mrajwa · 2019-06-11T08:52:54Z

@lgirdwood, updated.

mrajwa requested review from a team, lbetlej, lgirdwood, mmaka1 and plbossart as code owners May 21, 2019 12:56

keyonjie reviewed May 22, 2019

View reviewed changes

jajanusz reviewed May 22, 2019

View reviewed changes

lgirdwood requested changes May 22, 2019

View reviewed changes

mrajwa mentioned this pull request May 29, 2019

kwd: add support for 24 bit samples #1492

Merged

wenqingfu requested a review from singalsu May 29, 2019 22:02

mrajwa force-pushed the ghw_release branch from 3b687f2 to 281499f Compare May 29, 2019 22:31

lgirdwood requested changes May 30, 2019

View reviewed changes

Comment thread src/platform/apollolake/include/platform/memory.h Resolved

Comment thread src/audio/kpb.c Resolved

mrajwa force-pushed the ghw_release branch from 281499f to 691f47d Compare May 30, 2019 09:43

tlauda suggested changes May 30, 2019

View reviewed changes

mrajwa force-pushed the ghw_release branch from 691f47d to 574c033 Compare May 30, 2019 12:42

tlauda suggested changes May 31, 2019

View reviewed changes

tlauda mentioned this pull request May 31, 2019

kpb: stop pipeline copy at draining #1435

Closed

mrajwa force-pushed the ghw_release branch from 574c033 to 8917525 Compare June 5, 2019 20:57

lgirdwood added the ABI ABI change involved label Jun 6, 2019

mrajwa added 2 commits June 7, 2019 12:06

mrajwa added 5 commits June 7, 2019 12:36

topology: provide default config for new detector params

5c6c90c

This patch provides default parameters for newly added detect_preamble and history_depth. Both by default are set to max buffering time of KPB. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

detector: remove unused notification data

b69b47a

Remove history_begin and history_end members from kpb as they are not used anywhere. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

kpb: add additional log message into draining task

1517583

This patch adds two new trace messages into draining task - the amount of drained data and time needed to drain them in milliseconds. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

detector: remove unnecessary preamble time verification

ab40a04

Remove unnecessary check of preamble time. Previously it was allowing preamble time smaller than kpb buffering time. There is no reason for such a constrain. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

detector: set default preamble time to 0

1fec568

With this patch default preamble is set to zero meaning KWD is activated from start, no delay. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

mrajwa force-pushed the ghw_release branch from 8917525 to 9d98797 Compare June 7, 2019 11:13

tlauda suggested changes Jun 10, 2019

View reviewed changes

Comment thread src/audio/kpb.c Resolved

Comment thread src/audio/kpb.c Resolved

Comment thread src/audio/kpb.c Resolved

mrajwa added 3 commits June 10, 2019 12:16

kpb: replace draining and copy *memcpy* use with safer method

a652651

memcpy function doesn't care about "wrap-ups" and sample sizes which are crucial in copy of audio signals. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

mrajwa force-pushed the ghw_release branch from 9d98797 to 649bfc9 Compare June 10, 2019 10:17

tlauda approved these changes Jun 10, 2019

View reviewed changes

platform: include clk.h in platform.h for imx8 target.

bc4887f

The CLK_CPU macro is undefined for imx8 platform therefore shall be added. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

lgirdwood approved these changes Jun 10, 2019

View reviewed changes

jajanusz approved these changes Jun 10, 2019

View reviewed changes

cmocka: update KPB tests for new dependencies

508e245

KPB source code started to use timer related features therefore cmocka tests need to include these dependencies. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

tlauda merged commit fb111cd into thesofproject:master Jun 11, 2019

Conversation

mrajwa commented May 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keyonjie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrajwa May 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

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!

Uh oh!

Uh oh!

jajanusz left a comment

Choose a reason for hiding this comment

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

mrajwa commented May 29, 2019

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mrajwa commented May 30, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

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

mrajwa commented May 21, 2019 •

edited

Loading

mrajwa May 29, 2019 •

edited

Loading