[PATCH] soundwire: stream: only change state if needed
In a multi-cpu DAI context, the stream routines may be called from multiple DAI callbacks. Make sure the stream state only changes for the first call, and don't return error messages if the target state is already reached.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- drivers/soundwire/stream.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)
diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c index 1b43d03c79ea..3319121cd706 100644 --- a/drivers/soundwire/stream.c +++ b/drivers/soundwire/stream.c @@ -1572,6 +1572,7 @@ int sdw_prepare_stream(struct sdw_stream_runtime *stream) sdw_acquire_bus_lock(stream);
if (stream->state == SDW_STREAM_PREPARED) { + /* nothing to do */ ret = 0; goto state_err; } @@ -1661,6 +1662,12 @@ int sdw_enable_stream(struct sdw_stream_runtime *stream)
sdw_acquire_bus_lock(stream);
+ if (stream->state == SDW_STREAM_ENABLED) { + /* nothing to do */ + ret = 0; + goto state_err; + } + if (stream->state != SDW_STREAM_PREPARED && stream->state != SDW_STREAM_DISABLED) { pr_err("%s: %s: inconsistent state state %d\n", @@ -1744,6 +1751,12 @@ int sdw_disable_stream(struct sdw_stream_runtime *stream)
sdw_acquire_bus_lock(stream);
+ if (stream->state == SDW_STREAM_DISABLED) { + /* nothing to do */ + ret = 0; + goto state_err; + } + if (stream->state != SDW_STREAM_ENABLED) { pr_err("%s: %s: inconsistent state state %d\n", __func__, stream->name, stream->state); @@ -1809,6 +1822,12 @@ int sdw_deprepare_stream(struct sdw_stream_runtime *stream)
sdw_acquire_bus_lock(stream);
+ if (stream->state == SDW_STREAM_DEPREPARED) { + /* nothing to do */ + ret = 0; + goto state_err; + } + if (stream->state != SDW_STREAM_PREPARED && stream->state != SDW_STREAM_DISABLED) { pr_err("%s: %s: inconsistent state state %d\n",
Thanks Pierre for this patch,
On 17/03/2020 10:51, Pierre-Louis Bossart wrote:
In a multi-cpu DAI context, the stream routines may be called from multiple DAI callbacks. Make sure the stream state only changes for the first call, and don't return error messages if the target state is already reached.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
drivers/soundwire/stream.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)
This patch did not work for me as it is as wsa881x codec does prepare and enable in one function, which breaks some of the assumptions in this patch.
However with below change I could get it working without moving stream handling to machine driver.
---------------------------->cut<------------------------------- diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c index be71af4671a4..4a94ea64c1c5 100644 --- a/drivers/soundwire/stream.c +++ b/drivers/soundwire/stream.c @@ -1574,7 +1574,8 @@ int sdw_prepare_stream(struct sdw_stream_runtime *stream)
sdw_acquire_bus_lock(stream);
- if (stream->state == SDW_STREAM_PREPARED) { + if (stream->state == SDW_STREAM_PREPARED || + stream->state == SDW_STREAM_ENABLED) { /* nothing to do */ ret = 0; goto state_err; @@ -1754,7 +1755,8 @@ int sdw_disable_stream(struct sdw_stream_runtime *stream)
sdw_acquire_bus_lock(stream);
- if (stream->state == SDW_STREAM_DISABLED) { + if (stream->state == SDW_STREAM_DISABLED || + stream->state == SDW_STREAM_DEPREPARED) { /* nothing to do */ ret = 0; goto state_err; ---------------------------->cut<-------------------------------
--srini
diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c index 1b43d03c79ea..3319121cd706 100644 --- a/drivers/soundwire/stream.c +++ b/drivers/soundwire/stream.c @@ -1572,6 +1572,7 @@ int sdw_prepare_stream(struct sdw_stream_runtime *stream) sdw_acquire_bus_lock(stream);
if (stream->state == SDW_STREAM_PREPARED) {
/* nothing to do */@@ -1661,6 +1662,12 @@ int sdw_enable_stream(struct sdw_stream_runtime *stream)
sdw_acquire_bus_lock(stream);
- if (stream->state == SDW_STREAM_ENABLED) {
/* nothing to do */ret = 0;goto state_err;- }
- if (stream->state != SDW_STREAM_PREPARED && stream->state != SDW_STREAM_DISABLED) { pr_err("%s: %s: inconsistent state state %d\n",
@@ -1744,6 +1751,12 @@ int sdw_disable_stream(struct sdw_stream_runtime *stream)
sdw_acquire_bus_lock(stream);
- if (stream->state == SDW_STREAM_DISABLED) {
/* nothing to do */ret = 0;goto state_err;- }
- if (stream->state != SDW_STREAM_ENABLED) { pr_err("%s: %s: inconsistent state state %d\n", __func__, stream->name, stream->state);
@@ -1809,6 +1822,12 @@ int sdw_deprepare_stream(struct sdw_stream_runtime *stream)
sdw_acquire_bus_lock(stream);
- if (stream->state == SDW_STREAM_DEPREPARED) {
/* nothing to do */ret = 0;goto state_err;- }
- if (stream->state != SDW_STREAM_PREPARED && stream->state != SDW_STREAM_DISABLED) { pr_err("%s: %s: inconsistent state state %d\n",
Hi Srinivas,
This patch did not work for me as it is as wsa881x codec does prepare and enable in one function, which breaks some of the assumptions in this patch.
Ah yes, if two transitions happen in the same DAI callback that wouldn't work indeed. We should probably add this restriction to the state machine documentation, the suggested mapping from ASoC DAI states to stream states did not account for compound cases.
However with below change I could get it working without moving stream handling to machine driver.
The change below would be an error case for Intel, so it's probably better if we go with your suggestion. You have a very specific state handling due to your power amps and it's probably better to keep it platform-specific.
Can you confirm though that this patch works fine if you move all the stream transitions to the machine driver? That should be a no-op but better make sure there's no misunderstanding.
Thanks -Pierre
---------------------------->cut<------------------------------- diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c index be71af4671a4..4a94ea64c1c5 100644 --- a/drivers/soundwire/stream.c +++ b/drivers/soundwire/stream.c @@ -1574,7 +1574,8 @@ int sdw_prepare_stream(struct sdw_stream_runtime *stream)
sdw_acquire_bus_lock(stream);
- if (stream->state == SDW_STREAM_PREPARED) { + if (stream->state == SDW_STREAM_PREPARED || + stream->state == SDW_STREAM_ENABLED) { /* nothing to do */ ret = 0; goto state_err; @@ -1754,7 +1755,8 @@ int sdw_disable_stream(struct sdw_stream_runtime *stream)
sdw_acquire_bus_lock(stream);
- if (stream->state == SDW_STREAM_DISABLED) { + if (stream->state == SDW_STREAM_DISABLED || + stream->state == SDW_STREAM_DEPREPARED) { /* nothing to do */ ret = 0; goto state_err; ---------------------------->cut<-------------------------------
--srini
diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c index 1b43d03c79ea..3319121cd706 100644 --- a/drivers/soundwire/stream.c +++ b/drivers/soundwire/stream.c @@ -1572,6 +1572,7 @@ int sdw_prepare_stream(struct sdw_stream_runtime *stream) sdw_acquire_bus_lock(stream); if (stream->state == SDW_STREAM_PREPARED) { + /* nothing to do */ ret = 0; goto state_err; } @@ -1661,6 +1662,12 @@ int sdw_enable_stream(struct sdw_stream_runtime *stream) sdw_acquire_bus_lock(stream); + if (stream->state == SDW_STREAM_ENABLED) { + /* nothing to do */ + ret = 0; + goto state_err; + }
if (stream->state != SDW_STREAM_PREPARED && stream->state != SDW_STREAM_DISABLED) { pr_err("%s: %s: inconsistent state state %d\n", @@ -1744,6 +1751,12 @@ int sdw_disable_stream(struct sdw_stream_runtime *stream) sdw_acquire_bus_lock(stream); + if (stream->state == SDW_STREAM_DISABLED) { + /* nothing to do */ + ret = 0; + goto state_err; + }
if (stream->state != SDW_STREAM_ENABLED) { pr_err("%s: %s: inconsistent state state %d\n", __func__, stream->name, stream->state); @@ -1809,6 +1822,12 @@ int sdw_deprepare_stream(struct sdw_stream_runtime *stream) sdw_acquire_bus_lock(stream); + if (stream->state == SDW_STREAM_DEPREPARED) { + /* nothing to do */ + ret = 0; + goto state_err; + }
if (stream->state != SDW_STREAM_PREPARED && stream->state != SDW_STREAM_DISABLED) { pr_err("%s: %s: inconsistent state state %d\n",
On 17/03/2020 12:22, Pierre-Louis Bossart wrote:
Hi Srinivas,
This patch did not work for me as it is as wsa881x codec does prepare and enable in one function, which breaks some of the assumptions in this patch.
Ah yes, if two transitions happen in the same DAI callback that wouldn't work indeed. We should probably add this restriction to the state machine documentation, the suggested mapping from ASoC DAI states to stream states did not account for compound cases.
However with below change I could get it working without moving stream handling to machine driver.
The change below would be an error case for Intel, so it's probably better if we go with your suggestion. You have a very specific state handling due to your power amps and it's probably better to keep it platform-specific.
Can you confirm though that this patch works fine if you move all the stream transitions to the machine driver? That should be a no-op but better make sure there's no misunderstanding.
yes, it works with this patch + moving wsa stream handing to machine driver.
--srini
Thanks -Pierre
On 17/03/2020 12:22, Pierre-Louis Bossart wrote:
The change below would be an error case for Intel, so it's probably better if we go with your suggestion. You have a very specific state handling due to your power amps and it's probably better to keep it platform-specific.
Just trying to understand, why would it be error for Intel case?
IMO, If stream state is SDW_STREAM_ENABLED that also implicit that its prepared too. Similar thing with SDW_STREAM_DEPREPARED. Isn't it?
--srini
On 3/17/20 8:04 AM, Srinivas Kandagatla wrote:
On 17/03/2020 12:22, Pierre-Louis Bossart wrote:
The change below would be an error case for Intel, so it's probably better if we go with your suggestion. You have a very specific state handling due to your power amps and it's probably better to keep it platform-specific.
Just trying to understand, why would it be error for Intel case?
IMO, If stream state is SDW_STREAM_ENABLED that also implicit that its prepared too. Similar thing with SDW_STREAM_DEPREPARED. Isn't it?
the stream state is a scalar value, not a mask. The state machine only allows transition from CONFIGURED TO PREPARED or from DEPREPARED TO PREPARED, or DISABLED to PREPARED. There is no allowed transition from ENABLED TO PREPARED, you have to go through the DISABLED state and make sure a bank switch occurred, and re-do a bank switch to prepare again.
On 17/03/2020 13:19, Pierre-Louis Bossart wrote:
On 3/17/20 8:04 AM, Srinivas Kandagatla wrote:
On 17/03/2020 12:22, Pierre-Louis Bossart wrote:
The change below would be an error case for Intel, so it's probably better if we go with your suggestion. You have a very specific state handling due to your power amps and it's probably better to keep it platform-specific.
Just trying to understand, why would it be error for Intel case?
IMO, If stream state is SDW_STREAM_ENABLED that also implicit that its prepared too. Similar thing with SDW_STREAM_DEPREPARED. Isn't it?
the stream state is a scalar value, not a mask. The state machine only allows transition from CONFIGURED TO PREPARED or from DEPREPARED TO PREPARED, or DISABLED to PREPARED. There is no allowed transition from ENABLED TO PREPARED, you have to go through the DISABLED state and make sure a bank switch occurred, and re-do a bank switch to prepare again.
I agree with you if are on single dai case. Am happy to move to stream handling to machine driver for now.
But this also means that in cases like multi-codec its not recommended to call sdw_prepare and sdw_enable in a single function from codec. Which might be worth documenting.
--srini
The change below would be an error case for Intel, so it's probably better if we go with your suggestion. You have a very specific state handling due to your power amps and it's probably better to keep it platform-specific.
Just trying to understand, why would it be error for Intel case?
IMO, If stream state is SDW_STREAM_ENABLED that also implicit that its prepared too. Similar thing with SDW_STREAM_DEPREPARED. Isn't it?
the stream state is a scalar value, not a mask. The state machine only allows transition from CONFIGURED TO PREPARED or from DEPREPARED TO PREPARED, or DISABLED to PREPARED. There is no allowed transition from ENABLED TO PREPARED, you have to go through the DISABLED state and make sure a bank switch occurred, and re-do a bank switch to prepare again.
I agree with you if are on single dai case. Am happy to move to stream handling to machine driver for now.
But this also means that in cases like multi-codec its not recommended to call sdw_prepare and sdw_enable in a single function from codec. Which might be worth documenting.
Well, the bigger question is why use sdw_stream functions at the codec level in the first place? All other codec drivers seem to have no issue leaving the dais owned by the master control stream state changes.
I am not saying I object or it's bad, just that there were significant changes in usages of the 'stream' concept since it was introduced, as well as threads in MIPI circles to clarify the prepare/enable dependencies, so it'd be useful to have a complete picture of what different platforms need/want.
On 17-03-20, 05:51, Pierre-Louis Bossart wrote:
In a multi-cpu DAI context, the stream routines may be called from multiple DAI callbacks. Make sure the stream state only changes for the first call, and don't return error messages if the target state is already reached.
For stream-apis we have documented explicitly in Documentation/driver-api/soundwire/stream.rst
"Bus implements below API for allocate a stream which needs to be called once per stream. From ASoC DPCM framework, this stream state maybe linked to .startup() operation.
.. code-block:: c
int sdw_alloc_stream(char * stream_name); "
This is documented for all stream-apis.
This can be resolved by moving the calling of these APIs from master-dais/slave-dais to machine-dais. They are unique in the card.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
drivers/soundwire/stream.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)
diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c index 1b43d03c79ea..3319121cd706 100644 --- a/drivers/soundwire/stream.c +++ b/drivers/soundwire/stream.c @@ -1572,6 +1572,7 @@ int sdw_prepare_stream(struct sdw_stream_runtime *stream) sdw_acquire_bus_lock(stream);
if (stream->state == SDW_STREAM_PREPARED) {
/* nothing to do */@@ -1661,6 +1662,12 @@ int sdw_enable_stream(struct sdw_stream_runtime *stream)
sdw_acquire_bus_lock(stream);
- if (stream->state == SDW_STREAM_ENABLED) {
/* nothing to do */ret = 0;goto state_err;- }
- if (stream->state != SDW_STREAM_PREPARED && stream->state != SDW_STREAM_DISABLED) { pr_err("%s: %s: inconsistent state state %d\n",
@@ -1744,6 +1751,12 @@ int sdw_disable_stream(struct sdw_stream_runtime *stream)
sdw_acquire_bus_lock(stream);
- if (stream->state == SDW_STREAM_DISABLED) {
/* nothing to do */ret = 0;goto state_err;- }
- if (stream->state != SDW_STREAM_ENABLED) { pr_err("%s: %s: inconsistent state state %d\n", __func__, stream->name, stream->state);
@@ -1809,6 +1822,12 @@ int sdw_deprepare_stream(struct sdw_stream_runtime *stream)
sdw_acquire_bus_lock(stream);
- if (stream->state == SDW_STREAM_DEPREPARED) {
/* nothing to do */ret = 0;goto state_err;- }
- if (stream->state != SDW_STREAM_PREPARED && stream->state != SDW_STREAM_DISABLED) { pr_err("%s: %s: inconsistent state state %d\n",
-- 2.20.1
On 3/20/20 9:15 AM, Vinod Koul wrote:
On 17-03-20, 05:51, Pierre-Louis Bossart wrote:
In a multi-cpu DAI context, the stream routines may be called from multiple DAI callbacks. Make sure the stream state only changes for the first call, and don't return error messages if the target state is already reached.
For stream-apis we have documented explicitly in Documentation/driver-api/soundwire/stream.rst
"Bus implements below API for allocate a stream which needs to be called once per stream. From ASoC DPCM framework, this stream state maybe linked to .startup() operation.
.. code-block:: c
int sdw_alloc_stream(char * stream_name); "
This is documented for all stream-apis.
This can be resolved by moving the calling of these APIs from master-dais/slave-dais to machine-dais. They are unique in the card.
this change is about prepare/enable/disable/deprepare, not allocation or startup.
I see no reason to burden the machine driver with all these steps. It's not because QCOM needs this transition that everyone does.
As discussed earlier, QCOM cannot use this functionality because the prepare/enable and disable/deprepare are done in the hw_params and hw_free respectively. This was never the intended use, but Intel let it happen so I'd like you to return the favor. This change has no impact for QCOM and simplifies the Intel solution, so why would you object?
Seriously, your replies on all Intel contributions make me wonder if this is the QCOM/Linaro SoundWire subsystem, or if we are going to find common ground to deal with vastly different underlying architectures?
On 20-03-20, 09:33, Pierre-Louis Bossart wrote:
On 3/20/20 9:15 AM, Vinod Koul wrote:
On 17-03-20, 05:51, Pierre-Louis Bossart wrote:
In a multi-cpu DAI context, the stream routines may be called from multiple DAI callbacks. Make sure the stream state only changes for the first call, and don't return error messages if the target state is already reached.
Again I ask you to read emails carefully and not jump the gun.
For stream-apis we have documented explicitly in Documentation/driver-api/soundwire/stream.rst
< begins with quote from the file stream.rst>
"Bus implements below API for allocate a stream which needs to be called once per stream. From ASoC DPCM framework, this stream state maybe linked to .startup() operation.
.. code-block:: c
int sdw_alloc_stream(char * stream_name); "
<end quote>
This is documented for all stream-apis.
This line is very important. But seems to have skipped
This can be resolved by moving the calling of these APIs from master-dais/slave-dais to machine-dais. They are unique in the card.
This is suggestion, feel free to do anything as long as that conforms to documentation laid out, which incidentally was written by Sanyog and signed off by you. See 89634f99a83e ("Documentation: soundwire: Add more documentation")
this change is about prepare/enable/disable/deprepare, not allocation or startup.
Sad to see this. Now am going to quote for all these, since you skipped the line in above email.
For prepare: "Bus implements below API for PREPARE state which needs to be called once per stream. From ASoC DPCM framework, this stream state is linked to .prepare() operation. Since the .trigger() operations may not follow the .prepare(), a direct transition from ``SDW_STREAM_PREPARED`` to ``SDW_STREAM_DEPREPARED`` is allowed."
See the point about "once per stream".
On Enable: "Bus implements below API for ENABLE state which needs to be called once per stream. From ASoC DPCM framework, this stream state is linked to .trigger() start operation."
See the point about "once per stream".
For Disable: "Bus implements below API for DISABLED state which needs to be called once per stream. From ASoC DPCM framework, this stream state is linked to .trigger() stop operation."
See the point about "once per stream".
For deprepare: "Bus implements below API for DEPREPARED state which needs to be called once per stream. ALSA/ASoC do not have a concept of 'deprepare', and the mapping from this stream state to ALSA/ASoC operation may be implementation specific."
See the point about "once per stream".
I see no reason to burden the machine driver with all these steps. It's not because QCOM needs this transition that everyone does.
As discussed earlier, QCOM cannot use this functionality because the prepare/enable and disable/deprepare are done in the hw_params and hw_free respectively. This was never the intended use, but Intel let it happen so I'd like you to return the favor. This change has no impact for QCOM and simplifies the Intel solution, so why would you object?
Seriously, your replies on all Intel contributions make me wonder if this is the QCOM/Linaro SoundWire subsystem, or if we are going to find common ground to deal with vastly different underlying architectures?
It is extremely sad that you chose to say this and didn't even try to read the email and recheck the documentation you have signed off.
This has nothing to do with Qcom/Linaro. If you look at git tree in Intel you will find that the original implementation was to use these from machine driver. I am sure of this fact as I had written that code.
I am really getting extremely annoyed at your attitude in your conversations and accusing me. This is not nice. My replies are based on documentation and questioning the implementation, which is my job here. I dont care if you feel bad about me questions. I guess you dont like that someone is questioning, but I dont care.
participants (3)
-
Pierre-Louis Bossart -
Srinivas Kandagatla -
Vinod Koul