ASoC: SOF: keep prepare/unprepare widgets in sink path - #4022
Conversation
|
Thanks @ranj063 for finding this issue. |
We should unprepare the widget if its use_count = 1. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
The existing code return when a widget doesn't need to prepare/unprepare. This will prevent widgets in the sink path from being prepared/unprepared. Link: thesofproject#4021 Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
|
|
||
| /* skip if the widget is in use or if it is already unprepared */ | ||
| if (!swidget->prepared || swidget->use_count > 0) | ||
| if (swidget->use_count > 0) |
There was a problem hiding this comment.
commit title should be ASoC: SOF: sof-audio: remove swidget->prepared flag
| goto sink_prepare; | ||
|
|
||
| if (!widget_ops[widget->id].ipc_prepare || swidget->prepared) | ||
| if (!widget_ops[widget->id].ipc_prepare && !swidget->use_count) |
There was a problem hiding this comment.
This works as long as we have always prepare followed by setup (where the reference count is increased).
If this is not the case and we can have multiple calls to prepare, then this would not work.
In the first case, there should be a deeper explanation in the commit message as to why this is ok.
There was a problem hiding this comment.
@plbossart @ranj063 On the second thought, I think it is better to keep the prepared flag. Think about if a widget is prepared, but fail to setup. The use_count will be 0, but it is prepared. It will likely be a problem.
There was a problem hiding this comment.
But @bardliao don't we call unprepare if widget setup fails?
There was a problem hiding this comment.
But @bardliao don't we call unprepare if widget setup fails?
Yes, we call sof_walk_widgets_in_order SOF_WIDGET_UNPREPARE if sof_walk_widgets_in_order SOF_WIDGET_SETUP failed. But what if one of the widget is failed and others are success? Those widgets that have already set up will not be unprepared since their use_count is 1. My point is that prepare and setup are handled in different helper functions, someone can easily break it even if it works well now.
There was a problem hiding this comment.
@bardliao when widget setup fails, we unroll and free all widgets that were previously set up in sof_set_up_widgets_in_path(). I am fairly certain the prepare flag is redundant
There was a problem hiding this comment.
But this PR is good enough for what it was intended for. We should follow up with another PR to remove the prepared flag
|
|
||
| /* return if the widget is in use or if it is already unprepared */ | ||
| if (!swidget->prepared || swidget->use_count > 1) | ||
| if (!swidget->prepared || swidget->use_count > 0) |
There was a problem hiding this comment.
Can the use_count go negative?
if (!swidget->prepared || swidget->use_count)There was a problem hiding this comment.
Can the use_count go negative?
if (!swidget->prepared || swidget->use_count)
Ideally, no. But I do see it is negative when I meet IPC errors. And sof_widget_free() will never work at that time.
|
@bardliao can you please take a look at the IPC4 device test failures |
|
SOFCI TEST |
5 participants
The existing code return when a widget doesn't need to prepare/unprepare. This will prevent widgets in the sink path from being prepared/unprepared.
Signed-off-by: Bard Liao yung-chuan.liao@linux.intel.com
Fixes: #4021