MUX: Generic Mux component implementation. by akloniex · Pull Request #1275 · thesofproject/sof

akloniex · 2019-04-16T20:37:26Z

Mux component implementation. Support for mux and demux operation
modes, channel routing and summation with sturation.

Signed-off-by: ArturX Kloniecki arturx.kloniecki@linux.intel.com

Mux component can operate in 2 modes:

demux - with single input stream and multiple output streams (up to MUX_MAX_STREAMS, defined in header)
mux - with multiple input streams and single output stream

It can also work with 1:1 I/O in both modes to act as channel router / summator.

It is configured via binary blob specifying channel routing using bitmasks.
In case of demux mode every output stream has an array of bitmasks, one mask per channel, specifying which of the input channels should be used to compose it.
Similarly for mux, each input stream has bitmask for each of it's channels to specify to which output channel it contributes.

tlauda

Initial review.
Many changes not related to mux implementation need to be extracted to separate PR.

lgirdwood · 2019-04-17T13:41:28Z

@akloniex there are also some CI failures to fix too.

ranj063 · 2019-04-19T00:52:36Z

+	union {
+		mux_func mux;
+		demux_func demux;
+	};


@akloniex I think we should remove the union and replace it with one function. Then we can simplify a bunch of other duplicated code. see my other comments

ranj063 · 2019-04-19T00:53:41Z

+	}
+
+	return NULL;
+}


if we only have one function in the comp_data, we can pass the mode, and pick the appropriate function instead of 2 separate functions which have the exact same logic

ranj063 · 2019-04-19T00:54:37Z

+			ret = -EINVAL;
+			goto err;
+		}
+	} else {


same here remove duplication and pass mode to get_processing_function()

ranj063 · 2019-04-19T03:15:38Z

@akloniex also the mux_process and demux_process functions seems to mirrored code. Is there a way we can use one function and set the source/sink based on mode. might be worth giving it a try.

mmaka1 · 2019-04-19T09:09:30Z

+	uint8_t mask[PLATFORM_MAX_CHANNELS];
+};
+
+typedef void(*mux_func)(struct comp_dev *dev, struct comp_buffer *sink,


If this is a public API header (looks like), do those functions prototypes need to be exposed?

This is public API header, and these functions do not need to be exposed, you're correct.
These functions are meant to be used by mux privately, althought, my thought process behind exposing these was to provide interface, used in mux.c file (which handles all general component duties) and implemented in mux_generic.c file (and to be implemented using hifi functions).
I could add another, private, header to provide the interface, but thought that it would look much cleaner the way it is right now.
But I won't fight for it if you think that hiding these details in private header or by calling extern functions would work better.

juimonen · 2019-04-23T08:06:53Z

So is this supposed to be "processing component" or "mux" from alsa and topology perspective? check what we did for e.g. with selector... this will effect the implementation on the driver side. If it is "processing" it is easier. With general "mux" component we have to think more carefully what is the control interface etc...

juimonen · 2019-04-23T08:13:59Z

still to emphasize, we should not just use binary control always if it makes thing easier. Currently it is also cumbersome as we need an alsa control for it (even though it is not ever used). With eq's it is natural because we are sending variable length coefficients and it makes no sense to define fixed filter coefficients (length wise). If mux is for general use we should define the parameters with tuples in topology.

dbaluta · 2019-04-23T08:24:39Z

@akloniex

* mux - with single input stream and multiple output streams (up to MUX_MAX_STREAMS, defined in header)

* demux - with multiple input streams and single output stream

Isn't the naming the other way around? I mean a muxer has multiple inputs and single output.

akloniex · 2019-04-23T12:09:17Z

@juimonen
This component should operate as processing component under ALSA classification, and I'm now adjusting implementation to match that.
Also, I will add configuration at instantiation.

juimonen · 2019-05-09T10:38:55Z

corresponding ABI change in driver:
thesofproject/linux#870

lgirdwood

@akloniex there are some C++ style comments which can be fixed incrementally in another PR

jajanusz

Part related to mux is ok for initial version, but it looks that we sitll didn't decide what to do with dependency of it -> #1383
So I 'request changes' just to not merge PR that it includes by accident.

juimonen · 2019-05-22T06:30:59Z

@jajanusz there's now some kind of solution in #1383 and I think @akloniex updated it to this PR also. You should see if this is suitable for you (and maybe @tlauda also)..

I was testing this with mixer and demux pipeline in up2 and they both seem to work, so I didn't see any regressions there...

jajanusz · 2019-05-22T11:51:28Z

@jajanusz there's now some kind of solution in #1383 and I think @akloniex updated it to this PR also. You should see if this is suitable for you (and maybe @tlauda also)..

I was testing this with mixer and demux pipeline in up2 and they both seem to work, so I didn't see any regressions there...

It's good, I'll just keep as Request changes until #1383 will be merged.

lgirdwood · 2019-05-22T21:13:48Z

@jajanusz @mmaka1 @tlauda have your review comments been addressed ?

jajanusz · 2019-05-23T10:50:56Z

@lgirdwood I just need more reviews in #1383 ;)

jajanusz · 2019-05-23T10:54:26Z

@lgirdwood BTW IMO it's bad idea to split dependency into another PR, because it causes mess like this one. If it's not that big it's better to just make it another commit in the same PR, this way you have something+it's dependency in 1 place and in the end it looks the same in git history. It would be problematic only if you have super long conversations mixed when some of texts are for original PR and some for dependency.

Mux component implementation. Support for mux and demux operation modes, channel routing and summation with saturation. Mux component can operate in 2 modes: demux - with single input stream and multiple output streams (up to MUX_MAX_STREAMS) mux - with multiple input streams and single output stream It can also work with 1:1 I/O in both modes to act as channel router / summator. It is configured via binary blob specifying channel routing using bitmasks. In case of demux mode every output stream has an array of bitmasks, one mask per channel, specifying which of the input channels should be used to compose it. Similarly for mux, each input stream has bitmask for each of it's channels to specify to which output channel it contributes. Signed-off-by: ArturX Kloniecki <arturx.kloniecki@linux.intel.com>

jajanusz · 2019-05-24T08:13:12Z

@tlauda @mmaka1 ping

wenqingfu · 2019-05-27T00:35:58Z

@lgirdwood BTW IMO it's bad idea to split dependency into another PR, because it causes mess like this one. If it's not that big it's better to just make it another commit in the same PR, this way you have something+it's dependency in 1 place and in the end it looks the same in git history. It would be problematic only if you have super long conversations mixed when some of texts are for original PR and some for dependency.

We need to remind and reinforce "master first" policy, too... And, corresponding patch in the driver repository has thesofproject/linux#870 has been merged, so ping @tlauda @mmaka1

akloniex requested review from jajanusz, lbetlej, lgirdwood, mmaka1, plbossart, ranj063 and tlauda April 16, 2019 20:37

akloniex force-pushed the mux branch from 4e52b6d to eb8bd5e Compare April 16, 2019 21:17

tlauda suggested changes Apr 16, 2019

View reviewed changes

akloniex force-pushed the mux branch 4 times, most recently from 65185e1 to 49f10ea Compare April 16, 2019 23:41

jajanusz suggested changes Apr 17, 2019

View reviewed changes

Comment thread src/audio/mux.c Resolved

akloniex force-pushed the mux branch 2 times, most recently from 8f4cc68 to cd7b61f Compare April 18, 2019 23:43

akloniex requested a review from tlauda April 18, 2019 23:48

ranj063 reviewed Apr 19, 2019

View reviewed changes

mmaka1 requested changes Apr 19, 2019

View reviewed changes

tlauda mentioned this pull request Apr 19, 2019

comp: mux: add init/free and cmd op code to mux #1001

Closed

wenqingfu requested a review from juimonen April 20, 2019 02:19

ranj063 reviewed Apr 23, 2019

View reviewed changes

Comment thread src/audio/mux.c Resolved

juimonen mentioned this pull request May 9, 2019

ASoC: SOF: topology: add support for mux processing component thesofproject/linux#870

Merged

akloniex force-pushed the mux branch 5 times, most recently from 3c3b409 to b7fc384 Compare May 14, 2019 12:19

akloniex requested a review from jajanusz May 14, 2019 12:19

akloniex mentioned this pull request May 15, 2019

DAI: Need echo reference stream #631

Closed

lgirdwood approved these changes May 20, 2019

View reviewed changes

jajanusz suggested changes May 20, 2019

View reviewed changes

akloniex force-pushed the mux branch 2 times, most recently from 31d19ec to 7d2b058 Compare May 21, 2019 18:42

akloniex force-pushed the mux branch from 7d2b058 to 73eec6e Compare May 22, 2019 08:25

akloniex force-pushed the mux branch from 73eec6e to e9cb358 Compare May 22, 2019 12:01

akloniex requested a review from jajanusz May 22, 2019 12:30

akloniex force-pushed the mux branch from e9cb358 to e71de7f Compare May 23, 2019 12:03

akloniex force-pushed the mux branch from e71de7f to c935000 Compare May 24, 2019 07:36

jajanusz approved these changes May 24, 2019

View reviewed changes

mmaka1 approved these changes May 28, 2019

View reviewed changes

akloniex changed the title ~~[RFC] MUX: Generic Mux component implementation.~~ MUX: Generic Mux component implementation. May 29, 2019

lgirdwood merged commit 529bf68 into thesofproject:master May 29, 2019

Conversation

akloniex commented Apr 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tlauda left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lgirdwood commented Apr 17, 2019

Uh oh!

ranj063 Apr 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ranj063 Apr 19, 2019

Choose a reason for hiding this comment

Uh oh!

ranj063 Apr 19, 2019

Choose a reason for hiding this comment

Uh oh!

ranj063 commented Apr 19, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mmaka1 Apr 19, 2019

Choose a reason for hiding this comment

Uh oh!

akloniex Apr 19, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

juimonen commented Apr 23, 2019

Uh oh!

juimonen commented Apr 23, 2019

Uh oh!

dbaluta commented Apr 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

akloniex commented Apr 23, 2019

Uh oh!

juimonen commented May 9, 2019

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

jajanusz left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juimonen commented May 22, 2019

Uh oh!

jajanusz commented May 22, 2019

Uh oh!

lgirdwood commented May 22, 2019

Uh oh!

jajanusz commented May 23, 2019

Uh oh!

jajanusz commented May 23, 2019

Uh oh!

jajanusz commented May 24, 2019

Uh oh!

wenqingfu commented May 27, 2019

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

akloniex commented Apr 16, 2019 •

edited

Loading

ranj063 Apr 19, 2019 •

edited

Loading

dbaluta commented Apr 23, 2019 •

edited

Loading

jajanusz left a comment •

edited

Loading