* [PATCH 0/5] ASoC: Intel: IPC framework updates
@ 2019-07-20 19:45 Cezary Rojewski
2019-07-20 19:45 ` [PATCH 1/5] ASoC: Intel: Update request-reply IPC model Cezary Rojewski
` (6 more replies)
0 siblings, 7 replies; 13+ messages in thread
From: Cezary Rojewski @ 2019-07-20 19:45 UTC (permalink / raw
To: alsa-devel
Cc: lgirdwood, Cezary Rojewski, broonie, tiwai, pierre-louis.bossart
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(-)
--
2.17.1
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/5] ASoC: Intel: Update request-reply IPC model
2019-07-20 19:45 [PATCH 0/5] ASoC: Intel: IPC framework updates Cezary Rojewski
@ 2019-07-20 19:45 ` Cezary Rojewski
2019-07-20 19:45 ` [PATCH 2/5] ASoC: Intel: Haswell: Align with updated request-reply model Cezary Rojewski
` (5 subsequent siblings)
6 siblings, 0 replies; 13+ messages in thread
From: Cezary Rojewski @ 2019-07-20 19:45 UTC (permalink / raw
To: alsa-devel
Cc: lgirdwood, Cezary Rojewski, broonie, tiwai, pierre-louis.bossart
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);
--
2.17.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/5] ASoC: Intel: Haswell: Align with updated request-reply model
2019-07-20 19:45 [PATCH 0/5] ASoC: Intel: IPC framework updates Cezary Rojewski
2019-07-20 19:45 ` [PATCH 1/5] ASoC: Intel: Update request-reply IPC model Cezary Rojewski
@ 2019-07-20 19:45 ` Cezary Rojewski
2019-07-20 19:45 ` [PATCH 3/5] ASoC: Intel: Baytrail: " Cezary Rojewski
` (4 subsequent siblings)
6 siblings, 0 replies; 13+ messages in thread
From: Cezary Rojewski @ 2019-07-20 19:45 UTC (permalink / raw
To: alsa-devel
Cc: lgirdwood, Cezary Rojewski, broonie, tiwai, pierre-louis.bossart
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)
--
2.17.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 3/5] ASoC: Intel: Baytrail: Align with updated request-reply model
2019-07-20 19:45 [PATCH 0/5] ASoC: Intel: IPC framework updates Cezary Rojewski
2019-07-20 19:45 ` [PATCH 1/5] ASoC: Intel: Update request-reply IPC model Cezary Rojewski
2019-07-20 19:45 ` [PATCH 2/5] ASoC: Intel: Haswell: Align with updated request-reply model Cezary Rojewski
@ 2019-07-20 19:45 ` Cezary Rojewski
2019-07-20 19:45 ` [PATCH 4/5] ASoC: Intel: Skylake: " Cezary Rojewski
` (3 subsequent siblings)
6 siblings, 0 replies; 13+ messages in thread
From: Cezary Rojewski @ 2019-07-20 19:45 UTC (permalink / raw
To: alsa-devel
Cc: lgirdwood, Cezary Rojewski, broonie, tiwai, pierre-louis.bossart
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)
--
2.17.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 4/5] ASoC: Intel: Skylake: Align with updated request-reply model
2019-07-20 19:45 [PATCH 0/5] ASoC: Intel: IPC framework updates Cezary Rojewski
` (2 preceding siblings ...)
2019-07-20 19:45 ` [PATCH 3/5] ASoC: Intel: Baytrail: " Cezary Rojewski
@ 2019-07-20 19:45 ` Cezary Rojewski
2019-07-20 19:45 ` [PATCH 5/5] ASoC: Intel: Skylake: large_config_get overhaul Cezary Rojewski
` (2 subsequent siblings)
6 siblings, 0 replies; 13+ messages in thread
From: Cezary Rojewski @ 2019-07-20 19:45 UTC (permalink / raw
To: alsa-devel
Cc: lgirdwood, Cezary Rojewski, broonie, tiwai, pierre-louis.bossart
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);
--
2.17.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 5/5] ASoC: Intel: Skylake: large_config_get overhaul
2019-07-20 19:45 [PATCH 0/5] ASoC: Intel: IPC framework updates Cezary Rojewski
` (3 preceding siblings ...)
2019-07-20 19:45 ` [PATCH 4/5] ASoC: Intel: Skylake: " Cezary Rojewski
@ 2019-07-20 19:45 ` Cezary Rojewski
2019-07-22 16:02 ` [PATCH 0/5] ASoC: Intel: IPC framework updates Pierre-Louis Bossart
2019-07-22 21:12 ` Pierre-Louis Bossart
6 siblings, 0 replies; 13+ messages in thread
From: Cezary Rojewski @ 2019-07-20 19:45 UTC (permalink / raw
To: alsa-devel
Cc: lgirdwood, Cezary Rojewski, broonie, tiwai, pierre-louis.bossart
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);
--
2.17.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 0/5] ASoC: Intel: IPC framework updates
2019-07-20 19:45 [PATCH 0/5] ASoC: Intel: IPC framework updates Cezary Rojewski
` (4 preceding siblings ...)
2019-07-20 19:45 ` [PATCH 5/5] ASoC: Intel: Skylake: large_config_get overhaul Cezary Rojewski
@ 2019-07-22 16:02 ` Pierre-Louis Bossart
2019-07-22 18:32 ` Cezary Rojewski
2019-07-22 21:12 ` Pierre-Louis Bossart
6 siblings, 1 reply; 13+ messages in thread
From: Pierre-Louis Bossart @ 2019-07-22 16:02 UTC (permalink / raw
To: Cezary Rojewski, alsa-devel; +Cc: broonie, tiwai, lgirdwood
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(-)
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/5] ASoC: Intel: IPC framework updates
2019-07-22 16:02 ` [PATCH 0/5] ASoC: Intel: IPC framework updates Pierre-Louis Bossart
@ 2019-07-22 18:32 ` Cezary Rojewski
2019-07-22 19:05 ` Pierre-Louis Bossart
0 siblings, 1 reply; 13+ messages in thread
From: Cezary Rojewski @ 2019-07-22 18:32 UTC (permalink / raw
To: Pierre-Louis Bossart; +Cc: alsa-devel, broonie, tiwai, lgirdwood
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/5] ASoC: Intel: IPC framework updates
2019-07-22 18:32 ` Cezary Rojewski
@ 2019-07-22 19:05 ` Pierre-Louis Bossart
2019-07-22 20:21 ` Cezary Rojewski
0 siblings, 1 reply; 13+ messages in thread
From: Pierre-Louis Bossart @ 2019-07-22 19:05 UTC (permalink / raw
To: Cezary Rojewski; +Cc: alsa-devel, broonie, tiwai, lgirdwood
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?
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/5] ASoC: Intel: IPC framework updates
2019-07-22 19:05 ` Pierre-Louis Bossart
@ 2019-07-22 20:21 ` Cezary Rojewski
0 siblings, 0 replies; 13+ messages in thread
From: Cezary Rojewski @ 2019-07-22 20:21 UTC (permalink / raw
To: Pierre-Louis Bossart; +Cc: alsa-devel, broonie, tiwai, lgirdwood
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?
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/5] ASoC: Intel: IPC framework updates
2019-07-20 19:45 [PATCH 0/5] ASoC: Intel: IPC framework updates Cezary Rojewski
` (5 preceding siblings ...)
2019-07-22 16:02 ` [PATCH 0/5] ASoC: Intel: IPC framework updates Pierre-Louis Bossart
@ 2019-07-22 21:12 ` Pierre-Louis Bossart
2019-07-22 21:37 ` Cezary Rojewski
2019-07-23 16:59 ` Mark Brown
6 siblings, 2 replies; 13+ messages in thread
From: Pierre-Louis Bossart @ 2019-07-22 21:12 UTC (permalink / raw
To: Cezary Rojewski, alsa-devel; +Cc: broonie, lgirdwood, tiwai
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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/5] ASoC: Intel: IPC framework updates
2019-07-22 21:12 ` Pierre-Louis Bossart
@ 2019-07-22 21:37 ` Cezary Rojewski
2019-07-23 16:59 ` Mark Brown
1 sibling, 0 replies; 13+ messages in thread
From: Cezary Rojewski @ 2019-07-22 21:37 UTC (permalink / raw
To: Pierre-Louis Bossart, alsa-devel; +Cc: broonie, lgirdwood, tiwai
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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/5] ASoC: Intel: IPC framework updates
2019-07-22 21:12 ` Pierre-Louis Bossart
2019-07-22 21:37 ` Cezary Rojewski
@ 2019-07-23 16:59 ` Mark Brown
1 sibling, 0 replies; 13+ messages in thread
From: Mark Brown @ 2019-07-23 16:59 UTC (permalink / raw
To: Pierre-Louis Bossart; +Cc: Cezary Rojewski, tiwai, alsa-devel, lgirdwood
[-- Attachment #1.1: Type: text/plain, Size: 485 bytes --]
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.
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2019-07-23 16:59 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-07-20 19:45 [PATCH 0/5] ASoC: Intel: IPC framework updates Cezary Rojewski
2019-07-20 19:45 ` [PATCH 1/5] ASoC: Intel: Update request-reply IPC model Cezary Rojewski
2019-07-20 19:45 ` [PATCH 2/5] ASoC: Intel: Haswell: Align with updated request-reply model Cezary Rojewski
2019-07-20 19:45 ` [PATCH 3/5] ASoC: Intel: Baytrail: " Cezary Rojewski
2019-07-20 19:45 ` [PATCH 4/5] ASoC: Intel: Skylake: " Cezary Rojewski
2019-07-20 19:45 ` [PATCH 5/5] ASoC: Intel: Skylake: large_config_get overhaul Cezary Rojewski
2019-07-22 16:02 ` [PATCH 0/5] ASoC: Intel: IPC framework updates Pierre-Louis Bossart
2019-07-22 18:32 ` Cezary Rojewski
2019-07-22 19:05 ` Pierre-Louis Bossart
2019-07-22 20:21 ` Cezary Rojewski
2019-07-22 21:12 ` Pierre-Louis Bossart
2019-07-22 21:37 ` Cezary Rojewski
2019-07-23 16:59 ` Mark Brown
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.