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

From: Takashi Iwai <tiwai@suse.de>
To: "Yang, Libin" <libin.yang@intel.com>
Cc: "'alsa-devel@alsa-project.org'" <alsa-devel@alsa-project.org>,
	"'broonie@kernel.org'" <broonie@kernel.org>
Subject: Re: [PATCH V2] ASoC: fix hdmi codec driver contest in S3
Date: Thu, 28 Mar 2019 08:31:50 +0100	[thread overview]
Message-ID: <s5hsgv7o3o9.wl-tiwai@suse.de> (raw)
In-Reply-To: <96A12704CE18D347B625EE2D4A099D19527FD51D@SHSMSX103.ccr.corp.intel.com>

On Thu, 28 Mar 2019 08:28:06 +0100,
Yang, Libin wrote:
> 
> 
> >-----Original Message-----
> >From: Takashi Iwai [mailto:tiwai@suse.de]
> >Sent: Thursday, March 28, 2019 3:15 PM
> >To: Yang, Libin <libin.yang@intel.com>
> >Cc: 'alsa-devel@alsa-project.org' <alsa-devel@alsa-project.org>;
> >'broonie@kernel.org' <broonie@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()?

It needs pm_runtime_force_resume() if it's suspended by
pm_runtime_force_suspend(), I suppose.

Takashi