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

[Yang, Libin]

Hi Takashi,

>-----Original Message-----
>From: Takashi Iwai [mailto:tiwai at suse.de]
>Sent: Tuesday, March 26, 2019 7:23 PM
>To: Yang, Libin <libin.yang at intel.com>
>Cc: alsa-devel at alsa-project.org; broonie at kernel.org
>Subject: Re: [alsa-devel] [PATCH V2] ASoC: fix hdmi codec driver contest in S3
>
>On Tue, 26 Mar 2019 12:19:15 +0100,
>Yang, Libin wrote:
>>
>> Hi Takashi,
>>
>> >> Hi Takashi,
>> >>
>> >> >> >
>> >> >> >On Tue, 26 Mar 2019 06:42:15 +0100, Yang, Libin wrote:
>> >> >> >(snip)
>> >> >> >> Hi Takashi,
>> >> >> >> Below is my draft patch, which doesn't work with
>> >> >> >> pm_runtime_get_sync(). Is there anything wrong in my code?
>> >> >> >(snip)
>> >> >> >> And the dmesg is:
>> >> >> >> [   36.087224] HDMI HDA Codec ehdaudio0D2: in
>hdmi_codec_resume
>> >> >1890
>> >> >> >ylb
>> >> >> >> [   36.087230] HDMI HDA Codec ehdaudio0D2: in
>> >> >> >hdac_hdmi_runtime_resume 2122 ylb 1
>> >> >> >> [   36.087335] HDMI HDA Codec ehdaudio0D2: in
>> >> >> >hdac_hdmi_cvt_output_widget_event 812 ylb
>> >> >> >> [   40.097586] HDMI HDA Codec ehdaudio0D2: in
>> >> >> >hdac_hdmi_runtime_resume 2135 ylb 2
>> >> >> >> [   40.097766] HDMI HDA Codec ehdaudio0D2: in
>> >> >> >hdac_hdmi_pin_output_widget_event 767 ylb
>> >> >> >> [   45.108632] HDMI HDA Codec ehdaudio0D2: in
>> >> >> >hdac_hdmi_pin_mux_widget_event 857 ylb
>> >> >> >>
>> >> >> >> >From the dmesg, hdac_hdmi_cvt_output_widget_event()
>> >> >> >> is called between "ylb 1" and "ylb 2" in runtime_resume().
>> >> >> >> This means xxx_event() registers setting runs before display
>> >> >> >> power is turned on.
>> >> >> >
>> >> >> >OK, now I understood what went wrong.  It's a similar issue as
>> >> >> >we've hit for the legacy HD-audio and figured out recently.
>> >> >> >
>> >> >> >If my understanding is correct, the problem is the call of
>> >> >> >pm_runtime_force_resume() in PM resume callback combined with
>> >> >> >an async work.  pm_runtime_force_resume() sets the runtime
>> >> >> >state immediately to RPM_ACTIVE even before finishing the task.
>> >> >> >The code seems assuming blindly that there is no other
>> >> >> >conflicting task while
>> >> >> >S3/S4 resume, but this is a problem.  That's why the next
>> >> >> >pm_runtime_get_sync() wasn't blocked but just went through;
>> >> >> >since the RPM flag was already set to RPM_ACTIVE in
>> >> >> >pm_runtime_force_resume(), it thought as if it were already
>> >> >> >active, while the real runtime resume code
>> >> >was still processing the callback.
>> >> >>
>> >> >> Yes, exactly right. And I have checked
>> >> >> dev->power.runtime_status, it's RPM_ACTIVE when xxx_event() calls
>pm_runtime_get_sync().
>> >> >>
>> >> >> >
>> >> >> >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 have worked out another patch. This patch decouples the
>> >> >> xxx_event() and runtime suspend/resume. This means in whichever
>> >> >> case, xxx_event() can make sure display power is in the correct status.
>> >What do you think?
>> >> >> On the same time, I will try the component suspend/resume. My
>> >> >> understanding is that snd_hdac_display_power() should be called
>> >> >> either in runtime_suspend/ resume or in component suspend/resume.
>> >> >
>> >> >This might work around the particular case, yes.  However it still
>> >> >makes me uneasy as the root cause isn't full covered -- the other
>> >> >part of runtime resume might be still pending while executing the
>> >> >DAPM
>> >event.
>> >> >
>> >> >Now, considering the idea with device_link_add() again: I guess
>> >> >it's
>> >> >snd_soc_resume() who kicks off the DAPM even work?  If so, and if
>> >> >snd_soc_resume() gets called from the machine driver, we can
>> >> >assure the PM order via device_link_add() so that the codec driver
>> >> >is resumed before the machine driver.  Then at the time the
>> >> >deferred resume work is executed, the codec is ready, so the display
>power is on.
>> >>
>> >> Yes, snd_soc_resume() kicks off the DAPM event work. The device PM
>> >> sequence is good. The root cause is when hdmi runtime_resume calls
>> >> snd_hdac_display_power(), it may sleep. The flow is:
>> >>
>> >> 1. HDMI runtime_resume runs
>> >> 2. HDMI runtime_resume calls snd_hdac_display_power(), but display
>> >> driver is also operating display power and the mutex_lock is hold.
>> >> So HDMI runtime_resume sleeps.
>> >> 3. the deferred work is scheduled and xxx_event() is called.
>> >>
>> >> This is wrong. All these happen because of the mutex_lock is hold
>> >> by display driver, which causes HDMI runtime_resume sleep.
>> >
>> >Well, the mutex lock itself is OK, and it's designed to behave so.
>> >
>> >As mentioned, the root cause is pm_runtime_force_resume() that sets
>> >RPM_ACTIVE while a concurrent task is running.
>> >
>> >OTOH, this can be seen indeed as a PM dependency between devices, and
>> >if we set the order explicitly, the problem can be avoided, too.
>> >Then the runtime resume of codec finishes before snd_soc_resume() is
>> >called from the machine driver resume.
>>
>> Do you have any idea to set the order explicitly? What I can think of
>> is that we set HDMI device to be the machine device parent?
>
>device_link_add() should serve for defining the explicit PM dependency.

Get it. I will check this function. Thanks.

Regards,
Libin

>
>
>Takashi