From f6ea8493b557327d99f8ae4e4727baec5332327e Mon Sep 17 00:00:00 2001 From: Guennadi Liakhovetski Date: Tue, 24 May 2022 12:03:59 +0200 Subject: [PATCH 1/3] dai: add a DAI operations copy method Add a DAI copy method to be called before dma_copy(). Signed-off-by: Guennadi Liakhovetski --- src/audio/dai-legacy.c | 3 +++ src/audio/dai-zephyr.c | 3 +++ src/include/sof/lib/dai.h | 1 + 3 files changed, 7 insertions(+) diff --git a/src/audio/dai-legacy.c b/src/audio/dai-legacy.c index 0f1f19cc2bcd..8bcf2281f1d1 100644 --- a/src/audio/dai-legacy.c +++ b/src/audio/dai-legacy.c @@ -952,6 +952,9 @@ static int dai_copy(struct comp_dev *dev) return 0; } + if (dd->dai->drv->ops.copy) + dd->dai->drv->ops.copy(dd->dai); + ret = dma_copy_legacy(dd->chan, copy_bytes, 0); if (ret < 0) { dai_report_xrun(dev, copy_bytes); diff --git a/src/audio/dai-zephyr.c b/src/audio/dai-zephyr.c index 654bd99c4e7d..801f91befb75 100644 --- a/src/audio/dai-zephyr.c +++ b/src/audio/dai-zephyr.c @@ -1052,6 +1052,9 @@ static int dai_copy(struct comp_dev *dev) return 0; } + if (dd->dai->drv->ops.copy) + dd->dai->drv->ops.copy(dd->dai); + struct dma_cb_data next = { .channel = dd->chan, .elem = { .size = copy_bytes }, diff --git a/src/include/sof/lib/dai.h b/src/include/sof/lib/dai.h index 4fa37897a9cf..34129e1fe128 100644 --- a/src/include/sof/lib/dai.h +++ b/src/include/sof/lib/dai.h @@ -86,6 +86,7 @@ struct dai_ops { int (*remove)(struct dai *dai); uint32_t (*get_init_delay_ms)(struct dai *dai); int (*get_fifo_depth)(struct dai *dai, int direction); + void (*copy)(struct dai *dai); /* Can be used by DAIs to prepare for data copying */ }; struct timestamp_cfg { From 4fc3de11436a048153f719f599a549efc5f34352 Mon Sep 17 00:00:00 2001 From: Guennadi Liakhovetski Date: Tue, 31 May 2022 14:27:11 +0200 Subject: [PATCH 2/3] dai: let DAI drivers access the underlying component object DAI drivers sometimes need to access the underlying component object. Make this possible by adding a pointer to struct dai_data into struct dai and to struct comp_dev into struct dai_data and by assigning those pointers accordingly. Signed-off-by: Guennadi Liakhovetski --- src/audio/dai-legacy.c | 1 + src/include/sof/lib/dai.h | 2 ++ src/ipc/ipc3/dai.c | 2 ++ src/ipc/ipc4/dai.c | 2 ++ 4 files changed, 7 insertions(+) diff --git a/src/audio/dai-legacy.c b/src/audio/dai-legacy.c index 8bcf2281f1d1..b00010b06a8d 100644 --- a/src/audio/dai-legacy.c +++ b/src/audio/dai-legacy.c @@ -179,6 +179,7 @@ static struct comp_dev *dai_new(const struct comp_driver *drv, comp_cl_err(&comp_dai, "dai_new(): dai_get() failed to create DAI."); goto error; } + dd->dai->dd = dd; dd->ipc_config = *dai; /* request GP LP DMA with shared access privilege */ diff --git a/src/include/sof/lib/dai.h b/src/include/sof/lib/dai.h index 34129e1fe128..07d1d959fce5 100644 --- a/src/include/sof/lib/dai.h +++ b/src/include/sof/lib/dai.h @@ -161,6 +161,7 @@ struct dai_data { #ifdef __ZEPHYR__ struct dma_config *z_config; #endif + struct comp_dev *dai_dev; struct comp_buffer *dma_buffer; struct comp_buffer *local_buffer; struct timestamp_cfg ts_config; @@ -199,6 +200,7 @@ struct dai { int sref; /**< simple ref counter, guarded by lock */ struct dai_plat_data plat_data; const struct dai_driver *drv; + const struct dai_data *dd; void *priv_data; }; diff --git a/src/ipc/ipc3/dai.c b/src/ipc/ipc3/dai.c index a054aa1e62e6..2e2ad0bde82f 100644 --- a/src/ipc/ipc3/dai.c +++ b/src/ipc/ipc3/dai.c @@ -284,6 +284,8 @@ int dai_config(struct comp_dev *dev, struct ipc_config_dai *common_config, return 0; } + dd->dai_dev = dev; + switch (config->flags & SOF_DAI_CONFIG_FLAGS_CMD_MASK) { case SOF_DAI_CONFIG_FLAGS_HW_PARAMS: /* set the delayed_dma_stop flag */ diff --git a/src/ipc/ipc4/dai.c b/src/ipc/ipc4/dai.c index f0e9a322837a..54b218612d10 100644 --- a/src/ipc/ipc4/dai.c +++ b/src/ipc/ipc4/dai.c @@ -266,6 +266,8 @@ int dai_config(struct comp_dev *dev, struct ipc_config_dai *common_config, if (dai_config_dma_channel(dev, spec_config) == DMA_CHAN_INVALID) return 0; + dd->dai_dev = dev; + /* allocated dai_config if not yet */ if (!dd->dai_spec_config) { size = sizeof(*copier_cfg); From 01d393732a51441e1dcde7915bad58224bee83e4 Mon Sep 17 00:00:00 2001 From: Guennadi Liakhovetski Date: Tue, 31 May 2022 14:42:07 +0200 Subject: [PATCH 3/3] dmic: move gain ramping to DAI .copy() context Currently the DMIC driver uses a separate task to implement its gain ramping. This creates a race during driver clean up. Avoid an additional asynchronous context by performing the ramping in DAI's .copy() context. Use the DAI's .copy() method for that. Signed-off-by: Guennadi Liakhovetski --- src/drivers/intel/dmic/dmic.c | 86 ++++++++++++++++------------------ src/include/sof/drivers/dmic.h | 5 +- 2 files changed, 43 insertions(+), 48 deletions(-) diff --git a/src/drivers/intel/dmic/dmic.c b/src/drivers/intel/dmic/dmic.c index c67bdc09ba28..07b9e4b02bc2 100644 --- a/src/drivers/intel/dmic/dmic.c +++ b/src/drivers/intel/dmic/dmic.c @@ -22,9 +22,6 @@ #include #include #include -#include -#include -#include #include #include #include @@ -43,10 +40,6 @@ DECLARE_SOF_UUID("dmic-dai", dmic_uuid, 0xaafc26fe, 0x3b8d, 0x498d, DECLARE_TR_CTX(dmic_tr, SOF_UUID(dmic_uuid), LOG_LEVEL_INFO); -/* 59c87728-d8f9-42f6-b89d-5870a87b0e1e */ -DECLARE_SOF_UUID("dmic-work", dmic_work_task_uuid, 0x59c87728, 0xd8f9, 0x42f6, - 0xb8, 0x9d, 0x58, 0x70, 0xa8, 0x7b, 0x0e, 0x1e); - /* Configuration ABI version, increment if not compatible with previous * version. */ @@ -59,16 +52,31 @@ static const uint32_t base[4] = {PDM0, PDM1, PDM2, PDM3}; static SHARED_DATA struct dmic_global_shared dmic_global; /* this ramps volume changes over time */ -static enum task_state dmic_work(void *data) +static void dmic_gain_ramp(struct dai *dai) { - struct dai *dai = (struct dai *)data; struct dmic_pdata *dmic = dai_get_drvdata(dai); k_spinlock_key_t key; int32_t gval; uint32_t val; int i; - dai_dbg(dai, "dmic_work()"); + /* Currently there's no DMIC HW internal mutings and wait times + * applied into this start sequence. It can be implemented here if + * start of audio capture would contain clicks and/or noise and it + * is not suppressed by gain ramp somewhere in the capture pipe. + */ + + dai_dbg(dai, "dmic_gain_ramp()"); + + /* + * At run-time dmic->gain is only changed in this function, and this + * function runs in the pipeline task context, so it cannot run + * concurrently on multiple cores, since there's always only one + * task associated with each DAI, so we don't need to hold the lock to + * read the value here. + */ + if (dmic->gain == DMIC_HW_FIR_GAIN_MAX << 11) + return; key = k_spin_lock(&dai->lock); @@ -87,8 +95,10 @@ static enum task_state dmic_work(void *data) * sets gain bypass mode in HW. Zero value will be applied after ramp * is complete. It is because exact 1.0 gain is not possible with Q1.19. */ - if (gval > DMIC_HW_FIR_GAIN_MAX) + if (gval > DMIC_HW_FIR_GAIN_MAX) { gval = 0; + dmic->gain = DMIC_HW_FIR_GAIN_MAX << 11; + } /* Write gain to registers */ for (i = 0; i < DMIC_HW_CONTROLLERS; i++) { @@ -127,13 +137,11 @@ static enum task_state dmic_work(void *data) k_spin_unlock(&dai->lock, key); - - return gval ? SOF_TASK_STATE_RESCHEDULE : SOF_TASK_STATE_COMPLETED; } /* get DMIC hw params */ static int dmic_get_hw_params(struct dai *dai, - struct sof_ipc_stream_params *params, int dir) + struct sof_ipc_stream_params *params, int dir) { #if CONFIG_INTEL_DMIC_TPLG_PARAMS return dmic_get_hw_params_computed(dai, params, dir); @@ -149,8 +157,6 @@ static int dmic_get_hw_params(struct dai *dai, static int dmic_set_config(struct dai *dai, struct ipc_config_dai *common_config, void *spec_config) { struct dmic_pdata *dmic = dai_get_drvdata(dai); - int32_t unmute_ramp_time_ms; - int32_t step_db; int ret = 0; int di = dai->index; k_spinlock_key_t key; @@ -197,13 +203,15 @@ static int dmic_set_config(struct dai *dai, struct ipc_config_dai *common_config * if it is non-zero, otherwise use default length. */ if (config->dmic.unmute_ramp_time) - unmute_ramp_time_ms = config->dmic.unmute_ramp_time; + dmic->unmute_ramp_time_ms = config->dmic.unmute_ramp_time; else - unmute_ramp_time_ms = dmic_get_unmute_ramp_from_samplerate(config->dmic.fifo_fs); + dmic->unmute_ramp_time_ms = dmic_get_unmute_ramp_from_samplerate( + config->dmic.fifo_fs); - if (unmute_ramp_time_ms < LOGRAMP_TIME_MIN_MS || - unmute_ramp_time_ms > LOGRAMP_TIME_MAX_MS) { - dai_err(dai, "dmic_set_config(): Illegal ramp time = %d", unmute_ramp_time_ms); + if (dmic->unmute_ramp_time_ms < LOGRAMP_TIME_MIN_MS || + dmic->unmute_ramp_time_ms > LOGRAMP_TIME_MAX_MS) { + dai_err(dai, "dmic_set_config(): Illegal ramp time = %d", + dmic->unmute_ramp_time_ms); ret = -EINVAL; goto out; } @@ -236,7 +244,7 @@ static int dmic_set_config(struct dai *dai, struct ipc_config_dai *common_config ret = dmic_set_config_nhlt(dai, spec_config); /* There's no unmute ramp duration in blob, so the default rate dependent is used. */ - unmute_ramp_time_ms = dmic_get_unmute_ramp_from_samplerate(dmic->dai_rate); + dmic->unmute_ramp_time_ms = dmic_get_unmute_ramp_from_samplerate(dmic->dai_rate); #else ret = -EINVAL; #endif @@ -246,10 +254,7 @@ static int dmic_set_config(struct dai *dai, struct ipc_config_dai *common_config goto out; } - /* Compute unmute ramp gain update coefficient. */ - step_db = LOGRAMP_CONST_TERM / unmute_ramp_time_ms; - dmic->gain_coef = db2lin_fixed(step_db); - dai_info(dai, "dmic_set_config(): unmute_ramp_time_ms = %d", unmute_ramp_time_ms); + dai_info(dai, "dmic_set_config(): unmute_ramp_time_ms = %d", dmic->unmute_ramp_time_ms); dmic->state = COMP_STATE_PREPARE; @@ -263,6 +268,7 @@ static void dmic_start(struct dai *dai) { struct dmic_pdata *dmic = dai_get_drvdata(dai); k_spinlock_key_t key; + int32_t step_db; int i; int mic_a; int mic_b; @@ -274,6 +280,14 @@ static void dmic_start(struct dai *dai) dai_dbg(dai, "dmic_start()"); dmic->startcount = 0; + /* + * Compute unmute ramp gain update coefficient, based on DAI processing + * period in microseconds. + */ + step_db = dai->dd->dai_dev->period * (int64_t)-LOGRAMP_START_DB / + (1000 * dmic->unmute_ramp_time_ms); + dmic->gain_coef = db2lin_fixed(step_db); + /* Initial gain value, convert Q12.20 to Q2.30 */ dmic->gain = Q_SHIFT_LEFT(db2lin_fixed(LOGRAMP_START_DB), 20, 30); @@ -369,16 +383,6 @@ static void dmic_start(struct dai *dai) dmic->state = COMP_STATE_ACTIVE; k_spin_unlock(&dai->lock, key); - /* Currently there's no DMIC HW internal mutings and wait times - * applied into this start sequence. It can be implemented here if - * start of audio capture would contain clicks and/or noise and it - * is not suppressed by gain ramp somewhere in the capture pipe. - */ - - schedule_task(&dmic->dmicwork, DMIC_UNMUTE_RAMP_US, - DMIC_UNMUTE_RAMP_US); - - dai_info(dai, "dmic_start(), dmic_active_fifos_mask = 0x%x", dmic->global->active_fifos_mask); } @@ -449,7 +453,6 @@ static void dmic_stop(struct dai *dai, bool stop_is_pause) } } - schedule_task_cancel(&dmic->dmicwork); k_spin_unlock(&dai->lock, key); } @@ -561,11 +564,6 @@ static int dmic_probe(struct dai *dai) return ret; } - /* Initialize start sequence handler */ - schedule_task_init_ll(&dmic->dmicwork, SOF_UUID(dmic_work_task_uuid), - SOF_SCHEDULE_LL_TIMER, - SOF_TASK_PRI_MED, dmic_work, dai, cpu_get_id(), 0); - /* Enable DMIC power */ pm_runtime_get_sync(DMIC_POW, dai->index); @@ -586,9 +584,6 @@ static int dmic_remove(struct dai *dai) interrupt_disable(dmic->irq, dai); interrupt_unregister(dmic->irq, dai); - /* remove scheduling */ - schedule_task_free(&dmic->dmicwork); - dai_info(dai, "dmic_remove(), dmic_active_fifos_mask = 0x%x, dmic_pause_mask = 0x%x", active_fifos_mask, pause_mask); dai_set_drvdata(dai, NULL); @@ -637,6 +632,7 @@ const struct dai_driver dmic_driver = { .get_fifo_depth = dmic_get_fifo_depth, .probe = dmic_probe, .remove = dmic_remove, + .copy = dmic_gain_ramp, }, .ts_ops = { .ts_config = timestamp_dmic_config, diff --git a/src/include/sof/drivers/dmic.h b/src/include/sof/drivers/dmic.h index 07b6167da862..f180dca86366 100644 --- a/src/include/sof/drivers/dmic.h +++ b/src/include/sof/drivers/dmic.h @@ -76,7 +76,6 @@ #include #include #include -#include #include /* Parameters used in modes computation */ @@ -472,12 +471,12 @@ struct dmic_global_shared { /* DMIC private data */ struct dmic_pdata { struct dmic_global_shared *global; /* Common data for all DMIC DAI instances */ - struct task dmicwork; /* HW gain ramp update task */ uint16_t enable[DMIC_HW_CONTROLLERS]; /* Mic 0 and 1 enable bits array for PDMx */ uint32_t state; /* Driver component state */ - int32_t startcount; /* Counter in dmicwork that controls HW unmute */ + int32_t startcount; /* Counter that controls HW unmute */ int32_t gain_coef; /* Gain update constant */ int32_t gain; /* Gain value to be applied to HW */ + int32_t unmute_ramp_time_ms; /* Unmute ramp time in milliseconds */ int irq; /* Interrupt number used */ enum sof_ipc_frame dai_format; /* PCM format s32_le etc. */ int dai_channels; /* Channels count */