[WIP]audio: src: update component's value validation logic #3349
Conversation
In src component structure, for it to be valid, the source rate and sink rate values aren't supposed to be zero. The current logic check marks it as an error only if both are zero, but even if one is zero, it's an error. The new logic updates the check for the same. Signed-off-by: Mohana Datta Yelugoti <ymdatta.work@gmail.com>
|
Fixes: #3335 |
|
I thought only one was defined by the tplg and the other came from the ssp or hw params? |
|
@cujomalainey : I think both are defined by the tplg. If you take a look at this line: tplg_parser#1519, here count is the number of tokens (in this case 2, one for SOF_TKN_SRC_RATE_IN which sets source_rate and other SOF_TKN_SRC_RATE_OUT which sets sink_rate). At the end of function sof_parse_string_tokens both source_rate and sink_rate are set. |
|
@ymdatta data doesn't match the defined topologies https://github.com/thesofproject/sof/blob/master/tools/topology/sof/pipe-asrc-playback.m4 I think its supposed to be open ended so userspace can playback any framerate and component converts to the required format. |
|
@ymdatta looks like testbench is failing here to load the SRC on Travis CI, do we need to include an update for testbench too ? |
|
@lgirdwood this won't be the final solution as it restricts the original design of the src. We are working on finding the correct solution |
| /* validate init data - either SRC sink or source rate must be set */ | ||
| if (ipc_src->source_rate == 0 && ipc_src->sink_rate == 0) { | ||
| if (ipc_src->source_rate == 0 || ipc_src->sink_rate == 0) { | ||
| comp_cl_err(&comp_src, "src_new(): SRC sink and source rate are not set"); | ||
| return NULL; | ||
| } |
There was a problem hiding this comment.
please change condition in error message also, and -> or
There was a problem hiding this comment.
HI @ktrzcinx : This is not the final solution. Once it's decided, will update the error messages accordingly.
There was a problem hiding this comment.
ok, can we have an inline comment (as a reminder) that this is intermediate solution and the final solution is pending on X.
There was a problem hiding this comment.
Yes. I will do that.
There was a problem hiding this comment.
Be careful! In topologies load in new() the the fixed side rate is set only, the variable side (from host or PCM params) is left to zero. Have you tested this in a real device with real topology?
There was a problem hiding this comment.
As far as I know there's no CI test for SRC. The testbench run overrides the topology for rates so it's not the same as IPC from linux kernel.
There was a problem hiding this comment.
@singalsu : Sorry for the late reply. I didn't test this on a real device with real topology. But, the logic i have used here (&& -> ||) is wrong. I have to change this.
|
@cujomalainey @ymdatta @singalsu any update ? I guess we can test this now with the UUID testbench PR ? |
|
@lgirdwood : Still working on it. The logic isn't right yet. I will change the PR title to include WIP tag and will remove it once the logic is fixed. |
ymdatta
changed the title
|
Lets close this and leave the bug open as this change I think restricts the intended usage of the SRC. |
5 participants
In src component structure, for it to be valid, the source rate
and sink rate values aren't supposed to be zero.
The current logic check marks it as an error only if both
are zero, but even if one is zero, it's an error. The new
logic updates the check for the same.
Signed-off-by: Mohana Datta Yelugoti ymdatta.work@gmail.com