[alsa-devel] [PATCH 0/5] ASoC: Intel: IPC framework updates
Existing IPC framework omits crucial part of the entire communication: reply header. Some IPCs cannot function at all without the access to said header. Update the sst-ipc with new sst_ipc_message structure to represent both request and reply allowing reply-processing handlers to save received responses.
Despite the range of changes required for model to be updated, no functional changes are made for core hanswell, baytrail and skylake message handlers. Reply-processing handlers now save received response header yet no usage is added by default.
To allow for future changes, righful kings of IPC kingdom need to be put back on the throne. This update addresses one of them: LARGE_CONFIG_GET.
Cezary Rojewski (5): ASoC: Intel: Update request-reply IPC model ASoC: Intel: Haswell: Align with updated request-reply model ASoC: Intel: Baytrail: Align with updated request-reply model ASoC: Intel: Skylake: Align with updated request-reply model ASoC: Intel: Skylake: large_config_get overhaul
sound/soc/intel/baytrail/sst-baytrail-ipc.c | 65 ++++---- sound/soc/intel/common/sst-ipc.c | 68 ++++---- sound/soc/intel/common/sst-ipc.h | 27 ++-- sound/soc/intel/haswell/sst-haswell-ipc.c | 164 +++++++++++--------- sound/soc/intel/skylake/cnl-sst.c | 6 +- sound/soc/intel/skylake/skl-messages.c | 3 +- sound/soc/intel/skylake/skl-sst-ipc.c | 152 ++++++++++-------- sound/soc/intel/skylake/skl-sst-ipc.h | 3 +- 8 files changed, 259 insertions(+), 229 deletions(-)
struct ipc_message contains fields: header, tx_data and tx_size which represent TX i.e. request while RX is represented by rx_data and rx_size with reply's header equivalent missing.
Reply header may contain some vital information including, but not limited to, received payload size. Some IPCs have entire payload found within RX header instead. Content and value of said header is context dependent and may vary between firmware versions and target platform. Current model does not allow such IPCs to function at all.
Rather than appending yet another parameter to an already long list of such for sst_ipc_tx_message_XXXs, declare message container in form of struct sst_ipc_message and add them to parent's ipc_message declaration.
Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- sound/soc/intel/common/sst-ipc.c | 69 ++++++++++++++++---------------- sound/soc/intel/common/sst-ipc.h | 27 +++++++------ 2 files changed, 49 insertions(+), 47 deletions(-)
diff --git a/sound/soc/intel/common/sst-ipc.c b/sound/soc/intel/common/sst-ipc.c index ef5b66af1cd2..1186a03a88d6 100644 --- a/sound/soc/intel/common/sst-ipc.c +++ b/sound/soc/intel/common/sst-ipc.c @@ -43,7 +43,7 @@ static struct ipc_message *msg_get_empty(struct sst_generic_ipc *ipc) }
static int tx_wait_done(struct sst_generic_ipc *ipc, - struct ipc_message *msg, void *rx_data) + struct ipc_message *msg, struct sst_ipc_message *reply) { unsigned long flags; int ret; @@ -62,8 +62,11 @@ static int tx_wait_done(struct sst_generic_ipc *ipc, } else {
/* copy the data returned from DSP */ - if (rx_data) - memcpy(rx_data, msg->rx_data, msg->rx_size); + if (reply) { + reply->header = msg->rx.header; + if (reply->data) + memcpy(reply->data, msg->rx.data, msg->rx.size); + } ret = msg->errno; }
@@ -72,9 +75,9 @@ static int tx_wait_done(struct sst_generic_ipc *ipc, return ret; }
-static int ipc_tx_message(struct sst_generic_ipc *ipc, u64 header, - void *tx_data, size_t tx_bytes, void *rx_data, - size_t rx_bytes, int wait) +static int ipc_tx_message(struct sst_generic_ipc *ipc, + struct sst_ipc_message request, + struct sst_ipc_message *reply, int wait) { struct ipc_message *msg; unsigned long flags; @@ -87,23 +90,24 @@ static int ipc_tx_message(struct sst_generic_ipc *ipc, u64 header, return -EBUSY; }
- msg->header = header; - msg->tx_size = tx_bytes; - msg->rx_size = rx_bytes; + msg->tx.header = request.header; + msg->tx.size = request.size; + msg->rx.header = 0; + msg->rx.size = reply ? reply->size : 0; msg->wait = wait; msg->errno = 0; msg->pending = false; msg->complete = false;
- if ((tx_bytes) && (ipc->ops.tx_data_copy != NULL)) - ipc->ops.tx_data_copy(msg, tx_data, tx_bytes); + if ((request.size) && (ipc->ops.tx_data_copy != NULL)) + ipc->ops.tx_data_copy(msg, request.data, request.size);
list_add_tail(&msg->list, &ipc->tx_list); schedule_work(&ipc->kwork); spin_unlock_irqrestore(&ipc->dsp->spinlock, flags);
if (wait) - return tx_wait_done(ipc, msg, rx_data); + return tx_wait_done(ipc, msg, reply); else return 0; } @@ -118,13 +122,13 @@ static int msg_empty_list_init(struct sst_generic_ipc *ipc) return -ENOMEM;
for (i = 0; i < IPC_EMPTY_LIST_SIZE; i++) { - ipc->msg[i].tx_data = kzalloc(ipc->tx_data_max_size, GFP_KERNEL); - if (ipc->msg[i].tx_data == NULL) + ipc->msg[i].tx.data = kzalloc(ipc->tx_data_max_size, GFP_KERNEL); + if (ipc->msg[i].tx.data == NULL) goto free_mem;
- ipc->msg[i].rx_data = kzalloc(ipc->rx_data_max_size, GFP_KERNEL); - if (ipc->msg[i].rx_data == NULL) { - kfree(ipc->msg[i].tx_data); + ipc->msg[i].rx.data = kzalloc(ipc->rx_data_max_size, GFP_KERNEL); + if (ipc->msg[i].rx.data == NULL) { + kfree(ipc->msg[i].tx.data); goto free_mem; }
@@ -136,8 +140,8 @@ static int msg_empty_list_init(struct sst_generic_ipc *ipc)
free_mem: while (i > 0) { - kfree(ipc->msg[i-1].tx_data); - kfree(ipc->msg[i-1].rx_data); + kfree(ipc->msg[i-1].tx.data); + kfree(ipc->msg[i-1].rx.data); --i; } kfree(ipc->msg); @@ -173,8 +177,8 @@ static void ipc_tx_msgs(struct work_struct *work) spin_unlock_irq(&ipc->dsp->spinlock); }
-int sst_ipc_tx_message_wait(struct sst_generic_ipc *ipc, u64 header, - void *tx_data, size_t tx_bytes, void *rx_data, size_t rx_bytes) +int sst_ipc_tx_message_wait(struct sst_generic_ipc *ipc, + struct sst_ipc_message request, struct sst_ipc_message *reply) { int ret;
@@ -187,8 +191,7 @@ int sst_ipc_tx_message_wait(struct sst_generic_ipc *ipc, u64 header, if (ipc->ops.check_dsp_lp_on(ipc->dsp, true)) return -EIO;
- ret = ipc_tx_message(ipc, header, tx_data, tx_bytes, - rx_data, rx_bytes, 1); + ret = ipc_tx_message(ipc, request, reply, 1);
if (ipc->ops.check_dsp_lp_on) if (ipc->ops.check_dsp_lp_on(ipc->dsp, false)) @@ -198,19 +201,17 @@ int sst_ipc_tx_message_wait(struct sst_generic_ipc *ipc, u64 header, } EXPORT_SYMBOL_GPL(sst_ipc_tx_message_wait);
-int sst_ipc_tx_message_nowait(struct sst_generic_ipc *ipc, u64 header, - void *tx_data, size_t tx_bytes) +int sst_ipc_tx_message_nowait(struct sst_generic_ipc *ipc, + struct sst_ipc_message request) { - return ipc_tx_message(ipc, header, tx_data, tx_bytes, - NULL, 0, 0); + return ipc_tx_message(ipc, request, NULL, 0); } EXPORT_SYMBOL_GPL(sst_ipc_tx_message_nowait);
-int sst_ipc_tx_message_nopm(struct sst_generic_ipc *ipc, u64 header, - void *tx_data, size_t tx_bytes, void *rx_data, size_t rx_bytes) +int sst_ipc_tx_message_nopm(struct sst_generic_ipc *ipc, + struct sst_ipc_message request, struct sst_ipc_message *reply) { - return ipc_tx_message(ipc, header, tx_data, tx_bytes, - rx_data, rx_bytes, 1); + return ipc_tx_message(ipc, request, reply, 1); } EXPORT_SYMBOL_GPL(sst_ipc_tx_message_nopm);
@@ -230,7 +231,7 @@ struct ipc_message *sst_ipc_reply_find_msg(struct sst_generic_ipc *ipc, }
list_for_each_entry(msg, &ipc->rx_list, list) { - if ((msg->header & mask) == header) + if ((msg->tx.header & mask) == header) return msg; }
@@ -304,8 +305,8 @@ void sst_ipc_fini(struct sst_generic_ipc *ipc)
if (ipc->msg) { for (i = 0; i < IPC_EMPTY_LIST_SIZE; i++) { - kfree(ipc->msg[i].tx_data); - kfree(ipc->msg[i].rx_data); + kfree(ipc->msg[i].tx.data); + kfree(ipc->msg[i].rx.data); } kfree(ipc->msg); } diff --git a/sound/soc/intel/common/sst-ipc.h b/sound/soc/intel/common/sst-ipc.h index ef38600e88f7..08c4831b2664 100644 --- a/sound/soc/intel/common/sst-ipc.h +++ b/sound/soc/intel/common/sst-ipc.h @@ -17,15 +17,16 @@
#define IPC_MAX_MAILBOX_BYTES 256
-struct ipc_message { - struct list_head list; +struct sst_ipc_message { u64 header; + void *data; + size_t size; +};
- /* direction wrt host CPU */ - char *tx_data; - size_t tx_size; - char *rx_data; - size_t rx_size; +struct ipc_message { + struct list_head list; + struct sst_ipc_message tx; + struct sst_ipc_message rx;
wait_queue_head_t waitq; bool pending; @@ -66,14 +67,14 @@ struct sst_generic_ipc { struct sst_plat_ipc_ops ops; };
-int sst_ipc_tx_message_wait(struct sst_generic_ipc *ipc, u64 header, - void *tx_data, size_t tx_bytes, void *rx_data, size_t rx_bytes); +int sst_ipc_tx_message_wait(struct sst_generic_ipc *ipc, + struct sst_ipc_message request, struct sst_ipc_message *reply);
-int sst_ipc_tx_message_nowait(struct sst_generic_ipc *ipc, u64 header, - void *tx_data, size_t tx_bytes); +int sst_ipc_tx_message_nowait(struct sst_generic_ipc *ipc, + struct sst_ipc_message request);
-int sst_ipc_tx_message_nopm(struct sst_generic_ipc *ipc, u64 header, - void *tx_data, size_t tx_bytes, void *rx_data, size_t rx_bytes); +int sst_ipc_tx_message_nopm(struct sst_generic_ipc *ipc, + struct sst_ipc_message request, struct sst_ipc_message *reply);
struct ipc_message *sst_ipc_reply_find_msg(struct sst_generic_ipc *ipc, u64 header);
There is struct sst_ipc_message instance representing request and reply each, and params: header, data and size need no longer to be declared explicitly. Align with that model.
Update reply processing function to save RX header within message container.
Despite the range of changes, status quo is achieved.
Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- sound/soc/intel/haswell/sst-haswell-ipc.c | 164 ++++++++++++---------- 1 file changed, 88 insertions(+), 76 deletions(-)
diff --git a/sound/soc/intel/haswell/sst-haswell-ipc.c b/sound/soc/intel/haswell/sst-haswell-ipc.c index a83b92d6bea8..5c73b11375e3 100644 --- a/sound/soc/intel/haswell/sst-haswell-ipc.c +++ b/sound/soc/intel/haswell/sst-haswell-ipc.c @@ -511,7 +511,7 @@ static void hsw_notification_work(struct work_struct *work) static void hsw_stream_update(struct sst_hsw *hsw, struct ipc_message *msg) { struct sst_hsw_stream *stream; - u32 header = msg->header & ~(IPC_STATUS_MASK | IPC_GLB_REPLY_MASK); + u32 header = msg->tx.header & ~(IPC_STATUS_MASK | IPC_GLB_REPLY_MASK); u32 stream_id = msg_get_stream_id(header); u32 stream_msg = msg_get_stream_type(header);
@@ -552,6 +552,7 @@ static int hsw_process_reply(struct sst_hsw *hsw, u32 header) return -EIO; }
+ msg->rx.header = header; /* first process the header */ switch (reply) { case IPC_GLB_REPLY_PENDING: @@ -562,13 +563,13 @@ static int hsw_process_reply(struct sst_hsw *hsw, u32 header) case IPC_GLB_REPLY_SUCCESS: if (msg->pending) { trace_ipc_pending_reply("completed", header); - sst_dsp_inbox_read(hsw->dsp, msg->rx_data, - msg->rx_size); + sst_dsp_inbox_read(hsw->dsp, msg->rx.data, + msg->rx.size); hsw->ipc.pending = false; } else { /* copy data from the DSP */ - sst_dsp_outbox_read(hsw->dsp, msg->rx_data, - msg->rx_size); + sst_dsp_outbox_read(hsw->dsp, msg->rx.data, + msg->rx.size); } break; /* these will be rare - but useful for debug */ @@ -810,11 +811,13 @@ static irqreturn_t hsw_irq_thread(int irq, void *context) int sst_hsw_fw_get_version(struct sst_hsw *hsw, struct sst_hsw_ipc_fw_version *version) { + struct sst_ipc_message request = {0}, reply = {0}; int ret;
- ret = sst_ipc_tx_message_wait(&hsw->ipc, - IPC_GLB_TYPE(IPC_GLB_GET_FW_VERSION), - NULL, 0, version, sizeof(*version)); + request.header = IPC_GLB_TYPE(IPC_GLB_GET_FW_VERSION); + reply.data = version; + reply.size = sizeof(*version); + ret = sst_ipc_tx_message_wait(&hsw->ipc, request, &reply); if (ret < 0) dev_err(hsw->dev, "error: get version failed\n");
@@ -840,7 +843,7 @@ int sst_hsw_stream_set_volume(struct sst_hsw *hsw, struct sst_hsw_stream *stream, u32 stage_id, u32 channel, u32 volume) { struct sst_hsw_ipc_volume_req *req; - u32 header; + struct sst_ipc_message request; int ret;
trace_ipc_request("set stream volume", stream->reply.stream_hw_id); @@ -848,11 +851,11 @@ int sst_hsw_stream_set_volume(struct sst_hsw *hsw, if (channel >= 2 && channel != SST_HSW_CHANNELS_ALL) return -EINVAL;
- header = IPC_GLB_TYPE(IPC_GLB_STREAM_MESSAGE) | + request.header = IPC_GLB_TYPE(IPC_GLB_STREAM_MESSAGE) | IPC_STR_TYPE(IPC_STR_STAGE_MESSAGE); - header |= (stream->reply.stream_hw_id << IPC_STR_ID_SHIFT); - header |= (IPC_STG_SET_VOLUME << IPC_STG_TYPE_SHIFT); - header |= (stage_id << IPC_STG_ID_SHIFT); + request.header |= (stream->reply.stream_hw_id << IPC_STR_ID_SHIFT); + request.header |= (IPC_STG_SET_VOLUME << IPC_STG_TYPE_SHIFT); + request.header |= (stage_id << IPC_STG_ID_SHIFT);
req = &stream->vol_req; req->target_volume = volume; @@ -877,8 +880,9 @@ int sst_hsw_stream_set_volume(struct sst_hsw *hsw, req->channel = channel; }
- ret = sst_ipc_tx_message_wait(&hsw->ipc, header, req, - sizeof(*req), NULL, 0); + request.data = req; + request.size = sizeof(*req); + ret = sst_ipc_tx_message_wait(&hsw->ipc, request, NULL); if (ret < 0) { dev_err(hsw->dev, "error: set stream volume failed\n"); return ret; @@ -905,7 +909,7 @@ int sst_hsw_mixer_set_volume(struct sst_hsw *hsw, u32 stage_id, u32 channel, u32 volume) { struct sst_hsw_ipc_volume_req req; - u32 header; + struct sst_ipc_message request; int ret;
trace_ipc_request("set mixer volume", volume); @@ -933,18 +937,19 @@ int sst_hsw_mixer_set_volume(struct sst_hsw *hsw, u32 stage_id, u32 channel, req.channel = channel; }
- header = IPC_GLB_TYPE(IPC_GLB_STREAM_MESSAGE) | + request.header = IPC_GLB_TYPE(IPC_GLB_STREAM_MESSAGE) | IPC_STR_TYPE(IPC_STR_STAGE_MESSAGE); - header |= (hsw->mixer_info.mixer_hw_id << IPC_STR_ID_SHIFT); - header |= (IPC_STG_SET_VOLUME << IPC_STG_TYPE_SHIFT); - header |= (stage_id << IPC_STG_ID_SHIFT); + request.header |= (hsw->mixer_info.mixer_hw_id << IPC_STR_ID_SHIFT); + request.header |= (IPC_STG_SET_VOLUME << IPC_STG_TYPE_SHIFT); + request.header |= (stage_id << IPC_STG_ID_SHIFT);
req.curve_duration = hsw->curve_duration; req.curve_type = hsw->curve_type; req.target_volume = volume;
- ret = sst_ipc_tx_message_wait(&hsw->ipc, header, &req, - sizeof(req), NULL, 0); + request.data = &req; + request.size = sizeof(req); + ret = sst_ipc_tx_message_wait(&hsw->ipc, request, NULL); if (ret < 0) { dev_err(hsw->dev, "error: set mixer volume failed\n"); return ret; @@ -983,7 +988,7 @@ struct sst_hsw_stream *sst_hsw_stream_new(struct sst_hsw *hsw, int id,
int sst_hsw_stream_free(struct sst_hsw *hsw, struct sst_hsw_stream *stream) { - u32 header; + struct sst_ipc_message request; int ret = 0; struct sst_dsp *sst = hsw->dsp; unsigned long flags; @@ -1000,10 +1005,11 @@ int sst_hsw_stream_free(struct sst_hsw *hsw, struct sst_hsw_stream *stream) trace_ipc_request("stream free", stream->host_id);
stream->free_req.stream_id = stream->reply.stream_hw_id; - header = IPC_GLB_TYPE(IPC_GLB_FREE_STREAM); + request.header = IPC_GLB_TYPE(IPC_GLB_FREE_STREAM); + request.data = &stream->free_req; + request.size = sizeof(stream->free_req);
- ret = sst_ipc_tx_message_wait(&hsw->ipc, header, &stream->free_req, - sizeof(stream->free_req), NULL, 0); + ret = sst_ipc_tx_message_wait(&hsw->ipc, request, NULL); if (ret < 0) { dev_err(hsw->dev, "error: free stream %d failed\n", stream->free_req.stream_id); @@ -1175,9 +1181,7 @@ int sst_hsw_stream_set_module_info(struct sst_hsw *hsw,
int sst_hsw_stream_commit(struct sst_hsw *hsw, struct sst_hsw_stream *stream) { - struct sst_hsw_ipc_stream_alloc_req *str_req = &stream->request; - struct sst_hsw_ipc_stream_alloc_reply *reply = &stream->reply; - u32 header; + struct sst_ipc_message request, reply = {0}; int ret;
if (!stream) { @@ -1192,10 +1196,13 @@ int sst_hsw_stream_commit(struct sst_hsw *hsw, struct sst_hsw_stream *stream)
trace_ipc_request("stream alloc", stream->host_id);
- header = IPC_GLB_TYPE(IPC_GLB_ALLOCATE_STREAM); + request.header = IPC_GLB_TYPE(IPC_GLB_ALLOCATE_STREAM); + request.data = &stream->request; + request.size = sizeof(stream->request); + reply.data = &stream->reply; + reply.size = sizeof(stream->reply);
- ret = sst_ipc_tx_message_wait(&hsw->ipc, header, str_req, - sizeof(*str_req), reply, sizeof(*reply)); + ret = sst_ipc_tx_message_wait(&hsw->ipc, request, &reply); if (ret < 0) { dev_err(hsw->dev, "error: stream commit failed\n"); return ret; @@ -1235,23 +1242,22 @@ void sst_hsw_stream_set_silence_start(struct sst_hsw *hsw, ABI to be opaque to client PCM drivers to cope with any future ABI changes */ int sst_hsw_mixer_get_info(struct sst_hsw *hsw) { - struct sst_hsw_ipc_stream_info_reply *reply; - u32 header; + struct sst_ipc_message request = {0}, reply = {0}; int ret;
- reply = &hsw->mixer_info; - header = IPC_GLB_TYPE(IPC_GLB_GET_MIXER_STREAM_INFO); + request.header = IPC_GLB_TYPE(IPC_GLB_GET_MIXER_STREAM_INFO); + reply.data = &hsw->mixer_info; + reply.size = sizeof(hsw->mixer_info);
trace_ipc_request("get global mixer info", 0);
- ret = sst_ipc_tx_message_wait(&hsw->ipc, header, NULL, 0, - reply, sizeof(*reply)); + ret = sst_ipc_tx_message_wait(&hsw->ipc, request, &reply); if (ret < 0) { dev_err(hsw->dev, "error: get stream info failed\n"); return ret; }
- trace_hsw_mixer_info_reply(reply); + trace_hsw_mixer_info_reply(&hsw->mixer_info);
return 0; } @@ -1260,16 +1266,15 @@ int sst_hsw_mixer_get_info(struct sst_hsw *hsw) static int sst_hsw_stream_operations(struct sst_hsw *hsw, int type, int stream_id, int wait) { - u32 header; + struct sst_ipc_message request = {0};
- header = IPC_GLB_TYPE(IPC_GLB_STREAM_MESSAGE) | IPC_STR_TYPE(type); - header |= (stream_id << IPC_STR_ID_SHIFT); + request.header = IPC_GLB_TYPE(IPC_GLB_STREAM_MESSAGE); + request.header |= IPC_STR_TYPE(type) | (stream_id << IPC_STR_ID_SHIFT);
if (wait) - return sst_ipc_tx_message_wait(&hsw->ipc, header, - NULL, 0, NULL, 0); + return sst_ipc_tx_message_wait(&hsw->ipc, request, NULL); else - return sst_ipc_tx_message_nowait(&hsw->ipc, header, NULL, 0); + return sst_ipc_tx_message_nowait(&hsw->ipc, request); }
/* Stream ALSA trigger operations */ @@ -1377,8 +1382,8 @@ int sst_hsw_device_set_config(struct sst_hsw *hsw, enum sst_hsw_device_id dev, enum sst_hsw_device_mclk mclk, enum sst_hsw_device_mode mode, u32 clock_divider) { + struct sst_ipc_message request; struct sst_hsw_ipc_device_config_req config; - u32 header; int ret;
trace_ipc_request("set device config", dev); @@ -1394,10 +1399,11 @@ int sst_hsw_device_set_config(struct sst_hsw *hsw,
trace_hsw_device_config_req(&config);
- header = IPC_GLB_TYPE(IPC_GLB_SET_DEVICE_FORMATS); + request.header = IPC_GLB_TYPE(IPC_GLB_SET_DEVICE_FORMATS); + request.data = &config; + request.size = sizeof(config);
- ret = sst_ipc_tx_message_wait(&hsw->ipc, header, &config, - sizeof(config), NULL, 0); + ret = sst_ipc_tx_message_wait(&hsw->ipc, request, NULL); if (ret < 0) dev_err(hsw->dev, "error: set device formats failed\n");
@@ -1409,16 +1415,20 @@ EXPORT_SYMBOL_GPL(sst_hsw_device_set_config); int sst_hsw_dx_set_state(struct sst_hsw *hsw, enum sst_hsw_dx_state state, struct sst_hsw_ipc_dx_reply *dx) { - u32 header, state_; + struct sst_ipc_message request, reply = {0}; + u32 state_; int ret, item;
- header = IPC_GLB_TYPE(IPC_GLB_ENTER_DX_STATE); state_ = state; + request.header = IPC_GLB_TYPE(IPC_GLB_ENTER_DX_STATE); + request.data = &state_; + request.size = sizeof(state_); + reply.data = dx; + reply.size = sizeof(*dx);
trace_ipc_request("PM enter Dx state", state);
- ret = sst_ipc_tx_message_wait(&hsw->ipc, header, &state_, - sizeof(state_), dx, sizeof(*dx)); + ret = sst_ipc_tx_message_wait(&hsw->ipc, request, &reply); if (ret < 0) { dev_err(hsw->dev, "ipc: error set dx state %d failed\n", state); return ret; @@ -1878,7 +1888,7 @@ int sst_hsw_module_enable(struct sst_hsw *hsw, u32 module_id, u32 instance_id) { int ret; - u32 header = 0; + struct sst_ipc_message request; struct sst_hsw_ipc_module_config config; struct sst_module *module; struct sst_module_runtime *runtime; @@ -1907,10 +1917,10 @@ int sst_hsw_module_enable(struct sst_hsw *hsw, return -ENXIO; }
- header = IPC_GLB_TYPE(IPC_GLB_MODULE_OPERATION) | + request.header = IPC_GLB_TYPE(IPC_GLB_MODULE_OPERATION) | IPC_MODULE_OPERATION(IPC_MODULE_ENABLE) | IPC_MODULE_ID(module_id); - dev_dbg(dev, "module enable header: %x\n", header); + dev_dbg(dev, "module enable header: %x\n", (u32)request.header);
config.map.module_entries_count = 1; config.map.module_entries[0].module_id = module->id; @@ -1932,8 +1942,9 @@ int sst_hsw_module_enable(struct sst_hsw *hsw, config.scratch_mem.size, config.scratch_mem.offset, config.map.module_entries[0].entry_point);
- ret = sst_ipc_tx_message_wait(&hsw->ipc, header, - &config, sizeof(config), NULL, 0); + request.data = &config; + request.size = sizeof(config); + ret = sst_ipc_tx_message_wait(&hsw->ipc, request, NULL); if (ret < 0) dev_err(dev, "ipc: module enable failed - %d\n", ret); else @@ -1946,7 +1957,7 @@ int sst_hsw_module_disable(struct sst_hsw *hsw, u32 module_id, u32 instance_id) { int ret; - u32 header; + struct sst_ipc_message request = {0}; struct sst_module *module; struct device *dev = hsw->dev; struct sst_dsp *dsp = hsw->dsp; @@ -1967,11 +1978,11 @@ int sst_hsw_module_disable(struct sst_hsw *hsw, return -ENXIO; }
- header = IPC_GLB_TYPE(IPC_GLB_MODULE_OPERATION) | + request.header = IPC_GLB_TYPE(IPC_GLB_MODULE_OPERATION) | IPC_MODULE_OPERATION(IPC_MODULE_DISABLE) | IPC_MODULE_ID(module_id);
- ret = sst_ipc_tx_message_wait(&hsw->ipc, header, NULL, 0, NULL, 0); + ret = sst_ipc_tx_message_wait(&hsw->ipc, request, NULL); if (ret < 0) dev_err(dev, "module disable failed - %d\n", ret); else @@ -1985,15 +1996,16 @@ int sst_hsw_module_set_param(struct sst_hsw *hsw, u32 param_size, char *param) { int ret; - u32 header = 0; - u32 payload_size = 0, transfer_parameter_size = 0; + struct sst_ipc_message request = {0}; + u32 payload_size = 0; struct sst_hsw_transfer_parameter *parameter; struct device *dev = hsw->dev;
- header = IPC_GLB_TYPE(IPC_GLB_MODULE_OPERATION) | + request.header = IPC_GLB_TYPE(IPC_GLB_MODULE_OPERATION) | IPC_MODULE_OPERATION(IPC_MODULE_SET_PARAMETER) | IPC_MODULE_ID(module_id); - dev_dbg(dev, "sst_hsw_module_set_param header=%x\n", header); + dev_dbg(dev, "sst_hsw_module_set_param header=%x\n", + (u32)request.header);
payload_size = param_size + sizeof(struct sst_hsw_transfer_parameter) - @@ -2003,14 +2015,14 @@ int sst_hsw_module_set_param(struct sst_hsw *hsw,
if (payload_size <= SST_HSW_IPC_MAX_SHORT_PARAMETER_SIZE) { /* short parameter, mailbox can contain data */ - dev_dbg(dev, "transfer parameter size : %d\n", - transfer_parameter_size); + dev_dbg(dev, "transfer parameter size : %lu\n", + request.size);
- transfer_parameter_size = ALIGN(payload_size, 4); - dev_dbg(dev, "transfer parameter aligned size : %d\n", - transfer_parameter_size); + request.size = ALIGN(payload_size, 4); + dev_dbg(dev, "transfer parameter aligned size : %lu\n", + request.size);
- parameter = kzalloc(transfer_parameter_size, GFP_KERNEL); + parameter = kzalloc(request.size, GFP_KERNEL); if (parameter == NULL) return -ENOMEM;
@@ -2022,9 +2034,9 @@ int sst_hsw_module_set_param(struct sst_hsw *hsw,
parameter->parameter_id = parameter_id; parameter->data_size = param_size; + request.data = parameter;
- ret = sst_ipc_tx_message_wait(&hsw->ipc, header, - parameter, transfer_parameter_size , NULL, 0); + ret = sst_ipc_tx_message_wait(&hsw->ipc, request, NULL); if (ret < 0) dev_err(dev, "ipc: module set parameter failed - %d\n", ret);
@@ -2041,8 +2053,8 @@ static struct sst_dsp_device hsw_dev = { static void hsw_tx_msg(struct sst_generic_ipc *ipc, struct ipc_message *msg) { /* send the message */ - sst_dsp_outbox_write(ipc->dsp, msg->tx_data, msg->tx_size); - sst_dsp_ipc_msg_tx(ipc->dsp, msg->header); + sst_dsp_outbox_write(ipc->dsp, msg->tx.data, msg->tx.size); + sst_dsp_ipc_msg_tx(ipc->dsp, msg->tx.header); }
static void hsw_shim_dbg(struct sst_generic_ipc *ipc, const char *text) @@ -2063,7 +2075,7 @@ static void hsw_shim_dbg(struct sst_generic_ipc *ipc, const char *text) static void hsw_tx_data_copy(struct ipc_message *msg, char *tx_data, size_t tx_size) { - memcpy(msg->tx_data, tx_data, tx_size); + memcpy(msg->tx.data, tx_data, tx_size); }
static u64 hsw_reply_msg_match(u64 header, u64 *mask)
There is struct sst_ipc_message instance representing request and reply each, and params: header, data and size need no longer to be declared explicitly. Align with that model.
Update reply processing function to save RX header within message container.
Despite the range of changes, status quo is achieved.
Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- sound/soc/intel/baytrail/sst-baytrail-ipc.c | 65 ++++++++++----------- 1 file changed, 32 insertions(+), 33 deletions(-)
diff --git a/sound/soc/intel/baytrail/sst-baytrail-ipc.c b/sound/soc/intel/baytrail/sst-baytrail-ipc.c index 8bd1eddcc091..74274bd38f7a 100644 --- a/sound/soc/intel/baytrail/sst-baytrail-ipc.c +++ b/sound/soc/intel/baytrail/sst-baytrail-ipc.c @@ -211,7 +211,7 @@ static struct sst_byt_stream *sst_byt_get_stream(struct sst_byt *byt, static void sst_byt_stream_update(struct sst_byt *byt, struct ipc_message *msg) { struct sst_byt_stream *stream; - u64 header = msg->header; + u64 header = msg->tx.header; u8 stream_id = sst_byt_header_str_id(header); u8 stream_msg = sst_byt_header_msg_id(header);
@@ -240,9 +240,10 @@ static int sst_byt_process_reply(struct sst_byt *byt, u64 header) if (msg == NULL) return 1;
+ msg->rx.header = header; if (header & IPC_HEADER_LARGE(true)) { - msg->rx_size = sst_byt_header_data(header); - sst_dsp_inbox_read(byt->dsp, msg->rx_data, msg->rx_size); + msg->rx.size = sst_byt_header_data(header); + sst_dsp_inbox_read(byt->dsp, msg->rx.data, msg->rx.size); }
/* update any stream states */ @@ -407,17 +408,18 @@ int sst_byt_stream_buffer(struct sst_byt *byt, struct sst_byt_stream *stream,
int sst_byt_stream_commit(struct sst_byt *byt, struct sst_byt_stream *stream) { - struct sst_byt_alloc_params *str_req = &stream->request; - struct sst_byt_alloc_response *reply = &stream->reply; - u64 header; + struct sst_ipc_message request, reply = {0}; int ret;
- header = sst_byt_header(IPC_IA_ALLOC_STREAM, - sizeof(*str_req) + sizeof(u32), + request.header = sst_byt_header(IPC_IA_ALLOC_STREAM, + sizeof(stream->request) + sizeof(u32), true, stream->str_id); - ret = sst_ipc_tx_message_wait(&byt->ipc, header, str_req, - sizeof(*str_req), - reply, sizeof(*reply)); + request.data = &stream->request; + request.size = sizeof(stream->request); + reply.data = &stream->reply; + reply.size = sizeof(stream->reply); + + ret = sst_ipc_tx_message_wait(&byt->ipc, request, &reply); if (ret < 0) { dev_err(byt->dev, "ipc: error stream commit failed\n"); return ret; @@ -430,7 +432,7 @@ int sst_byt_stream_commit(struct sst_byt *byt, struct sst_byt_stream *stream)
int sst_byt_stream_free(struct sst_byt *byt, struct sst_byt_stream *stream) { - u64 header; + struct sst_ipc_message request = {0}; int ret = 0; struct sst_dsp *sst = byt->dsp; unsigned long flags; @@ -438,8 +440,9 @@ int sst_byt_stream_free(struct sst_byt *byt, struct sst_byt_stream *stream) if (!stream->commited) goto out;
- header = sst_byt_header(IPC_IA_FREE_STREAM, 0, false, stream->str_id); - ret = sst_ipc_tx_message_wait(&byt->ipc, header, NULL, 0, NULL, 0); + request.header = sst_byt_header(IPC_IA_FREE_STREAM, + 0, false, stream->str_id); + ret = sst_ipc_tx_message_wait(&byt->ipc, request, NULL); if (ret < 0) { dev_err(byt->dev, "ipc: free stream %d failed\n", stream->str_id); @@ -459,15 +462,13 @@ int sst_byt_stream_free(struct sst_byt *byt, struct sst_byt_stream *stream) static int sst_byt_stream_operations(struct sst_byt *byt, int type, int stream_id, int wait) { - u64 header; + struct sst_ipc_message request = {0};
- header = sst_byt_header(type, 0, false, stream_id); + request.header = sst_byt_header(type, 0, false, stream_id); if (wait) - return sst_ipc_tx_message_wait(&byt->ipc, header, NULL, - 0, NULL, 0); + return sst_ipc_tx_message_wait(&byt->ipc, request, NULL); else - return sst_ipc_tx_message_nowait(&byt->ipc, header, - NULL, 0); + return sst_ipc_tx_message_nowait(&byt->ipc, request); }
/* stream ALSA trigger operations */ @@ -475,19 +476,17 @@ int sst_byt_stream_start(struct sst_byt *byt, struct sst_byt_stream *stream, u32 start_offset) { struct sst_byt_start_stream_params start_stream; - void *tx_msg; - size_t size; - u64 header; + struct sst_ipc_message request; int ret;
start_stream.byte_offset = start_offset; - header = sst_byt_header(IPC_IA_START_STREAM, + request.header = sst_byt_header(IPC_IA_START_STREAM, sizeof(start_stream) + sizeof(u32), true, stream->str_id); - tx_msg = &start_stream; - size = sizeof(start_stream); + request.data = &start_stream; + request.size = sizeof(start_stream);
- ret = sst_ipc_tx_message_nowait(&byt->ipc, header, tx_msg, size); + ret = sst_ipc_tx_message_nowait(&byt->ipc, request); if (ret < 0) dev_err(byt->dev, "ipc: error failed to start stream %d\n", stream->str_id); @@ -623,10 +622,10 @@ EXPORT_SYMBOL_GPL(sst_byt_dsp_wait_for_ready);
static void byt_tx_msg(struct sst_generic_ipc *ipc, struct ipc_message *msg) { - if (msg->header & IPC_HEADER_LARGE(true)) - sst_dsp_outbox_write(ipc->dsp, msg->tx_data, msg->tx_size); + if (msg->tx.header & IPC_HEADER_LARGE(true)) + sst_dsp_outbox_write(ipc->dsp, msg->tx.data, msg->tx.size);
- sst_dsp_shim_write64_unlocked(ipc->dsp, SST_IPCX, msg->header); + sst_dsp_shim_write64_unlocked(ipc->dsp, SST_IPCX, msg->tx.header); }
static void byt_shim_dbg(struct sst_generic_ipc *ipc, const char *text) @@ -648,9 +647,9 @@ static void byt_tx_data_copy(struct ipc_message *msg, char *tx_data, size_t tx_size) { /* msg content = lower 32-bit of the header + data */ - *(u32 *)msg->tx_data = (u32)(msg->header & (u32)-1); - memcpy(msg->tx_data + sizeof(u32), tx_data, tx_size); - msg->tx_size += sizeof(u32); + *(u32 *)msg->tx.data = (u32)(msg->tx.header & (u32)-1); + memcpy(msg->tx.data + sizeof(u32), tx_data, tx_size); + msg->tx.size += sizeof(u32); }
static u64 byt_reply_msg_match(u64 header, u64 *mask)
There is struct sst_ipc_message instance representing request and reply each, and params: header, data and size need no longer to be declared explicitly. Align with that model.
Update reply processing function to save RX header within message container.
Despite the range of changes, status quo is achieved.
Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- sound/soc/intel/skylake/cnl-sst.c | 6 +- sound/soc/intel/skylake/skl-sst-ipc.c | 110 ++++++++++++++++---------- 2 files changed, 70 insertions(+), 46 deletions(-)
diff --git a/sound/soc/intel/skylake/cnl-sst.c b/sound/soc/intel/skylake/cnl-sst.c index 2d748a335bcf..4f64f097e9ae 100644 --- a/sound/soc/intel/skylake/cnl-sst.c +++ b/sound/soc/intel/skylake/cnl-sst.c @@ -366,10 +366,10 @@ static struct sst_dsp_device cnl_dev = {
static void cnl_ipc_tx_msg(struct sst_generic_ipc *ipc, struct ipc_message *msg) { - struct skl_ipc_header *header = (struct skl_ipc_header *)(&msg->header); + struct skl_ipc_header *header = (struct skl_ipc_header *)(&msg->tx.header);
- if (msg->tx_size) - sst_dsp_outbox_write(ipc->dsp, msg->tx_data, msg->tx_size); + if (msg->tx.size) + sst_dsp_outbox_write(ipc->dsp, msg->tx.data, msg->tx.size); sst_dsp_shim_write_unlocked(ipc->dsp, CNL_ADSP_REG_HIPCIDD, header->extension); sst_dsp_shim_write_unlocked(ipc->dsp, CNL_ADSP_REG_HIPCIDR, diff --git a/sound/soc/intel/skylake/skl-sst-ipc.c b/sound/soc/intel/skylake/skl-sst-ipc.c index ee1493acc9a8..a2b69a02aab2 100644 --- a/sound/soc/intel/skylake/skl-sst-ipc.c +++ b/sound/soc/intel/skylake/skl-sst-ipc.c @@ -281,7 +281,7 @@ void skl_ipc_tx_data_copy(struct ipc_message *msg, char *tx_data, size_t tx_size) { if (tx_size) - memcpy(msg->tx_data, tx_data, tx_size); + memcpy(msg->tx.data, tx_data, tx_size); }
static bool skl_ipc_is_dsp_busy(struct sst_dsp *dsp) @@ -295,10 +295,10 @@ static bool skl_ipc_is_dsp_busy(struct sst_dsp *dsp) /* Lock to be held by caller */ static void skl_ipc_tx_msg(struct sst_generic_ipc *ipc, struct ipc_message *msg) { - struct skl_ipc_header *header = (struct skl_ipc_header *)(&msg->header); + struct skl_ipc_header *header = (struct skl_ipc_header *)(&msg->tx.header);
- if (msg->tx_size) - sst_dsp_outbox_write(ipc->dsp, msg->tx_data, msg->tx_size); + if (msg->tx.size) + sst_dsp_outbox_write(ipc->dsp, msg->tx.data, msg->tx.size); sst_dsp_shim_write_unlocked(ipc->dsp, SKL_ADSP_REG_HIPCIE, header->extension); sst_dsp_shim_write_unlocked(ipc->dsp, SKL_ADSP_REG_HIPCI, @@ -447,11 +447,12 @@ void skl_ipc_process_reply(struct sst_generic_ipc *ipc, return; }
+ msg->rx.header = *ipc_header; /* first process the header */ if (reply == IPC_GLB_REPLY_SUCCESS) { dev_dbg(ipc->dev, "ipc FW reply %x: success\n", header.primary); /* copy the rx data from the mailbox */ - sst_dsp_inbox_read(ipc->dsp, msg->rx_data, msg->rx_size); + sst_dsp_inbox_read(ipc->dsp, msg->rx.data, msg->rx.size); switch (IPC_GLB_NOTIFY_MSG_TYPE(header.primary)) { case IPC_GLB_LOAD_MULTIPLE_MODS: case IPC_GLB_LOAD_LIBRARY: @@ -635,7 +636,7 @@ int skl_ipc_create_pipeline(struct sst_generic_ipc *ipc, u16 ppl_mem_size, u8 ppl_type, u8 instance_id, u8 lp_mode) { struct skl_ipc_header header = {0}; - u64 *ipc_header = (u64 *)(&header); + struct sst_ipc_message request = {0}; int ret;
header.primary = IPC_MSG_TARGET(IPC_FW_GEN_MSG); @@ -646,9 +647,10 @@ int skl_ipc_create_pipeline(struct sst_generic_ipc *ipc, header.primary |= IPC_PPL_MEM_SIZE(ppl_mem_size);
header.extension = IPC_PPL_LP_MODE(lp_mode); + request.header = *(u64 *)(&header);
dev_dbg(ipc->dev, "In %s header=%d\n", __func__, header.primary); - ret = sst_ipc_tx_message_wait(ipc, *ipc_header, NULL, 0, NULL, 0); + ret = sst_ipc_tx_message_wait(ipc, request, NULL); if (ret < 0) { dev_err(ipc->dev, "ipc: create pipeline fail, err: %d\n", ret); return ret; @@ -661,16 +663,17 @@ EXPORT_SYMBOL_GPL(skl_ipc_create_pipeline); int skl_ipc_delete_pipeline(struct sst_generic_ipc *ipc, u8 instance_id) { struct skl_ipc_header header = {0}; - u64 *ipc_header = (u64 *)(&header); + struct sst_ipc_message request = {0}; int ret;
header.primary = IPC_MSG_TARGET(IPC_FW_GEN_MSG); header.primary |= IPC_MSG_DIR(IPC_MSG_REQUEST); header.primary |= IPC_GLB_TYPE(IPC_GLB_DELETE_PPL); header.primary |= IPC_INSTANCE_ID(instance_id); + request.header = *(u64 *)(&header);
dev_dbg(ipc->dev, "In %s header=%d\n", __func__, header.primary); - ret = sst_ipc_tx_message_wait(ipc, *ipc_header, NULL, 0, NULL, 0); + ret = sst_ipc_tx_message_wait(ipc, request, NULL); if (ret < 0) { dev_err(ipc->dev, "ipc: delete pipeline failed, err %d\n", ret); return ret; @@ -684,7 +687,7 @@ int skl_ipc_set_pipeline_state(struct sst_generic_ipc *ipc, u8 instance_id, enum skl_ipc_pipeline_state state) { struct skl_ipc_header header = {0}; - u64 *ipc_header = (u64 *)(&header); + struct sst_ipc_message request = {0}; int ret;
header.primary = IPC_MSG_TARGET(IPC_FW_GEN_MSG); @@ -692,9 +695,10 @@ int skl_ipc_set_pipeline_state(struct sst_generic_ipc *ipc, header.primary |= IPC_GLB_TYPE(IPC_GLB_SET_PPL_STATE); header.primary |= IPC_INSTANCE_ID(instance_id); header.primary |= IPC_PPL_STATE(state); + request.header = *(u64 *)(&header);
dev_dbg(ipc->dev, "In %s header=%d\n", __func__, header.primary); - ret = sst_ipc_tx_message_wait(ipc, *ipc_header, NULL, 0, NULL, 0); + ret = sst_ipc_tx_message_wait(ipc, request, NULL); if (ret < 0) { dev_err(ipc->dev, "ipc: set pipeline state failed, err: %d\n", ret); return ret; @@ -707,7 +711,7 @@ int skl_ipc_save_pipeline(struct sst_generic_ipc *ipc, u8 instance_id, int dma_id) { struct skl_ipc_header header = {0}; - u64 *ipc_header = (u64 *)(&header); + struct sst_ipc_message request = {0}; int ret;
header.primary = IPC_MSG_TARGET(IPC_FW_GEN_MSG); @@ -716,8 +720,10 @@ skl_ipc_save_pipeline(struct sst_generic_ipc *ipc, u8 instance_id, int dma_id) header.primary |= IPC_INSTANCE_ID(instance_id);
header.extension = IPC_DMA_ID(dma_id); + request.header = *(u64 *)(&header); + dev_dbg(ipc->dev, "In %s header=%d\n", __func__, header.primary); - ret = sst_ipc_tx_message_wait(ipc, *ipc_header, NULL, 0, NULL, 0); + ret = sst_ipc_tx_message_wait(ipc, request, NULL); if (ret < 0) { dev_err(ipc->dev, "ipc: save pipeline failed, err: %d\n", ret); return ret; @@ -730,16 +736,17 @@ EXPORT_SYMBOL_GPL(skl_ipc_save_pipeline); int skl_ipc_restore_pipeline(struct sst_generic_ipc *ipc, u8 instance_id) { struct skl_ipc_header header = {0}; - u64 *ipc_header = (u64 *)(&header); + struct sst_ipc_message request = {0}; int ret;
header.primary = IPC_MSG_TARGET(IPC_FW_GEN_MSG); header.primary |= IPC_MSG_DIR(IPC_MSG_REQUEST); header.primary |= IPC_GLB_TYPE(IPC_GLB_RESTORE_PPL); header.primary |= IPC_INSTANCE_ID(instance_id); + request.header = *(u64 *)(&header);
dev_dbg(ipc->dev, "In %s header=%d\n", __func__, header.primary); - ret = sst_ipc_tx_message_wait(ipc, *ipc_header, NULL, 0, NULL, 0); + ret = sst_ipc_tx_message_wait(ipc, request, NULL); if (ret < 0) { dev_err(ipc->dev, "ipc: restore pipeline failed, err: %d\n", ret); return ret; @@ -753,7 +760,7 @@ int skl_ipc_set_dx(struct sst_generic_ipc *ipc, u8 instance_id, u16 module_id, struct skl_ipc_dxstate_info *dx) { struct skl_ipc_header header = {0}; - u64 *ipc_header = (u64 *)(&header); + struct sst_ipc_message request; int ret;
header.primary = IPC_MSG_TARGET(IPC_MOD_MSG); @@ -762,10 +769,13 @@ int skl_ipc_set_dx(struct sst_generic_ipc *ipc, u8 instance_id, header.primary |= IPC_MOD_INSTANCE_ID(instance_id); header.primary |= IPC_MOD_ID(module_id);
+ request.header = *(u64 *)(&header); + request.data = dx; + request.size = sizeof(*dx); + dev_dbg(ipc->dev, "In %s primary =%x ext=%x\n", __func__, header.primary, header.extension); - ret = sst_ipc_tx_message_wait(ipc, *ipc_header, - dx, sizeof(*dx), NULL, 0); + ret = sst_ipc_tx_message_wait(ipc, request, NULL); if (ret < 0) { dev_err(ipc->dev, "ipc: set dx failed, err %d\n", ret); return ret; @@ -779,7 +789,7 @@ int skl_ipc_init_instance(struct sst_generic_ipc *ipc, struct skl_ipc_init_instance_msg *msg, void *param_data) { struct skl_ipc_header header = {0}; - u64 *ipc_header = (u64 *)(&header); + struct sst_ipc_message request; int ret; u32 *buffer = (u32 *)param_data; /* param_block_size must be in dwords */ @@ -799,10 +809,13 @@ int skl_ipc_init_instance(struct sst_generic_ipc *ipc, header.extension |= IPC_PARAM_BLOCK_SIZE(param_block_size); header.extension |= IPC_DOMAIN(msg->domain);
+ request.header = *(u64 *)(&header); + request.data = param_data; + request.size = msg->param_data_size; + dev_dbg(ipc->dev, "In %s primary =%x ext=%x\n", __func__, header.primary, header.extension); - ret = sst_ipc_tx_message_wait(ipc, *ipc_header, param_data, - msg->param_data_size, NULL, 0); + ret = sst_ipc_tx_message_wait(ipc, request, NULL);
if (ret < 0) { dev_err(ipc->dev, "ipc: init instance failed\n"); @@ -817,7 +830,7 @@ int skl_ipc_bind_unbind(struct sst_generic_ipc *ipc, struct skl_ipc_bind_unbind_msg *msg) { struct skl_ipc_header header = {0}; - u64 *ipc_header = (u64 *)(&header); + struct sst_ipc_message request = {0}; u8 bind_unbind = msg->bind ? IPC_MOD_BIND : IPC_MOD_UNBIND; int ret;
@@ -831,10 +844,11 @@ int skl_ipc_bind_unbind(struct sst_generic_ipc *ipc, header.extension |= IPC_DST_MOD_INSTANCE_ID(msg->dst_instance_id); header.extension |= IPC_DST_QUEUE(msg->dst_queue); header.extension |= IPC_SRC_QUEUE(msg->src_queue); + request.header = *(u64 *)(&header);
dev_dbg(ipc->dev, "In %s hdr=%x ext=%x\n", __func__, header.primary, header.extension); - ret = sst_ipc_tx_message_wait(ipc, *ipc_header, NULL, 0, NULL, 0); + ret = sst_ipc_tx_message_wait(ipc, request, NULL); if (ret < 0) { dev_err(ipc->dev, "ipc: bind/unbind failed\n"); return ret; @@ -854,7 +868,7 @@ int skl_ipc_load_modules(struct sst_generic_ipc *ipc, u8 module_cnt, void *data) { struct skl_ipc_header header = {0}; - u64 *ipc_header = (u64 *)(&header); + struct sst_ipc_message request; int ret;
header.primary = IPC_MSG_TARGET(IPC_FW_GEN_MSG); @@ -862,8 +876,11 @@ int skl_ipc_load_modules(struct sst_generic_ipc *ipc, header.primary |= IPC_GLB_TYPE(IPC_GLB_LOAD_MULTIPLE_MODS); header.primary |= IPC_LOAD_MODULE_CNT(module_cnt);
- ret = sst_ipc_tx_message_nowait(ipc, *ipc_header, data, - (sizeof(u16) * module_cnt)); + request.header = *(u64 *)(&header); + request.data = data; + request.size = sizeof(u16) * module_cnt; + + ret = sst_ipc_tx_message_nowait(ipc, request); if (ret < 0) dev_err(ipc->dev, "ipc: load modules failed :%d\n", ret);
@@ -875,7 +892,7 @@ int skl_ipc_unload_modules(struct sst_generic_ipc *ipc, u8 module_cnt, void *data) { struct skl_ipc_header header = {0}; - u64 *ipc_header = (u64 *)(&header); + struct sst_ipc_message request; int ret;
header.primary = IPC_MSG_TARGET(IPC_FW_GEN_MSG); @@ -883,8 +900,11 @@ int skl_ipc_unload_modules(struct sst_generic_ipc *ipc, u8 module_cnt, header.primary |= IPC_GLB_TYPE(IPC_GLB_UNLOAD_MULTIPLE_MODS); header.primary |= IPC_LOAD_MODULE_CNT(module_cnt);
- ret = sst_ipc_tx_message_wait(ipc, *ipc_header, data, - (sizeof(u16) * module_cnt), NULL, 0); + request.header = *(u64 *)(&header); + request.data = data; + request.size = sizeof(u16) * module_cnt; + + ret = sst_ipc_tx_message_wait(ipc, request, NULL); if (ret < 0) dev_err(ipc->dev, "ipc: unload modules failed :%d\n", ret);
@@ -896,7 +916,7 @@ int skl_ipc_set_large_config(struct sst_generic_ipc *ipc, struct skl_ipc_large_config_msg *msg, u32 *param) { struct skl_ipc_header header = {0}; - u64 *ipc_header = (u64 *)(&header); + struct sst_ipc_message request; int ret = 0; size_t sz_remaining, tx_size, data_offset;
@@ -923,9 +943,11 @@ int skl_ipc_set_large_config(struct sst_generic_ipc *ipc, header.primary, header.extension); dev_dbg(ipc->dev, "transmitting offset: %#x, size: %#x\n", (unsigned)data_offset, (unsigned)tx_size); - ret = sst_ipc_tx_message_wait(ipc, *ipc_header, - ((char *)param) + data_offset, - tx_size, NULL, 0); + + request.header = *(u64 *)(&header); + request.data = ((char *)param) + data_offset; + request.size = tx_size; + ret = sst_ipc_tx_message_wait(ipc, request, NULL); if (ret < 0) { dev_err(ipc->dev, "ipc: set large config fail, err: %d\n", ret); @@ -950,7 +972,7 @@ int skl_ipc_get_large_config(struct sst_generic_ipc *ipc, struct skl_ipc_large_config_msg *msg, u32 *param) { struct skl_ipc_header header = {0}; - u64 *ipc_header = (u64 *)(&header); + struct sst_ipc_message request = {0}, reply = {0}; int ret = 0; size_t sz_remaining, rx_size, data_offset;
@@ -974,9 +996,10 @@ int skl_ipc_get_large_config(struct sst_generic_ipc *ipc, if (rx_size == sz_remaining) header.extension |= IPC_FINAL_BLOCK(1);
- ret = sst_ipc_tx_message_wait(ipc, *ipc_header, NULL, 0, - ((char *)param) + data_offset, - msg->param_data_size); + request.header = *(u64 *)(&header); + reply.data = ((char *)param) + data_offset; + reply.size = msg->param_data_size; + ret = sst_ipc_tx_message_wait(ipc, request, &reply); if (ret < 0) { dev_err(ipc->dev, "ipc: get large config fail, err: %d\n", ret); @@ -1001,7 +1024,7 @@ int skl_sst_ipc_load_library(struct sst_generic_ipc *ipc, u8 dma_id, u8 table_id, bool wait) { struct skl_ipc_header header = {0}; - u64 *ipc_header = (u64 *)(&header); + struct sst_ipc_message request = {0}; int ret = 0;
header.primary = IPC_MSG_TARGET(IPC_FW_GEN_MSG); @@ -1009,12 +1032,12 @@ int skl_sst_ipc_load_library(struct sst_generic_ipc *ipc, header.primary |= IPC_GLB_TYPE(IPC_GLB_LOAD_LIBRARY); header.primary |= IPC_MOD_INSTANCE_ID(table_id); header.primary |= IPC_MOD_ID(dma_id); + request.header = *(u64 *)(&header);
if (wait) - ret = sst_ipc_tx_message_wait(ipc, *ipc_header, - NULL, 0, NULL, 0); + ret = sst_ipc_tx_message_wait(ipc, request, NULL); else - ret = sst_ipc_tx_message_nowait(ipc, *ipc_header, NULL, 0); + ret = sst_ipc_tx_message_nowait(ipc, request);
if (ret < 0) dev_err(ipc->dev, "ipc: load lib failed\n"); @@ -1026,7 +1049,7 @@ EXPORT_SYMBOL_GPL(skl_sst_ipc_load_library); int skl_ipc_set_d0ix(struct sst_generic_ipc *ipc, struct skl_ipc_d0ix_msg *msg) { struct skl_ipc_header header = {0}; - u64 *ipc_header = (u64 *)(&header); + struct sst_ipc_message request = {0}; int ret;
header.primary = IPC_MSG_TARGET(IPC_MOD_MSG); @@ -1037,6 +1060,7 @@ int skl_ipc_set_d0ix(struct sst_generic_ipc *ipc, struct skl_ipc_d0ix_msg *msg)
header.extension = IPC_D0IX_WAKE(msg->wake); header.extension |= IPC_D0IX_STREAMING(msg->streaming); + request.header = *(u64 *)(&header);
dev_dbg(ipc->dev, "In %s primary=%x ext=%x\n", __func__, header.primary, header.extension); @@ -1044,7 +1068,7 @@ int skl_ipc_set_d0ix(struct sst_generic_ipc *ipc, struct skl_ipc_d0ix_msg *msg) /* * Use the nopm IPC here as we dont want it checking for D0iX */ - ret = sst_ipc_tx_message_nopm(ipc, *ipc_header, NULL, 0, NULL, 0); + ret = sst_ipc_tx_message_nopm(ipc, request, NULL); if (ret < 0) dev_err(ipc->dev, "ipc: set d0ix failed, err %d\n", ret);
LARGE_CONFIG_GET is among the most crucial IPCs. Host is expected to send single request first to obtain total payload size from received header's data_off_size. From then on, it loops for each frame exceeding inbox size until entire payload is read. If entire payload is contained within the very first frame, no looping is performed.
LARGE_CONFIG_GET is a flexible IPC, it not only receives payload but may carry one with them to provide list of params DSP module should return info for. This behavior is usually reserved for vendors and IPC handler should not touch that content. To achieve that, simply pass provided payload and bytes to sst_ipc_tx_message_wait as a part of request.
In current state, LARGE_CONFIG_GET message handler does nothing of that, in consequence making it dysfunctional. Overhaul said handler allowing rightful king of IPCs to return back on his throne - kingdom he shares with his twin brother: LARGE_CONFIG_SET.
The looping has not been added in this update as payloads of such size do not exist in practice. Will need to create custom module specifically for that very case and test against it before that feature can be added.
Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- sound/soc/intel/skylake/skl-messages.c | 3 +- sound/soc/intel/skylake/skl-sst-ipc.c | 54 +++++++++++--------------- sound/soc/intel/skylake/skl-sst-ipc.h | 3 +- 3 files changed, 27 insertions(+), 33 deletions(-)
diff --git a/sound/soc/intel/skylake/skl-messages.c b/sound/soc/intel/skylake/skl-messages.c index e8cc710f092b..84f0e6f58eb5 100644 --- a/sound/soc/intel/skylake/skl-messages.c +++ b/sound/soc/intel/skylake/skl-messages.c @@ -1379,11 +1379,12 @@ int skl_get_module_params(struct skl_dev *skl, u32 *params, int size, u32 param_id, struct skl_module_cfg *mcfg) { struct skl_ipc_large_config_msg msg; + size_t bytes = size;
msg.module_id = mcfg->id.module_id; msg.instance_id = mcfg->id.pvt_id; msg.param_data_size = size; msg.large_param_id = param_id;
- return skl_ipc_get_large_config(&skl->ipc, &msg, params); + return skl_ipc_get_large_config(&skl->ipc, &msg, ¶ms, &bytes); } diff --git a/sound/soc/intel/skylake/skl-sst-ipc.c b/sound/soc/intel/skylake/skl-sst-ipc.c index a2b69a02aab2..9d269a5f8bd9 100644 --- a/sound/soc/intel/skylake/skl-sst-ipc.c +++ b/sound/soc/intel/skylake/skl-sst-ipc.c @@ -969,12 +969,17 @@ int skl_ipc_set_large_config(struct sst_generic_ipc *ipc, EXPORT_SYMBOL_GPL(skl_ipc_set_large_config);
int skl_ipc_get_large_config(struct sst_generic_ipc *ipc, - struct skl_ipc_large_config_msg *msg, u32 *param) + struct skl_ipc_large_config_msg *msg, + unsigned int **payload, size_t *bytes) { struct skl_ipc_header header = {0}; - struct sst_ipc_message request = {0}, reply = {0}; - int ret = 0; - size_t sz_remaining, rx_size, data_offset; + struct sst_ipc_message request, reply = {0}; + unsigned int *buf; + int ret; + + reply.data = kzalloc(SKL_ADSP_W1_SZ, GFP_KERNEL); + if (!reply.data) + return -ENOMEM;
header.primary = IPC_MSG_TARGET(IPC_MOD_MSG); header.primary |= IPC_MSG_DIR(IPC_MSG_REQUEST); @@ -987,34 +992,21 @@ int skl_ipc_get_large_config(struct sst_generic_ipc *ipc, header.extension |= IPC_FINAL_BLOCK(1); header.extension |= IPC_INITIAL_BLOCK(1);
- sz_remaining = msg->param_data_size; - data_offset = 0; - - while (sz_remaining != 0) { - rx_size = sz_remaining > SKL_ADSP_W1_SZ - ? SKL_ADSP_W1_SZ : sz_remaining; - if (rx_size == sz_remaining) - header.extension |= IPC_FINAL_BLOCK(1); - - request.header = *(u64 *)(&header); - reply.data = ((char *)param) + data_offset; - reply.size = msg->param_data_size; - ret = sst_ipc_tx_message_wait(ipc, request, &reply); - if (ret < 0) { - dev_err(ipc->dev, - "ipc: get large config fail, err: %d\n", ret); - return ret; - } - sz_remaining -= rx_size; - data_offset = msg->param_data_size - sz_remaining; + request.header = *(u64 *)&header; + request.data = *payload; + request.size = *bytes; + reply.size = SKL_ADSP_W1_SZ;
- /* clear the fields */ - header.extension &= IPC_INITIAL_BLOCK_CLEAR; - header.extension &= IPC_DATA_OFFSET_SZ_CLEAR; - /* fill the fields */ - header.extension |= IPC_INITIAL_BLOCK(1); - header.extension |= IPC_DATA_OFFSET_SZ(data_offset); - } + ret = sst_ipc_tx_message_wait(ipc, request, &reply); + if (ret < 0) + dev_err(ipc->dev, "ipc: get large config fail: %d\n", ret); + + reply.size = (reply.header >> 32) & IPC_DATA_OFFSET_SZ_MASK; + buf = krealloc(reply.data, reply.size, GFP_KERNEL); + if (!buf) + return -ENOMEM; + *payload = buf; + *bytes = reply.size;
return ret; } diff --git a/sound/soc/intel/skylake/skl-sst-ipc.h b/sound/soc/intel/skylake/skl-sst-ipc.h index 93af08cf41d2..a7ab2c589cc5 100644 --- a/sound/soc/intel/skylake/skl-sst-ipc.h +++ b/sound/soc/intel/skylake/skl-sst-ipc.h @@ -139,7 +139,8 @@ int skl_ipc_set_large_config(struct sst_generic_ipc *ipc, struct skl_ipc_large_config_msg *msg, u32 *param);
int skl_ipc_get_large_config(struct sst_generic_ipc *ipc, - struct skl_ipc_large_config_msg *msg, u32 *param); + struct skl_ipc_large_config_msg *msg, + unsigned int **payload, size_t *bytes);
int skl_sst_ipc_load_library(struct sst_generic_ipc *ipc, u8 dma_id, u8 table_id, bool wait);
On 7/20/19 2:45 PM, Cezary Rojewski wrote:
Existing IPC framework omits crucial part of the entire communication: reply header. Some IPCs cannot function at all without the access to said header. Update the sst-ipc with new sst_ipc_message structure to represent both request and reply allowing reply-processing handlers to save received responses.
I don't get the idea at all.
the DSP provides rx_data and a size. If there is a header at the start of the IPC data in some cases, why can't you cast and extract the header when it's needed for your SKL usages? Why does adding a header make the IPC work better?
I have the feeling this is blurring layers between receiving data and processing it in platform-specific ways, and I am nervous about touching Baytrail and Broadwell legacy, which are way past maintenance mode and should be deprecated really.
And last this patchset does not apply on top of Mark's tree?
Despite the range of changes required for model to be updated, no functional changes are made for core hanswell, baytrail and skylake message handlers. Reply-processing handlers now save received response header yet no usage is added by default.
To allow for future changes, righful kings of IPC kingdom need to be put back on the throne. This update addresses one of them: LARGE_CONFIG_GET.
Cezary Rojewski (5): ASoC: Intel: Update request-reply IPC model ASoC: Intel: Haswell: Align with updated request-reply model ASoC: Intel: Baytrail: Align with updated request-reply model ASoC: Intel: Skylake: Align with updated request-reply model ASoC: Intel: Skylake: large_config_get overhaul
sound/soc/intel/baytrail/sst-baytrail-ipc.c | 65 ++++---- sound/soc/intel/common/sst-ipc.c | 68 ++++---- sound/soc/intel/common/sst-ipc.h | 27 ++-- sound/soc/intel/haswell/sst-haswell-ipc.c | 164 +++++++++++--------- sound/soc/intel/skylake/cnl-sst.c | 6 +- sound/soc/intel/skylake/skl-messages.c | 3 +- sound/soc/intel/skylake/skl-sst-ipc.c | 152 ++++++++++-------- sound/soc/intel/skylake/skl-sst-ipc.h | 3 +- 8 files changed, 259 insertions(+), 229 deletions(-)
On 2019-07-22 18:02, Pierre-Louis Bossart wrote:
On 7/20/19 2:45 PM, Cezary Rojewski wrote:
Existing IPC framework omits crucial part of the entire communication: reply header. Some IPCs cannot function at all without the access to said header. Update the sst-ipc with new sst_ipc_message structure to represent both request and reply allowing reply-processing handlers to save received responses.
I don't get the idea at all.
It's not "an idea", this is cAVS design. GET_PIPELINE_STATE, LARGE_CONFIG_GET and such define portion of their essential _payload_ within header instead of actual SRAM window. Some of these contain entire _payload_ within header instead - as already stated.
the DSP provides rx_data and a size. If there is a header at the start of the IPC data in some cases, why can't you cast and extract the header when it's needed for your SKL usages? Why does adding a header make the IPC work better?
This is cAVS solution we speak of, not an aDSP one. Not all IPCs have constant size defined here. Moreover, define "size" - from architectural point of view this is *only* size an application is prepared to handle, it's usually overestimated. The actual size of _get_ is known only after reply is received. If it exceeds frame's window, then it gets more complicated..
"If there is a header at the start of the IPC data in some cases, why can't you cast and extract the header (...)" ?? An IPC Header - primary and extension - is found within Host HW registers e.g.: HICPI & HIPCIE what differs from data - payload - which is located in SRAM windows: downlink and uplink - depending on the direction. Saving header does not "make the IPC work better" - it allows them to function in the first place. Again, this is not the case for all IPCs.
Publishing such IPCs (e.g.: _get_large_config found in skl-sst-ipc.c) makes no sense if you do not process them correctly. I know not why dysfunctional code has been upstreamed or why does it not follow cAVS design pattern. All the series being posted currently aim to correct the very fundations of Skylake which are/ were broken, clearly.
I have the feeling this is blurring layers between receiving data and processing it in platform-specific ways, and I am nervous about touching Baytrail and Broadwell legacy, which are way past maintenance mode and should be deprecated really.
I'm not nervous at all. Baytrail IPC is deprecated in favor of /atom/sst - God knows why yet another _common_ folder has been created there.. Haswell with DSP does not really exist. Broadwell is the only one left and my changes only _update_ the framework - these do not change its behavior.
Truth be told, it did not age well at all. 99% of Skylake+ IPC communication is done in synchronized fashion yet during initialization there are always 8 messages allocated (times two: tx & rx). In total we are allocating 8 x 2 x PAGE_SIZE memory. These days memory might not be a problem, though does not change the fact it could have been done better. Lot of wasted memory.. There's certainly more to do, that's the message.
And last this patchset does not apply on top of Mark's tree?
I can recheck this, been a week since I did a snapshot of 5.3. Thanks for heads up.
Despite the range of changes required for model to be updated, no functional changes are made for core hanswell, baytrail and skylake message handlers. Reply-processing handlers now save received response header yet no usage is added by default.
To allow for future changes, righful kings of IPC kingdom need to be put back on the throne. This update addresses one of them: LARGE_CONFIG_GET.
On 7/22/19 1:32 PM, Cezary Rojewski wrote:
On 2019-07-22 18:02, Pierre-Louis Bossart wrote:
On 7/20/19 2:45 PM, Cezary Rojewski wrote:
Existing IPC framework omits crucial part of the entire communication: reply header. Some IPCs cannot function at all without the access to said header. Update the sst-ipc with new sst_ipc_message structure to represent both request and reply allowing reply-processing handlers to save received responses.
I don't get the idea at all.
It's not "an idea", this is cAVS design. GET_PIPELINE_STATE, LARGE_CONFIG_GET and such define portion of their essential _payload_ within header instead of actual SRAM window. Some of these contain entire _payload_ within header instead - as already stated.
No one outside of your team has a clue what cAVS design means, so we'll have to agree on what this really means, see below. seems to me like 'header' refers to a hardware capability, not the data format.
the DSP provides rx_data and a size. If there is a header at the start of the IPC data in some cases, why can't you cast and extract the header when it's needed for your SKL usages? Why does adding a header make the IPC work better?
This is cAVS solution we speak of, not an aDSP one. Not all IPCs have constant size defined here. Moreover, define "size" - from architectural point of view this is *only* size an application is prepared to handle, it's usually overestimated. The actual size of _get_ is known only after reply is received. If it exceeds frame's window, then it gets more complicated..
"If there is a header at the start of the IPC data in some cases, why can't you cast and extract the header (...)" ?? An IPC Header - primary and extension - is found within Host HW registers e.g.: HICPI & HIPCIE what differs from data - payload - which is located in SRAM windows: downlink and uplink - depending on the direction. Saving header does not "make the IPC work better" - it allows them to function in the first place. Again, this is not the case for all IPCs.
The limited understanding I have of the architecture is that the IPC can be done either by transmitting compressed information in an IPC register, using up to 60 bits of information, or a full-blown version in SRAM windows - with the caveat that the SRAM windows may not be available in all power states. All four combinations between TX and RX are possible, e.g. you can transmit a command over the IPC register and get a reply in the SRAM window. I am not aware of a case where the IPC register and SRAM window contain unrelated information, it's either self-contained in the IPC register or the IPC register has a qualifier to help interpret the SRAM window contents. Please correct me here if these explanations are incorrect.
This solution works for SKL+ only, so I am really wondering why you'd want to align legacy platforms with newer stuff. Not to mention that there are two versions of the IPC registers for cAVS.
Publishing such IPCs (e.g.: _get_large_config found in skl-sst-ipc.c) makes no sense if you do not process them correctly. I know not why dysfunctional code has been upstreamed or why does it not follow cAVS design pattern. All the series being posted currently aim to correct the very fundations of Skylake which are/ were broken, clearly.
Since Skylake+ has a fundamentally different way of doing things, and likely more changes required down the road, why not create your own IPC layer when you can knock yourself out, rather than changing stuff that doesn't need any of this?
I have the feeling this is blurring layers between receiving data and processing it in platform-specific ways, and I am nervous about touching Baytrail and Broadwell legacy, which are way past maintenance mode and should be deprecated really.
I'm not nervous at all. Baytrail IPC is deprecated in favor of /atom/sst
- God knows why yet another _common_ folder has been created there..
deprecated doesn't mean removed, there are still users and we don't break their solution.
Haswell with DSP does not really exist. Broadwell is the only one left and my changes only _update_ the framework - these do not change its behavior.
When you change something that impacts multiple platforms, the expectation is that it's been tested on other platforms. Precisely when something is deprecated you either don't touch it or make sure the code changes are tested. And with the difficulty getting access to platforms - we have exactly two Broadwell devices with I2S - I am inclined to avoid any changes.
Truth be told, it did not age well at all. 99% of Skylake+ IPC communication is done in synchronized fashion yet during initialization there are always 8 messages allocated (times two: tx & rx). In total we are allocating 8 x 2 x PAGE_SIZE memory. These days memory might not be a problem, though does not change the fact it could have been done better. Lot of wasted memory..
I don't know what this has to do with this patchset, but it's pretty common to allocate multiple messages to queue them up?
On 2019-07-22 21:05, Pierre-Louis Bossart wrote:
On 7/22/19 1:32 PM, Cezary Rojewski wrote:
On 2019-07-22 18:02, Pierre-Louis Bossart wrote:
On 7/20/19 2:45 PM, Cezary Rojewski wrote:
Existing IPC framework omits crucial part of the entire communication: reply header. Some IPCs cannot function at all without the access to said header. Update the sst-ipc with new sst_ipc_message structure to represent both request and reply allowing reply-processing handlers to save received responses.
I don't get the idea at all.
It's not "an idea", this is cAVS design. GET_PIPELINE_STATE, LARGE_CONFIG_GET and such define portion of their essential _payload_ within header instead of actual SRAM window. Some of these contain entire _payload_ within header instead - as already stated.
No one outside of your team has a clue what cAVS design means, so we'll have to agree on what this really means, see below. seems to me like 'header' refers to a hardware capability, not the data format.
the DSP provides rx_data and a size. If there is a header at the start of the IPC data in some cases, why can't you cast and extract the header when it's needed for your SKL usages? Why does adding a header make the IPC work better?
This is cAVS solution we speak of, not an aDSP one. Not all IPCs have constant size defined here. Moreover, define "size" - from architectural point of view this is *only* size an application is prepared to handle, it's usually overestimated. The actual size of _get_ is known only after reply is received. If it exceeds frame's window, then it gets more complicated..
"If there is a header at the start of the IPC data in some cases, why can't you cast and extract the header (...)" ?? An IPC Header - primary and extension - is found within Host HW registers e.g.: HICPI & HIPCIE what differs from data - payload - which is located in SRAM windows: downlink and uplink - depending on the direction. Saving header does not "make the IPC work better" - it allows them to function in the first place. Again, this is not the case for all IPCs.
The limited understanding I have of the architecture is that the IPC can be done either by transmitting compressed information in an IPC register, using up to 60 bits of information, or a full-blown version in SRAM windows - with the caveat that the SRAM windows may not be available in all power states. All four combinations between TX and RX are possible, e.g. you can transmit a command over the IPC register and get a reply in the SRAM window. I am not aware of a case where the IPC register and SRAM window contain unrelated information, it's either self-contained in the IPC register or the IPC register has a qualifier to help interpret the SRAM window contents. Please correct me here if these explanations are incorrect.
This solution works for SKL+ only, so I am really wondering why you'd want to align legacy platforms with newer stuff. Not to mention that there are two versions of the IPC registers for cAVS.
Two versions of IPC registers is just a mapping thingy, IPC framework does not care about that, leaves that to the caller. Legacy platforms won't notice the difference at all. Status quo is achieved.
If current _get_large_config is not enough for you, see GET_PIPELINE_STATE - first 5 bits of reply.header.extension contain actual data. Now, how do I get that info using current framework capabilities?
About the architecture. SOF's design is different, cAVS (/skylake) always makes use of host registers (IPC header is ALWAYS found there and is never located within SRAM window) with SRAM being optional and context dependent - data that cannot be contained with the very 60 available bits is found there.
Publishing such IPCs (e.g.: _get_large_config found in skl-sst-ipc.c) makes no sense if you do not process them correctly. I know not why dysfunctional code has been upstreamed or why does it not follow cAVS design pattern. All the series being posted currently aim to correct the very fundations of Skylake which are/ were broken, clearly.
Since Skylake+ has a fundamentally different way of doing things, and likely more changes required down the road, why not create your own IPC layer when you can knock yourself out, rather than changing stuff that doesn't need any of this?
Does it really differ that much when it comes to handling IPCs? Me re-implementing - or let's call it what it would really be: copying - IPC framework into /skylake is a lazy option. I prefer doing _simple_ changes to an existing /common stuff and thus reducing burden of maintenance in months to come.
There are fields and methods /skylake does not require too and yet these are there only for the sake of legacy platforms. Long time ago someone decided to make them all share one framework. Do you think splitting these now is a good idea?
I have the feeling this is blurring layers between receiving data and processing it in platform-specific ways, and I am nervous about touching Baytrail and Broadwell legacy, which are way past maintenance mode and should be deprecated really.
I'm not nervous at all. Baytrail IPC is deprecated in favor of /atom/sst - God knows why yet another _common_ folder has been created there..
deprecated doesn't mean removed, there are still users and we don't break their solution.
Sure thing, wasn't my intention - that's why changes maintain status quo.
Haswell with DSP does not really exist. Broadwell is the only one left and my changes only _update_ the framework - these do not change its behavior.
When you change something that impacts multiple platforms, the expectation is that it's been tested on other platforms. Precisely when something is deprecated you either don't touch it or make sure the code changes are tested. And with the difficulty getting access to platforms
- we have exactly two Broadwell devices with I2S - I am inclined to
avoid any changes.
Gathered some legacy platforms, but yeah I was not fortunate yet to test these out. Validating on several different platforms starting from SKL instead.
Here to makes things right, hope we all are. Skylake+ has been dysfunctional for long enough. I'd understand the concern if these were functional changes to legacy platforms. They aren't really. Would like not to delay the resurrection process any longer.
Truth be told, it did not age well at all. 99% of Skylake+ IPC communication is done in synchronized fashion yet during initialization there are always 8 messages allocated (times two: tx & rx). In total we are allocating 8 x 2 x PAGE_SIZE memory. These days memory might not be a problem, though does not change the fact it could have been done better. Lot of wasted memory..
I don't know what this has to do with this patchset, but it's pretty common to allocate multiple messages to queue them up?
On 7/20/19 2:45 PM, Cezary Rojewski wrote:
Existing IPC framework omits crucial part of the entire communication: reply header. Some IPCs cannot function at all without the access to said header. Update the sst-ipc with new sst_ipc_message structure to represent both request and reply allowing reply-processing handlers to save received responses.
Despite the range of changes required for model to be updated, no functional changes are made for core hanswell, baytrail and skylake message handlers. Reply-processing handlers now save received response header yet no usage is added by default.
To allow for future changes, righful kings of IPC kingdom need to be put back on the throne. This update addresses one of them: LARGE_CONFIG_GET.
Cezary Rojewski (5): ASoC: Intel: Update request-reply IPC model
I had a doubt on the structure of this patchset since you first change the structure definitions/prototypes and then use them in follow-up patches, and sure enough if I apply the first patch only the compilation is broken, see below.
The rule is that we can't break git bisect. And if you squash the patches together, then you have a really complicated patch to review/test, so like I said earlier such invasive changes in shared prototypes are really painful.
ASoC: Intel: Haswell: Align with updated request-reply model ASoC: Intel: Baytrail: Align with updated request-reply model ASoC: Intel: Skylake: Align with updated request-reply model ASoC: Intel: Skylake: large_config_get overhaul
sound/soc/intel/baytrail/sst-baytrail-ipc.c | 65 ++++---- sound/soc/intel/common/sst-ipc.c | 68 ++++---- sound/soc/intel/common/sst-ipc.h | 27 ++-- sound/soc/intel/haswell/sst-haswell-ipc.c | 164 +++++++++++--------- sound/soc/intel/skylake/cnl-sst.c | 6 +- sound/soc/intel/skylake/skl-messages.c | 3 +- sound/soc/intel/skylake/skl-sst-ipc.c | 152 ++++++++++-------- sound/soc/intel/skylake/skl-sst-ipc.h | 3 +- 8 files changed, 259 insertions(+), 229 deletions(-)
CC [M] sound/soc/intel/baytrail/sst-baytrail-ipc.o /data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c: In function ‘sst_byt_stream_update’: /data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:214:18: error: ‘struct ipc_message’ has no member named ‘header’ u64 header = msg->header; ^~ /data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c: In function ‘sst_byt_process_reply’: /data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:244:6: error: ‘struct ipc_message’ has no member named ‘rx_size’ msg->rx_size = sst_byt_header_data(header); ^~ /data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:245:35: error: ‘struct ipc_message’ has no member named ‘rx_data’ sst_dsp_inbox_read(byt->dsp, msg->rx_data, msg->rx_size); ^~ /data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:245:49: error: ‘struct ipc_message’ has no member named ‘rx_size’ sst_dsp_inbox_read(byt->dsp, msg->rx_data, msg->rx_size); ^~ /data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c: In function ‘sst_byt_stream_commit’: /data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:418:43: error: incompatible type for argument 2 of ‘sst_ipc_tx_message_wait’ ret = sst_ipc_tx_message_wait(&byt->ipc, header, str_req, ^~~~~~ In file included from /data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:25: /data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/../common/sst-ipc.h:70:25: note: expected ‘struct sst_ipc_message’ but argument is of type ‘u64’ {aka ‘long long unsigned int’} struct sst_ipc_message request, struct sst_ipc_message *reply); ~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~ /data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:418:51: error: passing argument 3 of ‘sst_ipc_tx_message_wait’ from incompatible pointer type [-Werror=incompatible-pointer-types] ret = sst_ipc_tx_message_wait(&byt->ipc, header, str_req, ^~~~~~~ In file included from /data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:25: /data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/../common/sst-ipc.h:70:58: note: expected ‘struct sst_ipc_message *’ but argument is of type ‘struct sst_byt_alloc_params *’ struct sst_ipc_message request, struct sst_ipc_message *reply); ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~ /data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:418:8: error: too many arguments to function ‘sst_ipc_tx_message_wait’ ret = sst_ipc_tx_message_wait(&byt->ipc, header, str_req, ^~~~~~~~~~~~~~~~~~~~~~~ In file included from /data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:25: /data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/../common/sst-ipc.h:69:5: note: declared here int sst_ipc_tx_message_wait(struct sst_generic_ipc *ipc, ^~~~~~~~~~~~~~~~~~~~~~~ /data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c: In function ‘sst_byt_stream_free’: /data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:442:43: error: incompatible type for argument 2 of ‘sst_ipc_tx_message_wait’ ret = sst_ipc_tx_message_wait(&byt->ipc, header, NULL, 0, NULL, 0); ^~~~~~ In file included from /data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:25: /data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/../common/sst-ipc.h:70:25: note: expected ‘struct sst_ipc_message’ but argument is of type ‘u64’ {aka ‘long long unsigned int’} struct sst_ipc_message request, struct sst_ipc_message *reply); ~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~ /data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:442:8: error: too many arguments to function ‘sst_ipc_tx_message_wait’ ret = sst_ipc_tx_message_wait(&byt->ipc, header, NULL, 0, NULL, 0); ^~~~~~~~~~~~~~~~~~~~~~~ In file included from /data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:25: /data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/../common/sst-ipc.h:69:5: note: declared here int sst_ipc_tx_message_wait(struct sst_generic_ipc *ipc, ^~~~~~~~~~~~~~~~~~~~~~~ /data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c: In function ‘sst_byt_stream_operations’: /data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:466:45: error: incompatible type for argument 2 of ‘sst_ipc_tx_message_wait’ return sst_ipc_tx_message_wait(&byt->ipc, header, NULL, ^~~~~~ In file included from /data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:25: /data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/../common/sst-ipc.h:70:25: note: expected ‘struct sst_ipc_message’ but argument is of type ‘u64’ {aka ‘long long unsigned int’} struct sst_ipc_message request, struct sst_ipc_message *reply); ~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~ /data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:466:10: error: too many arguments to function ‘sst_ipc_tx_message_wait’ return sst_ipc_tx_message_wait(&byt->ipc, header, NULL, ^~~~~~~~~~~~~~~~~~~~~~~ In file included from /data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:25: /data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/../common/sst-ipc.h:69:5: note: declared here int sst_ipc_tx_message_wait(struct sst_generic_ipc *ipc, ^~~~~~~~~~~~~~~~~~~~~~~ /data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:469:47: error: incompatible type for argument 2 of ‘sst_ipc_tx_message_nowait’ return sst_ipc_tx_message_nowait(&byt->ipc, header, ^~~~~~ In file included from /data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:25: /data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/../common/sst-ipc.h:73:25: note: expected ‘struct sst_ipc_message’ but argument is of type ‘u64’ {aka ‘long long unsigned int’} struct sst_ipc_message request); ~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~ /data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:469:10: error: too many arguments to function ‘sst_ipc_tx_message_nowait’ return sst_ipc_tx_message_nowait(&byt->ipc, header, ^~~~~~~~~~~~~~~~~~~~~~~~~ In file included from /data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:25: /data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/../common/sst-ipc.h:72:5: note: declared here int sst_ipc_tx_message_nowait(struct sst_generic_ipc *ipc, ^~~~~~~~~~~~~~~~~~~~~~~~~ /data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c: In function ‘sst_byt_stream_start’: /data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:490:45: error: incompatible type for argument 2 of ‘sst_ipc_tx_message_nowait’ ret = sst_ipc_tx_message_nowait(&byt->ipc, header, tx_msg, size); ^~~~~~ In file included from /data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:25: /data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/../common/sst-ipc.h:73:25: note: expected ‘struct sst_ipc_message’ but argument is of type ‘u64’ {aka ‘long long unsigned int’} struct sst_ipc_message request); ~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~ /data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:490:8: error: too many arguments to function ‘sst_ipc_tx_message_nowait’ ret = sst_ipc_tx_message_nowait(&byt->ipc, header, tx_msg, size); ^~~~~~~~~~~~~~~~~~~~~~~~~ In file included from /data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:25: /data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/../common/sst-ipc.h:72:5: note: declared here int sst_ipc_tx_message_nowait(struct sst_generic_ipc *ipc, ^~~~~~~~~~~~~~~~~~~~~~~~~ /data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c: In function ‘byt_tx_msg’: /data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:626:9: error: ‘struct ipc_message’ has no member named ‘header’ if (msg->header & IPC_HEADER_LARGE(true)) ^~ /data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:627:37: error: ‘struct ipc_message’ has no member named ‘tx_data’ sst_dsp_outbox_write(ipc->dsp, msg->tx_data, msg->tx_size); ^~ /data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:627:51: error: ‘struct ipc_message’ has no member named ‘tx_size’ sst_dsp_outbox_write(ipc->dsp, msg->tx_data, msg->tx_size); ^~ /data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:629:55: error: ‘struct ipc_message’ has no member named ‘header’ sst_dsp_shim_write64_unlocked(ipc->dsp, SST_IPCX, msg->header); ^~ /data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c: In function ‘byt_tx_data_copy’: /data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:651:13: error: ‘struct ipc_message’ has no member named ‘tx_data’ *(u32 *)msg->tx_data = (u32)(msg->header & (u32)-1); ^~ /data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:651:34: error: ‘struct ipc_message’ has no member named ‘header’ *(u32 *)msg->tx_data = (u32)(msg->header & (u32)-1); ^~ /data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:652:12: error: ‘struct ipc_message’ has no member named ‘tx_data’ memcpy(msg->tx_data + sizeof(u32), tx_data, tx_size); ^~ /data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:653:5: error: ‘struct ipc_message’ has no member named ‘tx_size’ msg->tx_size += sizeof(u32); ^~ /data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c: In function ‘sst_byt_stream_operations’: /data/pbossart/ktest/broonie-next/sound/soc/intel/baytrail/sst-baytrail-ipc.c:471:1: warning: control reaches end of non-void function [-Wreturn-type] } ^ cc1: some warnings being treated as errors
On 2019-07-22 23:12, Pierre-Louis Bossart wrote:
On 7/20/19 2:45 PM, Cezary Rojewski wrote:
Existing IPC framework omits crucial part of the entire communication: reply header. Some IPCs cannot function at all without the access to said header. Update the sst-ipc with new sst_ipc_message structure to represent both request and reply allowing reply-processing handlers to save received responses.
Despite the range of changes required for model to be updated, no functional changes are made for core hanswell, baytrail and skylake message handlers. Reply-processing handlers now save received response header yet no usage is added by default.
To allow for future changes, righful kings of IPC kingdom need to be put back on the throne. This update addresses one of them: LARGE_CONFIG_GET.
Cezary Rojewski (5): ASoC: Intel: Update request-reply IPC model
I had a doubt on the structure of this patchset since you first change the structure definitions/prototypes and then use them in follow-up patches, and sure enough if I apply the first patch only the compilation is broken, see below.
The rule is that we can't break git bisect. And if you squash the patches together, then you have a really complicated patch to review/test, so like I said earlier such invasive changes in shared prototypes are really painful.
Thanks for your time and input, Pierre!
Agreed on the patchset structure. This wasn't a random mistake, though. Knew that meshing them all together immediately (v1) would be very hard for readers to review, despite the _simplicity_ of actual solution: explicit listing of message parts -> containment within sst_ipc_message.
I'll combine them together - except for the large_config_get one. Had these issues been addressed earlier, patches such as this wouldn't have been needed at all ;/
Czarek
On Mon, Jul 22, 2019 at 04:12:19PM -0500, Pierre-Louis Bossart wrote:
The rule is that we can't break git bisect. And if you squash the patches together, then you have a really complicated patch to review/test, so like I said earlier such invasive changes in shared prototypes are really painful.
Yeah, I build test each commit as they get applied (though not everything gets covered so things can get missed) and it's generally helpful for people doing things like bisection.
participants (3)
-
Cezary Rojewski -
Mark Brown -
Pierre-Louis Bossart