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

[Pierre-Louis Bossart]

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?