[RFC PATCH v3 0/3] support DP MST audio

[RFC PATCH v3 0/3] support DP MST audio

[libin.yang]

From: Libin Yang <libin.yang@linux.intel.com>

This patchset starts to support DP MST audio.

This patchset is based on drm-intel-nightly on
git://anongit.freedesktop.org/drm-intel

Libin Yang (3):
  ALSA: hda - add DP mst verb support
  ALSA: hda - add DP mst audio support
  ALSA: Documentation about HDA DP MST pin init and connection

 Documentation/sound/alsa/HD-Audio-DP-MST-audio.txt |  17 ++
 sound/pci/hda/hda_codec.c                          |  77 ++++++-
 sound/pci/hda/hda_codec.h                          |   3 +
 sound/pci/hda/patch_hdmi.c                         | 243 ++++++++++++++++-----
 4 files changed, 285 insertions(+), 55 deletions(-)

-- 
1.9.1

[RFC PATCH v3 1/3] ALSA: hda - add DP mst verb support

[libin.yang]

From: Libin Yang <libin.yang@linux.intel.com>

Add snd_hda_get_dev_select() and snd_hda_set_dev_select() functions
for DP MST audio support.

Signed-off-by: Libin Yang <libin.yang@linux.intel.com>
---
 sound/pci/hda/hda_codec.c | 73 ++++++++++++++++++++++++++++++++++++++++++++---
 sound/pci/hda/hda_codec.h |  3 ++
 2 files changed, 72 insertions(+), 4 deletions(-)

diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index 9913be8..caa2c9d 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -311,9 +311,16 @@ int snd_hda_get_conn_index(struct hda_codec *codec, hda_nid_t mux,
 }
 EXPORT_SYMBOL_GPL(snd_hda_get_conn_index);
 
-
-/* return DEVLIST_LEN parameter of the given widget */
-static unsigned int get_num_devices(struct hda_codec *codec, hda_nid_t nid)
+/**
+ * snd_hda_get_num_devices - get DEVLIST_LEN parameter of the given widget
+ *  @codec: the HDA codec
+ *  @nid: NID of the pin to parse
+ *
+ * Get the device entry number on the given widget.
+ * This is a feature of DP MST audio. Each pin can
+ * have several device entries in it.
+ */
+unsigned int snd_hda_get_num_devices(struct hda_codec *codec, hda_nid_t nid)
 {
 	unsigned int wcaps = get_wcaps(codec, nid);
 	unsigned int parm;
@@ -327,6 +334,7 @@ static unsigned int get_num_devices(struct hda_codec *codec, hda_nid_t nid)
 		parm = 0;
 	return parm & AC_DEV_LIST_LEN_MASK;
 }
+EXPORT_SYMBOL_GPL(snd_hda_get_num_devices);
 
 /**
  * snd_hda_get_devices - copy device list without cache
@@ -344,7 +352,7 @@ int snd_hda_get_devices(struct hda_codec *codec, hda_nid_t nid,
 	unsigned int parm;
 	int i, dev_len, devices;
 
-	parm = get_num_devices(codec, nid);
+	parm = snd_hda_get_num_devices(codec, nid);
 	if (!parm)	/* not multi-stream capable */
 		return 0;
 
@@ -368,6 +376,63 @@ int snd_hda_get_devices(struct hda_codec *codec, hda_nid_t nid,
 	return devices;
 }
 
