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

[Yang, Libin]

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?

Regards,
Libin

>
>Also, moving to component PM would solve the problem, too, since it also
>assures finishing the codec resume before the schedule_work() call in
>snd_soc_resume().
>
>
>thanks,
>
>Takashi