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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

[Index of Archives]     [ALSA User]     [Linux Audio Users]     [Pulse Audio]     [Kernel Archive]     [Asterisk PBX]     [Photo Sharing]     [Linux Sound]     [Video 4 Linux]     [Gimp]     [Yosemite News]

  Powered by Linux