+/**
+ * snd_hda_get_dev_select - get device entry select on the pin
+ * @codec: the HDA codec
+ * @nid: NID of the pin to get device entry select
+ *
+ * Get the devcie entry select on the pin. Return the device entry
+ * id selected on the pin. Return 0 means the first device entry
+ * is selected or MST is not supported.
+ */
+int snd_hda_get_dev_select(struct hda_codec *codec, hda_nid_t nid)
+{
+	/* not support dp_mst will always return 0, using first dev_entry */
+	if (!codec->dp_mst)
+		return 0;
+
+	return snd_hda_codec_read(codec, nid, 0, AC_VERB_GET_DEVICE_SEL, 0);
+}
+EXPORT_SYMBOL_GPL(snd_hda_get_dev_select);
+
+/**
+ * snd_hda_set_dev_select - set device entry select on the pin
+ * @codec: the HDA codec
+ * @nid: NID of the pin to set device entry select
+ * @dev_id: device entry id to be set
+ *
+ * Set the device entry select on the pin nid.
+ */
+int snd_hda_set_dev_select(struct hda_codec *codec, hda_nid_t nid, int dev_id)
+{
+	int ret, num_devices;
+
+	/* not support dp_mst will always return 0, using first dev_entry */
+	if (!codec->dp_mst)
+		return 0;
+
+	/* AC_PAR_DEVLIST_LEN is 0 based. */
+	num_devices = snd_hda_get_num_devices(codec, nid) + 1;
+	/* If Device List Length is 0 (num_device = 1),
+	 * the pin is not multi stream capable.
+	 * Do nothing in this case.
+	 */
+	if (num_devices == 1)
+		return 0;
+
+	/* Behavior of setting index being equal to or greater than
+	 * Device List Length is not predictable
+	 */
+	if (num_devices <= dev_id)
+		return -EINVAL;
+
+	ret = snd_hda_codec_write(codec, nid, 0,
+			AC_VERB_SET_DEVICE_SEL, dev_id);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(snd_hda_set_dev_select);
+
 /*
  * read widget caps for each widget and store in cache
  */
diff --git a/sound/pci/hda/hda_codec.h b/sound/pci/hda/hda_codec.h
index 373fcad..f17f252 100644
--- a/sound/pci/hda/hda_codec.h
+++ b/sound/pci/hda/hda_codec.h
@@ -347,8 +347,11 @@ int snd_hda_override_conn_list(struct hda_codec *codec, hda_nid_t nid, int nums,
 			  const hda_nid_t *list);
 int snd_hda_get_conn_index(struct hda_codec *codec, hda_nid_t mux,
 			   hda_nid_t nid, int recursive);
+unsigned int snd_hda_get_num_devices(struct hda_codec *codec, hda_nid_t nid);
 int snd_hda_get_devices(struct hda_codec *codec, hda_nid_t nid,
 			u8 *dev_list, int max_devices);
+int snd_hda_get_dev_select(struct hda_codec *codec, hda_nid_t nid);
+int snd_hda_set_dev_select(struct hda_codec *codec, hda_nid_t nid, int dev_id);
 
 struct hda_verb {
 	hda_nid_t nid;
-- 
1.9.1

Re: [RFC PATCH v3 1/3] ALSA: hda - add DP mst verb support

[Takashi Iwai]

On Thu, 13 Oct 2016 09:49:35 +0200,
libin.yang@linux.intel.com wrote:
> 
> From: Libin Yang <libin.yang@linux.intel.com>
> 
> Add snd_hda_get_dev_select() and snd_hda_set_dev_select() functions
> for DP MST audio support.
> 
> Signed-off-by: Libin Yang <libin.yang@linux.intel.com>
> ---
>  sound/pci/hda/hda_codec.c | 73 ++++++++++++++++++++++++++++++++++++++++++++---
>  sound/pci/hda/hda_codec.h |  3 ++
>  2 files changed, 72 insertions(+), 4 deletions(-)
> 
> diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> index 9913be8..caa2c9d 100644
> --- a/sound/pci/hda/hda_codec.c
> +++ b/sound/pci/hda/hda_codec.c
> @@ -311,9 +311,16 @@ int snd_hda_get_conn_index(struct hda_codec *codec, hda_nid_t mux,
>  }
>  EXPORT_SYMBOL_GPL(snd_hda_get_conn_index);
>  
> -
> -/* return DEVLIST_LEN parameter of the given widget */
> -static unsigned int get_num_devices(struct hda_codec *codec, hda_nid_t nid)
> +/**
> + * snd_hda_get_num_devices - get DEVLIST_LEN parameter of the given widget
> + *  @codec: the HDA codec
> + *  @nid: NID of the pin to parse
> + *
> + * Get the device entry number on the given widget.
> + * This is a feature of DP MST audio. Each pin can
> + * have several device entries in it.

Try to fill more letters aligned in 80 columns, as we don't need to
write a Haiku here.


Takashi

Re: [RFC PATCH v3 1/3] ALSA: hda - add DP mst verb support

[Yang, Libin]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Thursday, October 20, 2016 11:27 PM
> To: libin.yang@linux.intel.com
> Cc: alsa-devel@alsa-project.org; Yang, Libin <libin.yang@intel.com>; Lin,
> Mengdong <mengdong.lin@intel.com>
> Subject: Re: [alsa-devel] [RFC PATCH v3 1/3] ALSA: hda - add DP mst verb
> support
> 
> On Thu, 13 Oct 2016 09:49:35 +0200,
> libin.yang@linux.intel.com wrote:
> >
> > From: Libin Yang <libin.yang@linux.intel.com>
> >
> > Add snd_hda_get_dev_select() and snd_hda_set_dev_select() functions
> > for DP MST audio support.
> >
> > Signed-off-by: Libin Yang <libin.yang@linux.intel.com>
> > ---
> >  sound/pci/hda/hda_codec.c | 73
> > ++++++++++++++++++++++++++++++++++++++++++++---
> >  sound/pci/hda/hda_codec.h |  3 ++
> >  2 files changed, 72 insertions(+), 4 deletions(-)
> >
> > diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> > index 9913be8..caa2c9d 100644
> > --- a/sound/pci/hda/hda_codec.c
> > +++ b/sound/pci/hda/hda_codec.c
> > @@ -311,9 +311,16 @@ int snd_hda_get_conn_index(struct hda_codec
> > *codec, hda_nid_t mux,  }
> EXPORT_SYMBOL_GPL(snd_hda_get_conn_index);
> >
> > -
> > -/* return DEVLIST_LEN parameter of the given widget */ -static
> > unsigned int get_num_devices(struct hda_codec *codec, hda_nid_t nid)
> > +/**
> > + * snd_hda_get_num_devices - get DEVLIST_LEN parameter of the given
> > +widget
> > + *  @codec: the HDA codec
> > + *  @nid: NID of the pin to parse
> > + *
> > + * Get the device entry number on the given widget.
> > + * This is a feature of DP MST audio. Each pin can
> > + * have several device entries in it.
> 
> Try to fill more letters aligned in 80 columns, as we don't need to write a
> Haiku here.

Get it. Thanks :)

Regards,
Libin

> 
> 
> Takashi

[RFC PATCH v3 2/3] ALSA: hda - add DP mst audio support

[libin.yang]

From: Libin Yang <libin.yang@linux.intel.com>

This patch adds the DP mst audio support.

Signed-off-by: Libin Yang <libin.yang@linux.intel.com>
---
 sound/pci/hda/hda_codec.c  |   4 +
 sound/pci/hda/patch_hdmi.c | 243 +++++++++++++++++++++++++++++++++++----------
 2 files changed, 196 insertions(+), 51 deletions(-)

diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index caa2c9d..8a6b0a2 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -468,6 +468,10 @@ static int read_pin_defaults(struct hda_codec *codec)
 		pin->nid = nid;
 		pin->cfg = snd_hda_codec_read(codec, nid, 0,
 					      AC_VERB_GET_CONFIG_DEFAULT, 0);
+		/*
+		 * all device entries are the same widget control so far
+		 * fixme: if any codec is different, need fix here
+		 */
 		pin->ctrl = snd_hda_codec_read(codec, nid, 0,
 					       AC_VERB_GET_PIN_WIDGET_CONTROL,
 					       0);
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index cf9bc042..422e05e 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -76,6 +76,7 @@ struct hdmi_spec_per_cvt {
 
 struct hdmi_spec_per_pin {
 	hda_nid_t pin_nid;
+	int dev_id;
 	/* pin idx, different device entries on the same pin use the same idx */
 	int pin_nid_idx;
 	int num_mux_nids;
@@ -130,7 +131,23 @@ struct hdmi_spec {
 	struct snd_array cvts; /* struct hdmi_spec_per_cvt */
 	hda_nid_t cvt_nids[4]; /* only for haswell fix */
 
+	/*
+	 * num_pins is the number of virtual pins
+	 * for example, there are 3 pins, and each pin
+	 * has 4 device entries, then the num_pins is 12
+	 */
 	int num_pins;
+	/*
+	 * num_nids is the number of real pins
+	 * In the above example, num_nids is 3
+	 */
+	int num_nids;
+	/*
+	 * dev_num is the number of device entries
+	 * on each pin.
+	 * In the above example, dev_num is 4
+	 */
+	int dev_num;
 	struct snd_array pins; /* struct hdmi_spec_per_pin */
 	struct hdmi_pcm pcm_rec[16];
 	struct mutex pcm_lock;
@@ -217,14 +234,26 @@ union audio_infoframe {
 /* obtain hda_pcm object assigned to idx */
 #define get_pcm_rec(spec, idx)	(get_hdmi_pcm(spec, idx)->pcm)
 
-static int pin_nid_to_pin_index(struct hda_codec *codec, hda_nid_t pin_nid)
+static int pin_id_to_pin_index(struct hda_codec *codec,
+			       hda_nid_t pin_nid, int dev_id)
 {
 	struct hdmi_spec *spec = codec->spec;
 	int pin_idx;
+	struct hdmi_spec_per_pin *per_pin;
+
+	/*
+	 * (dev_id == -1) means it is NON-MST pin
+	 * return the first virtual pin on this port
+	 */
+	if (dev_id == -1)
+		dev_id = 0;
 
-	for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++)
-		if (get_pin(spec, pin_idx)->pin_nid == pin_nid)
+	for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) {
+		per_pin = get_pin(spec, pin_idx);
+		if ((per_pin->pin_nid == pin_nid) &&
+			(per_pin->dev_id == dev_id))
 			return pin_idx;
+	}
 
 	codec_warn(codec, "HDMI: pin nid %d not registered\n", pin_nid);
 	return -EINVAL;
@@ -724,10 +753,11 @@ static void hdmi_setup_audio_infoframe(struct hda_codec *codec,
 
 static bool hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll);
 
-static void check_presence_and_report(struct hda_codec *codec, hda_nid_t nid)
+static void check_presence_and_report(struct hda_codec *codec, hda_nid_t nid,
+				      int dev_id)
 {
 	struct hdmi_spec *spec = codec->spec;
-	int pin_idx = pin_nid_to_pin_index(codec, nid);
+	int pin_idx = pin_id_to_pin_index(codec, nid, dev_id);
 
 	if (pin_idx < 0)
 		return;
@@ -738,7 +768,8 @@ static void check_presence_and_report(struct hda_codec *codec, hda_nid_t nid)
 static void jack_callback(struct hda_codec *codec,
 			  struct hda_jack_callback *jack)
 {
-	check_presence_and_report(codec, jack->nid);
+	/* hda_jack don't support DP MST */
+	check_presence_and_report(codec, jack->nid, 0);
 }
 
 static void hdmi_intrinsic_event(struct hda_codec *codec, unsigned int res)
@@ -747,6 +778,12 @@ static void hdmi_intrinsic_event(struct hda_codec *codec, unsigned int res)
 	struct hda_jack_tbl *jack;
 	int dev_entry = (res & AC_UNSOL_RES_DE) >> AC_UNSOL_RES_DE_SHIFT;
 
+	/*
+	 * assume DP MST uses dyn_pcm_assign and acomp and
+	 * never comes here
+	 * if DP MST supports unsol event, below code need
+	 * consider dev_entry
+	 */
 	jack = snd_hda_jack_tbl_get_from_tag(codec, tag);
 	if (!jack)
 		return;
@@ -757,7 +794,8 @@ static void hdmi_intrinsic_event(struct hda_codec *codec, unsigned int res)
 		codec->addr, jack->nid, dev_entry, !!(res & AC_UNSOL_RES_IA),
 		!!(res & AC_UNSOL_RES_PD), !!(res & AC_UNSOL_RES_ELDV));
 
-	check_presence_and_report(codec, jack->nid);
+	/* hda_jack don't support DP MST */
+	check_presence_and_report(codec, jack->nid, 0);
 }
 
 static void hdmi_non_intrinsic_event(struct hda_codec *codec, unsigned int res)
@@ -970,28 +1008,60 @@ static int intel_cvt_id_to_mux_idx(struct hdmi_spec *spec,
  * by any other pins.
  */
 static void intel_not_share_assigned_cvt(struct hda_codec *codec,
-			hda_nid_t pin_nid, int mux_idx)
+					 hda_nid_t pin_nid,
+					 int dev_id, int mux_idx)
 {
 	struct hdmi_spec *spec = codec->spec;
 	hda_nid_t nid;
 	int cvt_idx, curr;
 	struct hdmi_spec_per_cvt *per_cvt;
+	struct hdmi_spec_per_pin *per_pin;
+	int pin_idx;
 
 	/* configure all pins, including "no physical connection" ones */
-	for_each_hda_codec_node(nid, codec) {
-		unsigned int wid_caps = get_wcaps(codec, nid);
-		unsigned int wid_type = get_wcaps_type(wid_caps);
+	for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) {
+		int dev_id_saved;
+		int dev_num;
+
+		per_pin = get_pin(spec, pin_idx);
+		/*
+		 * pin not connected to monitor
+		 * no need to operate on it
+		 */
+		if (!per_pin->pcm)
+			continue;
 
-		if (wid_type != AC_WID_PIN)
+		if ((per_pin->pin_nid == pin_nid) &&
+			(per_pin->dev_id == dev_id))
 			continue;
 
-		if (nid == pin_nid)
+		/*
+		 * if per_pin->dev_id >= dev_num,
+		 * snd_hda_get_dev_select() will fail,
+		 * and the following operation is unpredictable.
+		 * So skip this situation.
+		 */
+		dev_num = snd_hda_get_num_devices(codec, per_pin->pin_nid) + 1;
+		if (per_pin->dev_id >= dev_num)
 			continue;
 
+		nid = per_pin->pin_nid;
+
+		/*
+		 * Calling this function should not impact
+		 * on the device entry selection
+		 * So let's save the dev id for each pin,
+		 * and restore it when return
+		 */
+		dev_id_saved = snd_hda_get_dev_select(codec, nid);
+		snd_hda_set_dev_select(codec, nid, per_pin->dev_id);
 		curr = snd_hda_codec_read(codec, nid, 0,
 					  AC_VERB_GET_CONNECT_SEL, 0);
-		if (curr != mux_idx)
+		if (curr != mux_idx) {
+			snd_hda_set_dev_select(codec, nid, dev_id_saved);
 			continue;
+		}
+
 
 		/* choose an unassigned converter. The conveters in the
 		 * connection list are in the same order as in the codec.
@@ -1008,12 +1078,13 @@ static void intel_not_share_assigned_cvt(struct hda_codec *codec,
 				break;
 			}
 		}
+		snd_hda_set_dev_select(codec, nid, dev_id_saved);
 	}
 }
 
 /* A wrapper of intel_not_share_asigned_cvt() */
 static void intel_not_share_assigned_cvt_nid(struct hda_codec *codec,
-			hda_nid_t pin_nid, hda_nid_t cvt_nid)
+			hda_nid_t pin_nid, int dev_id, hda_nid_t cvt_nid)
 {
 	int mux_idx;
 	struct hdmi_spec *spec = codec->spec;
@@ -1025,7 +1096,7 @@ static void intel_not_share_assigned_cvt_nid(struct hda_codec *codec,
 	 */
 	mux_idx = intel_cvt_id_to_mux_idx(spec, cvt_nid);
 	if (mux_idx >= 0)
-		intel_not_share_assigned_cvt(codec, pin_nid, mux_idx);
+		intel_not_share_assigned_cvt(codec, pin_nid, dev_id, mux_idx);
 }
 
 /* skeleton caller of pin_cvt_fixup ops */
@@ -1140,6 +1211,7 @@ static int hdmi_pcm_open(struct hda_pcm_stream *hinfo,
 	per_pin->cvt_nid = per_cvt->cvt_nid;
 	hinfo->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);
@@ -1198,6 +1270,7 @@ static int hdmi_read_pin_conn(struct hda_codec *codec, int pin_idx)
 		return -EINVAL;
 	}
 
+	/* all the device entries on the same pin have the same conn list */
 	per_pin->num_mux_nids = snd_hda_get_connections(codec, pin_nid,
 							per_pin->mux_nids,
 							HDA_MAX_CONNECTIONS);
@@ -1215,13 +1288,13 @@ static int hdmi_find_pcm_slot(struct hdmi_spec *spec,
 		return per_pin->pin_nid_idx;
 
 	/* have a second try; check the "reserved area" over num_pins */
-	for (i = spec->num_pins; i < spec->pcm_used; i++) {
+	for (i = spec->num_nids; i < spec->pcm_used; i++) {
 		if (!test_bit(i, &spec->pcm_bitmap))
 			return i;
 	}
 
 	/* the last try; check the empty slots in pins */
-	for (i = 0; i < spec->num_pins; i++) {
+	for (i = 0; i < spec->num_nids; i++) {
 		if (!test_bit(i, &spec->pcm_bitmap))
 			return i;
 	}
@@ -1296,10 +1369,13 @@ static void hdmi_pcm_setup_pin(struct hdmi_spec *spec,
 	per_pin->cvt_nid = hinfo->nid;
 
 	mux_idx = hdmi_get_pin_cvt_mux(spec, per_pin, hinfo->nid);
-	if (mux_idx < per_pin->num_mux_nids)
+	if (mux_idx < per_pin->num_mux_nids) {
+		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,
 				mux_idx);
+	}
 	snd_hda_spdif_ctls_assign(codec, per_pin->pcm_idx, hinfo->nid);
 
 	non_pcm = check_non_pcm_per_cvt(codec, hinfo->nid);
@@ -1467,6 +1543,11 @@ static struct snd_jack *pin_idx_to_jack(struct hda_codec *codec,
 	if (per_pin->pcm_idx >= 0 && spec->dyn_pcm_assign)
 		jack = spec->pcm_rec[per_pin->pcm_idx].jack;
 	else if (!spec->dyn_pcm_assign) {
+		/*
+		 * jack tbl doesn't support DP MST
+		 * DP MST will use dyn_pcm_assign,
+		 * so DP MST will never come here
+		 */
 		jack_tbl = snd_hda_jack_tbl_get(codec, per_pin->pin_nid);
 		if (jack_tbl)
 			jack = jack_tbl->jack;
@@ -1485,9 +1566,9 @@ static void sync_eld_via_acomp(struct hda_codec *codec,
 
 	mutex_lock(&per_pin->lock);
 	eld->monitor_present = false;
-	size = snd_hdac_acomp_get_eld(&codec->core, per_pin->pin_nid, -1,
-				      &eld->monitor_present, eld->eld_buffer,
-				      ELD_MAX_SIZE);
+	size = snd_hdac_acomp_get_eld(&codec->core, per_pin->pin_nid,
+				      per_pin->dev_id, &eld->monitor_present,
+				      eld->eld_buffer, ELD_MAX_SIZE);
 	if (size > 0) {
 		size = min(size, ELD_MAX_SIZE);
 		if (snd_hdmi_parse_eld(codec, &eld->info,
@@ -1565,38 +1646,81 @@ static int hdmi_add_pin(struct hda_codec *codec, hda_nid_t pin_nid)
 	int pin_idx;
 	struct hdmi_spec_per_pin *per_pin;
 	int err;
+	int dev_num, i;
 
 	caps = snd_hda_query_pin_caps(codec, pin_nid);
 	if (!(caps & (AC_PINCAP_HDMI | AC_PINCAP_DP)))
 		return 0;
 
+	/*
+	 * For DP MST audio, Configuration Default is the same for
+	 * all device entries on the same pin
+	 */
 	config = snd_hda_codec_get_pincfg(codec, pin_nid);
 	if (get_defcfg_connect(config) == AC_JACK_PORT_NONE)
 		return 0;
 
-	if (is_haswell_plus(codec))
-		intel_haswell_fixup_connect_list(codec, pin_nid);
-
-	pin_idx = spec->num_pins;
-	per_pin = snd_array_new(&spec->pins);
-	if (!per_pin)
-		return -ENOMEM;
-
-	per_pin->pin_nid = pin_nid;
-	per_pin->non_pcm = false;
-	if (spec->dyn_pcm_assign)
-		per_pin->pcm_idx = -1;
-	else {
-		per_pin->pcm = get_hdmi_pcm(spec, pin_idx);
-		per_pin->pcm_idx = pin_idx;
+	/*
+	 * To simplify the implementation, malloc all
+	 * the virtual pins in the initialization statically
+	 */
+	if (is_haswell_plus(codec)) {
+		/*
+		 * On Intel platforms, device entries number is
+		 * changed dynamically. If there is a DP MST
+		 * hub connected, the device entries number is 3.
+		 * Otherwise, it is 1.
+		 * Here we manually set dev_num to 3, so that
+		 * we can initialize all the device entries when
+		 * bootup statically.
+		 */
+		dev_num = 3;
+		spec->dev_num = 3;
+	} else if (spec->dyn_pcm_assign && codec->dp_mst) {
+		dev_num = snd_hda_get_num_devices(codec, pin_nid) + 1;
+		/*
+		 * spec->dev_num is the maxinum number of device entries
+		 * among all the pins
+		 */
+		spec->dev_num = (spec->dev_num > dev_num) ?
+			spec->dev_num : dev_num;
+	} else {
+		/*
+		 * If the platform doesn't support DP MST,
+		 * manually set dev_num to 1. This means
+		 * the pin has only one device entry.
+		 */
+		dev_num = 1;
+		spec->dev_num = 1;
 	}
-	per_pin->pin_nid_idx = pin_idx;
 
-	err = hdmi_read_pin_conn(codec, pin_idx);
-	if (err < 0)
-		return err;
+	for (i = 0; i < dev_num; i++) {
+		pin_idx = spec->num_pins;
+		per_pin = snd_array_new(&spec->pins);
 
-	spec->num_pins++;
+		if (!per_pin)
+			return -ENOMEM;
+
+		if (spec->dyn_pcm_assign) {
+			per_pin->pcm = NULL;
+			per_pin->pcm_idx = -1;
+		} else {
+			per_pin->pcm = get_hdmi_pcm(spec, pin_idx);
+			per_pin->pcm_idx = pin_idx;
+		}
+		per_pin->pin_nid = pin_nid;
+		per_pin->pin_nid_idx = spec->num_nids;
+		per_pin->dev_id = i;
+		per_pin->non_pcm = false;
+		snd_hda_set_dev_select(codec, pin_nid, i);
+		if (is_haswell_plus(codec))
+			intel_haswell_fixup_connect_list(codec, pin_nid);
+		err = hdmi_read_pin_conn(codec, pin_idx);
+		if (err < 0)
+			return err;
+		spec->num_pins++;
+	}
+	spec->num_nids++;
 
 	return 0;
 }
@@ -1744,7 +1868,7 @@ static int generic_hdmi_playback_pcm_prepare(struct hda_pcm_stream *hinfo,
 	/* Call sync_audio_rate to set the N/CTS/M manually if necessary */
 	/* Todo: add DP1.2 MST audio support later */
 	if (codec_has_acomp(codec))
-		snd_hdac_sync_audio_rate(&codec->core, pin_nid, -1,
+		snd_hdac_sync_audio_rate(&codec->core, pin_nid, per_pin->dev_id,
 					 runtime->rate);
 
 	non_pcm = check_non_pcm_per_cvt(codec, cvt_nid);
@@ -1762,6 +1886,7 @@ static int generic_hdmi_playback_pcm_prepare(struct hda_pcm_stream *hinfo,
 				    pinctl | PIN_OUT);
 	}
 
+	/* snd_hda_set_dev_select() has been called before */
 	err = spec->ops.setup_stream(codec, cvt_nid, pin_nid,
 				 stream_tag, format);
 	mutex_unlock(&spec->pcm_lock);
@@ -1897,17 +2022,23 @@ static bool is_hdmi_pcm_attached(struct hdac_device *hdac, int pcm_idx)
 static int generic_hdmi_build_pcms(struct hda_codec *codec)
 {
 	struct hdmi_spec *spec = codec->spec;
-	int pin_idx;
+	int idx;
 
-	for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) {
+	/*
+	 * for non-mst mode, pcm number is the same as before
+	 * for DP MST mode, pcm number is (nid number + dev_num - 1)
+	 *  dev_num is the device entry number in a pin
+	 *
+	 */
+	for (idx = 0; idx < spec->num_nids + spec->dev_num - 1; idx++) {
 		struct hda_pcm *info;
 		struct hda_pcm_stream *pstr;
 
-		info = snd_hda_codec_pcm_new(codec, "HDMI %d", pin_idx);
+		info = snd_hda_codec_pcm_new(codec, "HDMI %d", idx);
 		if (!info)
 			return -ENOMEM;
 
-		spec->pcm_rec[pin_idx].pcm = info;
+		spec->pcm_rec[idx].pcm = info;
 		spec->pcm_used++;
 		info->pcm_type = HDA_PCM_TYPE_HDMI;
 		info->own_chmap = true;
@@ -1915,6 +2046,9 @@ static int generic_hdmi_build_pcms(struct hda_codec *codec)
 		pstr = &info->stream[SNDRV_PCM_STREAM_PLAYBACK];
 		pstr->substreams = 1;
 		pstr->ops = generic_ops;
+		/* pcm number is less than 16 */
+		if (spec->pcm_used >= 16)
+			break;
 		/* other pstr fields are set in open */
 	}
 
@@ -2070,7 +2204,9 @@ static int generic_hdmi_init(struct hda_codec *codec)
 	for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) {
 		struct hdmi_spec_per_pin *per_pin = get_pin(spec, pin_idx);
 		hda_nid_t pin_nid = per_pin->pin_nid;
+		int dev_id = per_pin->dev_id;
 
+		snd_hda_set_dev_select(codec, pin_nid, dev_id);
 		hdmi_init_pin(codec, pin_nid);
 		if (!codec_has_acomp(codec))
 			snd_hda_jack_detect_enable_callback(codec, pin_nid,
@@ -2178,6 +2314,7 @@ static int alloc_generic_hdmi(struct hda_codec *codec)
 		return -ENOMEM;
 
 	spec->ops = generic_standard_hdmi_ops;
+	spec->dev_num = 1;	/* initialize to 1 */
 	mutex_init(&spec->pcm_lock);
 	snd_hdac_register_chmap_ops(&codec->core, &spec->chmap);
 
@@ -2295,6 +2432,7 @@ static void intel_pin_eld_notify(void *audio_ptr, int port, int pipe)
 {
 	struct hda_codec *codec = audio_ptr;
 	int pin_nid;
+	int dev_id = pipe;
 
 	/* we assume only from port-B to port-D */
 	if (port < 1 || port > 3)
@@ -2321,7 +2459,7 @@ static void intel_pin_eld_notify(void *audio_ptr, int port, int pipe)
 		return;
 
 	snd_hdac_i915_set_bclk(&codec->bus->core);
-	check_presence_and_report(codec, pin_nid);
+	check_presence_and_report(codec, pin_nid, dev_id);
 }
 
 /* register i915 component pin_eld_notify callback */
@@ -2354,11 +2492,13 @@ static void i915_pin_cvt_fixup(struct hda_codec *codec,
 			       hda_nid_t cvt_nid)
 {
 	if (per_pin) {
+		snd_hda_set_dev_select(codec, per_pin->pin_nid,
+			       per_pin->dev_id);
 		intel_verify_pin_cvt_connect(codec, per_pin);
 		intel_not_share_assigned_cvt(codec, per_pin->pin_nid,
-					     per_pin->mux_idx);
+				     per_pin->dev_id, per_pin->mux_idx);
 	} else {
-		intel_not_share_assigned_cvt_nid(codec, 0, cvt_nid);
+		intel_not_share_assigned_cvt_nid(codec, 0, 0, cvt_nid);
 	}
 }
 
@@ -2378,6 +2518,8 @@ static int patch_i915_hsw_hdmi(struct hda_codec *codec)
 	if (err < 0)
 		return err;
 	spec = codec->spec;
+	codec->dp_mst = true;
+	spec->dyn_pcm_assign = true;
 
 	intel_haswell_enable_all_pins(codec, true);
 	intel_haswell_fixup_enable_dp12(codec);
@@ -2389,7 +2531,6 @@ static int patch_i915_hsw_hdmi(struct hda_codec *codec)
 		codec->core.link_power_control = 1;
 
 	codec->patch_ops.set_power_state = haswell_set_power_state;
-	codec->dp_mst = true;
 	codec->depop_delay = 0;
 	codec->auto_runtime_pm = 1;
 
-- 
1.9.1

Re: [RFC PATCH v3 2/3] ALSA: hda - add DP mst audio support

[Takashi Iwai]

On Thu, 13 Oct 2016 09:49:36 +0200,
libin.yang@linux.intel.com wrote:
> 
> From: Libin Yang <libin.yang@linux.intel.com>
> 
> This patch adds the DP mst audio support.

A bit more useful texts here please.  I know you'll give more
explanations in the document in the later patch, but still something
better should be mentioned here.

> 
> Signed-off-by: Libin Yang <libin.yang@linux.intel.com>
> ---
>  sound/pci/hda/hda_codec.c  |   4 +
>  sound/pci/hda/patch_hdmi.c | 243 +++++++++++++++++++++++++++++++++++----------
>  2 files changed, 196 insertions(+), 51 deletions(-)
> 
> diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> index caa2c9d..8a6b0a2 100644
> --- a/sound/pci/hda/hda_codec.c
> +++ b/sound/pci/hda/hda_codec.c
> @@ -468,6 +468,10 @@ static int read_pin_defaults(struct hda_codec *codec)
>  		pin->nid = nid;
>  		pin->cfg = snd_hda_codec_read(codec, nid, 0,
>  					      AC_VERB_GET_CONFIG_DEFAULT, 0);
> +		/*
> +		 * all device entries are the same widget control so far
> +		 * fixme: if any codec is different, need fix here
> +		 */
>  		pin->ctrl = snd_hda_codec_read(codec, nid, 0,
>  					       AC_VERB_GET_PIN_WIDGET_CONTROL,
>  					       0);
> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> index cf9bc042..422e05e 100644
> --- a/sound/pci/hda/patch_hdmi.c
> +++ b/sound/pci/hda/patch_hdmi.c
> @@ -76,6 +76,7 @@ struct hdmi_spec_per_cvt {
>  
>  struct hdmi_spec_per_pin {
>  	hda_nid_t pin_nid;
> +	int dev_id;
>  	/* pin idx, different device entries on the same pin use the same idx */
>  	int pin_nid_idx;
>  	int num_mux_nids;
> @@ -130,7 +131,23 @@ struct hdmi_spec {
>  	struct snd_array cvts; /* struct hdmi_spec_per_cvt */
>  	hda_nid_t cvt_nids[4]; /* only for haswell fix */
>  
> +	/*
> +	 * num_pins is the number of virtual pins
> +	 * for example, there are 3 pins, and each pin
> +	 * has 4 device entries, then the num_pins is 12
> +	 */
>  	int num_pins;
> +	/*
> +	 * num_nids is the number of real pins
> +	 * In the above example, num_nids is 3
> +	 */
> +	int num_nids;
> +	/*
> +	 * dev_num is the number of device entries
> +	 * on each pin.
> +	 * In the above example, dev_num is 4
> +	 */
> +	int dev_num;
>  	struct snd_array pins; /* struct hdmi_spec_per_pin */
>  	struct hdmi_pcm pcm_rec[16];
>  	struct mutex pcm_lock;
> @@ -217,14 +234,26 @@ union audio_infoframe {
>  /* obtain hda_pcm object assigned to idx */
>  #define get_pcm_rec(spec, idx)	(get_hdmi_pcm(spec, idx)->pcm)
>  
> -static int pin_nid_to_pin_index(struct hda_codec *codec, hda_nid_t pin_nid)
> +static int pin_id_to_pin_index(struct hda_codec *codec,
> +			       hda_nid_t pin_nid, int dev_id)
>  {
>  	struct hdmi_spec *spec = codec->spec;
>  	int pin_idx;
> +	struct hdmi_spec_per_pin *per_pin;
> +
> +	/*
> +	 * (dev_id == -1) means it is NON-MST pin
> +	 * return the first virtual pin on this port
> +	 */
> +	if (dev_id == -1)
> +		dev_id = 0;
>  
> -	for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++)
> -		if (get_pin(spec, pin_idx)->pin_nid == pin_nid)
> +	for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) {
> +		per_pin = get_pin(spec, pin_idx);
> +		if ((per_pin->pin_nid == pin_nid) &&
> +			(per_pin->dev_id == dev_id))
>  			return pin_idx;
> +	}
>  
>  	codec_warn(codec, "HDMI: pin nid %d not registered\n", pin_nid);
>  	return -EINVAL;
> @@ -724,10 +753,11 @@ static void hdmi_setup_audio_infoframe(struct hda_codec *codec,
>  
>  static bool hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll);
>  
> -static void check_presence_and_report(struct hda_codec *codec, hda_nid_t nid)
> +static void check_presence_and_report(struct hda_codec *codec, hda_nid_t nid,
> +				      int dev_id)
>  {
>  	struct hdmi_spec *spec = codec->spec;
> -	int pin_idx = pin_nid_to_pin_index(codec, nid);
> +	int pin_idx = pin_id_to_pin_index(codec, nid, dev_id);
>  
>  	if (pin_idx < 0)
>  		return;
> @@ -738,7 +768,8 @@ static void check_presence_and_report(struct hda_codec *codec, hda_nid_t nid)
>  static void jack_callback(struct hda_codec *codec,
>  			  struct hda_jack_callback *jack)
>  {
> -	check_presence_and_report(codec, jack->nid);
> +	/* hda_jack don't support DP MST */
> +	check_presence_and_report(codec, jack->nid, 0);
>  }
>  
>  static void hdmi_intrinsic_event(struct hda_codec *codec, unsigned int res)
> @@ -747,6 +778,12 @@ static void hdmi_intrinsic_event(struct hda_codec *codec, unsigned int res)
>  	struct hda_jack_tbl *jack;
>  	int dev_entry = (res & AC_UNSOL_RES_DE) >> AC_UNSOL_RES_DE_SHIFT;
>  
> +	/*
> +	 * assume DP MST uses dyn_pcm_assign and acomp and
> +	 * never comes here
> +	 * if DP MST supports unsol event, below code need
> +	 * consider dev_entry
> +	 */
>  	jack = snd_hda_jack_tbl_get_from_tag(codec, tag);
>  	if (!jack)
>  		return;
> @@ -757,7 +794,8 @@ static void hdmi_intrinsic_event(struct hda_codec *codec, unsigned int res)
>  		codec->addr, jack->nid, dev_entry, !!(res & AC_UNSOL_RES_IA),
>  		!!(res & AC_UNSOL_RES_PD), !!(res & AC_UNSOL_RES_ELDV));
>  
> -	check_presence_and_report(codec, jack->nid);
> +	/* hda_jack don't support DP MST */
> +	check_presence_and_report(codec, jack->nid, 0);
>  }
>  
>  static void hdmi_non_intrinsic_event(struct hda_codec *codec, unsigned int res)
> @@ -970,28 +1008,60 @@ static int intel_cvt_id_to_mux_idx(struct hdmi_spec *spec,
>   * by any other pins.
>   */
>  static void intel_not_share_assigned_cvt(struct hda_codec *codec,
> -			hda_nid_t pin_nid, int mux_idx)
> +					 hda_nid_t pin_nid,
> +					 int dev_id, int mux_idx)
>  {
>  	struct hdmi_spec *spec = codec->spec;
>  	hda_nid_t nid;
>  	int cvt_idx, curr;
>  	struct hdmi_spec_per_cvt *per_cvt;
> +	struct hdmi_spec_per_pin *per_pin;
> +	int pin_idx;
>  
>  	/* configure all pins, including "no physical connection" ones */
> -	for_each_hda_codec_node(nid, codec) {
> -		unsigned int wid_caps = get_wcaps(codec, nid);
> -		unsigned int wid_type = get_wcaps_type(wid_caps);
> +	for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) {
> +		int dev_id_saved;
> +		int dev_num;
> +
> +		per_pin = get_pin(spec, pin_idx);
> +		/*
> +		 * pin not connected to monitor
> +		 * no need to operate on it
> +		 */
> +		if (!per_pin->pcm)
> +			continue;
>  
> -		if (wid_type != AC_WID_PIN)
> +		if ((per_pin->pin_nid == pin_nid) &&
> +			(per_pin->dev_id == dev_id))
>  			continue;
>  
> -		if (nid == pin_nid)
> +		/*
> +		 * if per_pin->dev_id >= dev_num,
> +		 * snd_hda_get_dev_select() will fail,
> +		 * and the following operation is unpredictable.
> +		 * So skip this situation.
> +		 */
> +		dev_num = snd_hda_get_num_devices(codec, per_pin->pin_nid) + 1;
> +		if (per_pin->dev_id >= dev_num)
>  			continue;
>  
> +		nid = per_pin->pin_nid;
> +
> +		/*
> +		 * Calling this function should not impact
> +		 * on the device entry selection
> +		 * So let's save the dev id for each pin,
> +		 * and restore it when return
> +		 */
> +		dev_id_saved = snd_hda_get_dev_select(codec, nid);
> +		snd_hda_set_dev_select(codec, nid, per_pin->dev_id);
>  		curr = snd_hda_codec_read(codec, nid, 0,
>  					  AC_VERB_GET_CONNECT_SEL, 0);
> -		if (curr != mux_idx)
> +		if (curr != mux_idx) {
> +			snd_hda_set_dev_select(codec, nid, dev_id_saved);
>  			continue;
> +		}
> +
>  
>  		/* choose an unassigned converter. The conveters in the
>  		 * connection list are in the same order as in the codec.
> @@ -1008,12 +1078,13 @@ static void intel_not_share_assigned_cvt(struct hda_codec *codec,
>  				break;
>  			}
>  		}
> +		snd_hda_set_dev_select(codec, nid, dev_id_saved);

I guess this revert won't be reached when you break or continue the
loop beforehand?

Also, this rewrite changes the loop completely, so we you should
update the comment appropriately.  It's no longer scanning the all
pins (including unused ones).  And I'm not 100% sure whether it's OK
to change in that way; the function was written to do that (touching
the unused pin as well) on purpose, IIRC.

Hm, or is it OK because we are listing always the three pins?
More explanation please, either in comments or in the changelog.


Takashi

>  	}
>  }
>  
>  /* A wrapper of intel_not_share_asigned_cvt() */
>  static void intel_not_share_assigned_cvt_nid(struct hda_codec *codec,
> -			hda_nid_t pin_nid, hda_nid_t cvt_nid)
> +			hda_nid_t pin_nid, int dev_id, hda_nid_t cvt_nid)
>  {
>  	int mux_idx;
>  	struct hdmi_spec *spec = codec->spec;
> @@ -1025,7 +1096,7 @@ static void intel_not_share_assigned_cvt_nid(struct hda_codec *codec,
>  	 */
>  	mux_idx = intel_cvt_id_to_mux_idx(spec, cvt_nid);
>  	if (mux_idx >= 0)
> -		intel_not_share_assigned_cvt(codec, pin_nid, mux_idx);
> +		intel_not_share_assigned_cvt(codec, pin_nid, dev_id, mux_idx);
>  }
>  
>  /* skeleton caller of pin_cvt_fixup ops */
> @@ -1140,6 +1211,7 @@ static int hdmi_pcm_open(struct hda_pcm_stream *hinfo,
>  	per_pin->cvt_nid = per_cvt->cvt_nid;
>  	hinfo->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);
> @@ -1198,6 +1270,7 @@ static int hdmi_read_pin_conn(struct hda_codec *codec, int pin_idx)
>  		return -EINVAL;
>  	}
>  
> +	/* all the device entries on the same pin have the same conn list */
>  	per_pin->num_mux_nids = snd_hda_get_connections(codec, pin_nid,
>  							per_pin->mux_nids,
>  							HDA_MAX_CONNECTIONS);
> @@ -1215,13 +1288,13 @@ static int hdmi_find_pcm_slot(struct hdmi_spec *spec,
>  		return per_pin->pin_nid_idx;
>  
>  	/* have a second try; check the "reserved area" over num_pins */
> -	for (i = spec->num_pins; i < spec->pcm_used; i++) {
> +	for (i = spec->num_nids; i < spec->pcm_used; i++) {
>  		if (!test_bit(i, &spec->pcm_bitmap))
>  			return i;
>  	}
>  
>  	/* the last try; check the empty slots in pins */
> -	for (i = 0; i < spec->num_pins; i++) {
> +	for (i = 0; i < spec->num_nids; i++) {
>  		if (!test_bit(i, &spec->pcm_bitmap))
>  			return i;
>  	}
> @@ -1296,10 +1369,13 @@ static void hdmi_pcm_setup_pin(struct hdmi_spec *spec,
>  	per_pin->cvt_nid = hinfo->nid;
>  
>  	mux_idx = hdmi_get_pin_cvt_mux(spec, per_pin, hinfo->nid);
> -	if (mux_idx < per_pin->num_mux_nids)
> +	if (mux_idx < per_pin->num_mux_nids) {
> +		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,
>  				mux_idx);
> +	}
>  	snd_hda_spdif_ctls_assign(codec, per_pin->pcm_idx, hinfo->nid);
>  
>  	non_pcm = check_non_pcm_per_cvt(codec, hinfo->nid);
> @@ -1467,6 +1543,11 @@ static struct snd_jack *pin_idx_to_jack(struct hda_codec *codec,
>  	if (per_pin->pcm_idx >= 0 && spec->dyn_pcm_assign)
>  		jack = spec->pcm_rec[per_pin->pcm_idx].jack;
>  	else if (!spec->dyn_pcm_assign) {
> +		/*
> +		 * jack tbl doesn't support DP MST
> +		 * DP MST will use dyn_pcm_assign,
> +		 * so DP MST will never come here
> +		 */
>  		jack_tbl = snd_hda_jack_tbl_get(codec, per_pin->pin_nid);
>  		if (jack_tbl)
>  			jack = jack_tbl->jack;
> @@ -1485,9 +1566,9 @@ static void sync_eld_via_acomp(struct hda_codec *codec,
>  
>  	mutex_lock(&per_pin->lock);
>  	eld->monitor_present = false;
> -	size = snd_hdac_acomp_get_eld(&codec->core, per_pin->pin_nid, -1,
> -				      &eld->monitor_present, eld->eld_buffer,
> -				      ELD_MAX_SIZE);
> +	size = snd_hdac_acomp_get_eld(&codec->core, per_pin->pin_nid,
> +				      per_pin->dev_id, &eld->monitor_present,
> +				      eld->eld_buffer, ELD_MAX_SIZE);
>  	if (size > 0) {
>  		size = min(size, ELD_MAX_SIZE);
>  		if (snd_hdmi_parse_eld(codec, &eld->info,
> @@ -1565,38 +1646,81 @@ static int hdmi_add_pin(struct hda_codec *codec, hda_nid_t pin_nid)
>  	int pin_idx;
>  	struct hdmi_spec_per_pin *per_pin;
>  	int err;
> +	int dev_num, i;
>  
>  	caps = snd_hda_query_pin_caps(codec, pin_nid);
>  	if (!(caps & (AC_PINCAP_HDMI | AC_PINCAP_DP)))
>  		return 0;
>  
> +	/*
> +	 * For DP MST audio, Configuration Default is the same for
> +	 * all device entries on the same pin
> +	 */
>  	config = snd_hda_codec_get_pincfg(codec, pin_nid);
>  	if (get_defcfg_connect(config) == AC_JACK_PORT_NONE)
>  		return 0;
>  
> -	if (is_haswell_plus(codec))
> -		intel_haswell_fixup_connect_list(codec, pin_nid);
> -
> -	pin_idx = spec->num_pins;
> -	per_pin = snd_array_new(&spec->pins);
> -	if (!per_pin)
> -		return -ENOMEM;
> -
> -	per_pin->pin_nid = pin_nid;
> -	per_pin->non_pcm = false;
> -	if (spec->dyn_pcm_assign)
> -		per_pin->pcm_idx = -1;
> -	else {
> -		per_pin->pcm = get_hdmi_pcm(spec, pin_idx);
> -		per_pin->pcm_idx = pin_idx;
> +	/*
> +	 * To simplify the implementation, malloc all
> +	 * the virtual pins in the initialization statically
> +	 */
> +	if (is_haswell_plus(codec)) {
> +		/*
> +		 * On Intel platforms, device entries number is
> +		 * changed dynamically. If there is a DP MST
> +		 * hub connected, the device entries number is 3.
> +		 * Otherwise, it is 1.
> +		 * Here we manually set dev_num to 3, so that
> +		 * we can initialize all the device entries when
> +		 * bootup statically.
> +		 */
> +		dev_num = 3;
> +		spec->dev_num = 3;
> +	} else if (spec->dyn_pcm_assign && codec->dp_mst) {
> +		dev_num = snd_hda_get_num_devices(codec, pin_nid) + 1;
> +		/*
> +		 * spec->dev_num is the maxinum number of device entries
> +		 * among all the pins
> +		 */
> +		spec->dev_num = (spec->dev_num > dev_num) ?
> +			spec->dev_num : dev_num;
> +	} else {
> +		/*
> +		 * If the platform doesn't support DP MST,
> +		 * manually set dev_num to 1. This means
> +		 * the pin has only one device entry.
> +		 */
> +		dev_num = 1;
> +		spec->dev_num = 1;
>  	}
> -	per_pin->pin_nid_idx = pin_idx;
>  
> -	err = hdmi_read_pin_conn(codec, pin_idx);
> -	if (err < 0)
> -		return err;
> +	for (i = 0; i < dev_num; i++) {
> +		pin_idx = spec->num_pins;
> +		per_pin = snd_array_new(&spec->pins);
>  
> -	spec->num_pins++;
> +		if (!per_pin)
> +			return -ENOMEM;
> +
> +		if (spec->dyn_pcm_assign) {
> +			per_pin->pcm = NULL;
> +			per_pin->pcm_idx = -1;
> +		} else {
> +			per_pin->pcm = get_hdmi_pcm(spec, pin_idx);
> +			per_pin->pcm_idx = pin_idx;
> +		}
> +		per_pin->pin_nid = pin_nid;
> +		per_pin->pin_nid_idx = spec->num_nids;
> +		per_pin->dev_id = i;
> +		per_pin->non_pcm = false;
> +		snd_hda_set_dev_select(codec, pin_nid, i);
> +		if (is_haswell_plus(codec))
> +			intel_haswell_fixup_connect_list(codec, pin_nid);
> +		err = hdmi_read_pin_conn(codec, pin_idx);
> +		if (err < 0)
> +			return err;
> +		spec->num_pins++;
> +	}
> +	spec->num_nids++;
>  
>  	return 0;
>  }
> @@ -1744,7 +1868,7 @@ static int generic_hdmi_playback_pcm_prepare(struct hda_pcm_stream *hinfo,
>  	/* Call sync_audio_rate to set the N/CTS/M manually if necessary */
>  	/* Todo: add DP1.2 MST audio support later */
>  	if (codec_has_acomp(codec))
> -		snd_hdac_sync_audio_rate(&codec->core, pin_nid, -1,
> +		snd_hdac_sync_audio_rate(&codec->core, pin_nid, per_pin->dev_id,
>  					 runtime->rate);
>  
>  	non_pcm = check_non_pcm_per_cvt(codec, cvt_nid);
> @@ -1762,6 +1886,7 @@ static int generic_hdmi_playback_pcm_prepare(struct hda_pcm_stream *hinfo,
>  				    pinctl | PIN_OUT);
>  	}
>  
> +	/* snd_hda_set_dev_select() has been called before */
>  	err = spec->ops.setup_stream(codec, cvt_nid, pin_nid,
>  				 stream_tag, format);
>  	mutex_unlock(&spec->pcm_lock);
> @@ -1897,17 +2022,23 @@ static bool is_hdmi_pcm_attached(struct hdac_device *hdac, int pcm_idx)
>  static int generic_hdmi_build_pcms(struct hda_codec *codec)
>  {
>  	struct hdmi_spec *spec = codec->spec;
> -	int pin_idx;
> +	int idx;
>  
> -	for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) {
> +	/*
> +	 * for non-mst mode, pcm number is the same as before
> +	 * for DP MST mode, pcm number is (nid number + dev_num - 1)
> +	 *  dev_num is the device entry number in a pin
> +	 *
> +	 */
> +	for (idx = 0; idx < spec->num_nids + spec->dev_num - 1; idx++) {
>  		struct hda_pcm *info;
>  		struct hda_pcm_stream *pstr;
>  
> -		info = snd_hda_codec_pcm_new(codec, "HDMI %d", pin_idx);
> +		info = snd_hda_codec_pcm_new(codec, "HDMI %d", idx);
>  		if (!info)
>  			return -ENOMEM;
>  
> -		spec->pcm_rec[pin_idx].pcm = info;
> +		spec->pcm_rec[idx].pcm = info;
>  		spec->pcm_used++;
>  		info->pcm_type = HDA_PCM_TYPE_HDMI;
>  		info->own_chmap = true;
> @@ -1915,6 +2046,9 @@ static int generic_hdmi_build_pcms(struct hda_codec *codec)
>  		pstr = &info->stream[SNDRV_PCM_STREAM_PLAYBACK];
>  		pstr->substreams = 1;
>  		pstr->ops = generic_ops;
> +		/* pcm number is less than 16 */
> +		if (spec->pcm_used >= 16)
> +			break;
>  		/* other pstr fields are set in open */
>  	}
>  
> @@ -2070,7 +2204,9 @@ static int generic_hdmi_init(struct hda_codec *codec)
>  	for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) {
>  		struct hdmi_spec_per_pin *per_pin = get_pin(spec, pin_idx);
>  		hda_nid_t pin_nid = per_pin->pin_nid;
> +		int dev_id = per_pin->dev_id;
>  
> +		snd_hda_set_dev_select(codec, pin_nid, dev_id);
>  		hdmi_init_pin(codec, pin_nid);
>  		if (!codec_has_acomp(codec))
>  			snd_hda_jack_detect_enable_callback(codec, pin_nid,
> @@ -2178,6 +2314,7 @@ static int alloc_generic_hdmi(struct hda_codec *codec)
>  		return -ENOMEM;
>  
>  	spec->ops = generic_standard_hdmi_ops;
> +	spec->dev_num = 1;	/* initialize to 1 */
>  	mutex_init(&spec->pcm_lock);
>  	snd_hdac_register_chmap_ops(&codec->core, &spec->chmap);
>  
> @@ -2295,6 +2432,7 @@ static void intel_pin_eld_notify(void *audio_ptr, int port, int pipe)
>  {
>  	struct hda_codec *codec = audio_ptr;
>  	int pin_nid;
> +	int dev_id = pipe;
>  
>  	/* we assume only from port-B to port-D */
>  	if (port < 1 || port > 3)
> @@ -2321,7 +2459,7 @@ static void intel_pin_eld_notify(void *audio_ptr, int port, int pipe)
>  		return;
>  
>  	snd_hdac_i915_set_bclk(&codec->bus->core);
> -	check_presence_and_report(codec, pin_nid);
> +	check_presence_and_report(codec, pin_nid, dev_id);
>  }
>  
>  /* register i915 component pin_eld_notify callback */
> @@ -2354,11 +2492,13 @@ static void i915_pin_cvt_fixup(struct hda_codec *codec,
>  			       hda_nid_t cvt_nid)
>  {
>  	if (per_pin) {
> +		snd_hda_set_dev_select(codec, per_pin->pin_nid,
> +			       per_pin->dev_id);
>  		intel_verify_pin_cvt_connect(codec, per_pin);
>  		intel_not_share_assigned_cvt(codec, per_pin->pin_nid,
> -					     per_pin->mux_idx);
> +				     per_pin->dev_id, per_pin->mux_idx);
>  	} else {
> -		intel_not_share_assigned_cvt_nid(codec, 0, cvt_nid);
> +		intel_not_share_assigned_cvt_nid(codec, 0, 0, cvt_nid);
>  	}
>  }
>  
> @@ -2378,6 +2518,8 @@ static int patch_i915_hsw_hdmi(struct hda_codec *codec)
>  	if (err < 0)
>  		return err;
>  	spec = codec->spec;
> +	codec->dp_mst = true;
> +	spec->dyn_pcm_assign = true;
>  
>  	intel_haswell_enable_all_pins(codec, true);
>  	intel_haswell_fixup_enable_dp12(codec);
> @@ -2389,7 +2531,6 @@ static int patch_i915_hsw_hdmi(struct hda_codec *codec)
>  		codec->core.link_power_control = 1;
>  
>  	codec->patch_ops.set_power_state = haswell_set_power_state;
> -	codec->dp_mst = true;
>  	codec->depop_delay = 0;
>  	codec->auto_runtime_pm = 1;
>  
> -- 
> 1.9.1
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 

Re: [RFC PATCH v3 2/3] ALSA: hda - add DP mst audio support

[Yang, Libin]

Hi Takashi,


> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Thursday, October 20, 2016 11:34 PM
> To: libin.yang@linux.intel.com
> Cc: alsa-devel@alsa-project.org; Yang, Libin <libin.yang@intel.com>; Lin,
> Mengdong <mengdong.lin@intel.com>
> Subject: Re: [alsa-devel] [RFC PATCH v3 2/3] ALSA: hda - add DP mst audio
> support
> 
> On Thu, 13 Oct 2016 09:49:36 +0200,
> libin.yang@linux.intel.com wrote:
> >
> > From: Libin Yang <libin.yang@linux.intel.com>
> >
> > This patch adds the DP mst audio support.
> 
> A bit more useful texts here please.  I know you'll give more explanations in
> the document in the later patch, but still something better should be
> mentioned here.

Yes, I will add more description here.

> 
> >
> > Signed-off-by: Libin Yang <libin.yang@linux.intel.com>
> > ---
> >  sound/pci/hda/hda_codec.c  |   4 +
> >  sound/pci/hda/patch_hdmi.c | 243
> > +++++++++++++++++++++++++++++++++++----------
> >  2 files changed, 196 insertions(+), 51 deletions(-)
> >
> > diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> > index caa2c9d..8a6b0a2 100644
> > --- a/sound/pci/hda/hda_codec.c
> > +++ b/sound/pci/hda/hda_codec.c
> > @@ -468,6 +468,10 @@ static int read_pin_defaults(struct hda_codec
> *codec)
> >  		pin->nid = nid;

... 

> > +
> >
> >  		/* choose an unassigned converter. The conveters in the
> >  		 * connection list are in the same order as in the codec.
> > @@ -1008,12 +1078,13 @@ static void intel_not_share_assigned_cvt(struct
> hda_codec *codec,
> >  				break;
> >  			}
> >  		}
> > +		snd_hda_set_dev_select(codec, nid, dev_id_saved);
> 
> I guess this revert won't be reached when you break or continue the loop
> beforehand?

To make it easier to explain, please let me attach the full patched code here:

for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) {
		int dev_id_saved;
		int dev_num;

		per_pin = get_pin(spec, pin_idx);
		/*
		 * pin not connected to monitor
		 * no need to operate on it
		 */
		if (!per_pin->pcm)
			continue;

		if ((per_pin->pin_nid == pin_nid) &&
			(per_pin->dev_id == dev_id))
			continue;

		/*
		 * if per_pin->dev_id >= dev_num,
		 * snd_hda_get_dev_select() will fail,
		 * and the following operation is unpredictable.
		 * So skip this situation.
		 */
		dev_num = snd_hda_get_num_devices(codec, per_pin->pin_nid) + 1;
		if (per_pin->dev_id >= dev_num)
			continue;

		nid = per_pin->pin_nid;

		/*
		 * Calling this function should not impact
		 * on the device entry selection
		 * So let's save the dev id for each pin,
		 * and restore it when return
		 */
		dev_id_saved = snd_hda_get_dev_select(codec, nid);
tag 1		snd_hda_set_dev_select(codec, nid, per_pin->dev_id); 
		curr = snd_hda_codec_read(codec, nid, 0,
					  AC_VERB_GET_CONNECT_SEL, 0);
		if (curr != mux_idx) {
tag 2			snd_hda_set_dev_select(codec, nid, dev_id_saved);
			continue;
		}


		/* choose an unassigned converter. The conveters in the
		 * connection list are in the same order as in the codec.
		 */
		for (cvt_idx = 0; cvt_idx < spec->num_cvts; cvt_idx++) {
			per_cvt = get_cvt(spec, cvt_idx);
			if (!per_cvt->assigned) {
				codec_dbg(codec,
					  "choose cvt %d for pin nid %d\n",
					cvt_idx, nid);
				snd_hda_codec_write_cache(codec, nid, 0,
					    AC_VERB_SET_CONNECT_SEL,
					    cvt_idx);
tag 3				break;
			}
		}
		snd_hda_set_dev_select(codec, nid, dev_id_saved);
	}

We need revert dev_id only after we change the dev_id setting before.
This is done by: snd_hda_set_dev_select(codec, nid, per_pn->dev_id) (tag1)
There is one "continue" and we have reverted dev_id before continue (tag 2)
There is one "break" (tag 3). However, this is for another for loop.

> 
> Also, this rewrite changes the loop completely, so we you should update the
> comment appropriately.  It's no longer scanning the all pins (including
> unused ones).  And I'm not 100% sure whether it's OK to change in that way;
> the function was written to do that (touching the unused pin as well) on
> purpose, IIRC.
> 
> Hm, or is it OK because we are listing always the three pins?
> More explanation please, either in comments or in the changelog.
> 

This function is a little complex now. Please let me explain it in detail.

As you know, we need this function because we need fix: same cvt is
connecting to several pins. In this situation, playback on one pin will also
impact on other pins. Let's assume cvt 1 are connecting to pin 1, 2, 3.
When we playback on pin1, we need call this function to make sure
pin 2, 3 is connected to other cvts.

In MST mode,  we are using dynamic pcm. If no pcm is attached to
the pin, it means there is no monitor connected to the pin. So we
don't need to set the cvt. Let's still assume the cvt 1 are connecting to 
pin 1, 2, 3, 4, 5, 6, 7, 8, 9 (here pin is the virtual pin).

Let's assume playback on pin 1 (which is connected to cvt 1). 
So we should make sure all others pins are not connected to cvt1.

1) For other pins:
    if (!per_pin->pcm)
        continue;
This means no monitors is connected to the pin. So we don't care
the cvt connection because the pin doesn't output sound to monitor.
(when the virtual pin is connected to monitor later, this function will
be called in present_sense(), which will make sure the cvt connected
to the virtual pin is different from the cvt connected to pin 1)

2) For the following situation:
    dev_num = snd_hda_get_num_devices(codec, per_pin->pin_nid) + 1;
        if (per_pin->dev_id >= dev_num)
            continue;
The per_pin->dev_id is greater than dev_num. This means no such
virtual pin. We should skip this situation.

3) For the following situation:
    if (curr != mux_idx) {
        snd_hda_set_dev_select(codec, nid, dev_id_saved);
        continue;
    }
It means the cvt connected to the virtual pin is already different to 
the cvt connected to the pin 1. So we don't need to do anything. 

4) Otherwise (the cvt is the same to pin 1), we need change the cvt.

I will add more description for this function in the next version patch.

Regards,
Libin

> 
> Takashi
> 
> >  	}
> >  }
> >
> >  /* A wrapper of intel_not_share_asigned_cvt() */  static void
> > intel_not_share_assigned_cvt_nid(struct hda_codec *codec,
> > -			hda_nid_t pin_nid, hda_nid_t cvt_nid)
> > +			hda_nid_t pin_nid, int dev_id, hda_nid_t cvt_nid)
> >  {
> >  	int mux_idx;
> >  	struct hdmi_spec *spec = codec->spec; @@ -1025,7 +1096,7 @@
> static
> > void intel_not_share_assigned_cvt_nid(struct hda_codec *codec,
> >  	 */
> >  	mux_idx = intel_cvt_id_to_mux_idx(spec, cvt_nid);
> >  	if (mux_idx >= 0)
> > -		intel_not_share_assigned_cvt(codec, pin_nid, mux_idx);
> > +		intel_not_share_assigned_cvt(codec, pin_nid, dev_id,
> mux_idx);
> >  }
> >
> >  /* skeleton caller of pin_cvt_fixup ops */ @@ -1140,6 +1211,7 @@
> > static int hdmi_pcm_open(struct hda_pcm_stream *hinfo,
> >  	per_pin->cvt_nid = per_cvt->cvt_nid;
> >  	hinfo->nid = per_cvt->cvt_nid;
> >

Re: [RFC PATCH v3 2/3] ALSA: hda - add DP mst audio support

[Takashi Iwai]

On Fri, 21 Oct 2016 10:24:26 +0200,
Yang, Libin wrote:
> 
> Hi Takashi,
> 
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai@suse.de]
> > Sent: Thursday, October 20, 2016 11:34 PM
> > To: libin.yang@linux.intel.com
> > Cc: alsa-devel@alsa-project.org; Yang, Libin <libin.yang@intel.com>; Lin,
> > Mengdong <mengdong.lin@intel.com>
> > Subject: Re: [alsa-devel] [RFC PATCH v3 2/3] ALSA: hda - add DP mst audio
> > support
> > 
> > On Thu, 13 Oct 2016 09:49:36 +0200,
> > libin.yang@linux.intel.com wrote:
> > >
> > > From: Libin Yang <libin.yang@linux.intel.com>
> > >
> > > This patch adds the DP mst audio support.
> > 
> > A bit more useful texts here please.  I know you'll give more explanations in
> > the document in the later patch, but still something better should be
> > mentioned here.
> 
> Yes, I will add more description here.
> 
> > 
> > >
> > > Signed-off-by: Libin Yang <libin.yang@linux.intel.com>
> > > ---
> > >  sound/pci/hda/hda_codec.c  |   4 +
> > >  sound/pci/hda/patch_hdmi.c | 243
> > > +++++++++++++++++++++++++++++++++++----------
> > >  2 files changed, 196 insertions(+), 51 deletions(-)
> > >
> > > diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> > > index caa2c9d..8a6b0a2 100644
> > > --- a/sound/pci/hda/hda_codec.c
> > > +++ b/sound/pci/hda/hda_codec.c
> > > @@ -468,6 +468,10 @@ static int read_pin_defaults(struct hda_codec
> > *codec)
> > >  		pin->nid = nid;
> 
> ... 
> 
> > > +
> > >
> > >  		/* choose an unassigned converter. The conveters in the
> > >  		 * connection list are in the same order as in the codec.
> > > @@ -1008,12 +1078,13 @@ static void intel_not_share_assigned_cvt(struct
> > hda_codec *codec,
> > >  				break;
> > >  			}
> > >  		}
> > > +		snd_hda_set_dev_select(codec, nid, dev_id_saved);
> > 
> > I guess this revert won't be reached when you break or continue the loop
> > beforehand?
> 
> To make it easier to explain, please let me attach the full patched code here:
> 
> for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) {
> 		int dev_id_saved;
> 		int dev_num;
> 
> 		per_pin = get_pin(spec, pin_idx);
> 		/*
> 		 * pin not connected to monitor
> 		 * no need to operate on it
> 		 */
> 		if (!per_pin->pcm)
> 			continue;
> 
> 		if ((per_pin->pin_nid == pin_nid) &&
> 			(per_pin->dev_id == dev_id))
> 			continue;
> 
> 		/*
> 		 * if per_pin->dev_id >= dev_num,
> 		 * snd_hda_get_dev_select() will fail,
> 		 * and the following operation is unpredictable.
> 		 * So skip this situation.
> 		 */
> 		dev_num = snd_hda_get_num_devices(codec, per_pin->pin_nid) + 1;
> 		if (per_pin->dev_id >= dev_num)
> 			continue;
> 
> 		nid = per_pin->pin_nid;
> 
> 		/*
> 		 * Calling this function should not impact
> 		 * on the device entry selection
> 		 * So let's save the dev id for each pin,
> 		 * and restore it when return
> 		 */
> 		dev_id_saved = snd_hda_get_dev_select(codec, nid);
> tag 1		snd_hda_set_dev_select(codec, nid, per_pin->dev_id); 
> 		curr = snd_hda_codec_read(codec, nid, 0,
> 					  AC_VERB_GET_CONNECT_SEL, 0);
> 		if (curr != mux_idx) {
> tag 2			snd_hda_set_dev_select(codec, nid, dev_id_saved);
> 			continue;
> 		}
> 
> 
> 		/* choose an unassigned converter. The conveters in the
> 		 * connection list are in the same order as in the codec.
> 		 */
> 		for (cvt_idx = 0; cvt_idx < spec->num_cvts; cvt_idx++) {
> 			per_cvt = get_cvt(spec, cvt_idx);
> 			if (!per_cvt->assigned) {
> 				codec_dbg(codec,
> 					  "choose cvt %d for pin nid %d\n",
> 					cvt_idx, nid);
> 				snd_hda_codec_write_cache(codec, nid, 0,
> 					    AC_VERB_SET_CONNECT_SEL,
> 					    cvt_idx);
> tag 3				break;
> 			}
> 		}
> 		snd_hda_set_dev_select(codec, nid, dev_id_saved);
> 	}
> 
> We need revert dev_id only after we change the dev_id setting before.
> This is done by: snd_hda_set_dev_select(codec, nid, per_pn->dev_id) (tag1)
> There is one "continue" and we have reverted dev_id before continue (tag 2)
> There is one "break" (tag 3). However, this is for another for loop.

OK, fair enough.


> > Also, this rewrite changes the loop completely, so we you should update the
> > comment appropriately.  It's no longer scanning the all pins (including
> > unused ones).  And I'm not 100% sure whether it's OK to change in that way;
> > the function was written to do that (touching the unused pin as well) on
> > purpose, IIRC.
> > 
> > Hm, or is it OK because we are listing always the three pins?
> > More explanation please, either in comments or in the changelog.
> > 
> 
> This function is a little complex now. Please let me explain it in detail.
> 
> As you know, we need this function because we need fix: same cvt is
> connecting to several pins. In this situation, playback on one pin will also
> impact on other pins. Let's assume cvt 1 are connecting to pin 1, 2, 3.
> When we playback on pin1, we need call this function to make sure
> pin 2, 3 is connected to other cvts.
> 
> In MST mode,  we are using dynamic pcm. If no pcm is attached to
> the pin, it means there is no monitor connected to the pin. So we
> don't need to set the cvt. Let's still assume the cvt 1 are connecting to 
> pin 1, 2, 3, 4, 5, 6, 7, 8, 9 (here pin is the virtual pin).
> 
> Let's assume playback on pin 1 (which is connected to cvt 1). 
> So we should make sure all others pins are not connected to cvt1.
> 
> 1) For other pins:
>     if (!per_pin->pcm)
>         continue;
> This means no monitors is connected to the pin. So we don't care
> the cvt connection because the pin doesn't output sound to monitor.
> (when the virtual pin is connected to monitor later, this function will
> be called in present_sense(), which will make sure the cvt connected
> to the virtual pin is different from the cvt connected to pin 1)
> 
> 2) For the following situation:
>     dev_num = snd_hda_get_num_devices(codec, per_pin->pin_nid) + 1;
>         if (per_pin->dev_id >= dev_num)
>             continue;
> The per_pin->dev_id is greater than dev_num. This means no such
> virtual pin. We should skip this situation.
> 
> 3) For the following situation:
>     if (curr != mux_idx) {
>         snd_hda_set_dev_select(codec, nid, dev_id_saved);
>         continue;
>     }
> It means the cvt connected to the virtual pin is already different to 
> the cvt connected to the pin 1. So we don't need to do anything. 
> 
> 4) Otherwise (the cvt is the same to pin 1), we need change the cvt.
> 
> I will add more description for this function in the next version patch.

One thing that is still unclear to me is how to deal with the
unconfigured pins.  I thought that the pins with port=non pincfg are
still ignored in hdmi_add_pin() even after your patch.  So they won't
be listed in the pins / nids arrays.

Now, your new code only loops over the enumerated pins, i.e. these
unconfigured pins are ignored.  What if these pins are connected to
the active converters?  I thought excluding such a connection is one
of the purposes of this function, judging from the function
description.


thanks,

Takashi

Re: [RFC PATCH v3 2/3] ALSA: hda - add DP mst audio support

[Yang, Libin]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Friday, October 21, 2016 4:44 PM
> To: Yang, Libin <libin.yang@intel.com>
> Cc: libin.yang@linux.intel.com; alsa-devel@alsa-project.org; Lin, Mengdong
> <mengdong.lin@intel.com>
> Subject: Re: [alsa-devel] [RFC PATCH v3 2/3] ALSA: hda - add DP mst audio
> support
> 
> On Fri, 21 Oct 2016 10:24:26 +0200,
> Yang, Libin wrote:
> >
> > Hi Takashi,
> >
> >
> > > -----Original Message-----
> > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > Sent: Thursday, October 20, 2016 11:34 PM
> > > To: libin.yang@linux.intel.com
> > > Cc: alsa-devel@alsa-project.org; Yang, Libin <libin.yang@intel.com>;
> > > Lin, Mengdong <mengdong.lin@intel.com>
> > > Subject: Re: [alsa-devel] [RFC PATCH v3 2/3] ALSA: hda - add DP mst
> > > audio support
> > >
> > > On Thu, 13 Oct 2016 09:49:36 +0200,
> > > libin.yang@linux.intel.com wrote:
> > > >
> > > > From: Libin Yang <libin.yang@linux.intel.com>
> > > >
> > > > This patch adds the DP mst audio support.
> > >
> > > A bit more useful texts here please.  I know you'll give more
> > > explanations in the document in the later patch, but still something
> > > better should be mentioned here.
> >
> > Yes, I will add more description here.
> >
> > >
> > > >
> > > > Signed-off-by: Libin Yang <libin.yang@linux.intel.com>
> > > > ---
> > > >  sound/pci/hda/hda_codec.c  |   4 +
> > > >  sound/pci/hda/patch_hdmi.c | 243
> > > > +++++++++++++++++++++++++++++++++++----------
> > > >  2 files changed, 196 insertions(+), 51 deletions(-)
> > > >
> > > > diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> > > > index caa2c9d..8a6b0a2 100644
> > > > --- a/sound/pci/hda/hda_codec.c
> > > > +++ b/sound/pci/hda/hda_codec.c
> > > > @@ -468,6 +468,10 @@ static int read_pin_defaults(struct hda_codec
> > > *codec)
> > > >  		pin->nid = nid;
> >
> > ...
> >
> > > > +
> > > >
> > > >  		/* choose an unassigned converter. The conveters in the
> > > >  		 * connection list are in the same order as in the codec.
> > > > @@ -1008,12 +1078,13 @@ static void
> > > > intel_not_share_assigned_cvt(struct
> > > hda_codec *codec,
> > > >  				break;
> > > >  			}
> > > >  		}
> > > > +		snd_hda_set_dev_select(codec, nid, dev_id_saved);
> > >
> > > I guess this revert won't be reached when you break or continue the
> > > loop beforehand?
> >
> > To make it easier to explain, please let me attach the full patched code here:
> >
> > for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) {
> > 		int dev_id_saved;
> > 		int dev_num;
> >
> > 		per_pin = get_pin(spec, pin_idx);
> > 		/*
> > 		 * pin not connected to monitor
> > 		 * no need to operate on it
> > 		 */
> > 		if (!per_pin->pcm)
> > 			continue;
> >
> > 		if ((per_pin->pin_nid == pin_nid) &&
> > 			(per_pin->dev_id == dev_id))
> > 			continue;
> >
> > 		/*
> > 		 * if per_pin->dev_id >= dev_num,
> > 		 * snd_hda_get_dev_select() will fail,
> > 		 * and the following operation is unpredictable.
> > 		 * So skip this situation.
> > 		 */
> > 		dev_num = snd_hda_get_num_devices(codec, per_pin-
> >pin_nid) + 1;
> > 		if (per_pin->dev_id >= dev_num)
> > 			continue;
> >
> > 		nid = per_pin->pin_nid;
> >
> > 		/*
> > 		 * Calling this function should not impact
> > 		 * on the device entry selection
> > 		 * So let's save the dev id for each pin,
> > 		 * and restore it when return
> > 		 */
> > 		dev_id_saved = snd_hda_get_dev_select(codec, nid);
> > tag 1		snd_hda_set_dev_select(codec, nid, per_pin->dev_id);
> > 		curr = snd_hda_codec_read(codec, nid, 0,
> > 					  AC_VERB_GET_CONNECT_SEL, 0);
> > 		if (curr != mux_idx) {
> > tag 2			snd_hda_set_dev_select(codec, nid, dev_id_saved);
> > 			continue;
> > 		}
> >
> >
> > 		/* choose an unassigned converter. The conveters in the
> > 		 * connection list are in the same order as in the codec.
> > 		 */
> > 		for (cvt_idx = 0; cvt_idx < spec->num_cvts; cvt_idx++) {
> > 			per_cvt = get_cvt(spec, cvt_idx);
> > 			if (!per_cvt->assigned) {
> > 				codec_dbg(codec,
> > 					  "choose cvt %d for pin nid %d\n",
> > 					cvt_idx, nid);
> > 				snd_hda_codec_write_cache(codec, nid, 0,
> > 					    AC_VERB_SET_CONNECT_SEL,
> > 					    cvt_idx);
> > tag 3				break;
> > 			}
> > 		}
> > 		snd_hda_set_dev_select(codec, nid, dev_id_saved);
> > 	}
> >
> > We need revert dev_id only after we change the dev_id setting before.
> > This is done by: snd_hda_set_dev_select(codec, nid, per_pn->dev_id)
> > (tag1) There is one "continue" and we have reverted dev_id before
> > continue (tag 2) There is one "break" (tag 3). However, this is for another
> for loop.
> 
> OK, fair enough.
> 
> 
> > > Also, this rewrite changes the loop completely, so we you should
> > > update the comment appropriately.  It's no longer scanning the all
> > > pins (including unused ones).  And I'm not 100% sure whether it's OK
> > > to change in that way; the function was written to do that (touching
> > > the unused pin as well) on purpose, IIRC.
> > >
> > > Hm, or is it OK because we are listing always the three pins?
> > > More explanation please, either in comments or in the changelog.
> > >
> >
> > This function is a little complex now. Please let me explain it in detail.
> >
> > As you know, we need this function because we need fix: same cvt is
> > connecting to several pins. In this situation, playback on one pin
> > will also impact on other pins. Let's assume cvt 1 are connecting to pin 1, 2,
> 3.
> > When we playback on pin1, we need call this function to make sure pin
> > 2, 3 is connected to other cvts.
> >
> > In MST mode,  we are using dynamic pcm. If no pcm is attached to the
> > pin, it means there is no monitor connected to the pin. So we don't
> > need to set the cvt. Let's still assume the cvt 1 are connecting to
> > pin 1, 2, 3, 4, 5, 6, 7, 8, 9 (here pin is the virtual pin).
> >
> > Let's assume playback on pin 1 (which is connected to cvt 1).
> > So we should make sure all others pins are not connected to cvt1.
> >
> > 1) For other pins:
> >     if (!per_pin->pcm)
> >         continue;
> > This means no monitors is connected to the pin. So we don't care the
> > cvt connection because the pin doesn't output sound to monitor.
> > (when the virtual pin is connected to monitor later, this function
> > will be called in present_sense(), which will make sure the cvt
> > connected to the virtual pin is different from the cvt connected to
> > pin 1)
> >
> > 2) For the following situation:
> >     dev_num = snd_hda_get_num_devices(codec, per_pin->pin_nid) + 1;
> >         if (per_pin->dev_id >= dev_num)
> >             continue;
> > The per_pin->dev_id is greater than dev_num. This means no such
> > virtual pin. We should skip this situation.
> >
> > 3) For the following situation:
> >     if (curr != mux_idx) {
> >         snd_hda_set_dev_select(codec, nid, dev_id_saved);
> >         continue;
> >     }
> > It means the cvt connected to the virtual pin is already different to
> > the cvt connected to the pin 1. So we don't need to do anything.
> >
> > 4) Otherwise (the cvt is the same to pin 1), we need change the cvt.
> >
> > I will add more description for this function in the next version patch.
> 
> One thing that is still unclear to me is how to deal with the unconfigured pins.
> I thought that the pins with port=non pincfg are still ignored in
> hdmi_add_pin() even after your patch.  So they won't be listed in the pins /
> nids arrays.

Actually, MST code doesn't touch port=non code. It exists before.

My understanding of this code is that port=non is HW connection
related. If this pin is not connected to any jack, it means it is not
lined out and the pin is useless for the special machine.

> 
> Now, your new code only loops over the enumerated pins, i.e. these
> unconfigured pins are ignored.  What if these pins are connected to the
> active converters?  I thought excluding such a connection is one of the
> purposes of this function, judging from the function description.

If the pin is not lined out, no monitor will be connected to the pin. So even
the cvt is connected to the pin, it will impact nothing. For MST mode,
if it is not lined out, there will be never pcm attached to the pin. This means
you will never have chance to operate the cvt connected to the pin.
Besides, I found if there is no monitor connected to the pin, 
the connection list is not actually connecting to any cvt for the pin.

Regards,
Libin

> 
> 
> thanks,
> 
> Takashi

Re: [RFC PATCH v3 2/3] ALSA: hda - add DP mst audio support

[Takashi Iwai]

On Fri, 21 Oct 2016 10:56:24 +0200,
Yang, Libin wrote:
> 
> 
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai@suse.de]
> > Sent: Friday, October 21, 2016 4:44 PM
> > To: Yang, Libin <libin.yang@intel.com>
> > Cc: libin.yang@linux.intel.com; alsa-devel@alsa-project.org; Lin, Mengdong
> > <mengdong.lin@intel.com>
> > Subject: Re: [alsa-devel] [RFC PATCH v3 2/3] ALSA: hda - add DP mst audio
> > support
> > 
> > On Fri, 21 Oct 2016 10:24:26 +0200,
> > Yang, Libin wrote:
> > >
> > > Hi Takashi,
> > >
> > >
> > > > -----Original Message-----
> > > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > Sent: Thursday, October 20, 2016 11:34 PM
> > > > To: libin.yang@linux.intel.com
> > > > Cc: alsa-devel@alsa-project.org; Yang, Libin <libin.yang@intel.com>;
> > > > Lin, Mengdong <mengdong.lin@intel.com>
> > > > Subject: Re: [alsa-devel] [RFC PATCH v3 2/3] ALSA: hda - add DP mst
> > > > audio support
> > > >
> > > > On Thu, 13 Oct 2016 09:49:36 +0200,
> > > > libin.yang@linux.intel.com wrote:
> > > > >
> > > > > From: Libin Yang <libin.yang@linux.intel.com>
> > > > >
> > > > > This patch adds the DP mst audio support.
> > > >
> > > > A bit more useful texts here please.  I know you'll give more
> > > > explanations in the document in the later patch, but still something
> > > > better should be mentioned here.
> > >
> > > Yes, I will add more description here.
> > >
> > > >
> > > > >
> > > > > Signed-off-by: Libin Yang <libin.yang@linux.intel.com>
> > > > > ---
> > > > >  sound/pci/hda/hda_codec.c  |   4 +
> > > > >  sound/pci/hda/patch_hdmi.c | 243
> > > > > +++++++++++++++++++++++++++++++++++----------
> > > > >  2 files changed, 196 insertions(+), 51 deletions(-)
> > > > >
> > > > > diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> > > > > index caa2c9d..8a6b0a2 100644
> > > > > --- a/sound/pci/hda/hda_codec.c
> > > > > +++ b/sound/pci/hda/hda_codec.c
> > > > > @@ -468,6 +468,10 @@ static int read_pin_defaults(struct hda_codec
> > > > *codec)
> > > > >  		pin->nid = nid;
> > >
> > > ...
> > >
> > > > > +
> > > > >
> > > > >  		/* choose an unassigned converter. The conveters in the
> > > > >  		 * connection list are in the same order as in the codec.
> > > > > @@ -1008,12 +1078,13 @@ static void
> > > > > intel_not_share_assigned_cvt(struct
> > > > hda_codec *codec,
> > > > >  				break;
> > > > >  			}
> > > > >  		}
> > > > > +		snd_hda_set_dev_select(codec, nid, dev_id_saved);
> > > >
> > > > I guess this revert won't be reached when you break or continue the
> > > > loop beforehand?
> > >
> > > To make it easier to explain, please let me attach the full patched code here:
> > >
> > > for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) {
> > > 		int dev_id_saved;
> > > 		int dev_num;
> > >
> > > 		per_pin = get_pin(spec, pin_idx);
> > > 		/*
> > > 		 * pin not connected to monitor
> > > 		 * no need to operate on it
> > > 		 */
> > > 		if (!per_pin->pcm)
> > > 			continue;
> > >
> > > 		if ((per_pin->pin_nid == pin_nid) &&
> > > 			(per_pin->dev_id == dev_id))
> > > 			continue;
> > >
> > > 		/*
> > > 		 * if per_pin->dev_id >= dev_num,
> > > 		 * snd_hda_get_dev_select() will fail,
> > > 		 * and the following operation is unpredictable.
> > > 		 * So skip this situation.
> > > 		 */
> > > 		dev_num = snd_hda_get_num_devices(codec, per_pin-
> > >pin_nid) + 1;
> > > 		if (per_pin->dev_id >= dev_num)
> > > 			continue;
> > >
> > > 		nid = per_pin->pin_nid;
> > >
> > > 		/*
> > > 		 * Calling this function should not impact
> > > 		 * on the device entry selection
> > > 		 * So let's save the dev id for each pin,
> > > 		 * and restore it when return
> > > 		 */
> > > 		dev_id_saved = snd_hda_get_dev_select(codec, nid);
> > > tag 1		snd_hda_set_dev_select(codec, nid, per_pin->dev_id);
> > > 		curr = snd_hda_codec_read(codec, nid, 0,
> > > 					  AC_VERB_GET_CONNECT_SEL, 0);
> > > 		if (curr != mux_idx) {
> > > tag 2			snd_hda_set_dev_select(codec, nid, dev_id_saved);
> > > 			continue;
> > > 		}
> > >
> > >
> > > 		/* choose an unassigned converter. The conveters in the
> > > 		 * connection list are in the same order as in the codec.
> > > 		 */
> > > 		for (cvt_idx = 0; cvt_idx < spec->num_cvts; cvt_idx++) {
> > > 			per_cvt = get_cvt(spec, cvt_idx);
> > > 			if (!per_cvt->assigned) {
> > > 				codec_dbg(codec,
> > > 					  "choose cvt %d for pin nid %d\n",
> > > 					cvt_idx, nid);
> > > 				snd_hda_codec_write_cache(codec, nid, 0,
> > > 					    AC_VERB_SET_CONNECT_SEL,
> > > 					    cvt_idx);
> > > tag 3				break;
> > > 			}
> > > 		}
> > > 		snd_hda_set_dev_select(codec, nid, dev_id_saved);
> > > 	}
> > >
> > > We need revert dev_id only after we change the dev_id setting before.
> > > This is done by: snd_hda_set_dev_select(codec, nid, per_pn->dev_id)
> > > (tag1) There is one "continue" and we have reverted dev_id before
> > > continue (tag 2) There is one "break" (tag 3). However, this is for another
> > for loop.
> > 
> > OK, fair enough.
> > 
> > 
> > > > Also, this rewrite changes the loop completely, so we you should
> > > > update the comment appropriately.  It's no longer scanning the all
> > > > pins (including unused ones).  And I'm not 100% sure whether it's OK
> > > > to change in that way; the function was written to do that (touching
> > > > the unused pin as well) on purpose, IIRC.
> > > >
> > > > Hm, or is it OK because we are listing always the three pins?
> > > > More explanation please, either in comments or in the changelog.
> > > >
> > >
> > > This function is a little complex now. Please let me explain it in detail.
> > >
> > > As you know, we need this function because we need fix: same cvt is
> > > connecting to several pins. In this situation, playback on one pin
> > > will also impact on other pins. Let's assume cvt 1 are connecting to pin 1, 2,
> > 3.
> > > When we playback on pin1, we need call this function to make sure pin
> > > 2, 3 is connected to other cvts.
> > >
> > > In MST mode,  we are using dynamic pcm. If no pcm is attached to the
> > > pin, it means there is no monitor connected to the pin. So we don't
> > > need to set the cvt. Let's still assume the cvt 1 are connecting to
> > > pin 1, 2, 3, 4, 5, 6, 7, 8, 9 (here pin is the virtual pin).
> > >
> > > Let's assume playback on pin 1 (which is connected to cvt 1).
> > > So we should make sure all others pins are not connected to cvt1.
> > >
> > > 1) For other pins:
> > >     if (!per_pin->pcm)
> > >         continue;
> > > This means no monitors is connected to the pin. So we don't care the
> > > cvt connection because the pin doesn't output sound to monitor.
> > > (when the virtual pin is connected to monitor later, this function
> > > will be called in present_sense(), which will make sure the cvt
> > > connected to the virtual pin is different from the cvt connected to
> > > pin 1)
> > >
> > > 2) For the following situation:
> > >     dev_num = snd_hda_get_num_devices(codec, per_pin->pin_nid) + 1;
> > >         if (per_pin->dev_id >= dev_num)
> > >             continue;
> > > The per_pin->dev_id is greater than dev_num. This means no such
> > > virtual pin. We should skip this situation.
> > >
> > > 3) For the following situation:
> > >     if (curr != mux_idx) {
> > >         snd_hda_set_dev_select(codec, nid, dev_id_saved);
> > >         continue;
> > >     }
> > > It means the cvt connected to the virtual pin is already different to
> > > the cvt connected to the pin 1. So we don't need to do anything.
> > >
> > > 4) Otherwise (the cvt is the same to pin 1), we need change the cvt.
> > >
> > > I will add more description for this function in the next version patch.
> > 
> > One thing that is still unclear to me is how to deal with the unconfigured pins.
> > I thought that the pins with port=non pincfg are still ignored in
> > hdmi_add_pin() even after your patch.  So they won't be listed in the pins /
> > nids arrays.
> 
> Actually, MST code doesn't touch port=non code. It exists before.
> 
> My understanding of this code is that port=non is HW connection
> related. If this pin is not connected to any jack, it means it is not
> lined out and the pin is useless for the special machine.
> 
> > 
> > Now, your new code only loops over the enumerated pins, i.e. these
> > unconfigured pins are ignored.  What if these pins are connected to the
> > active converters?  I thought excluding such a connection is one of the
> > purposes of this function, judging from the function description.
> 
> If the pin is not lined out, no monitor will be connected to the pin. So even
> the cvt is connected to the pin, it will impact nothing. For MST mode,
> if it is not lined out, there will be never pcm attached to the pin. This means
> you will never have chance to operate the cvt connected to the pin.
> Besides, I found if there is no monitor connected to the pin, 
> the connection list is not actually connecting to any cvt for the pin.

But why the current function explicitly states that it does care about
the unused pins?  There must be a reason to do so.  I vaguely remember
that it was mentioned like that connecting to the unused pin causing
some unexpected behavior.


Takashi

Re: [RFC PATCH v3 2/3] ALSA: hda - add DP mst audio support

[Yang, Libin]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Friday, October 21, 2016 5:05 PM
> To: Yang, Libin <libin.yang@intel.com>
> Cc: libin.yang@linux.intel.com; alsa-devel@alsa-project.org; Lin, Mengdong
> <mengdong.lin@intel.com>
> Subject: Re: [alsa-devel] [RFC PATCH v3 2/3] ALSA: hda - add DP mst audio
> support
> 
> On Fri, 21 Oct 2016 10:56:24 +0200,
> Yang, Libin wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > Sent: Friday, October 21, 2016 4:44 PM
> > > To: Yang, Libin <libin.yang@intel.com>
> > > Cc: libin.yang@linux.intel.com; alsa-devel@alsa-project.org; Lin,
> > > Mengdong <mengdong.lin@intel.com>
> > > Subject: Re: [alsa-devel] [RFC PATCH v3 2/3] ALSA: hda - add DP mst
> > > audio support
> > >
> > > On Fri, 21 Oct 2016 10:24:26 +0200,
> > > Yang, Libin wrote:
> > > >
> > > > Hi Takashi,
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > > Sent: Thursday, October 20, 2016 11:34 PM
> > > > > To: libin.yang@linux.intel.com
> > > > > Cc: alsa-devel@alsa-project.org; Yang, Libin
> > > > > <libin.yang@intel.com>; Lin, Mengdong <mengdong.lin@intel.com>
> > > > > Subject: Re: [alsa-devel] [RFC PATCH v3 2/3] ALSA: hda - add DP
> > > > > mst audio support
> > > > >
> > > > > On Thu, 13 Oct 2016 09:49:36 +0200, libin.yang@linux.intel.com
> > > > > wrote:
> > > > > >
> > > > > > From: Libin Yang <libin.yang@linux.intel.com>
> > > > > >
> > > > > > This patch adds the DP mst audio support.
> > > > >
> > > > > A bit more useful texts here please.  I know you'll give more
> > > > > explanations in the document in the later patch, but still
> > > > > something better should be mentioned here.
> > > >
> > > > Yes, I will add more description here.
> > > >
> > > > >
> > > > > >
> > > > > > Signed-off-by: Libin Yang <libin.yang@linux.intel.com>
> > > > > > ---
> > > > > >  sound/pci/hda/hda_codec.c  |   4 +
> > > > > >  sound/pci/hda/patch_hdmi.c | 243
> > > > > > +++++++++++++++++++++++++++++++++++----------
> > > > > >  2 files changed, 196 insertions(+), 51 deletions(-)
> > > > > >
> > > > > > diff --git a/sound/pci/hda/hda_codec.c
> > > > > > b/sound/pci/hda/hda_codec.c index caa2c9d..8a6b0a2 100644
> > > > > > --- a/sound/pci/hda/hda_codec.c
> > > > > > +++ b/sound/pci/hda/hda_codec.c
> > > > > > @@ -468,6 +468,10 @@ static int read_pin_defaults(struct
> > > > > > hda_codec
> > > > > *codec)
> > > > > >  		pin->nid = nid;
> > > >
> > > > ...
> > > >
> > > > > > +
> > > > > >
> > > > > >  		/* choose an unassigned converter. The conveters in
> the
> > > > > >  		 * connection list are in the same order as in the
> codec.
> > > > > > @@ -1008,12 +1078,13 @@ static void
> > > > > > intel_not_share_assigned_cvt(struct
> > > > > hda_codec *codec,
> > > > > >  				break;
> > > > > >  			}
> > > > > >  		}
> > > > > > +		snd_hda_set_dev_select(codec, nid, dev_id_saved);
> > > > >
> > > > > I guess this revert won't be reached when you break or continue
> > > > > the loop beforehand?
> > > >
> > > > To make it easier to explain, please let me attach the full patched code
> here:
> > > >
> > > > for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) {
> > > > 		int dev_id_saved;
> > > > 		int dev_num;
> > > >
> > > > 		per_pin = get_pin(spec, pin_idx);
> > > > 		/*
> > > > 		 * pin not connected to monitor
> > > > 		 * no need to operate on it
> > > > 		 */
> > > > 		if (!per_pin->pcm)
> > > > 			continue;
> > > >
> > > > 		if ((per_pin->pin_nid == pin_nid) &&
> > > > 			(per_pin->dev_id == dev_id))
> > > > 			continue;
> > > >
> > > > 		/*
> > > > 		 * if per_pin->dev_id >= dev_num,
> > > > 		 * snd_hda_get_dev_select() will fail,
> > > > 		 * and the following operation is unpredictable.
> > > > 		 * So skip this situation.
> > > > 		 */
> > > > 		dev_num = snd_hda_get_num_devices(codec, per_pin-
> > > >pin_nid) + 1;
> > > > 		if (per_pin->dev_id >= dev_num)
> > > > 			continue;
> > > >
> > > > 		nid = per_pin->pin_nid;
> > > >
> > > > 		/*
> > > > 		 * Calling this function should not impact
> > > > 		 * on the device entry selection
> > > > 		 * So let's save the dev id for each pin,
> > > > 		 * and restore it when return
> > > > 		 */
> > > > 		dev_id_saved = snd_hda_get_dev_select(codec, nid);
> > > > tag 1		snd_hda_set_dev_select(codec, nid, per_pin-
> >dev_id);
> > > > 		curr = snd_hda_codec_read(codec, nid, 0,
> > > > 					  AC_VERB_GET_CONNECT_SEL, 0);
> > > > 		if (curr != mux_idx) {
> > > > tag 2			snd_hda_set_dev_select(codec, nid,
> dev_id_saved);
> > > > 			continue;
> > > > 		}
> > > >
> > > >
> > > > 		/* choose an unassigned converter. The conveters in the
> > > > 		 * connection list are in the same order as in the codec.
> > > > 		 */
> > > > 		for (cvt_idx = 0; cvt_idx < spec->num_cvts; cvt_idx++) {
> > > > 			per_cvt = get_cvt(spec, cvt_idx);
> > > > 			if (!per_cvt->assigned) {
> > > > 				codec_dbg(codec,
> > > > 					  "choose cvt %d for pin nid %d\n",
> > > > 					cvt_idx, nid);
> > > > 				snd_hda_codec_write_cache(codec, nid, 0,
> > > > 					    AC_VERB_SET_CONNECT_SEL,
> > > > 					    cvt_idx);
> > > > tag 3				break;
> > > > 			}
> > > > 		}
> > > > 		snd_hda_set_dev_select(codec, nid, dev_id_saved);
> > > > 	}
> > > >
> > > > We need revert dev_id only after we change the dev_id setting before.
> > > > This is done by: snd_hda_set_dev_select(codec, nid,
> > > > per_pn->dev_id)
> > > > (tag1) There is one "continue" and we have reverted dev_id before
> > > > continue (tag 2) There is one "break" (tag 3). However, this is
> > > > for another
> > > for loop.
> > >
> > > OK, fair enough.
> > >
> > >
> > > > > Also, this rewrite changes the loop completely, so we you should
> > > > > update the comment appropriately.  It's no longer scanning the
> > > > > all pins (including unused ones).  And I'm not 100% sure whether
> > > > > it's OK to change in that way; the function was written to do
> > > > > that (touching the unused pin as well) on purpose, IIRC.
> > > > >
> > > > > Hm, or is it OK because we are listing always the three pins?
> > > > > More explanation please, either in comments or in the changelog.
> > > > >
> > > >
> > > > This function is a little complex now. Please let me explain it in detail.
> > > >
> > > > As you know, we need this function because we need fix: same cvt
> > > > is connecting to several pins. In this situation, playback on one
> > > > pin will also impact on other pins. Let's assume cvt 1 are
> > > > connecting to pin 1, 2,
> > > 3.
> > > > When we playback on pin1, we need call this function to make sure
> > > > pin 2, 3 is connected to other cvts.
> > > >
> > > > In MST mode,  we are using dynamic pcm. If no pcm is attached to
> > > > the pin, it means there is no monitor connected to the pin. So we
> > > > don't need to set the cvt. Let's still assume the cvt 1 are
> > > > connecting to pin 1, 2, 3, 4, 5, 6, 7, 8, 9 (here pin is the virtual pin).
> > > >
> > > > Let's assume playback on pin 1 (which is connected to cvt 1).
> > > > So we should make sure all others pins are not connected to cvt1.
> > > >
> > > > 1) For other pins:
> > > >     if (!per_pin->pcm)
> > > >         continue;
> > > > This means no monitors is connected to the pin. So we don't care
> > > > the cvt connection because the pin doesn't output sound to monitor.
> > > > (when the virtual pin is connected to monitor later, this function
> > > > will be called in present_sense(), which will make sure the cvt
> > > > connected to the virtual pin is different from the cvt connected
> > > > to pin 1)
> > > >
> > > > 2) For the following situation:
> > > >     dev_num = snd_hda_get_num_devices(codec, per_pin->pin_nid) + 1;
> > > >         if (per_pin->dev_id >= dev_num)
> > > >             continue;
> > > > The per_pin->dev_id is greater than dev_num. This means no such
> > > > virtual pin. We should skip this situation.
> > > >
> > > > 3) For the following situation:
> > > >     if (curr != mux_idx) {
> > > >         snd_hda_set_dev_select(codec, nid, dev_id_saved);
> > > >         continue;
> > > >     }
> > > > It means the cvt connected to the virtual pin is already different
> > > > to the cvt connected to the pin 1. So we don't need to do anything.
> > > >
> > > > 4) Otherwise (the cvt is the same to pin 1), we need change the cvt.
> > > >
> > > > I will add more description for this function in the next version patch.
> > >
> > > One thing that is still unclear to me is how to deal with the unconfigured
> pins.
> > > I thought that the pins with port=non pincfg are still ignored in
> > > hdmi_add_pin() even after your patch.  So they won't be listed in
> > > the pins / nids arrays.
> >
> > Actually, MST code doesn't touch port=non code. It exists before.
> >
> > My understanding of this code is that port=non is HW connection
> > related. If this pin is not connected to any jack, it means it is not
> > lined out and the pin is useless for the special machine.
> >
> > >
> > > Now, your new code only loops over the enumerated pins, i.e. these
> > > unconfigured pins are ignored.  What if these pins are connected to
> > > the active converters?  I thought excluding such a connection is one
> > > of the purposes of this function, judging from the function description.
> >
> > If the pin is not lined out, no monitor will be connected to the pin.
> > So even the cvt is connected to the pin, it will impact nothing. For
> > MST mode, if it is not lined out, there will be never pcm attached to
> > the pin. This means you will never have chance to operate the cvt
> connected to the pin.
> > Besides, I found if there is no monitor connected to the pin, the
> > connection list is not actually connecting to any cvt for the pin.
> 
> But why the current function explicitly states that it does care about the
> unused pins?  There must be a reason to do so.  I vaguely remember that it
> was mentioned like that connecting to the unused pin causing some
> unexpected behavior.

