On Thu, Jan 10, 2019 at 02:11:32PM -0600, Pierre-Louis Bossart wrote:
- /* attach any data */
- if (msg_bytes)
memcpy(msg->msg_data, msg_data, msg_bytes);
How big do these messages get? Do we need to hold the lock while we memcpy()?
Messages can be as big as the mailbox, which is hardware dependent. It could be from a few bytes to a larger e.g. 4k page or more, and indeed we need to keep the lock.
Is this copying into an actual mailbox or into some data structure in memory? It looked like it's copying into a buffer for sending rather than the mailbox.
- /* schedule the message if not busy */
- if (snd_sof_dsp_is_ready(sdev))
schedule_work(&ipc->tx_kwork);
If the DSP is idle is there a reason this has to happen in another thread?
we will rename this as snd_sof_dsp_is_ipc_ready() to avoid any confusion with the DSP state. We only care about IPC registers/doorbells at this point, not the fact that the DSP is in its idle loop.
You're missing the point - why can't we just immediately send the message to the DSP from here, what's the benefit of scheduling some work to do that?
- spin_unlock_irqrestore(&sdev->ipc_lock, flags);
The thread is also going to take an irq spinlock after all.
didn't get this point, sorry.
One reason to bounce to another thread would be to do something that you can't do in this context like take a lock in a place you can't take locks but here we're taking a lock that needs thread context already.