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.
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