I don't know the story. Maybe in this situation, mute/unmute will be
messed. I will check with our internal team to see if someone knows
this reason. You question reminds me that I should try to find a proper 
platform to test the unused pin situation. My current test platforms 
can't cover such situation.

Regards,
Libin

> 
> 
> Takashi

Re: [RFC PATCH v3 2/3] ALSA: hda - add DP mst audio support

[Takashi Iwai]

On Fri, 21 Oct 2016 14:25:28 +0200,
Yang, Libin wrote:
> 
> > > > > > Also, this rewrite changes the loop completely, so we you should
> > > > > > update the comment appropriately.  It's no longer scanning the
> > > > > > all pins (including unused ones).  And I'm not 100% sure whether
> > > > > > it's OK to change in that way; the function was written to do
> > > > > > that (touching the unused pin as well) on purpose, IIRC.
> > > > > >
> > > > > > Hm, or is it OK because we are listing always the three pins?
> > > > > > More explanation please, either in comments or in the changelog.
> > > > > >
> > > > >
> > > > > This function is a little complex now. Please let me explain it in detail.
> > > > >
> > > > > As you know, we need this function because we need fix: same cvt
> > > > > is connecting to several pins. In this situation, playback on one
> > > > > pin will also impact on other pins. Let's assume cvt 1 are
> > > > > connecting to pin 1, 2,
> > > > 3.
> > > > > When we playback on pin1, we need call this function to make sure
> > > > > pin 2, 3 is connected to other cvts.
> > > > >
> > > > > In MST mode,  we are using dynamic pcm. If no pcm is attached to
> > > > > the pin, it means there is no monitor connected to the pin. So we
> > > > > don't need to set the cvt. Let's still assume the cvt 1 are
> > > > > connecting to pin 1, 2, 3, 4, 5, 6, 7, 8, 9 (here pin is the virtual pin).
> > > > >
> > > > > Let's assume playback on pin 1 (which is connected to cvt 1).
> > > > > So we should make sure all others pins are not connected to cvt1.
> > > > >
> > > > > 1) For other pins:
> > > > >     if (!per_pin->pcm)
> > > > >         continue;
> > > > > This means no monitors is connected to the pin. So we don't care
> > > > > the cvt connection because the pin doesn't output sound to monitor.
> > > > > (when the virtual pin is connected to monitor later, this function
> > > > > will be called in present_sense(), which will make sure the cvt
> > > > > connected to the virtual pin is different from the cvt connected
> > > > > to pin 1)
> > > > >
> > > > > 2) For the following situation:
> > > > >     dev_num = snd_hda_get_num_devices(codec, per_pin->pin_nid) + 1;
> > > > >         if (per_pin->dev_id >= dev_num)
> > > > >             continue;
> > > > > The per_pin->dev_id is greater than dev_num. This means no such
> > > > > virtual pin. We should skip this situation.
> > > > >
> > > > > 3) For the following situation:
> > > > >     if (curr != mux_idx) {
> > > > >         snd_hda_set_dev_select(codec, nid, dev_id_saved);
> > > > >         continue;
> > > > >     }
> > > > > It means the cvt connected to the virtual pin is already different
> > > > > to the cvt connected to the pin 1. So we don't need to do anything.
> > > > >
> > > > > 4) Otherwise (the cvt is the same to pin 1), we need change the cvt.
> > > > >
> > > > > I will add more description for this function in the next version patch.
> > > >
> > > > One thing that is still unclear to me is how to deal with the unconfigured
> > pins.
> > > > I thought that the pins with port=non pincfg are still ignored in
> > > > hdmi_add_pin() even after your patch.  So they won't be listed in
> > > > the pins / nids arrays.
> > >
> > > Actually, MST code doesn't touch port=non code. It exists before.
> > >
> > > My understanding of this code is that port=non is HW connection
> > > related. If this pin is not connected to any jack, it means it is not
> > > lined out and the pin is useless for the special machine.
> > >
> > > >
> > > > Now, your new code only loops over the enumerated pins, i.e. these
> > > > unconfigured pins are ignored.  What if these pins are connected to
> > > > the active converters?  I thought excluding such a connection is one
> > > > of the purposes of this function, judging from the function description.
> > >
> > > If the pin is not lined out, no monitor will be connected to the pin.
> > > So even the cvt is connected to the pin, it will impact nothing. For
> > > MST mode, if it is not lined out, there will be never pcm attached to
> > > the pin. This means you will never have chance to operate the cvt
> > connected to the pin.
> > > Besides, I found if there is no monitor connected to the pin, the
> > > connection list is not actually connecting to any cvt for the pin.
> > 
> > But why the current function explicitly states that it does care about the
> > unused pins?  There must be a reason to do so.  I vaguely remember that it
> > was mentioned like that connecting to the unused pin causing some
> > unexpected behavior.
> 
> I don't know the story. Maybe in this situation, mute/unmute will be
> messed. I will check with our internal team to see if someone knows
> this reason.

Mengdong?


> You question reminds me that I should try to find a proper 
> platform to test the unused pin situation. My current test platforms 
> can't cover such situation.

Yes, a wider test coverage would be really appreciated.


Takashi

Re: [RFC PATCH v3 2/3] ALSA: hda - add DP mst audio support

[Yang, Libin]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Friday, October 21, 2016 8:33 PM
> To: Yang, Libin <libin.yang@intel.com>
> Cc: libin.yang@linux.intel.com; alsa-devel@alsa-project.org; Lin, Mengdong
> <mengdong.lin@intel.com>
> Subject: Re: [alsa-devel] [RFC PATCH v3 2/3] ALSA: hda - add DP mst audio
> support
> 
> On Fri, 21 Oct 2016 14:25:28 +0200,
> Yang, Libin wrote:
> >
> > > > > > > Also, this rewrite changes the loop completely, so we you
> > > > > > > should update the comment appropriately.  It's no longer
> > > > > > > scanning the all pins (including unused ones).  And I'm not
> > > > > > > 100% sure whether it's OK to change in that way; the
> > > > > > > function was written to do that (touching the unused pin as well)
> on purpose, IIRC.
> > > > > > >
> > > > > > > Hm, or is it OK because we are listing always the three pins?
> > > > > > > More explanation please, either in comments or in the changelog.
> > > > > > >
> > > > > >
> > > > > > This function is a little complex now. Please let me explain it in detail.
> > > > > >
> > > > > > As you know, we need this function because we need fix: same
> > > > > > cvt is connecting to several pins. In this situation, playback
> > > > > > on one pin will also impact on other pins. Let's assume cvt 1
> > > > > > are connecting to pin 1, 2,
> > > > > 3.
> > > > > > When we playback on pin1, we need call this function to make
> > > > > > sure pin 2, 3 is connected to other cvts.
> > > > > >
> > > > > > In MST mode,  we are using dynamic pcm. If no pcm is attached
> > > > > > to the pin, it means there is no monitor connected to the pin.
> > > > > > So we don't need to set the cvt. Let's still assume the cvt 1
> > > > > > are connecting to pin 1, 2, 3, 4, 5, 6, 7, 8, 9 (here pin is the virtual
> pin).
> > > > > >
> > > > > > Let's assume playback on pin 1 (which is connected to cvt 1).
> > > > > > So we should make sure all others pins are not connected to cvt1.
> > > > > >
> > > > > > 1) For other pins:
> > > > > >     if (!per_pin->pcm)
> > > > > >         continue;
> > > > > > This means no monitors is connected to the pin. So we don't
> > > > > > care the cvt connection because the pin doesn't output sound to
> monitor.
> > > > > > (when the virtual pin is connected to monitor later, this
> > > > > > function will be called in present_sense(), which will make
> > > > > > sure the cvt connected to the virtual pin is different from
> > > > > > the cvt connected to pin 1)
> > > > > >
> > > > > > 2) For the following situation:
> > > > > >     dev_num = snd_hda_get_num_devices(codec, per_pin->pin_nid)
> + 1;
> > > > > >         if (per_pin->dev_id >= dev_num)
> > > > > >             continue;
> > > > > > The per_pin->dev_id is greater than dev_num. This means no
> > > > > > such virtual pin. We should skip this situation.
> > > > > >
> > > > > > 3) For the following situation:
> > > > > >     if (curr != mux_idx) {
> > > > > >         snd_hda_set_dev_select(codec, nid, dev_id_saved);
> > > > > >         continue;
> > > > > >     }
> > > > > > It means the cvt connected to the virtual pin is already
> > > > > > different to the cvt connected to the pin 1. So we don't need to do
> anything.
> > > > > >
> > > > > > 4) Otherwise (the cvt is the same to pin 1), we need change the cvt.
> > > > > >
> > > > > > I will add more description for this function in the next version patch.
> > > > >
> > > > > One thing that is still unclear to me is how to deal with the
> > > > > unconfigured
> > > pins.
> > > > > I thought that the pins with port=non pincfg are still ignored
> > > > > in
> > > > > hdmi_add_pin() even after your patch.  So they won't be listed
> > > > > in the pins / nids arrays.
> > > >
> > > > Actually, MST code doesn't touch port=non code. It exists before.
> > > >
> > > > My understanding of this code is that port=non is HW connection
> > > > related. If this pin is not connected to any jack, it means it is
> > > > not lined out and the pin is useless for the special machine.
> > > >
> > > > >
> > > > > Now, your new code only loops over the enumerated pins, i.e.
> > > > > these unconfigured pins are ignored.  What if these pins are
> > > > > connected to the active converters?  I thought excluding such a
> > > > > connection is one of the purposes of this function, judging from the
> function description.
> > > >
> > > > If the pin is not lined out, no monitor will be connected to the pin.
> > > > So even the cvt is connected to the pin, it will impact nothing.
> > > > For MST mode, if it is not lined out, there will be never pcm
> > > > attached to the pin. This means you will never have chance to
> > > > operate the cvt
> > > connected to the pin.
> > > > Besides, I found if there is no monitor connected to the pin, the
> > > > connection list is not actually connecting to any cvt for the pin.
> > >
> > > But why the current function explicitly states that it does care
> > > about the unused pins?  There must be a reason to do so.  I vaguely
> > > remember that it was mentioned like that connecting to the unused
> > > pin causing some unexpected behavior.
> >
> > I don't know the story. Maybe in this situation, mute/unmute will be
> > messed. I will check with our internal team to see if someone knows
> > this reason.
> 
> Mengdong?

