[alsa-devel] [PATCH V2] ASoC: fix hdmi codec driver contest in S3

[Takashi Iwai]

On Thu, 28 Mar 2019 08:10:34 +0100,
Yang, Libin wrote:
> 
> Hi Takashi,
> 
> >>> >> >In the case of legacy HD-audio, we "fixed" the problem by
> >>> >> >avoiding the trigger of async work at resume, but let it be
> >>> >> >triggered from runtime resume.  In ASoC case, however, it's no option.
> >>> >> >
> >>> >> >Maybe a possible solution in the sound driver side would be to
> >>> >> >move from system suspend/resume to ASoC component
> >>suspend/resume.
> >>> >> >The runtime suspend/resume can be kept as is, and call
> >>> >> >pm_runtime_force_suspend and resume from the component
> >>> >suspend/resume
> >>> >> >callbacks.  Since the component callbacks are certainly processed
> >>> >> >before DAPM events, this should assure the order.
> >>> >>
> >>> >> I tried to move display power setting from runtime suspend/resume
> >>> >> to component suspend/resume. I found there may be another issue:
> >>> >> playback will NOT call component suspend/resume.
> >>> >> This  means we will never have chance to set the display power if
> >>> >> we don't do the S3.
> >>> >
> >>> >The runtime suspend/resume are fine and need to be kept, I suppose.
> >>> >The only problem is the system suspend/resume callbacks that call
> >>> >pm_runtime_force_*().  Just moving these two to component would work?
> >>> >
> >>>
> >>> Let's see the flow of the suspend:
> >>> 1. Component suspend is called. It will call snd_hdac_display_power()
> >>> to turn off the display power.
> >>
> >>No, what I meant was to make the component suspend calling
> >>pm_runtime_force_suspend() instead of the system suspend.
> >>Ditto for the component resume calling pm_runtime_force_resume().
> >
> >It seems this component suspend/resume method works well.
> >I will do more test and send out the patch later.
> 
> I did some stress test. And I found if I remove the debug message,
> the issue can still be reproduced with component suspend/resume
> added. If I add the debug message, it will be much better.
> I'm not sure whether my patch is wrong or not. I've set the bus_control = 1
> in the dai driver.

I guess you forgot to remove the suspend/resume call from the codec
device PM?  Otherwise the codec device resume is still executed
concurrently.


Takashi

> 
> diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
> index 5eeb0fe..505efd9 100644
> --- a/sound/soc/codecs/hdac_hdmi.c
> +++ b/sound/soc/codecs/hdac_hdmi.c
> @@ -1900,9 +1900,31 @@ static int hdmi_codec_resume(struct device *dev)
>  #define hdmi_codec_resume NULL
>  #endif
> 
> +static int hdmi_component_suspend(struct snd_soc_component *component)
> +{
> +       struct hdac_hdmi_priv *hdmi = snd_soc_component_get_drvdata(component);
> +       struct hdac_device *hdev = hdmi->hdev;
> +
> +       pm_runtime_force_suspend(&hdev->dev);
> +
> +       return 0;
> +}
> +
> +static int hdmi_component_resume(struct snd_soc_component *component)
> +{
> +       struct hdac_hdmi_priv *hdmi = snd_soc_component_get_drvdata(component);
> +       struct hdac_device *hdev = hdmi->hdev;
> +
> +       pm_runtime_force_resume(&hdev->dev);
> +
> +       return 0;
> +}
> +
>  static const struct snd_soc_component_driver hdmi_hda_codec = {
>         .probe                  = hdmi_codec_probe,
>         .remove                 = hdmi_codec_remove,
> +       .suspend                = hdmi_component_suspend,
> +       .resume                 = hdmi_component_resume,
>         .use_pmdown_time        = 1,
>         .endianness             = 1,
>         .non_legacy_dai_naming  = 1,
> 
> >
> 
> Regards,
> Libin
>