Skip to content

Drivers: Intel: DMIC: Fix FW panic with multiple DAI capture, pausing, stopping and resuming #4313

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
lgirdwood merged 3 commits into from
Jun 8, 2021
Merged

Drivers: Intel: DMIC: Fix FW panic with multiple DAI capture, pausing, stopping and resuming #4313

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a1e2ec7
Drivers: Intel: DMIC: Change active_fifos_mask to uint32_t
singalsu Jun 8, 2021
1955ec3
Drivers: Intel: DMIC: Fix FW panic caused by too eager dmic_remove()
singalsu Jun 8, 2021
96e144e
Drivers: Intel: DMIC: Spinlocks notes add
singalsu Jun 8, 2021
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
43 changes: 30 additions & 13 deletions src/drivers/intel/dmic.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,8 @@ static const uint32_t coef_base_b[4] = {PDM0_COEFFICIENT_B, PDM1_COEFFICIENT_B,

/* Global configuration request for DMIC, need to use uncached address to access them */
static SHARED_DATA struct sof_ipc_dai_dmic_params *dmic_prm[DMIC_HW_FIFOS];
static SHARED_DATA int dmic_active_fifos_mask;
static SHARED_DATA uint32_t dmic_active_fifos_mask;
Comment thread
lgirdwood marked this conversation as resolved.
Resolved
static SHARED_DATA uint32_t dmic_pause_mask;

/* this ramps volume changes over time */
static enum task_state dmic_work(void *data)
Expand Down Expand Up @@ -913,6 +914,9 @@ static int configure_registers(struct dai *dai,
return ret;
}

/* Note about accessing dmic_active_fifos_mask: the dai spinlock has been set in
* the calling function dmic_set_config().
*/
for (i = 0; i < DMIC_HW_CONTROLLERS; i++) {
if (uncached_dmic_active_fifos_mask == 0) {
/* CIC */
Expand Down Expand Up @@ -1292,7 +1296,8 @@ static int dmic_set_config(struct dai *dai, struct ipc_config_dai *common_config
static void dmic_start(struct dai *dai)
{
struct sof_ipc_dai_dmic_params **uncached_dmic_prm = cache_to_uncache(&dmic_prm[0]);
int *uncached_dmic_active_fifos_mask = cache_to_uncache(&dmic_active_fifos_mask);
uint32_t *uncached_dmic_active_fifos_mask = cache_to_uncache(&dmic_active_fifos_mask);
uint32_t *uncached_dmic_pause_mask = cache_to_uncache(&dmic_pause_mask);
struct dmic_pdata *dmic = dai_get_drvdata(dai);
int i;
int mic_a;
Expand Down Expand Up @@ -1401,8 +1406,9 @@ static void dmic_start(struct dai *dai)
CIC_CONTROL_SOFT_RESET_BIT, 0);
}

/* Set bit dai->index */
/* Set bit dai->index for active FIFO, and clear pause bit */
*uncached_dmic_active_fifos_mask |= BIT(dai->index);
*uncached_dmic_pause_mask &= ~BIT(dai->index);

dmic->state = COMP_STATE_ACTIVE;
spin_unlock(&dai->lock);
Expand Down Expand Up @@ -1439,10 +1445,11 @@ static void dmic_stop_fifo_packers(struct dai *dai, int fifo_index)
}

/* stop the DMIC for capture */
static void dmic_stop(struct dai *dai)
static void dmic_stop(struct dai *dai, bool stop_is_pause)
{
struct dmic_pdata *dmic = dai_get_drvdata(dai);
int *uncached_dmic_active_fifos_mask = cache_to_uncache(&dmic_active_fifos_mask);
uint32_t *uncached_dmic_active_fifos_mask = cache_to_uncache(&dmic_active_fifos_mask);
uint32_t *uncached_dmic_pause_mask = cache_to_uncache(&dmic_pause_mask);
int i;

dai_dbg(dai, "dmic_stop()");
Expand All @@ -1455,8 +1462,14 @@ static void dmic_stop(struct dai *dai)
dai_info(dai, "dmic_stop(), dmic_active_fifos_mask = 0x%x",
*uncached_dmic_active_fifos_mask);

/* Clear bit dai->index */
/* Clear bit dai->index for active FIFO. If stop for pause, set pause mask bit.
* If stop is not for pausing, it is safe to clear the pause bit.
*/
*uncached_dmic_active_fifos_mask &= ~BIT(dai->index);
if (stop_is_pause)
*uncached_dmic_pause_mask |= BIT(dai->index);
else
*uncached_dmic_pause_mask &= ~BIT(dai->index);

for (i = 0; i < DMIC_HW_CONTROLLERS; i++) {
/* Don't stop CIC yet if one FIFO remains active */
Expand Down Expand Up @@ -1529,11 +1542,11 @@ static int dmic_trigger(struct dai *dai, int cmd, int direction)
break;
case COMP_TRIGGER_STOP:
dmic->state = COMP_STATE_PREPARE;
dmic_stop(dai);
dmic_stop(dai, false);
break;
case COMP_TRIGGER_PAUSE:
dmic->state = COMP_STATE_PAUSED;
dmic_stop(dai);
dmic_stop(dai, true);
break;
case COMP_TRIGGER_RESUME:
dmic_context_restore(dai);
Expand Down Expand Up @@ -1633,7 +1646,8 @@ static int dmic_probe(struct dai *dai)
static int dmic_remove(struct dai *dai)
{
struct sof_ipc_dai_dmic_params **uncached_dmic_prm = cache_to_uncache(&dmic_prm[0]);
int uncached_dmic_active_fifos_mask = *cache_to_uncache(&dmic_active_fifos_mask);
uint32_t uncached_dmic_active_fifos_mask = *cache_to_uncache(&dmic_active_fifos_mask);
uint32_t uncached_dmic_pause_mask = *cache_to_uncache(&dmic_pause_mask);
struct dmic_pdata *dmic = dai_get_drvdata(dai);
int i;

Expand All @@ -1648,10 +1662,13 @@ static int dmic_remove(struct dai *dai)
interrupt_disable(dmic->irq, dai);
interrupt_unregister(dmic->irq, dai);

/* The next end tasks must be passed if another DAI FIFO still runs */
dai_info(dai, "dmic_remove(), dmic_active_fifos_mask = 0x%x",
uncached_dmic_active_fifos_mask);
if (uncached_dmic_active_fifos_mask)
/* The next end tasks must be passed if another DAI FIFO still runs.
* Note: dai_put() function that calls remove() applies the spinlock
* so it is not needed here to protect access to mask bits.
*/
dai_info(dai, "dmic_remove(), dmic_active_fifos_mask = 0x%x, dmic_pause_mask = 0x%x",
uncached_dmic_active_fifos_mask, uncached_dmic_pause_mask);
if (uncached_dmic_active_fifos_mask || uncached_dmic_pause_mask)
return 0;

pm_runtime_put_sync(DMIC_CLK, dai->index);

@kv2019i kv2019i Jun 8, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is really problematic. There's no actual reference counting in cavs_pm_runtime_dis_dmic_clk_gating() and the "dai->index" passed to pm_runtime_put_sync() is just printed out.

The problem is that if we switch to pm_runtime_put_sync() that actual does refcounting, this code will break. FYI @lgirdwood @lyakh @marc-hb

Oh well, this goes beyond this PR's scope.

@lgirdwood lgirdwood Jun 8, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Lets get the ref counting in the PM code. This PM code should also be able to transfer to Zephyr with minimal refactoring.

@lgirdwood lgirdwood Jun 8, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

it looks like we may need some locking around the mask manipulations .

@singalsu singalsu Jun 8, 2021

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I added 3rd commit about spinlocks. I was testing spinlock add in dmic_remove() but it froze. It turned out that dai_put() had already applied spinlock. I also found out that the registers write function was already covered by spinlock in set_config() so the patch adds only comments about spinlocks applied.

Expand Down