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

[Yang, Libin]

>-----Original Message-----
>From: Takashi Iwai [mailto:tiwai at suse.de]
>Sent: Thursday, March 28, 2019 3:15 PM
>To: Yang, Libin <libin.yang at intel.com>
>Cc: 'alsa-devel at alsa-project.org' <alsa-devel at alsa-project.org>;
>'broonie at kernel.org' <broonie at kernel.org>
>Subject: Re: [alsa-devel] [PATCH V2] ASoC: fix hdmi codec driver contest in S3
>
>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.

... Yes, I forgot it. Thanks for reminder. By the way, do you think it is better to
use pm_runtime_force_resume() or pm_runtime_get_sync()?

Regards,
Libin

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