alsa-devel.alsa-project.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v7] ASoC: omap-mcbsp: Add PM QoS support for McBSP to prevent glitches
@ 2017-01-13  3:54 Matt Ranostay
  2017-01-13 15:20 ` Tony Lindgren
  0 siblings, 1 reply; 8+ messages in thread
From: Matt Ranostay @ 2017-01-13  3:54 UTC (permalink / raw)
  To: alsa-devel, inux-omap; +Cc: Tony Lindgren, Peter Ujfalusi, Matt Ranostay

We can get audio errors if hitting deeper idle states on omaps:

[alsa.c:230] error: Fatal problem with alsa output, error -5.
[audio.c:614] error: Error in writing audio (Input/output error?)!

This seems to happen with off mode idle enabled as power for the
whole SoC may get cut off between filling the McBSP fifo using DMA.
While active DMA blocks deeper idle states in hardware, McBSP
activity does not seem to do so.

Basing the QoS latency calculation on the FIFO size, threshold,
sample rate, and channels.

Based on the original patch by Tony Lindgren
Link: https://patchwork.kernel.org/patch/9305867/

Cc: Tony Lindgren <tony@atomide.com>
Cc: Peter Ujfalusi <peter.ujfalusi@ti.com>
Acked-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
---
Changes from v1:
* add calculations for latency per number of FIFO locations

Changes from v2:
* add missing mcbsp.h header change

Changes from v3:
* base the latency calculations on threshold, buffer size, sample
  rate, and channels

Changes from v4:
* using Peter Ujfalusi's suggestions for restoring a higher latency on
  audio stream completion, or if not applicable remove the QoS request

Changes from v5:
* clean up latency checking logic
* move logic to .prepare and .shutdown to avoid functions that can sleep

Changes from v6:
* move QoS removal to asoc_mcbsp_remove from omap_mcbsp_cleanup
* also remove header include that is unneeded

 sound/soc/omap/mcbsp.h      |  3 +++
 sound/soc/omap/omap-mcbsp.c | 47 ++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 49 insertions(+), 1 deletion(-)

diff --git a/sound/soc/omap/mcbsp.h b/sound/soc/omap/mcbsp.h
index 61e93b1c185d..46ae1269a698 100644
--- a/sound/soc/omap/mcbsp.h
+++ b/sound/soc/omap/mcbsp.h
@@ -323,8 +323,11 @@ struct omap_mcbsp {
 
 	unsigned int fmt;
 	unsigned int in_freq;
+	unsigned int latency[2];
 	int clk_div;
 	int wlen;
+
+	struct pm_qos_request pm_qos_req;
 };
 
 void omap_mcbsp_config(struct omap_mcbsp *mcbsp,
diff --git a/sound/soc/omap/omap-mcbsp.c b/sound/soc/omap/omap-mcbsp.c
index d018e966e533..3b8c9724d496 100644
--- a/sound/soc/omap/omap-mcbsp.c
+++ b/sound/soc/omap/omap-mcbsp.c
@@ -157,6 +157,16 @@ static void omap_mcbsp_dai_shutdown(struct snd_pcm_substream *substream,
 				    struct snd_soc_dai *cpu_dai)
 {
 	struct omap_mcbsp *mcbsp = snd_soc_dai_get_drvdata(cpu_dai);
+	int tx = (substream->stream == SNDRV_PCM_STREAM_PLAYBACK);
+	int stream1 = tx ? SNDRV_PCM_STREAM_PLAYBACK : SNDRV_PCM_STREAM_CAPTURE;
+	int stream2 = tx ? SNDRV_PCM_STREAM_CAPTURE : SNDRV_PCM_STREAM_PLAYBACK;
+
+	mcbsp->latency[stream1] = 0;
+	if (mcbsp->latency[stream2])
+		pm_qos_update_request(&mcbsp->pm_qos_req,
+				      mcbsp->latency[stream2]);
+	else
+		pm_qos_remove_request(&mcbsp->pm_qos_req);
 
 	if (!cpu_dai->active) {
 		omap_mcbsp_free(mcbsp);
@@ -164,6 +174,28 @@ static void omap_mcbsp_dai_shutdown(struct snd_pcm_substream *substream,
 	}
 }
 
+static int omap_mcbsp_dai_prepare(struct snd_pcm_substream *substream,
+				  struct snd_soc_dai *cpu_dai)
+{
+	struct omap_mcbsp *mcbsp = snd_soc_dai_get_drvdata(cpu_dai);
+	struct pm_qos_request *pm_qos_req = &mcbsp->pm_qos_req;
+	int tx = (substream->stream == SNDRV_PCM_STREAM_PLAYBACK);
+	int stream1 = tx ? SNDRV_PCM_STREAM_PLAYBACK : SNDRV_PCM_STREAM_CAPTURE;
+	int stream2 = tx ? SNDRV_PCM_STREAM_CAPTURE : SNDRV_PCM_STREAM_PLAYBACK;
+	int latency = mcbsp->latency[stream2];
+
+	/* Prevent omap hardware from hitting off between FIFO fills */
+	if (!latency || mcbsp->latency[stream1] < latency)
+		latency = mcbsp->latency[stream1];
+
+	if (pm_qos_request_active(pm_qos_req))
+		pm_qos_update_request(pm_qos_req, latency);
+	else if (latency)
+		pm_qos_add_request(pm_qos_req, PM_QOS_CPU_DMA_LATENCY, latency);
+
+	return 0;
+}
+
 static int omap_mcbsp_dai_trigger(struct snd_pcm_substream *substream, int cmd,
 				  struct snd_soc_dai *cpu_dai)
 {
@@ -226,6 +258,7 @@ static int omap_mcbsp_dai_hw_params(struct snd_pcm_substream *substream,
 	int wlen, channels, wpf;
 	int pkt_size = 0;
 	unsigned int format, div, framesize, master;
+	unsigned int buffer_size = mcbsp->pdata->buffer_size;
 
 	dma_data = snd_soc_dai_get_dma_data(cpu_dai, substream);
 	channels = params_channels(params);
@@ -240,7 +273,9 @@ static int omap_mcbsp_dai_hw_params(struct snd_pcm_substream *substream,
 	default:
 		return -EINVAL;
 	}
-	if (mcbsp->pdata->buffer_size) {
+	if (buffer_size) {
+		int latency;
+
 		if (mcbsp->dma_op_mode == MCBSP_DMA_MODE_THRESHOLD) {
 			int period_words, max_thrsh;
 			int divider = 0;
@@ -271,6 +306,12 @@ static int omap_mcbsp_dai_hw_params(struct snd_pcm_substream *substream,
 			/* Use packet mode for non mono streams */
 			pkt_size = channels;
 		}
+
+		latency = ((((buffer_size - pkt_size) / channels) * 1000)
+				 / (params->rate_num / params->rate_den));
+
+		mcbsp->latency[substream->stream] = latency;
+
 		omap_mcbsp_set_threshold(substream, pkt_size);
 	}
 
@@ -554,6 +595,7 @@ static int omap_mcbsp_dai_set_dai_sysclk(struct snd_soc_dai *cpu_dai,
 static const struct snd_soc_dai_ops mcbsp_dai_ops = {
 	.startup	= omap_mcbsp_dai_startup,
 	.shutdown	= omap_mcbsp_dai_shutdown,
+	.prepare	= omap_mcbsp_dai_prepare,
 	.trigger	= omap_mcbsp_dai_trigger,
 	.delay		= omap_mcbsp_dai_delay,
 	.hw_params	= omap_mcbsp_dai_hw_params,
@@ -835,6 +877,9 @@ static int asoc_mcbsp_remove(struct platform_device *pdev)
 	if (mcbsp->pdata->ops && mcbsp->pdata->ops->free)
 		mcbsp->pdata->ops->free(mcbsp->id);
 
+	if (pm_qos_request_active(&mcbsp->pm_qos_req))
+		pm_qos_remove_request(&mcbsp->pm_qos_req);
+
 	omap_mcbsp_cleanup(mcbsp);
 
 	clk_put(mcbsp->fclk);
-- 
2.10.2

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v7] ASoC: omap-mcbsp: Add PM QoS support for McBSP to prevent glitches
  2017-01-13  3:54 [PATCH v7] ASoC: omap-mcbsp: Add PM QoS support for McBSP to prevent glitches Matt Ranostay
@ 2017-01-13 15:20 ` Tony Lindgren
  2017-01-13 17:57   ` Matt Ranostay
  2017-01-14  5:15   ` Matt Ranostay
  0 siblings, 2 replies; 8+ messages in thread
From: Tony Lindgren @ 2017-01-13 15:20 UTC (permalink / raw)
  To: Matt Ranostay; +Cc: Peter Ujfalusi, alsa-devel, inux-omap

* Matt Ranostay <matt@ranostay.consulting> [170112 19:55]:
> We can get audio errors if hitting deeper idle states on omaps:
> 
> [alsa.c:230] error: Fatal problem with alsa output, error -5.
> [audio.c:614] error: Error in writing audio (Input/output error?)!
> 
> This seems to happen with off mode idle enabled as power for the
> whole SoC may get cut off between filling the McBSP fifo using DMA.
> While active DMA blocks deeper idle states in hardware, McBSP
> activity does not seem to do so.
> 
> Basing the QoS latency calculation on the FIFO size, threshold,
> sample rate, and channels.
> 
> Based on the original patch by Tony Lindgren
> Link: https://patchwork.kernel.org/patch/9305867/
> 
> Cc: Tony Lindgren <tony@atomide.com>
> Cc: Peter Ujfalusi <peter.ujfalusi@ti.com>
> Acked-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
> ---
> Changes from v1:
> * add calculations for latency per number of FIFO locations
> 
> Changes from v2:
> * add missing mcbsp.h header change
> 
> Changes from v3:
> * base the latency calculations on threshold, buffer size, sample
>   rate, and channels
> 
> Changes from v4:
> * using Peter Ujfalusi's suggestions for restoring a higher latency on
>   audio stream completion, or if not applicable remove the QoS request
> 
> Changes from v5:
> * clean up latency checking logic
> * move logic to .prepare and .shutdown to avoid functions that can sleep
> 
> Changes from v6:
> * move QoS removal to asoc_mcbsp_remove from omap_mcbsp_cleanup
> * also remove header include that is unneeded

Hmm now I'm seeing these when starting or stopping playback:

WARNING: CPU: 0 PID: 373 at kernel/power/qos.c:534 omap_mcbsp_dai_shutdown+0x6c/0x70 [snd_soc_omap_mcbsp]
pm_qos_remove_request() called for unknown object
Modules linked in: snd_soc_omap_twl4030 snd_soc_twl4030 snd_soc_omap_mcbsp evdev mwifiex_sdio mwifiex leds_gpio
 snd_soc_omap cfg80211 bq27xxx_battery_i2c leds_pca963x bq27xxx_battery led_class bq24190_charger phy_twl4030_usb omap2430 mus
 b_hdrc usbcore snd_soc_core twl4030_wdt snd_pcm_dmaengine rtc_twl snd_pcm snd_timer snd soundcore twl4030_pwrbutton pwm_twl_le
 d twl4030_keypad pwm_twl matrix_keymap omap_ssi hsi ledtrig_timer usb_f_serial usb_f_rndis usb_f_mass_storage usb_f_ecm u_ethe
 r usb_f_acm libcomposite u_serial udc_core usb_common autofs4 [last unloaded: snd_soc_omap_mcbsp]
 [   51.542541] CPU: 0 PID: 373 Comm: mpg123 Not tainted 4.10.0-rc2-next-20170109+ #751
 [   51.550231] Hardware name: Generic OMAP36xx (Flattened Device Tree)
 [   51.556579] [<c0110198>] (unwind_backtrace) from [<c010c208>] (show_stack+0x10/0x14)
 [   51.564361] [<c010c208>] (show_stack) from [<c04be0a0>] (dump_stack+0xac/0xe0)
 [   51.571655] [<c04be0a0>] (dump_stack) from [<c0137428>] (__warn+0xd8/0x104)
 [   51.578643] [<c0137428>] (__warn) from [<c0137488>] (warn_slowpath_fmt+0x34/0x44)
 [   51.586212] [<c0137488>] (warn_slowpath_fmt) from [<bf3538f4>] (omap_mcbsp_dai_shutdown+0x6c/0x70 [snd_soc_omap_mcbsp])
 [   51.597351] [<bf3538f4>] (omap_mcbsp_dai_shutdown [snd_soc_omap_mcbsp]) from [<bf11037c>] (soc_pcm_close+0xa0/0x2b8 [snd_so
 c_core])
 [   51.609497] [<bf11037c>] (soc_pcm_close [snd_soc_core]) from [<bf0d0654>] (snd_pcm_release_substream.part.13+0x48/0xac [snd
 _pcm])
 [   51.621337] [<bf0d0654>] (snd_pcm_release_substream.part.13 [snd_pcm]) from [<bf0d0764>] (snd_pcm_release+0x94/0xb4 [snd_pc
 m])
 [   51.632843] [<bf0d0764>] (snd_pcm_release [snd_pcm]) from [<c02b91d8>] (__fput+0x94/0x1e4)
 [   51.641174] [<c02b91d8>] (__fput) from [<c015b768>] (task_work_run+0xcc/0x104)
 [   51.648437] [<c015b768>] (task_work_run) from [<c010bb38>] (do_work_pending+0xbc/0xc0)
 [   51.656402] [<c010bb38>] (do_work_pending) from [<c01077c8>] (slow_work_pending+0xc/0x20)


Regards,

Tony

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v7] ASoC: omap-mcbsp: Add PM QoS support for McBSP to prevent glitches
  2017-01-13 15:20 ` Tony Lindgren
@ 2017-01-13 17:57   ` Matt Ranostay
  2017-01-14  5:15   ` Matt Ranostay
  1 sibling, 0 replies; 8+ messages in thread
From: Matt Ranostay @ 2017-01-13 17:57 UTC (permalink / raw)
  To: Tony Lindgren; +Cc: Peter Ujfalusi, alsa-devel, inux-omap

On Fri, Jan 13, 2017 at 7:20 AM, Tony Lindgren <tony@atomide.com> wrote:
> * Matt Ranostay <matt@ranostay.consulting> [170112 19:55]:
>> We can get audio errors if hitting deeper idle states on omaps:
>>
>> [alsa.c:230] error: Fatal problem with alsa output, error -5.
>> [audio.c:614] error: Error in writing audio (Input/output error?)!
>>
>> This seems to happen with off mode idle enabled as power for the
>> whole SoC may get cut off between filling the McBSP fifo using DMA.
>> While active DMA blocks deeper idle states in hardware, McBSP
>> activity does not seem to do so.
>>
>> Basing the QoS latency calculation on the FIFO size, threshold,
>> sample rate, and channels.
>>
>> Based on the original patch by Tony Lindgren
>> Link: https://patchwork.kernel.org/patch/9305867/
>>
>> Cc: Tony Lindgren <tony@atomide.com>
>> Cc: Peter Ujfalusi <peter.ujfalusi@ti.com>
>> Acked-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
>> Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
>> ---
>> Changes from v1:
>> * add calculations for latency per number of FIFO locations
>>
>> Changes from v2:
>> * add missing mcbsp.h header change
>>
>> Changes from v3:
>> * base the latency calculations on threshold, buffer size, sample
>>   rate, and channels
>>
>> Changes from v4:
>> * using Peter Ujfalusi's suggestions for restoring a higher latency on
>>   audio stream completion, or if not applicable remove the QoS request
>>
>> Changes from v5:
>> * clean up latency checking logic
>> * move logic to .prepare and .shutdown to avoid functions that can sleep
>>
>> Changes from v6:
>> * move QoS removal to asoc_mcbsp_remove from omap_mcbsp_cleanup
>> * also remove header include that is unneeded
>
> Hmm now I'm seeing these when starting or stopping playback:

Weird.. I'll test again tonight and get back to you
>
> WARNING: CPU: 0 PID: 373 at kernel/power/qos.c:534 omap_mcbsp_dai_shutdown+0x6c/0x70 [snd_soc_omap_mcbsp]
> pm_qos_remove_request() called for unknown object
> Modules linked in: snd_soc_omap_twl4030 snd_soc_twl4030 snd_soc_omap_mcbsp evdev mwifiex_sdio mwifiex leds_gpio
>  snd_soc_omap cfg80211 bq27xxx_battery_i2c leds_pca963x bq27xxx_battery led_class bq24190_charger phy_twl4030_usb omap2430 mus
>  b_hdrc usbcore snd_soc_core twl4030_wdt snd_pcm_dmaengine rtc_twl snd_pcm snd_timer snd soundcore twl4030_pwrbutton pwm_twl_le
>  d twl4030_keypad pwm_twl matrix_keymap omap_ssi hsi ledtrig_timer usb_f_serial usb_f_rndis usb_f_mass_storage usb_f_ecm u_ethe
>  r usb_f_acm libcomposite u_serial udc_core usb_common autofs4 [last unloaded: snd_soc_omap_mcbsp]
>  [   51.542541] CPU: 0 PID: 373 Comm: mpg123 Not tainted 4.10.0-rc2-next-20170109+ #751
>  [   51.550231] Hardware name: Generic OMAP36xx (Flattened Device Tree)
>  [   51.556579] [<c0110198>] (unwind_backtrace) from [<c010c208>] (show_stack+0x10/0x14)
>  [   51.564361] [<c010c208>] (show_stack) from [<c04be0a0>] (dump_stack+0xac/0xe0)
>  [   51.571655] [<c04be0a0>] (dump_stack) from [<c0137428>] (__warn+0xd8/0x104)
>  [   51.578643] [<c0137428>] (__warn) from [<c0137488>] (warn_slowpath_fmt+0x34/0x44)
>  [   51.586212] [<c0137488>] (warn_slowpath_fmt) from [<bf3538f4>] (omap_mcbsp_dai_shutdown+0x6c/0x70 [snd_soc_omap_mcbsp])
>  [   51.597351] [<bf3538f4>] (omap_mcbsp_dai_shutdown [snd_soc_omap_mcbsp]) from [<bf11037c>] (soc_pcm_close+0xa0/0x2b8 [snd_so
>  c_core])
>  [   51.609497] [<bf11037c>] (soc_pcm_close [snd_soc_core]) from [<bf0d0654>] (snd_pcm_release_substream.part.13+0x48/0xac [snd
>  _pcm])
>  [   51.621337] [<bf0d0654>] (snd_pcm_release_substream.part.13 [snd_pcm]) from [<bf0d0764>] (snd_pcm_release+0x94/0xb4 [snd_pc
>  m])
>  [   51.632843] [<bf0d0764>] (snd_pcm_release [snd_pcm]) from [<c02b91d8>] (__fput+0x94/0x1e4)
>  [   51.641174] [<c02b91d8>] (__fput) from [<c015b768>] (task_work_run+0xcc/0x104)
>  [   51.648437] [<c015b768>] (task_work_run) from [<c010bb38>] (do_work_pending+0xbc/0xc0)
>  [   51.656402] [<c010bb38>] (do_work_pending) from [<c01077c8>] (slow_work_pending+0xc/0x20)
>
>
> Regards,
>
> Tony

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v7] ASoC: omap-mcbsp: Add PM QoS support for McBSP to prevent glitches
  2017-01-13 15:20 ` Tony Lindgren
  2017-01-13 17:57   ` Matt Ranostay
@ 2017-01-14  5:15   ` Matt Ranostay
  2017-01-16 17:17     ` Tony Lindgren
  1 sibling, 1 reply; 8+ messages in thread
From: Matt Ranostay @ 2017-01-14  5:15 UTC (permalink / raw)
  To: Tony Lindgren; +Cc: Peter Ujfalusi, alsa-devel, Linux OMAP List

On Fri, Jan 13, 2017 at 7:20 AM, Tony Lindgren <tony@atomide.com> wrote:
> * Matt Ranostay <matt@ranostay.consulting> [170112 19:55]:
>> We can get audio errors if hitting deeper idle states on omaps:
>>
>> [alsa.c:230] error: Fatal problem with alsa output, error -5.
>> [audio.c:614] error: Error in writing audio (Input/output error?)!
>>
>> This seems to happen with off mode idle enabled as power for the
>> whole SoC may get cut off between filling the McBSP fifo using DMA.
>> While active DMA blocks deeper idle states in hardware, McBSP
>> activity does not seem to do so.
>>
>> Basing the QoS latency calculation on the FIFO size, threshold,
>> sample rate, and channels.
>>
>> Based on the original patch by Tony Lindgren
>> Link: https://patchwork.kernel.org/patch/9305867/
>>
>> Cc: Tony Lindgren <tony@atomide.com>
>> Cc: Peter Ujfalusi <peter.ujfalusi@ti.com>
>> Acked-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
>> Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
>> ---
>> Changes from v1:
>> * add calculations for latency per number of FIFO locations
>>
>> Changes from v2:
>> * add missing mcbsp.h header change
>>
>> Changes from v3:
>> * base the latency calculations on threshold, buffer size, sample
>>   rate, and channels
>>
>> Changes from v4:
>> * using Peter Ujfalusi's suggestions for restoring a higher latency on
>>   audio stream completion, or if not applicable remove the QoS request
>>
>> Changes from v5:
>> * clean up latency checking logic
>> * move logic to .prepare and .shutdown to avoid functions that can sleep
>>
>> Changes from v6:
>> * move QoS removal to asoc_mcbsp_remove from omap_mcbsp_cleanup
>> * also remove header include that is unneeded
>
> Hmm now I'm seeing these when starting or stopping playback:

Ok I can't reproduce but I have an idea why it is happening. There is
likely a race condition between pm_qos_remove_request() and
omap_mcbsp_dai_shutdown()... Should we do a pm_qos_request_active
check in omap_mcbsp_dai_shutdown? That seems to be hacking around the
issue.

Thanks,

Matt


>
> WARNING: CPU: 0 PID: 373 at kernel/power/qos.c:534 omap_mcbsp_dai_shutdown+0x6c/0x70 [snd_soc_omap_mcbsp]
> pm_qos_remove_request() called for unknown object
> Modules linked in: snd_soc_omap_twl4030 snd_soc_twl4030 snd_soc_omap_mcbsp evdev mwifiex_sdio mwifiex leds_gpio
>  snd_soc_omap cfg80211 bq27xxx_battery_i2c leds_pca963x bq27xxx_battery led_class bq24190_charger phy_twl4030_usb omap2430 mus
>  b_hdrc usbcore snd_soc_core twl4030_wdt snd_pcm_dmaengine rtc_twl snd_pcm snd_timer snd soundcore twl4030_pwrbutton pwm_twl_le
>  d twl4030_keypad pwm_twl matrix_keymap omap_ssi hsi ledtrig_timer usb_f_serial usb_f_rndis usb_f_mass_storage usb_f_ecm u_ethe
>  r usb_f_acm libcomposite u_serial udc_core usb_common autofs4 [last unloaded: snd_soc_omap_mcbsp]
>  [   51.542541] CPU: 0 PID: 373 Comm: mpg123 Not tainted 4.10.0-rc2-next-20170109+ #751
>  [   51.550231] Hardware name: Generic OMAP36xx (Flattened Device Tree)
>  [   51.556579] [<c0110198>] (unwind_backtrace) from [<c010c208>] (show_stack+0x10/0x14)
>  [   51.564361] [<c010c208>] (show_stack) from [<c04be0a0>] (dump_stack+0xac/0xe0)
>  [   51.571655] [<c04be0a0>] (dump_stack) from [<c0137428>] (__warn+0xd8/0x104)
>  [   51.578643] [<c0137428>] (__warn) from [<c0137488>] (warn_slowpath_fmt+0x34/0x44)
>  [   51.586212] [<c0137488>] (warn_slowpath_fmt) from [<bf3538f4>] (omap_mcbsp_dai_shutdown+0x6c/0x70 [snd_soc_omap_mcbsp])
>  [   51.597351] [<bf3538f4>] (omap_mcbsp_dai_shutdown [snd_soc_omap_mcbsp]) from [<bf11037c>] (soc_pcm_close+0xa0/0x2b8 [snd_so
>  c_core])
>  [   51.609497] [<bf11037c>] (soc_pcm_close [snd_soc_core]) from [<bf0d0654>] (snd_pcm_release_substream.part.13+0x48/0xac [snd
>  _pcm])
>  [   51.621337] [<bf0d0654>] (snd_pcm_release_substream.part.13 [snd_pcm]) from [<bf0d0764>] (snd_pcm_release+0x94/0xb4 [snd_pc
>  m])
>  [   51.632843] [<bf0d0764>] (snd_pcm_release [snd_pcm]) from [<c02b91d8>] (__fput+0x94/0x1e4)
>  [   51.641174] [<c02b91d8>] (__fput) from [<c015b768>] (task_work_run+0xcc/0x104)
>  [   51.648437] [<c015b768>] (task_work_run) from [<c010bb38>] (do_work_pending+0xbc/0xc0)
>  [   51.656402] [<c010bb38>] (do_work_pending) from [<c01077c8>] (slow_work_pending+0xc/0x20)
>
>
> Regards,
>
> Tony

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v7] ASoC: omap-mcbsp: Add PM QoS support for McBSP to prevent glitches
  2017-01-14  5:15   ` Matt Ranostay
@ 2017-01-16 17:17     ` Tony Lindgren
  2017-01-16 19:30       ` Matt Ranostay
  0 siblings, 1 reply; 8+ messages in thread
From: Tony Lindgren @ 2017-01-16 17:17 UTC (permalink / raw)
  To: Matt Ranostay; +Cc: Peter Ujfalusi, alsa-devel, Linux OMAP List

* Matt Ranostay <matt@ranostay.consulting> [170113 21:16]:
> On Fri, Jan 13, 2017 at 7:20 AM, Tony Lindgren <tony@atomide.com> wrote:
> > * Matt Ranostay <matt@ranostay.consulting> [170112 19:55]:
> >> We can get audio errors if hitting deeper idle states on omaps:
> >>
> >> [alsa.c:230] error: Fatal problem with alsa output, error -5.
> >> [audio.c:614] error: Error in writing audio (Input/output error?)!
> >>
> >> This seems to happen with off mode idle enabled as power for the
> >> whole SoC may get cut off between filling the McBSP fifo using DMA.
> >> While active DMA blocks deeper idle states in hardware, McBSP
> >> activity does not seem to do so.
> >>
> >> Basing the QoS latency calculation on the FIFO size, threshold,
> >> sample rate, and channels.
> >>
> >> Based on the original patch by Tony Lindgren
> >> Link: https://patchwork.kernel.org/patch/9305867/
> >>
> >> Cc: Tony Lindgren <tony@atomide.com>
> >> Cc: Peter Ujfalusi <peter.ujfalusi@ti.com>
> >> Acked-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> >> Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
> >> ---
> >> Changes from v1:
> >> * add calculations for latency per number of FIFO locations
> >>
> >> Changes from v2:
> >> * add missing mcbsp.h header change
> >>
> >> Changes from v3:
> >> * base the latency calculations on threshold, buffer size, sample
> >>   rate, and channels
> >>
> >> Changes from v4:
> >> * using Peter Ujfalusi's suggestions for restoring a higher latency on
> >>   audio stream completion, or if not applicable remove the QoS request
> >>
> >> Changes from v5:
> >> * clean up latency checking logic
> >> * move logic to .prepare and .shutdown to avoid functions that can sleep
> >>
> >> Changes from v6:
> >> * move QoS removal to asoc_mcbsp_remove from omap_mcbsp_cleanup
> >> * also remove header include that is unneeded
> >
> > Hmm now I'm seeing these when starting or stopping playback:
> 
> Ok I can't reproduce but I have an idea why it is happening. There is
> likely a race condition between pm_qos_remove_request() and
> omap_mcbsp_dai_shutdown()... Should we do a pm_qos_request_active
> check in omap_mcbsp_dai_shutdown? That seems to be hacking around the
> issue.

OK. Again this is with next using omap2plus_defconfig and:

CONFIG_DEBUG_LOCKDEP=y
CONFIG_DEBUG_ATOMIC_SLEEP=y

Regards,

Tony

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v7] ASoC: omap-mcbsp: Add PM QoS support for McBSP to prevent glitches
  2017-01-16 17:17     ` Tony Lindgren
@ 2017-01-16 19:30       ` Matt Ranostay
  2017-01-17 12:34         ` Peter Ujfalusi
  0 siblings, 1 reply; 8+ messages in thread
From: Matt Ranostay @ 2017-01-16 19:30 UTC (permalink / raw)
  To: Tony Lindgren; +Cc: Peter Ujfalusi, alsa-devel, Linux OMAP List

On Mon, Jan 16, 2017 at 9:17 AM, Tony Lindgren <tony@atomide.com> wrote:
> * Matt Ranostay <matt@ranostay.consulting> [170113 21:16]:
>> On Fri, Jan 13, 2017 at 7:20 AM, Tony Lindgren <tony@atomide.com> wrote:
>> > * Matt Ranostay <matt@ranostay.consulting> [170112 19:55]:
>> >> We can get audio errors if hitting deeper idle states on omaps:
>> >>
>> >> [alsa.c:230] error: Fatal problem with alsa output, error -5.
>> >> [audio.c:614] error: Error in writing audio (Input/output error?)!
>> >>
>> >> This seems to happen with off mode idle enabled as power for the
>> >> whole SoC may get cut off between filling the McBSP fifo using DMA.
>> >> While active DMA blocks deeper idle states in hardware, McBSP
>> >> activity does not seem to do so.
>> >>
>> >> Basing the QoS latency calculation on the FIFO size, threshold,
>> >> sample rate, and channels.
>> >>
>> >> Based on the original patch by Tony Lindgren
>> >> Link: https://patchwork.kernel.org/patch/9305867/
>> >>
>> >> Cc: Tony Lindgren <tony@atomide.com>
>> >> Cc: Peter Ujfalusi <peter.ujfalusi@ti.com>
>> >> Acked-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
>> >> Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
>> >> ---
>> >> Changes from v1:
>> >> * add calculations for latency per number of FIFO locations
>> >>
>> >> Changes from v2:
>> >> * add missing mcbsp.h header change
>> >>
>> >> Changes from v3:
>> >> * base the latency calculations on threshold, buffer size, sample
>> >>   rate, and channels
>> >>
>> >> Changes from v4:
>> >> * using Peter Ujfalusi's suggestions for restoring a higher latency on
>> >>   audio stream completion, or if not applicable remove the QoS request
>> >>
>> >> Changes from v5:
>> >> * clean up latency checking logic
>> >> * move logic to .prepare and .shutdown to avoid functions that can sleep
>> >>
>> >> Changes from v6:
>> >> * move QoS removal to asoc_mcbsp_remove from omap_mcbsp_cleanup
>> >> * also remove header include that is unneeded
>> >
>> > Hmm now I'm seeing these when starting or stopping playback:
>>
>> Ok I can't reproduce but I have an idea why it is happening. There is
>> likely a race condition between pm_qos_remove_request() and
>> omap_mcbsp_dai_shutdown()... Should we do a pm_qos_request_active
>> check in omap_mcbsp_dai_shutdown? That seems to be hacking around the
>> issue.
>
> OK. Again this is with next using omap2plus_defconfig and:
>
> CONFIG_DEBUG_LOCKDEP=y
> CONFIG_DEBUG_ATOMIC_SLEEP=y

Yep I was testing with that enabled. Not sure why I'm not getting it
all... maybe different side effects like sample rate of the audio
being played?

Peter, is there chance that there is race condition is possible
between pm_qos_remove_request and omap_mcbsp_dai_shutdown?

>
> Regards,
>
> Tony

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v7] ASoC: omap-mcbsp: Add PM QoS support for McBSP to prevent glitches
  2017-01-16 19:30       ` Matt Ranostay
@ 2017-01-17 12:34         ` Peter Ujfalusi
  2017-01-17 23:04           ` Matt Ranostay
  0 siblings, 1 reply; 8+ messages in thread
From: Peter Ujfalusi @ 2017-01-17 12:34 UTC (permalink / raw)
  To: Matt Ranostay, Tony Lindgren; +Cc: alsa-devel, Linux OMAP List

On 01/16/2017 09:30 PM, Matt Ranostay wrote:
> Yep I was testing with that enabled. Not sure why I'm not getting it
> all... maybe different side effects like sample rate of the audio
> being played?
> 
> Peter, is there chance that there is race condition is possible
> between pm_qos_remove_request and omap_mcbsp_dai_shutdown?

Not with my knowledge.
However one thing might happen which is not handled:
application opens up the audio card, does some hw_param setup, then it
decides to close the card. In this case the pm_qos is not placed, but in
dai_shutdown we try to remove it -> WARN().

I think we need to check either if the pm_qos is active and only then
bother with the removal/updated or we can check if the
mcbsp->latency[stream1] is not 0, which should be the same as checking
for the pm_qos active.

-- 
Péter
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v7] ASoC: omap-mcbsp: Add PM QoS support for McBSP to prevent glitches
  2017-01-17 12:34         ` Peter Ujfalusi
@ 2017-01-17 23:04           ` Matt Ranostay
  0 siblings, 0 replies; 8+ messages in thread
From: Matt Ranostay @ 2017-01-17 23:04 UTC (permalink / raw)
  To: Peter Ujfalusi; +Cc: Tony Lindgren, alsa-devel, Linux OMAP List

On Tue, Jan 17, 2017 at 4:34 AM, Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
> On 01/16/2017 09:30 PM, Matt Ranostay wrote:
>> Yep I was testing with that enabled. Not sure why I'm not getting it
>> all... maybe different side effects like sample rate of the audio
>> being played?
>>
>> Peter, is there chance that there is race condition is possible
>> between pm_qos_remove_request and omap_mcbsp_dai_shutdown?
>
> Not with my knowledge.
> However one thing might happen which is not handled:
> application opens up the audio card, does some hw_param setup, then it
> decides to close the card. In this case the pm_qos is not placed, but in
> dai_shutdown we try to remove it -> WARN().
>
> I think we need to check either if the pm_qos is active and only then
> bother with the removal/updated or we can check if the
> mcbsp->latency[stream1] is not 0, which should be the same as checking
> for the pm_qos active

Ok makes sense. Weird I'm not seeing it but I wanted to be sure adding
checks wasn't just hiding a issue.
Will submit v8...  and hopefully that is the final one! *fingers crossed*

>
> --
> Péter
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2017-01-17 23:04 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-01-13  3:54 [PATCH v7] ASoC: omap-mcbsp: Add PM QoS support for McBSP to prevent glitches Matt Ranostay
2017-01-13 15:20 ` Tony Lindgren
2017-01-13 17:57   ` Matt Ranostay
2017-01-14  5:15   ` Matt Ranostay
2017-01-16 17:17     ` Tony Lindgren
2017-01-16 19:30       ` Matt Ranostay
2017-01-17 12:34         ` Peter Ujfalusi
2017-01-17 23:04           ` Matt Ranostay

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).