[RFC PATCH v2 0/3] start to support MST

[RFC PATCH v2 0/3] start to support MST

[libin.yang]

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

These patches are draft patches to support MST. The patches doesn't
work because of lacking support in gfx driver.

Libin Yang (3):
  ALSA: hda - add DP mst verb support
  ALSA - hda: add devid support in acom
  ALSA - hda: add DP MST support

 drivers/gpu/drm/i915/intel_audio.c |   2 +-
 include/drm/i915_component.h       |   2 +-
 include/sound/hda_i915.h           |   6 +-
 sound/hda/hdac_i915.c              |   7 +-
 sound/pci/hda/hda_codec.c          |  86 ++++++++++++++++-
 sound/pci/hda/hda_codec.h          |   3 +
 sound/pci/hda/patch_hdmi.c         | 184 +++++++++++++++++++++++++++----------
 7 files changed, 227 insertions(+), 63 deletions(-)

-- 
1.9.1

[RFC PATCH v2 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 | 83 ++++++++++++++++++++++++++++++++++++++++++++---
 sound/pci/hda/hda_codec.h |  3 ++
 2 files changed, 82 insertions(+), 4 deletions(-)

diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index d17e38e..db94a66 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -325,9 +325,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;
@@ -341,6 +348,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
@@ -358,7 +366,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;
 
@@ -382,6 +390,73 @@ 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, dev, num_devices;
+	unsigned long timeout;
+
+	/* 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);
+	/* If Device List Length is 0, the pin is not multi stream capable.
+	 */
+	if (num_devices == 0)
+		return 0;
+
+	/* Behavior of setting index being equal to or greater than
+	 * Device List Length is not predictable
+	 */
+	if (num_devices + 1 <= dev_id)
+		return 0;
+
+	timeout = jiffies + msecs_to_jiffies(500);
+
+	/* make sure the device entry selection takes effect */
+	do {
+		ret = snd_hda_codec_write(codec, nid, 0,
+			AC_VERB_SET_DEVICE_SEL, dev_id);
+		udelay(20);
+
+		dev = snd_hda_get_dev_select(codec, nid);
+		if (dev == dev_id)
+			break;
+		msleep(20);
+	} while (time_before(jiffies, timeout));
+
+	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 4852c1a..57d1659 100644
--- a/sound/pci/hda/hda_codec.h
+++ b/sound/pci/hda/hda_codec.h
@@ -348,8 +348,11 @@ int snd_hda_override_conn_list(struct hda_codec *codec, hda_nid_t nid,
 			       int dev_id, 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 dev_id, 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

[RFC PATCH v2 2/3] ALSA - hda: add devid support in acom

[libin.yang]

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

This is a tmp API patch for gfx. Detail is still in discussion
---
 drivers/gpu/drm/i915/intel_audio.c | 2 +-
 include/drm/i915_component.h       | 2 +-
 include/sound/hda_i915.h           | 6 +++---
 sound/hda/hdac_i915.c              | 7 ++++---
 sound/pci/hda/patch_hdmi.c         | 5 +++--
 5 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
index 31f6d21..3fdb6d9 100644
--- a/drivers/gpu/drm/i915/intel_audio.c
+++ b/drivers/gpu/drm/i915/intel_audio.c
@@ -710,7 +710,7 @@ static int i915_audio_component_sync_audio_rate(struct device *dev,
 }
 
 static int i915_audio_component_get_eld(struct device *dev, int port,
-					bool *enabled,
+					int dev_id, bool *enabled,
 					unsigned char *buf, int max_bytes)
 {
 	struct drm_i915_private *dev_priv = dev_to_i915(dev);
diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
index b46fa0e..3c4475a 100644
--- a/include/drm/i915_component.h
+++ b/include/drm/i915_component.h
@@ -77,7 +77,7 @@ struct i915_audio_component_ops {
 	 * Note that the returned size may be over @max_bytes.  Then it
 	 * implies that only a part of ELD has been copied to the buffer.
 	 */
-	int (*get_eld)(struct device *, int port, bool *enabled,
+	int (*get_eld)(struct device *, int port, int dev_id, bool *enabled,
 		       unsigned char *buf, int max_bytes);
 };
 
diff --git a/include/sound/hda_i915.h b/include/sound/hda_i915.h
index fa341fc..07403f7 100644
--- a/include/sound/hda_i915.h
+++ b/include/sound/hda_i915.h
@@ -11,7 +11,7 @@ int snd_hdac_set_codec_wakeup(struct hdac_bus *bus, bool enable);
 int snd_hdac_display_power(struct hdac_bus *bus, bool enable);
 int snd_hdac_get_display_clk(struct hdac_bus *bus);
 int snd_hdac_sync_audio_rate(struct hdac_bus *bus, hda_nid_t nid, int rate);
-int snd_hdac_acomp_get_eld(struct hdac_bus *bus, hda_nid_t nid,
+int snd_hdac_acomp_get_eld(struct hdac_bus *bus, hda_nid_t nid, int dev_id,
 			   bool *audio_enabled, char *buffer, int max_bytes);
 int snd_hdac_i915_init(struct hdac_bus *bus);
 int snd_hdac_i915_exit(struct hdac_bus *bus);
@@ -35,8 +35,8 @@ static inline int snd_hdac_sync_audio_rate(struct hdac_bus *bus, hda_nid_t nid,
 	return 0;
 }
 static inline int snd_hdac_acomp_get_eld(struct hdac_bus *bus, hda_nid_t nid,
-					 bool *audio_enabled, char *buffer,
-					 int max_bytes)
+					 int dev_id, bool *audio_enabled,
+					 char *buffer, int max_bytes)
 {
 	return -ENODEV;
 }
diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c
index f6854db..18173c7 100644
--- a/sound/hda/hdac_i915.c
+++ b/sound/hda/hdac_i915.c
@@ -155,6 +155,7 @@ EXPORT_SYMBOL_GPL(snd_hdac_sync_audio_rate);
  * snd_hdac_acomp_get_eld - Get the audio state and ELD via component
  * @bus: HDA core bus
  * @nid: the pin widget NID
+ * @dev_id: the device entry id
  * @audio_enabled: the pointer to store the current audio state
  * @buffer: the buffer pointer to store ELD bytes
  * @max_bytes: the max bytes to be stored on @buffer
@@ -171,7 +172,7 @@ EXPORT_SYMBOL_GPL(snd_hdac_sync_audio_rate);
  * thus it may be over @max_bytes.  If it's over @max_bytes, it implies
  * that only a part of ELD bytes have been fetched.
  */
-int snd_hdac_acomp_get_eld(struct hdac_bus *bus, hda_nid_t nid,
+int snd_hdac_acomp_get_eld(struct hdac_bus *bus, hda_nid_t nid, int dev_id,
 			   bool *audio_enabled, char *buffer, int max_bytes)
 {
 	struct i915_audio_component *acomp = bus->audio_component;
@@ -179,8 +180,8 @@ int snd_hdac_acomp_get_eld(struct hdac_bus *bus, hda_nid_t nid,
 	if (!acomp || !acomp->ops || !acomp->ops->get_eld)
 		return -ENODEV;
 
-	return acomp->ops->get_eld(acomp->dev, pin2port(nid), audio_enabled,
-				   buffer, max_bytes);
+	return acomp->ops->get_eld(acomp->dev, pin2port(nid), dev_id,
+				   audio_enabled, buffer, max_bytes);
 }
 EXPORT_SYMBOL_GPL(snd_hdac_acomp_get_eld);
 
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index 7e03200..5987a31 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -75,6 +75,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;
@@ -2013,8 +2014,8 @@ static void sync_eld_via_acomp(struct hda_codec *codec,
 
 	mutex_lock(&per_pin->lock);
 	size = snd_hdac_acomp_get_eld(&codec->bus->core, per_pin->pin_nid,
-				      &eld->monitor_present, eld->eld_buffer,
-				      ELD_MAX_SIZE);
+				      per_pin->dev_id, &eld->monitor_present,
+				      eld->eld_buffer, ELD_MAX_SIZE);
 	if (size < 0)
 		goto unlock;
 	if (size > 0) {
-- 
1.9.1

[RFC PATCH v2 3/3] ALSA - hda: add DP MST support

[libin.yang]

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

This patch adds the DP MST support in hdmi audio driver.
---
 sound/pci/hda/hda_codec.c  |   3 +
 sound/pci/hda/patch_hdmi.c | 179 ++++++++++++++++++++++++++++++++-------------
 2 files changed, 133 insertions(+), 49 deletions(-)

diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index db94a66..d4be769 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -492,6 +492,9 @@ 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 5987a31..67e191c 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -144,7 +144,20 @@ 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;
@@ -1248,6 +1261,10 @@ static void hdmi_setup_audio_infoframe(struct hda_codec *codec,
 
 static bool hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll);
 
+/* todo:
+ * To fully support DP MST, check_presence_and_report need param dev_id
+ * to tell which device entry occurs monitor connection event
+ */
 static void check_presence_and_report(struct hda_codec *codec, hda_nid_t nid)
 {
 	struct hdmi_spec *spec = codec->spec;
@@ -1271,6 +1288,11 @@ 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;
@@ -1499,28 +1521,42 @@ 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;
 
-		if (wid_type != AC_WID_PIN)
+		per_pin = get_pin(spec, pin_idx);
+		/* pin not connected to monitor
+		 * no need to operate on it
+		 */
+		if (!per_pin->pcm)
 			continue;
 
-		if (nid == pin_nid)
+		if ((per_pin->pin_nid == pin_nid) &&
+			(per_pin->dev_id == dev_id))
 			continue;
 
+		nid = per_pin->pin_nid;
+
+		/* save the dev id for echo 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.
@@ -1537,12 +1573,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;
@@ -1557,7 +1594,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);
 }
 
 /* called in hdmi_pcm_open when no pin is assigned to the PCM
@@ -1585,7 +1622,7 @@ static int hdmi_pcm_open_no_pin(struct hda_pcm_stream *hinfo,
 	per_cvt->assigned = 1;
 	hinfo->nid = per_cvt->cvt_nid;
 
-	intel_not_share_assigned_cvt_nid(codec, 0, per_cvt->cvt_nid);
+	intel_not_share_assigned_cvt_nid(codec, 0, 0, per_cvt->cvt_nid);
 
 	set_bit(pcm_idx, &spec->pcm_in_use);
 	/* todo: setup spdif ctls assign */
@@ -1661,13 +1698,15 @@ 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,
 			    mux_idx);
 
 	/* configure unused pins to choose other converters */
 	if (is_haswell_plus(codec) || is_valleyview_plus(codec))
-		intel_not_share_assigned_cvt(codec, per_pin->pin_nid, mux_idx);
+		intel_not_share_assigned_cvt(codec, per_pin->pin_nid,
+					     per_pin->dev_id, mux_idx);
 
 	snd_hda_spdif_ctls_assign(codec, pcm_idx, per_cvt->cvt_nid);
 
@@ -1737,13 +1776,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;
 	}
@@ -1818,10 +1857,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);
@@ -1902,7 +1944,7 @@ static void update_eld(struct hda_codec *codec,
 		if (is_haswell_plus(codec) || is_valleyview_plus(codec)) {
 			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);
 		}
 
 		hdmi_setup_audio_infoframe(codec, per_pin, per_pin->non_pcm);
@@ -1996,6 +2038,7 @@ 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 not support mst
 		jack_tbl = snd_hda_jack_tbl_get(codec, per_pin->pin_nid);
 		if (jack_tbl)
 			jack = jack_tbl->jack;
@@ -2013,6 +2056,9 @@ static void sync_eld_via_acomp(struct hda_codec *codec,
 	int size;
 
 	mutex_lock(&per_pin->lock);
+	/* todo:
+	 * to fully support DP MST, need add param dev_id
+	 */
 	size = snd_hdac_acomp_get_eld(&codec->bus->core, per_pin->pin_nid,
 				      per_pin->dev_id, &eld->monitor_present,
 				      eld->eld_buffer, ELD_MAX_SIZE);
@@ -2079,7 +2125,7 @@ static void hdmi_repoll_eld(struct work_struct *work)
 }
 
 static void intel_haswell_fixup_connect_list(struct hda_codec *codec,
-					     hda_nid_t nid);
+					     hda_nid_t nid, int dev_id);
 
 static int hdmi_add_pin(struct hda_codec *codec, hda_nid_t pin_nid)
 {
@@ -2088,38 +2134,59 @@ 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;
+	if (is_haswell_plus(codec)) {
+		dev_num = 3;
+		spec->dev_num = 3;
+	} else {
+		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;
 	}
-	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);
+
+		if (!per_pin)
+			return -ENOMEM;
 
-	spec->num_pins++;
+		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, i);
+		//need check here
+		err = hdmi_read_pin_conn(codec, pin_idx);
+		if (err < 0)
+			return err;
+		spec->num_pins++;
+	}
+	spec->num_nids++;
 
 	return 0;
 }
@@ -2235,7 +2302,7 @@ static int generic_hdmi_playback_pcm_prepare(struct hda_pcm_stream *hinfo,
 		 * skip pin setup and return 0 to make audio playback
 		 * be ongoing
 		 */
-		intel_not_share_assigned_cvt_nid(codec, 0, cvt_nid);
+		intel_not_share_assigned_cvt_nid(codec, 0, 0, cvt_nid);
 		snd_hda_codec_setup_stream(codec, cvt_nid,
 					stream_tag, 0, format);
 		mutex_unlock(&spec->pcm_lock);
@@ -2257,8 +2324,10 @@ static int generic_hdmi_playback_pcm_prepare(struct hda_pcm_stream *hinfo,
 		 * which can cause a resumed audio playback become silent
 		 * after S3.
 		 */
+		snd_hda_set_dev_select(codec, pin_nid, per_pin->dev_id);
 		intel_verify_pin_cvt_connect(codec, per_pin);
-		intel_not_share_assigned_cvt(codec, pin_nid, per_pin->mux_idx);
+		intel_not_share_assigned_cvt(codec, pin_nid, per_pin->dev_id,
+				     per_pin->mux_idx);
 	}
 
 	/* Call sync_audio_rate to set the N/CTS/M manually if necessary */
@@ -2280,6 +2349,7 @@ static int generic_hdmi_playback_pcm_prepare(struct hda_pcm_stream *hinfo,
 				    pinctl | PIN_OUT);
 	}
 
+	//need call snd_hda_set_dev_select()???
 	err = spec->ops.setup_stream(codec, cvt_nid, pin_nid,
 				 stream_tag, format);
 	mutex_unlock(&spec->pcm_lock);
@@ -2533,17 +2603,22 @@ static int hdmi_chmap_ctl_put(struct snd_kcontrol *kcontrol,
 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;
@@ -2551,6 +2626,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 */
 	}
 
@@ -2720,7 +2798,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,
@@ -2813,21 +2893,21 @@ static const struct hdmi_ops generic_standard_hdmi_ops = {
 
 
 static void intel_haswell_fixup_connect_list(struct hda_codec *codec,
-					     hda_nid_t nid)
+					     hda_nid_t nid, int dev_id)
 {
 	struct hdmi_spec *spec = codec->spec;
 	hda_nid_t conns[4];
 	int nconns;
 
-	nconns = snd_hda_get_connections(codec, nid, 0, conns,
-					 ARRAY_SIZE(conns));
+	nconns = snd_hda_get_connections(codec, nid, dev_id,
+					 conns, ARRAY_SIZE(conns));
 	if (nconns == spec->num_cvts &&
 	    !memcmp(conns, spec->cvt_nids, spec->num_cvts * sizeof(hda_nid_t)))
 		return;
 
 	/* override pins connection list */
 	codec_dbg(codec, "hdmi: haswell: override pin connection 0x%x\n", nid);
-	snd_hda_override_conn_list(codec, nid, 0, spec->num_cvts,
+	snd_hda_override_conn_list(codec, nid, dev_id, spec->num_cvts,
 				   spec->cvt_nids);
 }
 
@@ -2914,6 +2994,7 @@ static int patch_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);
 	codec->spec = spec;
 	hdmi_array_init(spec, 4);
@@ -2927,6 +3008,8 @@ static int patch_generic_hdmi(struct hda_codec *codec)
 	if (is_haswell_plus(codec)) {
 		intel_haswell_enable_all_pins(codec, true);
 		intel_haswell_fixup_enable_dp12(codec);
+		codec->dp_mst = true;
+		spec->dyn_pcm_assign = true;
 	}
 
 	/* For Valleyview/Cherryview, only the display codec is in the display
@@ -2947,10 +3030,8 @@ static int patch_generic_hdmi(struct hda_codec *codec)
 		return -EINVAL;
 	}
 	codec->patch_ops = generic_hdmi_patch_ops;
-	if (is_haswell_plus(codec)) {
+	if (is_haswell_plus(codec))
 		codec->patch_ops.set_power_state = haswell_set_power_state;
-		codec->dp_mst = true;
-	}
 
 	/* Enable runtime pm for HDMI audio codec of HSW/BDW/SKL/BYT/BSW */
 	if (is_haswell_plus(codec) || is_valleyview_plus(codec))
-- 
1.9.1

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

[Takashi Iwai]

On Mon, 21 Mar 2016 09:37:54 +0100,
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 | 83 ++++++++++++++++++++++++++++++++++++++++++++---
>  sound/pci/hda/hda_codec.h |  3 ++
>  2 files changed, 82 insertions(+), 4 deletions(-)
> 
> diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> index d17e38e..db94a66 100644
> --- a/sound/pci/hda/hda_codec.c
> +++ b/sound/pci/hda/hda_codec.c
> @@ -325,9 +325,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;
> @@ -341,6 +348,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
> @@ -358,7 +366,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;
>  
> @@ -382,6 +390,73 @@ 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, dev, num_devices;
> +	unsigned long timeout;
> +
> +	/* 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);
> +	/* If Device List Length is 0, the pin is not multi stream capable.
> +	 */
> +	if (num_devices == 0)
> +		return 0;
> +
> +	/* Behavior of setting index being equal to or greater than
> +	 * Device List Length is not predictable
> +	 */
> +	if (num_devices + 1 <= dev_id)
> +		return 0;

Is this correct?  Doesn't num_devices=1 mean that there is only a
single device?  If dev_id=1 must be invalid, but the check above
passes.  The standard form of such index check is like

	if (dev_id >= num_devices)
		error;

And this also leads to the question.  Shouldn't we return an error if
an invalid dev_id is given?

> +
> +	timeout = jiffies + msecs_to_jiffies(500);
> +
> +	/* make sure the device entry selection takes effect */
> +	do {
> +		ret = snd_hda_codec_write(codec, nid, 0,
> +			AC_VERB_SET_DEVICE_SEL, dev_id);
> +		udelay(20);
> +
> +		dev = snd_hda_get_dev_select(codec, nid);
> +		if (dev == dev_id)
> +			break;
> +		msleep(20);
> +	} while (time_before(jiffies, timeout));

Is this loop needed?  Most verbs take effect immediately, and so far,
the only exception is the power state.


Takashi

Re: [RFC PATCH v2 3/3] ALSA - hda: add DP MST support

[Takashi Iwai]

On Mon, 21 Mar 2016 09:37:56 +0100,
libin.yang@linux.intel.com wrote:
> 
> From: Libin Yang <libin.yang@linux.intel.com>
> 
> This patch adds the DP MST support in hdmi audio driver.
> ---
>  sound/pci/hda/hda_codec.c  |   3 +
>  sound/pci/hda/patch_hdmi.c | 179 ++++++++++++++++++++++++++++++++-------------
>  2 files changed, 133 insertions(+), 49 deletions(-)
> 
> diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> index db94a66..d4be769 100644
> --- a/sound/pci/hda/hda_codec.c
> +++ b/sound/pci/hda/hda_codec.c
> @@ -492,6 +492,9 @@ 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 5987a31..67e191c 100644
> --- a/sound/pci/hda/patch_hdmi.c
> +++ b/sound/pci/hda/patch_hdmi.c
> @@ -144,7 +144,20 @@ 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;
> @@ -1248,6 +1261,10 @@ static void hdmi_setup_audio_infoframe(struct hda_codec *codec,
>  
>  static bool hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll);
>  
> +/* todo:
> + * To fully support DP MST, check_presence_and_report need param dev_id
> + * to tell which device entry occurs monitor connection event
> + */
>  static void check_presence_and_report(struct hda_codec *codec, hda_nid_t nid)
>  {
>  	struct hdmi_spec *spec = codec->spec;
> @@ -1271,6 +1288,11 @@ 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;
> @@ -1499,28 +1521,42 @@ 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++) {

Is the condition correct?  i.e. pin_idx = spec->num_pins is valid?


> +		int dev_id_saved;
>  
> -		if (wid_type != AC_WID_PIN)
> +		per_pin = get_pin(spec, pin_idx);
> +		/* pin not connected to monitor
> +		 * no need to operate on it
> +		 */
> +		if (!per_pin->pcm)
>  			continue;
>  
> -		if (nid == pin_nid)
> +		if ((per_pin->pin_nid == pin_nid) &&
> +			(per_pin->dev_id == dev_id))
>  			continue;
>  
> +		nid = per_pin->pin_nid;
> +
> +		/* save the dev id for echo 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.
> @@ -1537,12 +1573,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;
> @@ -1557,7 +1594,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);
>  }
>  
>  /* called in hdmi_pcm_open when no pin is assigned to the PCM
> @@ -1585,7 +1622,7 @@ static int hdmi_pcm_open_no_pin(struct hda_pcm_stream *hinfo,
>  	per_cvt->assigned = 1;
>  	hinfo->nid = per_cvt->cvt_nid;
>  
> -	intel_not_share_assigned_cvt_nid(codec, 0, per_cvt->cvt_nid);
> +	intel_not_share_assigned_cvt_nid(codec, 0, 0, per_cvt->cvt_nid);
>  
>  	set_bit(pcm_idx, &spec->pcm_in_use);
>  	/* todo: setup spdif ctls assign */
> @@ -1661,13 +1698,15 @@ 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,
>  			    mux_idx);
>  
>  	/* configure unused pins to choose other converters */
>  	if (is_haswell_plus(codec) || is_valleyview_plus(codec))
> -		intel_not_share_assigned_cvt(codec, per_pin->pin_nid, mux_idx);
> +		intel_not_share_assigned_cvt(codec, per_pin->pin_nid,
> +					     per_pin->dev_id, mux_idx);
>  
>  	snd_hda_spdif_ctls_assign(codec, pcm_idx, per_cvt->cvt_nid);
>  
> @@ -1737,13 +1776,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;
>  	}
> @@ -1818,10 +1857,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);
> @@ -1902,7 +1944,7 @@ static void update_eld(struct hda_codec *codec,
>  		if (is_haswell_plus(codec) || is_valleyview_plus(codec)) {
>  			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);
>  		}
>  
>  		hdmi_setup_audio_infoframe(codec, per_pin, per_pin->non_pcm);
> @@ -1996,6 +2038,7 @@ 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 not support mst
>  		jack_tbl = snd_hda_jack_tbl_get(codec, per_pin->pin_nid);
>  		if (jack_tbl)
>  			jack = jack_tbl->jack;
> @@ -2013,6 +2056,9 @@ static void sync_eld_via_acomp(struct hda_codec *codec,
>  	int size;
>  
>  	mutex_lock(&per_pin->lock);
> +	/* todo:
> +	 * to fully support DP MST, need add param dev_id
> +	 */
>  	size = snd_hdac_acomp_get_eld(&codec->bus->core, per_pin->pin_nid,
>  				      per_pin->dev_id, &eld->monitor_present,
>  				      eld->eld_buffer, ELD_MAX_SIZE);
> @@ -2079,7 +2125,7 @@ static void hdmi_repoll_eld(struct work_struct *work)
>  }
>  
>  static void intel_haswell_fixup_connect_list(struct hda_codec *codec,
> -					     hda_nid_t nid);
> +					     hda_nid_t nid, int dev_id);
>  
>  static int hdmi_add_pin(struct hda_codec *codec, hda_nid_t pin_nid)
>  {
> @@ -2088,38 +2134,59 @@ 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;
> +	if (is_haswell_plus(codec)) {
> +		dev_num = 3;
> +		spec->dev_num = 3;
> +	} else {
> +		dev_num = snd_hda_get_num_devices(codec, pin_nid) + 1;

Does snd_hda_get_num_devices() return 0?  What if it's no mst-capable
codec, and what if a MST-capable codec having a single device?


> +		/* 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;
>  	}

The presence of is_haswell_*() or such macro in every place makes
really hard to maintain the code.  In my new patchset (I'll post soon
later), I tried to reduce these usages, and split the functions to
Intel-specific ones.  Let's try not to contaminate too much again.


Takashi

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

[Yang, Libin]

Hi Takashi,

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Tuesday, March 22, 2016 7:17 PM
> To: libin.yang@linux.intel.com
> Cc: alsa-devel@alsa-project.org; Lin, Mengdong; Yang, Libin
> Subject: Re: [alsa-devel] [RFC PATCH v2 1/3] ALSA: hda - add DP mst verb
> support
> 
> On Mon, 21 Mar 2016 09:37:54 +0100,
> 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 | 83
> ++++++++++++++++++++++++++++++++++++++++++++---
> >  sound/pci/hda/hda_codec.h |  3 ++
> >  2 files changed, 82 insertions(+), 4 deletions(-)
> >
> > diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> > index d17e38e..db94a66 100644
> > --- a/sound/pci/hda/hda_codec.c
> > +++ b/sound/pci/hda/hda_codec.c
> > @@ -325,9 +325,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;
> > @@ -341,6 +348,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
> > @@ -358,7 +366,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;
> >
> > @@ -382,6 +390,73 @@ 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, dev, num_devices;
> > +	unsigned long timeout;
> > +
> > +	/* 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);
> > +	/* If Device List Length is 0, the pin is not multi stream capable.
> > +	 */
> > +	if (num_devices == 0)
> > +		return 0;
> > +
> > +	/* Behavior of setting index being equal to or greater than
> > +	 * Device List Length is not predictable
> > +	 */
> > +	if (num_devices + 1 <= dev_id)
> > +		return 0;
> 
> Is this correct?  Doesn't num_devices=1 mean that there is only a
> single device?  If dev_id=1 must be invalid, but the check above
> passes.  The standard form of such index check is like

num_device is the return value of the verb AC_PAR_DEVLIST_LEN.
device number is AC_PAR_DEVLIST_LEN + 1. 

>From the spec:
Device List Length is a 0 based integer value indicating the number
 of sink device that a multi stream capable Digital Display Pin 
Widget can support. If Device List Length is value is 0, there is 
only one sink device connection possible indicating the Pin Widget
 is not multi stream capable, and there is no Device Select control

And dev_id is 0 based.

If dev_id = 1, it means set the second device entry, and
num_device + 1 (the real number of device entries)
must be equal to or greater than 2. 
(num_device + 1 > dev_id )

So (num_device + 1 <= dev_id) is wrong situation.

> 
> 	if (dev_id >= num_devices)
> 		error;
> 
> And this also leads to the question.  Shouldn't we return an error if
> an invalid dev_id is given?

I'm thinking if the wrong dev_id is given, we just leave as it is before.
I will return the -EINVAL in next version.

> 
> > +
> > +	timeout = jiffies + msecs_to_jiffies(500);
> > +
> > +	/* make sure the device entry selection takes effect */
> > +	do {
> > +		ret = snd_hda_codec_write(codec, nid, 0,
> > +			AC_VERB_SET_DEVICE_SEL, dev_id);
> > +		udelay(20);
> > +
> > +		dev = snd_hda_get_dev_select(codec, nid);
> > +		if (dev == dev_id)
> > +			break;
> > +		msleep(20);
> > +	} while (time_before(jiffies, timeout));
> 
> Is this loop needed?  Most verbs take effect immediately, and so far,
> the only exception is the power state.

OK, I will remove the loop.

Regards,
Libin

> 
> 
> Takashi

Re: [RFC PATCH v2 3/3] ALSA - hda: add DP MST support

[Yang, Libin]

Hi Takashi,

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Tuesday, March 22, 2016 7:28 PM
> To: libin.yang@linux.intel.com
> Cc: alsa-devel@alsa-project.org; Lin, Mengdong; Yang, Libin
> Subject: Re: [alsa-devel] [RFC PATCH v2 3/3] ALSA - hda: add DP MST
> support
> 
> On Mon, 21 Mar 2016 09:37:56 +0100,
> libin.yang@linux.intel.com wrote:
> >
> > From: Libin Yang <libin.yang@linux.intel.com>
> >
> > This patch adds the DP MST support in hdmi audio driver.
> > ---
> >  sound/pci/hda/hda_codec.c  |   3 +
> >  sound/pci/hda/patch_hdmi.c | 179
> ++++++++++++++++++++++++++++++++-------------
> >  2 files changed, 133 insertions(+), 49 deletions(-)
> >
> > diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> > index db94a66..d4be769 100644
> > --- a/sound/pci/hda/hda_codec.c
> > +++ b/sound/pci/hda/hda_codec.c
> > @@ -492,6 +492,9 @@ 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 5987a31..67e191c 100644
> > --- a/sound/pci/hda/patch_hdmi.c
> > +++ b/sound/pci/hda/patch_hdmi.c
> > @@ -144,7 +144,20 @@ 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;
> > @@ -1248,6 +1261,10 @@ static void
> hdmi_setup_audio_infoframe(struct hda_codec *codec,
> >
> >  static bool hdmi_present_sense(struct hdmi_spec_per_pin *per_pin,
> int repoll);
> >
> > +/* todo:
> > + * To fully support DP MST, check_presence_and_report need param
> dev_id
> > + * to tell which device entry occurs monitor connection event
> > + */
> >  static void check_presence_and_report(struct hda_codec *codec,
> hda_nid_t nid)
> >  {
> >  	struct hdmi_spec *spec = codec->spec;
> > @@ -1271,6 +1288,11 @@ 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;
> > @@ -1499,28 +1521,42 @@ 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++) {
> 
> Is the condition correct?  i.e. pin_idx = spec->num_pins is valid?

You are right. My code is wrong.

> 
> 
> > +		int dev_id_saved;
> >
> > -		if (wid_type != AC_WID_PIN)
> > +		per_pin = get_pin(spec, pin_idx);
> > +		/* pin not connected to monitor
> > +		 * no need to operate on it
> > +		 */
> > +		if (!per_pin->pcm)
> >  			continue;
> >
> > -		if (nid == pin_nid)
> > +		if ((per_pin->pin_nid == pin_nid) &&
> > +			(per_pin->dev_id == dev_id))
> >  			continue;
> >
> > +		nid = per_pin->pin_nid;
> > +
> > +		/* save the dev id for echo 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.
> > @@ -1537,12 +1573,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;
> > @@ -1557,7 +1594,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);
> >  }
> >
> >  /* called in hdmi_pcm_open when no pin is assigned to the PCM
> > @@ -1585,7 +1622,7 @@ static int hdmi_pcm_open_no_pin(struct
> hda_pcm_stream *hinfo,
> >  	per_cvt->assigned = 1;
> >  	hinfo->nid = per_cvt->cvt_nid;
> >
> > -	intel_not_share_assigned_cvt_nid(codec, 0, per_cvt->cvt_nid);
> > +	intel_not_share_assigned_cvt_nid(codec, 0, 0, per_cvt->cvt_nid);
> >
> >  	set_bit(pcm_idx, &spec->pcm_in_use);
> >  	/* todo: setup spdif ctls assign */
> > @@ -1661,13 +1698,15 @@ 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,
> >  			    mux_idx);
> >
> >  	/* configure unused pins to choose other converters */
> >  	if (is_haswell_plus(codec) || is_valleyview_plus(codec))
> > -		intel_not_share_assigned_cvt(codec, per_pin->pin_nid,
> mux_idx);
> > +		intel_not_share_assigned_cvt(codec, per_pin->pin_nid,
> > +					     per_pin->dev_id, mux_idx);
> >
> >  	snd_hda_spdif_ctls_assign(codec, pcm_idx, per_cvt->cvt_nid);
> >
> > @@ -1737,13 +1776,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;
> >  	}
> > @@ -1818,10 +1857,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);
> > @@ -1902,7 +1944,7 @@ static void update_eld(struct hda_codec
> *codec,
> >  		if (is_haswell_plus(codec) || is_valleyview_plus(codec)) {
> >  			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);
> >  		}
> >
> >  		hdmi_setup_audio_infoframe(codec, per_pin, per_pin-
> >non_pcm);
> > @@ -1996,6 +2038,7 @@ 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 not support mst
> >  		jack_tbl = snd_hda_jack_tbl_get(codec, per_pin-
> >pin_nid);
> >  		if (jack_tbl)
> >  			jack = jack_tbl->jack;
> > @@ -2013,6 +2056,9 @@ static void sync_eld_via_acomp(struct
> hda_codec *codec,
> >  	int size;
> >
> >  	mutex_lock(&per_pin->lock);
> > +	/* todo:
> > +	 * to fully support DP MST, need add param dev_id
> > +	 */
> >  	size = snd_hdac_acomp_get_eld(&codec->bus->core, per_pin-
> >pin_nid,
> >  				      per_pin->dev_id, &eld-
> >monitor_present,
> >  				      eld->eld_buffer, ELD_MAX_SIZE);
> > @@ -2079,7 +2125,7 @@ static void hdmi_repoll_eld(struct
> work_struct *work)
> >  }
> >
> >  static void intel_haswell_fixup_connect_list(struct hda_codec *codec,
> > -					     hda_nid_t nid);
> > +					     hda_nid_t nid, int dev_id);
> >
> >  static int hdmi_add_pin(struct hda_codec *codec, hda_nid_t pin_nid)
> >  {
> > @@ -2088,38 +2134,59 @@ 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;
> > +	if (is_haswell_plus(codec)) {
> > +		dev_num = 3;
> > +		spec->dev_num = 3;
> > +	} else {
> > +		dev_num = snd_hda_get_num_devices(codec, pin_nid)
> + 1;
> 
> Does snd_hda_get_num_devices() return 0?  What if it's no mst-capable
> codec, and what if a MST-capable codec having a single device?

If it is no mst-capable, it will return 0. It means only one device is support.
Based on my testing, if I connect MST hub on the port, and connect more
than 1 monitor on the hub, the value will be 2, which means it supports
3 device entries. Otherwise, it will return 0. I'm not sure this is the same
on all other platforms. So I only use the workaround on HSW+.

> 
> 
> > +		/* 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;
> >  	}
> 
> The presence of is_haswell_*() or such macro in every place makes
> really hard to maintain the code.  In my new patchset (I'll post soon
> later), I tried to reduce these usages, and split the functions to
> Intel-specific ones.  Let's try not to contaminate too much again.

I see. I will modify the code based on your change.

Regards,
Libin

> 
> 
> Takashi

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

[Takashi Iwai]

On Thu, 24 Mar 2016 09:44:32 +0100,
Yang, Libin wrote:
> 
> Hi Takashi,
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai@suse.de]
> > Sent: Tuesday, March 22, 2016 7:17 PM
> > To: libin.yang@linux.intel.com
> > Cc: alsa-devel@alsa-project.org; Lin, Mengdong; Yang, Libin
> > Subject: Re: [alsa-devel] [RFC PATCH v2 1/3] ALSA: hda - add DP mst verb
> > support
> > 
> > On Mon, 21 Mar 2016 09:37:54 +0100,
> > 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 | 83
> > ++++++++++++++++++++++++++++++++++++++++++++---
> > >  sound/pci/hda/hda_codec.h |  3 ++
> > >  2 files changed, 82 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> > > index d17e38e..db94a66 100644
> > > --- a/sound/pci/hda/hda_codec.c
> > > +++ b/sound/pci/hda/hda_codec.c
> > > @@ -325,9 +325,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;
> > > @@ -341,6 +348,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
> > > @@ -358,7 +366,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;
> > >
> > > @@ -382,6 +390,73 @@ 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, dev, num_devices;
> > > +	unsigned long timeout;
> > > +
> > > +	/* 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);
> > > +	/* If Device List Length is 0, the pin is not multi stream capable.
> > > +	 */
> > > +	if (num_devices == 0)
> > > +		return 0;
> > > +
> > > +	/* Behavior of setting index being equal to or greater than
> > > +	 * Device List Length is not predictable
> > > +	 */
> > > +	if (num_devices + 1 <= dev_id)
> > > +		return 0;
> > 
> > Is this correct?  Doesn't num_devices=1 mean that there is only a
> > single device?  If dev_id=1 must be invalid, but the check above
> > passes.  The standard form of such index check is like
> 
> num_device is the return value of the verb AC_PAR_DEVLIST_LEN.
> device number is AC_PAR_DEVLIST_LEN + 1. 
> 
> >From the spec:
> Device List Length is a 0 based integer value indicating the number
>  of sink device that a multi stream capable Digital Display Pin 
> Widget can support. If Device List Length is value is 0, there is 
> only one sink device connection possible indicating the Pin Widget
>  is not multi stream capable, and there is no Device Select control
> 
> And dev_id is 0 based.
> 
> If dev_id = 1, it means set the second device entry, and
> num_device + 1 (the real number of device entries)
> must be equal to or greater than 2. 
> (num_device + 1 > dev_id )
> 
> So (num_device + 1 <= dev_id) is wrong situation.

If so, the term "num_devices" is counter-intuitive.  It should be
corrected to the natural value, i.e. based on 1, or renamed to a
different one.  Readers would assume that the number of devices is 1
if there is a single element, not 0.  If this isn't true, it just
confuses readers.


> > 
> > 	if (dev_id >= num_devices)
> > 		error;
> > 
> > And this also leads to the question.  Shouldn't we return an error if
> > an invalid dev_id is given?
> 
> I'm thinking if the wrong dev_id is given, we just leave as it is before.
> I will return the -EINVAL in next version.

IMO, it's safer to return an error.  Since non-zero dev_id is only for
MST, the code path that passes such a value must handle MST properly,
i.e. not through the default code path.


> > > +
> > > +	timeout = jiffies + msecs_to_jiffies(500);
> > > +
> > > +	/* make sure the device entry selection takes effect */
> > > +	do {
> > > +		ret = snd_hda_codec_write(codec, nid, 0,
> > > +			AC_VERB_SET_DEVICE_SEL, dev_id);
> > > +		udelay(20);
> > > +
> > > +		dev = snd_hda_get_dev_select(codec, nid);
> > > +		if (dev == dev_id)
> > > +			break;
> > > +		msleep(20);
> > > +	} while (time_before(jiffies, timeout));
> > 
> > Is this loop needed?  Most verbs take effect immediately, and so far,
> > the only exception is the power state.
> 
> OK, I will remove the loop.

Well, I don't mean to remove forcibly.  Just want to be sure.
If such a loop is needed practically, we must have it, of course.
Let's check.


thanks,

Takashi

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

[Yang, Libin]

Hi Takashi,

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Thursday, March 24, 2016 5:19 PM
> To: Yang, Libin
> Cc: libin.yang@linux.intel.com; alsa-devel@alsa-project.org; Lin,
> Mengdong
> Subject: Re: [alsa-devel] [RFC PATCH v2 1/3] ALSA: hda - add DP mst verb
> support
> 
> On Thu, 24 Mar 2016 09:44:32 +0100,
> Yang, Libin wrote:
> >
> > Hi Takashi,
> >
> > > -----Original Message-----
> > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > Sent: Tuesday, March 22, 2016 7:17 PM
> > > To: libin.yang@linux.intel.com
> > > Cc: alsa-devel@alsa-project.org; Lin, Mengdong; Yang, Libin
> > > Subject: Re: [alsa-devel] [RFC PATCH v2 1/3] ALSA: hda - add DP mst
> verb
> > > support
> > >
> > > On Mon, 21 Mar 2016 09:37:54 +0100,
> > > 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 | 83
> > > ++++++++++++++++++++++++++++++++++++++++++++---
> > > >  sound/pci/hda/hda_codec.h |  3 ++
> > > >  2 files changed, 82 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/sound/pci/hda/hda_codec.c
> b/sound/pci/hda/hda_codec.c
> > > > index d17e38e..db94a66 100644
> > > > --- a/sound/pci/hda/hda_codec.c
> > > > +++ b/sound/pci/hda/hda_codec.c
> > > > @@ -325,9 +325,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;
> > > > @@ -341,6 +348,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
> > > > @@ -358,7 +366,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;
> > > >
> > > > @@ -382,6 +390,73 @@ 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, dev, num_devices;
> > > > +	unsigned long timeout;
> > > > +
> > > > +	/* 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);
> > > > +	/* If Device List Length is 0, the pin is not multi stream capable.
> > > > +	 */
> > > > +	if (num_devices == 0)
> > > > +		return 0;
> > > > +
> > > > +	/* Behavior of setting index being equal to or greater than
> > > > +	 * Device List Length is not predictable
> > > > +	 */
> > > > +	if (num_devices + 1 <= dev_id)
> > > > +		return 0;
> > >
> > > Is this correct?  Doesn't num_devices=1 mean that there is only a
> > > single device?  If dev_id=1 must be invalid, but the check above
> > > passes.  The standard form of such index check is like
> >
> > num_device is the return value of the verb AC_PAR_DEVLIST_LEN.
> > device number is AC_PAR_DEVLIST_LEN + 1.
> >
> > >From the spec:
> > Device List Length is a 0 based integer value indicating the number
> >  of sink device that a multi stream capable Digital Display Pin
> > Widget can support. If Device List Length is value is 0, there is
> > only one sink device connection possible indicating the Pin Widget
> >  is not multi stream capable, and there is no Device Select control
> >
> > And dev_id is 0 based.
> >
> > If dev_id = 1, it means set the second device entry, and
> > num_device + 1 (the real number of device entries)
> > must be equal to or greater than 2.
> > (num_device + 1 > dev_id )
> >
> > So (num_device + 1 <= dev_id) is wrong situation.
> 
> If so, the term "num_devices" is counter-intuitive.  It should be
> corrected to the natural value, i.e. based on 1, or renamed to a
> different one.  Readers would assume that the number of devices is 1
> if there is a single element, not 0.  If this isn't true, it just
> confuses readers.

Get it. So what do you think if using:

if (dev_id > num_device)  //both is 0 based.

Regards,
Libin

> 
> 
> > >
> > > 	if (dev_id >= num_devices)
> > > 		error;
> > >
> > > And this also leads to the question.  Shouldn't we return an error if
> > > an invalid dev_id is given?
> >
> > I'm thinking if the wrong dev_id is given, we just leave as it is before.
> > I will return the -EINVAL in next version.
> 
> IMO, it's safer to return an error.  Since non-zero dev_id is only for
> MST, the code path that passes such a value must handle MST properly,
> i.e. not through the default code path.
> 
> 
> > > > +
> > > > +	timeout = jiffies + msecs_to_jiffies(500);
> > > > +
> > > > +	/* make sure the device entry selection takes effect */
> > > > +	do {
> > > > +		ret = snd_hda_codec_write(codec, nid, 0,
> > > > +			AC_VERB_SET_DEVICE_SEL, dev_id);
> > > > +		udelay(20);
> > > > +
> > > > +		dev = snd_hda_get_dev_select(codec, nid);
> > > > +		if (dev == dev_id)
> > > > +			break;
> > > > +		msleep(20);
> > > > +	} while (time_before(jiffies, timeout));
> > >
> > > Is this loop needed?  Most verbs take effect immediately, and so far,
> > > the only exception is the power state.
> >
> > OK, I will remove the loop.
> 
> Well, I don't mean to remove forcibly.  Just want to be sure.
> If such a loop is needed practically, we must have it, of course.
> Let's check.
> 
> 
> thanks,
> 
> Takashi

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

[Takashi Iwai]

On Fri, 25 Mar 2016 03:26:06 +0100,
Yang, Libin wrote:
> 
> Hi Takashi,
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai@suse.de]
> > Sent: Thursday, March 24, 2016 5:19 PM
> > To: Yang, Libin
> > Cc: libin.yang@linux.intel.com; alsa-devel@alsa-project.org; Lin,
> > Mengdong
> > Subject: Re: [alsa-devel] [RFC PATCH v2 1/3] ALSA: hda - add DP mst verb
> > support
> > 
> > On Thu, 24 Mar 2016 09:44:32 +0100,
> > Yang, Libin wrote:
> > >
> > > Hi Takashi,
> > >
> > > > -----Original Message-----
> > > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > Sent: Tuesday, March 22, 2016 7:17 PM
> > > > To: libin.yang@linux.intel.com
> > > > Cc: alsa-devel@alsa-project.org; Lin, Mengdong; Yang, Libin
> > > > Subject: Re: [alsa-devel] [RFC PATCH v2 1/3] ALSA: hda - add DP mst
> > verb
> > > > support
> > > >
> > > > On Mon, 21 Mar 2016 09:37:54 +0100,
> > > > 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 | 83
> > > > ++++++++++++++++++++++++++++++++++++++++++++---
> > > > >  sound/pci/hda/hda_codec.h |  3 ++
> > > > >  2 files changed, 82 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/sound/pci/hda/hda_codec.c
> > b/sound/pci/hda/hda_codec.c
> > > > > index d17e38e..db94a66 100644
> > > > > --- a/sound/pci/hda/hda_codec.c
> > > > > +++ b/sound/pci/hda/hda_codec.c
> > > > > @@ -325,9 +325,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;
> > > > > @@ -341,6 +348,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
> > > > > @@ -358,7 +366,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;
> > > > >
> > > > > @@ -382,6 +390,73 @@ 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, dev, num_devices;
> > > > > +	unsigned long timeout;
> > > > > +
> > > > > +	/* 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);
> > > > > +	/* If Device List Length is 0, the pin is not multi stream capable.
> > > > > +	 */
> > > > > +	if (num_devices == 0)
> > > > > +		return 0;
> > > > > +
> > > > > +	/* Behavior of setting index being equal to or greater than
> > > > > +	 * Device List Length is not predictable
> > > > > +	 */
> > > > > +	if (num_devices + 1 <= dev_id)
> > > > > +		return 0;
> > > >
> > > > Is this correct?  Doesn't num_devices=1 mean that there is only a
> > > > single device?  If dev_id=1 must be invalid, but the check above
> > > > passes.  The standard form of such index check is like
> > >
> > > num_device is the return value of the verb AC_PAR_DEVLIST_LEN.
> > > device number is AC_PAR_DEVLIST_LEN + 1.
> > >
> > > >From the spec:
> > > Device List Length is a 0 based integer value indicating the number
> > >  of sink device that a multi stream capable Digital Display Pin
> > > Widget can support. If Device List Length is value is 0, there is
> > > only one sink device connection possible indicating the Pin Widget
> > >  is not multi stream capable, and there is no Device Select control
> > >
> > > And dev_id is 0 based.
> > >
> > > If dev_id = 1, it means set the second device entry, and
> > > num_device + 1 (the real number of device entries)
> > > must be equal to or greater than 2.
> > > (num_device + 1 > dev_id )
> > >
> > > So (num_device + 1 <= dev_id) is wrong situation.
> > 
> > If so, the term "num_devices" is counter-intuitive.  It should be
> > corrected to the natural value, i.e. based on 1, or renamed to a
> > different one.  Readers would assume that the number of devices is 1
> > if there is a single element, not 0.  If this isn't true, it just
> > confuses readers.
> 
> Get it. So what do you think if using:
> 
> if (dev_id > num_device)  //both is 0 based.

num_device is still a confusing name.  If you really want to keep it
zero-based, use max_device_id or such.

Or, just return +1 from *_get_num_devices() instead.


Takashi