Skip to content

Conversation

@tmleman
Copy link
Contributor

@tmleman tmleman commented Feb 10, 2022

Regularly used flow when removing pipeline:

  1. Set ppl state PAUSED,
  2. Unbinding modules,
  3. Set ppl state RESET,
  4. Deleting Pipeline

Changes made:

  • allowing to unbind modules under the same pipeline. It should be ignored under ipc4, but not cause an error.
  • changing modules state during pipe reset, without this FW reports error at pipeline deletion.

@lgirdwood lgirdwood added this to the v2.1 milestone Feb 10, 2022
@lgirdwood
Copy link
Member

@marcinszkudlinski

Copy link
Collaborator

@ranj063 ranj063 Feb 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tmleman can I ask you a question regarding the unbinding of modules, lets say you have a topology like this:
tplg

And pipeline 12 is stopped while pipeline 1 is still active. Should we unbind mixin12.1 and mixout.2.1 before pipeline 12 is deleted? Would it cause any issues because mixout.2.1 is in active state still?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made some research and its was my mistake to assume that unbind of modules under the same pipeline should be normally processed.
Documentation informs that this operation should be ignored, so error reporting via IPC is also incorrect.
@RanderWang will returning 0 and adding a warning in the log will be enough?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tmleman I didnt get what you mean by returning 0? in which case would be return 0? when 2 modules belong to different pipelines?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return 0 instead of the error code as it has been done so far. Change is for case when modules belong to the same pipeline.

Copy link
Collaborator

@RanderWang RanderWang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on windows. Is pipaline a typo ?

@tmleman tmleman force-pushed the topic/ppl_reset_fix branch 2 times, most recently from 59001fc to 3cfae37 Compare February 11, 2022 14:56
When both module instances are parts of the same pipeline Unbind IPC
would be ignored by FW since FW does not support changing internal
topology of pipeline during run-time. The only way to change pipeline
topology is to delete the whole pipeline and create it in modified form.

Signed-off-by: Tomasz Leman <tomasz.m.leman@intel.com>
During pipeline reset all components remains in previous state. This
cause issue at pipeline deletion. This patch allowed to propagate state
transition to all pipeline components.

Signed-off-by: Tomasz Leman <tomasz.m.leman@intel.com>
@tmleman tmleman force-pushed the topic/ppl_reset_fix branch from 3cfae37 to 84c53f2 Compare February 14, 2022 10:02
@tmleman tmleman requested a review from RanderWang February 14, 2022 10:06
@lgirdwood
Copy link
Member

CI showing known and unrelated issues.

@lgirdwood lgirdwood merged commit 819c023 into thesofproject:main Feb 14, 2022
@RanderWang
Copy link
Collaborator

@tmleman for commit ipc4: fix pipeline reset I found a regression issue on windows. The problem is propagate_state_to_ppl_comp set component to reset and pipeline_reset will also set component to reset then a error will returned that the component is already set to reset. I want to know in which case pipeline_reset() can't set component to reset state ? It should set them to reset state and don't need to explicitly set it to reset

		ret = propagate_state_to_ppl_comp(ipc, id, COMP_TRIGGER_RESET);
		if (ret != 0)
			return ret;

		/* resource is not released by triggering reset which is used by current FW */
		ret = pipeline_reset(host->cd->pipeline, host->cd);

@tmleman
Copy link
Contributor Author

tmleman commented Mar 8, 2022

@RanderWang function propagate_state_to_ppl_comp was added because pipeline_reset did not set reset state to pipeline components. If that has changed this function is no longer necessary.

@RanderWang
Copy link
Collaborator

@tmleman do you have any test case for me to validate it ? thanks. If none, I will remove this function

@tmleman
Copy link
Contributor Author

tmleman commented Mar 9, 2022

@RanderWang can you point to a change that changes the behavior of pipeline_reset? I can't find in history in which cases problem described on commit message was visible.
I think it is best to let this change pass through our CI on some dev build.

@RanderWang
Copy link
Collaborator

@tmleman please check pipeline_reset function. I don't make any change but it should call comp_reset to set component state to reset. So I want to check your case to find why it doesn't work.

pipeline_reset
----  pipeline_comp_reset
-------- comp_reset

@tmleman
Copy link
Contributor Author

tmleman commented Mar 10, 2022

@RanderWang earlier pipeline components was not set to RESET state and that caused error during pipeline delete. I don't see this behavior right now. My fault I didn't see pipeline_comp_reset. I think you can delete propagate_state_to_ppl_comp function.

@RanderWang
Copy link
Collaborator

@tmleman thanks for your explanation. I will do it.

@tmleman tmleman deleted the topic/ppl_reset_fix branch November 29, 2022 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants