Skip to content

ams: add support for ams to kd and kpb modules - #7158

Merged
kv2019i merged 5 commits into
thesofproject:mainfrom
iganakov:prv-kd-ams
May 24, 2023
Merged

ams: add support for ams to kd and kpb modules#7158
kv2019i merged 5 commits into
thesofproject:mainfrom
iganakov:prv-kd-ams

Conversation

@iganakov

@iganakoviganakov commented Feb 22, 2023

Copy link
Copy Markdown
Contributor

Add 'Key-phrase detected' message to AMS service.
Add AMS service helper functions.
Add support for AMS event notifications to KD and KPB modules.

Comment threadsrc/include/sof/lib/ams.h Outdated
Comment threadsrc/lib/ams.c Outdated
Comment threadsrc/audio/kpb.c Outdated
Comment threadsrc/audio/kpb.c Outdated
Comment threadsrc/samples/audio/detect_test.c Outdated

@plbossartplbossart left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sustained objection from me on the way the GUIDs are declared as part of the ams.c code. This cannot work if you want applications to make use of this service, we clearly do not want an endless set of patches to add new GUIDs when two modules need to communicate.

@lgirdwood@mwasko FYI

Comment threadsrc/include/sof/lib/ams.h Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this does not scale, the AMS service should not have to know about 3rd-party IP messages.

Put differently, we are not going to add a new variable every time someone makes use of AMS.

Comment threadsrc/lib/ams.c Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AMS should be a service that uses UUIDs blindly. Definitions of 3rd party module GUIDs should not be there.

Even if it was the case, this should be conditionally added on KPB being compiled in.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iganakov we do need a solution where the UUIDs are discovered at runtime, i.e. we have no hard coding in the AMS core. The AMS core should be usable by any module without AMS knowing about that module. This will mean extending the AMS API so that new clients can register their UUIDs (and any other meta data) at runtime (needs to support FW boot and post boot registrations).

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@plbossart@lgirdwood Sure, I agree that messages shouldn't be hardcoded in core AMS implementation. What I'd like to do is to put ams messages definitions for SOF built-in modules into one common place, e.g. src\include\sof\lib\ams_msg.h, thus it will be easier to manage them. 3rd party modules can define their own messages independently.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that would make it easier to manage them. Both the consumer and the producer need access to the UUID and they'd also usually need access to a type declaration for the data, transferred with the AMS. So they'll have a common header. Then why don't they declare that UUID in that header too? That way when adding new AMS users they won't need to touch a global header with all UUIDs.

Comment threadsrc/audio/kpb.c Outdated

@lgirdwoodlgirdwood left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if storing basefw, library and/or module GUIDs in an elf linker section with metadata is enough to scale here so that we don't need to change any SW or FW when new external AMS users are added.

Could topology additionally be used to help establish relationships between AMS users with metadata ?

Comment threadsrc/idc/idc.c Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably need a mask macro here to align all users.

Comment threadsrc/audio/kpb.c Outdated
Comment threadsrc/audio/kpb.c Outdated
Comment threadsrc/samples/audio/detect_test.c Outdated
@iganakov

Copy link
Copy Markdown
ContributorAuthor

Add "Key-phrase detected" ams message definition in ams_msg.h/ams_msg.c files which will be used by built-in modules

Comment threadsrc/lib/ams_msg.c Outdated

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we're still discussing whether we want to collect UUIDs, right? We probably won't get too many AMS users but still I don't see a value in having to change a common file every time you add a new user.

Comment threadsrc/samples/audio/detect_test.c Outdated

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it make sense to make an exported kpb_register_ams_producer(const struct comp_dev *dev, struct ams_message_payload *payload) and only let the caller add payload->message and payload->message_length after calling it?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented in new revision

Comment threadsrc/samples/audio/detect_test.c Outdated

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could be a helper too, just add a uuid parameter

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented in new revision

@iganakov
iganakov marked this pull request as ready for review March 20, 2023 08:33
@iganakov
iganakovforce-pushed the prv-kd-ams branch 2 times, most recently from 0f2b3ed to 212dcd8CompareMarch 24, 2023 16:25
@iganakov

Copy link
Copy Markdown
ContributorAuthor

In new revision I put AMS messages to KPB module. Added "Register KPB client message". Also added helpers for both AMS consumer and producer.

@iganakov
iganakov requested a review from lyakhMarch 24, 2023 16:34
@mwasko

Copy link
Copy Markdown
Contributor

@plbossart, @lyakh let me know if it is good for merge now.

Comment threadsrc/audio/kpb.c Outdated
Comment on lines +143 to +152
const uint8_t ams_kpd_msg_uuid[UUID_SIZE] = {0x80, 0xa1, 0x11, 0x22, 0xb3, 0x6c,
0x11, 0xed, 0xaf, 0xa1, 0x02, 0x42,
0xac, 0x12, 0x00, 0x02};

/* Register KPB client uuid: c5d9850f-cbdd-4bf4-9c6a-43dfc5faee9f*/
const uint8_t ams_reg_cli_msg_uuid[UUID_SIZE] = {0xcd, 0x59, 0x85, 0x0f, 0xdd,
0xcb, 0xf4, 0x4b, 0x9c, 0x6a,
0x43, 0xdf, 0xc5, 0xfa, 0xee,
0x9f};

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iganakov how are the UUIDs shared with AMS messaging peers ? i.e. if a 3rd party module wanted to send/recive a AMS messge with KPB ?
I understand we register with AMS using a client UUID, but how do peers who want to communicate with the client know the client UUID or the message type UUID when they are not built in ? i.e from a library ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iganakov ping - need to unblock. Can we have UUIDs in a global header as a macro definition of the UUID values. Then C files can then include the header and use the named macro with the UUID value ?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lgirdwood, sorry for the late reply. Yes, it seems this is the most reasonable way to do it. Thanks!
I'm working on a new revision of this PR

@sys-pt1s

Copy link
Copy Markdown

Can one of the admins verify this patch?

Comment threadsrc/include/sof/lib/ams_msg.h Outdated

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this macro AMS-specific? If it is it would be less confusing if you called it AMS_DEFINE_UUID

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is AMS-specific. I'll change the name to AMS_DEFINE_UUID

plbossart
plbossart previously requested changes Apr 17, 2023

@plbossartplbossart left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only minor style changes, looks much better than the initial version. Thanks @iganakov

Comment threadsrc/include/sof/lib/ams_msg.h Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what was wrong with DECLARE_SOF_UUID() and friends?

This macro style may be legit but it's error probe without any ways to check the number of arguments, etc.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DECLARE_SOF_UUID() defines sof_uuid_entry as a static so it can't be exported. DECLARE_SOF_RT_UUID implicitly invokes DECLARE_SOF_UUID() which will result to static variable defined but not used compilation error.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This gives a checkpatch error "ERROR: Macros with complex values should be enclosed in parentheses" , but I think it's ok. At least I can't come up with a better way to wrap this. @lyakh ideas on this?

@marc-hbmarc-hbApr 19, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand what problem DEFINE_UUID() solves. How about something much simpler and endianness-free like this:

typedefuint8_tsimple_uuid_t[16];
#defineAMS_KPD_MSG_UUID { 0x80, 0xa1, 0x11, 0x22, 0xb3, 0x6c, 0x11, 0xed, 0xaf, 0xa1, 0x02, 0x42, 0xac, 0x12, 0x00, 0x02 }
staticconstsimple_uuid_tams_kpd_msg_uuid=AMS_KPD_MSG_UUID;

EDIT: see longer thread above.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case alignement was a concern:

typedefunion {
uint8_tbytes[16];
uint32_t_; // just for alignment
} simple_uuid_t;
#defineAMS_KPD_MSG_UUID { 0x80, 0xa1, 0x11, 0x22, 0xb3, 0x6c, 0x11, 0xed, 0xaf, 0xa1, 0x02, 0x42, 0xac, 0x12, 0x00, 0x02 }
staticconstsimple_uuid_tams_kpd_msg_uuid= {
.bytes=AMS_KPD_MSG_UUID
};

@marc-hbmarc-hbApr 21, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, if at all possible, would be best to use the common SOF UUID tooling.

The entire idea behind UUIDs is that they can be universally exchanged across all projects/files/modules/languages/etc. without having to share or re-use anything: no header file, no API, no macro, no naming convention, no central registration,... and no tooling. Yet somehow over-engineering manages to creep in anyway :-(

I agree we shouldn't copy/paste/diverge __section(".static_uuids") or solutions to endianness issues, in those cases some shared "tooling" is needed. However if some files somewhere merely want to share some universal 16 bytes constant ONLY (either macro or extern) and nothing else then these files shouldn't have to worry about re-using any tooling; that would be overkill.

PS: https://www.kernel.org/doc/html/latest/dev-tools/checkpatch.html

checkpatch is not always right. Your judgement takes precedence over checkpatch messages

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

without having to share or re-use anything: no header file, no API,...

Another key feature of UUIDs is IMMUTABILITY. I hate duplication more than anyone else but immutability means UUID values can be and in fact are copied if/when needed. Not just for fun of course but if/when it makes things easier.

“A little copying is better than a little dependency”

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'll use a macro like @lgirdwood suggested I won't be able to path its value to DECLARE_SOF_UUID() without splitting it to proper args. So I suppose additional macro would be needed.

@iganakov I've just tried this and it seems to have worked. Whether or not we decide to define UUIDs in headers, it seems like it's also possible with the existing API:

diff --git a/src/lib/notifier.c b/src/lib/notifier.c
index a601538d7..ccb524ff6 100644
--- a/src/lib/notifier.c
+++ b/src/lib/notifier.c
@@ -22,8 +22,9 @@
LOG_MODULE_REGISTER(notifier, CONFIG_SOF_LOG_LEVEL);
/* 1fb15a7a-83cd-4c2e-8b32-4da1b2adeeaf */
-DECLARE_SOF_UUID("notifier", notifier_uuid, 0x1fb15a7a, 0x83cd, 0x4c2e,
- 0x8b, 0x32, 0x4d, 0xa1, 0xb2, 0xad, 0xee, 0xaf);
+#define uuid_notify 0x1fb15a7a, 0x83cd, 0x4c2e, 0x8b, 0x32, 0x4d, 0xa1, 0xb2, 0xad, 0xee, 0xaf
+#define __DECLARE_SOF_UUID(...) DECLARE_SOF_UUID(__VA_ARGS__)
+__DECLARE_SOF_UUID("notifier", notifier_uuid, uuid_notify);
DECLARE_TR_CTX(nt_tr, SOF_UUID(notifier_uuid), LOG_LEVEL_INFO);

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lyakh I've tried your approach and I'm unable to access uuid value in runtime using DECLARE_SOF_UUID(). Then I noticed the macro description which tells there is need of uuid dictionary from ldc file to do that. Is there a way to avoid this using SOF uuid infrastructure?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed uuid.h seems to have grown some coupling with SMEX and .ldc files which the most advanced Zephyr configurations... don't even use anymore.

Again the entire purpose and design of UUIDs is be "universal" and simple by NOT requiring any API/framework/tooling. If the DECLARE_SOF_UUID() "tooling" does not work in this particular case then don't use any tooling at all because none is required yet everything will STILL be compatible with anything else using the same UUIDs. That's the magic of UUIDs: compatibility with zero API.

Comment threadsrc/audio/kpb.c Outdated
Comment threadsrc/audio/kpb.c Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spurious change?

Comment threadsrc/audio/kpb.c Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the cast necessary? It's a classic with UUIDs to keep cast pointers, can we not use a 'native' uuid_t or something that everyone can agree on?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See below.

Comment threadsrc/samples/audio/detect_test.c Outdated
Comment threadsrc/samples/audio/detect_test.c Outdated
Comment threadsrc/samples/audio/detect_test.c Outdated
Comment threadsrc/samples/audio/detect_test.c Outdated
Comment threadsrc/samples/audio/detect_test.c Outdated

@kv2019ikv2019i left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few clarification/cleanups, but this starts to seem ready to go. For the one checkpath error, I think this is something we can live with.

Comment threadsrc/include/sof/lib/ams_msg.h Outdated

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This gives a checkpatch error "ERROR: Macros with complex values should be enclosed in parentheses" , but I think it's ok. At least I can't come up with a better way to wrap this. @lyakh ideas on this?

Comment threadsrc/include/sof/lib/ams_msg.h Outdated

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why uint32_t and the endianness and VA_ARGS complexity? This seems to be always cast to uint8t_t * in the end anyway.

Comment threadsrc/audio/kpb.c Outdated

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See below.

Comment threadsrc/include/sof/lib/ams_msg.h Outdated

@marc-hbmarc-hbApr 19, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand what problem DEFINE_UUID() solves. How about something much simpler and endianness-free like this:

typedefuint8_tsimple_uuid_t[16];
#defineAMS_KPD_MSG_UUID { 0x80, 0xa1, 0x11, 0x22, 0xb3, 0x6c, 0x11, 0xed, 0xaf, 0xa1, 0x02, 0x42, 0xac, 0x12, 0x00, 0x02 }
staticconstsimple_uuid_tams_kpd_msg_uuid=AMS_KPD_MSG_UUID;

EDIT: see longer thread above.

@iganakov
iganakovforce-pushed the prv-kd-ams branch 2 times, most recently from 10689ae to 5526978CompareMay 16, 2023 15:01
@iganakov

Copy link
Copy Markdown
ContributorAuthor

In new revision I defined AMS message uuid the way @marc-hb suggested.
@lgirdwood@kv2019i@lyakh@plbossart please review again

@marc-hb

marc-hb commented May 16, 2023

Copy link
Copy Markdown
Collaborator

I spotted a few genuine compilation issues "hidden" between the unrelated http://azure.archive.ubuntu.com/ and http://archive.ubuntu.com/ubuntu timeouts

https://github.com/thesofproject/sof/actions/runs/4993519332/jobs/8942765968?pr=7158

/home/runner/work/sof/sof/workspace/sof/src/include/sof/coherent.h:70:3: error: use of undeclared identifier 'DCACHE_LINE_SIZE'
} __coherent;
^
/home/runner/work/sof/sof/workspace/sof/src/include/sof/coherent.h:20:51: note: expanded from macro '__coherent'
#define __coherent __attribute__((packed, aligned(DCACHE_LINE_SIZE)))
^
/home/runner/work/sof/sof/workspace/sof/src/include/sof/coherent.h:321:3: warning: implicit declaration of function 'dcache_invalidate_region' is invalid in C99 [-Wimplicit-function-declaration]
dcache_invalidate_region(cc, size);
^
/home/runner/work/sof/sof/workspace/sof/src/include/sof/coherent.h:334:3: warning: implicit declaration of function 'dcache_writeback_invalidate_region' is invalid in C99 [-Wimplicit-function-declaration]
dcache_writeback_invalidate_region(c, size);
^
/home/runner/work/sof/sof/workspace/sof/src/include/sof/coherent.h:386:3: warning: implicit declaration of function 'dcache_invalidate_region' is invalid in C99 [-Wimplicit-function-declaration]
dcache_invalidate_region(cc, size);
^
/home/runner/work/sof/sof/workspace/sof/src/include/sof/coherent.h:399:3: warning: implicit declaration of function 'dcache_writeback_invalidate_region' is invalid in C99 [-Wimplicit-function-declaration]
dcache_writeback_invalidate_region(c, size);
^

The "zmain" failures are known and unrelated:

Comment threadsrc/audio/kpb.c Outdated

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to unregister AMS here?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, there is no need to unregister AMS here. kpb_reset() is called during PIPELINE_RESET state. RESET on pipeline could be called in two cases:

  1. Init -> Reset -> Pause -> Running
  2. Pause -> Reset -> Delete

So calling ams_helper_unregister_consumer() in kpb_free() will be sufficient.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then why is notifier_unregister() called both in reset and free? Is that superfluous?

@kv2019ikv2019i left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me now. One odditity in first commit, the Kconfig change should be a separate commit.

Comment threadsrc/platform/Kconfig Outdated
@kv2019i

Copy link
Copy Markdown
Collaborator

@plbossart Can you check?
@iganakov Can you address the Kconfig change in first patch?

@kv2019i
kv2019i requested a review from plbossartMay 22, 2023 12:08
@iganakov

Copy link
Copy Markdown
ContributorAuthor

@kv2019i Sure, I'll update it today

Add "Key Phrase Detected" message uuid
Signed-off-by: Ievgen Ganakov <ievgen.ganakov@intel.com>
Add IPC4 dependency for AMS
Signed-off-by: Ievgen Ganakov <ievgen.ganakov@intel.com>
@marc-hb

Copy link
Copy Markdown
Collaborator

Compilation still failing in https://github.com/thesofproject/sof/actions/runs/5048359483/jobs/9056458593?pr=7158

 -c /home/runner/work/sof/sof/workspace/sof/src/ipc/ipc4/ams_helpers.c
In file included from /home/runner/work/sof/sof/workspace/sof/src/ipc/ipc4/ams_helpers.c:8:
In file included from /home/runner/work/sof/sof/workspace/sof/src/include/ipc4/ams_helpers.h:11:
In file included from /home/runner/work/sof/sof/workspace/sof/src/include/sof/lib/ams.h:15:
/home/runner/work/sof/sof/workspace/sof/src/include/sof/coherent.h:70:3: error: use of undeclared identifier 'DCACHE_LINE_SIZE'
} __coherent;
^
/home/runner/work/sof/sof/workspace/sof/src/include/sof/coherent.h:20:51: note: expanded from macro '__coherent'
#define __coherent __attribute__((packed, aligned(DCACHE_LINE_SIZE)))
^
/home/runner/work/sof/sof/workspace/sof/src/include/sof/coherent.h:321:3: warning: implicit declaration of function 'dcache_invalidate_region' is invalid in C99 [-Wimplicit-function-declaration]
dcache_invalidate_region(cc, size);
^
/home/runner/work/sof/sof/workspace/sof/src/include/sof/coherent.h:334:3: warning: implicit declaration of function 'dcache_writeback_invalidate_region' is invalid in C99 [-Wimplicit-function-declaration]
dcache_writeback_invalidate_region(c, size);
^
/home/runner/work/sof/sof/workspace/sof/src/include/sof/coherent.h:386:3: warning: implicit declaration of function 'dcache_invalidate_region' is invalid in C99 [-Wimplicit-function-declaration]
dcache_invalidate_region(cc, size);
^
/home/runner/work/sof/sof/workspace/sof/src/include/sof/coherent.h:399:3: warning: implicit declaration of function 'dcache_writeback_invalidate_region' is invalid in C99 [-Wimplicit-function-declaration]
dcache_writeback_invalidate_region(c, size);
^
In file included from /home/runner/work/sof/sof/workspace/sof/src/ipc/ipc4/ams_helpers.c:9:
In file included from /home/runner/work/sof/sof/workspace/sof/src/include/sof/audio/component.h:19:
In file included from /home/runner/work/sof/sof/workspace/sof/src/include/sof/audio/buffer.h:11:
In file included from /home/runner/work/sof/sof/workspace/sof/src/include/sof/audio/audio_stream.h:22:
In file included from /home/runner/work/sof/sof/workspace/sof/zephyr/include/rtos/cache.h:11:
/home/runner/work/sof/sof/workspace/sof/src/arch/host/include/arch/lib/cache.h:16:20: error: static declaration of 'dcache_invalidate_region' follows non-static declaration
static inline void dcache_invalidate_region(void *addr, size_t size) {}
^
/home/runner/work/sof/sof/workspace/sof/src/include/sof/coherent.h:321:3: note: previous implicit declaration is here
dcache_invalidate_region(cc, size);
^
In file included from /home/runner/work/sof/sof/workspace/sof/src/ipc/ipc4/ams_helpers.c:9:
In file included from /home/runner/work/sof/sof/workspace/sof/src/include/sof/audio/component.h:19:
In file included from /home/runner/work/sof/sof/workspace/sof/src/include/sof/audio/buffer.h:11:
In file included from /home/runner/work/sof/sof/workspace/sof/src/include/sof/audio/audio_stream.h:22:
In file included from /home/runner/work/sof/sof/workspace/sof/zephyr/include/rtos/cache.h:11:
/home/runner/work/sof/sof/workspace/sof/src/arch/host/include/arch/lib/cache.h:18:20: error: static declaration of 'dcache_writeback_invalidate_region' follows non-static declaration
static inline void dcache_writeback_invalidate_region(void *addr,
^
/home/runner/work/sof/sof/workspace/sof/src/include/sof/coherent.h:334:3: note: previous implicit declaration is here
dcache_writeback_invalidate_region(c, size);
^
4 warnings and 3 errors generated.

Comment threadzephyr/CMakeLists.txt Outdated

@marc-hbmarc-hbMay 22, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You probably need this:

zephyr_library_sources_ifdef(CONFIG_AMS
${SOF_IPC_PATH}/ipc4/ams_helpers.c
)

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @marc-hb! Fixed

@kv2019ikv2019i left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good now @iganakov . Can you check the ipc4 fuzzer build error in CI? @marc-hb 's idea probably will be enough to fix it.

@plbossart ping again, this would be ready to go

Every module which needs AMS for notifications uses the same flow when
configuring itself as a consumer or producer for such notifications.
Add AMS helper functions to avoid code repetition.
Signed-off-by: Ievgen Ganakov <ievgen.ganakov@intel.com>
Add AMS notifications support to KPB module
Signed-off-by: Ievgen Ganakov <ievgen.ganakov@intel.com>
Add support for AMS notifications
Signed-off-by: Ievgen Ganakov <ievgen.ganakov@intel.com>
@iganakov

Copy link
Copy Markdown
ContributorAuthor

@kv2019i ipc4 fuzzer error is fixed, thanks

@marc-hb

Copy link
Copy Markdown
Collaborator

And now of course the old fuzzer started failing! However this time it's unrelated and can be ignored
https://github.com/thesofproject/sof/actions/runs/5065883155/jobs/9094978916?pr=7158

@kv2019i
kv2019i dismissed plbossart’s stale reviewMay 24, 2023 12:46

Multiple updates since the review and no re-review

@kv2019i

Copy link
Copy Markdown
Collaborator

@kv2019i
kv2019i merged commit ead25e8 into thesofproject:mainMay 24, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@iganakov@mwasko@sys-pt1s@marc-hb@kv2019i@plbossart@lyakh@lgirdwood