[PATCH 0/5] ASoC: Intel: catpt: Offload fixes and code optimization
First two of the series address bugs connected mainly to offload streams: - scenarios with very low buffer sizes: RESET_STREAM IPC timeouts - fix lp clock selection when switching between PAUSE <-> RESUME states: glitches on first offload when no additional stream is opened simultaneously
Follow ups are: code reduction and optimization oriented patches. This has been foretold in:
[PATCH v10 00/14] ASoC: Intel: Catpt - Lynx and Wildcat point https://www.spinics.net/lists/alsa-devel/msg116440.html Note: LPT power up/down sequences might get aligned with WPT once enough testing is done as capabilities are shared for both DSPs.
First, optimize applying of user settings - prevent redundand calls from happening - and then as mentioned above, streamline power on/off sequence for LPT and WPT.
Cezary Rojewski (5): ASoC: Intel: catpt: Skip position update for unprepared streams ASoC: Intel: catpt: Correct clock selection for dai trigger ASoC: Intel: catpt: Optimize applying user settings ASoC: Intel: catpt: Streamline power routines across LPT and WPT ASoC: Intel: catpt: Cleanup after power routines streamlining
sound/soc/intel/catpt/core.h | 10 ++- sound/soc/intel/catpt/device.c | 18 +++--- sound/soc/intel/catpt/dsp.c | 56 ++-------------- sound/soc/intel/catpt/pcm.c | 113 ++++++++++++++++----------------- 4 files changed, 74 insertions(+), 123 deletions(-)
Playing with very low period sizes may lead to timeouts when awaiting RESET_STREAM reply for offload streams. This is caused by NOTIFY_POSITION appearing in the middle of trigger(stop).
Stream is unprepared during trigger(stop) where PAUSE_STREAM IPC gets invoked. However, all data that is already mixed in DSP firmware's mixer stream will still be played regardless of the pause. For offload streams, this means possibility for another NOTIFY_POSITION to process. Keep these notifications in check by only handling them when stream is in prepared state.
Fixes: a126750fc865 ("ASoC: Intel: catpt: PCM operations") Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- sound/soc/intel/catpt/pcm.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/sound/soc/intel/catpt/pcm.c b/sound/soc/intel/catpt/pcm.c index ba653ebea7d1..afdf6be605aa 100644 --- a/sound/soc/intel/catpt/pcm.c +++ b/sound/soc/intel/catpt/pcm.c @@ -534,6 +534,8 @@ void catpt_stream_update_position(struct catpt_dev *cdev,
dsppos = bytes_to_frames(r, pos->stream_position);
+ if (!stream->prepared) + goto exit; /* only offload is set_write_pos driven */ if (stream->template->type != CATPT_STRM_TYPE_RENDER) goto exit;
During stream start DSP firmware requires LPCS disabled as that moment in time is resource heavy. Currently high-clock is selected on start of second stream onwards while low-clock is re-selected before stream actually leaves RESUME state i.e. PAUSE_STREAM call. Fix this by always updating clock before RESUME_STREAM and directly after PAUSE_STREAM.
Fixes: a126750fc865 ("ASoC: Intel: catpt: PCM operations") Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- sound/soc/intel/catpt/pcm.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/sound/soc/intel/catpt/pcm.c b/sound/soc/intel/catpt/pcm.c index afdf6be605aa..408e64e3b5fb 100644 --- a/sound/soc/intel/catpt/pcm.c +++ b/sound/soc/intel/catpt/pcm.c @@ -458,10 +458,6 @@ static int catpt_dai_prepare(struct snd_pcm_substream *substream, if (ret) return CATPT_IPC_ERROR(ret);
- ret = catpt_dsp_update_lpclock(cdev); - if (ret) - return ret; - ret = catpt_dai_apply_usettings(dai, stream); if (ret) return ret; @@ -500,6 +496,7 @@ static int catpt_dai_trigger(struct snd_pcm_substream *substream, int cmd, case SNDRV_PCM_TRIGGER_RESUME: case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: resume_stream: + catpt_dsp_update_lpclock(cdev); ret = catpt_ipc_resume_stream(cdev, stream->info.stream_hw_id); if (ret) return CATPT_IPC_ERROR(ret); @@ -507,11 +504,11 @@ static int catpt_dai_trigger(struct snd_pcm_substream *substream, int cmd,
case SNDRV_PCM_TRIGGER_STOP: stream->prepared = false; - catpt_dsp_update_lpclock(cdev); fallthrough; case SNDRV_PCM_TRIGGER_SUSPEND: case SNDRV_PCM_TRIGGER_PAUSE_PUSH: ret = catpt_ipc_pause_stream(cdev, stream->info.stream_hw_id); + catpt_dsp_update_lpclock(cdev); if (ret) return CATPT_IPC_ERROR(ret); break;
Initial user settings such as volume control need to be applied only once after stream is allocated. As prepare() operation can be invoked multiple times during the stream's lifetime, relocate catpt_dai_apply_usettings() and call it directly within catpt_dai_hw_params() rather than on every catpt_dai_prepare().
catpt_dai_apply_usettings() remains unchanged.
Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- sound/soc/intel/catpt/pcm.c | 104 ++++++++++++++++++------------------ 1 file changed, 52 insertions(+), 52 deletions(-)
diff --git a/sound/soc/intel/catpt/pcm.c b/sound/soc/intel/catpt/pcm.c index 408e64e3b5fb..10b483695ade 100644 --- a/sound/soc/intel/catpt/pcm.c +++ b/sound/soc/intel/catpt/pcm.c @@ -324,6 +324,54 @@ static void catpt_dai_shutdown(struct snd_pcm_substream *substream, snd_soc_dai_set_dma_data(dai, substream, NULL); }
+static int catpt_set_dspvol(struct catpt_dev *cdev, u8 stream_id, long *ctlvol); + +static int catpt_dai_apply_usettings(struct snd_soc_dai *dai, + struct catpt_stream_runtime *stream) +{ + struct catpt_dev *cdev = dev_get_drvdata(dai->dev); + struct snd_soc_component *component = dai->component; + struct snd_kcontrol *pos, *kctl = NULL; + const char *name; + int ret; + u32 id = stream->info.stream_hw_id; + + /* only selected streams have individual controls */ + switch (id) { + case CATPT_PIN_ID_OFFLOAD1: + name = "Media0 Playback Volume"; + break; + case CATPT_PIN_ID_OFFLOAD2: + name = "Media1 Playback Volume"; + break; + case CATPT_PIN_ID_CAPTURE1: + name = "Mic Capture Volume"; + break; + case CATPT_PIN_ID_REFERENCE: + name = "Loopback Mute"; + break; + default: + return 0; + }; + + list_for_each_entry(pos, &component->card->snd_card->controls, list) { + if (pos->private_data == component && + !strncmp(name, pos->id.name, sizeof(pos->id.name))) { + kctl = pos; + break; + } + } + if (!kctl) + return -ENOENT; + + if (stream->template->type != CATPT_STRM_TYPE_LOOPBACK) + return catpt_set_dspvol(cdev, id, (long *)kctl->private_value); + ret = catpt_ipc_mute_loopback(cdev, id, *(bool *)kctl->private_value); + if (ret) + return CATPT_IPC_ERROR(ret); + return 0; +} + static int catpt_dai_hw_params(struct snd_pcm_substream *substream, struct snd_pcm_hw_params *params, struct snd_soc_dai *dai) @@ -370,6 +418,10 @@ static int catpt_dai_hw_params(struct snd_pcm_substream *substream, if (ret) return CATPT_IPC_ERROR(ret);
+ ret = catpt_dai_apply_usettings(dai, stream); + if (ret) + return ret; + stream->allocated = true; return 0; } @@ -391,54 +443,6 @@ static int catpt_dai_hw_free(struct snd_pcm_substream *substream, return 0; }
-static int catpt_set_dspvol(struct catpt_dev *cdev, u8 stream_id, long *ctlvol); - -static int catpt_dai_apply_usettings(struct snd_soc_dai *dai, - struct catpt_stream_runtime *stream) -{ - struct catpt_dev *cdev = dev_get_drvdata(dai->dev); - struct snd_soc_component *component = dai->component; - struct snd_kcontrol *pos, *kctl = NULL; - const char *name; - int ret; - u32 id = stream->info.stream_hw_id; - - /* only selected streams have individual controls */ - switch (id) { - case CATPT_PIN_ID_OFFLOAD1: - name = "Media0 Playback Volume"; - break; - case CATPT_PIN_ID_OFFLOAD2: - name = "Media1 Playback Volume"; - break; - case CATPT_PIN_ID_CAPTURE1: - name = "Mic Capture Volume"; - break; - case CATPT_PIN_ID_REFERENCE: - name = "Loopback Mute"; - break; - default: - return 0; - }; - - list_for_each_entry(pos, &component->card->snd_card->controls, list) { - if (pos->private_data == component && - !strncmp(name, pos->id.name, sizeof(pos->id.name))) { - kctl = pos; - break; - } - } - if (!kctl) - return -ENOENT; - - if (stream->template->type != CATPT_STRM_TYPE_LOOPBACK) - return catpt_set_dspvol(cdev, id, (long *)kctl->private_value); - ret = catpt_ipc_mute_loopback(cdev, id, *(bool *)kctl->private_value); - if (ret) - return CATPT_IPC_ERROR(ret); - return 0; -} - static int catpt_dai_prepare(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) { @@ -458,10 +462,6 @@ static int catpt_dai_prepare(struct snd_pcm_substream *substream, if (ret) return CATPT_IPC_ERROR(ret);
- ret = catpt_dai_apply_usettings(dai, stream); - if (ret) - return ret; - stream->prepared = true; return 0; }
There is no need for separate power on/off routines for LPT and WPT as as the protocol is shared for both platforms. Make WPT routines generic and reuse them in LPT case too.
Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- sound/soc/intel/catpt/core.h | 6 ++++-- sound/soc/intel/catpt/device.c | 18 +++++++++--------- sound/soc/intel/catpt/dsp.c | 10 +++++----- 3 files changed, 18 insertions(+), 16 deletions(-)
diff --git a/sound/soc/intel/catpt/core.h b/sound/soc/intel/catpt/core.h index 88dc3fb6306f..693dc4a4b621 100644 --- a/sound/soc/intel/catpt/core.h +++ b/sound/soc/intel/catpt/core.h @@ -80,6 +80,8 @@ struct catpt_spec { u32 host_ssp_offset[CATPT_SSP_COUNT]; u32 dram_mask; u32 iram_mask; + u32 d3srampgd_bit; + u32 d3pgd_bit; void (*pll_shutdown)(struct catpt_dev *cdev, bool enable); int (*power_up)(struct catpt_dev *cdev); int (*power_down)(struct catpt_dev *cdev); @@ -128,8 +130,8 @@ void lpt_dsp_pll_shutdown(struct catpt_dev *cdev, bool enable); void wpt_dsp_pll_shutdown(struct catpt_dev *cdev, bool enable); int lpt_dsp_power_up(struct catpt_dev *cdev); int lpt_dsp_power_down(struct catpt_dev *cdev); -int wpt_dsp_power_up(struct catpt_dev *cdev); -int wpt_dsp_power_down(struct catpt_dev *cdev); +int catpt_dsp_power_up(struct catpt_dev *cdev); +int catpt_dsp_power_down(struct catpt_dev *cdev); int catpt_dsp_stall(struct catpt_dev *cdev, bool stall); void catpt_dsp_update_srampge(struct catpt_dev *cdev, struct resource *sram, unsigned long mask); diff --git a/sound/soc/intel/catpt/device.c b/sound/soc/intel/catpt/device.c index a70179959795..b1d380868d8c 100644 --- a/sound/soc/intel/catpt/device.c +++ b/sound/soc/intel/catpt/device.c @@ -69,7 +69,7 @@ static int __maybe_unused catpt_suspend(struct device *dev) dma_release_channel(chan); if (ret) return ret; - return cdev->spec->power_down(cdev); + return catpt_dsp_power_down(cdev); }
static int __maybe_unused catpt_resume(struct device *dev) @@ -77,7 +77,7 @@ static int __maybe_unused catpt_resume(struct device *dev) struct catpt_dev *cdev = dev_get_drvdata(dev); int ret, i;
- ret = cdev->spec->power_up(cdev); + ret = catpt_dsp_power_up(cdev); if (ret) return ret;
@@ -162,7 +162,7 @@ static int catpt_probe_components(struct catpt_dev *cdev) { int ret;
- ret = cdev->spec->power_up(cdev); + ret = catpt_dsp_power_up(cdev); if (ret) return ret;
@@ -204,7 +204,7 @@ static int catpt_probe_components(struct catpt_dev *cdev) err_boot_fw: catpt_dmac_remove(cdev); err_dmac_probe: - cdev->spec->power_down(cdev); + catpt_dsp_power_down(cdev);
return ret; } @@ -293,7 +293,7 @@ static int catpt_acpi_remove(struct platform_device *pdev)
snd_soc_unregister_component(cdev->dev); catpt_dmac_remove(cdev); - cdev->spec->power_down(cdev); + catpt_dsp_power_down(cdev);
catpt_sram_free(&cdev->iram); catpt_sram_free(&cdev->dram); @@ -311,9 +311,9 @@ static struct catpt_spec lpt_desc = { .host_ssp_offset = { 0x0E8000, 0x0E9000 }, .dram_mask = LPT_VDRTCTL0_DSRAMPGE_MASK, .iram_mask = LPT_VDRTCTL0_ISRAMPGE_MASK, + .d3srampgd_bit = LPT_VDRTCTL0_D3SRAMPGD, + .d3pgd_bit = LPT_VDRTCTL0_D3PGD, .pll_shutdown = lpt_dsp_pll_shutdown, - .power_up = lpt_dsp_power_up, - .power_down = lpt_dsp_power_down, };
static struct catpt_spec wpt_desc = { @@ -326,9 +326,9 @@ static struct catpt_spec wpt_desc = { .host_ssp_offset = { 0x0FC000, 0x0FD000 }, .dram_mask = WPT_VDRTCTL0_DSRAMPGE_MASK, .iram_mask = WPT_VDRTCTL0_ISRAMPGE_MASK, + .d3srampgd_bit = WPT_VDRTCTL0_D3SRAMPGD, + .d3pgd_bit = WPT_VDRTCTL0_D3PGD, .pll_shutdown = wpt_dsp_pll_shutdown, - .power_up = wpt_dsp_power_up, - .power_down = wpt_dsp_power_down, };
static const struct acpi_device_id catpt_ids[] = { diff --git a/sound/soc/intel/catpt/dsp.c b/sound/soc/intel/catpt/dsp.c index 9e807b941732..dfa442a3c1f1 100644 --- a/sound/soc/intel/catpt/dsp.c +++ b/sound/soc/intel/catpt/dsp.c @@ -390,7 +390,7 @@ int lpt_dsp_power_up(struct catpt_dev *cdev) return 0; }
-int wpt_dsp_power_down(struct catpt_dev *cdev) +int catpt_dsp_power_down(struct catpt_dev *cdev) { u32 mask, val;
@@ -420,8 +420,8 @@ int wpt_dsp_power_down(struct catpt_dev *cdev) cdev->spec->dram_mask); catpt_dsp_set_srampge(cdev, &cdev->iram, cdev->spec->iram_mask, cdev->spec->iram_mask); - mask = WPT_VDRTCTL0_D3SRAMPGD | WPT_VDRTCTL0_D3PGD; - catpt_updatel_pci(cdev, VDRTCTL0, mask, WPT_VDRTCTL0_D3PGD); + mask = cdev->spec->d3srampgd_bit | cdev->spec->d3pgd_bit; + catpt_updatel_pci(cdev, VDRTCTL0, mask, cdev->spec->d3pgd_bit);
catpt_updatel_pci(cdev, PMCS, PCI_PM_CTRL_STATE_MASK, PCI_D3hot); /* give hw time to drop off */ @@ -435,7 +435,7 @@ int wpt_dsp_power_down(struct catpt_dev *cdev) return 0; }
-int wpt_dsp_power_up(struct catpt_dev *cdev) +int catpt_dsp_power_up(struct catpt_dev *cdev) { u32 mask, val;
@@ -450,7 +450,7 @@ int wpt_dsp_power_up(struct catpt_dev *cdev) catpt_updatel_pci(cdev, PMCS, PCI_PM_CTRL_STATE_MASK, PCI_D0);
/* SRAM power gating none */ - mask = WPT_VDRTCTL0_D3SRAMPGD | WPT_VDRTCTL0_D3PGD; + mask = cdev->spec->d3srampgd_bit | cdev->spec->d3pgd_bit; catpt_updatel_pci(cdev, VDRTCTL0, mask, mask); catpt_dsp_set_srampge(cdev, &cdev->dram, cdev->spec->dram_mask, 0); catpt_dsp_set_srampge(cdev, &cdev->iram, cdev->spec->iram_mask, 0);
With LPT switching to WPT-based power on/off routines, functions that have been previously used by it are rendered redundant so remove them.
Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- sound/soc/intel/catpt/core.h | 4 ---- sound/soc/intel/catpt/dsp.c | 46 ------------------------------------ 2 files changed, 50 deletions(-)
diff --git a/sound/soc/intel/catpt/core.h b/sound/soc/intel/catpt/core.h index 693dc4a4b621..0f53a0d43254 100644 --- a/sound/soc/intel/catpt/core.h +++ b/sound/soc/intel/catpt/core.h @@ -83,8 +83,6 @@ struct catpt_spec { u32 d3srampgd_bit; u32 d3pgd_bit; void (*pll_shutdown)(struct catpt_dev *cdev, bool enable); - int (*power_up)(struct catpt_dev *cdev); - int (*power_down)(struct catpt_dev *cdev); };
struct catpt_dev { @@ -128,8 +126,6 @@ int catpt_dma_memcpy_fromdsp(struct catpt_dev *cdev, struct dma_chan *chan,
void lpt_dsp_pll_shutdown(struct catpt_dev *cdev, bool enable); void wpt_dsp_pll_shutdown(struct catpt_dev *cdev, bool enable); -int lpt_dsp_power_up(struct catpt_dev *cdev); -int lpt_dsp_power_down(struct catpt_dev *cdev); int catpt_dsp_power_up(struct catpt_dev *cdev); int catpt_dsp_power_down(struct catpt_dev *cdev); int catpt_dsp_stall(struct catpt_dev *cdev, bool stall); diff --git a/sound/soc/intel/catpt/dsp.c b/sound/soc/intel/catpt/dsp.c index dfa442a3c1f1..9c5fd18f2600 100644 --- a/sound/soc/intel/catpt/dsp.c +++ b/sound/soc/intel/catpt/dsp.c @@ -344,52 +344,6 @@ static void catpt_dsp_set_regs_defaults(struct catpt_dev *cdev) } }
-int lpt_dsp_power_down(struct catpt_dev *cdev) -{ - catpt_dsp_reset(cdev, true); - - /* set 24Mhz clock for both SSPs */ - catpt_updatel_shim(cdev, CS1, CATPT_CS_SBCS(0) | CATPT_CS_SBCS(1), - CATPT_CS_SBCS(0) | CATPT_CS_SBCS(1)); - catpt_dsp_select_lpclock(cdev, true, false); - - /* DRAM power gating all */ - catpt_dsp_set_srampge(cdev, &cdev->dram, cdev->spec->dram_mask, - cdev->spec->dram_mask); - catpt_dsp_set_srampge(cdev, &cdev->iram, cdev->spec->iram_mask, - cdev->spec->iram_mask); - - catpt_updatel_pci(cdev, PMCS, PCI_PM_CTRL_STATE_MASK, PCI_D3hot); - /* give hw time to drop off */ - udelay(50); - - return 0; -} - -int lpt_dsp_power_up(struct catpt_dev *cdev) -{ - /* SRAM power gating none */ - catpt_dsp_set_srampge(cdev, &cdev->dram, cdev->spec->dram_mask, 0); - catpt_dsp_set_srampge(cdev, &cdev->iram, cdev->spec->iram_mask, 0); - - catpt_updatel_pci(cdev, PMCS, PCI_PM_CTRL_STATE_MASK, PCI_D0); - /* give hw time to wake up */ - udelay(100); - - catpt_dsp_select_lpclock(cdev, false, false); - catpt_updatel_shim(cdev, CS1, - CATPT_CS_SBCS(0) | CATPT_CS_SBCS(1), - CATPT_CS_SBCS(0) | CATPT_CS_SBCS(1)); - /* stagger DSP reset after clock selection */ - udelay(50); - - catpt_dsp_reset(cdev, false); - /* generate int deassert msg to fix inversed int logic */ - catpt_updatel_shim(cdev, IMC, CATPT_IMC_IPCDB | CATPT_IMC_IPCCD, 0); - - return 0; -} - int catpt_dsp_power_down(struct catpt_dev *cdev) { u32 mask, val;
On Mon, 16 Nov 2020 14:33:27 +0100, Cezary Rojewski wrote:
First two of the series address bugs connected mainly to offload streams:
- scenarios with very low buffer sizes: RESET_STREAM IPC timeouts
- fix lp clock selection when switching between PAUSE <-> RESUME states: glitches on first offload when no additional stream is opened simultaneously
Follow ups are: code reduction and optimization oriented patches. This has been foretold in:
[...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[1/5] ASoC: Intel: catpt: Skip position update for unprepared streams commit: aa9e3fa4992d83acb7311fc86d11d0d53e7ffb8e [2/5] ASoC: Intel: catpt: Correct clock selection for dai trigger commit: 1072460a1aabacf6ececda98acd3b5ecaad23fd2 [3/5] ASoC: Intel: catpt: Optimize applying user settings commit: 768a3a3b327da88c2fa6856806d32852a90e75d5 [4/5] ASoC: Intel: catpt: Streamline power routines across LPT and WPT commit: c440c72474e12fcf79bbe716d4796d16b7201031 [5/5] ASoC: Intel: catpt: Cleanup after power routines streamlining commit: 3d32489838bbf3119bb1ea59cdbed0077d7dbf3c
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
participants (2)
-
Cezary Rojewski -
Mark Brown