Skip to content

Conversation

@bkokoszx
Copy link
Collaborator

Array streams represents streams on "many" side i.e. input
for MUX and output for DEMUX.
For DEMUX each stream has masks array - 1 mask per output
channel. Each mask shows, from which input channel data
should be taken.

This commit reverts "demux" part of commit:
b1b31e7

Signed-off-by: Bartosz Kokoszko bartoszx.kokoszko@linux.intel.com

@bkokoszx
Copy link
Collaborator Author

@lgirdwood lgirdwood added this to the v1.7 milestone Feb 15, 2021
Array streams represents streams on "many" side i.e. input
for MUX and output for DEMUX.
For DEMUX each stream has masks array - 1 mask per output
channel. Each mask shows, from which input channel data
should be taken.

This commit reverts "demux" part of commit:
"b1b31e7154a5c159d81459634eabd8013b434181"

Signed-off-by: Bartosz Kokoszko <bartoszx.kokoszko@linux.intel.com>
@juimonen
Copy link

hold on here... so is something broken? what is this fixing?

@juimonen
Copy link

so matrix row is input and column is output, despite is it mux or demux? so isn't this going exactly wrong now?

@juimonen
Copy link

so in case of mux you have multiple inputs and 1 output, so every matrix correspond to every input stream. with demux you have 1 input and multiple outputs so matrices correspond to the output streams. Anyway, the matrix maps the stream one-to-one, input to output and the matrix input channel is the row and output channel is the mask (column). You just have to always think which 2 streams this matrix connects.

@bkokoszx
Copy link
Collaborator Author

bkokoszx commented Feb 15, 2021

@juimonen
Yes, stream refers always for "many" side - for MUX "many" side is input, for DEMUX output.
For MUX each input stream has masks array - one per input channel. Each mask shows, to which
output channel data should be copied.
For DEMUC each output stream has masks array - one per output channel. Each mask shows, from
which input channel data should be taken.

It fixes issue which appeared in demux python tests.

@juimonen
Copy link

For DEMUC each output stream has masks array - one per output channel. Each mask shows, from
which input channel data should be taken.

So this we exactly disagree :) IMHO the matrix interpretation of input/output should not change is it mux/demux. Mask should always show to which output the channel (row/input channel) is copied. At least to me it makes it really difficult to operate if I have to change the matrix interpretation.

So I mean we can do it both ways, but I don't think we should :) @mwasko @lgirdwood your opinions?

@lgirdwood
Copy link
Member

For DEMUC each output stream has masks array - one per output channel. Each mask shows, from
which input channel data should be taken.

So this we exactly disagree :) IMHO the matrix interpretation of input/output should not change is it mux/demux. Mask should always show to which output the channel (row/input channel) is copied. At least to me it makes it really difficult to operate if I have to change the matrix interpretation.

So I mean we can do it both ways, but I don't think we should :) @mwasko @lgirdwood your opinions?

We need to make sure we align with the upstream topologies being used today. We may have a mix of different mappings that are working with our simple uses cases. @juimonen can you report on what the topologies are doing today (and hopefully they are all doing the same thing) ?

@juimonen
Copy link

@lgirdwood

All public demux configs I could find (I could've maybe missed something) are diagonal with 1's -> so doesn't too much matter
how you define the rows/columns, it only starts to matter when you start to do different mappings.

except in pipe-amp-ref-capture.m4 (used in sof-smart-amplifier.m4) :
if REF_CHMAP is not defined as 8 the matrix is:

1,0,0,0,0,0,0,0,
0,0,1,0,0,0,0,0,
0,0,0,0,0,0,0,0,
0,0,0,0,0,0,0,0,
0,0,0,0,0,0,0,0,
0,0,0,0,0,0,0,0,
0,0,0,0,0,0,0,0,
0,0,0,0,0,0,0,0,

Second row is the interesting: with my interpretation it means copy input channel 2 to output 3, with @bkokoszx it is copy input channel 3 to output 2? @RDharageswari any idea what was the idea here?

But yeah that's the only existing demux config that might cause issues...

@lgirdwood
Copy link
Member

@juimonen thanks, I guess that pipe-amp-ref-capture.m4 is used in shipping devices so we should align to it's use case and add/update comments and sof-docs to this effect.

@lgirdwood
Copy link
Member

@RDharageswari @bkokoszx can someone comment on pipe-amp-ref-capture.m4 as we need to align with this use case. This is blocking release.

@lgirdwood
Copy link
Member

@juimonen are you ok to run this topology setting and check it. @keyonjie also worked on this but cant help atm.

@juimonen
Copy link

@lgirdwood this is run with smart amp (closed) which I don't have, also I don't have smart amp spec so not sure what channels are supposed to be sent to user space in echo ref pcm. So I'm currently kind of blinded what is the use case here... the feedback channel matrix seems to be diagonal configuration, so that should matter here.

@bkokoszx
Copy link
Collaborator Author

bkokoszx commented Feb 16, 2021

@lgirdwood @juimonen
Smart amplifier codec provides current and voltage information about playback channels to the capture stream. For example, if we have 2 channels on playback, smart amplifier codec will provide V0, I0, V1, I1 (I - current, V- voltage, 0 - first pb channel, 1 - second pb channel) to the capture stream.
To the host, we would like to provide only Voltage information. Therefore, the above matrix says that:

  • take 1 input channel (V0) to 1 output channel;
  • take 3 input channel (V1) to 2 output channel;

With this PR we do not have to change topology file.

@juimonen
Copy link

@bkokoszx ok. But this will then change forever how the demux is though to be configured -> and I think it is wrong. I don't want it to be like this from this point until the end of world.

So is someone using this echo ref currently in products somewhere -> do we have possible issues with updating the matrix?

@juimonen
Copy link

So I mean if this is not used yet, just change:
define(REF_CHMAP',0x01,0x04,0x00,0x00,0x00,0x00,0x00,0x00')' to: define(REF_CHMAP',0x01,0x00,0x02,0x00,0x00,0x00,0x00,0x00')'
in pipe-amp-ref-capture.m4 and we should be good to go...

@bkokoszx
Copy link
Collaborator Author

bkokoszx commented Feb 16, 2021

@juimonen This is used e.g. in: sof-tgl-max98373-rt5682.m4

@lgirdwood
I've already merged it to stable-v1.7. I'm a little bit confused now, should I revert it there also? Then there is also need to update python tests and rerun tests before v1.7-rc2 release.

@juimonen
Copy link

@bkokoszx yeah I know that it is in that topology -> the question is that is this echo ref used in real life somewhere in some product? So is there some sw in user space doing something with this data? if not -> nobody cares what we do now. If it is used -> we have a danger someone maybe using mismatched fw and topology -> other channel in echo ref is 0's.

Just saying that later it just gets more difficult to change these. If this is not changed then please also fix sof docs so that it reads how to correctly configure the demux. Also was this the functionality before the simplification? if it was, I could have misinterpreted the functionality all the time -> and even if I did misinterpret it, it still doesn't seems logical. (that we interpret the matrix differently in case of mux compared to demux).

@bkokoszx
Copy link
Collaborator Author

@juimonen I'm not aware whether this data are used by somebody in user space. Sorry, but I think I do not also understand your second question: "Also was this the functionality before the simplification?". What functionality and simplification do you mean?

@lgirdwood
Copy link
Member

This is now aligned.
@bkokoszx will add/update comments for demux in doxygen.
@juimonen will fix sof-docs to align.

@lgirdwood lgirdwood merged commit ce4cd06 into thesofproject:master Feb 17, 2021
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