diff mbox series

[v2] ALSA: hda/hdmi: fix silent stream for first playback to DP

Message ID 20201210174445.3134104-1-kai.vehmanen@linux.intel.com (mailing list archive)
State Accepted
Commit b1a5039759cb7bfcb2157f28604dbda0bca58598
Headers show
Series [v2] ALSA: hda/hdmi: fix silent stream for first playback to DP | expand

Commit Message

Kai Vehmanen Dec. 10, 2020, 5:44 p.m. UTC
A problem exists in enabling silent stream when connection type is
DisplayPort. Silent stream programming is completed when a new DP
receiver is connected, but infoframe transmission does not actually
start until PCM is opened for the first time. This can result in audible
gap of multiple seconds. This only affects the first PCM open.

Fix the issue by properly assigning a converter to the silent stream,
and modifying the required stream ID programming sequence.

This change only affects Intel display audio codecs.

BugLink: https://github.com/thesofproject/linux/issues/2468
Fixes: 951894cf30f4 ("ALSA: hda/hdmi: Add Intel silent stream support")
Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
---
 sound/pci/hda/patch_hdmi.c | 98 +++++++++++++++++++++++++++++++++-----
 1 file changed, 86 insertions(+), 12 deletions(-)

v2 changes:
 - locking missing in silent_stream_disable()

Comments

Takashi Iwai Dec. 10, 2020, 6:58 p.m. UTC | #1
On Thu, 10 Dec 2020 18:44:45 +0100,
Kai Vehmanen wrote:
> 
> A problem exists in enabling silent stream when connection type is
> DisplayPort. Silent stream programming is completed when a new DP
> receiver is connected, but infoframe transmission does not actually
> start until PCM is opened for the first time. This can result in audible
> gap of multiple seconds. This only affects the first PCM open.
> 
> Fix the issue by properly assigning a converter to the silent stream,
> and modifying the required stream ID programming sequence.
> 
> This change only affects Intel display audio codecs.
> 
> BugLink: https://github.com/thesofproject/linux/issues/2468
> Fixes: 951894cf30f4 ("ALSA: hda/hdmi: Add Intel silent stream support")
> Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
> ---
>  sound/pci/hda/patch_hdmi.c | 98 +++++++++++++++++++++++++++++++++-----
>  1 file changed, 86 insertions(+), 12 deletions(-)
> 
> v2 changes:
>  - locking missing in silent_stream_disable()

Applied now.  Thanks.


Takashi
Jan Alexander Steffens (heftig) Jan. 1, 2021, 5:15 a.m. UTC | #2
On Thu, 2020-12-10 at 19:44 +0200, Kai Vehmanen wrote:
> +static void silent_stream_disable(struct hda_codec *codec,
> +                                 struct hdmi_spec_per_pin *per_pin)
> +{
> +       struct hdmi_spec *spec = codec->spec;
> +       struct hdmi_spec_per_cvt *per_cvt;
> +       int cvt_idx;
> +
> +       mutex_lock(&per_pin->lock);
> +       if (!per_pin->silent_stream)
> +               goto unlock_out;
> +
> +       codec_dbg(codec, "HDMI: disable silent stream on pin-NID=0x%x
> cvt-NID=0x%x\n",
> +                 per_pin->pin_nid, per_pin->cvt_nid);
> +
> +       cvt_idx = cvt_nid_to_cvt_index(codec, per_pin->cvt_nid);
> +       if (cvt_idx >= 0 && cvt_idx < spec->num_cvts) {
> +               per_cvt = get_cvt(spec, cvt_idx);
> +               per_cvt->assigned = 0;
> +       }
> +
> +       per_pin->cvt_nid = 0;
> +       per_pin->silent_stream = false;
> +
> + unlock_out:
> +       mutex_unlock(&spec->pcm_lock);

Shouldn't this be `mutex_unlock(&per_pin->lock);`?

I'm experiencing deadlocks since 5.10.4, which backported this patch.
Takashi Iwai Jan. 1, 2021, 7:57 a.m. UTC | #3
On Fri, 01 Jan 2021 06:15:42 +0100,
Jan Alexander Steffens (heftig) wrote:
> 
> On Thu, 2020-12-10 at 19:44 +0200, Kai Vehmanen wrote:
> > +static void silent_stream_disable(struct hda_codec *codec,
> > +                                 struct hdmi_spec_per_pin *per_pin)
> > +{
> > +       struct hdmi_spec *spec = codec->spec;
> > +       struct hdmi_spec_per_cvt *per_cvt;
> > +       int cvt_idx;
> > +
> > +       mutex_lock(&per_pin->lock);
> > +       if (!per_pin->silent_stream)
> > +               goto unlock_out;
> > +
> > +       codec_dbg(codec, "HDMI: disable silent stream on pin-NID=0x%x
> > cvt-NID=0x%x\n",
> > +                 per_pin->pin_nid, per_pin->cvt_nid);
> > +
> > +       cvt_idx = cvt_nid_to_cvt_index(codec, per_pin->cvt_nid);
> > +       if (cvt_idx >= 0 && cvt_idx < spec->num_cvts) {
> > +               per_cvt = get_cvt(spec, cvt_idx);
> > +               per_cvt->assigned = 0;
> > +       }
> > +
> > +       per_pin->cvt_nid = 0;
> > +       per_pin->silent_stream = false;
> > +
> > + unlock_out:
> > +       mutex_unlock(&spec->pcm_lock);
> 
> Shouldn't this be `mutex_unlock(&per_pin->lock);`?

Oh yes, obviously.

> I'm experiencing deadlocks since 5.10.4, which backported this patch.

Could you check whether correcting the unlock fixes the problem?


thanks,

Takashi
Jan Alexander Steffens (heftig) Jan. 2, 2021, 1:23 a.m. UTC | #4
On Fri, Jan 1, 2021 at 8:57 AM Takashi Iwai <tiwai@suse.de> wrote:
> On Fri, 01 Jan 2021 06:15:42 +0100, Jan Alexander Steffens (heftig) wrote:
> > I'm experiencing deadlocks since 5.10.4, which backported this patch.
>
> Could you check whether correcting the unlock fixes the problem?

Yes, correcting the unlock fixes my problem.

Thanks,
Jan
diff mbox series

Patch

diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index b0068f8ca46d..2ddc27db8c01 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -78,6 +78,7 @@  struct hdmi_spec_per_pin {
 	int pcm_idx; /* which pcm is attached. -1 means no pcm is attached */
 	int repoll_count;
 	bool setup; /* the stream has been set up by prepare callback */
+	bool silent_stream;
 	int channels; /* current number of channels */
 	bool non_pcm;
 	bool chmap_set;		/* channel-map override by ALSA API? */
@@ -979,6 +980,13 @@  static int hdmi_choose_cvt(struct hda_codec *codec,
 	else
 		per_pin = get_pin(spec, pin_idx);
 
+	if (per_pin && per_pin->silent_stream) {
+		cvt_idx = cvt_nid_to_cvt_index(codec, per_pin->cvt_nid);
+		if (cvt_id)
+			*cvt_id = cvt_idx;
+		return 0;
+	}
+
 	/* Dynamically assign converter to stream */
 	for (cvt_idx = 0; cvt_idx < spec->num_cvts; cvt_idx++) {
 		per_cvt = get_cvt(spec, cvt_idx);
@@ -1642,30 +1650,95 @@  static void hdmi_present_sense_via_verbs(struct hdmi_spec_per_pin *per_pin,
 	snd_hda_power_down_pm(codec);
 }
 
+#define I915_SILENT_RATE		48000
+#define I915_SILENT_CHANNELS		2
+#define I915_SILENT_FORMAT		SNDRV_PCM_FORMAT_S16_LE
+#define I915_SILENT_FORMAT_BITS	16
+#define I915_SILENT_FMT_MASK		0xf
+
 static void silent_stream_enable(struct hda_codec *codec,
-				struct hdmi_spec_per_pin *per_pin)
+				 struct hdmi_spec_per_pin *per_pin)
 {
-	unsigned int newval, oldval;
-
-	codec_dbg(codec, "hdmi: enabling silent stream for NID %d\n",
-			per_pin->pin_nid);
+	struct hdmi_spec *spec = codec->spec;
+	struct hdmi_spec_per_cvt *per_cvt;
+	int cvt_idx, pin_idx, err;
+	unsigned int format;
 
 	mutex_lock(&per_pin->lock);
 
-	if (!per_pin->channels)
-		per_pin->channels = 2;
+	if (per_pin->setup) {
+		codec_dbg(codec, "hdmi: PCM already open, no silent stream\n");
+		goto unlock_out;
+	}
 
-	oldval = snd_hda_codec_read(codec, per_pin->pin_nid, 0,
-			AC_VERB_GET_CONV, 0);
-	newval = (oldval & 0xF0) | 0xF;
-	snd_hda_codec_write(codec, per_pin->pin_nid, 0,
-			AC_VERB_SET_CHANNEL_STREAMID, newval);
+	pin_idx = pin_id_to_pin_index(codec, per_pin->pin_nid, per_pin->dev_id);
+	err = hdmi_choose_cvt(codec, pin_idx, &cvt_idx);
+	if (err) {
+		codec_err(codec, "hdmi: no free converter to enable silent mode\n");
+		goto unlock_out;
+	}
+
+	per_cvt = get_cvt(spec, cvt_idx);
+	per_cvt->assigned = 1;
+	per_pin->cvt_nid = per_cvt->cvt_nid;
+	per_pin->silent_stream = true;
 
+	codec_dbg(codec, "hdmi: enabling silent stream pin-NID=0x%x cvt-NID=0x%x\n",
+		  per_pin->pin_nid, per_cvt->cvt_nid);
+
+	snd_hda_set_dev_select(codec, per_pin->pin_nid, per_pin->dev_id);
+	snd_hda_codec_write_cache(codec, per_pin->pin_nid, 0,
+				  AC_VERB_SET_CONNECT_SEL,
+				  per_pin->mux_idx);
+
+	/* configure unused pins to choose other converters */
+	pin_cvt_fixup(codec, per_pin, 0);
+
+	snd_hdac_sync_audio_rate(&codec->core, per_pin->pin_nid,
+				 per_pin->dev_id, I915_SILENT_RATE);
+
+	/* trigger silent stream generation in hw */
+	format = snd_hdac_calc_stream_format(I915_SILENT_RATE, I915_SILENT_CHANNELS,
+					     I915_SILENT_FORMAT, I915_SILENT_FORMAT_BITS, 0);
+	snd_hda_codec_setup_stream(codec, per_pin->cvt_nid,
+				   I915_SILENT_FMT_MASK, I915_SILENT_FMT_MASK, format);
+	usleep_range(100, 200);
+	snd_hda_codec_setup_stream(codec, per_pin->cvt_nid, I915_SILENT_FMT_MASK, 0, format);
+
+	per_pin->channels = I915_SILENT_CHANNELS;
 	hdmi_setup_audio_infoframe(codec, per_pin, per_pin->non_pcm);
 
+ unlock_out:
 	mutex_unlock(&per_pin->lock);
 }
 
+static void silent_stream_disable(struct hda_codec *codec,
+				  struct hdmi_spec_per_pin *per_pin)
+{
+	struct hdmi_spec *spec = codec->spec;
+	struct hdmi_spec_per_cvt *per_cvt;
+	int cvt_idx;
+
+	mutex_lock(&per_pin->lock);
+	if (!per_pin->silent_stream)
+		goto unlock_out;
+
+	codec_dbg(codec, "HDMI: disable silent stream on pin-NID=0x%x cvt-NID=0x%x\n",
+		  per_pin->pin_nid, per_pin->cvt_nid);
+
+	cvt_idx = cvt_nid_to_cvt_index(codec, per_pin->cvt_nid);
+	if (cvt_idx >= 0 && cvt_idx < spec->num_cvts) {
+		per_cvt = get_cvt(spec, cvt_idx);
+		per_cvt->assigned = 0;
+	}
+
+	per_pin->cvt_nid = 0;
+	per_pin->silent_stream = false;
+
+ unlock_out:
+	mutex_unlock(&spec->pcm_lock);
+}
+
 /* update ELD and jack state via audio component */
 static void sync_eld_via_acomp(struct hda_codec *codec,
 			       struct hdmi_spec_per_pin *per_pin)
@@ -1701,6 +1774,7 @@  static void sync_eld_via_acomp(struct hda_codec *codec,
 				pm_ret);
 			silent_stream_enable(codec, per_pin);
 		} else if (monitor_prev && !monitor_next) {
+			silent_stream_disable(codec, per_pin);
 			pm_ret = snd_hda_power_down_pm(codec);
 			if (pm_ret < 0)
 				codec_err(codec,