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

[Keyon Jie]

On 2018/12/13 下午1:24, Keyon Jie wrote:
> 
> 
> 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).

Sorry, as #2 is a schedule work where should not sleep, so it is better 
to use spin_lock/unlock_irq() here.

Thanks,
~Keyon

> 
> 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
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel at alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel