soundwire: stream: skip m_rt which is not in preparing stage #5246
Conversation
drivers/soundwire/stream.c
Outdated
There was a problem hiding this comment.
I wonder why this is not jumping to restore_params?
There was a problem hiding this comment.
We didn't change bus->params at this point yet.
drivers/soundwire/stream.c
Outdated
There was a problem hiding this comment.
I think in other patches we also moved the set to PREPARED state prior that we actually prepare things.
I got the reason and at the same time it sounds wrong to set something as PREPARED when it is not...
There was a problem hiding this comment.
I agree that the stream->state may not represent the actual state perfectly. But, I can't find a better way to skip the m_rt that should not be counted yet. And stream->state is used by the stream helpers only and the helpers will be called one by one. So, I think it is safe to change the stream->state earlier in the same stream helper.
There was a problem hiding this comment.
@bardliao this really seems like a hack or a workaround. Maybe the best thing to do would be to revisit the state machine so that we can gracefully skip the runtime thats not ready to be included yet?
There was a problem hiding this comment.
I think the issue manifests because sdw_prepare_stream() looks at the m_rt associated with the stream but sdw_compute_group_params() looks at the bus->m_rt_list. Can you maybe filter this list in this function by passing the stream as an argument when called from sdw_prepare_stream()?
There was a problem hiding this comment.
I thought about that. sdw_compute_group_params() lools at the bus->m_rt_list is because it needs to calculate all the active stream's bandwidth in the bus. We assume that all m_rt in the bus are active, but that is not true. And we did similar change in d7722a643421. That's why I choose to set the PREPARE state earlier.
But I am fine with the passing stream solution, too. PR updated.
bardliao
force-pushed
the
fix-multilane
branch
2 times, most recently
from
819dda7 to
6628b7a
Compare
There was a problem hiding this comment.
and this is not intuitive at all. What are you trying to say here? Ignore a runtime thats not associated with the stream if it has already been activated once?
There was a problem hiding this comment.
No, to the opposite. Only calculate the bandwidth of the active streams, and add the bandwidth of the current stream.
There was a problem hiding this comment.
ok then the check for != ENABLED makes sense. but why also check for != DISABLED? Is this the case with open but paused streams? Anyway, I think this needs a comment here with the explanation. And the commit message needs to be modified too based on whatever explanation you add here.
There was a problem hiding this comment.
bardliao
force-pushed
the
fix-multilane
branch
from
6628b7a to
82126db
Compare
There was a problem hiding this comment.
no need for an else after a continue btw
There was a problem hiding this comment.
no need for an else after a continue btw
We still need an else because it will not continue if m_rt->stream == stream and stream->state == SDW_STREAM_CONFIGURED and it will then continue because m_rt->stream->state != SDW_STREAM_ENABLED && m_rt->stream->state != SDW_STREAM_DISABLED.
We only want to check m_rt->stream->state != SDW_STREAM_ENABLED && m_rt->stream->state != SDW_STREAM_DISABLED when the m_rt->stream != stream.
bardliao
force-pushed
the
fix-multilane
branch
from
82126db to
5075d87
Compare
There was a problem hiding this comment.
a better comment would be to include the runtime only during prepare
There was a problem hiding this comment.
@bardliao I think looking at the enum definition, its easy to see this. A better explanation would be something like "Include runtimes with running (ENABLED state) and paused (DISABLED state) streams"
bardliao
force-pushed
the
fix-multilane
branch
from
5075d87 to
904a483
Compare
|
@bardliao there's a build error in qcom.c. You'll need to fix it too because you changed the signature |
The stream parameter will be used in the follow up commit. No function change. Signed-off-by: Bard Liao <[email protected]>
bardliao
force-pushed
the
fix-multilane
branch
from
904a483 to
924e9f9
Compare
Thanks for pointing this out. Fix it now. |
…e streams only sdw_compute_group_params() should only count payload bandwidth of the active streams which is in the ENABLED and DISABLED state in the bus. And add the payload bandwidth of the stream that calls sdw_compute_group_params() in sdw_prepare_stream(). Signed-off-by: Bard Liao <[email protected]>
bardliao
force-pushed
the
fix-multilane
branch
from
924e9f9 to
6f676f9
Compare
|
SOFCI TEST |
3 participants
sdw_compute_group_params() will count payload bandwidth and it count all m_rt in the bus. The m_rt will be added to the bus by sdw_stream_add_master(). If 2 streams are opened simultaneously, the m_rt of the second stream will be counted when
sdw_compute_group_params() is counting the payload bandwidth of the first stream. Which is incorrect.
Set PREPARED state earlier so that sdw_compute_group_params() can skip the m_rt that is not in the preparing stage yet.