-
Notifications
You must be signed in to change notification settings - Fork 321
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
base: main
Are you sure you want to change the base?
Conversation
a77e942
to
325197a
Compare
} | ||
|
||
frames = audio_stream_avail_frames_aligned(input_buffers[0].data, | ||
output_buffers[0].data); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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++; |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
fb824e4
to
454a641
Compare
d51ecab
to
22dfc94
Compare
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>
12df67d
to
d65042b
Compare
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>
There was a problem hiding this 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) { |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
&&
?
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:
And the optimized performance is: