[alsa-devel] [PATCH] ALSA: hda - delay resume haswell hdmi codec in system resume

[Takashi Iwai]

At Mon, 08 Apr 2013 11:49:09 +0200,
David Henningsson wrote:
> 
> On 04/07/2013 02:10 AM, Lin, Mengdong wrote:
> >> -----Original Message-----
> >> From: Takashi Iwai [mailto:tiwai at suse.de]
> >> Sent: Saturday, April 06, 2013 1:05 AM
> >
> >
> >>>>> 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.
> >>>>
> >>>
> >>> I think there is no race for Haswell. In my test I observed that the
> >>> unsol evnet will not be received until intel_haswell_wait_ready_to_resume()
> >> enables the unsol event on the pins.
> >>> So I put "spec->ready_to_resume = 0" before enabling the unsol event.
> >>
> >> Practically your patch would work well, but theoretically the unsol event might
> >> be issued and handled by hdmi_intrinsic_event() before you go into
> >> intel_haswell_wait_ready_to_resume().  That is, you _clear_
> >> spec->ready_to_resume again and wait for the unsol event that had been
> >> already processed.  That's the race David pointed.
> >>
> >> The fix is simply not to clear spec->ready_to_resume in
> >> intel_haswell_wait_ready_to_resume() but clear it much earlier already before
> >> the resume path as my patch does.  Then wait_event() passes immediately
> >> (and the unsol enablement of is anyway harmless).
> >>
> >
> > Hi Takashi,
> >
> > I see. Thanks for clarifying!
> >
> > I'll revise the patch and test it again when I'm in office on Monday.
> > Also I'll rebase the patch to for-next branch of sound git tree and fix the indentations and spaces errors.
> 
> I'm still not understanding this patch.
> 
> We of course cannot have a situation where HDMI jack detection is not 
> correctly working after S3, which looks like would be the case here?

No.  The patch itself has nothing to do with the HDMI jack detection
at all.  This intrinsic unsol event is (according to Intel) guaranteed
to be issued once at the audio codec initialization, at least for
Haswell.

> Second, I just saw on a machine we're working on, the symptoms: one of 
> the pins in D3 and the right channel muted. And this was on a clean 
> boot, not after S3 suspend/resume cycle.

Maybe another problem.  Maybe not.  Does the problem persist if you
reload the driver (without invocation of alsactl via udev)?

> To look at the problem again: If the problem is that something must be 
> done on the graphics driver side first and then on the audio side, 
> wouldn't the solution be for the video driver and audio driver to 
> communicate somehow?

Yes, the pm domain support is necessary sooner or later, as I already
discussed shortly with Dave Airlie.  However...

> And resume (and possibly init?) would not complete 
> until first the graphics driver has done its resume, and after that, the 
> audio driver. I e, userspace will remain frozen until both drivers have 
> completed a correct resume.

... the resume problem can't be fixed only by the usual pm domain
serialization.  The graphics initialization for the audio bit is done
asynchronously, thus we can't guarantee the audio codec is really
ready after the graphics driver returns from the resume callback.
It shows as if it's ready (you can turn it to D0) but actually it
doesn't change on the hardware.  Thus, the serialization with an unsol
event wait seems  mandatory for the time being.


Takashi