Re: [PATCH v3 04/14] ASoC: SOF: Add support for IPC IO between DSP and Host

Mark Brown
2019-01-22T19:04:25Z

[-- Attachment #1.1: Type: text/plain, Size: 1535 bytes --]

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 #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]