[alsa-devel] [PATCH] ASoC: cAVS: add device_link to HDMI audio

[Ranjani Sridharan]

On Mon, 2019-04-08 at 08:14 -0500, Pierre-Louis Bossart wrote:
> 
> On 4/7/19 8:21 PM, libin.yang at intel.com wrote:
> > From: Libin Yang <libin.yang at 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 at 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 at alsa-project.org
> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel