Skip to content

kwd: improvements - #1689

Merged
tlauda merged 11 commits into
thesofproject:masterfrom
mrajwa:ghw_dev
Sep 5, 2019
Merged

tlauda merged 11 commits into
thesofproject:masterfrom
mrajwa:ghw_dev

Conversation

@mrajwa

@mrajwa mrajwa commented Jul 30, 2019

Copy link
Copy Markdown
Contributor

This patch adds some improvements aimed to extend KPB logging
by providing informations like kpb state or timeout duration.

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

@mrajwa
mrajwa requested review from jajanusz and tlauda July 30, 2019 21:25
@michalgrodzicki michalgrodzicki added the P1 Blocker bugs or important features label Jul 30, 2019

@tlauda tlauda left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo in second commit message.

Comment thread src/audio/kpb.c Outdated
@tlauda

tlauda commented Aug 3, 2019

Copy link
Copy Markdown
Contributor

@mrajwa Can you fix the typo in second commit message, co we could merge this?

This patch changes the datatype of timeout variable and
adds more information into error log message. Also
we set up timeout limit right before we enter the while loop.

Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>
This patch improves kpb_reset() log message by adding the
state from which we start resetting.

Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>
This patch adds state to the error log message of
kpb_copy() function.

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

mrajwa commented Sep 2, 2019

Copy link
Copy Markdown
Contributor Author

@tlauda @lgirdwood @jajanusz , PR updated with new patches.

Comment thread src/audio/kpb.c Outdated
Comment thread src/audio/kpb.c Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need 64 bits?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wider data size gives us the possibility to log more states. So I would keep it as 64 bits.

Comment thread src/audio/kpb.c Outdated
Comment thread src/audio/kpb.c Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Space at the end not needed.

Comment thread src/audio/kpb.c Outdated
Comment thread src/audio/kpb.c Outdated
Comment thread src/audio/kpb.c Outdated
Comment thread src/audio/kpb.c Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The last sentence of the commit description seems weird.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, corrected now.

Comment thread src/audio/kpb.c Outdated
Comment thread src/audio/kpb.c Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it intentional change?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, during host copy we no longer need selector and detector components.

@mrajwa

mrajwa commented Sep 5, 2019

Copy link
Copy Markdown
Contributor Author

@tlauda, updated.

@tlauda

tlauda commented Sep 5, 2019

Copy link
Copy Markdown
Contributor

@mrajwa Can you fix the checkpatch?

This patch adds a container of KPB statuses.
So every time KPB changes its status it can
also log that change. In case of any error
this log of status changes can be read back
providing nice history of KPB actions.

Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>
This patch replaces all explicit kpb state changes
via a call to dedicated interface which in the same
time also logs the change in the internal logger.

Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>
This patch adds history of status changes into some
important error logs. Thanks to this, in case of some
serious failure we can check how KPB was changing its state
prior to failure.

Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>
This patch checks during prepare we managed to find KPB's
sinks.

Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>
This patch improves KPB reset handling by adding explicit
state handling.

Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>
This patch fixes the bug of zero buffering size during
draining.

Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>
This patch checks if "copy_bytes" which is the minimum
of sink->free and source->avail is != 0. If so we go
further with copy and internal buffering.
If no we log error message and return from
copy function.

Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>
This patch fixes state change in buffering function before
the check of state from previous stage. This issue resulted
in missing samples from stream during draining.

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

mrajwa commented Sep 5, 2019 •

Copy link
Copy Markdown
Contributor Author

@tlauda, sure I have just done it.

@tlauda
tlauda merged commit cd3de64 into thesofproject:master Sep 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P1 Blocker bugs or important features

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants