Re: [PATCH V2] ASoC: fix hdmi codec driver contest in S3

[Takashi Iwai <tiwai@suse.de>]

On Tue, 26 Mar 2019 08:58:34 +0100,
Yang, Libin wrote:
> 
> Hi Takashi,
> 
> 
> >-----Original Message-----
> >From: Takashi Iwai [mailto:tiwai@suse.de]
> >Sent: Tuesday, March 26, 2019 3:37 PM
> >To: Yang, Libin <libin.yang@intel.com>
> >Cc: alsa-devel@alsa-project.org; broonie@kernel.org
> >Subject: Re: [alsa-devel] [PATCH V2] ASoC: fix hdmi codec driver contest in S3
> >
> >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.


thanks,

Takashi