-
Notifications
You must be signed in to change notification settings - Fork 131
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ASoC: SOF: intel: set stream decouple false in stream_free #361
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not completely clear if this is done by symmetry with the inits+hw_params or if this is truly required in the hardware documentation?
sound/soc/sof/intel/hda-stream.c
Outdated
@@ -659,13 +659,15 @@ void hda_dsp_stream_free(struct snd_sof_dev *sdev) | |||
#endif | |||
|
|||
list_for_each_entry_safe(s, _s, &bus->stream_list, list) { | |||
stream = stream_to_hdac_ext_stream(s); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should stream be defined conditionally? We don't want to use hdac_ext_stream in all cases
move snd_hdac_ext_stream_decouple(bus, stream, false) to hda_link_hw_free() as snd_hdac_ext_stream_decouple(bus,stream, true) is called by hda_link_hw_params(). Signed-off-by: Bard liao <[email protected]>
@plbossart decouple is required from hardware. All streams will be configured as decoupled in init, that's done by hdac_ext_link_stream_assign(). I don't feel comfortable with current code. It is set in hw_params and pcm_trigger which is not symmetry. How about we move snd_hdac_ext_stream_decouple(false) to hw_free? So it will be configured to couple when the stream is not active and we don't need to configure it when the device is removed. |
d28113f
to
5768813
Compare
@bardliao can you remind me how it's done in the existing SST driver? |
@plbossart Below is what I found, but I don't know if there is anything missed since I didn't test sst driver. snd_hdac_ext_stream_decouple(true):
snd_hdac_ext_stream_decouple(false):
|
@ranj063 we could use your help here.
@bardliao showed we currently set it to
I agree his patch seems more logical to use hw_params/free but I am concerned about the suspend part. What is the flow on resume, would .prepare or .hw_params be reissued? |
@plbossart let me look into the code and get back on this. |
@plbossart yes, hw_params will definitely be called upon resume. I am not sure about prepare as we dont have that callback set in our hda dai link ops. But this change looks good to me. Also, the suspend trigger was something I never saw only the stop trigger before suspend. |
@ranj063 you probably only saw the stop trigger because that's how CRAS operates (streams stopped). We'd still figure out if we can suspend a running stream, that's a valid ALSA use case. |
When sock_alloc_file fails to allocate a file, it will call sock_release. __sys_socket_file should then not call sock_release again, otherwise there will be a double free. [ 89.319884] ------------[ cut here ]------------ [ 89.320286] kernel BUG at fs/inode.c:1764! [ 89.320656] invalid opcode: 0000 [thesofproject#1] PREEMPT SMP NOPTI [ 89.321051] CPU: 7 PID: 125 Comm: iou-sqp-124 Not tainted 6.2.0+ thesofproject#361 [ 89.321535] RIP: 0010:iput+0x1ff/0x240 [ 89.321808] Code: d1 83 e1 03 48 83 f9 02 75 09 48 81 fa 00 10 00 00 77 05 83 e2 01 75 1f 4c 89 ef e8 fb d2 ba 00 e9 80 fe ff ff c3 cc cc cc cc <0f> 0b 0f 0b e9 d0 fe ff ff 0f 0b eb 8d 49 8d b4 24 08 01 00 00 48 [ 89.322760] RSP: 0018:ffffbdd60068bd50 EFLAGS: 00010202 [ 89.323036] RAX: 0000000000000000 RBX: ffff9d7ad3cacac0 RCX: 0000000000001107 [ 89.323412] RDX: 000000000003af00 RSI: 0000000000000000 RDI: ffff9d7ad3cacb40 [ 89.323785] RBP: ffffbdd60068bd68 R08: ffffffffffffffff R09: ffffffffab606438 [ 89.324157] R10: ffffffffacb3dfa0 R11: 6465686361657256 R12: ffff9d7ad3cacb40 [ 89.324529] R13: 0000000080000001 R14: 0000000080000001 R15: 0000000000000002 [ 89.324904] FS: 00007f7b28516740(0000) GS:ffff9d7aeb1c0000(0000) knlGS:0000000000000000 [ 89.325328] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 89.325629] CR2: 00007f0af52e96c0 CR3: 0000000002a02006 CR4: 0000000000770ee0 [ 89.326004] PKRU: 55555554 [ 89.326161] Call Trace: [ 89.326298] <TASK> [ 89.326419] __sock_release+0xb5/0xc0 [ 89.326632] __sys_socket_file+0xb2/0xd0 [ 89.326844] io_socket+0x88/0x100 [ 89.327039] ? io_issue_sqe+0x6a/0x430 [ 89.327258] io_issue_sqe+0x67/0x430 [ 89.327450] io_submit_sqes+0x1fe/0x670 [ 89.327661] io_sq_thread+0x2e6/0x530 [ 89.327859] ? __pfx_autoremove_wake_function+0x10/0x10 [ 89.328145] ? __pfx_io_sq_thread+0x10/0x10 [ 89.328367] ret_from_fork+0x29/0x50 [ 89.328576] RIP: 0033:0x0 [ 89.328732] Code: Unable to access opcode bytes at 0xffffffffffffffd6. [ 89.329073] RSP: 002b:0000000000000000 EFLAGS: 00000202 ORIG_RAX: 00000000000001a9 [ 89.329477] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 00007f7b28637a3d [ 89.329845] RDX: 00007fff4e4318a8 RSI: 00007fff4e4318b0 RDI: 0000000000000400 [ 89.330216] RBP: 00007fff4e431830 R08: 00007fff4e431711 R09: 00007fff4e4318b0 [ 89.330584] R10: 0000000000000000 R11: 0000000000000202 R12: 00007fff4e441b38 [ 89.330950] R13: 0000563835e3e725 R14: 0000563835e40d10 R15: 00007f7b28784040 [ 89.331318] </TASK> [ 89.331441] Modules linked in: [ 89.331617] ---[ end trace 0000000000000000 ]--- Fixes: da214a4 ("net: add __sys_socket_file()") Signed-off-by: Thadeu Lima de Souza Cascardo <[email protected]> Reviewed-by: Jens Axboe <[email protected]> Reviewed-by: Eric Dumazet <[email protected]> Reviewed-by: Kuniyuki Iwashima <[email protected]> Link: https://lore.kernel.org/r/[email protected] Signed-off-by: Jakub Kicinski <[email protected]>
snd_hdac_ext_stream_decouple(bus, stream, true) will be called
on boot up and hw_params. So add snd_hdac_ext_stream_decouple(bus,
stream, false) in hda_dsp_stream_free().
Signed-off-by: Bard liao [email protected]
Fix #299