Yes, Mengdong or our silicon team.

Regards,
Libin

> 
> 
> > You question reminds me that I should try to find a proper platform to
> > test the unused pin situation. My current test platforms can't cover
> > such situation.
> 
> Yes, a wider test coverage would be really appreciated.
> 
> 
> Takashi

Re: [RFC PATCH v3 2/3] ALSA: hda - add DP mst audio support

[Yang, Libin]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Friday, October 21, 2016 8:33 PM
> To: Yang, Libin <libin.yang@intel.com>
> Cc: libin.yang@linux.intel.com; alsa-devel@alsa-project.org; Lin, Mengdong
> <mengdong.lin@intel.com>
> Subject: Re: [alsa-devel] [RFC PATCH v3 2/3] ALSA: hda - add DP mst audio
> support
> 
> On Fri, 21 Oct 2016 14:25:28 +0200,
> Yang, Libin wrote:
> >
> > > > > > > Also, this rewrite changes the loop completely, so we you
> > > > > > > should update the comment appropriately.  It's no longer
> > > > > > > scanning the all pins (including unused ones).  And I'm not
> > > > > > > 100% sure whether it's OK to change in that way; the
> > > > > > > function was written to do that (touching the unused pin as well)
> on purpose, IIRC.
> > > > > > >
> > > > > > > Hm, or is it OK because we are listing always the three pins?
> > > > > > > More explanation please, either in comments or in the changelog.
> > > > > > >
> > > > > >
> > > > > > This function is a little complex now. Please let me explain it in detail.
> > > > > >
> > > > > > As you know, we need this function because we need fix: same
> > > > > > cvt is connecting to several pins. In this situation, playback
> > > > > > on one pin will also impact on other pins. Let's assume cvt 1
> > > > > > are connecting to pin 1, 2,
> > > > > 3.
> > > > > > When we playback on pin1, we need call this function to make
> > > > > > sure pin 2, 3 is connected to other cvts.
> > > > > >
> > > > > > In MST mode,  we are using dynamic pcm. If no pcm is attached
> > > > > > to the pin, it means there is no monitor connected to the pin.
> > > > > > So we don't need to set the cvt. Let's still assume the cvt 1
> > > > > > are connecting to pin 1, 2, 3, 4, 5, 6, 7, 8, 9 (here pin is the virtual
> pin).
> > > > > >
> > > > > > Let's assume playback on pin 1 (which is connected to cvt 1).
> > > > > > So we should make sure all others pins are not connected to cvt1.
> > > > > >
> > > > > > 1) For other pins:
> > > > > >     if (!per_pin->pcm)
> > > > > >         continue;
> > > > > > This means no monitors is connected to the pin. So we don't
> > > > > > care the cvt connection because the pin doesn't output sound to
> monitor.
> > > > > > (when the virtual pin is connected to monitor later, this
> > > > > > function will be called in present_sense(), which will make
> > > > > > sure the cvt connected to the virtual pin is different from
> > > > > > the cvt connected to pin 1)
> > > > > >
> > > > > > 2) For the following situation:
> > > > > >     dev_num = snd_hda_get_num_devices(codec, per_pin->pin_nid)
> + 1;
> > > > > >         if (per_pin->dev_id >= dev_num)
> > > > > >             continue;
> > > > > > The per_pin->dev_id is greater than dev_num. This means no
> > > > > > such virtual pin. We should skip this situation.
> > > > > >
> > > > > > 3) For the following situation:
> > > > > >     if (curr != mux_idx) {
> > > > > >         snd_hda_set_dev_select(codec, nid, dev_id_saved);
> > > > > >         continue;
> > > > > >     }
> > > > > > It means the cvt connected to the virtual pin is already
> > > > > > different to the cvt connected to the pin 1. So we don't need to do
> anything.
> > > > > >
> > > > > > 4) Otherwise (the cvt is the same to pin 1), we need change the cvt.
> > > > > >
> > > > > > I will add more description for this function in the next version patch.
> > > > >
> > > > > One thing that is still unclear to me is how to deal with the
> > > > > unconfigured
> > > pins.
> > > > > I thought that the pins with port=non pincfg are still ignored
> > > > > in
> > > > > hdmi_add_pin() even after your patch.  So they won't be listed
> > > > > in the pins / nids arrays.
> > > >
> > > > Actually, MST code doesn't touch port=non code. It exists before.
> > > >
> > > > My understanding of this code is that port=non is HW connection
> > > > related. If this pin is not connected to any jack, it means it is
> > > > not lined out and the pin is useless for the special machine.
> > > >
> > > > >
> > > > > Now, your new code only loops over the enumerated pins, i.e.
> > > > > these unconfigured pins are ignored.  What if these pins are
> > > > > connected to the active converters?  I thought excluding such a
> > > > > connection is one of the purposes of this function, judging from the
> function description.
> > > >
> > > > If the pin is not lined out, no monitor will be connected to the pin.
> > > > So even the cvt is connected to the pin, it will impact nothing.
> > > > For MST mode, if it is not lined out, there will be never pcm
> > > > attached to the pin. This means you will never have chance to
> > > > operate the cvt
> > > connected to the pin.
> > > > Besides, I found if there is no monitor connected to the pin, the
> > > > connection list is not actually connecting to any cvt for the pin.
> > >
> > > But why the current function explicitly states that it does care
> > > about the unused pins?  There must be a reason to do so.  I vaguely
> > > remember that it was mentioned like that connecting to the unused
> > > pin causing some unexpected behavior.
> >
> > I don't know the story. Maybe in this situation, mute/unmute will be
> > messed. I will check with our internal team to see if someone knows
> > this reason.
> 
> Mengdong?

The patch of checking AC_JACK_PORT_NONE is from Stephen Warren,
who is from nvidia. I have checked with Mengdong, and she didn't 
know the patch. But it seems to be similar with intel_not_share_xxx()
function.

Regards,
Libin

> 
> 
> > You question reminds me that I should try to find a proper platform to
> > test the unused pin situation. My current test platforms can't cover
> > such situation.
> 
> Yes, a wider test coverage would be really appreciated.
> 
> 
> Takashi

Re: [RFC PATCH v3 2/3] ALSA: hda - add DP mst audio support

[Takashi Iwai]

On Mon, 24 Oct 2016 09:09:07 +0200,
Yang, Libin wrote:
> 
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai@suse.de]
> > Sent: Friday, October 21, 2016 8:33 PM
> > To: Yang, Libin <libin.yang@intel.com>
> > Cc: libin.yang@linux.intel.com; alsa-devel@alsa-project.org; Lin, Mengdong
> > <mengdong.lin@intel.com>
> > Subject: Re: [alsa-devel] [RFC PATCH v3 2/3] ALSA: hda - add DP mst audio
> > support
> > 
> > On Fri, 21 Oct 2016 14:25:28 +0200,
> > Yang, Libin wrote:
> > >
> > > > > > > > Also, this rewrite changes the loop completely, so we you
> > > > > > > > should update the comment appropriately.  It's no longer
> > > > > > > > scanning the all pins (including unused ones).  And I'm not
> > > > > > > > 100% sure whether it's OK to change in that way; the
> > > > > > > > function was written to do that (touching the unused pin as well)
> > on purpose, IIRC.
> > > > > > > >
> > > > > > > > Hm, or is it OK because we are listing always the three pins?
> > > > > > > > More explanation please, either in comments or in the changelog.
> > > > > > > >
> > > > > > >
> > > > > > > This function is a little complex now. Please let me explain it in detail.
> > > > > > >
> > > > > > > As you know, we need this function because we need fix: same
> > > > > > > cvt is connecting to several pins. In this situation, playback
> > > > > > > on one pin will also impact on other pins. Let's assume cvt 1
> > > > > > > are connecting to pin 1, 2,
> > > > > > 3.
> > > > > > > When we playback on pin1, we need call this function to make
> > > > > > > sure pin 2, 3 is connected to other cvts.
> > > > > > >
> > > > > > > In MST mode,  we are using dynamic pcm. If no pcm is attached
> > > > > > > to the pin, it means there is no monitor connected to the pin.
> > > > > > > So we don't need to set the cvt. Let's still assume the cvt 1
> > > > > > > are connecting to pin 1, 2, 3, 4, 5, 6, 7, 8, 9 (here pin is the virtual
> > pin).
> > > > > > >
> > > > > > > Let's assume playback on pin 1 (which is connected to cvt 1).
> > > > > > > So we should make sure all others pins are not connected to cvt1.
> > > > > > >
> > > > > > > 1) For other pins:
> > > > > > >     if (!per_pin->pcm)
> > > > > > >         continue;
> > > > > > > This means no monitors is connected to the pin. So we don't
> > > > > > > care the cvt connection because the pin doesn't output sound to
> > monitor.
> > > > > > > (when the virtual pin is connected to monitor later, this
> > > > > > > function will be called in present_sense(), which will make
> > > > > > > sure the cvt connected to the virtual pin is different from
> > > > > > > the cvt connected to pin 1)
> > > > > > >
> > > > > > > 2) For the following situation:
> > > > > > >     dev_num = snd_hda_get_num_devices(codec, per_pin->pin_nid)
> > + 1;
> > > > > > >         if (per_pin->dev_id >= dev_num)
> > > > > > >             continue;
> > > > > > > The per_pin->dev_id is greater than dev_num. This means no
> > > > > > > such virtual pin. We should skip this situation.
> > > > > > >
> > > > > > > 3) For the following situation:
> > > > > > >     if (curr != mux_idx) {
> > > > > > >         snd_hda_set_dev_select(codec, nid, dev_id_saved);
> > > > > > >         continue;
> > > > > > >     }
> > > > > > > It means the cvt connected to the virtual pin is already
> > > > > > > different to the cvt connected to the pin 1. So we don't need to do
> > anything.
> > > > > > >
> > > > > > > 4) Otherwise (the cvt is the same to pin 1), we need change the cvt.
> > > > > > >
> > > > > > > I will add more description for this function in the next version patch.
> > > > > >
> > > > > > One thing that is still unclear to me is how to deal with the
> > > > > > unconfigured
> > > > pins.
> > > > > > I thought that the pins with port=non pincfg are still ignored
> > > > > > in
> > > > > > hdmi_add_pin() even after your patch.  So they won't be listed
> > > > > > in the pins / nids arrays.
> > > > >
> > > > > Actually, MST code doesn't touch port=non code. It exists before.
> > > > >
> > > > > My understanding of this code is that port=non is HW connection
> > > > > related. If this pin is not connected to any jack, it means it is
> > > > > not lined out and the pin is useless for the special machine.
> > > > >
> > > > > >
> > > > > > Now, your new code only loops over the enumerated pins, i.e.
> > > > > > these unconfigured pins are ignored.  What if these pins are
> > > > > > connected to the active converters?  I thought excluding such a
> > > > > > connection is one of the purposes of this function, judging from the
> > function description.
> > > > >
> > > > > If the pin is not lined out, no monitor will be connected to the pin.
> > > > > So even the cvt is connected to the pin, it will impact nothing.
> > > > > For MST mode, if it is not lined out, there will be never pcm
> > > > > attached to the pin. This means you will never have chance to
> > > > > operate the cvt
> > > > connected to the pin.
> > > > > Besides, I found if there is no monitor connected to the pin, the
> > > > > connection list is not actually connecting to any cvt for the pin.
> > > >
> > > > But why the current function explicitly states that it does care
> > > > about the unused pins?  There must be a reason to do so.  I vaguely
> > > > remember that it was mentioned like that connecting to the unused
> > > > pin causing some unexpected behavior.
> > >
> > > I don't know the story. Maybe in this situation, mute/unmute will be
> > > messed. I will check with our internal team to see if someone knows
> > > this reason.
> > 
> > Mengdong?
> 
> The patch of checking AC_JACK_PORT_NONE is from Stephen Warren,
> who is from nvidia. I have checked with Mengdong, and she didn't 
> know the patch. But it seems to be similar with intel_not_share_xxx()
> function.

Well, ignoring the unused port there is basically irrelevant with the
behavior in intel_not_share_*().  It was introduced in commit
116dcde638065a6b94344ca570e1e79c8e857826
    ALSA: HDA: Remove unconnected PCM devices for Intel HDMI

Meanwhile intel_not_share_*() stuff was introduced much later in
commit 7ef166b831237e67b2ea83ce0c933c46ddd6eb26
    ALSA: hda - Avoid choose same converter for unused pins


Takashi

Re: [RFC PATCH v3 2/3] ALSA: hda - add DP mst audio support

[Yang, Libin]

Hi Takashi,

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Monday, October 24, 2016 4:41 PM
> To: Yang, Libin <libin.yang@intel.com>
> Cc: libin.yang@linux.intel.com; alsa-devel@alsa-project.org; Lin, Mengdong
> <mengdong.lin@intel.com>
> Subject: Re: [alsa-devel] [RFC PATCH v3 2/3] ALSA: hda - add DP mst audio
> support
> 
> On Mon, 24 Oct 2016 09:09:07 +0200,
> Yang, Libin wrote:
> >
> >
> > > -----Original Message-----
> > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > Sent: Friday, October 21, 2016 8:33 PM
> > > To: Yang, Libin <libin.yang@intel.com>
> > > Cc: libin.yang@linux.intel.com; alsa-devel@alsa-project.org; Lin,
> > > Mengdong <mengdong.lin@intel.com>
> > > Subject: Re: [alsa-devel] [RFC PATCH v3 2/3] ALSA: hda - add DP mst
> > > audio support
> > >
> > > On Fri, 21 Oct 2016 14:25:28 +0200,
> > > Yang, Libin wrote:
> > > >
> > > > > > > > > Also, this rewrite changes the loop completely, so we
> > > > > > > > > you should update the comment appropriately.  It's no
> > > > > > > > > longer scanning the all pins (including unused ones).
> > > > > > > > > And I'm not 100% sure whether it's OK to change in that
> > > > > > > > > way; the function was written to do that (touching the
> > > > > > > > > unused pin as well)
> > > on purpose, IIRC.
> > > > > > > > >
> > > > > > > > > Hm, or is it OK because we are listing always the three pins?
> > > > > > > > > More explanation please, either in comments or in the
> changelog.
> > > > > > > > >
> > > > > > > >
> > > > > > > > This function is a little complex now. Please let me explain it in
> detail.
> > > > > > > >
> > > > > > > > As you know, we need this function because we need fix:
> > > > > > > > same cvt is connecting to several pins. In this situation,
> > > > > > > > playback on one pin will also impact on other pins. Let's
> > > > > > > > assume cvt 1 are connecting to pin 1, 2,
> > > > > > > 3.
> > > > > > > > When we playback on pin1, we need call this function to
> > > > > > > > make sure pin 2, 3 is connected to other cvts.
> > > > > > > >
> > > > > > > > In MST mode,  we are using dynamic pcm. If no pcm is
> > > > > > > > attached to the pin, it means there is no monitor connected to
> the pin.
> > > > > > > > So we don't need to set the cvt. Let's still assume the
> > > > > > > > cvt 1 are connecting to pin 1, 2, 3, 4, 5, 6, 7, 8, 9
> > > > > > > > (here pin is the virtual
> > > pin).
> > > > > > > >
> > > > > > > > Let's assume playback on pin 1 (which is connected to cvt 1).
> > > > > > > > So we should make sure all others pins are not connected to cvt1.
> > > > > > > >
> > > > > > > > 1) For other pins:
> > > > > > > >     if (!per_pin->pcm)
> > > > > > > >         continue;
> > > > > > > > This means no monitors is connected to the pin. So we
> > > > > > > > don't care the cvt connection because the pin doesn't
> > > > > > > > output sound to
> > > monitor.
> > > > > > > > (when the virtual pin is connected to monitor later, this
> > > > > > > > function will be called in present_sense(), which will
> > > > > > > > make sure the cvt connected to the virtual pin is
> > > > > > > > different from the cvt connected to pin 1)
> > > > > > > >
> > > > > > > > 2) For the following situation:
> > > > > > > >     dev_num = snd_hda_get_num_devices(codec,
> > > > > > > > per_pin->pin_nid)
> > > + 1;
> > > > > > > >         if (per_pin->dev_id >= dev_num)
> > > > > > > >             continue;
> > > > > > > > The per_pin->dev_id is greater than dev_num. This means no
> > > > > > > > such virtual pin. We should skip this situation.
> > > > > > > >
> > > > > > > > 3) For the following situation:
> > > > > > > >     if (curr != mux_idx) {
> > > > > > > >         snd_hda_set_dev_select(codec, nid, dev_id_saved);
> > > > > > > >         continue;
> > > > > > > >     }
> > > > > > > > It means the cvt connected to the virtual pin is already
> > > > > > > > different to the cvt connected to the pin 1. So we don't
> > > > > > > > need to do
> > > anything.
> > > > > > > >
> > > > > > > > 4) Otherwise (the cvt is the same to pin 1), we need change the
> cvt.
> > > > > > > >
> > > > > > > > I will add more description for this function in the next version
> patch.
> > > > > > >
> > > > > > > One thing that is still unclear to me is how to deal with
> > > > > > > the unconfigured
> > > > > pins.
> > > > > > > I thought that the pins with port=non pincfg are still
> > > > > > > ignored in
> > > > > > > hdmi_add_pin() even after your patch.  So they won't be
> > > > > > > listed in the pins / nids arrays.
> > > > > >
> > > > > > Actually, MST code doesn't touch port=non code. It exists before.
> > > > > >
> > > > > > My understanding of this code is that port=non is HW
> > > > > > connection related. If this pin is not connected to any jack,
> > > > > > it means it is not lined out and the pin is useless for the special
> machine.
> > > > > >
> > > > > > >
> > > > > > > Now, your new code only loops over the enumerated pins, i.e.
> > > > > > > these unconfigured pins are ignored.  What if these pins are
> > > > > > > connected to the active converters?  I thought excluding
> > > > > > > such a connection is one of the purposes of this function,
> > > > > > > judging from the
> > > function description.
> > > > > >
> > > > > > If the pin is not lined out, no monitor will be connected to the pin.
> > > > > > So even the cvt is connected to the pin, it will impact nothing.
> > > > > > For MST mode, if it is not lined out, there will be never pcm
> > > > > > attached to the pin. This means you will never have chance to
> > > > > > operate the cvt
> > > > > connected to the pin.
> > > > > > Besides, I found if there is no monitor connected to the pin,
> > > > > > the connection list is not actually connecting to any cvt for the pin.
> > > > >
> > > > > But why the current function explicitly states that it does care
> > > > > about the unused pins?  There must be a reason to do so.  I
> > > > > vaguely remember that it was mentioned like that connecting to
> > > > > the unused pin causing some unexpected behavior.
> > > >
> > > > I don't know the story. Maybe in this situation, mute/unmute will
> > > > be messed. I will check with our internal team to see if someone
> > > > knows this reason.
> > >
> > > Mengdong?
> >
> > The patch of checking AC_JACK_PORT_NONE is from Stephen Warren, who
> is
> > from nvidia. I have checked with Mengdong, and she didn't know the
> > patch. But it seems to be similar with intel_not_share_xxx() function.
> 
> Well, ignoring the unused port there is basically irrelevant with the behavior
> in intel_not_share_*().  It was introduced in commit
> 116dcde638065a6b94344ca570e1e79c8e857826
>     ALSA: HDA: Remove unconnected PCM devices for Intel HDMI
> 
> Meanwhile intel_not_share_*() stuff was introduced much later in commit
> 7ef166b831237e67b2ea83ce0c933c46ddd6eb26
>     ALSA: hda - Avoid choose same converter for unused pins

Get it. It is used to reduce the user confusion. Thanks.

Regards,
Libin

> 
> 
> Takashi

[RFC PATCH v3 3/3] ALSA: Documentation about HDA DP MST pin init and connection

[libin.yang]

From: Libin Yang <libin.yang@linux.intel.com>

Add the documentation about HD-audio DP MST:
1. pin initialization
2. device entry connection list

Signed-off-by: Libin Yang <libin.yang@linux.intel.com>
---
 Documentation/sound/alsa/HD-Audio-DP-MST-audio.txt | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/Documentation/sound/alsa/HD-Audio-DP-MST-audio.txt b/Documentation/sound/alsa/HD-Audio-DP-MST-audio.txt
index 82744ac..0c5ea7d 100644
--- a/Documentation/sound/alsa/HD-Audio-DP-MST-audio.txt
+++ b/Documentation/sound/alsa/HD-Audio-DP-MST-audio.txt
@@ -15,6 +15,23 @@ PCM
 ===
 To be added
 
+Pin Initialization
+==================
+Each pin may have several device entries (virtual pins). On Intel platform,
+the device entries number is dynamically changed. If DP MST hub is connected,
+it is in DP MST mode, and the device entries number is 3. Otherwise, the
+device entries number is 1.
+
+To simplify the implementation, all the device entries will be initialized
+when bootup no matter whether it is in DP MST mode or not.
+
+Connection list
+===============
+DP MST reuses connection list code. The code can be reused because
+device entries on the same pin have the same connection list.
+
+This means DP MST gets the device entry connection list without the
+device entry setting.
 
 Jack
 ====
-- 
1.9.1

Re: [RFC PATCH v3 0/3] support DP MST audio

[Yang, Libin]

Hi Takashi,

Pandiyan, Dhinakaran has root caused the MST flicker issue. He is
still working with gfx team to decide how to make the patch.
The draft patch can be found:
https://patchwork.freedesktop.org/patch/115585/

BTW: Not sure if you have time to review my updated
audio patches. Thanks.

Regards,
Libin


> -----Original Message-----
> From: libin.yang@linux.intel.com [mailto:libin.yang@linux.intel.com]
> Sent: Thursday, October 13, 2016 3:50 PM
> To: alsa-devel@alsa-project.org; tiwai@suse.de
> Cc: Lin, Mengdong <mengdong.lin@intel.com>; Yang, Libin
> <libin.yang@intel.com>; Libin Yang <libin.yang@linux.intel.com>
> Subject: [alsa-devel] [RFC PATCH v3 0/3] support DP MST audio
> 
> From: Libin Yang <libin.yang@linux.intel.com>
> 
> This patchset starts to support DP MST audio.
> 
> This patchset is based on drm-intel-nightly on
> git://anongit.freedesktop.org/drm-intel
> 
> Libin Yang (3):
>   ALSA: hda - add DP mst verb support
>   ALSA: hda - add DP mst audio support
>   ALSA: Documentation about HDA DP MST pin init and connection
> 
>  Documentation/sound/alsa/HD-Audio-DP-MST-audio.txt |  17 ++
>  sound/pci/hda/hda_codec.c                          |  77 ++++++-
>  sound/pci/hda/hda_codec.h                          |   3 +
>  sound/pci/hda/patch_hdmi.c                         | 243 ++++++++++++++++-----
>  4 files changed, 285 insertions(+), 55 deletions(-)
> 
> --
> 1.9.1

Re: [RFC PATCH v3 0/3] support DP MST audio

[Takashi Iwai]

On Thu, 20 Oct 2016 09:23:57 +0200,
Yang, Libin wrote:
> 
> Hi Takashi,
> 
> Pandiyan, Dhinakaran has root caused the MST flicker issue. He is
> still working with gfx team to decide how to make the patch.
> The draft patch can be found:
> https://patchwork.freedesktop.org/patch/115585/
> 
> BTW: Not sure if you have time to review my updated
> audio patches. Thanks.

The code changes look mostly OK through a quick glance.
Some places have minor coding style issues and more changelogs would
be required.

I'll give detailed review comments later.


thanks,

Takashi