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

[Keyon Jie]

On 2018/12/12 下午11:19, Pierre-Louis Bossart wrote:
> 
>>> diff --git a/include/sound/sof/control.h b/include/sound/sof/control.h
>>>
>>> +/* generic channel mapped value data */
>>> +struct sof_ipc_ctrl_value_chan {
>>> +    uint32_t channel;    /**< channel map - enum sof_ipc_chmap */
>>> +    uint32_t value;
>> Any reason to avoid s32 and u32?
>> If this is supposed to be shared with user-space (or user-space may
>> use this as a reference of data struct), we should consider placing in
>> uapi directory, too.
> 
> it's intentional
> 
> The includes shared with userspace are in include/uapi/sound/sof.
> 
> All the files in include/sound/sof, and this one in particular, are more 
> for host-dsp IPC.
> 
> In those two cases, uapi and IPC files, we don't use s32 and u32. we 
> could move this directory under include/uapi/sound/sof-ipc if you prefer?
> 
>>
>>
>>> +/* wait for IPC message reply */
>>> +static int tx_wait_done(struct snd_sof_ipc *ipc, struct 
>>> snd_sof_ipc_msg *msg,
>>> +            void *reply_data)
>>> +{
>>> +    struct snd_sof_dev *sdev = ipc->sdev;
>>> +    struct sof_ipc_cmd_hdr *hdr = (struct sof_ipc_cmd_hdr 
>>> *)msg->msg_data;
>>> +    unsigned long flags;
>>> +    int ret;
>>> +
>>> +    /* wait for DSP IPC completion */
>>> +    ret = wait_event_timeout(msg->waitq, msg->ipc_complete,
>>> +                 msecs_to_jiffies(IPC_TIMEOUT_MSECS));
>>> +
>>> +    spin_lock_irqsave(&sdev->ipc_lock, flags);
>> Since this must be a sleepable context, you can safely use
>> spin_lock_irq() here.
>>
>>> +/* send IPC message from host to DSP */
>>> +int sof_ipc_tx_message(struct snd_sof_ipc *ipc, u32 header,
>>> +               void *msg_data, size_t msg_bytes, void *reply_data,
>>> +               size_t reply_bytes)
>>> +{
>>> +    struct snd_sof_dev *sdev = ipc->sdev;
>>> +    struct snd_sof_ipc_msg *msg;
>>> +    unsigned long flags;
>>> +
>>> +    spin_lock_irqsave(&sdev->ipc_lock, flags);
>> Ditto.  This one calls tx_wait_done() later.
>>
>> It's better to define more strictly which one can be called from the
>> spinlocked context and which not.
> 
> This one is for Keyon and team. I've asked that question multiple times 
> and was told the irqsave was needed. Keyon, can you please chime in?

we basically have 3 parts where using this ipc_lock:

1. sof_ipc_tx_message(), get lock, update tx_list, schedule tx_work, put 
lock, then call tx_wait_done();
2. ipc_tx_next_msg() (tx_work itself), get lock, send message, put lock;
2.5. ack/reply ipc interrupt arrived, mark ipc_complete in handler.
3. tx_wait_done(), wait until ipc_complete(or timeout), then get lock, 
handle the ack/reply, and put lock at last.

|1 -[--]-|-> 3------(done)-[--]-|
       |             ^
       V             |
       |2-[--]-|     |
               |2.5--|

those []s means holding locks.

So, all those 3 functions can't be called from the spin-locked context 
as they need to hold the lock inside them.

I admit that we are too conservative that using 
spin_lock_irqsave/restore() here, as Takashi mentioned, here all 3 
functions are actually run in normal thread context, I think we can even 
run them with interrupt enabled(using spin_lock/unlock() directly).

Thanks,
~Keyon

> 
>>
>>
>>> +void snd_sof_ipc_free(struct snd_sof_dev *sdev)
>>> +{
>>> +    cancel_work_sync(&sdev->ipc->tx_kwork);
>>> +    cancel_work_sync(&sdev->ipc->rx_kwork);
>>> +}
>>> +EXPORT_SYMBOL(snd_sof_ipc_free);
>> Not specific to this function but a general question:
>> why not EXPORT_SYMBOL_GPL() in general in the whole SOF codes?
> 
> We use a dual license (copied below)
> 
> // SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
> //
> // This file is provided under a dual BSD/GPLv2 license.  When using or
> // redistributing this file, you may do so under either license.
> 
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel at alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel