Skip to content
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

topology: Add support for buffer flags #8513

Merged
merged 1 commit into from
Nov 30, 2023

Conversation

cujomalainey
Copy link
Contributor

Flags have been a part of IPC3 for ages, add support for them in the topology.

Flags have been a part of IPC3 for ages, add support for them in the topology.

Signed-off-by: Curtis Malainey <[email protected]>
@cujomalainey
Copy link
Contributor Author

@andyross FYI

Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

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

Good find - I guess we missed that one. @ranj063 good for you ?

Copy link
Collaborator

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

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

Code looks good but I wonder why we did not expose this in the end. The two flags we have in FW in buffer flags are SOF_BUF_UNDERRUN_PERMITTED and SOF_BUF_OVERRUN_PERMITTED . I have no personal recollection of what these are but these were added in 2020 in commit ee3f7d9 and as far as I can tell, never used (either via IPC, or by setting the underrun/overrun permitted flag with internal logic).

Do we have some fresh need for these flags, or need to add more flags?

I ask because in IPC4, the buffers get added by FW and @marcinszkudlinski has been working on the "audio stream" replacement of buffers -- to allow more complex implementations of buffers (e.g. to save SRAM by pooling memory of buffers in a single pipeline). So I wonder if such behaviour changing flags would better be put to the pipeline nodes versus in the connections (buffers).

OTOH, no harm, so if there's concrete need, I see no issue in proceeding with.

@plbossart
Copy link
Member

I could be wrong, IIRC the flags were added so that "complicated cases" such as the echo reference stream would be allowed to overrun if the stream is not opened by the host.
Indeed it's anyone's guess what this means for IPC4....

@cujomalainey
Copy link
Contributor Author

I could be wrong, IIRC the flags were added so that "complicated cases" such as the echo reference stream would be allowed to overrun if the stream is not opened by the host. Indeed it's anyone's guess what this means for IPC4....

That was my assumption too, hence my comment in the other PR about "how is echo ref even working?"

@ranj063
Copy link
Collaborator

ranj063 commented Nov 28, 2023

I could be wrong, IIRC the flags were added so that "complicated cases" such as the echo reference stream would be allowed to overrun if the stream is not opened by the host. Indeed it's anyone's guess what this means for IPC4....

That was my assumption too, hence my comment in the other PR about "how is echo ref even working?"

@cujomalainey but how does this PR help? Adding this token in topology means the job is still only half done isnt it? Don't you also need a kernel counterpart?

@cujomalainey
Copy link
Contributor Author

I could be wrong, IIRC the flags were added so that "complicated cases" such as the echo reference stream would be allowed to overrun if the stream is not opened by the host. Indeed it's anyone's guess what this means for IPC4....

That was my assumption too, hence my comment in the other PR about "how is echo ref even working?"

@cujomalainey but how does this PR help? Adding this token in topology means the job is still only half done isnt it? Don't you also need a kernel counterpart?

@ranj063 see thesofproject/linux#4714

@kv2019i kv2019i merged commit 285c090 into thesofproject:main Nov 30, 2023
@cujomalainey cujomalainey deleted the flags branch November 30, 2023 16:02
Sign up for free to 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.

5 participants