PM issue with Intel SST Atom driver

PM issue with Intel SST Atom driver

[Takashi Iwai]

Hi,

I noticed that the unstable PM behavior on my Cherrytrail laptop
actually comes from the sound driver.  First off, the atom/sst/sst.c
has the following suspend code:

static int intel_sst_suspend(struct device *dev)
{
....
	/*
	 * check if any stream is active and running
	 * they should already by suspend by soc_suspend
	 */
	for (i = 1; i <= ctx->info.max_streams; i++) {
		struct stream_info *stream = &ctx->streams[i];

		if (stream->status == STREAM_RUNNING) {
			dev_err(dev, "stream %d is running, can't suspend, abort\n", i);
			return -EBUSY;
		}
	}

This doesn't look good, and I actually hit this when I tried to
suspend while playing something.  In general, the driver shouldn't
reject at this point, because this is the PM suspend callback,
i.e. the user wants to suspend the device inevitably.  The driver
should return an error only when it faces to a fatal error.

But I wondered why this happened at all, and noticed that the machine
driver (in my case bytcr_rt5640) has no its own PM ops.  But hooking
the snd_soc_pm_ops there seems causing a hang up at suspend by some
reason.

Maybe this wasn't a big problem until now since the BYT/CHT didn't
support the suspend/resume properly in the past.  But now PM suspend
is supported on these devices, so the problem surfaced more often.

Could you guys check the status of these drivers?


thanks,

Takashi

Re: PM issue with Intel SST Atom driver

[Vinod Koul]

On Fri, Apr 21, 2017 at 03:42:43PM +0200, Takashi Iwai wrote:
> Hi,
> 
> I noticed that the unstable PM behavior on my Cherrytrail laptop
> actually comes from the sound driver.  First off, the atom/sst/sst.c
> has the following suspend code:
> 
> static int intel_sst_suspend(struct device *dev)
> {
> ....
> 	/*
> 	 * check if any stream is active and running
> 	 * they should already by suspend by soc_suspend
> 	 */
> 	for (i = 1; i <= ctx->info.max_streams; i++) {
> 		struct stream_info *stream = &ctx->streams[i];
> 
> 		if (stream->status == STREAM_RUNNING) {
> 			dev_err(dev, "stream %d is running, can't suspend, abort\n", i);
> 			return -EBUSY;
> 		}
> 	}
> 
> This doesn't look good, and I actually hit this when I tried to
> suspend while playing something.  In general, the driver shouldn't
> reject at this point, because this is the PM suspend callback,
> i.e. the user wants to suspend the device inevitably.  The driver
> should return an error only when it faces to a fatal error.

Mea culpa

And you are now second person to complain about this so I wonder if we
should rework this

So from hardware POV, we need to first suspend all running streams and then
save the context from DSP (in order to restore them later) and then we can
shutdown the DSP.

The problem is driver being split into platform (which knows streams) and
sst dsp driver. So we devised a careful sequence where platform suspend is
invoked first (calls using asoc pm ops) and then DSP

This results is ASoC doing stream suspend for us and when we hit
intel_sst_suspend() we are supposed to have stream suspended, so save and
shut off DSP.

Now for you issue you see can you check why platform suspend is not getting
called?

> 
> But I wondered why this happened at all, and noticed that the machine
> driver (in my case bytcr_rt5640) has no its own PM ops.  But hooking
> the snd_soc_pm_ops there seems causing a hang up at suspend by some
> reason.

O yes, thats due to double suspend

See 3639ac1cd5177685a5c8abb7230096b680e1d497

> 
> Maybe this wasn't a big problem until now since the BYT/CHT didn't
> support the suspend/resume properly in the past.  But now PM suspend
> is supported on these devices, so the problem surfaced more often.

The Chromebooks shipped on BSW use this method so..

> Could you guys check the status of these drivers?

-- 
~Vinod

Re: PM issue with Intel SST Atom driver

[Takashi Iwai]

On Mon, 24 Apr 2017 07:01:44 +0200,
Vinod Koul wrote:
> 
> On Fri, Apr 21, 2017 at 03:42:43PM +0200, Takashi Iwai wrote:
> > Hi,
> > 
> > I noticed that the unstable PM behavior on my Cherrytrail laptop
> > actually comes from the sound driver.  First off, the atom/sst/sst.c
> > has the following suspend code:
> > 
> > static int intel_sst_suspend(struct device *dev)
> > {
> > ....
> > 	/*
> > 	 * check if any stream is active and running
> > 	 * they should already by suspend by soc_suspend
> > 	 */
> > 	for (i = 1; i <= ctx->info.max_streams; i++) {
> > 		struct stream_info *stream = &ctx->streams[i];
> > 
> > 		if (stream->status == STREAM_RUNNING) {
> > 			dev_err(dev, "stream %d is running, can't suspend, abort\n", i);
> > 			return -EBUSY;
> > 		}
> > 	}
> > 
> > This doesn't look good, and I actually hit this when I tried to
> > suspend while playing something.  In general, the driver shouldn't
> > reject at this point, because this is the PM suspend callback,
> > i.e. the user wants to suspend the device inevitably.  The driver
> > should return an error only when it faces to a fatal error.
> 
> Mea culpa
> 
> And you are now second person to complain about this so I wonder if we
> should rework this

Well, something is definitely wrong :)

> So from hardware POV, we need to first suspend all running streams and then
> save the context from DSP (in order to restore them later) and then we can
> shutdown the DSP.
> 
> The problem is driver being split into platform (which knows streams) and
> sst dsp driver. So we devised a careful sequence where platform suspend is
> invoked first (calls using asoc pm ops) and then DSP
> 
> This results is ASoC doing stream suspend for us and when we hit
> intel_sst_suspend() we are supposed to have stream suspended, so save and
> shut off DSP.
> 
> Now for you issue you see can you check why platform suspend is not getting
> called?
> 
> > 
> > But I wondered why this happened at all, and noticed that the machine
> > driver (in my case bytcr_rt5640) has no its own PM ops.  But hooking
> > the snd_soc_pm_ops there seems causing a hang up at suspend by some
> > reason.
> 
> O yes, thats due to double suspend
> 
> See 3639ac1cd5177685a5c8abb7230096b680e1d497

I haven't followed the code deeply enough.  Who is calling to trigger
double-suspend?


> > Maybe this wasn't a big problem until now since the BYT/CHT didn't
> > support the suspend/resume properly in the past.  But now PM suspend
> > is supported on these devices, so the problem surfaced more often.
> 
> The Chromebooks shipped on BSW use this method so..

Interestingly, when I checked another CHT machine with cx2072x codec,
the PM works (although the playback doesn't restart at resume
properly).

Wait...  Now closely looking at the code, I noticed the
"ignore_suspend" marks in many places in bytcr_rt5640.c.  Why is this
needed?

Other two machine drivers I've tested (cht_bsw_rt5672 and Pierre's
cht_cx2072x) have no such a flag set, thus they work.  With the
ignore_suspend, the PCM suspend calls are prevented, and it shall hit
the problem above.


thanks,

Takashi

Re: PM issue with Intel SST Atom driver

[Takashi Iwai]

On Mon, 24 Apr 2017 09:15:04 +0200,
Takashi Iwai wrote:
> 
> > > But I wondered why this happened at all, and noticed that the machine
> > > driver (in my case bytcr_rt5640) has no its own PM ops.  But hooking
> > > the snd_soc_pm_ops there seems causing a hang up at suspend by some
> > > reason.
> > 
> > O yes, thats due to double suspend
> > 
> > See 3639ac1cd5177685a5c8abb7230096b680e1d497
> 
> I haven't followed the code deeply enough.  Who is calling to trigger
> double-suspend?

Never mind, I figured out that it's in sst_soc_prepare().
So, it's specific to Atom (and Haswell).


 > > Maybe this wasn't a big problem until now since the BYT/CHT didn't
> > > support the suspend/resume properly in the past.  But now PM suspend
> > > is supported on these devices, so the problem surfaced more often.
> > 
> > The Chromebooks shipped on BSW use this method so..
> 
> Interestingly, when I checked another CHT machine with cx2072x codec,
> the PM works (although the playback doesn't restart at resume
> properly).
> 
> Wait...  Now closely looking at the code, I noticed the
> "ignore_suspend" marks in many places in bytcr_rt5640.c.  Why is this
> needed?
> 
> Other two machine drivers I've tested (cht_bsw_rt5672 and Pierre's
> cht_cx2072x) have no such a flag set, thus they work.  With the
> ignore_suspend, the PCM suspend calls are prevented, and it shall hit
> the problem above.

Removing ignore_suspend makes the PM succeeds.  But it hits some other
ugly kernel bugs.

At suspending:

[  567.913706] WARNING: CPU: 3 PID: 3144 at ../kernel/softirq.c:161 __local_bh_enable_ip+0x71/0x90
[  567.913842] CPU: 3 PID: 3144 Comm: systemd-sleep Tainted: G         C O    4.11.0-rc7-3.g64b92e2-default #1
[  567.913847] Call Trace:
[  567.913861]  dump_stack+0x5c/0x7a
[  567.913869]  __warn+0xbe/0xe0
[  567.913879]  __local_bh_enable_ip+0x71/0x90
[  567.913891]  sst_create_block+0x83/0xd0 [snd_intel_sst_core]
[  567.913906]  sst_create_block_and_ipc_msg+0x4a/0x70 [snd_intel_sst_core]
[  567.913920]  sst_prepare_and_post_msg+0x1a0/0x960 [snd_intel_sst_core]
[  567.913936]  sst_pause_stream+0x9b/0x110 [snd_intel_sst_core]
[  567.913952]  sst_platform_pcm_trigger+0x123/0x1b0 [snd_soc_sst_atom_hifi2_platform]
[  567.913980]  soc_pcm_trigger+0xa0/0x120 [snd_soc_core]
[  567.913996]  ? sst_soc_complete+0xa0/0xa0 [snd_soc_sst_atom_hifi2_platform]
[  567.914012]  dpcm_fe_dai_do_trigger+0xc8/0x1f0 [snd_soc_core]
[  567.914034]  snd_pcm_do_suspend+0x3d/0x40 [snd_pcm]
[  567.914054]  snd_pcm_action_single+0x2a/0x70 [snd_pcm]
[  567.914065]  snd_pcm_suspend+0x2c/0x40 [snd_pcm]
[  567.914076]  snd_pcm_suspend_all+0x32/0x70 [snd_pcm]
[  567.914092]  snd_soc_suspend+0x15c/0x3d0 [snd_soc_core]
[  567.914102]  sst_soc_prepare+0x23/0xa0 [snd_soc_sst_atom_hifi2_platform]
[  567.914108]  dpm_prepare+0x237/0x480
[  567.914113]  dpm_suspend_start+0xd/0x50
[  567.914117]  suspend_devices_and_enter+0xac/0x6f0
[  567.914123]  pm_suspend+0x304/0x380
[  567.914128]  state_store+0x5e/0x90
[  567.914134]  kernfs_fop_write+0xfc/0x190
[  567.914140]  __vfs_write+0x23/0x140
[  567.914146]  ? handle_mm_fault+0xd3/0x240
[  567.914148]  ? security_file_permission+0x36/0xb0
[  567.914151]  vfs_write+0xb0/0x190
[  567.914156]  SyS_write+0x42/0x90
[  567.914160]  entry_SYSCALL_64_fastpath+0x1e/0xad
[  567.914164] ---[ end trace b3703d94611f9a06 ]---
[  567.914176] BUG: scheduling while atomic: systemd-sleep/3144/0x00000003
[  567.914260] CPU: 3 PID: 3144 Comm: systemd-sleep Tainted: G        WC O    4.11.0-rc7-3.g64b92e2-default #1
[  567.914262] Call Trace:
[  567.914273]  dump_stack+0x5c/0x7a
[  567.914278]  __schedule_bug+0x55/0x70
[  567.914284]  __schedule+0x63c/0x8c0
[  567.914290]  schedule+0x3d/0x90
[  567.914295]  schedule_timeout+0x16b/0x320
[  567.914301]  ? del_timer_sync+0x50/0x50
[  567.914308]  ? sst_wait_timeout+0xa9/0x170 [snd_intel_sst_core]
[  567.914313]  ? sst_wait_timeout+0xa9/0x170 [snd_intel_sst_core]
[  567.914316]  ? remove_wait_queue+0x60/0x60
[  567.914321]  ? sst_prepare_and_post_msg+0x275/0x960 [snd_intel_sst_core]
[  567.914326]  ? sst_pause_stream+0x9b/0x110 [snd_intel_sst_core]
[  567.914333]  ? sst_platform_pcm_trigger+0x123/0x1b0 [snd_soc_sst_atom_hifi2_platform]
[  567.914346]  ? soc_pcm_trigger+0xa0/0x120 [snd_soc_core]
[  567.914353]  ? sst_soc_complete+0xa0/0xa0 [snd_soc_sst_atom_hifi2_platform]
[  567.914365]  ? dpcm_fe_dai_do_trigger+0xc8/0x1f0 [snd_soc_core]
[  567.914373]  ? snd_pcm_do_suspend+0x3d/0x40 [snd_pcm]
[  567.914381]  ? snd_pcm_action_single+0x2a/0x70 [snd_pcm]
[  567.914389]  ? snd_pcm_suspend+0x2c/0x40 [snd_pcm]
[  567.914396]  ? snd_pcm_suspend_all+0x32/0x70 [snd_pcm]
[  567.914408]  ? snd_soc_suspend+0x15c/0x3d0 [snd_soc_core]
[  567.914415]  ? sst_soc_prepare+0x23/0xa0 [snd_soc_sst_atom_hifi2_platform]
[  567.914418]  ? dpm_prepare+0x237/0x480
[  567.914421]  ? dpm_suspend_start+0xd/0x50
[  567.914423]  ? suspend_devices_and_enter+0xac/0x6f0
[  567.914426]  ? pm_suspend+0x304/0x380
[  567.914428]  ? state_store+0x5e/0x90
[  567.914430]  ? kernfs_fop_write+0xfc/0x190
[  567.914433]  ? __vfs_write+0x23/0x140
[  567.914437]  ? handle_mm_fault+0xd3/0x240
[  567.914439]  ? security_file_permission+0x36/0xb0
[  567.914442]  ? vfs_write+0xb0/0x190
[  567.914445]  ? SyS_write+0x42/0x90
[  567.914447]  ? entry_SYSCALL_64_fastpath+0x1e/0xad
[  567.914857] BUG: scheduling while atomic: systemd-sleep/3144/0x7fffffff
[  567.915222] CPU: 1 PID: 3144 Comm: systemd-sleep Tainted: G        WC O    4.11.0-rc7-3.g64b92e2-default #1
[  567.915231] Call Trace:
[  567.915300]  dump_stack+0x5c/0x7a
[  567.915324]  __schedule_bug+0x55/0x70
[  567.915345]  __schedule+0x63c/0x8c0
[  567.915375]  schedule+0x3d/0x90
[  567.915392]  async_synchronize_cookie_domain+0x85/0x130
[  567.915414]  ? remove_wait_queue+0x60/0x60
[  567.915472]  dapm_power_widgets+0x3d4/0x9c0 [snd_soc_core]
[  567.915530]  ? sst_soc_complete+0xa0/0xa0 [snd_soc_sst_atom_hifi2_platform]
[  567.915579]  ? snd_soc_dapm_stream_event+0x87/0xa0 [snd_soc_core]
[  567.915628]  ? snd_soc_dapm_stream_event+0x87/0xa0 [snd_soc_core]
[  567.915674]  ? snd_soc_suspend+0x39c/0x3d0 [snd_soc_core]
[  567.915699]  ? sst_soc_prepare+0x23/0xa0 [snd_soc_sst_atom_hifi2_platform]
[  567.915712]  ? dpm_prepare+0x237/0x480
[  567.915723]  ? dpm_suspend_start+0xd/0x50
[  567.915738]  ? suspend_devices_and_enter+0xac/0x6f0
[  567.915749]  ? pm_suspend+0x304/0x380
[  567.915757]  ? state_store+0x5e/0x90
[  567.915771]  ? kernfs_fop_write+0xfc/0x190
[  567.915785]  ? __vfs_write+0x23/0x140
[  567.915800]  ? handle_mm_fault+0xd3/0x240
[  567.915814]  ? security_file_permission+0x36/0xb0
[  567.915824]  ? vfs_write+0xb0/0x190
[  567.915834]  ? SyS_write+0x42/0x90
[  567.915846]  ? entry_SYSCALL_64_fastpath+0x1e/0xad


