[RFC PATCH 0/4] MST DP audio support

[RFC PATCH 0/4] MST DP audio support

[libin.yang]

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

The patchset are tmp patches. The API between audio and gfx is
still in discussion.

Libin Yang (4):
  ALSA: hda - codec add DP MST support for connection list
  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          | 217 ++++++++++++++++++++++++++++++++++---
 sound/pci/hda/hda_codec.h          |   8 ++
 sound/pci/hda/patch_hdmi.c         | 172 ++++++++++++++++++++---------
 7 files changed, 340 insertions(+), 74 deletions(-)

-- 
1.9.1

[RFC PATCH 1/4] ALSA: hda - codec add DP MST support for connection list

[libin.yang]

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

This patches adds the support of connection list for DP MST.
With this, hdmi driver in DP MST mode can easily reuse
the connection list mechanism.

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

diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index 8374188..d4c81f7 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -110,23 +110,24 @@ struct hda_conn_list {
 	struct list_head list;
 	int len;
 	hda_nid_t nid;
+	int dev_id;
 	hda_nid_t conns[0];
 };
 
 /* look up the cached results */
 static struct hda_conn_list *
-lookup_conn_list(struct hda_codec *codec, hda_nid_t nid)
+lookup_conn_list(struct hda_codec *codec, hda_nid_t nid, int dev_id)
 {
 	struct hda_conn_list *p;
 	list_for_each_entry(p, &codec->conn_list, list) {
-		if (p->nid == nid)
+		if ((p->nid == nid) && (p->dev_id == dev_id))
 			return p;
 	}
 	return NULL;
 }
 
-static int add_conn_list(struct hda_codec *codec, hda_nid_t nid, int len,
-			 const hda_nid_t *list)
+static int add_conn_list(struct hda_codec *codec, hda_nid_t nid,
+			 int dev_id, int len, const hda_nid_t *list)
 {
 	struct hda_conn_list *p;
 
@@ -135,6 +136,7 @@ static int add_conn_list(struct hda_codec *codec, hda_nid_t nid, int len,
 		return -ENOMEM;
 	p->len = len;
 	p->nid = nid;
+	p->dev_id = dev_id;
 	memcpy(p->conns, list, len * sizeof(hda_nid_t));
 	list_add(&p->list, &codec->conn_list);
 	return 0;
@@ -150,8 +152,13 @@ static void remove_conn_list(struct hda_codec *codec)
 	}
 }
 
-/* read the connection and add to the cache */
-static int read_and_add_raw_conns(struct hda_codec *codec, hda_nid_t nid)
+/*
+ * read the connection and add to the cache
+ * the caller should select the device entry by sending the
+ * corresponding verb if necessary before calling this function
+ */
+static int read_and_add_raw_conns(struct hda_codec *codec, hda_nid_t nid,
+				  int dev_id)
 {
 	hda_nid_t list[32];
 	hda_nid_t *result = list;
@@ -166,7 +173,8 @@ static int read_and_add_raw_conns(struct hda_codec *codec, hda_nid_t nid)
 		len = snd_hda_get_raw_connections(codec, nid, result, len);
 	}
 	if (len >= 0)
-		len = snd_hda_override_conn_list(codec, nid, len, result);
+		len = snd_hda_override_conn_list_mst(codec, nid,
+					     dev_id, len, result);
 	if (result != list)
 		kfree(result);
 	return len;
