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

[Mark Brown]

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20190122/53bdcd5d/attachment.sig>