Re: [BUG] ASoC: Intel: sof_es8336: speaker amp GPIO stays off after stream stop/restart (no sound until "Speaker Switch" is toggled)

[Pierre-Louis Bossart <pierre-louis.bossart@linux.dev>]

> Problem
> -------
> The speaker randomly goes silent, typically when a browser stops and
> restarts playback (for example YouTube ad transitions). While it is
> silent the PCM is RUNNING and hw_ptr advances at 48 kHz, PipeWire reports
> no errors, the ALSA mixer is unchanged and nothing is logged in dmesg.
> Toggling "Speaker Switch" off and on restores the sound without closing
> the PCM. Before I worked around it, this happened 15 times in about 10
> hours of use on one day.
my money is on the asymmetric code structure which mixes DAPM and stream
triggers, see below.

> Evidence
> --------
> I captured the full DAPM debugfs tree, the ES8336 regmap registers and
> /sys/kernel/debug/gpio in the broken and in the working state. Everything
> is identical (including "Speaker Power: On" and "Speaker: On") except the
> speaker enable GPIO:
> 
>   broken:   gpio-541 (speakers-enable) out lo ACTIVE LOW
>   working:  gpio-541 (speakers-enable) out hi ACTIVE LOW
> 
> Setting pmdown_time to 0 on the codec and BE links did not prevent it.
> 
> Analysis
> --------
> In sound/soc/intel/boards/sof_es8336.c, sof_8336_trigger() drives the
> speaker GPIO to the "off" level on PAUSE_PUSH, SUSPEND and STOP without
> updating priv->speaker_en, while START, PAUSE_RELEASE and RESUME do
> nothing. The GPIO is only restored by sof_es8316_speaker_power_event()
> when DAPM actually powers the "Speaker Power" supply down and up again.
> If the stream is stopped and started again without that full power
> cycle, nothing re-enables the amplifier: the GPIO stays off while DAPM
> still considers the speaker on. I have not determined which exact path
> triggers it on my machine (pause/resume, xrun recovery, or a restart
> within pmdown_time).

Does this happen if the time between stop and restart is greater than
3s, or only when the delta is really short? In the latter case, IIRC
there's a DAPM feature where the power down happens with a delay, to
avoid unnecessary power transitions. so if the new stream starts
immediately, then the power-up transition will never happen.

> Proposed change (sketch, not a formal patch)
> --------------------------------------------
> Re-enable the amplifier on resume when the driver believes it is on:
> 
> case SNDRV_PCM_TRIGGER_START:
> case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> case SNDRV_PCM_TRIGGER_RESUME:
> + if (substream->stream == 0 && priv->speaker_en == false)
> + schedule_delayed_work(&priv->pcm_pop_work,
> +      msecs_to_jiffies(70));
> break;

alternatively you could deal with power management only in
sof_es8316_speaker_power_event(), and completely remove the transitions
on trigger start/stop/pause. In general it's a bad idea to add delays to
the trigger, it's supposed to be real fast.