module_adapter: fix buffer consumption logic in simple_copy mode #6909
Conversation
When simple_copy mode is used, some of the module sinks may be skipped (e.g. source in different state) and the number of inputs passed to module_process() can thus be less than the number of sinks (dev->bsink_list). As a result, the call to comp_update_buffer_consume() may be done with an incorrect pair of comp_buffer (source_c[i]) and matching consumed data (mod->input_buffers[i].consumed). This will lead to asserts triggering as the consumed value can be undefined. Link: thesofproject#6896 Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
kv2019i
requested review from
lgirdwood,
plbossart,
mmaka1,
lbetlej and
dbaluta
as code owners
| i = 0; | ||
| list_for_item(blist, &dev->bsource_list) { | ||
| comp_update_buffer_consume(source_c[i], mod->input_buffers[i].consumed); | ||
| /* some sources may be skipped, so skip_list must be used */ | ||
| if (!(skip_list & BIT(i))) { |
There was a problem hiding this comment.
Isn't this still problematic? Now you've got the wrong consumed value right? What we need to do instead is iterate through 0 -> num_source_buffers and skip those with a different state isnt it?
There was a problem hiding this comment.
@ranj063 I'm open to better ways to do this, but wanted to keep this small to avoid conflicts with your #6836 (but I think we are anyways conflicting already).
But. but. I think this is right. The module process sees "num_input_buffers" of input buffers and reports consumed on each of them. In this loop, we iteratate over these buffers using "mod_input". The "i" variable goes over all bsources.
The consumed is updated in module_process() using the "mod->input_buffers, num_input_buffers" passed as inputs. Thus to pair right source with consumed value, we need to use "input_buffers[mod_input].consumed" as that's the buffer we passed for source "source_c[i]" and specific "i != mod_input" when any sources are skipped.
I can see values of 3+ million bytes passed as argument to comp_update_buffer_consume() when I run pause-resume tests on current main with current code and this PR fixes that. With asserts not enabled, this just silently leads to undefined behaviours.
There was a problem hiding this comment.
@ranj063 This is mimicking the loop on L650, but we can't do the state check here again as the state might have changed (unlikely but would be a very hard to debug problem), and rather we should stick to the decision we do on L655. If a bsource is skipped, we ignore it when calling the consume after process as well.
|
No longer needed, alternative fix merged as #6836 |
When simple_copy mode is used, some of the module sinks may be skipped (e.g. source in different state) and the number of inputs passed to module_process() can thus be less than the number of sinks (dev->bsink_list).
As a result, the call to comp_update_buffer_consume() may be done with an incorrect pair of comp_buffer (source_c[i]) and matching consumed data (mod->input_buffers[i].consumed).
This will lead to asserts triggering as the consumed value can be undefined.
Link: #6896
Signed-off-by: Kai Vehmanen kai.vehmanen@linux.intel.com