dp: make dp_queue using externally provided params set #9063
Conversation
the backdoor removed in this commit has never been used A new backdoor will be introduced in new commit Signed-off-by: Marcin Szkudlinski <marcin.szkudlinski@intel.com>
marcinszkudlinski
requested review from
ranj063,
jxstelter,
lgirdwood,
plbossart,
mmaka1,
lbetlej,
dbaluta and
kv2019i
as code owners
Dp_queue is currently used only as a comp_buffer "shadow" for DP modules. Shadow buffers must have the very same param set as main buffers. Problem: till now the complete vector of params was copied from comp_buffer to dp_queue inm prepare method. Problem is that if anything will change any of params afterwards, the change won't propagate. This commit introduces sharing the param vector between comp_buffer and its shadow Signed-off-by: Marcin Szkudlinski <marcin.szkudlinski@intel.com>
marcinszkudlinski
force-pushed
the
bugfix_dp_params
branch
from
39200b5
to
aa07558
Compare
There was a problem hiding this comment.
@marcinszkudlinski Code looks good. One clarifying question on second patch, please check.
| @@ -176,18 +176,6 @@ struct sof_source *dp_queue_get_source(struct dp_queue *dp_queue) | |||
| return &dp_queue->_source_api; | |||
There was a problem hiding this comment.
I think to avoid unncessary concerns, I'd avoid use of word "backdoor". Somehow a github commit to "add a backdoor" can raise unwanted attention. "Side interface" perhaps?
There was a problem hiding this comment.
as you see this code is removed by this commit ;)
| dp_queue = dp_queue_create(min_available, min_free_space, dp_mode, | ||
| buf_get_id(source_buffer)); | ||
| buf_get_id(source_buffer), | ||
| &source_buffer->stream.runtime_stream_params); |
There was a problem hiding this comment.
Just to double-check: given the pointer to object is owned outside, how to we handle case where the buffer object is freed? I think module reset is ok, but how about unbind?
There was a problem hiding this comment.
Good question, and there may be more problematic places...
The solution would be moving management of shadow dpQueues from "prepare/reset" to "bind/unbind", to keep them always strictly attached to each other.
There was a problem hiding this comment.
@marcinszkudlinski I guess you have this as a follow up PR ? or is it more complex than just moving the logic ?
There was a problem hiding this comment.
@lgirdwood At the moment we can proceed with this PR, as it won't make anything worse, and is fixing some scenarios
if comp_buffer is freed before dp the system is in big trouble anyway
There was a problem hiding this comment.
ok with this @marcinszkudlinski . can you add some FIXME/TODO entry and/or a bug on this?
There was a problem hiding this comment.
I have a following PR almost ready.
There was a problem hiding this comment.
@marcinszkudlinski ok, merged you can send next PR on top of this one.
Dp_queue is currently used only as a comp_buffer "shadow" for DP modules.
Shadow buffers must have the very same param set as main buffers.
Problem: till now the complete vector of params was copied from comp_buffer to dp_queue inm prepare method.
If anything will change any of params afterwards, the change won't propagate.
This commit introduces sharing the param vector between comp_buffer and its shadow