| Message ID | 20200818024120.20721-8-yung-chuan.liao@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | soundwire: intel: add multi-link support | expand |
On 18-08-20, 10:41, Bard Liao wrote: > We should call these APIs once per stream. So we can only call it > when the dai ops is invoked for the first cpu dai. > > Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com> > Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com> > --- > drivers/soundwire/intel.c | 45 +++++++++++++++++++++++++++++++++------ > 1 file changed, 39 insertions(+), 6 deletions(-) > > diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c > index 89a8ad1f80e8..7c63581270fd 100644 > --- a/drivers/soundwire/intel.c > +++ b/drivers/soundwire/intel.c > @@ -941,11 +941,13 @@ static int intel_hw_params(struct snd_pcm_substream *substream, > static int intel_prepare(struct snd_pcm_substream *substream, > struct snd_soc_dai *dai) > { > + struct snd_soc_pcm_runtime *rtd = substream->private_data; > + struct snd_soc_dai *first_cpu_dai = asoc_rtd_to_cpu(rtd, 0); > struct sdw_cdns *cdns = snd_soc_dai_get_drvdata(dai); > struct sdw_intel *sdw = cdns_to_intel(cdns); > struct sdw_cdns_dma_data *dma; > int ch, dir; > - int ret; > + int ret = 0; > > dma = snd_soc_dai_get_dma_data(dai, substream); > if (!dma) { > @@ -985,7 +987,13 @@ static int intel_prepare(struct snd_pcm_substream *substream, > goto err; > } > > - ret = sdw_prepare_stream(dma->stream); > + /* > + * All cpu dais belong to a stream. To ensure sdw_prepare_stream > + * is called once per stream, we should call it only when > + * dai = first_cpu_dai. > + */ > + if (first_cpu_dai == dai) > + ret = sdw_prepare_stream(dma->stream); Hmmm why not use the one place which is unique in the card to call this, hint machine dais are only called once. > > err: > return ret; > @@ -994,9 +1002,19 @@ static int intel_prepare(struct snd_pcm_substream *substream, > static int intel_trigger(struct snd_pcm_substream *substream, int cmd, > struct snd_soc_dai *dai) > { > + struct snd_soc_pcm_runtime *rtd = substream->private_data; > + struct snd_soc_dai *first_cpu_dai = asoc_rtd_to_cpu(rtd, 0); > struct sdw_cdns_dma_data *dma; > int ret; > > + /* > + * All cpu dais belong to a stream. To ensure sdw_enable/disable_stream > + * are called once per stream, we should call them only when > + * dai = first_cpu_dai. > + */ > + if (first_cpu_dai != dai) > + return 0; > + > dma = snd_soc_dai_get_dma_data(dai, substream); > if (!dma) { > dev_err(dai->dev, "failed to get dma data in %s", __func__); > @@ -1031,6 +1049,8 @@ static int intel_trigger(struct snd_pcm_substream *substream, int cmd, > static int > intel_hw_free(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) > { > + struct snd_soc_pcm_runtime *rtd = substream->private_data; > + struct snd_soc_dai *first_cpu_dai = asoc_rtd_to_cpu(rtd, 0); > struct sdw_cdns *cdns = snd_soc_dai_get_drvdata(dai); > struct sdw_intel *sdw = cdns_to_intel(cdns); > struct sdw_cdns_dma_data *dma; > @@ -1040,12 +1060,25 @@ intel_hw_free(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) > if (!dma) > return -EIO; > > - ret = sdw_deprepare_stream(dma->stream); > - if (ret) { > - dev_err(dai->dev, "sdw_deprepare_stream: failed %d", ret); > - return ret; > + /* > + * All cpu dais belong to a stream. To ensure sdw_deprepare_stream > + * is called once per stream, we should call it only when > + * dai = first_cpu_dai. > + */ > + if (first_cpu_dai == dai) { > + ret = sdw_deprepare_stream(dma->stream); > + if (ret) { > + dev_err(dai->dev, "sdw_deprepare_stream: failed %d", ret); > + return ret; > + } > } > > + /* > + * The sdw stream state will transition to RELEASED when stream-> > + * master_list is empty. So the stream state will transition to > + * DEPREPARED for the first cpu-dai and to RELEASED for the last > + * cpu-dai. > + */ > ret = sdw_stream_remove_master(&cdns->bus, dma->stream); > if (ret < 0) { > dev_err(dai->dev, "remove master from stream %s failed: %d\n", > -- > 2.17.1
>> - ret = sdw_prepare_stream(dma->stream); >> + /* >> + * All cpu dais belong to a stream. To ensure sdw_prepare_stream >> + * is called once per stream, we should call it only when >> + * dai = first_cpu_dai. >> + */ >> + if (first_cpu_dai == dai) >> + ret = sdw_prepare_stream(dma->stream); > > Hmmm why not use the one place which is unique in the card to call this, > hint machine dais are only called once. we are already calling directly sdw_startup_stream() and sdw_shutdown_stream() from the machine driver. We could call sdw_stream_enable() in the dailink .trigger as well, since it only calls the stream API. However for both .prepare() and .hw_free() there are a set of dai-level configurations using static functions defined only in intel.c, and I don't think we can move the code to the machine driver, or split the prepare/hw_free in two (dailink and dai operations). I am not against your idea, I am not sure if it can be done. Would you be ok to merge this as a first step and let us work on an optimization later (which would require ASoC/SoundWire synchronization)?
> -----Original Message----- > From: Vinod Koul <vkoul@kernel.org> > Sent: Wednesday, August 26, 2020 5:47 PM > To: Bard Liao <yung-chuan.liao@linux.intel.com> > Cc: alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org; tiwai@suse.de; > broonie@kernel.org; gregkh@linuxfoundation.org; jank@cadence.com; > srinivas.kandagatla@linaro.org; rander.wang@linux.intel.com; > ranjani.sridharan@linux.intel.com; hui.wang@canonical.com; pierre- > louis.bossart@linux.intel.com; Kale, Sanyog R <sanyog.r.kale@intel.com>; Lin, > Mengdong <mengdong.lin@intel.com>; Liao, Bard <bard.liao@intel.com> > Subject: Re: [PATCH 07/11] soundwire: intel: Only call sdw stream APIs for > the first cpu_dai > > On 18-08-20, 10:41, Bard Liao wrote: > > We should call these APIs once per stream. So we can only call it when > > the dai ops is invoked for the first cpu dai. > > > > Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com> > > Reviewed-by: Pierre-Louis Bossart > > <pierre-louis.bossart@linux.intel.com> > > Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com> > > --- > > drivers/soundwire/intel.c | 45 > > +++++++++++++++++++++++++++++++++------ > > 1 file changed, 39 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c > > index 89a8ad1f80e8..7c63581270fd 100644 > > --- a/drivers/soundwire/intel.c > > +++ b/drivers/soundwire/intel.c > > @@ -941,11 +941,13 @@ static int intel_hw_params(struct > > snd_pcm_substream *substream, static int intel_prepare(struct > snd_pcm_substream *substream, > > struct snd_soc_dai *dai) > > { > > + struct snd_soc_pcm_runtime *rtd = substream->private_data; > > + struct snd_soc_dai *first_cpu_dai = asoc_rtd_to_cpu(rtd, 0); > > struct sdw_cdns *cdns = snd_soc_dai_get_drvdata(dai); > > struct sdw_intel *sdw = cdns_to_intel(cdns); > > struct sdw_cdns_dma_data *dma; > > int ch, dir; > > - int ret; > > + int ret = 0; > > > > dma = snd_soc_dai_get_dma_data(dai, substream); > > if (!dma) { > > @@ -985,7 +987,13 @@ static int intel_prepare(struct > snd_pcm_substream *substream, > > goto err; > > } > > > > - ret = sdw_prepare_stream(dma->stream); > > + /* > > + * All cpu dais belong to a stream. To ensure sdw_prepare_stream > > + * is called once per stream, we should call it only when > > + * dai = first_cpu_dai. > > + */ > > + if (first_cpu_dai == dai) > > + ret = sdw_prepare_stream(dma->stream); > > Hmmm why not use the one place which is unique in the card to call this, > hint machine dais are only called once. Yes, we can call it in machine driver. But, shouldn't it belong to platform level? The point is that if we move the stuff to machine driver, it will force people to implement these stuff on their own Intel machine driver. > > ~Vinod
On 28-08-20, 01:47, Liao, Bard wrote: > > snd_pcm_substream *substream, > > > goto err; > > > } > > > > > > - ret = sdw_prepare_stream(dma->stream); > > > + /* > > > + * All cpu dais belong to a stream. To ensure sdw_prepare_stream > > > + * is called once per stream, we should call it only when > > > + * dai = first_cpu_dai. > > > + */ > > > + if (first_cpu_dai == dai) > > > + ret = sdw_prepare_stream(dma->stream); > > > > Hmmm why not use the one place which is unique in the card to call this, > > hint machine dais are only called once. > > Yes, we can call it in machine driver. But, shouldn't it belong to platform > level? The point is that if we move the stuff to machine driver, it will > force people to implement these stuff on their own Intel machine driver. nothing stops anyone from doing that right! machine driver is another component so it can be moved there as well
On 26-08-20, 09:35, Pierre-Louis Bossart wrote: > > > > - ret = sdw_prepare_stream(dma->stream); > > > + /* > > > + * All cpu dais belong to a stream. To ensure sdw_prepare_stream > > > + * is called once per stream, we should call it only when > > > + * dai = first_cpu_dai. > > > + */ > > > + if (first_cpu_dai == dai) > > > + ret = sdw_prepare_stream(dma->stream); > > > > Hmmm why not use the one place which is unique in the card to call this, > > hint machine dais are only called once. > > we are already calling directly sdw_startup_stream() and > sdw_shutdown_stream() from the machine driver. > > We could call sdw_stream_enable() in the dailink .trigger as well, since it > only calls the stream API. Correct :) > However for both .prepare() and .hw_free() there are a set of dai-level > configurations using static functions defined only in intel.c, and I don't > think we can move the code to the machine driver, or split the > prepare/hw_free in two (dailink and dai operations). Cant they be exported and continue to call those apis > I am not against your idea, I am not sure if it can be done. > > Would you be ok to merge this as a first step and let us work on an > optimization later (which would require ASoC/SoundWire synchronization)? The problem is that we add one flag then another and it does become an issue eventually, better to do the right thing now than later.
On 8/28/20 2:45 AM, Vinod Koul wrote: > On 28-08-20, 01:47, Liao, Bard wrote: >>> snd_pcm_substream *substream, >>>> goto err; >>>> } >>>> >>>> - ret = sdw_prepare_stream(dma->stream); >>>> + /* >>>> + * All cpu dais belong to a stream. To ensure sdw_prepare_stream >>>> + * is called once per stream, we should call it only when >>>> + * dai = first_cpu_dai. >>>> + */ >>>> + if (first_cpu_dai == dai) >>>> + ret = sdw_prepare_stream(dma->stream); >>> >>> Hmmm why not use the one place which is unique in the card to call this, >>> hint machine dais are only called once. >> >> Yes, we can call it in machine driver. But, shouldn't it belong to platform >> level? The point is that if we move the stuff to machine driver, it will >> force people to implement these stuff on their own Intel machine driver. > > nothing stops anyone from doing that right! machine driver is another > component so it can be moved there as well What Bard is saying is that there is nothing board-specific here. This is platform-driver code that is independent of the actual machine configuration. Machine drivers can be board-specific, so we would have to add the code for prepare/deprepare/trigger to every machine driver. Today it's true that we worked to have a single machine driver for all SoundWire-based devices, so the change is a 1:1 move, but we can't guarantee that this would be the case moving forward. In fact, we *know* we will need a different machine driver when we parse platform firmware to create the card for SDCA support. So in the end there would be duplication of code. See the code we worked on at https://github.com/thesofproject/linux/commit/7322e1d25ce2ec9bb78c6e861919f61e0be7cc0b.patch it'd really a bit silly to have this generic code in the machine driver. it would be fine to call a set of helpers called from the machine driver dailink, but where would we put these helpers? on the ASoC or SoundWire sides?
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index 89a8ad1f80e8..7c63581270fd 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -941,11 +941,13 @@ static int intel_hw_params(struct snd_pcm_substream *substream, static int intel_prepare(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) { + struct snd_soc_pcm_runtime *rtd = substream->private_data; + struct snd_soc_dai *first_cpu_dai = asoc_rtd_to_cpu(rtd, 0); struct sdw_cdns *cdns = snd_soc_dai_get_drvdata(dai); struct sdw_intel *sdw = cdns_to_intel(cdns); struct sdw_cdns_dma_data *dma; int ch, dir; - int ret; + int ret = 0; dma = snd_soc_dai_get_dma_data(dai, substream); if (!dma) { @@ -985,7 +987,13 @@ static int intel_prepare(struct snd_pcm_substream *substream, goto err; } - ret = sdw_prepare_stream(dma->stream); + /* + * All cpu dais belong to a stream. To ensure sdw_prepare_stream + * is called once per stream, we should call it only when + * dai = first_cpu_dai. + */ + if (first_cpu_dai == dai) + ret = sdw_prepare_stream(dma->stream); err: return ret; @@ -994,9 +1002,19 @@ static int intel_prepare(struct snd_pcm_substream *substream, static int intel_trigger(struct snd_pcm_substream *substream, int cmd, struct snd_soc_dai *dai) { + struct snd_soc_pcm_runtime *rtd = substream->private_data; + struct snd_soc_dai *first_cpu_dai = asoc_rtd_to_cpu(rtd, 0); struct sdw_cdns_dma_data *dma; int ret; + /* + * All cpu dais belong to a stream. To ensure sdw_enable/disable_stream + * are called once per stream, we should call them only when + * dai = first_cpu_dai. + */ + if (first_cpu_dai != dai) + return 0; + dma = snd_soc_dai_get_dma_data(dai, substream); if (!dma) { dev_err(dai->dev, "failed to get dma data in %s", __func__); @@ -1031,6 +1049,8 @@ static int intel_trigger(struct snd_pcm_substream *substream, int cmd, static int intel_hw_free(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) { + struct snd_soc_pcm_runtime *rtd = substream->private_data; + struct snd_soc_dai *first_cpu_dai = asoc_rtd_to_cpu(rtd, 0); struct sdw_cdns *cdns = snd_soc_dai_get_drvdata(dai); struct sdw_intel *sdw = cdns_to_intel(cdns); struct sdw_cdns_dma_data *dma; @@ -1040,12 +1060,25 @@ intel_hw_free(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) if (!dma) return -EIO; - ret = sdw_deprepare_stream(dma->stream); - if (ret) { - dev_err(dai->dev, "sdw_deprepare_stream: failed %d", ret); - return ret; + /* + * All cpu dais belong to a stream. To ensure sdw_deprepare_stream + * is called once per stream, we should call it only when + * dai = first_cpu_dai. + */ + if (first_cpu_dai == dai) { + ret = sdw_deprepare_stream(dma->stream); + if (ret) { + dev_err(dai->dev, "sdw_deprepare_stream: failed %d", ret); + return ret; + } } + /* + * The sdw stream state will transition to RELEASED when stream-> + * master_list is empty. So the stream state will transition to + * DEPREPARED for the first cpu-dai and to RELEASED for the last + * cpu-dai. + */ ret = sdw_stream_remove_master(&cdns->bus, dma->stream); if (ret < 0) { dev_err(dai->dev, "remove master from stream %s failed: %d\n",