From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takashi Iwai Subject: Re: [PATCH] ALSA: hda - delay resume haswell hdmi codec in system resume Date: Fri, 05 Apr 2013 15:08:05 +0200 Message-ID: References: <1364321572-2634-1-git-send-email-mengdong.lin@intel.com> <51517F53.7070608@canonical.com> <5152AB91.4050501@canonical.com> <515AC6AC.8040504@canonical.com> <515AD3E4.2000208@canonical.com> Mime-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mx2.suse.de (cantor2.suse.de [195.135.220.15]) by alsa0.perex.cz (Postfix) with ESMTP id B2634265FA5 for ; Fri, 5 Apr 2013 15:08:05 +0200 (CEST) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org To: David Henningsson Cc: "Lin, Mengdong" , "alsa-devel@alsa-project.org" List-Id: alsa-devel@alsa-project.org At Fri, 05 Apr 2013 15:01:47 +0200, Takashi Iwai wrote: > > At Tue, 02 Apr 2013 14:49:40 +0200, > David Henningsson wrote: > > > > On 04/02/2013 02:18 PM, Takashi Iwai wrote: > > > At Tue, 02 Apr 2013 13:53:16 +0200, > > > David Henningsson wrote: > > >> > > >> On 03/27/2013 11:01 AM, Lin, Mengdong wrote: > > >>>> -----Original Message----- > > >>>> From: David Henningsson [mailto:david.henningsson@canonical.com] > > >>>> Sent: Wednesday, March 27, 2013 4:19 PM > > >>> > > >>>>> The Haswell codec set_power_state ops (intel_haswell_set_power_state) will > > >>>> only wait if this is a delayed resume and clear this flag after waiting. And > > >>>> actually, there is no waiting even in this case. Because when 1st user operation > > >>>> after system resume happens, Gfx already finishes resuming and audio > > >>>> initialization, so as long as intel_haswell_wait_ready_to_resume() enable the > > >>>> unsol event, the unsol event comes and so so waiting finishes. The 300ms time > > >>>> out is set for safety consideration in case unsol event is lost. I've not observed > > >>>> any unsol event lost till now. > > >>>> > > >>>> As for the timeout, I suggest you use the codec->bus->workq instead of > > >>>> creating a new workq. I think that will also give us some serialisation, i e, > > >>>> protection against race conditions if the timeout happen at the same time as > > >>>> the unsol event. > > >>> > > >>> Hi David, > > >>> > > >>> The new added "resume_wq" for hdmi codec is a wait queue, not a work queue like codec->bus->workq. > > >>> It's expected to wake up as soon as the unsol event is got. > > >> > > >> Sure; but I don't see why you need a wait queue for that? Why don't you > > >> just call the resume path from the unsol event handler > > >> (hdmi_present_sense, or its caller), and then also cancel the timeout > > >> handler (which can then be in the normal workq)? > > > > > > Because the delayed resume actually fakes as if the resume is done. > > > This is necessary not to block other device's resume operation. > > > > > > Since it looks as if ready, user-space might restart things soon again > > > before the delayed resume is really finished. So, some serialization > > > is required there. > > > > Lin, would it be possible to add some chain of events description to the > > bug commit? The different contexts are just boggling my mind :-) > > Yeah, a nice code flow picture would be helpful :) > > > Assume we have two cases, system idle after S3 and immediate playback > > after S3. > > > > Is this correct: > > > > System idle: > > 1. System skips hda_call_codec_resume and sets codec->resume_delayed. > > 2. Since the codec is not reinitialized, nothing powers up the codec, > > so this is all that happens. > > 3. Since unsol events are not enabled (?), we have a bug that jack > > detection does not work and cannot be detected from userspace...? > > > > Immediate playback: > > 1. System skips hda_call_codec_resume and sets codec->resume_delayed. > > 2. Process context wants to start playback, which powers up the codec > > and calls intel_haswell_set_power_state. > > 3. intel_haswell_wait_ready_to_resume enables unsol events and starts > > to wait on ready_to_resume > > 4. Gfx init finishes, and workq context fires the unsol event, which > > calls hdmi_intrinsic_event, which triggers the resume_wq. > > 5. Process context and workq context now continues in parallel, > > potentially (but hopefully not) leading to race conditions? > > The current patch has a small race, but it can be avoided by clearing > spec->ready_to_resume early enough, e.g. in suspend callback or in > init callback, like the patch below. > > > Takashi > > --- > diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c > index 67e11ba..d0eafee 100644 > --- a/sound/pci/hda/patch_hdmi.c > +++ b/sound/pci/hda/patch_hdmi.c > @@ -1740,6 +1740,9 @@ static int generic_hdmi_init(struct hda_codec *codec) > hdmi_init_pin(codec, pin_nid); > snd_hda_jack_detect_enable(codec, pin_nid, pin_nid); > } > + > + spec->ready_to_resume = 0; > + Here needs #ifdef CONFIG_PM, obviously... Takashi > return 0; > } > > @@ -1876,8 +1879,6 @@ static void intel_haswell_wait_ready_to_resume(struct hda_codec *codec) > struct hdmi_spec *spec = codec->spec; > int pin_idx; > > - spec->ready_to_resume = 0; > - > for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) { > struct hdmi_spec_per_pin *per_pin; > hda_nid_t pin_nid; > _______________________________________________ > Alsa-devel mailing list > Alsa-devel@alsa-project.org > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel >