kpb: add new state by mrajwa · Pull Request #2331 · thesofproject/sof

mrajwa · 2020-01-28T21:56:41Z

This adds new state KPB_STATE_INIT_DRAINING
into KPB.

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

lgirdwood

@mrajwa can you add some more context in the commit message as to why this is needed. i.e. are we fixing an issue.
Btw, why do we not use the standard component states here ?

mrajwa · 2020-01-29T10:14:38Z

@lgirdwood, @dbaluta this patch doesn't fix any issue, it just adds new state so we can have more detailed info in logs. Right now we will see a proper KPB state change like PREPARE -> RUN -> BUFFER -> INIT_DRAINING -> DRAINING -> ... -> RESET

Btw, why do we not use the standard component states here ?

Because these are private states of KPB and generic ones don't apply here.

dbaluta · 2020-01-29T10:48:36Z

@lgirdwood, @dbaluta this patch doesn't fix any issue, it just adds new state so we can have more detailed info in logs. Right now we will see a proper KPB state change like PREPARE -> RUN -> BUFFER -> INIT_DRAINING -> DRAINING -> ... -> RESET

@mrajwa Oh, I see. Please add this information in the commit message.

bkokoszx · 2020-01-31T10:20:39Z

@mrajwa
Do we really need to come back to preserved_state? I think we should stay in KPB_STATE_INIT_DRAINING (and add support in kpb_copy()) or change to KPB_STATE_DRAINING, because after init_draining() we do not have to copy data to sink (selector) buffer. Can you take a look at #2343? I've added there handling KPB_STATE_INIT_DRAINING_STATE in kpb_copy().

lgirdwood · 2020-01-31T10:23:31Z

@mrajwa @bkokoszx Please see my comments in #2343, it seems the same patch pushed to both PRs atm.

mrajwa · 2020-01-31T11:45:04Z

@mrajwa
Do we really need to come back to preserved_state? I think we should stay in KPB_STATE_INIT_DRAINING (and add support in kpb_copy()) or change to KPB_STATE_DRAINING, because after init_draining() we do not have to copy data to sink (selector) buffer. Can you take a look at #2343? I've added there handling KPB_STATE_INIT_DRAINING_STATE in kpb_copy().

I am against treating KPB_STATE_INIT_DRAINING and KPB_STATE_DRAINING as one state. This is unnatural and not true and will confuse people reading the code, logs etc. As for not switching back from KPB_STATE_INIT_DRAINING to preserved state - I think its sane to switch back as soon as init of draining is done since as I described above this state blocks all others (internal buffering being the most crucial one here)

lgirdwood

@mrajwa is the state diagram pending for this PR ?

mrajwa · 2020-02-06T20:23:50Z

@mrajwa is the state diagram pending for this PR ?

I have UML code for it also but the question is where should I commit it? Into sof-dosc? I see there is no parser for uml only picture source is linked.

lgirdwood · 2020-02-07T09:50:22Z

@mrajwa looks good, I'm no UML parsing expert for docs, so better ask @mmaka1. Can you convert this to ASCII from UML to insert into the header (if we cant do this then a big comment is needed stating that any state changes must also be reflected in the UML diagram). i.e. I want to make it easy for anyone who changes code to alos update diagram.

lgirdwood

I think we are almost good, lets just make it easier to follow for other developers.

lgirdwood · 2020-02-07T09:51:43Z

Lets just state the assumption about state in a comment here.

lgirdwood · 2020-02-07T09:52:51Z

and we can probably have a big fat comment here that any state changes must also be reflected in ASCII header diagram or UML diagram on sof-docs (with a link to said UML diagram).

Yes very good idea, so let me first finish UML merging and once this is done I will come back here with the update.

mrajwa · 2020-02-09T20:50:51Z

@lgirdwood, I tried to convert it to ASCII using plantuml built in feature but it doesn't look good. So I would include it in the sof dosc in the form of plantuml code and/or actual rendered picture.

keyonjie · 2020-02-10T02:39:55Z

@lgirdwood, I tried to convert it to ASCII using plantuml built in feature but it doesn't look good. So I would include it in the sof dosc in the form of plantuml code and/or actual rendered picture.

@mrajwa you could try asciiflow here: http://asciiflow.com/

mrajwa · 2020-02-10T09:14:24Z

@keyonjie, this tool seems to be aimed to draw ASCII diagrams and it might work however, I don't see any option to save it as a code and no option to generate a picture for sof-dosc. Especially the former concerns me since it will make our diagrams hard to maintain - each time you would like to add new state you would have to redraw it yourself or tinker with ASCII in your own...nightmare. Plantuml is based on their code, so each and every time you modify something it all boils down to changing specific line in code. Not to mention it generates a nice picture right away.

lgirdwood · 2020-02-10T11:07:52Z

@mrajwa ok, lets do it in sof-docs, as long as we have some comment that explains this with command line for using the tool to generate the output.

mrajwa · 2020-02-10T13:11:44Z

I have added a PR into sof-docs thesofproject/sof-docs#183 and added comments you requested in the FW. Can we merge now? I would like to merge it fast as I want to open new, bigger PR for KWD aimed to make it even more secure in corner cases.

This patch add a log message indicating wrong state of KPB during buffering. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

This adds new state KPB_STATE_INIT_DRAINING into KPB internal states. This way we can see failed init draining in logs. This new state is also needed to change KPBs regular behavior in a situation where we have initied draining task, therefore calculated read position but the task doesn't start before next pipeline_copy happens. In this case, having this new state we can recognize it and update read position accordingly to new data. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

mrajwa requested review from dbaluta, jajanusz, lbetlej, lgirdwood, mmaka1, plbossart and tlauda as code owners January 28, 2020 21:56

lgirdwood requested changes Jan 28, 2020

View reviewed changes

dbaluta reviewed Jan 29, 2020

View reviewed changes

Comment thread src/audio/kpb.c Resolved

dbaluta reviewed Jan 29, 2020

View reviewed changes

Comment thread src/audio/kpb.c Resolved

dbaluta reviewed Jan 29, 2020

View reviewed changes

Comment thread src/include/sof/audio/kpb.h Resolved

bkokoszx mentioned this pull request Jan 31, 2020

kpb: calculate free space in history buffer #2343

Closed

mrajwa force-pushed the ghw_dev branch 3 times, most recently from 701dd0e to 243ce18 Compare February 5, 2020 14:17

lgirdwood reviewed Feb 6, 2020

View reviewed changes

mrajwa force-pushed the ghw_dev branch from 243ce18 to 71f5ca4 Compare February 6, 2020 14:25

lgirdwood requested changes Feb 7, 2020

View reviewed changes

mrajwa force-pushed the ghw_dev branch from 71f5ca4 to 8af945d Compare February 10, 2020 13:01

mrajwa force-pushed the ghw_dev branch from 8af945d to c40e955 Compare February 10, 2020 13:07

lgirdwood approved these changes Feb 10, 2020

View reviewed changes

mrajwa force-pushed the ghw_dev branch 2 times, most recently from 1119860 to d30b561 Compare February 11, 2020 11:40

mrajwa added 2 commits February 13, 2020 10:56

kpb: add wrong state log into kpb_buffer_data

1b3ae8f

This patch add a log message indicating wrong state of KPB during buffering. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

mrajwa force-pushed the ghw_dev branch from d30b561 to e86a152 Compare February 13, 2020 10:01

jajanusz approved these changes Feb 13, 2020

View reviewed changes

lgirdwood merged commit 36d4c9f into thesofproject:master Feb 13, 2020

Conversation

mrajwa commented Jan 28, 2020

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mrajwa commented Jan 29, 2020

Uh oh!

dbaluta commented Jan 29, 2020

Uh oh!

bkokoszx commented Jan 31, 2020

Uh oh!

lgirdwood commented Jan 31, 2020

Uh oh!

mrajwa commented Jan 31, 2020

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

mrajwa commented Feb 6, 2020

Uh oh!

lgirdwood commented Feb 7, 2020

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

lgirdwood Feb 7, 2020

Choose a reason for hiding this comment

Uh oh!

lgirdwood Feb 7, 2020

Choose a reason for hiding this comment

Uh oh!

mrajwa Feb 9, 2020

Choose a reason for hiding this comment

Uh oh!

mrajwa commented Feb 9, 2020

Uh oh!

keyonjie commented Feb 10, 2020

Uh oh!

mrajwa commented Feb 10, 2020

Uh oh!

lgirdwood commented Feb 10, 2020

Uh oh!

mrajwa commented Feb 10, 2020

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants