-
Notifications
You must be signed in to change notification settings - Fork 350
ipc4: notification: Add filtering feature #10552
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,7 +26,6 @@ | |
| #include <zephyr/pm/policy.h> | ||
| #include <rtos/init.h> | ||
| #if CONFIG_XRUN_NOTIFICATIONS_ENABLE | ||
| #include <sof/ipc/notification_pool.h> | ||
| #include <ipc4/notification.h> | ||
| #endif | ||
|
|
||
|
|
@@ -150,19 +149,14 @@ static int chain_get_dma_status(struct chain_dma_data *cd, struct dma_chan_data | |
| int ret = dma_get_status(chan->dma->z_dev, chan->index, stat); | ||
| #if CONFIG_XRUN_NOTIFICATIONS_ENABLE | ||
| if (ret == -EPIPE && !cd->xrun_notification_sent) { | ||
| struct ipc_msg *notify = ipc_notification_pool_get(IPC4_RESOURCE_EVENT_SIZE); | ||
|
|
||
| if (notify) { | ||
| if (cd->stream_direction == SOF_IPC_STREAM_PLAYBACK) | ||
| gateway_underrun_notif_msg_init(notify, | ||
| cd->link_connector_node_id.dw); | ||
| else | ||
| gateway_overrun_notif_msg_init(notify, | ||
| cd->link_connector_node_id.dw); | ||
|
|
||
| ipc_msg_send(notify, notify->tx_data, false); | ||
| cd->xrun_notification_sent = true; | ||
| } | ||
| uint32_t node_id = cd->link_connector_node_id.dw; | ||
| bool notif_sent = false; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. superfluous initialisation |
||
|
|
||
| if (cd->stream_direction == SOF_IPC_STREAM_PLAYBACK) | ||
| notif_sent = send_gateway_underrun_notif_msg(node_id); | ||
| else | ||
| notif_sent = send_gateway_overrun_notif_msg(node_id); | ||
| cd->xrun_notification_sent = notif_sent; | ||
| } else if (!ret) { | ||
| cd->xrun_notification_sent = false; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,7 +37,6 @@ | |
| #include <stdint.h> | ||
|
|
||
| #if CONFIG_XRUN_NOTIFICATIONS_ENABLE | ||
| #include <sof/ipc/notification_pool.h> | ||
| #include <ipc4/notification.h> | ||
| #endif | ||
|
|
||
|
|
@@ -1478,19 +1477,14 @@ static int dai_get_status(struct comp_dev *dev, struct dai_data *dd, struct dma_ | |
| int ret = sof_dma_get_status(dd->chan->dma, dd->chan->index, stat); | ||
| #if CONFIG_XRUN_NOTIFICATIONS_ENABLE | ||
| if (ret == -EPIPE && !dd->xrun_notification_sent) { | ||
| struct ipc_msg *notify = ipc_notification_pool_get(IPC4_RESOURCE_EVENT_SIZE); | ||
| uint32_t ppl_id = dev->pipeline->pipeline_id; | ||
| bool notif_sent = false; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto |
||
|
|
||
| if (notify) { | ||
| if (dev->direction == SOF_IPC_STREAM_PLAYBACK) | ||
| copier_gateway_underrun_notif_msg_init(notify, | ||
| dev->pipeline->pipeline_id); | ||
| else | ||
| copier_gateway_overrun_notif_msg_init(notify, | ||
| dev->pipeline->pipeline_id); | ||
|
|
||
| ipc_msg_send(notify, notify->tx_data, false); | ||
| dd->xrun_notification_sent = true; | ||
| } | ||
| if (dev->direction == SOF_IPC_STREAM_PLAYBACK) | ||
| notif_sent = send_copier_gateway_underrun_notif_msg(ppl_id); | ||
| else | ||
| notif_sent = send_copier_gateway_overrun_notif_msg(ppl_id); | ||
| dd->xrun_notification_sent = notif_sent; | ||
| } else if (!ret) { | ||
| dd->xrun_notification_sent = false; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,7 +33,6 @@ | |
| #include <stdint.h> | ||
|
|
||
| #if CONFIG_XRUN_NOTIFICATIONS_ENABLE | ||
| #include <sof/ipc/notification_pool.h> | ||
| #include <ipc4/notification.h> | ||
| #endif | ||
|
|
||
|
|
@@ -368,19 +367,14 @@ static int host_get_status(struct comp_dev *dev, struct host_data *hd, struct dm | |
| int ret = sof_dma_get_status(hd->chan->dma, hd->chan->index, stat); | ||
| #if CONFIG_XRUN_NOTIFICATIONS_ENABLE | ||
| if (ret == -EPIPE && !hd->xrun_notification_sent) { | ||
| struct ipc_msg *notify = ipc_notification_pool_get(IPC4_RESOURCE_EVENT_SIZE); | ||
|
|
||
| if (notify) { | ||
| if (dev->direction == SOF_IPC_STREAM_PLAYBACK) | ||
| copier_gateway_underrun_notif_msg_init(notify, | ||
| dev->pipeline->pipeline_id); | ||
| else | ||
| copier_gateway_overrun_notif_msg_init(notify, | ||
| dev->pipeline->pipeline_id); | ||
|
|
||
| ipc_msg_send(notify, notify->tx_data, false); | ||
| hd->xrun_notification_sent = true; | ||
| } | ||
| uint32_t ppl_id = dev->pipeline->pipeline_id; | ||
| bool notif_sent = false; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same here |
||
|
|
||
| if (dev->direction == SOF_IPC_STREAM_PLAYBACK) | ||
| notif_sent = send_copier_gateway_underrun_notif_msg(ppl_id); | ||
| else | ||
| notif_sent = send_copier_gateway_overrun_notif_msg(ppl_id); | ||
| hd->xrun_notification_sent = notif_sent; | ||
| } else if (!ret) { | ||
| hd->xrun_notification_sent = false; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -288,15 +288,15 @@ struct ipc4_resource_event_data_notification { | |
|
|
||
| #define IPC4_RESOURCE_EVENT_SIZE sizeof(struct ipc4_resource_event_data_notification) | ||
|
|
||
| void process_data_error_notif_msg_init(struct ipc_msg *msg, uint32_t resource_id, | ||
| uint32_t error_code); | ||
| void send_process_data_error_notif_msg(uint32_t resource_id, uint32_t error_code); | ||
|
|
||
| void copier_gateway_underrun_notif_msg_init(struct ipc_msg *msg, uint32_t pipeline_id); | ||
| void copier_gateway_overrun_notif_msg_init(struct ipc_msg *msg, uint32_t pipeline_id); | ||
| void gateway_underrun_notif_msg_init(struct ipc_msg *msg, uint32_t resource_id); | ||
| void gateway_overrun_notif_msg_init(struct ipc_msg *msg, uint32_t resource_id); | ||
| bool send_copier_gateway_underrun_notif_msg(uint32_t pipeline_id); | ||
| bool send_copier_gateway_overrun_notif_msg(uint32_t pipeline_id); | ||
| bool send_gateway_underrun_notif_msg(uint32_t resource_id); | ||
| bool send_gateway_overrun_notif_msg(uint32_t resource_id); | ||
|
|
||
| void mixer_underrun_notif_msg_init(struct ipc_msg *msg, uint32_t resource_id, uint32_t eos_flag, | ||
| uint32_t data_mixed, uint32_t expected_data_mixed); | ||
| void send_mixer_underrun_notif_msg(uint32_t resource_id, uint32_t eos_flag, uint32_t data_mixed, | ||
| uint32_t expected_data_mixed); | ||
|
Comment on lines
-291
to
+299
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: A consistent interface would probably be better, either we return true when notification has been sent or we return nothing. |
||
| void ipc4_update_notification_mask(uint32_t ntfy_mask, uint32_t enabled_mask); | ||
|
|
||
| #endif /* __IPC4_NOTIFICATION_H__ */ | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,85 +10,116 @@ | |
| #include <sof/ipc/msg.h> | ||
| #include <stdbool.h> | ||
| #include <ipc4/notification.h> | ||
| #include <sof/ipc/notification_pool.h> | ||
|
|
||
| #include <rtos/symbol.h> | ||
|
|
||
| static void resource_notif_header_init(struct ipc_msg *msg) | ||
| static uint32_t notification_mask = 0xFFFFFFFF; | ||
|
|
||
| static bool is_notif_filtered_out(uint32_t event_type) | ||
| { | ||
| struct ipc4_resource_event_data_notification *notif_data = msg->tx_data; | ||
| uint32_t notif_idx; | ||
|
|
||
| switch (event_type) { | ||
| case SOF_IPC4_GATEWAY_UNDERRUN_DETECTED: | ||
| notif_idx = IPC4_UNDERRUN_AT_GATEWAY_NOTIFICATION_MASK_IDX; | ||
| break; | ||
| case SOF_IPC4_MIXER_UNDERRUN_DETECTED: | ||
| notif_idx = IPC4_UNDERRUN_AT_MIXER_NOTIFICATION_MASK_IDX; | ||
| break; | ||
| case SOF_IPC4_GATEWAY_OVERRUN_DETECTED: | ||
| notif_idx = IPC4_OVERRUN_AT_GATEWAY_NOTIFICATION_MASK_IDX; | ||
| break; | ||
| default: | ||
| return false; | ||
| } | ||
|
|
||
| return (notification_mask & BIT(notif_idx)) == 0; | ||
| } | ||
|
|
||
| static bool send_resource_notif(uint32_t resource_id, uint32_t event_type, uint32_t resource_type, | ||
| void *data, uint32_t data_size) | ||
| { | ||
| struct ipc_msg *msg; | ||
|
|
||
| if (is_notif_filtered_out(event_type)) | ||
| return true; //silently ignore | ||
|
|
||
| msg = ipc_notification_pool_get(IPC4_RESOURCE_EVENT_SIZE); | ||
| if (!msg) | ||
| return false; | ||
|
|
||
| struct ipc4_resource_event_data_notification *notif = msg->tx_data; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I assume this cannot be
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, the payload is allocated along with the msg as a single rzalloc operation. |
||
| union ipc4_notification_header header; | ||
|
|
||
| header.r.notif_type = SOF_IPC4_NOTIFY_RESOURCE_EVENT; | ||
| header.r.type = SOF_IPC4_GLB_NOTIFICATION; | ||
| header.r.rsp = SOF_IPC4_MESSAGE_DIR_MSG_REQUEST; | ||
| header.r.msg_tgt = SOF_IPC4_MESSAGE_TARGET_FW_GEN_MSG; | ||
| msg->header = header.dat; | ||
| memset(¬if_data->event_data, 0, sizeof(notif_data->event_data)); | ||
| } | ||
|
|
||
| void copier_gateway_underrun_notif_msg_init(struct ipc_msg *msg, uint32_t pipeline_id) | ||
| { | ||
| struct ipc4_resource_event_data_notification *notif_data = msg->tx_data; | ||
| notif->resource_id = resource_id; | ||
| notif->event_type = event_type; | ||
| notif->resource_type = resource_type; | ||
| memset(¬if->event_data, 0, sizeof(notif->event_data)); | ||
| if (data && data_size) | ||
| memcpy_s(¬if->event_data, sizeof(notif->event_data), data, data_size); | ||
|
|
||
| resource_notif_header_init(msg); | ||
| notif_data->resource_id = pipeline_id; | ||
| notif_data->event_type = SOF_IPC4_GATEWAY_UNDERRUN_DETECTED; | ||
| notif_data->resource_type = SOF_IPC4_PIPELINE; | ||
| ipc_msg_send(msg, msg->tx_data, false); | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| void gateway_underrun_notif_msg_init(struct ipc_msg *msg, uint32_t resource_id) | ||
| void ipc4_update_notification_mask(uint32_t ntfy_mask, uint32_t enabled_mask) | ||
| { | ||
| struct ipc4_resource_event_data_notification *notif_data = msg->tx_data; | ||
|
|
||
| resource_notif_header_init(msg); | ||
| notif_data->resource_id = resource_id; | ||
| notif_data->event_type = SOF_IPC4_GATEWAY_UNDERRUN_DETECTED; | ||
| notif_data->resource_type = SOF_IPC4_GATEWAY; | ||
| notification_mask &= enabled_mask | (~ntfy_mask); | ||
| notification_mask |= enabled_mask & ntfy_mask; | ||
| } | ||
|
|
||
| void copier_gateway_overrun_notif_msg_init(struct ipc_msg *msg, uint32_t pipeline_id) | ||
| bool send_copier_gateway_underrun_notif_msg(uint32_t pipeline_id) | ||
| { | ||
| struct ipc4_resource_event_data_notification *notif_data = msg->tx_data; | ||
| return send_resource_notif(pipeline_id, SOF_IPC4_GATEWAY_UNDERRUN_DETECTED, | ||
| SOF_IPC4_PIPELINE, NULL, 0); | ||
| } | ||
|
|
||
| resource_notif_header_init(msg); | ||
| notif_data->resource_id = pipeline_id; | ||
| notif_data->event_type = SOF_IPC4_GATEWAY_OVERRUN_DETECTED; | ||
| notif_data->resource_type = SOF_IPC4_PIPELINE; | ||
| bool send_gateway_underrun_notif_msg(uint32_t resource_id) | ||
| { | ||
| return send_resource_notif(resource_id, SOF_IPC4_GATEWAY_UNDERRUN_DETECTED, | ||
| SOF_IPC4_GATEWAY, NULL, 0); | ||
| } | ||
|
|
||
| void gateway_overrun_notif_msg_init(struct ipc_msg *msg, uint32_t resource_id) | ||
| bool send_copier_gateway_overrun_notif_msg(uint32_t pipeline_id) | ||
| { | ||
| struct ipc4_resource_event_data_notification *notif_data = msg->tx_data; | ||
| return send_resource_notif(pipeline_id, SOF_IPC4_GATEWAY_OVERRUN_DETECTED, | ||
| SOF_IPC4_PIPELINE, NULL, 0); | ||
| } | ||
|
|
||
| resource_notif_header_init(msg); | ||
| notif_data->resource_id = resource_id; | ||
| notif_data->event_type = SOF_IPC4_GATEWAY_OVERRUN_DETECTED; | ||
| notif_data->resource_type = SOF_IPC4_GATEWAY; | ||
| bool send_gateway_overrun_notif_msg(uint32_t resource_id) | ||
| { | ||
| return send_resource_notif(resource_id, SOF_IPC4_GATEWAY_OVERRUN_DETECTED, | ||
| SOF_IPC4_GATEWAY, NULL, 0); | ||
| } | ||
|
|
||
| void mixer_underrun_notif_msg_init(struct ipc_msg *msg, uint32_t resource_id, uint32_t eos_flag, | ||
| uint32_t data_mixed, uint32_t expected_data_mixed) | ||
| void send_mixer_underrun_notif_msg(uint32_t resource_id, uint32_t eos_flag, uint32_t data_mixed, | ||
| uint32_t expected_data_mixed) | ||
| { | ||
| struct ipc4_resource_event_data_notification *notif_data = msg->tx_data; | ||
|
|
||
| resource_notif_header_init(msg); | ||
| notif_data->resource_id = resource_id; | ||
| notif_data->event_type = SOF_IPC4_MIXER_UNDERRUN_DETECTED; | ||
| notif_data->resource_type = SOF_IPC4_PIPELINE; | ||
| notif_data->event_data.mixer_underrun.eos_flag = eos_flag; | ||
| notif_data->event_data.mixer_underrun.data_mixed = data_mixed; | ||
| notif_data->event_data.mixer_underrun.expected_data_mixed = expected_data_mixed; | ||
| struct ipc4_mixer_underrun_event_data mixer_underrun_data; | ||
|
|
||
| mixer_underrun_data.eos_flag = eos_flag; | ||
| mixer_underrun_data.data_mixed = data_mixed; | ||
| mixer_underrun_data.expected_data_mixed = expected_data_mixed; | ||
|
|
||
| send_resource_notif(resource_id, SOF_IPC4_MIXER_UNDERRUN_DETECTED, SOF_IPC4_PIPELINE, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. how about we only export
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are pros and cons. Function send_resource_notif takes numerous arguments. If we keep it static, we give the compile a chance to optimize them. On the other hand, your idea would allow us to move the "if (cd->stream_direction)" condition into a single static function that determines the right arguments to send_resource_notif instead of having to choose the right function based on the condition. Its a tough call and I didn't think about this. My focus was to allow efficient implantation of the notification mask without making the existing code too cumbersome.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also some middle ground approach is possible. For example we can merge send_gateway_underrun_notif_msg and send_gateway_overrun_notif_msg into a single function send_gateway_xrun_notif_msg but still keep it in the notification module. This approach would eliminate the repetition of the following code in dai-zephyr.c and host-zephyr.c |
||
| &mixer_underrun_data, sizeof(mixer_underrun_data)); | ||
| } | ||
| EXPORT_SYMBOL(mixer_underrun_notif_msg_init); | ||
| EXPORT_SYMBOL(send_mixer_underrun_notif_msg); | ||
|
|
||
| void process_data_error_notif_msg_init(struct ipc_msg *msg, uint32_t resource_id, | ||
| uint32_t error_code) | ||
| void send_process_data_error_notif_msg(uint32_t resource_id, uint32_t error_code) | ||
| { | ||
| struct ipc4_resource_event_data_notification *notif_data = msg->tx_data; | ||
| struct ipc4_process_data_error_event_data error_data; | ||
|
|
||
| error_data.error_code = error_code; | ||
|
|
||
| resource_notif_header_init(msg); | ||
| notif_data->resource_id = resource_id; | ||
| notif_data->event_type = SOF_IPC4_PROCESS_DATA_ERROR; | ||
| notif_data->resource_type = SOF_IPC4_MODULE_INSTANCE; | ||
| notif_data->event_data.process_data_error.error_code = error_code; | ||
| send_resource_notif(resource_id, SOF_IPC4_PROCESS_DATA_ERROR, SOF_IPC4_MODULE_INSTANCE, | ||
| &error_data, sizeof(error_data)); | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could make the argument
const void *dataand just initialisestruct ipc4_notification_mask_info *mask_info = data;directly hereThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are right. I just used a similar function as a template ;)