On 1/22/19 1:04 PM, Mark Brown wrote:
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.
I realize between your feedback and Daniel's that we have a terminology
issue. On the Intel side we don't have a dedicated hardware mailbox
unit, it's only doorbell registers and shared memory windows.
+ /* 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?
I realize this might be Intel-specific and will likely have to evolve.
Keyon/Liam, this is your code, can you comment on Mark's comments (here)
+ 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.
Liam/Keyon comment needed here as well.
I think there are multiple questions that should be better answered to
1. why do we schedule a thread when the DSP is not busy
2. what happens if it is?
3. can we avoid freeing the lock to take it again when we schedule?
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