@@ -197,7 +205,7 @@ int snd_hda_get_conn_list(struct hda_codec *codec, hda_nid_t nid,
 		const struct hda_conn_list *p;
 
 		/* if the connection-list is already cached, read it */
-		p = lookup_conn_list(codec, nid);
+		p = lookup_conn_list(codec, nid, 0);
 		if (p) {
 			if (listp)
 				*listp = p->conns;
@@ -206,7 +214,7 @@ int snd_hda_get_conn_list(struct hda_codec *codec, hda_nid_t nid,
 		if (snd_BUG_ON(added))
 			return -EINVAL;
 
-		err = read_and_add_raw_conns(codec, nid);
+		err = read_and_add_raw_conns(codec, nid, 0);
 		if (err < 0)
 			return err;
 		added = true;
@@ -215,6 +223,49 @@ int snd_hda_get_conn_list(struct hda_codec *codec, hda_nid_t nid,
 EXPORT_SYMBOL_GPL(snd_hda_get_conn_list);
 
 /**
+ * snd_hda_get_conn_list_mst - get connection list in mst mode
+ * @codec: the HDA codec
+ * @nid: NID to parse
+ * @dev_id: device entry id
+ * @listp: the pointer to store NID list
+ *
+ * Parses the connection list of the given widget and stores the pointer
+ * to the list of NIDs.
+ *
+ * Returns the number of connections, or a negative error code.
+ *
+ * Note that the returned pointer isn't protected against the list
+ * modification.  If snd_hda_override_conn_list() might be called
+ * concurrently, protect with a mutex appropriately.
+ */
+int snd_hda_get_conn_list_mst(struct hda_codec *codec, hda_nid_t nid,
+			      int dev_id, const hda_nid_t **listp)
+{
+	bool added = false;
+
+	for (;;) {
+		int err;
+		const struct hda_conn_list *p;
+
+		/* if the connection-list is already cached, read it */
+		p = lookup_conn_list(codec, nid, dev_id);
+		if (p) {
+			if (listp)
+				*listp = p->conns;
+			return p->len;
+		}
+		if (snd_BUG_ON(added))
+			return -EINVAL;
+
+		err = read_and_add_raw_conns(codec, nid, dev_id);
+		if (err < 0)
+			return err;
+		added = true;
+	}
+}
+EXPORT_SYMBOL_GPL(snd_hda_get_conn_list_mst);
+
+/**
  * snd_hda_get_connections - copy connection list
  * @codec: the HDA codec
  * @nid: NID to parse
@@ -246,6 +297,39 @@ int snd_hda_get_connections(struct hda_codec *codec, hda_nid_t nid,
 EXPORT_SYMBOL_GPL(snd_hda_get_connections);
 
 /**
+ * snd_hda_get_connections_mst - copy connection list in mst mode
+ * @codec: the HDA codec
+ * @nid: NID to parse
+ * @dev_id: device entry id
+ * @conn_list: connection list array; when NULL, checks only the size
+ * @max_conns: max. number of connections to store
+ *
+ * Parses the connection list of the given widget and stores the list
+ * of NIDs.
+ *
+ * Returns the number of connections, or a negative error code.
+ */
+int snd_hda_get_connections_mst(struct hda_codec *codec, hda_nid_t nid,
+				int dev_id, hda_nid_t *conn_list,
+				int max_conns)
+{
+	const hda_nid_t *list;
+	int len = snd_hda_get_conn_list_mst(codec, nid, dev_id, &list);
+
+	if (len > 0 && conn_list) {
+		if (len > max_conns) {
+			codec_err(codec, "Too many connections %d for NID 0x%x\n",
+				   len, nid);
+			return -EINVAL;
+		}
+		memcpy(conn_list, list, len * sizeof(hda_nid_t));
+	}
+
+	return len;
+}
+EXPORT_SYMBOL_GPL(snd_hda_get_connections_mst);
+
+/**
  * snd_hda_override_conn_list - add/modify the connection-list to cache
  * @codec: the HDA codec
  * @nid: NID to parse
@@ -262,17 +346,45 @@ int snd_hda_override_conn_list(struct hda_codec *codec, hda_nid_t nid, int len,
 {
 	struct hda_conn_list *p;
 
-	p = lookup_conn_list(codec, nid);
+	p = lookup_conn_list(codec, nid, 0);
 	if (p) {
 		list_del(&p->list);
 		kfree(p);
 	}
 
-	return add_conn_list(codec, nid, len, list);
+	return add_conn_list(codec, nid, 0, len, list);
 }
 EXPORT_SYMBOL_GPL(snd_hda_override_conn_list);
 
 /**
+ * snd_hda_override_conn_list_mst - add/modify the connection-list to cache
+ * @codec: the HDA codec
+ * @nid: NID to parse
+ * @dev_id: device entry id
+ * @len: number of connection list entries
+ * @list: the list of connection entries
+ *
+ * Add or modify the given connection-list to the cache.  If the corresponding
+ * cache already exists, invalidate it and append a new one.
+ *
+ * Returns zero or a negative error code.
+ */
+int snd_hda_override_conn_list_mst(struct hda_codec *codec, hda_nid_t nid,
+			   int dev_id, int len, const hda_nid_t *list)
+{
+	struct hda_conn_list *p;
+
+	p = lookup_conn_list(codec, nid, dev_id);
+	if (p) {
+		list_del(&p->list);
+		kfree(p);
+	}
+
+	return add_conn_list(codec, nid, dev_id, len, list);
+}
+EXPORT_SYMBOL_GPL(snd_hda_override_conn_list_mst);
+
+/**
  * snd_hda_get_conn_index - get the connection index of the given NID
  * @codec: the HDA codec
  * @mux: NID containing the list
diff --git a/sound/pci/hda/hda_codec.h b/sound/pci/hda/hda_codec.h
index 373fcad..deeed35 100644
--- a/sound/pci/hda/hda_codec.h
+++ b/sound/pci/hda/hda_codec.h
@@ -330,6 +330,9 @@ snd_hda_codec_write(struct hda_codec *codec, hda_nid_t nid, int flags,
 	snd_hdac_get_sub_nodes(&(codec)->core, nid, start_nid)
 int snd_hda_get_connections(struct hda_codec *codec, hda_nid_t nid,
 			    hda_nid_t *conn_list, int max_conns);
+int snd_hda_get_connections_mst(struct hda_codec *codec, hda_nid_t nid,
+				int dev_id, hda_nid_t *conn_list,
+				int max_conns);
 static inline int
 snd_hda_get_num_conns(struct hda_codec *codec, hda_nid_t nid)
 {
@@ -345,6 +348,8 @@ int snd_hda_get_conn_list(struct hda_codec *codec, hda_nid_t nid,
 			  const hda_nid_t **listp);
 int snd_hda_override_conn_list(struct hda_codec *codec, hda_nid_t nid, int nums,
 			  const hda_nid_t *list);
+int snd_hda_override_conn_list_mst(struct hda_codec *codec, hda_nid_t nid,
+			   int dev_id, int len, 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);
 int snd_hda_get_devices(struct hda_codec *codec, hda_nid_t nid,
-- 
1.9.1

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

diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index d4c81f7..1a42c51 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -424,8 +424,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;
@@ -439,6 +447,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
@@ -456,7 +465,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;
 
@@ -480,6 +489,71 @@ 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;
+	unsigned long timeout;
+
+	/* not support dp_mst will always return 0, using first dev_entry */
+	if (!codec->dp_mst)
+		return 0;
+
+	/* If Device List Length is 0, the pin is not multi stream capable.
+	 */
+	if ((snd_hda_get_num_devices(codec, nid) + 1) == 0)
+		return 0;
+
+	/* Behavior of setting index being equal to or greater than
+	 * Device List Length is not predictable
+	 */
+	if ((snd_hda_get_num_devices(codec, nid) + 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 deeed35..8362cb6 100644
--- a/sound/pci/hda/hda_codec.h
+++ b/sound/pci/hda/hda_codec.h
@@ -352,8 +352,11 @@ int snd_hda_override_conn_list_mst(struct hda_codec *codec, hda_nid_t nid,
 			   int dev_id, int len, 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

[RFC PATCH 3/4] 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 fe4141c..11be04a 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 4/4] 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 | 167 +++++++++++++++++++++++++++++++--------------
 2 files changed, 120 insertions(+), 50 deletions(-)

diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index 1a42c51..7244f87 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -589,6 +589,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 11be04a..26f5efd 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -144,7 +144,9 @@ struct hdmi_spec {
 	struct snd_array cvts; /* struct hdmi_spec_per_cvt */
 	hda_nid_t cvt_nids[4]; /* only for haswell fix */
 
-	int num_pins;
+	int num_pins; /* number of pins (including device entries) */
+	int num_nids; /* number of pin nids*/
+	int dev_num;
 	struct snd_array pins; /* struct hdmi_spec_per_pin */
 	struct hdmi_pcm pcm_rec[16];
 	struct mutex pcm_lock;
@@ -1248,6 +1250,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 +1277,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 +1510,40 @@ 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);
-
-		if (wid_type != AC_WID_PIN)
+	for (pin_idx = 0; pin_idx <= spec->num_pins; pin_idx++) {
+		int dev_id_saved;
+		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 +1560,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 +1581,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 +1609,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 +1685,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 +1763,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 +1844,12 @@ 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 +1930,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 +2024,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 +2042,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 +2111,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 +2120,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 +2288,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 +2310,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 +2335,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 +2589,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 +2612,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 +2784,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,20 +2879,20 @@ 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, conns, ARRAY_SIZE(conns));
+	nconns = snd_hda_get_connections_mst(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, spec->num_cvts, spec->cvt_nids);
+	snd_hda_override_conn_list_mst(codec, nid, dev_id, spec->num_cvts, spec->cvt_nids);
 }
 
 #define INTEL_VENDOR_NID 0x08
@@ -2912,6 +2978,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);
@@ -2925,6 +2992,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
@@ -2945,10 +3014,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 4/4] ALSA - hda: add DP MST support

[Takashi Iwai]

On Mon, 07 Mar 2016 15:57:46 +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 | 167 +++++++++++++++++++++++++++++++--------------
>  2 files changed, 120 insertions(+), 50 deletions(-)
> 
> diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> index 1a42c51..7244f87 100644
> --- a/sound/pci/hda/hda_codec.c
> +++ b/sound/pci/hda/hda_codec.c
> @@ -589,6 +589,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 11be04a..26f5efd 100644
> --- a/sound/pci/hda/patch_hdmi.c
> +++ b/sound/pci/hda/patch_hdmi.c
> @@ -144,7 +144,9 @@ struct hdmi_spec {
>  	struct snd_array cvts; /* struct hdmi_spec_per_cvt */
>  	hda_nid_t cvt_nids[4]; /* only for haswell fix */
>  
> -	int num_pins;
> +	int num_pins; /* number of pins (including device entries) */
> +	int num_nids; /* number of pin nids*/
> +	int dev_num;

These new definitions are unclear to me.  Please give a bit more
detailed comments there.  e.g. what actually means "including device
entries"?  You can give some example there.  I suppose num_nids is the
actual number of pins, right?  Also what is dev_num?  It's not an
index, right? 


thanks,

Takashi

Re: [RFC PATCH 1/4] ALSA: hda - codec add DP MST support for connection list

[Takashi Iwai]

On Mon, 07 Mar 2016 15:57:43 +0100,
libin.yang@linux.intel.com wrote:
> 
> From: Libin Yang <libin.yang@linux.intel.com>
> 
> This patches adds the support of connection list for DP MST.
> With this, hdmi driver in DP MST mode can easily reuse
> the connection list mechanism.
> 
> Signed-off-by: Libin Yang <libin.yang@linux.intel.com>

I would just replace the function with *_mst() and provide the old
ones via static inline wrappers instead of having two copies.


Takashi

> ---
>  sound/pci/hda/hda_codec.c | 134 ++++++++++++++++++++++++++++++++++++++++++----
>  sound/pci/hda/hda_codec.h |   5 ++
>  2 files changed, 128 insertions(+), 11 deletions(-)
> 
> diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> index 8374188..d4c81f7 100644
> --- a/sound/pci/hda/hda_codec.c
> +++ b/sound/pci/hda/hda_codec.c
> @@ -110,23 +110,24 @@ struct hda_conn_list {
>  	struct list_head list;
>  	int len;
>  	hda_nid_t nid;
> +	int dev_id;
>  	hda_nid_t conns[0];
>  };
>  
>  /* look up the cached results */
>  static struct hda_conn_list *
> -lookup_conn_list(struct hda_codec *codec, hda_nid_t nid)
> +lookup_conn_list(struct hda_codec *codec, hda_nid_t nid, int dev_id)
>  {
>  	struct hda_conn_list *p;
>  	list_for_each_entry(p, &codec->conn_list, list) {
> -		if (p->nid == nid)
> +		if ((p->nid == nid) && (p->dev_id == dev_id))
>  			return p;
>  	}
>  	return NULL;
>  }
>  
> -static int add_conn_list(struct hda_codec *codec, hda_nid_t nid, int len,
> -			 const hda_nid_t *list)
> +static int add_conn_list(struct hda_codec *codec, hda_nid_t nid,
> +			 int dev_id, int len, const hda_nid_t *list)
>  {
>  	struct hda_conn_list *p;
>  
> @@ -135,6 +136,7 @@ static int add_conn_list(struct hda_codec *codec, hda_nid_t nid, int len,
>  		return -ENOMEM;
>  	p->len = len;
>  	p->nid = nid;
> +	p->dev_id = dev_id;
>  	memcpy(p->conns, list, len * sizeof(hda_nid_t));
>  	list_add(&p->list, &codec->conn_list);
>  	return 0;
> @@ -150,8 +152,13 @@ static void remove_conn_list(struct hda_codec *codec)
>  	}
>  }
>  
> -/* read the connection and add to the cache */
> -static int read_and_add_raw_conns(struct hda_codec *codec, hda_nid_t nid)
> +/*
> + * read the connection and add to the cache
> + * the caller should select the device entry by sending the
> + * corresponding verb if necessary before calling this function
> + */
> +static int read_and_add_raw_conns(struct hda_codec *codec, hda_nid_t nid,
> +				  int dev_id)
>  {
>  	hda_nid_t list[32];
>  	hda_nid_t *result = list;
> @@ -166,7 +173,8 @@ static int read_and_add_raw_conns(struct hda_codec *codec, hda_nid_t nid)
>  		len = snd_hda_get_raw_connections(codec, nid, result, len);
>  	}
>  	if (len >= 0)
> -		len = snd_hda_override_conn_list(codec, nid, len, result);
> +		len = snd_hda_override_conn_list_mst(codec, nid,
> +					     dev_id, len, result);
>  	if (result != list)
>  		kfree(result);
>  	return len;
> @@ -197,7 +205,7 @@ int snd_hda_get_conn_list(struct hda_codec *codec, hda_nid_t nid,
>  		const struct hda_conn_list *p;
>  
>  		/* if the connection-list is already cached, read it */
> -		p = lookup_conn_list(codec, nid);
> +		p = lookup_conn_list(codec, nid, 0);
>  		if (p) {
>  			if (listp)
>  				*listp = p->conns;
> @@ -206,7 +214,7 @@ int snd_hda_get_conn_list(struct hda_codec *codec, hda_nid_t nid,
>  		if (snd_BUG_ON(added))
>  			return -EINVAL;
>  
> -		err = read_and_add_raw_conns(codec, nid);
> +		err = read_and_add_raw_conns(codec, nid, 0);
>  		if (err < 0)
>  			return err;
>  		added = true;
> @@ -215,6 +223,49 @@ int snd_hda_get_conn_list(struct hda_codec *codec, hda_nid_t nid,
>  EXPORT_SYMBOL_GPL(snd_hda_get_conn_list);
>  
>  /**
> + * snd_hda_get_conn_list_mst - get connection list in mst mode
> + * @codec: the HDA codec
> + * @nid: NID to parse
> + * @dev_id: device entry id
> + * @listp: the pointer to store NID list
> + *
> + * Parses the connection list of the given widget and stores the pointer
> + * to the list of NIDs.
> + *
> + * Returns the number of connections, or a negative error code.
> + *
> + * Note that the returned pointer isn't protected against the list
> + * modification.  If snd_hda_override_conn_list() might be called
> + * concurrently, protect with a mutex appropriately.
> + */
> +int snd_hda_get_conn_list_mst(struct hda_codec *codec, hda_nid_t nid,
> +			      int dev_id, const hda_nid_t **listp)
> +{
> +	bool added = false;
> +
> +	for (;;) {
> +		int err;
> +		const struct hda_conn_list *p;
> +
> +		/* if the connection-list is already cached, read it */
> +		p = lookup_conn_list(codec, nid, dev_id);
> +		if (p) {
> +			if (listp)
> +				*listp = p->conns;
> +			return p->len;
> +		}
> +		if (snd_BUG_ON(added))
> +			return -EINVAL;
> +
> +		err = read_and_add_raw_conns(codec, nid, dev_id);
> +		if (err < 0)
> +			return err;
> +		added = true;
> +	}
> +}
> +EXPORT_SYMBOL_GPL(snd_hda_get_conn_list_mst);
> +
> +/**
>   * snd_hda_get_connections - copy connection list
>   * @codec: the HDA codec
>   * @nid: NID to parse
> @@ -246,6 +297,39 @@ int snd_hda_get_connections(struct hda_codec *codec, hda_nid_t nid,
>  EXPORT_SYMBOL_GPL(snd_hda_get_connections);
>  
>  /**
> + * snd_hda_get_connections_mst - copy connection list in mst mode
> + * @codec: the HDA codec
> + * @nid: NID to parse
> + * @dev_id: device entry id
> + * @conn_list: connection list array; when NULL, checks only the size
> + * @max_conns: max. number of connections to store
> + *
> + * Parses the connection list of the given widget and stores the list
> + * of NIDs.
> + *
> + * Returns the number of connections, or a negative error code.
> + */
> +int snd_hda_get_connections_mst(struct hda_codec *codec, hda_nid_t nid,
> +				int dev_id, hda_nid_t *conn_list,
> +				int max_conns)
> +{
> +	const hda_nid_t *list;
> +	int len = snd_hda_get_conn_list_mst(codec, nid, dev_id, &list);
> +
> +	if (len > 0 && conn_list) {
> +		if (len > max_conns) {
> +			codec_err(codec, "Too many connections %d for NID 0x%x\n",
> +				   len, nid);
> +			return -EINVAL;
> +		}
> +		memcpy(conn_list, list, len * sizeof(hda_nid_t));
> +	}
> +
> +	return len;
> +}
> +EXPORT_SYMBOL_GPL(snd_hda_get_connections_mst);
> +
> +/**
>   * snd_hda_override_conn_list - add/modify the connection-list to cache
>   * @codec: the HDA codec
>   * @nid: NID to parse
> @@ -262,17 +346,45 @@ int snd_hda_override_conn_list(struct hda_codec *codec, hda_nid_t nid, int len,
>  {
>  	struct hda_conn_list *p;
>  
> -	p = lookup_conn_list(codec, nid);
> +	p = lookup_conn_list(codec, nid, 0);
>  	if (p) {
>  		list_del(&p->list);
>  		kfree(p);
>  	}
>  
> -	return add_conn_list(codec, nid, len, list);
> +	return add_conn_list(codec, nid, 0, len, list);
>  }
>  EXPORT_SYMBOL_GPL(snd_hda_override_conn_list);
>  
>  /**
> + * snd_hda_override_conn_list_mst - add/modify the connection-list to cache
> + * @codec: the HDA codec
> + * @nid: NID to parse
> + * @dev_id: device entry id
> + * @len: number of connection list entries
> + * @list: the list of connection entries
> + *
> + * Add or modify the given connection-list to the cache.  If the corresponding
> + * cache already exists, invalidate it and append a new one.
> + *
> + * Returns zero or a negative error code.
> + */
> +int snd_hda_override_conn_list_mst(struct hda_codec *codec, hda_nid_t nid,
> +			   int dev_id, int len, const hda_nid_t *list)
> +{
> +	struct hda_conn_list *p;
> +
> +	p = lookup_conn_list(codec, nid, dev_id);
> +	if (p) {
> +		list_del(&p->list);
> +		kfree(p);
> +	}
> +
> +	return add_conn_list(codec, nid, dev_id, len, list);
> +}
> +EXPORT_SYMBOL_GPL(snd_hda_override_conn_list_mst);
> +
> +/**
>   * snd_hda_get_conn_index - get the connection index of the given NID
>   * @codec: the HDA codec
>   * @mux: NID containing the list
> diff --git a/sound/pci/hda/hda_codec.h b/sound/pci/hda/hda_codec.h
> index 373fcad..deeed35 100644
> --- a/sound/pci/hda/hda_codec.h
> +++ b/sound/pci/hda/hda_codec.h
> @@ -330,6 +330,9 @@ snd_hda_codec_write(struct hda_codec *codec, hda_nid_t nid, int flags,
>  	snd_hdac_get_sub_nodes(&(codec)->core, nid, start_nid)
>  int snd_hda_get_connections(struct hda_codec *codec, hda_nid_t nid,
>  			    hda_nid_t *conn_list, int max_conns);
> +int snd_hda_get_connections_mst(struct hda_codec *codec, hda_nid_t nid,
> +				int dev_id, hda_nid_t *conn_list,
> +				int max_conns);
>  static inline int
>  snd_hda_get_num_conns(struct hda_codec *codec, hda_nid_t nid)
>  {
> @@ -345,6 +348,8 @@ int snd_hda_get_conn_list(struct hda_codec *codec, hda_nid_t nid,
>  			  const hda_nid_t **listp);
>  int snd_hda_override_conn_list(struct hda_codec *codec, hda_nid_t nid, int nums,
>  			  const hda_nid_t *list);
> +int snd_hda_override_conn_list_mst(struct hda_codec *codec, hda_nid_t nid,
> +			   int dev_id, int len, 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);
>  int snd_hda_get_devices(struct hda_codec *codec, hda_nid_t nid,
> -- 
> 1.9.1
> 

Re: [PATCH 2/4] ALSA: hda - add DP mst verb support

[Takashi Iwai]

On Mon, 07 Mar 2016 15:57:44 +0100,
libin.yang@linux.intel.com wrote:
> 
> +int snd_hda_set_dev_select(struct hda_codec *codec, hda_nid_t nid, int dev_id)
> +{
> +	int ret, dev;
> +	unsigned long timeout;
> +
> +	/* not support dp_mst will always return 0, using first dev_entry */
> +	if (!codec->dp_mst)
> +		return 0;
> +
> +	/* If Device List Length is 0, the pin is not multi stream capable.
> +	 */
> +	if ((snd_hda_get_num_devices(codec, nid) + 1) == 0)
> +		return 0;

I don't understand what this code is for.  Why +1?

> +
> +	/* Behavior of setting index being equal to or greater than
> +	 * Device List Length is not predictable
> +	 */
> +	if ((snd_hda_get_num_devices(codec, nid) + 1) <= dev_id)
> +		return 0;

Ditto.  And, calling the same function twice?


thanks,

Takashi

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

[Yang, Libin]

Hi Takashi,

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Monday, March 07, 2016 11:19 PM
> To: libin.yang@linux.intel.com
> Cc: alsa-devel@alsa-project.org; Lin, Mengdong; Yang, Libin
> Subject: Re: [alsa-devel] [RFC PATCH 4/4] ALSA - hda: add DP MST
> support
> 
> On Mon, 07 Mar 2016 15:57:46 +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 | 167
> +++++++++++++++++++++++++++++++--------------
> >  2 files changed, 120 insertions(+), 50 deletions(-)
> >
> > diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> > index 1a42c51..7244f87 100644
> > --- a/sound/pci/hda/hda_codec.c
> > +++ b/sound/pci/hda/hda_codec.c
> > @@ -589,6 +589,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 11be04a..26f5efd 100644
> > --- a/sound/pci/hda/patch_hdmi.c
> > +++ b/sound/pci/hda/patch_hdmi.c
> > @@ -144,7 +144,9 @@ struct hdmi_spec {
> >  	struct snd_array cvts; /* struct hdmi_spec_per_cvt */
> >  	hda_nid_t cvt_nids[4]; /* only for haswell fix */
> >
> > -	int num_pins;
> > +	int num_pins; /* number of pins (including device entries) */
> > +	int num_nids; /* number of pin nids*/
> > +	int dev_num;
> 
> These new definitions are unclear to me.  Please give a bit more
> detailed comments there.  e.g. what actually means "including device
> entries"?  You can give some example there.  I suppose num_nids is the
> actual number of pins, right?  Also what is dev_num?  It's not an
> index, right?

OK. I will add more comments in the code. For this case:
1. num_pins is the virtual pin number. For example, 
    there are 3 pins and each pin has 4 device entry, the num_pins will be 12.
2. num_nids is the actual number of pins. For the above case it will be 3
3. dev_num is the device entry number on each pin. For the above case, it is 4.
    dev_num is used to decide to create how many pcms (as we talked before,
    num_nids + dev_num - 1, on Intel platform, it is 5 pcms)
    For more complex case (not sure there is such case in reality), pin 1 has 2
    device entries, pin 2 and pin 3 has 3 device entries. The dev_num is 3.
    And 5 pcms will be created.

For device entry index, the index is located in per_pin struct.

Regards,
Libin

> 
> 
> thanks,
> 
> Takashi

Re: [RFC PATCH 1/4] ALSA: hda - codec add DP MST support for connection list

[Yang, Libin]

Hi Takashi,

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Tuesday, March 08, 2016 12:41 AM
> To: libin.yang@linux.intel.com
> Cc: alsa-devel@alsa-project.org; Lin, Mengdong; Yang, Libin
> Subject: Re: [alsa-devel] [RFC PATCH 1/4] ALSA: hda - codec add DP MST
> support for connection list
> 
> On Mon, 07 Mar 2016 15:57:43 +0100,
> libin.yang@linux.intel.com wrote:
> >
> > From: Libin Yang <libin.yang@linux.intel.com>
> >
> > This patches adds the support of connection list for DP MST.
> > With this, hdmi driver in DP MST mode can easily reuse
> > the connection list mechanism.
> >
> > Signed-off-by: Libin Yang <libin.yang@linux.intel.com>
> 
> I would just replace the function with *_mst() and provide the old
> ones via static inline wrappers instead of having two copies.

I see. I will change it.

Regards,
Libin

> 
> 
> Takashi
> 
> > ---
> >  sound/pci/hda/hda_codec.c | 134
> ++++++++++++++++++++++++++++++++++++++++++----
> >  sound/pci/hda/hda_codec.h |   5 ++
> >  2 files changed, 128 insertions(+), 11 deletions(-)
> >
> > diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> > index 8374188..d4c81f7 100644
> > --- a/sound/pci/hda/hda_codec.c
> > +++ b/sound/pci/hda/hda_codec.c
> > @@ -110,23 +110,24 @@ struct hda_conn_list {
> >  	struct list_head list;
> >  	int len;
> >  	hda_nid_t nid;
> > +	int dev_id;
> >  	hda_nid_t conns[0];
> >  };
> >
> >  /* look up the cached results */
> >  static struct hda_conn_list *
> > -lookup_conn_list(struct hda_codec *codec, hda_nid_t nid)
> > +lookup_conn_list(struct hda_codec *codec, hda_nid_t nid, int dev_id)
> >  {
> >  	struct hda_conn_list *p;
> >  	list_for_each_entry(p, &codec->conn_list, list) {
> > -		if (p->nid == nid)
> > +		if ((p->nid == nid) && (p->dev_id == dev_id))
> >  			return p;
> >  	}
> >  	return NULL;
> >  }
> >
> > -static int add_conn_list(struct hda_codec *codec, hda_nid_t nid, int
> len,
> > -			 const hda_nid_t *list)
> > +static int add_conn_list(struct hda_codec *codec, hda_nid_t nid,
> > +			 int dev_id, int len, const hda_nid_t *list)
> >  {
> >  	struct hda_conn_list *p;
> >
> > @@ -135,6 +136,7 @@ static int add_conn_list(struct hda_codec
> *codec, hda_nid_t nid, int len,
> >  		return -ENOMEM;
> >  	p->len = len;
> >  	p->nid = nid;
> > +	p->dev_id = dev_id;
> >  	memcpy(p->conns, list, len * sizeof(hda_nid_t));
> >  	list_add(&p->list, &codec->conn_list);
> >  	return 0;
> > @@ -150,8 +152,13 @@ static void remove_conn_list(struct
> hda_codec *codec)
> >  	}
> >  }
> >
> > -/* read the connection and add to the cache */
> > -static int read_and_add_raw_conns(struct hda_codec *codec,
> hda_nid_t nid)
> > +/*
> > + * read the connection and add to the cache
> > + * the caller should select the device entry by sending the
> > + * corresponding verb if necessary before calling this function
> > + */
> > +static int read_and_add_raw_conns(struct hda_codec *codec,
> hda_nid_t nid,
> > +				  int dev_id)
> >  {
> >  	hda_nid_t list[32];
> >  	hda_nid_t *result = list;
> > @@ -166,7 +173,8 @@ static int read_and_add_raw_conns(struct
> hda_codec *codec, hda_nid_t nid)
> >  		len = snd_hda_get_raw_connections(codec, nid, result,
> len);
> >  	}
> >  	if (len >= 0)
> > -		len = snd_hda_override_conn_list(codec, nid, len, result);
> > +		len = snd_hda_override_conn_list_mst(codec, nid,
> > +					     dev_id, len, result);
> >  	if (result != list)
> >  		kfree(result);
> >  	return len;
> > @@ -197,7 +205,7 @@ int snd_hda_get_conn_list(struct hda_codec
> *codec, hda_nid_t nid,
> >  		const struct hda_conn_list *p;
> >
> >  		/* if the connection-list is already cached, read it */
> > -		p = lookup_conn_list(codec, nid);
> > +		p = lookup_conn_list(codec, nid, 0);
> >  		if (p) {
> >  			if (listp)
> >  				*listp = p->conns;
> > @@ -206,7 +214,7 @@ int snd_hda_get_conn_list(struct hda_codec
> *codec, hda_nid_t nid,
> >  		if (snd_BUG_ON(added))
> >  			return -EINVAL;
> >
> > -		err = read_and_add_raw_conns(codec, nid);
> > +		err = read_and_add_raw_conns(codec, nid, 0);
> >  		if (err < 0)
> >  			return err;
> >  		added = true;
> > @@ -215,6 +223,49 @@ int snd_hda_get_conn_list(struct hda_codec
> *codec, hda_nid_t nid,
> >  EXPORT_SYMBOL_GPL(snd_hda_get_conn_list);
> >
> >  /**
> > + * snd_hda_get_conn_list_mst - get connection list in mst mode
> > + * @codec: the HDA codec
> > + * @nid: NID to parse
> > + * @dev_id: device entry id
> > + * @listp: the pointer to store NID list
> > + *
> > + * Parses the connection list of the given widget and stores the pointer
> > + * to the list of NIDs.
> > + *
> > + * Returns the number of connections, or a negative error code.
> > + *
> > + * Note that the returned pointer isn't protected against the list
> > + * modification.  If snd_hda_override_conn_list() might be called
> > + * concurrently, protect with a mutex appropriately.
> > + */
> > +int snd_hda_get_conn_list_mst(struct hda_codec *codec, hda_nid_t
> nid,
> > +			      int dev_id, const hda_nid_t **listp)
> > +{
> > +	bool added = false;
> > +
> > +	for (;;) {
> > +		int err;
> > +		const struct hda_conn_list *p;
> > +
> > +		/* if the connection-list is already cached, read it */
> > +		p = lookup_conn_list(codec, nid, dev_id);
> > +		if (p) {
> > +			if (listp)
> > +				*listp = p->conns;
> > +			return p->len;
> > +		}
> > +		if (snd_BUG_ON(added))
> > +			return -EINVAL;
> > +
> > +		err = read_and_add_raw_conns(codec, nid, dev_id);
> > +		if (err < 0)
> > +			return err;
> > +		added = true;
> > +	}
> > +}
> > +EXPORT_SYMBOL_GPL(snd_hda_get_conn_list_mst);
> > +
> > +/**
> >   * snd_hda_get_connections - copy connection list
> >   * @codec: the HDA codec
> >   * @nid: NID to parse
> > @@ -246,6 +297,39 @@ int snd_hda_get_connections(struct
> hda_codec *codec, hda_nid_t nid,
> >  EXPORT_SYMBOL_GPL(snd_hda_get_connections);
> >
> >  /**
> > + * snd_hda_get_connections_mst - copy connection list in mst mode
> > + * @codec: the HDA codec
> > + * @nid: NID to parse
> > + * @dev_id: device entry id
> > + * @conn_list: connection list array; when NULL, checks only the size
> > + * @max_conns: max. number of connections to store
> > + *
> > + * Parses the connection list of the given widget and stores the list
> > + * of NIDs.
> > + *
> > + * Returns the number of connections, or a negative error code.
> > + */
> > +int snd_hda_get_connections_mst(struct hda_codec *codec,
> hda_nid_t nid,
> > +				int dev_id, hda_nid_t *conn_list,
> > +				int max_conns)
> > +{
> > +	const hda_nid_t *list;
> > +	int len = snd_hda_get_conn_list_mst(codec, nid, dev_id, &list);
> > +
> > +	if (len > 0 && conn_list) {
> > +		if (len > max_conns) {
> > +			codec_err(codec, "Too many connections %d for
> NID 0x%x\n",
> > +				   len, nid);
> > +			return -EINVAL;
> > +		}
> > +		memcpy(conn_list, list, len * sizeof(hda_nid_t));
> > +	}
> > +
> > +	return len;
> > +}
> > +EXPORT_SYMBOL_GPL(snd_hda_get_connections_mst);
> > +
> > +/**
> >   * snd_hda_override_conn_list - add/modify the connection-list to
> cache
> >   * @codec: the HDA codec
> >   * @nid: NID to parse
> > @@ -262,17 +346,45 @@ int snd_hda_override_conn_list(struct
> hda_codec *codec, hda_nid_t nid, int len,
> >  {
> >  	struct hda_conn_list *p;
> >
> > -	p = lookup_conn_list(codec, nid);
> > +	p = lookup_conn_list(codec, nid, 0);
> >  	if (p) {
> >  		list_del(&p->list);
> >  		kfree(p);
> >  	}
> >
> > -	return add_conn_list(codec, nid, len, list);
> > +	return add_conn_list(codec, nid, 0, len, list);
> >  }
> >  EXPORT_SYMBOL_GPL(snd_hda_override_conn_list);
> >
> >  /**
> > + * snd_hda_override_conn_list_mst - add/modify the connection-list
> to cache
> > + * @codec: the HDA codec
> > + * @nid: NID to parse
> > + * @dev_id: device entry id
> > + * @len: number of connection list entries
> > + * @list: the list of connection entries
> > + *
> > + * Add or modify the given connection-list to the cache.  If the
> corresponding
> > + * cache already exists, invalidate it and append a new one.
> > + *
> > + * Returns zero or a negative error code.
> > + */
> > +int snd_hda_override_conn_list_mst(struct hda_codec *codec,
> hda_nid_t nid,
> > +			   int dev_id, int len, const hda_nid_t *list)
> > +{
> > +	struct hda_conn_list *p;
> > +
> > +	p = lookup_conn_list(codec, nid, dev_id);
> > +	if (p) {
> > +		list_del(&p->list);
> > +		kfree(p);
> > +	}
> > +
> > +	return add_conn_list(codec, nid, dev_id, len, list);
> > +}
> > +EXPORT_SYMBOL_GPL(snd_hda_override_conn_list_mst);
> > +
> > +/**
> >   * snd_hda_get_conn_index - get the connection index of the given NID
> >   * @codec: the HDA codec
> >   * @mux: NID containing the list
> > diff --git a/sound/pci/hda/hda_codec.h b/sound/pci/hda/hda_codec.h
> > index 373fcad..deeed35 100644
> > --- a/sound/pci/hda/hda_codec.h
> > +++ b/sound/pci/hda/hda_codec.h
> > @@ -330,6 +330,9 @@ snd_hda_codec_write(struct hda_codec
> *codec, hda_nid_t nid, int flags,
> >  	snd_hdac_get_sub_nodes(&(codec)->core, nid, start_nid)
> >  int snd_hda_get_connections(struct hda_codec *codec, hda_nid_t nid,
> >  			    hda_nid_t *conn_list, int max_conns);
> > +int snd_hda_get_connections_mst(struct hda_codec *codec,
> hda_nid_t nid,
> > +				int dev_id, hda_nid_t *conn_list,
> > +				int max_conns);
> >  static inline int
> >  snd_hda_get_num_conns(struct hda_codec *codec, hda_nid_t nid)
> >  {
> > @@ -345,6 +348,8 @@ int snd_hda_get_conn_list(struct hda_codec
> *codec, hda_nid_t nid,
> >  			  const hda_nid_t **listp);
> >  int snd_hda_override_conn_list(struct hda_codec *codec, hda_nid_t
> nid, int nums,
> >  			  const hda_nid_t *list);
> > +int snd_hda_override_conn_list_mst(struct hda_codec *codec,
> hda_nid_t nid,
> > +			   int dev_id, int len, 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);
> >  int snd_hda_get_devices(struct hda_codec *codec, hda_nid_t nid,
> > --
> > 1.9.1
> >

Re: [PATCH 2/4] ALSA: hda - add DP mst verb support

[Yang, Libin]

Hi Takashi,

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Tuesday, March 08, 2016 12:46 AM
> To: libin.yang@linux.intel.com
> Cc: alsa-devel@alsa-project.org; Lin, Mengdong; Yang, Libin
> Subject: Re: [alsa-devel] [PATCH 2/4] ALSA: hda - add DP mst verb
> support
> 
> On Mon, 07 Mar 2016 15:57:44 +0100,
> libin.yang@linux.intel.com wrote:
> >
> > +int snd_hda_set_dev_select(struct hda_codec *codec, hda_nid_t nid,
> int dev_id)
> > +{
> > +	int ret, dev;
> > +	unsigned long timeout;
> > +
> > +	/* not support dp_mst will always return 0, using first dev_entry
> */
> > +	if (!codec->dp_mst)
> > +		return 0;
> > +
> > +	/* If Device List Length is 0, the pin is not multi stream capable.
> > +	 */
> > +	if ((snd_hda_get_num_devices(codec, nid) + 1) == 0)
> > +		return 0;
> 
> I don't understand what this code is for.  Why +1?

Sorry, it should be if ((snd_hda_get_num_devices(codec, nid)) == 0).
0 means not MST capable. Thanks for point it out. 

> 
> > +
> > +	/* Behavior of setting index being equal to or greater than
> > +	 * Device List Length is not predictable
> > +	 */
> > +	if ((snd_hda_get_num_devices(codec, nid) + 1) <= dev_id)
> > +		return 0;
> 
> Ditto.  And, calling the same function twice?

(snd_hda_get_num_devices(codec, nid) + 1) means the device entry number.
If the (snd_hda_get_num_devices(codec, nid) + 1) == 3, it means there are
only 3 device entries. And dev_id being 0, 1, 2 is legal. It dev_id is 3 or greater,
it is illegal. 

Regards,
Libin

> 
> 
> thanks,
> 
> Takashi

Re: [PATCH 2/4] ALSA: hda - add DP mst verb support

[Takashi Iwai]

On Tue, 08 Mar 2016 07:54:44 +0100,
Yang, Libin wrote:
> 
> Hi Takashi,
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai@suse.de]
> > Sent: Tuesday, March 08, 2016 12:46 AM
> > To: libin.yang@linux.intel.com
> > Cc: alsa-devel@alsa-project.org; Lin, Mengdong; Yang, Libin
> > Subject: Re: [alsa-devel] [PATCH 2/4] ALSA: hda - add DP mst verb
> > support
> > 
> > On Mon, 07 Mar 2016 15:57:44 +0100,
> > libin.yang@linux.intel.com wrote:
> > >
> > > +int snd_hda_set_dev_select(struct hda_codec *codec, hda_nid_t nid,
> > int dev_id)
> > > +{
> > > +	int ret, dev;
> > > +	unsigned long timeout;
> > > +
> > > +	/* not support dp_mst will always return 0, using first dev_entry
> > */
> > > +	if (!codec->dp_mst)
> > > +		return 0;
> > > +
> > > +	/* If Device List Length is 0, the pin is not multi stream capable.
> > > +	 */
> > > +	if ((snd_hda_get_num_devices(codec, nid) + 1) == 0)
> > > +		return 0;
> > 
> > I don't understand what this code is for.  Why +1?
> 
> Sorry, it should be if ((snd_hda_get_num_devices(codec, nid)) == 0).
> 0 means not MST capable. Thanks for point it out. 
> 
> > 
> > > +
> > > +	/* Behavior of setting index being equal to or greater than
> > > +	 * Device List Length is not predictable
> > > +	 */
> > > +	if ((snd_hda_get_num_devices(codec, nid) + 1) <= dev_id)
> > > +		return 0;
> > 
> > Ditto.  And, calling the same function twice?
> 
> (snd_hda_get_num_devices(codec, nid) + 1) means the device entry number.
> If the (snd_hda_get_num_devices(codec, nid) + 1) == 3, it means there are
> only 3 device entries. And dev_id being 0, 1, 2 is legal. It dev_id is 3 or greater,
> it is illegal. 

So, shouldn't it be like below?
    if (dev_id >= num_devices)
        return 0;

Also my point is that you don't need to call snd_hda_get_num_devices()
twice.  It can be stored locally.


Takashi

Re: [PATCH 2/4] ALSA: hda - add DP mst verb support

[Yang, Libin]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Tuesday, March 08, 2016 3:39 PM
> To: Yang, Libin
> Cc: libin.yang@linux.intel.com; alsa-devel@alsa-project.org; Lin,
> Mengdong
> Subject: Re: [alsa-devel] [PATCH 2/4] ALSA: hda - add DP mst verb
> support
> 
> On Tue, 08 Mar 2016 07:54:44 +0100,
> Yang, Libin wrote:
> >
> > Hi Takashi,
> >
> > > -----Original Message-----
> > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > Sent: Tuesday, March 08, 2016 12:46 AM
> > > To: libin.yang@linux.intel.com
> > > Cc: alsa-devel@alsa-project.org; Lin, Mengdong; Yang, Libin
> > > Subject: Re: [alsa-devel] [PATCH 2/4] ALSA: hda - add DP mst verb
> > > support
> > >
> > > On Mon, 07 Mar 2016 15:57:44 +0100,
> > > libin.yang@linux.intel.com wrote:
> > > >
> > > > +int snd_hda_set_dev_select(struct hda_codec *codec, hda_nid_t
> nid,
> > > int dev_id)
> > > > +{
> > > > +	int ret, dev;
> > > > +	unsigned long timeout;
> > > > +
> > > > +	/* not support dp_mst will always return 0, using first dev_entry
> > > */
> > > > +	if (!codec->dp_mst)
> > > > +		return 0;
> > > > +
> > > > +	/* If Device List Length is 0, the pin is not multi stream capable.
> > > > +	 */
> > > > +	if ((snd_hda_get_num_devices(codec, nid) + 1) == 0)
> > > > +		return 0;
> > >
> > > I don't understand what this code is for.  Why +1?
> >
> > Sorry, it should be if ((snd_hda_get_num_devices(codec, nid)) == 0).
> > 0 means not MST capable. Thanks for point it out.
> >
> > >
> > > > +
> > > > +	/* Behavior of setting index being equal to or greater than
> > > > +	 * Device List Length is not predictable
> > > > +	 */
> > > > +	if ((snd_hda_get_num_devices(codec, nid) + 1) <= dev_id)
> > > > +		return 0;
> > >
> > > Ditto.  And, calling the same function twice?
> >
> > (snd_hda_get_num_devices(codec, nid) + 1) means the device entry
> number.
> > If the (snd_hda_get_num_devices(codec, nid) + 1) == 3, it means there
> are
> > only 3 device entries. And dev_id being 0, 1, 2 is legal. It dev_id is 3 or
> greater,
> > it is illegal.
> 
> So, shouldn't it be like below?
>     if (dev_id >= num_devices)
>         return 0;
> 
> Also my point is that you don't need to call snd_hda_get_num_devices()
> twice.  It can be stored locally.

Get it. I will store it locally.

Regards,
Libin

> 
> 
> Takashi