Skip to content

Conversation

@ranj063
Copy link
Collaborator

@ranj063 ranj063 commented Apr 28, 2021

Move the code that sets the scheduling comp for
a pipeline to ipc_pipeline_complete(). This removes the
restriction that the scheduling comp must be set up
before the pipeline widget and provides the
flexibility in the kernel to set up the widgets
in any order while parsing topology.

Signed-off-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com

@plbossart
Copy link
Member

@ranj063 is this related to a specific issue or evolution? Also wondering if this means a specific kernel would need a firmware with a minimal version, which could be problematic in terms of backwards compatibility.

@ranj063
Copy link
Collaborator Author

ranj063 commented Apr 28, 2021

@ranj063 is this related to a specific issue or evolution? Also wondering if this means a specific kernel would need a firmware with a minimal version, which could be problematic in terms of backwards compatibility.

@plbossart this is mainly required for IPC4 support with dynamic pipelines. A newer kernel that tries to set up the pipeline widget willl require this patch. So I suppose this calls for an ABI bump

@plbossart
Copy link
Member

@ranj063 is this related to a specific issue or evolution? Also wondering if this means a specific kernel would need a firmware with a minimal version, which could be problematic in terms of backwards compatibility.

@plbossart this is mainly required for IPC4 support with dynamic pipelines. A newer kernel that tries to set up the pipeline widget willl require this patch. So I suppose this calls for an ABI bump

I guessed this was for IPC4, but you modified common code and helper-ipc3.c, so not sure how this might work for current platforms.

@ranj063
Copy link
Collaborator Author

ranj063 commented Apr 28, 2021

I guessed this was for IPC4, but you modified common code and helper-ipc3.c, so not sure how this might work for current platforms.

yes, I suppose this could be in the common helper if things work the same way for IPC3 and IPC4.

@marc-hb
Copy link
Collaborator

marc-hb commented Apr 28, 2021

The https://github.com/thesofproject/sof/pull/4113/checks?check_run_id=2461033508 regression was added by #4111 and is unrelated to this PR

@lgirdwood
Copy link
Member

@ranj063 UT's build failing - best to locally check with ./scripts/run-mocks.sh

[ 76%] Built target find_min_int16
cc1: warnings being treated as errors
/quickbuild/workspace1/24733/SOF_FW/test/cmocka/src/audio/pipeline/pipeline_new.c: In function ‘test_audio_pipeline_pipeline_new_creation’:
/quickbuild/workspace1/24733/SOF_FW/test/cmocka/src/audio/pipeline/pipeline_new.c:50: warning: passing argument 1 of ‘pipeline_new’ makes integer from pointer without a cast
/quickbuild/workspace1/24733/SOF_FW/test/cmocka/src/audio/pipeline/pipeline_new.c:50: error: too many arguments to function ‘pipeline_new’
[ 77%] Building C object test/cmocka/src/audio/pipeline/CMakeFiles/pipeline_free.dir/pipeline_connection_mocks.c.o
test/cmocka/src/audio/pipeline/CMakeFiles/pipeline_new.dir/build.make:63: recipe for target 'test/cmocka/src/audio/pipeline/CMakeFiles/pipeline_new.dir/pipeline_new.c.o' failed
make[2]: *** [test/cmocka/src/audio/pipeline/CMakeFiles/pipeline_new.dir/pipeline_new.c.o] Error 2
make[2]: Leaving directory '/quickbuild/workspace1/24733/SOF_FW/build_ut'
cd /quickbuild/workspace1/24733/SOF_FW/build_ut/test/cmocka/src/audio/pipeline && /localdisk/tools/xtensa/RG-2017.8-linux/XtensaTools/bin/xt-xcc -DUNIT_TEST -D_UINTPTR_T_DEFINED -DRELATIVE_FILE=\"test/cmocka/src/audio/pipeline/pipeline_connection_mocks.c\" -I/quickbuild/workspace1/24733/SOF_FW/test/cmocka/include -I/localdisk/tools/cmocka/build_cavs2x_LX6HiFi3_2017_8/install/include -I/quickbuild/workspace1/24733/SOF_FW/src/arch/xtensa/include -I/quickbuild/workspace1/24733/SOF_FW/src/arch/xtensa/xtos -I/quickbuild/workspace1/24733/SOF_FW/src/platform/tigerlake/include -I/localdisk/tools/xtensa/RG-2017.8-linux/cavs2x_LX6HiFi3_2017_8/xtensa-elf/arch/include -I/localdisk/tools/xtensa/RG-2017.8-linux/cavs2x_LX6HiFi3_2017_8/xtensa-elf/include -I/quickbuild/workspace1/24733/SOF_FW/src/platform/intel/cavs/include -I/quickbuild/workspace1/24733/SOF_FW/src/include -I/quickbuild/workspace1/24733/SOF_FW/rimage/src/include -I/quickbuild/workspace1/24733/SOF_FW/build_ut/generated/include  -fdata-sections -ffunction-sections -fno-inline-functions -mlongcalls -O2 -g -Wall -Werror -Wl,-EL -Wmissing-prototypes -Wpointer-arith -mtext-section-literals -imacros/quickbuild/workspace1/24733/SOF_FW/build_ut/generated/include/autoconfig.h -o CMakeFiles/pipeline_free.dir/pipeline_connection_mocks.c.o   -c /quickbuild/workspace1/24733/SOF_FW/test/cmocka/src/audio/pipeline/pipeline_connection_mocks.c
CMakeFiles/Makefile2:1646: recipe for target 'test/cmocka/src/audio/pipeline/CMakeFiles/pipeline_new.dir/all' failed
make[1]: *** [test/cmocka/src/audio/pipeline/CMakeFiles/pipeline_new.dir/all] Error 2
make[1]: *** Waiting for unfinished jobs....

@lgirdwood
Copy link
Member

@ranj063 I assume you also need a kernel path (perhaps we can use the MINOR ABI) to set the order of the pipeline new IPC in the driver ?

@lgirdwood
Copy link
Member

@juimonen fyi - you will need this PR for pipeline allocator.

@ranj063
Copy link
Collaborator Author

ranj063 commented May 3, 2021

@ranj063 I assume you also need a kernel path (perhaps we can use the MINOR ABI) to set the order of the pipeline new IPC in the driver ?

@lgirdwood do I really need an ABI bump here? This change wont impact the static pipeline topologies that we have today. The hda-generic pipelines are the only ones using dynamic pipelines and Im not sure if they are used in any product. So if and when we switch to dynamic pipeliine in tplg, we can be assured that the FW will already have this change right?

@lgirdwood
Copy link
Member

@ranj063 I assume you also need a kernel path (perhaps we can use the MINOR ABI) to set the order of the pipeline new IPC in the driver ?

@lgirdwood do I really need an ABI bump here? This change wont impact the static pipeline topologies that we have today. The hda-generic pipelines are the only ones using dynamic pipelines and Im not sure if they are used in any product. So if and when we switch to dynamic pipeliine in tplg, we can be assured that the FW will already have this change right?

ok, sounds fine. Btw, lets get this in for v1.8-rc1, just needs the UT fixed. See above comment on running the UTs locally.

@ranj063 ranj063 force-pushed the fix/pipeline_setup branch from 113459b to 1904843 Compare May 5, 2021 20:22
@ranj063
Copy link
Collaborator Author

ranj063 commented May 5, 2021

ok, sounds fine. Btw, lets get this in for v1.8-rc1, just needs the UT fixed. See above comment on running the UTs locally.

@lgirdwood should be fixed now

@ranj063
Copy link
Collaborator Author

ranj063 commented May 5, 2021

this patch should be merged after #4143 though.

@ranj063 ranj063 force-pushed the fix/pipeline_setup branch from 1904843 to d5fe926 Compare May 6, 2021 00:59
@lgirdwood
Copy link
Member

lgirdwood commented May 6, 2021

@ranj063 looks like a pipeline error happening from time to time in QB logs.

[     1745240.026042] (           6.380208) c0 ipc                   src/ipc/helper-ipc3.c:411  ERROR ipc_pipeline_complete(): icd->core (0) != p->core (0) for pipeline scheduling component icd->id 0

@ranj063 ranj063 force-pushed the fix/pipeline_setup branch from d5fe926 to 1b02261 Compare May 6, 2021 15:49
@lgirdwood
Copy link
Member

@ranj063 I'm assuming this is a fix for v1.8 ? if so, we can get into rc2

@ranj063 ranj063 force-pushed the fix/pipeline_setup branch from 1b02261 to 76d0da1 Compare May 6, 2021 16:50
@ranj063
Copy link
Collaborator Author

ranj063 commented May 6, 2021

@zrombel could you please help me understand why the "17_03_TestMultiCoreMultiKernelHda48000hz24b32b2ch" test is failing with this PR?

@ranj063 ranj063 force-pushed the fix/pipeline_setup branch 2 times, most recently from 53c0f8b to 9556303 Compare May 6, 2021 19:40
Move the code that sets the scheduling comp for
a pipeline to allow setting it in ipc_pipeline_complete().
This removes the restriction that the scheduling comp must
be set up before the pipeline widget and provides the
flexibility in the kernel to set up the widgets in any order
while parsing topology.

Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
@lgirdwood lgirdwood merged commit cbf0a1e into thesofproject:main May 8, 2021
@ranj063 ranj063 deleted the fix/pipeline_setup branch May 12, 2021 18:28
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