Add get buffer ID to sink/src api #8433

marcinszkudlinski · 2023-11-02T14:44:41Z

sink/src API should grow to fulfill all requirements

this PR adds stream ID to sink/src api

kv2019i

Code looks good. The commit description is little light on the rationale. Is this enabling something, or is this just moving an attribute to a more correct place?

I'm a bit worried about the conceptual override where sink/src in theory allows to abstract the "id", while current SOF code has pretty specific semantics on the buffer id (e.g. using "IPC4_SINK_QUEUD_ID(buf_id)" macro on it). Nothing wrong with this itself, but I wonder if this is abstracted more (behind src/sink), should we documented the semantics better.

Btw, you have build errors in IPC3 targets in CI.

lgirdwood

I like the abstraction.

lgirdwood · 2023-11-03T16:43:31Z

src/audio/sink_api_helper.c

@@ -185,3 +185,9 @@ size_t sink_get_min_free_space(struct sof_sink *sink)
 {
 	return sink->min_free_space;
 }
+
+uint32_t sink_get_id(struct sof_sink *sink)


These should be static inline. There is PR in flow for splitting header directories and we will be able to catch this in the correct folder i.e. for baseFW or module usage.

static inline and put it tou sink_api.h and source_api.h accordingly.

yes. We agreed lately the full sink/src encapsulation is too expensive and all "small" functions should be inline (#8100)

It has been implemented for sink/src as a part of #8365. Till 8365 is merged I prefer to follow the obsolete way to avoid merge conflicts

marcinszkudlinski · 2023-11-06T11:11:56Z

Code looks good. The commit description is little light on the rationale. Is this enabling something, or is this just moving an attribute to a more correct place?

it is extension to current sink/src - we need feature match with current audio stream

I'm a bit worried about the conceptual override where sink/src in theory allows to abstract the "id", while current SOF code has pretty specific semantics on the buffer id (e.g. using "IPC4_SINK_QUEUD_ID(buf_id)" macro on it). Nothing wrong with this itself, but I wonder if this is abstracted more (behind src/sink), should we documented the semantics better.

i'll check if we can return sink/src part of id
by now i found something like this in code - a complete id used:

			j = buf_get_id(source);
			if (j == BASE_CFG_QUEUED_ID)

Btw, you have build errors in IPC3 targets in CI.

working to fix

lyakh · 2023-11-07T13:29:40Z

src/include/sof/audio/buffer.h

@@ -47,7 +47,7 @@ extern struct tr_ctx buffer_tr;
 #define trace_buf_get_id(buf_ptr) ((buf_ptr)->pipeline_id)

 /** \brief Retrieves subid (comp id) from the buffer */
-#define trace_buf_get_subid(buf_ptr) ((buf_ptr)->id)
+#define buf_get_id(buf_ptr) ((buf_ptr)->stream.runtime_stream_params.id)


is the ID really a runtime parameter? Isn't it persistent?

it is set once, not modified later. Anyway - in rutime
As it is a common parameter for all streams, no matter how buffers are implemented, so I put it to this container
But you're right, it the name of the container may be somehow misleading, I'll change it

lyakh · 2023-11-09T08:48:10Z

Sorry, I do not understand this PR. The code seems to add a parameter that isn't used apart from for logging ant the description "fulfill all requirements" doesn't really clarify it either. Is logging the only reason to add this?

marcinszkudlinski · 2023-11-09T11:18:35Z

@lyakh ???
see how many times buf_get_id() is used - its close to 50
yes, ID is used in logging, but also many other places - copier, google AEC, probe. Its purpose is to identify the pins the buffer is connected to.

the purpose of this PR is to provide access to the ID for components using sink/src interface

lyakh · 2023-11-10T08:05:55Z

@lyakh ??? see how many times buf_get_id() is used - its close to 50 yes, ID is used in logging, but also many other places - copier, google AEC, probe. Its purpose is to identify the pins the buffer is connected to. the purpose of this PR is to provide access to the ID for components using sink/src interface

I mean the second commit - an ID is added to source / sink and it doesn't seem to be used

marcinszkudlinski · 2023-11-20T11:28:52Z

rebase only

marcinszkudlinski · 2023-11-21T10:01:19Z

@lyakh it is used - in a PR mentioned above

kv2019i

PR looks good, but mandatory CI is failing again -> needs to be checked.

runtime_stream_params is a container for all parameters of a stream Add an API call to retrieve it Signed-off-by: Marcin Szkudlinski <[email protected]>

add get buffer ID to sink/src api Signed-off-by: Marcin Szkudlinski <[email protected]>

marcinszkudlinski · 2023-11-21T15:01:26Z

PR looks good, but mandatory CI is failing again -> needs to be checked.

lets retry - the logs indicates some issues with test setup
rebasing to head again

marcinszkudlinski · 2023-11-22T12:50:34Z

CI is green

marcinszkudlinski requested review from bardliao, pblaszko, a team, ranj063, jxstelter, fkwasowi, iganakov, RanderWang, abonislawski, lgirdwood, plbossart, mmaka1, lbetlej, dbaluta and kv2019i as code owners November 2, 2023 14:44

marcinszkudlinski force-pushed the sink_src_id branch from a444436 to 5079c2e Compare November 2, 2023 15:22

kv2019i reviewed Nov 2, 2023

View reviewed changes

lgirdwood reviewed Nov 3, 2023

View reviewed changes

cujomalainey requested a review from johnylin76 November 7, 2023 07:58

lyakh reviewed Nov 7, 2023

View reviewed changes

kv2019i approved these changes Nov 8, 2023

View reviewed changes

btian1 approved these changes Nov 9, 2023

View reviewed changes

marcinszkudlinski changed the title ~~add get buffer ID to sink/src api~~ [DNM - WIP] add get buffer ID to sink/src api Nov 9, 2023

marcinszkudlinski mentioned this pull request Nov 9, 2023

[DNM] Google aec sink src dp #8469

Closed

7 tasks

marcinszkudlinski force-pushed the sink_src_id branch from 5079c2e to cb170d1 Compare November 20, 2023 11:28

marcinszkudlinski changed the title ~~[DNM - WIP] add get buffer ID to sink/src api~~ Add get buffer ID to sink/src api Nov 20, 2023

kv2019i approved these changes Nov 21, 2023

View reviewed changes

marcinszkudlinski added 2 commits November 21, 2023 16:00

buf: move buffer/stream ID to runtime_stream_params

57daf1c

runtime_stream_params is a container for all parameters of a stream Add an API call to retrieve it Signed-off-by: Marcin Szkudlinski <[email protected]>

sink/src: add ID to sink/src API

9bb6599

add get buffer ID to sink/src api Signed-off-by: Marcin Szkudlinski <[email protected]>

marcinszkudlinski force-pushed the sink_src_id branch from cb170d1 to 9bb6599 Compare November 21, 2023 15:01

kv2019i merged commit c82226e into thesofproject:main Nov 22, 2023

marcinszkudlinski deleted the sink_src_id branch June 21, 2024 07:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add get buffer ID to sink/src api #8433

Add get buffer ID to sink/src api #8433

marcinszkudlinski commented Nov 2, 2023

kv2019i left a comment

lgirdwood left a comment

lgirdwood Nov 3, 2023

btian1 Nov 6, 2023

marcinszkudlinski Nov 6, 2023

marcinszkudlinski commented Nov 6, 2023 •

edited

Loading

lyakh Nov 7, 2023

marcinszkudlinski Nov 9, 2023

lyakh commented Nov 9, 2023

marcinszkudlinski commented Nov 9, 2023

lyakh commented Nov 10, 2023

marcinszkudlinski commented Nov 20, 2023

marcinszkudlinski commented Nov 21, 2023

kv2019i left a comment

marcinszkudlinski commented Nov 21, 2023

marcinszkudlinski commented Nov 22, 2023

Add get buffer ID to sink/src api #8433

Add get buffer ID to sink/src api #8433

Conversation

marcinszkudlinski commented Nov 2, 2023

kv2019i left a comment

Choose a reason for hiding this comment

lgirdwood left a comment

Choose a reason for hiding this comment

lgirdwood Nov 3, 2023

Choose a reason for hiding this comment

btian1 Nov 6, 2023

Choose a reason for hiding this comment

marcinszkudlinski Nov 6, 2023

Choose a reason for hiding this comment

marcinszkudlinski commented Nov 6, 2023 • edited Loading

lyakh Nov 7, 2023

Choose a reason for hiding this comment

marcinszkudlinski Nov 9, 2023

Choose a reason for hiding this comment

lyakh commented Nov 9, 2023

marcinszkudlinski commented Nov 9, 2023

lyakh commented Nov 10, 2023

marcinszkudlinski commented Nov 20, 2023

marcinszkudlinski commented Nov 21, 2023

kv2019i left a comment

Choose a reason for hiding this comment

marcinszkudlinski commented Nov 21, 2023

marcinszkudlinski commented Nov 22, 2023

marcinszkudlinski commented Nov 6, 2023 •

edited

Loading