diff mbox series

ASoC: cAVS: add device_link to HDMI audio

Message ID 1554686513-7770-1-git-send-email-libin.yang@intel.com (mailing list archive)
State New, archived
Headers show
Series ASoC: cAVS: add device_link to HDMI audio | expand

Commit Message

Yang, Libin April 8, 2019, 1:21 a.m. UTC 
From: Libin Yang <libin.yang@intel.com>

In resume from S3, HDAC HDMI codec driver dapm event callback may be
operated before HDMI codec driver turns on the display audio power
domain because of the contest between display driver and hdmi codec driver.

This patch adds the device_link between cAVS generic machine device
(consumer) and hdmi codec device (supplier) to make sure the sequence is
always correct.

Signed-off-by: Libin Yang <libin.yang@intel.com>
---
 sound/soc/intel/boards/skl_hda_dsp_common.c  | 15 +++++++++++++++
 sound/soc/intel/boards/skl_hda_dsp_common.h  |  1 +
 sound/soc/intel/boards/skl_hda_dsp_generic.c | 12 ++++++++++++
 3 files changed, 28 insertions(+)

Comments

Takashi Iwai April 8, 2019, 12:35 p.m. UTC | #1 
On Mon, 08 Apr 2019 03:21:53 +0200,
libin.yang@intel.com wrote:
> 
> From: Libin Yang <libin.yang@intel.com>
> 
> In resume from S3, HDAC HDMI codec driver dapm event callback may be
> operated before HDMI codec driver turns on the display audio power
> domain because of the contest between display driver and hdmi codec driver.
> 
> This patch adds the device_link between cAVS generic machine device
> (consumer) and hdmi codec device (supplier) to make sure the sequence is
> always correct.
> 
> Signed-off-by: Libin Yang <libin.yang@intel.com>

Looks good to me.  Maybe it's worth to put a brief comment in
skl_hdmi_init() why this device link is needed, though.

Feel free to take my ack:
  Reviewed-by: Takashi Iwai <tiwai@suse.de>


thanks,

Takashi

> ---
>  sound/soc/intel/boards/skl_hda_dsp_common.c  | 15 +++++++++++++++
>  sound/soc/intel/boards/skl_hda_dsp_common.h  |  1 +
>  sound/soc/intel/boards/skl_hda_dsp_generic.c | 12 ++++++++++++
>  3 files changed, 28 insertions(+)
> 
> diff --git a/sound/soc/intel/boards/skl_hda_dsp_common.c b/sound/soc/intel/boards/skl_hda_dsp_common.c
> index 9b30c0e..01c8937 100644
> --- a/sound/soc/intel/boards/skl_hda_dsp_common.c
> +++ b/sound/soc/intel/boards/skl_hda_dsp_common.c
> @@ -17,6 +17,18 @@
>  
>  #define NAME_SIZE	32
>  
> +static int skl_hdmi_init(struct snd_soc_pcm_runtime *rtd)
> +{
> +	struct skl_hda_private *ctx = snd_soc_card_get_drvdata(rtd->card);
> +	struct snd_soc_dai *dai = rtd->codec_dai;
> +
> +	if (!ctx->link)
> +		ctx->link = device_link_add(rtd->card->dev, dai->dev,
> +					    DL_FLAG_RPM_ACTIVE);
> +
> +	return 0;
> +}
> +
>  int skl_hda_hdmi_add_pcm(struct snd_soc_card *card, int device)
>  {
>  	struct skl_hda_private *ctx = snd_soc_card_get_drvdata(card);
> @@ -48,6 +60,7 @@ struct snd_soc_dai_link skl_hda_be_dai_links[HDA_DSP_MAX_BE_DAI_LINKS] = {
>  		.cpu_dai_name = "iDisp1 Pin",
>  		.codec_name = "ehdaudio0D2",
>  		.codec_dai_name = "intel-hdmi-hifi1",
> +		.init = skl_hdmi_init,
>  		.dpcm_playback = 1,
>  		.no_pcm = 1,
>  		.trigger[0] = SND_SOC_DPCM_TRIGGER_POST,
> @@ -58,6 +71,7 @@ struct snd_soc_dai_link skl_hda_be_dai_links[HDA_DSP_MAX_BE_DAI_LINKS] = {
>  		.cpu_dai_name = "iDisp2 Pin",
>  		.codec_name = "ehdaudio0D2",
>  		.codec_dai_name = "intel-hdmi-hifi2",
> +		.init = skl_hdmi_init,
>  		.dpcm_playback = 1,
>  		.no_pcm = 1,
>  		.trigger[0] = SND_SOC_DPCM_TRIGGER_POST,
> @@ -68,6 +82,7 @@ struct snd_soc_dai_link skl_hda_be_dai_links[HDA_DSP_MAX_BE_DAI_LINKS] = {
>  		.cpu_dai_name = "iDisp3 Pin",
>  		.codec_name = "ehdaudio0D2",
>  		.codec_dai_name = "intel-hdmi-hifi3",
> +		.init = skl_hdmi_init,
>  		.dpcm_playback = 1,
>  		.no_pcm = 1,
>  		.trigger[0] = SND_SOC_DPCM_TRIGGER_POST,
> diff --git a/sound/soc/intel/boards/skl_hda_dsp_common.h b/sound/soc/intel/boards/skl_hda_dsp_common.h
> index 87c50af..df5cc6b 100644
> --- a/sound/soc/intel/boards/skl_hda_dsp_common.h
> +++ b/sound/soc/intel/boards/skl_hda_dsp_common.h
> @@ -29,6 +29,7 @@ struct skl_hda_private {
>  	int pcm_count;
>  	int dai_index;
>  	const char *platform_name;
> +	struct device_link *link;
>  };
>  
>  extern struct snd_soc_dai_link skl_hda_be_dai_links[HDA_DSP_MAX_BE_DAI_LINKS];
> diff --git a/sound/soc/intel/boards/skl_hda_dsp_generic.c b/sound/soc/intel/boards/skl_hda_dsp_generic.c
> index b9a21e6..ceca11e 100644
> --- a/sound/soc/intel/boards/skl_hda_dsp_generic.c
> +++ b/sound/soc/intel/boards/skl_hda_dsp_generic.c
> @@ -166,8 +166,20 @@ static int skl_hda_audio_probe(struct platform_device *pdev)
>  	return devm_snd_soc_register_card(&pdev->dev, &hda_soc_card);
>  }
>  
> +static int skl_hda_audio_remove(struct platform_device *pdev)
> +{
> +	struct snd_soc_card *card = platform_get_drvdata(pdev);
> +	struct skl_hda_private *ctx = snd_soc_card_get_drvdata(card);
> +
> +	if (ctx->link)
> +		device_link_del(ctx->link);
> +
> +	return 0;
> +}
> +
>  static struct platform_driver skl_hda_audio = {
>  	.probe = skl_hda_audio_probe,
> +	.remove = skl_hda_audio_remove,
>  	.driver = {
>  		.name = "skl_hda_dsp_generic",
>  		.pm = &snd_soc_pm_ops,
> -- 
> 2.7.4
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>
Pierre-Louis Bossart April 8, 2019, 1:14 p.m. UTC | #2 
On 4/7/19 8:21 PM, libin.yang@intel.com wrote:
> From: Libin Yang <libin.yang@intel.com>
> 
> In resume from S3, HDAC HDMI codec driver dapm event callback may be
> operated before HDMI codec driver turns on the display audio power
> domain because of the contest between display driver and hdmi codec driver.
> 
> This patch adds the device_link between cAVS generic machine device
> (consumer) and hdmi codec device (supplier) to make sure the sequence is
> always correct.
> 
> Signed-off-by: Libin Yang <libin.yang@intel.com>
> ---
>   sound/soc/intel/boards/skl_hda_dsp_common.c  | 15 +++++++++++++++
>   sound/soc/intel/boards/skl_hda_dsp_common.h  |  1 +
>   sound/soc/intel/boards/skl_hda_dsp_generic.c | 12 ++++++++++++

It's Monday morning and I have mixed feelings about this fix.

0. What does 'resume from S3' mean? from the description it looks like 
the application was playing already before entering S3? I am ready to 
bet it's a use case that was never tested in the past given that 
Chromebooks tend to stop all streams before entering S3...

1. As discussed in other threads, Takashi suggested that maybe the 
INFO_RESUME flag should be removed?  What would be the impact should we 
indeed remove this flag, is this patch still necessary?

2. if this is really a generic issue then shouldn't it be fixed for all 
users of the iDISP link? Why stop at the HDAudio machine driver?

3. we already have the component model to deal with interaction between 
i915 and audio, now we are adding a second layer. That looks clunky.

Thanks
-Pierre

>   3 files changed, 28 insertions(+)
> 
> diff --git a/sound/soc/intel/boards/skl_hda_dsp_common.c b/sound/soc/intel/boards/skl_hda_dsp_common.c
> index 9b30c0e..01c8937 100644
> --- a/sound/soc/intel/boards/skl_hda_dsp_common.c
> +++ b/sound/soc/intel/boards/skl_hda_dsp_common.c
> @@ -17,6 +17,18 @@
>   
>   #define NAME_SIZE	32
>   
> +static int skl_hdmi_init(struct snd_soc_pcm_runtime *rtd)
> +{
> +	struct skl_hda_private *ctx = snd_soc_card_get_drvdata(rtd->card);
> +	struct snd_soc_dai *dai = rtd->codec_dai;
> +
> +	if (!ctx->link)
> +		ctx->link = device_link_add(rtd->card->dev, dai->dev,
> +					    DL_FLAG_RPM_ACTIVE);
> +
> +	return 0;
> +}
> +
>   int skl_hda_hdmi_add_pcm(struct snd_soc_card *card, int device)
>   {
>   	struct skl_hda_private *ctx = snd_soc_card_get_drvdata(card);
> @@ -48,6 +60,7 @@ struct snd_soc_dai_link skl_hda_be_dai_links[HDA_DSP_MAX_BE_DAI_LINKS] = {
>   		.cpu_dai_name = "iDisp1 Pin",
>   		.codec_name = "ehdaudio0D2",
>   		.codec_dai_name = "intel-hdmi-hifi1",
> +		.init = skl_hdmi_init,
>   		.dpcm_playback = 1,
>   		.no_pcm = 1,
>   		.trigger[0] = SND_SOC_DPCM_TRIGGER_POST,
> @@ -58,6 +71,7 @@ struct snd_soc_dai_link skl_hda_be_dai_links[HDA_DSP_MAX_BE_DAI_LINKS] = {
>   		.cpu_dai_name = "iDisp2 Pin",
>   		.codec_name = "ehdaudio0D2",
>   		.codec_dai_name = "intel-hdmi-hifi2",
> +		.init = skl_hdmi_init,
>   		.dpcm_playback = 1,
>   		.no_pcm = 1,
>   		.trigger[0] = SND_SOC_DPCM_TRIGGER_POST,
> @@ -68,6 +82,7 @@ struct snd_soc_dai_link skl_hda_be_dai_links[HDA_DSP_MAX_BE_DAI_LINKS] = {
>   		.cpu_dai_name = "iDisp3 Pin",
>   		.codec_name = "ehdaudio0D2",
>   		.codec_dai_name = "intel-hdmi-hifi3",
> +		.init = skl_hdmi_init,
>   		.dpcm_playback = 1,
>   		.no_pcm = 1,
>   		.trigger[0] = SND_SOC_DPCM_TRIGGER_POST,
> diff --git a/sound/soc/intel/boards/skl_hda_dsp_common.h b/sound/soc/intel/boards/skl_hda_dsp_common.h
> index 87c50af..df5cc6b 100644
> --- a/sound/soc/intel/boards/skl_hda_dsp_common.h
> +++ b/sound/soc/intel/boards/skl_hda_dsp_common.h
> @@ -29,6 +29,7 @@ struct skl_hda_private {
>   	int pcm_count;
>   	int dai_index;
>   	const char *platform_name;
> +	struct device_link *link;
>   };
>   
>   extern struct snd_soc_dai_link skl_hda_be_dai_links[HDA_DSP_MAX_BE_DAI_LINKS];
> diff --git a/sound/soc/intel/boards/skl_hda_dsp_generic.c b/sound/soc/intel/boards/skl_hda_dsp_generic.c
> index b9a21e6..ceca11e 100644
> --- a/sound/soc/intel/boards/skl_hda_dsp_generic.c
> +++ b/sound/soc/intel/boards/skl_hda_dsp_generic.c
> @@ -166,8 +166,20 @@ static int skl_hda_audio_probe(struct platform_device *pdev)
>   	return devm_snd_soc_register_card(&pdev->dev, &hda_soc_card);
>   }
>   
> +static int skl_hda_audio_remove(struct platform_device *pdev)
> +{
> +	struct snd_soc_card *card = platform_get_drvdata(pdev);
> +	struct skl_hda_private *ctx = snd_soc_card_get_drvdata(card);
> +
> +	if (ctx->link)
> +		device_link_del(ctx->link);
> +
> +	return 0;
> +}
> +
>   static struct platform_driver skl_hda_audio = {
>   	.probe = skl_hda_audio_probe,
> +	.remove = skl_hda_audio_remove,
>   	.driver = {
>   		.name = "skl_hda_dsp_generic",
>   		.pm = &snd_soc_pm_ops,
>
Yang, Libin April 8, 2019, 1:18 p.m. UTC | #3 
Hi Takashi,

>-----Original Message-----
>From: Takashi Iwai [mailto:tiwai@suse.de]
>Sent: Monday, April 8, 2019 8:35 PM
>To: Yang, Libin <libin.yang@intel.com>
>Cc: alsa-devel@alsa-project.org; broonie@kernel.org; pierre-
>louis.bossart@linux.intel.com
>Subject: Re: [alsa-devel] [PATCH] ASoC: cAVS: add device_link to HDMI audio
>
>On Mon, 08 Apr 2019 03:21:53 +0200,
>libin.yang@intel.com wrote:
>>
>> From: Libin Yang <libin.yang@intel.com>
>>
>> In resume from S3, HDAC HDMI codec driver dapm event callback may be
>> operated before HDMI codec driver turns on the display audio power
>> domain because of the contest between display driver and hdmi codec
>driver.
>>
>> This patch adds the device_link between cAVS generic machine device
>> (consumer) and hdmi codec device (supplier) to make sure the sequence
>> is always correct.
>>
>> Signed-off-by: Libin Yang <libin.yang@intel.com>
>
>Looks good to me.  Maybe it's worth to put a brief comment in
>skl_hdmi_init() why this device link is needed, though.

I will do it. Thanks for the review and the advice.

Regards,
Libin

>
>Feel free to take my ack:
>  Reviewed-by: Takashi Iwai <tiwai@suse.de>
>
>
>thanks,
>
>Takashi
>
>> ---
>>  sound/soc/intel/boards/skl_hda_dsp_common.c  | 15 +++++++++++++++
>> sound/soc/intel/boards/skl_hda_dsp_common.h  |  1 +
>> sound/soc/intel/boards/skl_hda_dsp_generic.c | 12 ++++++++++++
>>  3 files changed, 28 insertions(+)
>>
>> diff --git a/sound/soc/intel/boards/skl_hda_dsp_common.c
>> b/sound/soc/intel/boards/skl_hda_dsp_common.c
>> index 9b30c0e..01c8937 100644
>> --- a/sound/soc/intel/boards/skl_hda_dsp_common.c
>> +++ b/sound/soc/intel/boards/skl_hda_dsp_common.c
>> @@ -17,6 +17,18 @@
>>
>>  #define NAME_SIZE	32
>>
>> +static int skl_hdmi_init(struct snd_soc_pcm_runtime *rtd) {
>> +	struct skl_hda_private *ctx = snd_soc_card_get_drvdata(rtd->card);
>> +	struct snd_soc_dai *dai = rtd->codec_dai;
>> +
>> +	if (!ctx->link)
>> +		ctx->link = device_link_add(rtd->card->dev, dai->dev,
>> +					    DL_FLAG_RPM_ACTIVE);
>> +
>> +	return 0;
>> +}
>> +
>>  int skl_hda_hdmi_add_pcm(struct snd_soc_card *card, int device)  {
>>  	struct skl_hda_private *ctx = snd_soc_card_get_drvdata(card); @@
>> -48,6 +60,7 @@ struct snd_soc_dai_link
>skl_hda_be_dai_links[HDA_DSP_MAX_BE_DAI_LINKS] = {
>>  		.cpu_dai_name = "iDisp1 Pin",
>>  		.codec_name = "ehdaudio0D2",
>>  		.codec_dai_name = "intel-hdmi-hifi1",
>> +		.init = skl_hdmi_init,
>>  		.dpcm_playback = 1,
>>  		.no_pcm = 1,
>>  		.trigger[0] = SND_SOC_DPCM_TRIGGER_POST, @@ -58,6 +71,7
>@@ struct
>> snd_soc_dai_link skl_hda_be_dai_links[HDA_DSP_MAX_BE_DAI_LINKS] = {
>>  		.cpu_dai_name = "iDisp2 Pin",
>>  		.codec_name = "ehdaudio0D2",
>>  		.codec_dai_name = "intel-hdmi-hifi2",
>> +		.init = skl_hdmi_init,
>>  		.dpcm_playback = 1,
>>  		.no_pcm = 1,
>>  		.trigger[0] = SND_SOC_DPCM_TRIGGER_POST, @@ -68,6 +82,7
>@@ struct
>> snd_soc_dai_link skl_hda_be_dai_links[HDA_DSP_MAX_BE_DAI_LINKS] = {
>>  		.cpu_dai_name = "iDisp3 Pin",
>>  		.codec_name = "ehdaudio0D2",
>>  		.codec_dai_name = "intel-hdmi-hifi3",
>> +		.init = skl_hdmi_init,
>>  		.dpcm_playback = 1,
>>  		.no_pcm = 1,
>>  		.trigger[0] = SND_SOC_DPCM_TRIGGER_POST, diff --git
>> a/sound/soc/intel/boards/skl_hda_dsp_common.h
>> b/sound/soc/intel/boards/skl_hda_dsp_common.h
>> index 87c50af..df5cc6b 100644
>> --- a/sound/soc/intel/boards/skl_hda_dsp_common.h
>> +++ b/sound/soc/intel/boards/skl_hda_dsp_common.h
>> @@ -29,6 +29,7 @@ struct skl_hda_private {
>>  	int pcm_count;
>>  	int dai_index;
>>  	const char *platform_name;
>> +	struct device_link *link;
>>  };
>>
>>  extern struct snd_soc_dai_link
>> skl_hda_be_dai_links[HDA_DSP_MAX_BE_DAI_LINKS];
>> diff --git a/sound/soc/intel/boards/skl_hda_dsp_generic.c
>> b/sound/soc/intel/boards/skl_hda_dsp_generic.c
>> index b9a21e6..ceca11e 100644
>> --- a/sound/soc/intel/boards/skl_hda_dsp_generic.c
>> +++ b/sound/soc/intel/boards/skl_hda_dsp_generic.c
>> @@ -166,8 +166,20 @@ static int skl_hda_audio_probe(struct
>platform_device *pdev)
>>  	return devm_snd_soc_register_card(&pdev->dev, &hda_soc_card);  }
>>
>> +static int skl_hda_audio_remove(struct platform_device *pdev) {
>> +	struct snd_soc_card *card = platform_get_drvdata(pdev);
>> +	struct skl_hda_private *ctx = snd_soc_card_get_drvdata(card);
>> +
>> +	if (ctx->link)
>> +		device_link_del(ctx->link);
>> +
>> +	return 0;
>> +}
>> +
>>  static struct platform_driver skl_hda_audio = {
>>  	.probe = skl_hda_audio_probe,
>> +	.remove = skl_hda_audio_remove,
>>  	.driver = {
>>  		.name = "skl_hda_dsp_generic",
>>  		.pm = &snd_soc_pm_ops,
>> --
>> 2.7.4
>>
>> _______________________________________________
>> Alsa-devel mailing list
>> Alsa-devel@alsa-project.org
>> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>>
Yang, Libin April 8, 2019, 1:34 p.m. UTC | #4 
Hi Pierre,

>-----Original Message-----
>From: Pierre-Louis Bossart [mailto:pierre-louis.bossart@linux.intel.com]
>Sent: Monday, April 8, 2019 9:15 PM
>To: Yang, Libin <libin.yang@intel.com>; alsa-devel@alsa-project.org;
>tiwai@suse.de; broonie@kernel.org
>Subject: Re: [alsa-devel] [PATCH] ASoC: cAVS: add device_link to HDMI audio
>
>
>
>On 4/7/19 8:21 PM, libin.yang@intel.com wrote:
>> From: Libin Yang <libin.yang@intel.com>
>>
>> In resume from S3, HDAC HDMI codec driver dapm event callback may be
>> operated before HDMI codec driver turns on the display audio power
>> domain because of the contest between display driver and hdmi codec
>driver.
>>
>> This patch adds the device_link between cAVS generic machine device
>> (consumer) and hdmi codec device (supplier) to make sure the sequence
>> is always correct.
>>
>> Signed-off-by: Libin Yang <libin.yang@intel.com>
>> ---
>>   sound/soc/intel/boards/skl_hda_dsp_common.c  | 15 +++++++++++++++
>>   sound/soc/intel/boards/skl_hda_dsp_common.h  |  1 +
>>   sound/soc/intel/boards/skl_hda_dsp_generic.c | 12 ++++++++++++
>
>It's Monday morning and I have mixed feelings about this fix.
>
>0. What does 'resume from S3' mean? from the description it looks like the
>application was playing already before entering S3? I am ready to bet it's a use
>case that was never tested in the past given that Chromebooks tend to stop all
>streams before entering S3...

Yes, this patch is fixing the case of HDMI audio S3 with playback.
Chrome has different behavior from aplay and etc. As you described,
chrome will stop the streams when suspend and start the stream when resume.
So suspend/resume when HDMI playback in chrome will never hit this bug.
And our QA test result does show chrome is OK even without this patch.

>
>1. As discussed in other threads, Takashi suggested that maybe the
>INFO_RESUME flag should be removed?  What would be the impact should we
>indeed remove this flag, is this patch still necessary?

That is another issue. Actually, our sof has 2 bugs in HDMI S3 with playback.
One is DMA link setting. The other is the bug this patch try to fix.
Another thread is used to fix the DMA link setting.

>
>2. if this is really a generic issue then shouldn't it be fixed for all users of the
>iDISP link? Why stop at the HDAudio machine driver?

I will submit other patches for other machine drivers. Which HDAudio machine 
driver do you mean?

>
>3. we already have the component model to deal with interaction between
>i915 and audio, now we are adding a second layer. That looks clunky.

The component model between i915 and audio is used to communicate
between display driver and audio driver. This patch is used to setup the
consumer and supplier relationship.

Regards,
Libin

>
>Thanks
>-Pierre
>
>>   3 files changed, 28 insertions(+)
>>
>> diff --git a/sound/soc/intel/boards/skl_hda_dsp_common.c
>> b/sound/soc/intel/boards/skl_hda_dsp_common.c
>> index 9b30c0e..01c8937 100644
>> --- a/sound/soc/intel/boards/skl_hda_dsp_common.c
>> +++ b/sound/soc/intel/boards/skl_hda_dsp_common.c
>> @@ -17,6 +17,18 @@
>>
>>   #define NAME_SIZE	32
>>
>> +static int skl_hdmi_init(struct snd_soc_pcm_runtime *rtd) {
>> +	struct skl_hda_private *ctx = snd_soc_card_get_drvdata(rtd->card);
>> +	struct snd_soc_dai *dai = rtd->codec_dai;
>> +
>> +	if (!ctx->link)
>> +		ctx->link = device_link_add(rtd->card->dev, dai->dev,
>> +					    DL_FLAG_RPM_ACTIVE);
>> +
>> +	return 0;
>> +}
>> +
>>   int skl_hda_hdmi_add_pcm(struct snd_soc_card *card, int device)
>>   {
>>   	struct skl_hda_private *ctx = snd_soc_card_get_drvdata(card); @@
>> -48,6 +60,7 @@ struct snd_soc_dai_link
>skl_hda_be_dai_links[HDA_DSP_MAX_BE_DAI_LINKS] = {
>>   		.cpu_dai_name = "iDisp1 Pin",
>>   		.codec_name = "ehdaudio0D2",
>>   		.codec_dai_name = "intel-hdmi-hifi1",
>> +		.init = skl_hdmi_init,
>>   		.dpcm_playback = 1,
>>   		.no_pcm = 1,
>>   		.trigger[0] = SND_SOC_DPCM_TRIGGER_POST, @@ -58,6 +71,7
>@@ struct
>> snd_soc_dai_link skl_hda_be_dai_links[HDA_DSP_MAX_BE_DAI_LINKS] = {
>>   		.cpu_dai_name = "iDisp2 Pin",
>>   		.codec_name = "ehdaudio0D2",
>>   		.codec_dai_name = "intel-hdmi-hifi2",
>> +		.init = skl_hdmi_init,
>>   		.dpcm_playback = 1,
>>   		.no_pcm = 1,
>>   		.trigger[0] = SND_SOC_DPCM_TRIGGER_POST, @@ -68,6 +82,7
>@@ struct
>> snd_soc_dai_link skl_hda_be_dai_links[HDA_DSP_MAX_BE_DAI_LINKS] = {
>>   		.cpu_dai_name = "iDisp3 Pin",
>>   		.codec_name = "ehdaudio0D2",
>>   		.codec_dai_name = "intel-hdmi-hifi3",
>> +		.init = skl_hdmi_init,
>>   		.dpcm_playback = 1,
>>   		.no_pcm = 1,
>>   		.trigger[0] = SND_SOC_DPCM_TRIGGER_POST, diff --git
>> a/sound/soc/intel/boards/skl_hda_dsp_common.h
>> b/sound/soc/intel/boards/skl_hda_dsp_common.h
>> index 87c50af..df5cc6b 100644
>> --- a/sound/soc/intel/boards/skl_hda_dsp_common.h
>> +++ b/sound/soc/intel/boards/skl_hda_dsp_common.h
>> @@ -29,6 +29,7 @@ struct skl_hda_private {
>>   	int pcm_count;
>>   	int dai_index;
>>   	const char *platform_name;
>> +	struct device_link *link;
>>   };
>>
>>   extern struct snd_soc_dai_link
>> skl_hda_be_dai_links[HDA_DSP_MAX_BE_DAI_LINKS];
>> diff --git a/sound/soc/intel/boards/skl_hda_dsp_generic.c
>> b/sound/soc/intel/boards/skl_hda_dsp_generic.c
>> index b9a21e6..ceca11e 100644
>> --- a/sound/soc/intel/boards/skl_hda_dsp_generic.c
>> +++ b/sound/soc/intel/boards/skl_hda_dsp_generic.c
>> @@ -166,8 +166,20 @@ static int skl_hda_audio_probe(struct
>platform_device *pdev)
>>   	return devm_snd_soc_register_card(&pdev->dev, &hda_soc_card);
>>   }
>>
>> +static int skl_hda_audio_remove(struct platform_device *pdev) {
>> +	struct snd_soc_card *card = platform_get_drvdata(pdev);
>> +	struct skl_hda_private *ctx = snd_soc_card_get_drvdata(card);
>> +
>> +	if (ctx->link)
>> +		device_link_del(ctx->link);
>> +
>> +	return 0;
>> +}
>> +
>>   static struct platform_driver skl_hda_audio = {
>>   	.probe = skl_hda_audio_probe,
>> +	.remove = skl_hda_audio_remove,
>>   	.driver = {
>>   		.name = "skl_hda_dsp_generic",
>>   		.pm = &snd_soc_pm_ops,
>>
Ranjani Sridharan April 8, 2019, 2:36 p.m. UTC | #5 
On Mon, 2019-04-08 at 08:14 -0500, Pierre-Louis Bossart wrote:
> 
> On 4/7/19 8:21 PM, libin.yang@intel.com wrote:
> > From: Libin Yang <libin.yang@intel.com>
> > 
> > In resume from S3, HDAC HDMI codec driver dapm event callback may
> > be
> > operated before HDMI codec driver turns on the display audio power
> > domain because of the contest between display driver and hdmi codec
> > driver.
> > 
> > This patch adds the device_link between cAVS generic machine device
> > (consumer) and hdmi codec device (supplier) to make sure the
> > sequence is
> > always correct.
> > 
> > Signed-off-by: Libin Yang <libin.yang@intel.com>
> > ---
> >   sound/soc/intel/boards/skl_hda_dsp_common.c  | 15 +++++++++++++++
> >   sound/soc/intel/boards/skl_hda_dsp_common.h  |  1 +
> >   sound/soc/intel/boards/skl_hda_dsp_generic.c | 12 ++++++++++++
> 
> It's Monday morning and I have mixed feelings about this fix.
> 
> 0. What does 'resume from S3' mean? from the description it looks
> like 
> the application was playing already before entering S3? I am ready
> to 
> bet it's a use case that was never tested in the past given that 
> Chromebooks tend to stop all streams before entering S3...
> 
> 1. As discussed in other threads, Takashi suggested that maybe the 
> INFO_RESUME flag should be removed?  What would be the impact should
> we 
> indeed remove this flag, is this patch still necessary?
Hi Pierre,

I can confirm that this patch is necessary even if INFO_RESUME is not
set.

Thanks,
Ranjani
> 
> 2. if this is really a generic issue then shouldn't it be fixed for
> all 
> users of the iDISP link? Why stop at the HDAudio machine driver?
> 
> 3. we already have the component model to deal with interaction
> between 
> i915 and audio, now we are adding a second layer. That looks clunky.
> 
> Thanks
> -Pierre
> 
> >   3 files changed, 28 insertions(+)
> > 
> > diff --git a/sound/soc/intel/boards/skl_hda_dsp_common.c
> > b/sound/soc/intel/boards/skl_hda_dsp_common.c
> > index 9b30c0e..01c8937 100644
> > --- a/sound/soc/intel/boards/skl_hda_dsp_common.c
> > +++ b/sound/soc/intel/boards/skl_hda_dsp_common.c
> > @@ -17,6 +17,18 @@
> >   
> >   #define NAME_SIZE	32
> >   
> > +static int skl_hdmi_init(struct snd_soc_pcm_runtime *rtd)
> > +{
> > +	struct skl_hda_private *ctx = snd_soc_card_get_drvdata(rtd-
> > >card);
> > +	struct snd_soc_dai *dai = rtd->codec_dai;
> > +
> > +	if (!ctx->link)
> > +		ctx->link = device_link_add(rtd->card->dev, dai->dev,
> > +					    DL_FLAG_RPM_ACTIVE);
> > +
> > +	return 0;
> > +}
> > +
> >   int skl_hda_hdmi_add_pcm(struct snd_soc_card *card, int device)
> >   {
> >   	struct skl_hda_private *ctx = snd_soc_card_get_drvdata(card);
> > @@ -48,6 +60,7 @@ struct snd_soc_dai_link
> > skl_hda_be_dai_links[HDA_DSP_MAX_BE_DAI_LINKS] = {
> >   		.cpu_dai_name = "iDisp1 Pin",
> >   		.codec_name = "ehdaudio0D2",
> >   		.codec_dai_name = "intel-hdmi-hifi1",
> > +		.init = skl_hdmi_init,
> >   		.dpcm_playback = 1,
> >   		.no_pcm = 1,
> >   		.trigger[0] = SND_SOC_DPCM_TRIGGER_POST,
> > @@ -58,6 +71,7 @@ struct snd_soc_dai_link
> > skl_hda_be_dai_links[HDA_DSP_MAX_BE_DAI_LINKS] = {
> >   		.cpu_dai_name = "iDisp2 Pin",
> >   		.codec_name = "ehdaudio0D2",
> >   		.codec_dai_name = "intel-hdmi-hifi2",
> > +		.init = skl_hdmi_init,
> >   		.dpcm_playback = 1,
> >   		.no_pcm = 1,
> >   		.trigger[0] = SND_SOC_DPCM_TRIGGER_POST,
> > @@ -68,6 +82,7 @@ struct snd_soc_dai_link
> > skl_hda_be_dai_links[HDA_DSP_MAX_BE_DAI_LINKS] = {
> >   		.cpu_dai_name = "iDisp3 Pin",
> >   		.codec_name = "ehdaudio0D2",
> >   		.codec_dai_name = "intel-hdmi-hifi3",
> > +		.init = skl_hdmi_init,
> >   		.dpcm_playback = 1,
> >   		.no_pcm = 1,
> >   		.trigger[0] = SND_SOC_DPCM_TRIGGER_POST,
> > diff --git a/sound/soc/intel/boards/skl_hda_dsp_common.h
> > b/sound/soc/intel/boards/skl_hda_dsp_common.h
> > index 87c50af..df5cc6b 100644
> > --- a/sound/soc/intel/boards/skl_hda_dsp_common.h
> > +++ b/sound/soc/intel/boards/skl_hda_dsp_common.h
> > @@ -29,6 +29,7 @@ struct skl_hda_private {
> >   	int pcm_count;
> >   	int dai_index;
> >   	const char *platform_name;
> > +	struct device_link *link;
> >   };
> >   
> >   extern struct snd_soc_dai_link
> > skl_hda_be_dai_links[HDA_DSP_MAX_BE_DAI_LINKS];
> > diff --git a/sound/soc/intel/boards/skl_hda_dsp_generic.c
> > b/sound/soc/intel/boards/skl_hda_dsp_generic.c
> > index b9a21e6..ceca11e 100644
> > --- a/sound/soc/intel/boards/skl_hda_dsp_generic.c
> > +++ b/sound/soc/intel/boards/skl_hda_dsp_generic.c
> > @@ -166,8 +166,20 @@ static int skl_hda_audio_probe(struct
> > platform_device *pdev)
> >   	return devm_snd_soc_register_card(&pdev->dev, &hda_soc_card);
> >   }
> >   
> > +static int skl_hda_audio_remove(struct platform_device *pdev)
> > +{
> > +	struct snd_soc_card *card = platform_get_drvdata(pdev);
> > +	struct skl_hda_private *ctx = snd_soc_card_get_drvdata(card);
> > +
> > +	if (ctx->link)
> > +		device_link_del(ctx->link);
> > +
> > +	return 0;
> > +}
> > +
> >   static struct platform_driver skl_hda_audio = {
> >   	.probe = skl_hda_audio_probe,
> > +	.remove = skl_hda_audio_remove,
> >   	.driver = {
> >   		.name = "skl_hda_dsp_generic",
> >   		.pm = &snd_soc_pm_ops,
> > 
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Pierre-Louis Bossart April 8, 2019, 2:43 p.m. UTC | #6 
>> 2. if this is really a generic issue then shouldn't it be fixed for all users of the
>> iDISP link? Why stop at the HDAudio machine driver?
> 
> I will submit other patches for other machine drivers. Which HDAudio machine
> driver do you mean?

if you plan on updating other drivers, then by all means let's use a 
common set of helper functions. I see you have follow-up patches on 
github already, let's avoid copy/paste if possible if the same 
functionality is needed in multiple places.

>>
>> 3. we already have the component model to deal with interaction between
>> i915 and audio, now we are adding a second layer. That looks clunky.
> 
> The component model between i915 and audio is used to communicate
> between display driver and audio driver. This patch is used to setup the
> consumer and supplier relationship.

consumer of what? it's very vague for people who haven't really looked 
into the details.
Yang, Libin April 9, 2019, 1:09 a.m. UTC | #7 
Hi Pierre,

>-----Original Message-----
>From: Pierre-Louis Bossart [mailto:pierre-louis.bossart@linux.intel.com]
>Sent: Monday, April 8, 2019 10:43 PM
>To: Yang, Libin <libin.yang@intel.com>; alsa-devel@alsa-project.org;
>tiwai@suse.de; broonie@kernel.org
>Subject: Re: [alsa-devel] [PATCH] ASoC: cAVS: add device_link to HDMI audio
>
>
>>> 2. if this is really a generic issue then shouldn't it be fixed for
>>> all users of the iDISP link? Why stop at the HDAudio machine driver?
>>
>> I will submit other patches for other machine drivers. Which HDAudio
>> machine driver do you mean?
>
>if you plan on updating other drivers, then by all means let's use a common
>set of helper functions. I see you have follow-up patches on github already,
>let's avoid copy/paste if possible if the same functionality is needed in
>multiple places.

Sure, I will do it. Thanks for suggestion.

>
>>>
>>> 3. we already have the component model to deal with interaction between
>>> i915 and audio, now we are adding a second layer. That looks clunky.
>>
>> The component model between i915 and audio is used to communicate
>> between display driver and audio driver. This patch is used to setup the
>> consumer and supplier relationship.
>
>consumer of what? it's very vague for people who haven't really looked
>into the details.

In our case, the consumer is the machine device, and it's the consumer
of the HDMI codec device. I will add the comments in the patch.

Regards,
Libin
Mark Brown April 10, 2019, 11:29 a.m. UTC | #8 
On Mon, Apr 08, 2019 at 09:21:53AM +0800, libin.yang@intel.com wrote:
> From: Libin Yang <libin.yang@intel.com>
> 
> In resume from S3, HDAC HDMI codec driver dapm event callback may be
> operated before HDMI codec driver turns on the display audio power
> domain because of the contest between display driver and hdmi codec driver.

This doesn't apply against current code, please check and resend.
Yang, Libin April 10, 2019, 1:07 p.m. UTC | #9 
Hi Mark,

>-----Original Message-----
>From: Mark Brown [mailto:broonie@kernel.org]
>Sent: Wednesday, April 10, 2019 7:30 PM
>To: Yang, Libin <libin.yang@intel.com>
>Cc: alsa-devel@alsa-project.org; tiwai@suse.de; pierre-
>louis.bossart@linux.intel.com
>Subject: Re: [alsa-devel] [PATCH] ASoC: cAVS: add device_link to HDMI audio
>
>On Mon, Apr 08, 2019 at 09:21:53AM +0800, libin.yang@intel.com wrote:
>> From: Libin Yang <libin.yang@intel.com>
>>
>> In resume from S3, HDAC HDMI codec driver dapm event callback may be
>> operated before HDMI codec driver turns on the display audio power
>> domain because of the contest between display driver and hdmi codec
>driver.
>
>This doesn't apply against current code, please check and resend.

Got it. Thanks. I will update the patch based on the latest code.

Regards,
Libin
diff mbox series

Patch

diff --git a/sound/soc/intel/boards/skl_hda_dsp_common.c b/sound/soc/intel/boards/skl_hda_dsp_common.c
index 9b30c0e..01c8937 100644
--- a/sound/soc/intel/boards/skl_hda_dsp_common.c
+++ b/sound/soc/intel/boards/skl_hda_dsp_common.c
@@ -17,6 +17,18 @@ 
 
 #define NAME_SIZE	32
 
+static int skl_hdmi_init(struct snd_soc_pcm_runtime *rtd)
+{
+	struct skl_hda_private *ctx = snd_soc_card_get_drvdata(rtd->card);
+	struct snd_soc_dai *dai = rtd->codec_dai;
+
+	if (!ctx->link)
+		ctx->link = device_link_add(rtd->card->dev, dai->dev,
+					    DL_FLAG_RPM_ACTIVE);
+
+	return 0;
+}
+
 int skl_hda_hdmi_add_pcm(struct snd_soc_card *card, int device)
 {
 	struct skl_hda_private *ctx = snd_soc_card_get_drvdata(card);
@@ -48,6 +60,7 @@  struct snd_soc_dai_link skl_hda_be_dai_links[HDA_DSP_MAX_BE_DAI_LINKS] = {
 		.cpu_dai_name = "iDisp1 Pin",
 		.codec_name = "ehdaudio0D2",
 		.codec_dai_name = "intel-hdmi-hifi1",
+		.init = skl_hdmi_init,
 		.dpcm_playback = 1,
 		.no_pcm = 1,
 		.trigger[0] = SND_SOC_DPCM_TRIGGER_POST,
@@ -58,6 +71,7 @@  struct snd_soc_dai_link skl_hda_be_dai_links[HDA_DSP_MAX_BE_DAI_LINKS] = {
 		.cpu_dai_name = "iDisp2 Pin",
 		.codec_name = "ehdaudio0D2",
 		.codec_dai_name = "intel-hdmi-hifi2",
+		.init = skl_hdmi_init,
 		.dpcm_playback = 1,
 		.no_pcm = 1,
 		.trigger[0] = SND_SOC_DPCM_TRIGGER_POST,
@@ -68,6 +82,7 @@  struct snd_soc_dai_link skl_hda_be_dai_links[HDA_DSP_MAX_BE_DAI_LINKS] = {
 		.cpu_dai_name = "iDisp3 Pin",
 		.codec_name = "ehdaudio0D2",
 		.codec_dai_name = "intel-hdmi-hifi3",
+		.init = skl_hdmi_init,
 		.dpcm_playback = 1,
 		.no_pcm = 1,
 		.trigger[0] = SND_SOC_DPCM_TRIGGER_POST,
diff --git a/sound/soc/intel/boards/skl_hda_dsp_common.h b/sound/soc/intel/boards/skl_hda_dsp_common.h
index 87c50af..df5cc6b 100644
--- a/sound/soc/intel/boards/skl_hda_dsp_common.h
+++ b/sound/soc/intel/boards/skl_hda_dsp_common.h
@@ -29,6 +29,7 @@  struct skl_hda_private {
 	int pcm_count;
 	int dai_index;
 	const char *platform_name;
+	struct device_link *link;
 };
 
 extern struct snd_soc_dai_link skl_hda_be_dai_links[HDA_DSP_MAX_BE_DAI_LINKS];
diff --git a/sound/soc/intel/boards/skl_hda_dsp_generic.c b/sound/soc/intel/boards/skl_hda_dsp_generic.c
index b9a21e6..ceca11e 100644
--- a/sound/soc/intel/boards/skl_hda_dsp_generic.c
+++ b/sound/soc/intel/boards/skl_hda_dsp_generic.c
@@ -166,8 +166,20 @@  static int skl_hda_audio_probe(struct platform_device *pdev)
 	return devm_snd_soc_register_card(&pdev->dev, &hda_soc_card);
 }
 
+static int skl_hda_audio_remove(struct platform_device *pdev)
+{
+	struct snd_soc_card *card = platform_get_drvdata(pdev);
+	struct skl_hda_private *ctx = snd_soc_card_get_drvdata(card);
+
+	if (ctx->link)
+		device_link_del(ctx->link);
+
+	return 0;
+}
+
 static struct platform_driver skl_hda_audio = {
 	.probe = skl_hda_audio_probe,
+	.remove = skl_hda_audio_remove,
 	.driver = {
 		.name = "skl_hda_dsp_generic",
 		.pm = &snd_soc_pm_ops,