... and at resume:

[  574.320255] BUG: scheduling while atomic: alsa-source-3/1729/0x00000003
[  574.320435] CPU: 1 PID: 1729 Comm: alsa-source-3 Tainted: G        WC O    4.11.0-rc7-3.g64b92e2-default #1
[  574.320440] Call Trace:
[  574.320474]  dump_stack+0x5c/0x7a
[  574.320484]  __schedule_bug+0x55/0x70
[  574.320493]  __schedule+0x63c/0x8c0
[  574.320503]  schedule+0x3d/0x90
[  574.320509]  schedule_timeout+0x16b/0x320
[  574.320517]  ? del_timer_sync+0x50/0x50
[  574.320529]  ? sst_wait_timeout+0xa9/0x170 [snd_intel_sst_core]
[  574.320535]  ? sst_wait_timeout+0xa9/0x170 [snd_intel_sst_core]
[  574.320539]  ? remove_wait_queue+0x60/0x60
[  574.320546]  ? sst_prepare_and_post_msg+0x275/0x960 [snd_intel_sst_core]
[  574.320553]  ? sst_resume_stream+0x5b/0x100 [snd_intel_sst_core]
[  574.320564]  ? sst_platform_pcm_trigger+0x6b/0x1b0 [snd_soc_sst_atom_hifi2_platform]
[  574.320586]  ? soc_pcm_trigger+0xa0/0x120 [snd_soc_core]
[  574.320603]  ? dpcm_fe_dai_do_trigger+0xc8/0x1f0 [snd_soc_core]
[  574.320618]  ? snd_pcm_action_single+0x2a/0x70 [snd_pcm]
[  574.320628]  ? snd_pcm_common_ioctl1+0x2e5/0xc60 [snd_pcm]
[  574.320634]  ? do_signal+0x23/0x670
[  574.320644]  ? snd_pcm_capture_ioctl1+0x117/0x280 [snd_pcm]
[  574.320648]  ? ktime_get_ts64+0x4a/0xf0
[  574.320659]  ? snd_pcm_capture_ioctl+0x2a/0x30 [snd_pcm]
[  574.320663]  ? do_vfs_ioctl+0x8f/0x5d0
[  574.320667]  ? __fget+0x70/0xc0
[  574.320671]  ? SyS_ioctl+0x74/0x80
[  574.320675]  ? entry_SYSCALL_64_fastpath+0x1e/0xad
[  574.320799] show_signal_msg: 8 callbacks suppressed
[  574.320805] alsa-source-3[1729]: segfault at 7f28e8052000 ip 00007f28f635eac4 sp 00007f28f0ad4be8 error 6
[  574.320882] BUG: scheduling while atomic: alsa-source-3/1729/0x7fffffff
[  574.321040] CPU: 1 PID: 1729 Comm: alsa-source-3 Tainted: G        WC O    4.11.0-rc7-3.g64b92e2-default #1
[  574.321043] Call Trace:
[  574.321068]  dump_stack+0x5c/0x7a
[  574.321076]  __schedule_bug+0x55/0x70
[  574.321083]  __schedule+0x63c/0x8c0
[  574.321093]  ? wakeup_preempt_entity.isra.58+0x3c/0x50
[  574.321097]  schedule+0x3d/0x90
[  574.321104]  schedule_timeout+0x25a/0x320
[  574.321109]  ? check_preempt_curr+0x79/0x90
[  574.321113]  ? select_task_rq_rt+0x57/0xa0
[  574.321117]  ? sched_clock+0x5/0x10
[  574.321120]  ? sched_clock_cpu+0xc/0xc0
[  574.321124]  ? wait_for_completion+0xe6/0x150
[  574.321128]  ? wait_for_completion+0xe6/0x150
[  574.321131]  ? wake_up_q+0x80/0x80
[  574.321135]  ? do_coredump+0x3ab/0xf10
[  574.321139]  ? __wake_up+0x34/0x50
[  574.321144]  ? get_signal+0x33b/0x660
[  574.321150]  ? do_signal+0x23/0x670
[  574.321154]  ? __send_signal+0x213/0x4d0
[  574.321160]  ? force_sig_info_fault+0x88/0xd0
[  574.321166]  ? exit_to_usermode_loop+0x71/0xb0
[  574.321169]  ? prepare_exit_to_usermode+0x2a/0x30
[  574.321172]  ? retint_user+0x8/0x10


There seem many beasts hidden in this jungle...


Takashi

Re: PM issue with Intel SST Atom driver

[Vinod Koul]

On Mon, Apr 24, 2017 at 09:15:04AM +0200, Takashi Iwai wrote:
> On Mon, 24 Apr 2017 07:01:44 +0200,
> Vinod Koul wrote:
> > 
> > On Fri, Apr 21, 2017 at 03:42:43PM +0200, Takashi Iwai wrote:
> > > Hi,
> > > 
> > > I noticed that the unstable PM behavior on my Cherrytrail laptop
> > > actually comes from the sound driver.  First off, the atom/sst/sst.c
> > > has the following suspend code:
> > > 
> > > static int intel_sst_suspend(struct device *dev)
> > > {
> > > ....
> > > 	/*
> > > 	 * check if any stream is active and running
> > > 	 * they should already by suspend by soc_suspend
> > > 	 */
> > > 	for (i = 1; i <= ctx->info.max_streams; i++) {
> > > 		struct stream_info *stream = &ctx->streams[i];
> > > 
> > > 		if (stream->status == STREAM_RUNNING) {
> > > 			dev_err(dev, "stream %d is running, can't suspend, abort\n", i);
> > > 			return -EBUSY;
> > > 		}
> > > 	}
> > > 
> > > This doesn't look good, and I actually hit this when I tried to
> > > suspend while playing something.  In general, the driver shouldn't
> > > reject at this point, because this is the PM suspend callback,
> > > i.e. the user wants to suspend the device inevitably.  The driver
> > > should return an error only when it faces to a fatal error.
> > 
> > Mea culpa
> > 
> > And you are now second person to complain about this so I wonder if we
> > should rework this
> 
> Well, something is definitely wrong :)
> 
> > So from hardware POV, we need to first suspend all running streams and then
> > save the context from DSP (in order to restore them later) and then we can
> > shutdown the DSP.
> > 
> > The problem is driver being split into platform (which knows streams) and
> > sst dsp driver. So we devised a careful sequence where platform suspend is
> > invoked first (calls using asoc pm ops) and then DSP
> > 
> > This results is ASoC doing stream suspend for us and when we hit
> > intel_sst_suspend() we are supposed to have stream suspended, so save and
> > shut off DSP.
> > 
> > Now for you issue you see can you check why platform suspend is not getting
> > called?
> > 
> > > 
> > > But I wondered why this happened at all, and noticed that the machine
> > > driver (in my case bytcr_rt5640) has no its own PM ops.  But hooking
> > > the snd_soc_pm_ops there seems causing a hang up at suspend by some
> > > reason.
> > 
> > O yes, thats due to double suspend
> > 
> > See 3639ac1cd5177685a5c8abb7230096b680e1d497
> 
> I haven't followed the code deeply enough.  Who is calling to trigger
> double-suspend?

the machine and the platform see sst_soc_prepare()

> 
> 
> > > Maybe this wasn't a big problem until now since the BYT/CHT didn't
> > > support the suspend/resume properly in the past.  But now PM suspend
> > > is supported on these devices, so the problem surfaced more often.
> > 
> > The Chromebooks shipped on BSW use this method so..
> 
> Interestingly, when I checked another CHT machine with cx2072x codec,
> the PM works (although the playback doesn't restart at resume
> properly).

okay which machine driver was for cx2072x and which one are you using. I can
take a look at the code

> Wait...  Now closely looking at the code, I noticed the
> "ignore_suspend" marks in many places in bytcr_rt5640.c.  Why is this
> needed?
> 
> Other two machine drivers I've tested (cht_bsw_rt5672 and Pierre's
> cht_cx2072x) have no such a flag set, thus they work.  With the
> ignore_suspend, the PCM suspend calls are prevented, and it shall hit
> the problem above.

So ignore_suspend is used to keep PM on those devices even when platform is
suspended. It is quite used in keeping BIAS on when suspend, or doing
modem-codec interactions when SoC is in suspend.

I don't think we need this for a generic PC/laptop case, so removing them
should do the trick.

Use the working machine as ref :)

-- 
~Vinod

Re: PM issue with Intel SST Atom driver

[Vinod Koul]

On Mon, Apr 24, 2017 at 11:00:45AM +0200, Takashi Iwai wrote:
 
> Removing ignore_suspend makes the PM succeeds.  But it hits some other
> ugly kernel bugs.

Okay have you marked .nonatomic = true for the machine DAIs?

> 
> At suspending:
> 
> [  567.913706] WARNING: CPU: 3 PID: 3144 at ../kernel/softirq.c:161 __local_bh_enable_ip+0x71/0x90
> [  567.913842] CPU: 3 PID: 3144 Comm: systemd-sleep Tainted: G         C O    4.11.0-rc7-3.g64b92e2-default #1
> [  567.913847] Call Trace:
> [  567.913861]  dump_stack+0x5c/0x7a
> [  567.913869]  __warn+0xbe/0xe0
> [  567.913879]  __local_bh_enable_ip+0x71/0x90
> [  567.913891]  sst_create_block+0x83/0xd0 [snd_intel_sst_core]
> [  567.913906]  sst_create_block_and_ipc_msg+0x4a/0x70 [snd_intel_sst_core]
> [  567.913920]  sst_prepare_and_post_msg+0x1a0/0x960 [snd_intel_sst_core]
> [  567.913936]  sst_pause_stream+0x9b/0x110 [snd_intel_sst_core]
> [  567.913952]  sst_platform_pcm_trigger+0x123/0x1b0 [snd_soc_sst_atom_hifi2_platform]
> [  567.913980]  soc_pcm_trigger+0xa0/0x120 [snd_soc_core]
> [  567.913996]  ? sst_soc_complete+0xa0/0xa0 [snd_soc_sst_atom_hifi2_platform]
> [  567.914012]  dpcm_fe_dai_do_trigger+0xc8/0x1f0 [snd_soc_core]
> [  567.914034]  snd_pcm_do_suspend+0x3d/0x40 [snd_pcm]
> [  567.914054]  snd_pcm_action_single+0x2a/0x70 [snd_pcm]
> [  567.914065]  snd_pcm_suspend+0x2c/0x40 [snd_pcm]
> [  567.914076]  snd_pcm_suspend_all+0x32/0x70 [snd_pcm]
> [  567.914092]  snd_soc_suspend+0x15c/0x3d0 [snd_soc_core]
> [  567.914102]  sst_soc_prepare+0x23/0xa0 [snd_soc_sst_atom_hifi2_platform]
> [  567.914108]  dpm_prepare+0x237/0x480
> [  567.914113]  dpm_suspend_start+0xd/0x50
> [  567.914117]  suspend_devices_and_enter+0xac/0x6f0
> [  567.914123]  pm_suspend+0x304/0x380
> [  567.914128]  state_store+0x5e/0x90
> [  567.914134]  kernfs_fop_write+0xfc/0x190
> [  567.914140]  __vfs_write+0x23/0x140
> [  567.914146]  ? handle_mm_fault+0xd3/0x240
> [  567.914148]  ? security_file_permission+0x36/0xb0
> [  567.914151]  vfs_write+0xb0/0x190
> [  567.914156]  SyS_write+0x42/0x90
> [  567.914160]  entry_SYSCALL_64_fastpath+0x1e/0xad
> [  567.914164] ---[ end trace b3703d94611f9a06 ]---
> [  567.914176] BUG: scheduling while atomic: systemd-sleep/3144/0x00000003
> [  567.914260] CPU: 3 PID: 3144 Comm: systemd-sleep Tainted: G        WC O    4.11.0-rc7-3.g64b92e2-default #1
> [  567.914262] Call Trace:
> [  567.914273]  dump_stack+0x5c/0x7a
> [  567.914278]  __schedule_bug+0x55/0x70
> [  567.914284]  __schedule+0x63c/0x8c0
> [  567.914290]  schedule+0x3d/0x90
> [  567.914295]  schedule_timeout+0x16b/0x320
> [  567.914301]  ? del_timer_sync+0x50/0x50
> [  567.914308]  ? sst_wait_timeout+0xa9/0x170 [snd_intel_sst_core]
> [  567.914313]  ? sst_wait_timeout+0xa9/0x170 [snd_intel_sst_core]
> [  567.914316]  ? remove_wait_queue+0x60/0x60
> [  567.914321]  ? sst_prepare_and_post_msg+0x275/0x960 [snd_intel_sst_core]
> [  567.914326]  ? sst_pause_stream+0x9b/0x110 [snd_intel_sst_core]
> [  567.914333]  ? sst_platform_pcm_trigger+0x123/0x1b0 [snd_soc_sst_atom_hifi2_platform]
> [  567.914346]  ? soc_pcm_trigger+0xa0/0x120 [snd_soc_core]
> [  567.914353]  ? sst_soc_complete+0xa0/0xa0 [snd_soc_sst_atom_hifi2_platform]
> [  567.914365]  ? dpcm_fe_dai_do_trigger+0xc8/0x1f0 [snd_soc_core]
> [  567.914373]  ? snd_pcm_do_suspend+0x3d/0x40 [snd_pcm]
> [  567.914381]  ? snd_pcm_action_single+0x2a/0x70 [snd_pcm]
> [  567.914389]  ? snd_pcm_suspend+0x2c/0x40 [snd_pcm]
> [  567.914396]  ? snd_pcm_suspend_all+0x32/0x70 [snd_pcm]
> [  567.914408]  ? snd_soc_suspend+0x15c/0x3d0 [snd_soc_core]
> [  567.914415]  ? sst_soc_prepare+0x23/0xa0 [snd_soc_sst_atom_hifi2_platform]
> [  567.914418]  ? dpm_prepare+0x237/0x480
> [  567.914421]  ? dpm_suspend_start+0xd/0x50
> [  567.914423]  ? suspend_devices_and_enter+0xac/0x6f0
> [  567.914426]  ? pm_suspend+0x304/0x380
> [  567.914428]  ? state_store+0x5e/0x90
> [  567.914430]  ? kernfs_fop_write+0xfc/0x190
> [  567.914433]  ? __vfs_write+0x23/0x140
> [  567.914437]  ? handle_mm_fault+0xd3/0x240
> [  567.914439]  ? security_file_permission+0x36/0xb0
> [  567.914442]  ? vfs_write+0xb0/0x190
> [  567.914445]  ? SyS_write+0x42/0x90
> [  567.914447]  ? entry_SYSCALL_64_fastpath+0x1e/0xad
> [  567.914857] BUG: scheduling while atomic: systemd-sleep/3144/0x7fffffff
> [  567.915222] CPU: 1 PID: 3144 Comm: systemd-sleep Tainted: G        WC O    4.11.0-rc7-3.g64b92e2-default #1
> [  567.915231] Call Trace:
> [  567.915300]  dump_stack+0x5c/0x7a
> [  567.915324]  __schedule_bug+0x55/0x70
> [  567.915345]  __schedule+0x63c/0x8c0
> [  567.915375]  schedule+0x3d/0x90
> [  567.915392]  async_synchronize_cookie_domain+0x85/0x130
> [  567.915414]  ? remove_wait_queue+0x60/0x60
> [  567.915472]  dapm_power_widgets+0x3d4/0x9c0 [snd_soc_core]
> [  567.915530]  ? sst_soc_complete+0xa0/0xa0 [snd_soc_sst_atom_hifi2_platform]
> [  567.915579]  ? snd_soc_dapm_stream_event+0x87/0xa0 [snd_soc_core]
> [  567.915628]  ? snd_soc_dapm_stream_event+0x87/0xa0 [snd_soc_core]
> [  567.915674]  ? snd_soc_suspend+0x39c/0x3d0 [snd_soc_core]
> [  567.915699]  ? sst_soc_prepare+0x23/0xa0 [snd_soc_sst_atom_hifi2_platform]
> [  567.915712]  ? dpm_prepare+0x237/0x480
> [  567.915723]  ? dpm_suspend_start+0xd/0x50
> [  567.915738]  ? suspend_devices_and_enter+0xac/0x6f0
> [  567.915749]  ? pm_suspend+0x304/0x380
> [  567.915757]  ? state_store+0x5e/0x90
> [  567.915771]  ? kernfs_fop_write+0xfc/0x190
> [  567.915785]  ? __vfs_write+0x23/0x140
> [  567.915800]  ? handle_mm_fault+0xd3/0x240
> [  567.915814]  ? security_file_permission+0x36/0xb0
> [  567.915824]  ? vfs_write+0xb0/0x190
> [  567.915834]  ? SyS_write+0x42/0x90
> [  567.915846]  ? entry_SYSCALL_64_fastpath+0x1e/0xad
> 
> 
> ... and at resume:
> 
> [  574.320255] BUG: scheduling while atomic: alsa-source-3/1729/0x00000003
> [  574.320435] CPU: 1 PID: 1729 Comm: alsa-source-3 Tainted: G        WC O    4.11.0-rc7-3.g64b92e2-default #1
> [  574.320440] Call Trace:
> [  574.320474]  dump_stack+0x5c/0x7a
> [  574.320484]  __schedule_bug+0x55/0x70
> [  574.320493]  __schedule+0x63c/0x8c0
> [  574.320503]  schedule+0x3d/0x90
> [  574.320509]  schedule_timeout+0x16b/0x320
> [  574.320517]  ? del_timer_sync+0x50/0x50
> [  574.320529]  ? sst_wait_timeout+0xa9/0x170 [snd_intel_sst_core]
> [  574.320535]  ? sst_wait_timeout+0xa9/0x170 [snd_intel_sst_core]
> [  574.320539]  ? remove_wait_queue+0x60/0x60
> [  574.320546]  ? sst_prepare_and_post_msg+0x275/0x960 [snd_intel_sst_core]
> [  574.320553]  ? sst_resume_stream+0x5b/0x100 [snd_intel_sst_core]
> [  574.320564]  ? sst_platform_pcm_trigger+0x6b/0x1b0 [snd_soc_sst_atom_hifi2_platform]
> [  574.320586]  ? soc_pcm_trigger+0xa0/0x120 [snd_soc_core]
> [  574.320603]  ? dpcm_fe_dai_do_trigger+0xc8/0x1f0 [snd_soc_core]
> [  574.320618]  ? snd_pcm_action_single+0x2a/0x70 [snd_pcm]
> [  574.320628]  ? snd_pcm_common_ioctl1+0x2e5/0xc60 [snd_pcm]
> [  574.320634]  ? do_signal+0x23/0x670
> [  574.320644]  ? snd_pcm_capture_ioctl1+0x117/0x280 [snd_pcm]
> [  574.320648]  ? ktime_get_ts64+0x4a/0xf0
> [  574.320659]  ? snd_pcm_capture_ioctl+0x2a/0x30 [snd_pcm]
> [  574.320663]  ? do_vfs_ioctl+0x8f/0x5d0
> [  574.320667]  ? __fget+0x70/0xc0
> [  574.320671]  ? SyS_ioctl+0x74/0x80
> [  574.320675]  ? entry_SYSCALL_64_fastpath+0x1e/0xad
> [  574.320799] show_signal_msg: 8 callbacks suppressed
> [  574.320805] alsa-source-3[1729]: segfault at 7f28e8052000 ip 00007f28f635eac4 sp 00007f28f0ad4be8 error 6
> [  574.320882] BUG: scheduling while atomic: alsa-source-3/1729/0x7fffffff
> [  574.321040] CPU: 1 PID: 1729 Comm: alsa-source-3 Tainted: G        WC O    4.11.0-rc7-3.g64b92e2-default #1
> [  574.321043] Call Trace:
> [  574.321068]  dump_stack+0x5c/0x7a
> [  574.321076]  __schedule_bug+0x55/0x70
> [  574.321083]  __schedule+0x63c/0x8c0
> [  574.321093]  ? wakeup_preempt_entity.isra.58+0x3c/0x50
> [  574.321097]  schedule+0x3d/0x90
> [  574.321104]  schedule_timeout+0x25a/0x320
> [  574.321109]  ? check_preempt_curr+0x79/0x90
> [  574.321113]  ? select_task_rq_rt+0x57/0xa0
> [  574.321117]  ? sched_clock+0x5/0x10
> [  574.321120]  ? sched_clock_cpu+0xc/0xc0
> [  574.321124]  ? wait_for_completion+0xe6/0x150
> [  574.321128]  ? wait_for_completion+0xe6/0x150
> [  574.321131]  ? wake_up_q+0x80/0x80
> [  574.321135]  ? do_coredump+0x3ab/0xf10
> [  574.321139]  ? __wake_up+0x34/0x50
> [  574.321144]  ? get_signal+0x33b/0x660
> [  574.321150]  ? do_signal+0x23/0x670
> [  574.321154]  ? __send_signal+0x213/0x4d0
> [  574.321160]  ? force_sig_info_fault+0x88/0xd0
> [  574.321166]  ? exit_to_usermode_loop+0x71/0xb0
> [  574.321169]  ? prepare_exit_to_usermode+0x2a/0x30
> [  574.321172]  ? retint_user+0x8/0x10
> 
> 
> There seem many beasts hidden in this jungle...
> 
> 
> Takashi

