[alsa-devel] [PATCH 0/4] A few more extensions to LPE audio driver

[Takashi Iwai]

On Mon, 13 Feb 2017 23:56:36 +0100,
Pierre-Louis Bossart wrote:
> 
> 
> 
> On 02/13/2017 08:39 AM, Takashi Iwai wrote:
> > Hi,
> >
> > Linus decided to take another week for 4.11 merge window, so I could
> > play a bit more, and here is the result :)
> >
> > The first patch is the fix for possible GPU hang, and this has been
> > for a while in my local branch and tested.  This should be OK to merge
> > soon.  (It's also as same as what Ian tested in the last weekend.)
> >
> > The next one is merely a cleanup patch, so it's fine, too.  The third
> > one is a transition to use the runtime PM autosuspend, and it's a
> > preliminary for the last change.
> >
> > The last one is the new and experimental one: it enables the keep-link
> > feature.  With this patch, the device is managed as default via
> > autosuspend, and the link is opened (but silenced) for the pre-given
> > time (2 seconds) after the PCM close.  If the application restarts the
> > stream quickly, the receiver is still warm and can resume gracefully.
> >
> > All these patches are found in topic/intel-lpe-audio branch.
> > (BTW, I cleaned up my branches: now for-next branch tracks the latest
> >   stable patches to be merged, while topic/intel-lpe-audio branch
> >   tracks the experimental patches.  Beware that the latter may be
> >   rebased frequently.)
> I tried this topic/intel-lpe-audio branch on my Baytrail compute stick
> and CHT boards and I see a couple of problems while monkey testing.
> 
> 1. PulseAudio fails to load the HDMI card on startup on the Compute
> Stick and only shows the dummy output, killing and restarting
> PulseAudio solves the problem. I had not seen this before and this
> doesn't happen on CHT devices, not sure if this a pulseaudio problem?

I guess so.  Could you try to remove ~/.config/pulse and see whether
it still happens?


> 2. we had a set of errors in the past that are still present, i see
> them 100% of the time:
> E: [alsa-sink-HdmiLpeAudio] alsa-sink.c: ALSA woke us up to write new
> data to the device, but there was a
> ctually nothing to write!
> E: [alsa-sink-HdmiLpeAudio] alsa-sink.c: Most likely this is a bug in
> the ALSA driver 'snd_hdmi_lpe_audio
> '. Please report this issue to the ALSA developers.
> E: [alsa-sink-HdmiLpeAudio] alsa-sink.c: We were woken up with POLLOUT
> set -- however a subsequent snd_pc
> m_avail() returned 0 or another value < min_avail.

It's a oft-seen issue but interesting.  Though, I wonder whether PA
setup is proper when I looked at the below:

> 3. the third one is new, we have something wrong with the pointer
> management. This happened only once in my tests but still, not sure
> how it was introduced.
> 
> E: [alsa-sink-HdmiLpeAudio] alsa-util.c: snd_pcm_avail() returned a
> value that is exceptionally large: 13
> 6128 bytes (709 ms).
> E: [alsa-sink-HdmiLpeAudio] alsa-util.c: Most likely this is a bug in
> the ALSA driver 'snd_hdmi_lpe_audio
> '. Please report this issue to the ALSA developers.
> E: [alsa-sink-HdmiLpeAudio] alsa-util.c: snd_pcm_dump():
> E: [alsa-sink-HdmiLpeAudio] alsa-util.c: Hardware PCM card 0 'Intel
> HDMI/DP LPE Audio' device 0 subdevice
>  0
> E: [alsa-sink-HdmiLpeAudio] alsa-util.c: Its setup is:
> E: [alsa-sink-HdmiLpeAudio] alsa-util.c:   stream       : PLAYBACK
> E: [alsa-sink-HdmiLpeAudio] alsa-util.c:   access       : MMAP_INTERLEAVED
> E: [alsa-sink-HdmiLpeAudio] alsa-util.c:   format       : S16_LE
> E: [alsa-sink-HdmiLpeAudio] alsa-util.c:   subformat    : STD
> E: [alsa-sink-HdmiLpeAudio] alsa-util.c:   channels     : 2
> E: [alsa-sink-HdmiLpeAudio] alsa-util.c:   rate         : 48000
> E: [alsa-sink-HdmiLpeAudio] alsa-util.c:   exact rate   : 48000 (48000/1)
> E: [alsa-sink-HdmiLpeAudio] alsa-util.c:   msbits       : 16
> E: [alsa-sink-HdmiLpeAudio] alsa-util.c:   buffer_size  : 3840
> E: [alsa-sink-HdmiLpeAudio] alsa-util.c:   period_size  : 480

See the values above.

> E: [alsa-sink-HdmiLpeAudio] alsa-util.c:   period_time  : 10000
> E: [alsa-sink-HdmiLpeAudio] alsa-util.c:   tstamp_mode  : ENABLE
> E: [alsa-sink-HdmiLpeAudio] alsa-util.c:   period_step  : 1
> E: [alsa-sink-HdmiLpeAudio] alsa-util.c:   avail_min    : 480
> E: [alsa-sink-HdmiLpeAudio] alsa-util.c:   period_event : 1
> E: [alsa-sink-HdmiLpeAudio] alsa-util.c:   start_threshold  : -1
> E: [alsa-sink-HdmiLpeAudio] alsa-util.c:   stop_threshold   :
> 8646911284551352320
> E: [alsa-sink-HdmiLpeAudio] alsa-util.c:   silence_threshold: 0
> E: [alsa-sink-HdmiLpeAudio] alsa-util.c:   silence_size : 0
> E: [alsa-sink-HdmiLpeAudio] alsa-util.c:   boundary     :
> 8646911284551352320
> E: [alsa-sink-HdmiLpeAudio] alsa-util.c:   appl_ptr     : 301456
> E: [alsa-sink-HdmiLpeAudio] alsa-util.c:   hw_ptr       : 331680
> 
> Indeed this looks like a wrap-around?

Why PA takes such a small buffer / period size (3840 / 480)?
Do you have any special setup?

My test was with a bit old PA version, but it worked with a larger
buffer, IIRC.


> 4. the keep-link thing doesn't seem to work for me. Once PulseAudio
> suspends the output, if I try to play a short notification I lose the
> first second (as in hearing only 'left' in 'front left', as in with
> the first sound I play. I even increased the threshold to 10s and
> still no joy. Could there be a higher level suspend that turns the
> link off?

OK, that's somewhat expected in a current patch.  It keeps the link
*after* the PCM device is closed, but it doesn't change the behavior
so much *during* the PCM is being used by app.  So PA keeps the device
opened (maybe in PREPARED state?) after stopping the stream, and at
this state, the link gets off because it's prepared for the playback
trigger.

I don't know how we can avoid this: basically the silent output is a
fake underrun.  We need to set the invalid BDs to achieve it.
Meanwhile, at PREPARED state, the buffer and the h/w must be set ready
for streaming and just wait for the stream start toggle at TRIGGER.

Maybe there is some other way to achieve, but then I'd like to know
the reference implementation before further hacking.

> 5. if I change the display resolution while playing a sound through
> hw:0, at the ALSA level the playback stops with all sorts of bad
> descriptor errors, which is fine. If I play through PulseAudio the
> card is unloaded when the resolution is changed and the sound keeps
> playing but to the dummy output. PulseAudio needs to be killed and
> restarted to play sound again over HDMI. Wondering if this is the same
> problem as for the first case, PulseAudio not able to digest a uevent
> when a card is created?

The problem is that it's no "card" plug.  It's what I pointed in the
past about PCM DISCONNECT state handling.  The card itself isn't
changed but only the PCM state changes.  We could set PCM as
DISCONNECTED, and then PA would treat like the card removal, and the
device will be never reprobed unless the driver is really reprobed.
It's like what you've seen.  So, in the recent version, it never sets
DISCONNECTED but it just stops the stream and rest returns -ENODEV
error.

I'm not sure how PA react to this error case.  Maybe -ENODEV is a bad
choice.  Need to ask Tanu about it.


> 6. if I remove the HDMI output and reinsert it, PulseAudio again fails
> to detect. I have to kill pulseaudio and restart it. I also saw this
> log during plug/unplug
> W: [alsa-sink-HdmiLpeAudio] alsa-util.c: Could not recover from
> POLLERR|POLLNVAL|POLLHUP with snd_pcm_pre
> pare(): No such device

A similar situation, but it's strange that it still returns -ENODEV.
In the current code, -ENODEV should be returned only when the device
is "connected", i.e. the mode is set.

But now looking at the code, I found a superfluous code at hotplug
handler that should be removed:

-- 8< --
--- a/sound/x86/intel_hdmi_audio.c
+++ b/sound/x86/intel_hdmi_audio.c
@@ -1401,16 +1401,6 @@ static void had_process_hot_plug(struct snd_intelhad *intelhaddata)
 			__func__, __LINE__);
 	spin_unlock_irq(&intelhaddata->had_spinlock);
 
-	/* Safety check */
-	substream = had_substream_get(intelhaddata);
-	if (substream) {
-		dev_dbg(intelhaddata->dev,
-			"Force to stop the active stream by disconnection\n");
-		/* Set runtime->state to hw_params done */
-		snd_pcm_stop(substream, SNDRV_PCM_STATE_SETUP);
-		had_substream_put(intelhaddata);
-	}
-
 	had_build_channel_allocation_map(intelhaddata);
 }
 
-- 8< --

In anyway I'm going to check the behavior on my device, and reduce the
superfluous code.


> 7. I can't get the multichannel sound to work, anything with more that
> 2ch is silent except for FR and FL. Playback keeps going but only 2
> channels are playing. Not sure if this is my receiver sending bad ELD
> information or just that the hw_params are not limited to the
> capabilities of the display.

Could you check ELD output?  Now it's easily seen via ctl element.
I have no surround receiver, so cannot check the capabilities,
unfortunately.

About the multi-channel support, I haven't changed the code.
For the multi-channels, the driver does:
- Set AUD_CONFIG num_ch fields to channles-2
- Set AUD_CONFIG layout bit to 1
- Set IF2 chnl_cnt to channels-1
- Set IF2 chnl_alloc to CA value


> Overall it's pretty robust though compared to the legacy patches,
> except for the pulseaudio detection issue there was never a moment
> where I had to reboot or do something drastic to recover. Thanks for
> all your work Takashi!

Thanks for your testing!


Takashi