[PATCH 0/5] soundwire: handle stream at the dailink level
Currently, stream is handled at the dai level. But we have to handle stream at the dailink level in the multi-cpu dailink usage.
Pierre-Louis Bossart (5): soundwire: intel: implement get_sdw_stream() operations soundwire: stream: add helper to startup/shutdown streams soundwire: intel: remove stream allocation/free soundwire: cadence: allocate/free dma_data in set_sdw_stream soundwire: intel: don't free dma_data in DAI shutdown
drivers/soundwire/cadence_master.c | 52 ++++++++++++----- drivers/soundwire/intel.c | 82 +++++++-------------------- drivers/soundwire/stream.c | 90 ++++++++++++++++++++++++++++++ include/linux/soundwire/sdw.h | 2 + 4 files changed, 149 insertions(+), 77 deletions(-)
From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
This is needed to retrieve the information when the stream is allocated at the dai_link level.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Signed-off-by: Bard Liao yung-chuan.liao@linux.intel.com --- drivers/soundwire/intel.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index 13d3877bc6bb..861beb785171 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -1062,6 +1062,22 @@ static int intel_pdm_set_sdw_stream(struct snd_soc_dai *dai, return cdns_set_sdw_stream(dai, stream, false, direction); }
+static void *intel_get_sdw_stream(struct snd_soc_dai *dai, + int direction) +{ + struct sdw_cdns_dma_data *dma; + + if (direction == SNDRV_PCM_STREAM_PLAYBACK) + dma = dai->playback_dma_data; + else + dma = dai->capture_dma_data; + + if (!dma) + return NULL; + + return dma->stream; +} + static const struct snd_soc_dai_ops intel_pcm_dai_ops = { .startup = intel_startup, .hw_params = intel_hw_params, @@ -1070,6 +1086,7 @@ static const struct snd_soc_dai_ops intel_pcm_dai_ops = { .hw_free = intel_hw_free, .shutdown = intel_shutdown, .set_sdw_stream = intel_pcm_set_sdw_stream, + .get_sdw_stream = intel_get_sdw_stream, };
static const struct snd_soc_dai_ops intel_pdm_dai_ops = { @@ -1080,6 +1097,7 @@ static const struct snd_soc_dai_ops intel_pdm_dai_ops = { .hw_free = intel_hw_free, .shutdown = intel_shutdown, .set_sdw_stream = intel_pdm_set_sdw_stream, + .get_sdw_stream = intel_get_sdw_stream, };
static const struct snd_soc_component_driver dai_component = {
From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
To handle streams at the dailink level, expose two helpers that will be called from machine drivers.
Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Guennadi Liakhovetski guennadi.liakhovetski@linux.intel.com Reviewed-by: Kai Vehmanen kai.vehmanen@linux.intel.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Signed-off-by: Bard Liao yung-chuan.liao@linux.intel.com --- drivers/soundwire/stream.c | 90 +++++++++++++++++++++++++++++++++++ include/linux/soundwire/sdw.h | 2 + 2 files changed, 92 insertions(+)
diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c index a9a72574b34a..48daf4b48f51 100644 --- a/drivers/soundwire/stream.c +++ b/drivers/soundwire/stream.c @@ -13,6 +13,9 @@ #include <linux/slab.h> #include <linux/soundwire/sdw_registers.h> #include <linux/soundwire/sdw.h> +#include <sound/core.h> +#include <sound/pcm.h> +#include <sound/soc.h> #include "bus.h"
/* @@ -1826,3 +1829,90 @@ int sdw_deprepare_stream(struct sdw_stream_runtime *stream) return ret; } EXPORT_SYMBOL(sdw_deprepare_stream); + +static int set_stream(struct snd_pcm_substream *substream, + struct sdw_stream_runtime *sdw_stream) +{ + struct snd_soc_pcm_runtime *rtd = substream->private_data; + struct snd_soc_dai *dai; + int ret = 0; + int i; + + /* Set stream pointer on all DAIs */ + for_each_rtd_dais(rtd, i, dai) { + ret = snd_soc_dai_set_sdw_stream(dai, sdw_stream, + substream->stream); + if (ret < 0) { + dev_err(rtd->dev, + "failed to set stream pointer on dai %s", + dai->name); + break; + } + } + + return ret; +} + +int sdw_startup_stream(void *sdw_substream) +{ + struct snd_pcm_substream *substream = sdw_substream; + struct snd_soc_pcm_runtime *rtd = substream->private_data; + struct sdw_stream_runtime *sdw_stream; + char *name; + int ret; + + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) + name = kasprintf(GFP_KERNEL, "%s-Playback", substream->name); + else + name = kasprintf(GFP_KERNEL, "%s-Capture", substream->name); + + if (!name) + return -ENOMEM; + + sdw_stream = sdw_alloc_stream(name); + if (!sdw_stream) { + dev_err(rtd->dev, "alloc stream failed for substream DAI %s", + substream->name); + ret = -ENOMEM; + goto error; + } + + ret = set_stream(substream, sdw_stream); + if (ret < 0) + goto release_stream; + return 0; + +release_stream: + sdw_release_stream(sdw_stream); + set_stream(substream, NULL); +error: + kfree(name); + return ret; +} +EXPORT_SYMBOL(sdw_startup_stream); + +void sdw_shutdown_stream(void *sdw_substream) +{ + struct snd_pcm_substream *substream = sdw_substream; + struct snd_soc_pcm_runtime *rtd = substream->private_data; + struct sdw_stream_runtime *sdw_stream; + struct snd_soc_dai *dai; + + /* Find stream from first CPU DAI */ + dai = asoc_rtd_to_cpu(rtd, 0); + + sdw_stream = snd_soc_dai_get_sdw_stream(dai, substream->stream); + + if (!sdw_stream) { + dev_err(rtd->dev, "no stream found for DAI %s", dai->name); + return; + } + + /* release memory */ + kfree(sdw_stream->name); + sdw_release_stream(sdw_stream); + + /* clear DAI data */ + set_stream(substream, NULL); +} +EXPORT_SYMBOL(sdw_shutdown_stream); diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h index 9c27a32df9bb..51ecbd8faa8c 100644 --- a/include/linux/soundwire/sdw.h +++ b/include/linux/soundwire/sdw.h @@ -952,10 +952,12 @@ int sdw_stream_remove_master(struct sdw_bus *bus, struct sdw_stream_runtime *stream); int sdw_stream_remove_slave(struct sdw_slave *slave, struct sdw_stream_runtime *stream); +int sdw_startup_stream(void *sdw_substream); int sdw_prepare_stream(struct sdw_stream_runtime *stream); int sdw_enable_stream(struct sdw_stream_runtime *stream); int sdw_disable_stream(struct sdw_stream_runtime *stream); int sdw_deprepare_stream(struct sdw_stream_runtime *stream); +void sdw_shutdown_stream(void *sdw_substream); int sdw_bus_prep_clk_stop(struct sdw_bus *bus); int sdw_bus_clk_stop(struct sdw_bus *bus); int sdw_bus_exit_clk_stop(struct sdw_bus *bus);
On 23-06-20, 05:23, Bard Liao wrote:
From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
To handle streams at the dailink level, expose two helpers that will be called from machine drivers.
Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Guennadi Liakhovetski guennadi.liakhovetski@linux.intel.com Reviewed-by: Kai Vehmanen kai.vehmanen@linux.intel.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Signed-off-by: Bard Liao yung-chuan.liao@linux.intel.com
drivers/soundwire/stream.c | 90 +++++++++++++++++++++++++++++++++++ include/linux/soundwire/sdw.h | 2 + 2 files changed, 92 insertions(+)
diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c index a9a72574b34a..48daf4b48f51 100644 --- a/drivers/soundwire/stream.c +++ b/drivers/soundwire/stream.c @@ -13,6 +13,9 @@ #include <linux/slab.h> #include <linux/soundwire/sdw_registers.h> #include <linux/soundwire/sdw.h> +#include <sound/core.h>
Do we really need core header?
+#include <sound/pcm.h> +#include <sound/soc.h> #include "bus.h"
/* @@ -1826,3 +1829,90 @@ int sdw_deprepare_stream(struct sdw_stream_runtime *stream) return ret; } EXPORT_SYMBOL(sdw_deprepare_stream);
+static int set_stream(struct snd_pcm_substream *substream,
struct sdw_stream_runtime *sdw_stream)
sdw_set_stream() please
+{
- struct snd_soc_pcm_runtime *rtd = substream->private_data;
- struct snd_soc_dai *dai;
- int ret = 0;
- int i;
- /* Set stream pointer on all DAIs */
- for_each_rtd_dais(rtd, i, dai) {
ret = snd_soc_dai_set_sdw_stream(dai, sdw_stream,substream->stream);if (ret < 0) {dev_err(rtd->dev,"failed to set stream pointer on dai %s",dai->name);
lets use one line and shiny new 100 char limit, would make code read better!
break;
So on error should unset of stream pointer be done?
}- }
- return ret;
+}
+int sdw_startup_stream(void *sdw_substream)
Can we have kernel doc style Documentation for exported APIs?
+{
- struct snd_pcm_substream *substream = sdw_substream;
- struct snd_soc_pcm_runtime *rtd = substream->private_data;
- struct sdw_stream_runtime *sdw_stream;
- char *name;
- int ret;
- if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
name = kasprintf(GFP_KERNEL, "%s-Playback", substream->name);- else
name = kasprintf(GFP_KERNEL, "%s-Capture", substream->name);- if (!name)
return -ENOMEM;- sdw_stream = sdw_alloc_stream(name);
- if (!sdw_stream) {
dev_err(rtd->dev, "alloc stream failed for substream DAI %s",substream->name);ret = -ENOMEM;goto error;- }
- ret = set_stream(substream, sdw_stream);
- if (ret < 0)
goto release_stream;- return 0;
+release_stream:
- sdw_release_stream(sdw_stream);
- set_stream(substream, NULL);
+error:
- kfree(name);
- return ret;
+} +EXPORT_SYMBOL(sdw_startup_stream);
Thanks Vinod for the review.
--- a/drivers/soundwire/stream.c +++ b/drivers/soundwire/stream.c @@ -13,6 +13,9 @@ #include <linux/slab.h> #include <linux/soundwire/sdw_registers.h> #include <linux/soundwire/sdw.h> +#include <sound/core.h>
Do we really need core header?
No we don't, the only thing needed in sound/soc.h it seems.
+#include <sound/pcm.h> +#include <sound/soc.h> #include "bus.h"
/* @@ -1826,3 +1829,90 @@ int sdw_deprepare_stream(struct sdw_stream_runtime *stream) return ret; } EXPORT_SYMBOL(sdw_deprepare_stream);
+static int set_stream(struct snd_pcm_substream *substream,
struct sdw_stream_runtime *sdw_stream)sdw_set_stream() please
it's an internal helper not exported, the choice of not adding a prefix was intentional to avoid confusion with exported ones.
+{
- struct snd_soc_pcm_runtime *rtd = substream->private_data;
- struct snd_soc_dai *dai;
- int ret = 0;
- int i;
- /* Set stream pointer on all DAIs */
- for_each_rtd_dais(rtd, i, dai) {
ret = snd_soc_dai_set_sdw_stream(dai, sdw_stream,substream->stream);if (ret < 0) {dev_err(rtd->dev,"failed to set stream pointer on dai %s",dai->name);lets use one line and shiny new 100 char limit, would make code read better!
yes
break;So on error should unset of stream pointer be done?
it's already done below, see [1]
}- }
- return ret;
+}
+int sdw_startup_stream(void *sdw_substream)
Can we have kernel doc style Documentation for exported APIs?
yes, that's a miss indeed.
Though if we follow the existing examples it's not going to be very informative, e.g.
/** * sdw_disable_stream() - Disable SoundWire stream * * @stream: Soundwire stream * * Documentation/driver-api/soundwire/stream.rst explains this API in detail */
+{
- struct snd_pcm_substream *substream = sdw_substream;
- struct snd_soc_pcm_runtime *rtd = substream->private_data;
- struct sdw_stream_runtime *sdw_stream;
- char *name;
- int ret;
- if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
name = kasprintf(GFP_KERNEL, "%s-Playback", substream->name);- else
name = kasprintf(GFP_KERNEL, "%s-Capture", substream->name);- if (!name)
return -ENOMEM;- sdw_stream = sdw_alloc_stream(name);
- if (!sdw_stream) {
dev_err(rtd->dev, "alloc stream failed for substream DAI %s",substream->name);ret = -ENOMEM;goto error;- }
- ret = set_stream(substream, sdw_stream);
- if (ret < 0)
goto release_stream;
[1]
- return 0;
+release_stream:
- sdw_release_stream(sdw_stream);
- set_stream(substream, NULL);
+error:
- kfree(name);
- return ret;
+} +EXPORT_SYMBOL(sdw_startup_stream);
On 30-06-20, 11:58, Pierre-Louis Bossart wrote:
+int sdw_startup_stream(void *sdw_substream)
Can we have kernel doc style Documentation for exported APIs?
yes, that's a miss indeed.
Though if we follow the existing examples it's not going to be very informative, e.g.
Yeah but...
/**
- sdw_disable_stream() - Disable SoundWire stream
- @stream: Soundwire stream
- Documentation/driver-api/soundwire/stream.rst explains this API in detail
it would help to have this pointer. I plan to include the kernel-doc comments in Documentation for sdw so it would great to have these in place
From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
To support streaming across multiple links, the stream allocation/free needs to be at the dailink level, not the dai.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Signed-off-by: Bard Liao yung-chuan.liao@linux.intel.com --- drivers/soundwire/intel.c | 57 +-------------------------------------- 1 file changed, 1 insertion(+), 56 deletions(-)
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index 861beb785171..72ba3d22daf9 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -804,57 +804,6 @@ static int intel_post_bank_switch(struct sdw_bus *bus) * DAI routines */
-static int sdw_stream_setup(struct snd_pcm_substream *substream, - struct snd_soc_dai *dai) -{ - struct snd_soc_pcm_runtime *rtd = substream->private_data; - struct sdw_stream_runtime *sdw_stream = NULL; - char *name; - int i, ret; - - if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) - name = kasprintf(GFP_KERNEL, "%s-Playback", dai->name); - else - name = kasprintf(GFP_KERNEL, "%s-Capture", dai->name); - - if (!name) - return -ENOMEM; - - sdw_stream = sdw_alloc_stream(name); - if (!sdw_stream) { - dev_err(dai->dev, "alloc stream failed for DAI %s", dai->name); - ret = -ENOMEM; - goto error; - } - - /* Set stream pointer on CPU DAI */ - ret = snd_soc_dai_set_sdw_stream(dai, sdw_stream, substream->stream); - if (ret < 0) { - dev_err(dai->dev, "failed to set stream pointer on cpu dai %s", - dai->name); - goto release_stream; - } - - /* Set stream pointer on all CODEC DAIs */ - for (i = 0; i < rtd->num_codecs; i++) { - ret = snd_soc_dai_set_sdw_stream(asoc_rtd_to_codec(rtd, i), sdw_stream, - substream->stream); - if (ret < 0) { - dev_err(dai->dev, "failed to set stream pointer on codec dai %s", - asoc_rtd_to_codec(rtd, i)->name); - goto release_stream; - } - } - - return 0; - -release_stream: - sdw_release_stream(sdw_stream); -error: - kfree(name); - return ret; -} - static int intel_startup(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) { @@ -862,8 +811,7 @@ static int intel_startup(struct snd_pcm_substream *substream, * TODO: add pm_runtime support here, the startup callback * will make sure the IP is 'active' */ - - return sdw_stream_setup(substream, dai); + return 0; }
static int intel_hw_params(struct snd_pcm_substream *substream, @@ -1031,9 +979,6 @@ intel_hw_free(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) return ret; }
- kfree(dma->stream->name); - sdw_release_stream(dma->stream); - return 0; }
From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
The current memory allocation is somewhat strange: the dma_data is allocated in set_sdw_stream, but released in the intel DAI shutdown. This no longer works with the multi-cpu implementation, since the dma_data is released in the dai shutdown which takes place before the dailink shutdown.
Move to a more symmetric allocation where the dma_data is allocated with non-NULL SoundWire stream, and conversely released when a NULL stream is provided - for consistency with the stream startup and shutdown operations.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Signed-off-by: Bard Liao yung-chuan.liao@linux.intel.com --- drivers/soundwire/cadence_master.c | 52 ++++++++++++++++++++++-------- 1 file changed, 38 insertions(+), 14 deletions(-)
diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index 5c0df066bfc6..24eafe0aa1c3 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -1439,25 +1439,49 @@ int cdns_set_sdw_stream(struct snd_soc_dai *dai, struct sdw_cdns *cdns = snd_soc_dai_get_drvdata(dai); struct sdw_cdns_dma_data *dma;
- dma = kzalloc(sizeof(*dma), GFP_KERNEL); - if (!dma) - return -ENOMEM; + if (stream) { + /* first paranoia check */ + if (direction == SNDRV_PCM_STREAM_PLAYBACK) + dma = dai->playback_dma_data; + else + dma = dai->capture_dma_data;
- if (pcm) - dma->stream_type = SDW_STREAM_PCM; - else - dma->stream_type = SDW_STREAM_PDM; + if (dma) { + dev_err(dai->dev, + "dma_data already allocated for dai %s\n", + dai->name); + return -EINVAL; + }
- dma->bus = &cdns->bus; - dma->link_id = cdns->instance; + /* allocate and set dma info */ + dma = kzalloc(sizeof(*dma), GFP_KERNEL); + if (!dma) + return -ENOMEM;
- dma->stream = stream; + if (pcm) + dma->stream_type = SDW_STREAM_PCM; + else + dma->stream_type = SDW_STREAM_PDM;
- if (direction == SNDRV_PCM_STREAM_PLAYBACK) - dai->playback_dma_data = dma; - else - dai->capture_dma_data = dma; + dma->bus = &cdns->bus; + dma->link_id = cdns->instance;
+ dma->stream = stream; + + if (direction == SNDRV_PCM_STREAM_PLAYBACK) + dai->playback_dma_data = dma; + else + dai->capture_dma_data = dma; + } else { + /* for NULL stream we release allocated dma_data */ + if (direction == SNDRV_PCM_STREAM_PLAYBACK) { + kfree(dai->playback_dma_data); + dai->playback_dma_data = NULL; + } else { + kfree(dai->capture_dma_data); + dai->capture_dma_data = NULL; + } + } return 0; } EXPORT_SYMBOL(cdns_set_sdw_stream);
From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
Now that the DMA data is allocated/freed in set_sdw_stream(), remove free operations.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Signed-off-by: Bard Liao yung-chuan.liao@linux.intel.com --- drivers/soundwire/intel.c | 7 ------- 1 file changed, 7 deletions(-)
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index 72ba3d22daf9..98e98be6c0f8 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -985,14 +985,7 @@ intel_hw_free(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) static void intel_shutdown(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) { - struct sdw_cdns_dma_data *dma;
- dma = snd_soc_dai_get_dma_data(dai, substream); - if (!dma) - return; - - snd_soc_dai_set_dma_data(dai, substream, NULL); - kfree(dma); }
static int intel_pcm_set_sdw_stream(struct snd_soc_dai *dai,
participants (3)
-
Bard Liao -
Pierre-Louis Bossart -
Vinod Koul