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

[Keyon Jie]

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
> 
> 1. 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.

> 
> 2. what happens if it is?
> 
> 3. 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 at alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
>