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

Mixin/Mixout Optimization for simple pipelines #7824

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

ranj063
Copy link
Collaborator

@ranj063 ranj063 commented Jun 16, 2023

Includes the patch from #7792

For a pipeline like: host-copier->gain->mixin->mixout->gain->dai-copier
The original MCPS (Measured on TGL chromebook) is:

            host-copier.2.playback,   comp:0 0x40000,     3.46,     6.89
                          gain.5.1,   comp:0 0x60000,     2.17,     9.22
                         mixin.5.1,   comp:0 0x20000,     2.62,     5.94
                        mixout.6.1,   comp:1 0x30000,     1.89,     2.42
                          gain.6.1,   comp:1 0x60001,     2.22,    16.57
 dai-copier.SSP.NoCodec-2.playback,   comp:1 0x40001,     6.05,     6.41
                                  ,                 ,    18.42,    47.44

And the optimized performance is:

                              Name,        Mtrace id, avg MCPS, max MCPS
            host-copier.2.playback,   comp:0 0x40000,     3.42,     6.86
                          gain.5.1,   comp:0 0x60000,     2.19,     9.24
                         mixin.5.1,   comp:0 0x20000,     0.80,     0.82
                        mixout.6.1,   comp:1 0x30000,     2.14,     2.44
                          gain.6.1,   comp:1 0x60001,     2.19,     9.34
 dai-copier.SSP.NoCodec-2.playback,   comp:1 0x40001,     5.90,    13.75
                                  ,                 ,    16.64,    42.46

@ranj063 ranj063 force-pushed the pr7792 branch 5 times, most recently from a77e942 to 325197a Compare June 16, 2023 21:38
}

frames = audio_stream_avail_frames_aligned(input_buffers[0].data,
output_buffers[0].data);
Copy link
Contributor

Choose a reason for hiding this comment

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

why not use the input_buffers[0].size directly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

input_buffers[0].size is only whats available in the input_buffer isnt it? what we need is the MIN(avail, free) isnt it?

Copy link
Contributor

Choose a reason for hiding this comment

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

from your code line 814:
frames = audio_stream_avail_frames_aligned(&source_c->stream, &sink_c->stream);
mod->input_buffers[0].size = frames;
audio_stream_avail_frames_aligned return the MIN(avail, free).

struct comp_buffer *source;

source = list_first_item(&source_dev->bsource_list, struct comp_buffer,
sink_list);
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add a mod->source = source here. In case we may have pipeline like copier-> volume(bypass) -> mixin(bypass) ->mixout. If always take the last component's source as source may lead to unexpected error.

Copy link
Collaborator

@lyakh lyakh Jun 19, 2023

Choose a reason for hiding this comment

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

in practice only mixin will ever be "bypassed," right? Then multiple bypass modules in a row are impossible. But if we do want to be able to handle them, then we need a loop here?

Copy link
Contributor

@andrula-song andrula-song Jun 19, 2023

Choose a reason for hiding this comment

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

actually, I think if there is only one mixin and one mixout, then the mixin and mixout both can be "bypass". and since we trigger processing from pipeline start to end, I don't think we need a loop here if we always update the mod->source if its last component is bypass component.


bytes = frames * audio_stream_frame_bytes(output_buffers[0].data);
input_buffers[0].consumed = bytes;
output_buffers[0].size = bytes;
Copy link
Contributor

Choose a reason for hiding this comment

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

can we set mod->dev->source_bypass_capable = true; and store mod->source & mod->sink here? transfer pointer is faster than audio stream copy.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@andrula-song no we can't do that. The problem is that not all modules use the module adapter yet.

num_sources++;

list_for_item(blist, &dev->bsink_list)
num_sinks++;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I presume, we're sure that there's at least one item in each list, then how about

static bool module_adapter_is_1to1(struct comp_dev *dev)
{
	return list_item_is_last(dev->bsource_list.next, &dev->bsource_list) &&
				 list_item_is_last(dev->bsink_list.next, &dev->bsink_list);
}

and invert calls to module_adapter_is_multi_sink_source() which will remove 3 negations

if (source_dev->source_bypass_capable &&
!module_adapter_is_multi_sink_source(source_dev))
source = list_first_item(&source_dev->bsource_list, struct comp_buffer,
sink_list);
source_c[i++] = buffer_acquire(source);
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we put this above line 774 and use it in line 775?

struct comp_buffer *source;

source = list_first_item(&source_dev->bsource_list, struct comp_buffer,
sink_list);
Copy link
Collaborator

@lyakh lyakh Jun 19, 2023

Choose a reason for hiding this comment

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

in practice only mixin will ever be "bypassed," right? Then multiple bypass modules in a row are impossible. But if we do want to be able to handle them, then we need a loop here?

* buffer into the sink buffer. Otherwise, continue on to the normal mixout
* processing below.
*/
if (source_buffer->sink != mod->dev) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this looks suspicious. If we add generic bypass mechanism, then individual modules shouldn't need to identify such cases, it should be transparent to them. Or maybe we should always call module's .process() method and those modules, that can and should be bypassed can indicate a short-circuit condition in which case the module adapter calls the next .process() with the old input?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

same problem with not all modules not using the module adapter.

@ranj063 ranj063 force-pushed the pr7792 branch 2 times, most recently from fb824e4 to 454a641 Compare June 21, 2023 01:04
@ranj063 ranj063 force-pushed the pr7792 branch 2 times, most recently from d51ecab to 22dfc94 Compare June 28, 2023 21:36
In preparation for making the mixin module skip processingi when it has
only 1 sink, add a couple of new flags in struct comp_dev that will be
used to identify it as bypass capable and enable bypass.

When a module identifies as bypass_capable and bypass is enabled,
it's sink modules will skip their source buffers and directly use the
bypassed module's source buffers for processing.

Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
@ranj063 ranj063 force-pushed the pr7792 branch 4 times, most recently from 12df67d to d65042b Compare June 29, 2023 16:50
ranj063 added 3 commits June 29, 2023 11:06
The current implementation of mixin/mixout with the mixin performing the
mixing and writing out to the mixout's sink buffer is not well suited to
be used with the module adapter.

So change the implementation to perform simple audio stream copy at the
mixin when it has multiple sinks and always perform the mixing operation
in the mixout process callback. When a mixin only has a single sink, its
processing can be bypassed altogether to optimize the performance.

This change removes support for channel remapping and can be added back
later if needed.

Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
just a test

Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
just a test

Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Copy link
Collaborator

@lyakh lyakh left a comment

Choose a reason for hiding this comment

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

not a real review (yet), just a couple of questions, that occurred to me

source_c[i] = buffer_acquire(source);
source_dev = source_c[i]->source;
/* bypass the source dev processing if it has enabled bypass */
if (source_dev->bypass_processing) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

aren't we making sure below that only 1-to-1 processing can use a bypass?

struct module_source_info __sparse_cache *mod_source_info;
struct comp_dev *dev = mod->dev;
/* if there's only one active source, simply copy the source samples to the sink */
if (num_input_buffers == 1 & num_output_buffers == 1 && input_buffers[0].size)
Copy link
Collaborator

Choose a reason for hiding this comment

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

&&?

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.

None yet

3 participants