-- 
~Vinod

Re: PM issue with Intel SST Atom driver

[Takashi Iwai]

On Mon, 24 Apr 2017 11:12:14 +0200,
Vinod Koul wrote:
> 
> On Mon, Apr 24, 2017 at 11:00:45AM +0200, Takashi Iwai wrote:
>  
> > Removing ignore_suspend makes the PM succeeds.  But it hits some other
> > ugly kernel bugs.
> 
> Okay have you marked .nonatomic = true for the machine DAIs?

Ah that's it.  The patch below seems fixing the PM and the nonatomic
problems.  I'm not sure about the nonatomic flag for the compress
stream, though.

Also I fiddled only with FE.  Do we need the same flags for BE?  The
others don't look setting like that, so I left so.


thanks,

Takashi

---
 sound/soc/intel/boards/bytcr_rt5640.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--- a/sound/soc/intel/boards/bytcr_rt5640.c
+++ b/sound/soc/intel/boards/bytcr_rt5640.c
@@ -646,7 +646,7 @@ static struct snd_soc_dai_link byt_rt564
 		.codec_dai_name = "snd-soc-dummy-dai",
 		.codec_name = "snd-soc-dummy",
 		.platform_name = "sst-mfld-platform",
-		.ignore_suspend = 1,
+		.nonatomic = true,
 		.dynamic = 1,
 		.dpcm_playback = 1,
 		.dpcm_capture = 1,
@@ -659,7 +659,6 @@ static struct snd_soc_dai_link byt_rt564
 		.codec_dai_name = "snd-soc-dummy-dai",
 		.codec_name = "snd-soc-dummy",
 		.platform_name = "sst-mfld-platform",
-		.ignore_suspend = 1,
 		.nonatomic = true,
 		.dynamic = 1,
 		.dpcm_playback = 1,
@@ -672,6 +671,7 @@ static struct snd_soc_dai_link byt_rt564
 		.codec_dai_name = "snd-soc-dummy-dai",
 		.codec_name = "snd-soc-dummy",
 		.platform_name = "sst-mfld-platform",
+		.nonatomic = true,
 	},
 		/* back ends */
 	{

Re: PM issue with Intel SST Atom driver

[Vinod Koul]

On Mon, Apr 24, 2017 at 11:43:47AM +0200, Takashi Iwai wrote:
> On Mon, 24 Apr 2017 11:12:14 +0200,
> Vinod Koul wrote:
> > 
> > On Mon, Apr 24, 2017 at 11:00:45AM +0200, Takashi Iwai wrote:
> >  
> > > Removing ignore_suspend makes the PM succeeds.  But it hits some other
> > > ugly kernel bugs.
> > 
> > Okay have you marked .nonatomic = true for the machine DAIs?
> 
> Ah that's it.  The patch below seems fixing the PM and the nonatomic
> problems.  I'm not sure about the nonatomic flag for the compress
> stream, though.

Well we dont have upstream decoders so it wont be used in this case.

> Also I fiddled only with FE.  Do we need the same flags for BE?  The
> others don't look setting like that, so I left so.

I dont remember if BE needs or not FE should suffice.

> 
> 
> thanks,
> 
> Takashi
> 
> ---
>  sound/soc/intel/boards/bytcr_rt5640.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> --- a/sound/soc/intel/boards/bytcr_rt5640.c
> +++ b/sound/soc/intel/boards/bytcr_rt5640.c
> @@ -646,7 +646,7 @@ static struct snd_soc_dai_link byt_rt564
>  		.codec_dai_name = "snd-soc-dummy-dai",
>  		.codec_name = "snd-soc-dummy",
>  		.platform_name = "sst-mfld-platform",
> -		.ignore_suspend = 1,
> +		.nonatomic = true,
>  		.dynamic = 1,
>  		.dpcm_playback = 1,
>  		.dpcm_capture = 1,
> @@ -659,7 +659,6 @@ static struct snd_soc_dai_link byt_rt564
>  		.codec_dai_name = "snd-soc-dummy-dai",
>  		.codec_name = "snd-soc-dummy",
>  		.platform_name = "sst-mfld-platform",
> -		.ignore_suspend = 1,
>  		.nonatomic = true,
>  		.dynamic = 1,
>  		.dpcm_playback = 1,
> @@ -672,6 +671,7 @@ static struct snd_soc_dai_link byt_rt564
>  		.codec_dai_name = "snd-soc-dummy-dai",
>  		.codec_name = "snd-soc-dummy",
>  		.platform_name = "sst-mfld-platform",
> +		.nonatomic = true,
>  	},
>  		/* back ends */
>  	{

-- 
~Vinod

Re: PM issue with Intel SST Atom driver

[Takashi Iwai]

On Mon, 24 Apr 2017 11:52:44 +0200,
Vinod Koul wrote:
> 
> On Mon, Apr 24, 2017 at 11:43:47AM +0200, Takashi Iwai wrote:
> > On Mon, 24 Apr 2017 11:12:14 +0200,
> > Vinod Koul wrote:
> > > 
> > > On Mon, Apr 24, 2017 at 11:00:45AM +0200, Takashi Iwai wrote:
> > >  
> > > > Removing ignore_suspend makes the PM succeeds.  But it hits some other
> > > > ugly kernel bugs.
> > > 
> > > Okay have you marked .nonatomic = true for the machine DAIs?
> > 
> > Ah that's it.  The patch below seems fixing the PM and the nonatomic
> > problems.  I'm not sure about the nonatomic flag for the compress
> > stream, though.
> 
> Well we dont have upstream decoders so it wont be used in this case.
> 
> > Also I fiddled only with FE.  Do we need the same flags for BE?  The
> > others don't look setting like that, so I left so.
> 
> I dont remember if BE needs or not FE should suffice.

OK then I leave it as is.

When I submit the fix, I should put Cc to stable, and wonder which
version we assure the nonatomic ops in SST driver.  Did the code base
support nonatomic ops from the beginning?


thanks,

Takashi

> 
> > 
> > 
> > thanks,
> > 
> > Takashi
> > 
> > ---
> >  sound/soc/intel/boards/bytcr_rt5640.c |    4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > --- a/sound/soc/intel/boards/bytcr_rt5640.c
> > +++ b/sound/soc/intel/boards/bytcr_rt5640.c
> > @@ -646,7 +646,7 @@ static struct snd_soc_dai_link byt_rt564
> >  		.codec_dai_name = "snd-soc-dummy-dai",
> >  		.codec_name = "snd-soc-dummy",
> >  		.platform_name = "sst-mfld-platform",
> > -		.ignore_suspend = 1,
> > +		.nonatomic = true,
> >  		.dynamic = 1,
> >  		.dpcm_playback = 1,
> >  		.dpcm_capture = 1,
> > @@ -659,7 +659,6 @@ static struct snd_soc_dai_link byt_rt564
> >  		.codec_dai_name = "snd-soc-dummy-dai",
> >  		.codec_name = "snd-soc-dummy",
> >  		.platform_name = "sst-mfld-platform",
> > -		.ignore_suspend = 1,
> >  		.nonatomic = true,
> >  		.dynamic = 1,
> >  		.dpcm_playback = 1,
> > @@ -672,6 +671,7 @@ static struct snd_soc_dai_link byt_rt564
> >  		.codec_dai_name = "snd-soc-dummy-dai",
> >  		.codec_name = "snd-soc-dummy",
> >  		.platform_name = "sst-mfld-platform",
> > +		.nonatomic = true,
> >  	},
> >  		/* back ends */
> >  	{
> 
> -- 
> ~Vinod
> 

Re: PM issue with Intel SST Atom driver

[Vinod Koul]

On Mon, Apr 24, 2017 at 11:54:06AM +0200, Takashi Iwai wrote:
> On Mon, 24 Apr 2017 11:52:44 +0200,
> Vinod Koul wrote:
> > 
> > On Mon, Apr 24, 2017 at 11:43:47AM +0200, Takashi Iwai wrote:
> > > On Mon, 24 Apr 2017 11:12:14 +0200,
> > > Vinod Koul wrote:
> > > > 
> > > > On Mon, Apr 24, 2017 at 11:00:45AM +0200, Takashi Iwai wrote:
> > > >  
> > > > > Removing ignore_suspend makes the PM succeeds.  But it hits some other
> > > > > ugly kernel bugs.
> > > > 
> > > > Okay have you marked .nonatomic = true for the machine DAIs?
> > > 
> > > Ah that's it.  The patch below seems fixing the PM and the nonatomic
> > > problems.  I'm not sure about the nonatomic flag for the compress
> > > stream, though.
> > 
> > Well we dont have upstream decoders so it wont be used in this case.
> > 
> > > Also I fiddled only with FE.  Do we need the same flags for BE?  The
> > > others don't look setting like that, so I left so.
> > 
> > I dont remember if BE needs or not FE should suffice.
> 
> OK then I leave it as is.
> 
> When I submit the fix, I should put Cc to stable, and wonder which
> version we assure the nonatomic ops in SST driver.  Did the code base
> support nonatomic ops from the beginning?

4.1 onwards.

The PM was supported thru below commit which went into 4.1.  The nonatomic
was always a requirement for us due to nature of IPC. Non atomic support
went into 3.18 so 4.1 onwards would make sense :)

commit 4a8448d4289d7210053a43f9f21e42929beb159b
Author: Vinod Koul <vinod.koul@intel.com>
Date:   Tue Feb 24 11:39:44 2015 +0530

    ASoC: Intel: add pm support in sst ipc driver

    This adds support for system pm support. We need to save the dsp memory
    which gets lost on suspend and restore that on resume

    Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
    Signed-off-by: Vinod Koul <vinod.koul@intel.com>
    Signed-off-by: Mark Brown <broonie@kernel.org>

Thanks
-- 
~Vinod

Re: PM issue with Intel SST Atom driver

[Pierre-Louis Bossart]

