Re: [Sound-open-firmware] [alsa-devel] [PATCH v3 04/14] ASoC: SOF: Add support for IPC IO between DSP and Host
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?
On 2019/1/23 上午5:05, Pierre-Louis Bossart wrote:
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)
Hi Mark, I think you are right, we can actually call the message sending immediately here as we already hold the lock which can avoid race condition.
We scheduled another thread to do the sending task, aimed to make sure those messages(required to be sent from different thread) can be sent in sequence one by one(with the FIFO list), but that doesn't means we have to do it in another thread, let's change it.
+ 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
- why do we schedule a thread when the DSP is not busy
As explain above, I think it's not necessary, let's change it.
what happens if it is?
can we avoid freeing the lock to take it again when we schedule?
Then those #2 #3 issues not existed?
Thanks, ~Keyon
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
participants (2)
-
Keyon Jie -
Pierre-Louis Bossart