[alsa-devel] [PATCH] ASoC: mediatek: Use current HW pointer for pointer callback
Previously we recorded "last interrupt position" and used it in pointer callback. This is not correct implementation, and it causes underruns when user space monitors buffer level to decide when to send next data chunk in low latency application.
Remove position recording in IRQ handler and also hw_ptr in struct mtk_afe_memif used to record that, and let pointer callback reports current HW pointer instead.
Signed-off-by: Koro Chen koro.chen@mediatek.com --- sound/soc/mediatek/mtk-afe-common.h | 1 - sound/soc/mediatek/mtk-afe-pcm.c | 22 +++++++++++----------- 2 files changed, 11 insertions(+), 12 deletions(-)
diff --git a/sound/soc/mediatek/mtk-afe-common.h b/sound/soc/mediatek/mtk-afe-common.h index cc4393c..9b1af1a 100644 --- a/sound/soc/mediatek/mtk-afe-common.h +++ b/sound/soc/mediatek/mtk-afe-common.h @@ -92,7 +92,6 @@ struct mtk_afe_memif_data { struct mtk_afe_memif { unsigned int phys_buf_addr; int buffer_size; - unsigned int hw_ptr; /* Previous IRQ's HW ptr */ struct snd_pcm_substream *substream; const struct mtk_afe_memif_data *data; const struct mtk_afe_irq_data *irqdata; diff --git a/sound/soc/mediatek/mtk-afe-pcm.c b/sound/soc/mediatek/mtk-afe-pcm.c index 7f71343..5399a0e 100644 --- a/sound/soc/mediatek/mtk-afe-pcm.c +++ b/sound/soc/mediatek/mtk-afe-pcm.c @@ -175,8 +175,17 @@ static snd_pcm_uframes_t mtk_afe_pcm_pointer struct snd_soc_pcm_runtime *rtd = substream->private_data; struct mtk_afe *afe = snd_soc_platform_get_drvdata(rtd->platform); struct mtk_afe_memif *memif = &afe->memif[rtd->cpu_dai->id]; + unsigned int hw_ptr; + int ret; + + ret = regmap_read(afe->regmap, memif->data->reg_ofs_cur, &hw_ptr); + if (ret || hw_ptr == 0) { + dev_err(afe->dev, "%s hw_ptr err\n", __func__); + hw_ptr = memif->phys_buf_addr; + }
- return bytes_to_frames(substream->runtime, memif->hw_ptr); + return bytes_to_frames(substream->runtime, + hw_ptr - memif->phys_buf_addr); }
static const struct snd_pcm_ops mtk_afe_pcm_ops = { @@ -602,7 +611,6 @@ static int mtk_afe_dais_hw_params(struct snd_pcm_substream *substream,
memif->phys_buf_addr = substream->runtime->dma_addr; memif->buffer_size = substream->runtime->dma_bytes; - memif->hw_ptr = 0;
/* start */ regmap_write(afe->regmap, @@ -737,7 +745,6 @@ static int mtk_afe_dais_trigger(struct snd_pcm_substream *substream, int cmd, /* and clear pending IRQ */ regmap_write(afe->regmap, AFE_IRQ_CLR, 1 << memif->data->irq_clr_shift); - memif->hw_ptr = 0; return 0; default: return -EINVAL; @@ -1081,7 +1088,7 @@ static const struct regmap_config mtk_afe_regmap_config = { static irqreturn_t mtk_afe_irq_handler(int irq, void *dev_id) { struct mtk_afe *afe = dev_id; - unsigned int reg_value, hw_ptr; + unsigned int reg_value; int i, ret;
ret = regmap_read(afe->regmap, AFE_IRQ_STATUS, ®_value); @@ -1097,13 +1104,6 @@ static irqreturn_t mtk_afe_irq_handler(int irq, void *dev_id) if (!(reg_value & (1 << memif->data->irq_clr_shift))) continue;
- ret = regmap_read(afe->regmap, memif->data->reg_ofs_cur, - &hw_ptr); - if (ret || hw_ptr == 0) { - dev_err(afe->dev, "%s hw_ptr err\n", __func__); - hw_ptr = memif->phys_buf_addr; - } - memif->hw_ptr = hw_ptr - memif->phys_buf_addr; snd_pcm_period_elapsed(memif->substream); }
On Thu, 03 Dec 2015 08:53:28 +0100, Koro Chen wrote:
Previously we recorded "last interrupt position" and used it in pointer callback. This is not correct implementation, and it causes underruns when user space monitors buffer level to decide when to send next data chunk in low latency application.
Using the last irq position isn't incorrect, per se. Many hardware can do only in this way. But it's suboptimal for your device, yes.
Remove position recording in IRQ handler and also hw_ptr in struct mtk_afe_memif used to record that, and let pointer callback reports current HW pointer instead.
Signed-off-by: Koro Chen koro.chen@mediatek.com
While reading this patch, I wondered how regmap can be used safely in an irq-disabled context. Mark, do we have any API for that?
In this particular case, it would work since it specifies REGCACHE_NONE, so it drops all cache support. As its cost, the driver implements its own (kind of) cache in suspend/resume callbacks. It's not ideal, but still practically working.
Takashi
sound/soc/mediatek/mtk-afe-common.h | 1 - sound/soc/mediatek/mtk-afe-pcm.c | 22 +++++++++++----------- 2 files changed, 11 insertions(+), 12 deletions(-)
diff --git a/sound/soc/mediatek/mtk-afe-common.h b/sound/soc/mediatek/mtk-afe-common.h index cc4393c..9b1af1a 100644 --- a/sound/soc/mediatek/mtk-afe-common.h +++ b/sound/soc/mediatek/mtk-afe-common.h @@ -92,7 +92,6 @@ struct mtk_afe_memif_data { struct mtk_afe_memif { unsigned int phys_buf_addr; int buffer_size;
- unsigned int hw_ptr; /* Previous IRQ's HW ptr */ struct snd_pcm_substream *substream; const struct mtk_afe_memif_data *data; const struct mtk_afe_irq_data *irqdata;
diff --git a/sound/soc/mediatek/mtk-afe-pcm.c b/sound/soc/mediatek/mtk-afe-pcm.c index 7f71343..5399a0e 100644 --- a/sound/soc/mediatek/mtk-afe-pcm.c +++ b/sound/soc/mediatek/mtk-afe-pcm.c @@ -175,8 +175,17 @@ static snd_pcm_uframes_t mtk_afe_pcm_pointer struct snd_soc_pcm_runtime *rtd = substream->private_data; struct mtk_afe *afe = snd_soc_platform_get_drvdata(rtd->platform); struct mtk_afe_memif *memif = &afe->memif[rtd->cpu_dai->id];
- unsigned int hw_ptr;
- int ret;
- ret = regmap_read(afe->regmap, memif->data->reg_ofs_cur, &hw_ptr);
- if (ret || hw_ptr == 0) {
dev_err(afe->dev, "%s hw_ptr err\n", __func__);hw_ptr = memif->phys_buf_addr;- }
- return bytes_to_frames(substream->runtime, memif->hw_ptr);
- return bytes_to_frames(substream->runtime,
hw_ptr - memif->phys_buf_addr);}
static const struct snd_pcm_ops mtk_afe_pcm_ops = { @@ -602,7 +611,6 @@ static int mtk_afe_dais_hw_params(struct snd_pcm_substream *substream,
memif->phys_buf_addr = substream->runtime->dma_addr; memif->buffer_size = substream->runtime->dma_bytes;
memif->hw_ptr = 0;
/* start */ regmap_write(afe->regmap,
@@ -737,7 +745,6 @@ static int mtk_afe_dais_trigger(struct snd_pcm_substream *substream, int cmd, /* and clear pending IRQ */ regmap_write(afe->regmap, AFE_IRQ_CLR, 1 << memif->data->irq_clr_shift);
memif->hw_ptr = 0;@@ -1081,7 +1088,7 @@ static const struct regmap_config mtk_afe_regmap_config = { static irqreturn_t mtk_afe_irq_handler(int irq, void *dev_id) { struct mtk_afe *afe = dev_id;
- unsigned int reg_value, hw_ptr;
unsigned int reg_value; int i, ret;
ret = regmap_read(afe->regmap, AFE_IRQ_STATUS, ®_value);
@@ -1097,13 +1104,6 @@ static irqreturn_t mtk_afe_irq_handler(int irq, void *dev_id) if (!(reg_value & (1 << memif->data->irq_clr_shift))) continue;
ret = regmap_read(afe->regmap, memif->data->reg_ofs_cur,&hw_ptr);if (ret || hw_ptr == 0) {dev_err(afe->dev, "%s hw_ptr err\n", __func__);hw_ptr = memif->phys_buf_addr;}memif->hw_ptr = hw_ptr - memif->phys_buf_addr;-- 1.7.9.5
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Thu, Dec 03, 2015 at 10:41:38AM +0100, Takashi Iwai wrote:
While reading this patch, I wondered how regmap can be used safely in an irq-disabled context. Mark, do we have any API for that?
We can use user supplied locks or spin_lock_irqsave().
On Thu, 03 Dec 2015 12:01:58 +0100, Mark Brown wrote:
On Thu, Dec 03, 2015 at 10:41:38AM +0100, Takashi Iwai wrote:
While reading this patch, I wondered how regmap can be used safely in an irq-disabled context. Mark, do we have any API for that?
We can use user supplied locks or spin_lock_irqsave().
I meant how to guarantee to make regmap_read() working in an already spin-locked context, typically in an irq handler? regmap_read() involves with the regcache and it may invoke kmalloc().
Takashi
On Thu, 2015-12-03 at 12:07 +0100, Takashi Iwai wrote:
On Thu, 03 Dec 2015 12:01:58 +0100, Mark Brown wrote:
On Thu, Dec 03, 2015 at 10:41:38AM +0100, Takashi Iwai wrote:
While reading this patch, I wondered how regmap can be used safely in an irq-disabled context. Mark, do we have any API for that?
We can use user supplied locks or spin_lock_irqsave().
I meant how to guarantee to make regmap_read() working in an already spin-locked context, typically in an irq handler? regmap_read() involves with the regcache and it may invoke kmalloc().
Yes, we were hit by this before. When using devm_regmap_init_mmio() for regmap, it uses spin_lock_irqsave() before every read/write, and if cache type is RBTREE, then kmalloc will occur after spin_lock_irqsave() and it brings warning. That's why we changed RBTREE to NONE. Setting cache type to FLAT will also work, but we think our register accessing is fast enough without need of cache.
Takashi _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Thu, Dec 03, 2015 at 12:07:26PM +0100, Takashi Iwai wrote:
Mark Brown wrote:
On Thu, Dec 03, 2015 at 10:41:38AM +0100, Takashi Iwai wrote:
While reading this patch, I wondered how regmap can be used safely in an irq-disabled context. Mark, do we have any API for that?
We can use user supplied locks or spin_lock_irqsave().
I meant how to guarantee to make regmap_read() working in an already spin-locked context, typically in an irq handler? regmap_read() involves with the regcache and it may invoke kmalloc().
I know that's what you meant - that should be done by preallocating the cache (which can be done with defaults) and providing your own lock if there's a spinlock already held (since we use _irqsave() which IIRC isn't nestable). We can also use GFP_ATOMIC for some of the allocations in reasonable use cases but it's not in general supported.
On Thu, 03 Dec 2015 15:56:12 +0100, Mark Brown wrote:
On Thu, Dec 03, 2015 at 12:07:26PM +0100, Takashi Iwai wrote:
Mark Brown wrote:
On Thu, Dec 03, 2015 at 10:41:38AM +0100, Takashi Iwai wrote:
While reading this patch, I wondered how regmap can be used safely in an irq-disabled context. Mark, do we have any API for that?
We can use user supplied locks or spin_lock_irqsave().
I meant how to guarantee to make regmap_read() working in an already spin-locked context, typically in an irq handler? regmap_read() involves with the regcache and it may invoke kmalloc().
I know that's what you meant - that should be done by preallocating the cache (which can be done with defaults) and providing your own lock if there's a spinlock already held (since we use _irqsave() which IIRC isn't nestable).
The extra lock should be fine unless any mutex is involved silently in regmap code. But preallocation sounds not so intuitive.
We can also use GFP_ATOMIC for some of the allocations in reasonable use cases but it's not in general supported.
Yeah, but we have already map->alloc_flags, so we can use it appropriately as a fallback?
Takashi
The patch
ASoC: mediatek: Use current HW pointer for pointer callback
has been applied to the asoc tree at
git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
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
From 8d6f88ce961cf62137696627448cfd6038f07f41 Mon Sep 17 00:00:00 2001
From: Koro Chen koro.chen@mediatek.com Date: Thu, 3 Dec 2015 15:53:28 +0800 Subject: [PATCH] ASoC: mediatek: Use current HW pointer for pointer callback
Previously we recorded "last interrupt position" and used it in pointer callback. This is not correct implementation, and it causes underruns when user space monitors buffer level to decide when to send next data chunk in low latency application.
Remove position recording in IRQ handler and also hw_ptr in struct mtk_afe_memif used to record that, and let pointer callback reports current HW pointer instead.
Signed-off-by: Koro Chen koro.chen@mediatek.com Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/mediatek/mtk-afe-common.h | 1 - sound/soc/mediatek/mtk-afe-pcm.c | 22 +++++++++++----------- 2 files changed, 11 insertions(+), 12 deletions(-)
diff --git a/sound/soc/mediatek/mtk-afe-common.h b/sound/soc/mediatek/mtk-afe-common.h index cc4393cb1130..9b1af1a70874 100644 --- a/sound/soc/mediatek/mtk-afe-common.h +++ b/sound/soc/mediatek/mtk-afe-common.h @@ -92,7 +92,6 @@ struct mtk_afe_memif_data { struct mtk_afe_memif { unsigned int phys_buf_addr; int buffer_size; - unsigned int hw_ptr; /* Previous IRQ's HW ptr */ struct snd_pcm_substream *substream; const struct mtk_afe_memif_data *data; const struct mtk_afe_irq_data *irqdata; diff --git a/sound/soc/mediatek/mtk-afe-pcm.c b/sound/soc/mediatek/mtk-afe-pcm.c index 7f7134397f73..5399a0eead3e 100644 --- a/sound/soc/mediatek/mtk-afe-pcm.c +++ b/sound/soc/mediatek/mtk-afe-pcm.c @@ -175,8 +175,17 @@ static snd_pcm_uframes_t mtk_afe_pcm_pointer struct snd_soc_pcm_runtime *rtd = substream->private_data; struct mtk_afe *afe = snd_soc_platform_get_drvdata(rtd->platform); struct mtk_afe_memif *memif = &afe->memif[rtd->cpu_dai->id]; + unsigned int hw_ptr; + int ret; + + ret = regmap_read(afe->regmap, memif->data->reg_ofs_cur, &hw_ptr); + if (ret || hw_ptr == 0) { + dev_err(afe->dev, "%s hw_ptr err\n", __func__); + hw_ptr = memif->phys_buf_addr; + }
- return bytes_to_frames(substream->runtime, memif->hw_ptr); + return bytes_to_frames(substream->runtime, + hw_ptr - memif->phys_buf_addr); }
static const struct snd_pcm_ops mtk_afe_pcm_ops = { @@ -602,7 +611,6 @@ static int mtk_afe_dais_hw_params(struct snd_pcm_substream *substream,
memif->phys_buf_addr = substream->runtime->dma_addr; memif->buffer_size = substream->runtime->dma_bytes; - memif->hw_ptr = 0;
/* start */ regmap_write(afe->regmap, @@ -737,7 +745,6 @@ static int mtk_afe_dais_trigger(struct snd_pcm_substream *substream, int cmd, /* and clear pending IRQ */ regmap_write(afe->regmap, AFE_IRQ_CLR, 1 << memif->data->irq_clr_shift); - memif->hw_ptr = 0; return 0; default: return -EINVAL; @@ -1081,7 +1088,7 @@ static const struct regmap_config mtk_afe_regmap_config = { static irqreturn_t mtk_afe_irq_handler(int irq, void *dev_id) { struct mtk_afe *afe = dev_id; - unsigned int reg_value, hw_ptr; + unsigned int reg_value; int i, ret;
ret = regmap_read(afe->regmap, AFE_IRQ_STATUS, ®_value); @@ -1097,13 +1104,6 @@ static irqreturn_t mtk_afe_irq_handler(int irq, void *dev_id) if (!(reg_value & (1 << memif->data->irq_clr_shift))) continue;
- ret = regmap_read(afe->regmap, memif->data->reg_ofs_cur, - &hw_ptr); - if (ret || hw_ptr == 0) { - dev_err(afe->dev, "%s hw_ptr err\n", __func__); - hw_ptr = memif->phys_buf_addr; - } - memif->hw_ptr = hw_ptr - memif->phys_buf_addr; snd_pcm_period_elapsed(memif->substream); }
participants (3)
-
Koro Chen -
Mark Brown -
Takashi Iwai