fixup! ASoC: SOF: Add support for dynamic pipelines #2897
Conversation
ranj063
requested review from
bardliao,
dbaluta,
kv2019i,
lgirdwood and
plbossart
as code owners
|
Could you explain a bit why this is needed? Is this fixing something? |
@lyakh this doesnt fix anything but it is in preparation for IPC4. Without this change, we'd end up having an ugly check for ABI and keeping both sequences to support the existing sequence for IPC3 and IPC4 with dynamic pipelines. So, while there are no topologies upstream with dynamic pipelines, making this change will avoid that check |
sound/soc/sof/sof-audio.c
Outdated
There was a problem hiding this comment.
@ranj063 a little bit concern about the error path here, we have 5 failure cases and will jump to this same label here, is the flow below can handle all these 5 failures correctly? Does the num_widgets have enough information for us? If no, we might hit swidget->use_count imbalanced issue?
There was a problem hiding this comment.
@keyonjie yes, the same error path is good enough for all failures. num_widgets represents the number of widgets that are set up successfully and in case of ay error, only those set up successfully need to be freed.
@ranj063 does it qualify for a "fixup!" then? It sounds like a part of a new development to me, a new feature support. It could be just a part of that IPC4 work right? Or it could be just a separate "preparation" PR. Even if for some reason you really want to push it as a "fixup!"-style change to an outstanding patch before its upstreaming, I'd much rather have a proper commit message instead of "fix" with no description of what problem it's actually fixing |
|
@lyakh wrote:
But we have dynamic pipelines already merged to kernel, so the feature is already there even if topologies are not utilizing it. I think we need renable dynamic pipeline ASAP after 1.8 is released to reenable testing. Otherwise we have untested code in sof-dev, which is not sustainable. |
@lyakh it is a fixup only because the original patch is still not upstream. @kv2019i yes, I am going to reenable dynamic pipelines for testing very soon, one this PR is in. |
|
and oh as for the commit message, I dont know if it is worth it because it will be removed when the patches are squashed anyway. So, I suupose the discussion here is enough? |
ranj063
force-pushed
the
fix/pipelie_setup
branch
from
07e9a76 to
b9ad537
Compare
plbossart
left a comment
plbossart
left a comment
There was a problem hiding this comment.
still not fully clear on refcounts
Fix the sof_widget_list_setup() to set up the pipeline widget for the pipeline that the widget belongs to first before setting up the widget. This change is in preparation for IPC4 which requires the pipeline widget to be set up first. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
ranj063
force-pushed
the
fix/pipelie_setup
branch
from
b9ad537 to
d399ca6
Compare
|
CI failure seems unrelated, proceeding with merge. |
5 participants
Fix the sof_widget_list_setup() to set up the pipeline widget for the pipeline that the widget belongs to first
before setting up the widget. This change is legitimate after this FW PR: thesofproject/sof#4113