Skip to content

kwd: improvements #1689

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
tlauda merged 11 commits into from
Sep 5, 2019
Merged

kwd: improvements #1689

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
3789289
kpb: improve timeout handling in kpb_buffer_data
mrajwa Jul 30, 2019
0796604
kpb: add current state log to reset function
mrajwa Jul 30, 2019
2e43318
kpb: improve log message in kpb_copy() function
mrajwa Jul 30, 2019
b21dc20
kpb: add internal status logger
mrajwa Jul 31, 2019
6d1ca1e
kpb: change state only via interface
mrajwa Jul 31, 2019
79a0548
kpb: add history of state changes to some crucial logs
mrajwa Jul 31, 2019
c031dd8
kpb: check if KPB's sinks have been found at prepare()
mrajwa Jul 31, 2019
49eb7c0
kpb: improve reset handling
mrajwa Aug 1, 2019
13a428c
kpb: fix buffering size during draining
mrajwa Aug 1, 2019
62e858f
kpb: log error and stop pipeline copy if no bytes to copy
mrajwa Aug 1, 2019
b286510
kpb: fix premature state change in buffering function
mrajwa Aug 27, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
151 changes: 107 additions & 44 deletions src/audio/kpb.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@

/* KPB private data, runtime data */
struct comp_data {
uint64_t state_log;

@tlauda tlauda Sep 2, 2019

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?

@mrajwa mrajwa Sep 5, 2019

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.

enum kpb_state state; /**< current state of KPB component */
uint32_t kpb_no_of_clients; /**< number of registered clients */
struct kpb_client clients[KPB_MAX_NO_OF_CLIENTS];
Expand Down Expand Up @@ -81,6 +82,8 @@ static void kpb_drain_samples(void *source, struct comp_buffer *sink,
static void kpb_reset_history_buffer(struct hb *buff);
static inline bool validate_host_params(size_t host_period_size,
size_t host_buffer_size);
static inline void kpb_change_state(struct comp_data *kpb,
enum kpb_state state);

/**
* \brief Create a key phrase buffer component.
Expand Down Expand Up @@ -145,10 +148,11 @@ static struct comp_dev *kpb_new(struct sof_ipc_comp *comp)
/* Init basic component data */
kpb->history_buffer = NULL;
kpb->kpb_no_of_clients = 0;
kpb->state_log = 0;

/* Kpb has been created successfully */
dev->state = COMP_STATE_READY;
kpb->state = KPB_STATE_CREATED;
kpb_change_state(kpb, KPB_STATE_CREATED);

return dev;
}
Expand Down Expand Up @@ -340,7 +344,8 @@ static int kpb_prepare(struct comp_dev *dev)
if (kpb->state == KPB_STATE_RESETTING ||
kpb->state == KPB_STATE_RESET_FINISHING) {
trace_kpb_error("kpb_prepare() error: can not prepare KPB "
"due to ongoing reset.");
"due to ongoing reset, state log %d",
kpb->state_log);
return -EBUSY;
}

Expand All @@ -352,13 +357,15 @@ static int kpb_prepare(struct comp_dev *dev)
return PPL_STATUS_PATH_STOP;

/* Init private data */
kpb->state = KPB_STATE_PREPARING;
kpb_change_state(kpb, KPB_STATE_PREPARING);
kpb->kpb_no_of_clients = 0;
kpb->buffered_data = 0;
kpb->host_buffer_size = dev->params.buffer.size;
kpb->host_period_size = dev->params.host_period_bytes;
kpb->config.sampling_width = dev->params.sample_container_bytes * 8;
kpb->kpb_buffer_size = KPB_MAX_BUFFER_SIZE(kpb->config.sampling_width);
kpb->sel_sink = NULL;
kpb->host_sink = NULL;

if (!kpb->history_buffer) {
/* Allocate history buffer */
Expand Down Expand Up @@ -419,7 +426,14 @@ static int kpb_prepare(struct comp_dev *dev)
}
}

kpb->state = KPB_STATE_RUN;
if (!kpb->sel_sink || !kpb->host_sink) {
trace_kpb("kpb_prepare() error: could not find "
"sinks: sel_sink %d host_sink %d",
(uint32_t)kpb->sel_sink, (uint32_t)kpb->host_sink);
Comment thread
tlauda marked this conversation as resolved.
Resolved
ret = -EIO;
}

kpb_change_state(kpb, KPB_STATE_RUN);

return ret;
}
Expand Down Expand Up @@ -452,38 +466,46 @@ static void kpb_cache(struct comp_dev *dev, int cmd)
static int kpb_reset(struct comp_dev *dev)
{
struct comp_data *kpb = comp_get_drvdata(dev);
int ret = 0;

trace_kpb("kpb_reset()");

kpb->buffered_data = 0;
kpb->is_internal_buffer_full = false;
trace_kpb("kpb_reset(): resetting from state %d, state log %d",
kpb->state, kpb->state_log);

/* Change KPB state to RESET. If there is any ongoing job it will
* shut itself gracefully first.
*/
if (kpb->state == KPB_STATE_BUFFERING ||
kpb->state == KPB_STATE_DRAINING) {
switch (kpb->state) {
case KPB_STATE_BUFFERING:
case KPB_STATE_DRAINING:
/* KPB is performing some task now,
* terminate it gently.
*/
kpb->state = KPB_STATE_RESETTING;
return -EBUSY;
}
kpb_change_state(kpb, KPB_STATE_RESETTING);
ret = -EBUSY;
break;
case KPB_STATE_DISABLED:
case KPB_STATE_CREATED:
/* Nothing to reset */
ret = comp_set_state(dev, COMP_TRIGGER_RESET);
break;
default:
kpb->buffered_data = 0;
kpb->is_internal_buffer_full = false;

if (kpb->history_buffer) {
/* Reset history buffer - zero its data reset pointers
* and states.
*/
kpb_reset_history_buffer(kpb->history_buffer);
}
if (kpb->history_buffer) {
/* Reset history buffer - zero its data, reset pointers
* and states.
*/
kpb_reset_history_buffer(kpb->history_buffer);
}

/* Unregister KPB for async notification */
notifier_unregister(&kpb->kpb_events);
/* Unregister KPB for async notification */
notifier_unregister(&kpb->kpb_events);
/* Finally KPB is ready after reset */
kpb_change_state(kpb, KPB_STATE_PREPARING);

/* Finally KPB is ready after reset */
kpb->state = KPB_STATE_PREPARING;
ret = comp_set_state(dev, COMP_TRIGGER_RESET);
break;
}

return comp_set_state(dev, COMP_TRIGGER_RESET);
return ret;
}

/**
Expand Down Expand Up @@ -533,6 +555,14 @@ static int kpb_copy(struct comp_dev *dev)
}

copy_bytes = MIN(sink->free, source->avail);
if (!copy_bytes) {

@tlauda tlauda Sep 2, 2019

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.

@mrajwa mrajwa Sep 5, 2019

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.

trace_kpb_error("kpb_copy() error: nothing to copy "
"sink->free %d source->avail %d",
sink->free, source->avail);
ret = PPL_STATUS_PATH_STOP;
goto out;
}

kpb_copy_samples(sink, source, copy_bytes, sample_width);

/* Buffer source data internally in history buffer for future
Expand Down Expand Up @@ -571,19 +601,27 @@ static int kpb_copy(struct comp_dev *dev)
}

copy_bytes = MIN(sink->free, source->avail);
if (!copy_bytes) {
trace_kpb_error("kpb_copy() error: nothing to copy "
"sink->free %d source->avail %d",
sink->free, source->avail);
ret = PPL_STATUS_PATH_STOP;
goto out;
}

kpb_copy_samples(sink, source, copy_bytes, sample_width);

comp_update_buffer_produce(sink, copy_bytes);
comp_update_buffer_consume(source, copy_bytes);

ret = 0;
ret = PPL_STATUS_PATH_STOP;

@tlauda tlauda Sep 2, 2019

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?

@mrajwa mrajwa Sep 5, 2019

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.

break;
case KPB_STATE_DRAINING:
/* In draining state we only buffer data in internal,
* history buffer.
*/
if (source->avail <= kpb->kpb_buffer_size) {
ret = kpb_buffer_data(dev, source, copy_bytes);
ret = kpb_buffer_data(dev, source, source->avail);
if (ret) {
trace_kpb_error("kpb_copy(): internal "
"buffering failed.");
Expand All @@ -598,7 +636,9 @@ static int kpb_copy(struct comp_dev *dev)
ret = PPL_STATUS_PATH_STOP;
break;
default:
trace_kpb_error("kpb_copy(): wrong state! Copy forbidden.");
trace_kpb_error("kpb_copy(): wrong state, copy forbidden. "
"(state %d, state log %d)",
kpb->state, kpb->state_log);
ret = -EIO;
break;
}
Expand All @@ -624,8 +664,8 @@ static int kpb_buffer_data(struct comp_dev *dev, struct comp_buffer *source,
struct comp_data *kpb = comp_get_drvdata(dev);
struct hb *buff = kpb->history_buffer;
void *read_ptr = source->r_ptr;
size_t timeout = platform_timer_get(platform_timer) +
clock_ms_to_ticks(PLATFORM_DEFAULT_CLOCK, 1);
uint64_t timeout = 0;
uint64_t current_time = 0;
enum kpb_state state_preserved = kpb->state;
struct dd *draining_data = &kpb->draining_task_data;

Expand All @@ -634,25 +674,31 @@ static int kpb_buffer_data(struct comp_dev *dev, struct comp_buffer *source,
if (kpb->state != KPB_STATE_RUN && kpb->state != KPB_STATE_DRAINING)
return PPL_STATUS_PATH_STOP;

kpb->state = KPB_STATE_BUFFERING;

if (kpb->state == KPB_STATE_DRAINING)
draining_data->buffered_while_draining += size_to_copy;

kpb_change_state(kpb, KPB_STATE_BUFFERING);

timeout = platform_timer_get(platform_timer) +
Comment thread
mrajwa marked this conversation as resolved.
Resolved
clock_ms_to_ticks(PLATFORM_DEFAULT_CLOCK, 1);
/* Let's store audio stream data in internal history buffer */
while (size_to_copy) {
/* Reset was requested, it's time to stop buffering and finish
* KPB reset.
*/
if (kpb->state == KPB_STATE_RESETTING) {
kpb->state = KPB_STATE_RESET_FINISHING;
kpb_change_state(kpb, KPB_STATE_RESET_FINISHING);
kpb_reset(dev);
return PPL_STATUS_PATH_STOP;
}

/* Are we stuck in buffering? */
if (timeout < platform_timer_get(platform_timer)) {
trace_kpb_error("kpb_buffer_data(): timeout.");
current_time = platform_timer_get(platform_timer);
if (timeout < current_time) {
trace_kpb_error("kpb_buffer_data(): timeout of %d [ms] "
"(current state %d, state log %d)",
(current_time - timeout), kpb->state,
kpb->state_log);
return -ETIME;
}

Expand Down Expand Up @@ -711,7 +757,7 @@ static int kpb_buffer_data(struct comp_dev *dev, struct comp_buffer *source,
}
}

kpb->state = state_preserved;
kpb_change_state(kpb, state_preserved);
return ret;
}

Expand Down Expand Up @@ -923,7 +969,6 @@ static void kpb_init_draining(struct comp_dev *dev, struct kpb_client *cli)
kpb->draining_task_data.sink = kpb->host_sink;
kpb->draining_task_data.history_buffer = buff;
kpb->draining_task_data.history_depth = history_depth;
kpb->draining_task_data.state = &kpb->state;
kpb->draining_task_data.sample_width = sample_width;
kpb->draining_task_data.drain_interval = drain_interval;
kpb->draining_task_data.pb_limit = period_bytes_limit;
Expand Down Expand Up @@ -974,18 +1019,19 @@ static enum task_state kpb_draining_task(void *arg)
size_t period_copy_start = platform_timer_get(platform_timer);
size_t time_taken = 0;
size_t *rt_stream_update = &draining_data->buffered_while_draining;
struct comp_data *kpb = comp_get_drvdata(draining_data->dev);

trace_kpb("kpb_draining_task(), start.");

/* Change KPB internal state to DRAINING */
*draining_data->state = KPB_STATE_DRAINING;
kpb_change_state(kpb, KPB_STATE_DRAINING);

draining_time_start = platform_timer_get(platform_timer);

while (history_depth > 0) {
/* Have we received reset request? */
if (*draining_data->state == KPB_STATE_RESETTING) {
*draining_data->state = KPB_STATE_RESET_FINISHING;
if (kpb->state == KPB_STATE_RESETTING) {
kpb_change_state(kpb, KPB_STATE_RESET_FINISHING);
kpb_reset(draining_data->dev);
goto out;
}
Expand Down Expand Up @@ -1059,8 +1105,8 @@ static enum task_state kpb_draining_task(void *arg)
* Note! If KPB state changed during draining due to i.e reset request
* we should not change that state.
*/
*draining_data->state = (*draining_data->state == KPB_STATE_DRAINING) ?
KPB_STATE_HOST_COPY : *draining_data->state;
if (kpb->state == KPB_STATE_DRAINING)
kpb_change_state(kpb, KPB_STATE_HOST_COPY);

/* Reset host-sink copy mode back to unblocking */
comp_set_attribute(sink->sink, COMP_ATTR_COPY_TYPE, &copy_type);
Expand Down Expand Up @@ -1247,6 +1293,23 @@ static inline bool validate_host_params(size_t host_period_size,

return (drained_per_interval > host_period_size) ? true : false;
}

/**
* \brief Change KPB state and log this change internally.
* \param[in] kpb - KPB component data pointer.
* \param[in] state - current KPB state.
*
* \return none.
*/
static inline void kpb_change_state(struct comp_data *kpb,
enum kpb_state state)
{
trace_kpb("kpb_change_state(): from %d to %d",
kpb->state, state);
kpb->state = state;
kpb->state_log = (kpb->state_log << 4) | state;
}

struct comp_driver comp_kpb = {
.type = SOF_COMP_KPB,
.ops = {
Expand Down
1 change: 0 additions & 1 deletion src/include/sof/audio/kpb.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,6 @@ struct dd {
struct hb *history_buffer;
size_t history_depth;
uint8_t is_draining_active;
enum kpb_state *state;
size_t sample_width;
size_t buffered_while_draining;
size_t drain_interval;
Expand Down