On 4/24/17 4:54 AM, Takashi Iwai wrote:
> On Mon, 24 Apr 2017 11:52:44 +0200,
> Vinod Koul wrote:
>>
>> On Mon, Apr 24, 2017 at 11:43:47AM +0200, Takashi Iwai wrote:
>>> On Mon, 24 Apr 2017 11:12:14 +0200,
>>> Vinod Koul wrote:
>>>>
>>>> On Mon, Apr 24, 2017 at 11:00:45AM +0200, Takashi Iwai wrote:
>>>>
>>>>> Removing ignore_suspend makes the PM succeeds.  But it hits some other
>>>>> ugly kernel bugs.
>>>>
>>>> Okay have you marked .nonatomic = true for the machine DAIs?
>>>
>>> Ah that's it.  The patch below seems fixing the PM and the nonatomic
>>> problems.  I'm not sure about the nonatomic flag for the compress
>>> stream, though.
>>
>> Well we dont have upstream decoders so it wont be used in this case.
>>
>>> Also I fiddled only with FE.  Do we need the same flags for BE?  The
>>> others don't look setting like that, so I left so.
>>
>> I dont remember if BE needs or not FE should suffice.
>
> OK then I leave it as is.
>
> When I submit the fix, I should put Cc to stable, and wonder which
> version we assure the nonatomic ops in SST driver.  Did the code base
> support nonatomic ops from the beginning?

can we take this opportunity to align all drivers?
The .nonatomic=true is set in all drivers for the BE, except for 
cht_bsw_max98090_ti.c
It's either needed for all or not needed for all...

>
>
> thanks,
>
> Takashi
>
>>
>>>
>>>
>>> thanks,
>>>
>>> Takashi
>>>
>>> ---
>>>  sound/soc/intel/boards/bytcr_rt5640.c |    4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> --- a/sound/soc/intel/boards/bytcr_rt5640.c
>>> +++ b/sound/soc/intel/boards/bytcr_rt5640.c
>>> @@ -646,7 +646,7 @@ static struct snd_soc_dai_link byt_rt564
>>>  		.codec_dai_name = "snd-soc-dummy-dai",
>>>  		.codec_name = "snd-soc-dummy",
>>>  		.platform_name = "sst-mfld-platform",
>>> -		.ignore_suspend = 1,
>>> +		.nonatomic = true,
>>>  		.dynamic = 1,
>>>  		.dpcm_playback = 1,
>>>  		.dpcm_capture = 1,
>>> @@ -659,7 +659,6 @@ static struct snd_soc_dai_link byt_rt564
>>>  		.codec_dai_name = "snd-soc-dummy-dai",
>>>  		.codec_name = "snd-soc-dummy",
>>>  		.platform_name = "sst-mfld-platform",
>>> -		.ignore_suspend = 1,
>>>  		.nonatomic = true,
>>>  		.dynamic = 1,
>>>  		.dpcm_playback = 1,
>>> @@ -672,6 +671,7 @@ static struct snd_soc_dai_link byt_rt564
>>>  		.codec_dai_name = "snd-soc-dummy-dai",
>>>  		.codec_name = "snd-soc-dummy",
>>>  		.platform_name = "sst-mfld-platform",
>>> +		.nonatomic = true,
>>>  	},
>>>  		/* back ends */
>>>  	{
>>
>> --
>> ~Vinod
>>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>

Re: PM issue with Intel SST Atom driver

[Vinod Koul]

On Mon, Apr 24, 2017 at 09:22:38AM -0500, Pierre-Louis Bossart wrote:
> On 4/24/17 4:54 AM, Takashi Iwai wrote:
> >On Mon, 24 Apr 2017 11:52:44 +0200,
> >Vinod Koul wrote:
> >>
> >>On Mon, Apr 24, 2017 at 11:43:47AM +0200, Takashi Iwai wrote:
> >>>On Mon, 24 Apr 2017 11:12:14 +0200,
> >>>Vinod Koul wrote:
> >>>>
> >>>>On Mon, Apr 24, 2017 at 11:00:45AM +0200, Takashi Iwai wrote:
> >>>>
> >>>>>Removing ignore_suspend makes the PM succeeds.  But it hits some other
> >>>>>ugly kernel bugs.
> >>>>
> >>>>Okay have you marked .nonatomic = true for the machine DAIs?
> >>>
> >>>Ah that's it.  The patch below seems fixing the PM and the nonatomic
> >>>problems.  I'm not sure about the nonatomic flag for the compress
> >>>stream, though.
> >>
> >>Well we dont have upstream decoders so it wont be used in this case.
> >>
> >>>Also I fiddled only with FE.  Do we need the same flags for BE?  The
> >>>others don't look setting like that, so I left so.
> >>
> >>I dont remember if BE needs or not FE should suffice.
> >
> >OK then I leave it as is.
> >
> >When I submit the fix, I should put Cc to stable, and wonder which
> >version we assure the nonatomic ops in SST driver.  Did the code base
> >support nonatomic ops from the beginning?
> 
> can we take this opportunity to align all drivers?
> The .nonatomic=true is set in all drivers for the BE, except for
> cht_bsw_max98090_ti.c

??

$ grep nonatomic sound/soc/intel/boards/cht_bsw_max98090_ti.c
                .nonatomic = true,
                .nonatomic = true,

> It's either needed for all or not needed for all...

For the record it is must have for all of the drivers :)

$ grep -L nonatomic sound/soc/intel/boards/*.c
sound/soc/intel/boards/bdw-rt5677.c
sound/soc/intel/boards/broadwell.c
sound/soc/intel/boards/byt-max98090.c
sound/soc/intel/boards/byt-rt5640.c
sound/soc/intel/boards/haswell.c
sound/soc/intel/boards/mfld_machine.c

So we should add the remaining one byt-max98090.c as Takashi fixed
byt-rt5640.c one. I will send the patch for this one.

-- 
~Vinod

Re: PM issue with Intel SST Atom driver

[Takashi Iwai]

On Mon, 24 Apr 2017 18:27:38 +0200,
Vinod Koul wrote:
> 
> On Mon, Apr 24, 2017 at 09:22:38AM -0500, Pierre-Louis Bossart wrote:
> > On 4/24/17 4:54 AM, Takashi Iwai wrote:
> > >On Mon, 24 Apr 2017 11:52:44 +0200,
> > >Vinod Koul wrote:
> > >>
> > >>On Mon, Apr 24, 2017 at 11:43:47AM +0200, Takashi Iwai wrote:
> > >>>On Mon, 24 Apr 2017 11:12:14 +0200,
> > >>>Vinod Koul wrote:
> > >>>>
> > >>>>On Mon, Apr 24, 2017 at 11:00:45AM +0200, Takashi Iwai wrote:
> > >>>>
> > >>>>>Removing ignore_suspend makes the PM succeeds.  But it hits some other
> > >>>>>ugly kernel bugs.
> > >>>>
> > >>>>Okay have you marked .nonatomic = true for the machine DAIs?
> > >>>
> > >>>Ah that's it.  The patch below seems fixing the PM and the nonatomic
> > >>>problems.  I'm not sure about the nonatomic flag for the compress
> > >>>stream, though.
> > >>
> > >>Well we dont have upstream decoders so it wont be used in this case.
> > >>
> > >>>Also I fiddled only with FE.  Do we need the same flags for BE?  The
> > >>>others don't look setting like that, so I left so.
> > >>
> > >>I dont remember if BE needs or not FE should suffice.
> > >
> > >OK then I leave it as is.
> > >
> > >When I submit the fix, I should put Cc to stable, and wonder which
> > >version we assure the nonatomic ops in SST driver.  Did the code base
> > >support nonatomic ops from the beginning?
> > 
> > can we take this opportunity to align all drivers?
> > The .nonatomic=true is set in all drivers for the BE, except for
> > cht_bsw_max98090_ti.c
> 
> ??
> 
> $ grep nonatomic sound/soc/intel/boards/cht_bsw_max98090_ti.c
>                 .nonatomic = true,
>                 .nonatomic = true,
> 
> > It's either needed for all or not needed for all...
> 
> For the record it is must have for all of the drivers :)
> 
> $ grep -L nonatomic sound/soc/intel/boards/*.c
> sound/soc/intel/boards/bdw-rt5677.c
> sound/soc/intel/boards/broadwell.c
> sound/soc/intel/boards/byt-max98090.c
> sound/soc/intel/boards/byt-rt5640.c
> sound/soc/intel/boards/haswell.c
> sound/soc/intel/boards/mfld_machine.c
> 
> So we should add the remaining one byt-max98090.c as Takashi fixed
> byt-rt5640.c one. I will send the patch for this one.

Or maybe we should replace these definitions with some macro to expand
to the mostly same contents?  The difference is just a few callback
functions, basically.


Takashi

Re: PM issue with Intel SST Atom driver

[Pierre-Louis Bossart]

On 4/24/17 11:27 AM, Vinod Koul wrote:
> On Mon, Apr 24, 2017 at 09:22:38AM -0500, Pierre-Louis Bossart wrote:
>> On 4/24/17 4:54 AM, Takashi Iwai wrote:
>>> On Mon, 24 Apr 2017 11:52:44 +0200,
>>> Vinod Koul wrote:
>>>>
>>>> On Mon, Apr 24, 2017 at 11:43:47AM +0200, Takashi Iwai wrote:
>>>>> On Mon, 24 Apr 2017 11:12:14 +0200,
>>>>> Vinod Koul wrote:
>>>>>>
>>>>>> On Mon, Apr 24, 2017 at 11:00:45AM +0200, Takashi Iwai wrote:
>>>>>>
>>>>>>> Removing ignore_suspend makes the PM succeeds.  But it hits some other
>>>>>>> ugly kernel bugs.
>>>>>>
>>>>>> Okay have you marked .nonatomic = true for the machine DAIs?
>>>>>
>>>>> Ah that's it.  The patch below seems fixing the PM and the nonatomic
>>>>> problems.  I'm not sure about the nonatomic flag for the compress
>>>>> stream, though.
>>>>
>>>> Well we dont have upstream decoders so it wont be used in this case.
>>>>
>>>>> Also I fiddled only with FE.  Do we need the same flags for BE?  The
>>>>> others don't look setting like that, so I left so.
>>>>
>>>> I dont remember if BE needs or not FE should suffice.
>>>
>>> OK then I leave it as is.
>>>
>>> When I submit the fix, I should put Cc to stable, and wonder which
>>> version we assure the nonatomic ops in SST driver.  Did the code base
>>> support nonatomic ops from the beginning?
>>
>> can we take this opportunity to align all drivers?
>> The .nonatomic=true is set in all drivers for the BE, except for
>> cht_bsw_max98090_ti.c
>
> ??
>
> $ grep nonatomic sound/soc/intel/boards/cht_bsw_max98090_ti.c
>                 .nonatomic = true,
>                 .nonatomic = true,

Yes, the .nonatomic is set for the two PCM frontends but not for the 
SSP2 backend.

>
>> It's either needed for all or not needed for all...
>
> For the record it is must have for all of the drivers :)
>
> $ grep -L nonatomic sound/soc/intel/boards/*.c
> sound/soc/intel/boards/bdw-rt5677.c
> sound/soc/intel/boards/broadwell.c
> sound/soc/intel/boards/byt-max98090.c
> sound/soc/intel/boards/byt-rt5640.c
> sound/soc/intel/boards/haswell.c
> sound/soc/intel/boards/mfld_machine.c
>
> So we should add the remaining one byt-max98090.c as Takashi fixed
> byt-rt5640.c one. I will send the patch for this one.

Takashi fixed the bytcr-rt5640 driver, I am not sure we should touch the 
old byt- driver since they are not using the same firmware and they are 
not enabled by default. Same for broadwell, different driver, different 
problems.

I was also also talking about drivers that have .nonatomic field set but 
not everywhere consistently.

Re: PM issue with Intel SST Atom driver

[Vinod Koul]

On Mon, Apr 24, 2017 at 08:32:14PM +0200, Takashi Iwai wrote:
> On Mon, 24 Apr 2017 18:27:38 +0200,
> > 
> > So we should add the remaining one byt-max98090.c as Takashi fixed
> > byt-rt5640.c one. I will send the patch for this one.
> 
> Or maybe we should replace these definitions with some macro to expand
> to the mostly same contents?  The difference is just a few callback
> functions, basically.

And while at it, I cant help but wonder but if we can do better and mark it
in platform driver thus avoiding it replication in machines. Afterall the
atomic trigger is a platform property.

Thanks
-- 
~Vinod

Re: PM issue with Intel SST Atom driver

[Vinod Koul]

On Mon, Apr 24, 2017 at 02:01:44PM -0500, Pierre-Louis Bossart wrote:
> On 4/24/17 11:27 AM, Vinod Koul wrote:
> >On Mon, Apr 24, 2017 at 09:22:38AM -0500, Pierre-Louis Bossart wrote:
> >>On 4/24/17 4:54 AM, Takashi Iwai wrote:
> >>>On Mon, 24 Apr 2017 11:52:44 +0200,
> >>>Vinod Koul wrote:
> >>>>
> >>>>On Mon, Apr 24, 2017 at 11:43:47AM +0200, Takashi Iwai wrote:
> >>>>>On Mon, 24 Apr 2017 11:12:14 +0200,
> >>>>>Vinod Koul wrote:
> >>>>>>
> >>>>>>On Mon, Apr 24, 2017 at 11:00:45AM +0200, Takashi Iwai wrote:
> >>>>>>
> >>>>>>>Removing ignore_suspend makes the PM succeeds.  But it hits some other
> >>>>>>>ugly kernel bugs.
> >>>>>>
> >>>>>>Okay have you marked .nonatomic = true for the machine DAIs?
> >>>>>
> >>>>>Ah that's it.  The patch below seems fixing the PM and the nonatomic
> >>>>>problems.  I'm not sure about the nonatomic flag for the compress
> >>>>>stream, though.
> >>>>
> >>>>Well we dont have upstream decoders so it wont be used in this case.
> >>>>
> >>>>>Also I fiddled only with FE.  Do we need the same flags for BE?  The
> >>>>>others don't look setting like that, so I left so.
> >>>>
> >>>>I dont remember if BE needs or not FE should suffice.
> >>>
> >>>OK then I leave it as is.
> >>>
> >>>When I submit the fix, I should put Cc to stable, and wonder which
> >>>version we assure the nonatomic ops in SST driver.  Did the code base
> >>>support nonatomic ops from the beginning?
> >>
> >>can we take this opportunity to align all drivers?
> >>The .nonatomic=true is set in all drivers for the BE, except for
> >>cht_bsw_max98090_ti.c
> >
> >??
> >
> >$ grep nonatomic sound/soc/intel/boards/cht_bsw_max98090_ti.c
> >                .nonatomic = true,
> >                .nonatomic = true,
> 
> Yes, the .nonatomic is set for the two PCM frontends but not for the
> SSP2 backend.

Oh okay, sorry I misunderstood that, I dont think we need this for BE.

> 
> >
> >>It's either needed for all or not needed for all...
> >
> >For the record it is must have for all of the drivers :)
> >
> >$ grep -L nonatomic sound/soc/intel/boards/*.c
> >sound/soc/intel/boards/bdw-rt5677.c
> >sound/soc/intel/boards/broadwell.c
> >sound/soc/intel/boards/byt-max98090.c
> >sound/soc/intel/boards/byt-rt5640.c
> >sound/soc/intel/boards/haswell.c
> >sound/soc/intel/boards/mfld_machine.c
> >
> >So we should add the remaining one byt-max98090.c as Takashi fixed
> >byt-rt5640.c one. I will send the patch for this one.
> 
> Takashi fixed the bytcr-rt5640 driver, I am not sure we should touch
> the old byt- driver since they are not using the same firmware and
> they are not enabled by default. Same for broadwell, different
> driver, different problems.

on that not we should rename file to depict which driver they use older or
newer :)


> I was also also talking about drivers that have .nonatomic field set
> but not everywhere consistently.


-- 
~Vinod