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