diff mbox series

[07/11] soundwire: intel: Only call sdw stream APIs for the first cpu_dai

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

Commit Message

Bard Liao Aug. 18, 2020, 2:41 a.m. UTC
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(-)

Comments

Vinod Koul Aug. 26, 2020, 9:46 a.m. UTC | #1
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
Pierre-Louis Bossart Aug. 26, 2020, 2:35 p.m. UTC | #2
>> -	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)?
Liao, Bard Aug. 28, 2020, 1:47 a.m. UTC | #3
> -----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
Vinod Koul Aug. 28, 2020, 7:45 a.m. UTC | #4
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
Vinod Koul Aug. 28, 2020, 7:49 a.m. UTC | #5
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.
Pierre-Louis Bossart Aug. 28, 2020, 3:07 p.m. UTC | #6
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 mbox series

Patch

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",