Repository navigation
dmic: fix pause/release error leading to invalid dmic_active_fifos - #4283
Conversation
The 'in_active' parameter of dmic_stop() is used to control whether the fifo should be released or not. In case of COMP_TRIGGER_PAUSE, this should be false and the 'dmic_active_fifos' count should not be modified. Without this fix: PAUSED -> RELEASE -> dmic_start() -> no change on dmic_active_fifos ACTIVE -> PAUSE -> dmic_stop() -> dmic_active_fifos-- If a test case repeatedly pauses and releases, 'dmic_active_fifos' will go out-of-sync. Fix the issue that dmic_stop for PAUSE does not release the fifo reference. Fixes: 2273174 ("dmic: don't decrement active FIFO count below 0") Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
|
FYI @keyonjie and @ranj063 This could cause subtle errors in dmic_stop() and dmic_remove() that might show up e.g. with dynamic pipeline topologies. With our mainline SOF (not Zephyr), the "multiple-pause-resume.sh" test passes without error, but if you look at sof-logger, one can see: ... as active_fifos count goes to zero, this means dmic driver does not work as expected and more subtle errors can occur, even if not caught by this test case. |
| case COMP_TRIGGER_PAUSE: | ||
| dmic->state = COMP_STATE_PAUSED; | ||
| dmic_stop(dai, true); | ||
| dmic_stop(dai, false); |
There was a problem hiding this comment.
@kv2019i is this a better fix?
if (dmic->state == COMP_STATE_PREPARE || dmic->state == COMP_STATE_PAUSED)
(*uncached_dmic_active_fifos)++;
There was a problem hiding this comment.
@kv2019i I'm worried about active_fifos_count not tracking stopped FIFOs HW even if it fixes the reference loss. Non-matching count will alter the stop sequence and could leave bus clock on. I'm not sure about and need to test this.
@ranj063 Wouldn't that remove @lyakh 's add? Instead of in_active flag I wonder if the active fifos count should be replaced with a bit-field. In dmic_stop() the corresponding bit to DAI # is cleared, and set it dmic_start().
Maybe such bitmask "active_fifos_mask" would help to simplify to remove all recently added pause/stop bug fix logic?
There was a problem hiding this comment.
@ranj063 We shouldn't increase fifo-count when state is paused, so that's not right. I'd vote for using a bitfield instead. This would be both easier to understand and also easier to debug than having a counter.
singalsu
left a comment
singalsu
left a comment
There was a problem hiding this comment.
It fixes the issue so please merge. More fixes later.
|
CI showing APL DUT not booting. |
|
@lgirdwood Yes for rc3/final (the patch this is fixing is in rc2 so this should go as well) |
The 'in_active' parameter of dmic_stop() is used to control whether
the fifo should be released or not. In case of COMP_TRIGGER_PAUSE,
this should be false and the 'dmic_active_fifos' count should not
be modified.
Without this fix:
PAUSED -> RELEASE -> dmic_start() -> no change on dmic_active_fifos
ACTIVE -> PAUSE -> dmic_stop() -> dmic_active_fifos--
If a test case repeatedly pauses and releases, 'dmic_active_fifos'
will go out-of-sync.
Fix the issue that dmic_stop for PAUSE does not release the fifo
reference.
Fixes: 2273174 ("dmic: don't decrement active FIFO count below 0")
Signed-off-by: Kai Vehmanen kai.vehmanen@linux.intel.com